Conversation
em-redhat
left a comment
em-redhat
left a comment
There was a problem hiding this comment.
Code Review: PR #253 — fix: compute AssertionDetectionConfidence from external analyzer mappings
Reviewer: em-redhat (AI-assisted)
CI Status: All checks passed (MegaLinter, Unit+Integration Go 1.24/1.25, E2E Go 1.24/1.25)
Summary
Clean, focused fix for #251. The implementation correctly computes AssertionDetectionConfidence from external analyzer mapping data instead of hardcoding 0. The fix is purely additive — no existing behavior changes when --analyzer is not used.
Architecture: New computeDetectionConfidenceFromMappings function in internal/adapter/contract.go mirrors the Go-native computeDetectionConfidence semantics (recognized/total ratio, integer truncation). Detection confidence stored on ExternalContractCoverageProvider struct and accessed via comma-ok type assertion in buildExternalQualityReports. Summary aggregation uses arithmetic mean with rounding, matching the Go-native BuildPackageSummary pattern.
Tests: 13 new tests (9 table-driven unit, 3 method, 1 integration) with thorough edge-case coverage including nil/empty inputs, integer truncation (1/3=33), and unknown function lookups.
Non-goals verified: No protocol changes (fake analyzer only adds more mapping entries with existing schema), no Go-native path changes, no CLI flag changes, no JSON schema changes.
Findings
MEDIUM: Redundant iteration over mappings in Build
File: internal/adapter/contract.go — Build method
The new code iterates all mappings to find unique targets, then for each unique target calls computeDetectionConfidenceFromMappings which re-scans all mappings — O(n×k) total. buildContractLookup (called immediately before) already groups mappings by function via mappingsByFunc. Computing detection confidence inside buildContractLookup would be O(n). Practical impact is negligible for typical mapping set sizes, but architecturally wasteful.
LOW: Map key collision (theoretical)
File: internal/adapter/contract.go — detectionConfidence map key
Key format pkg + "/" + function could theoretically collide if package paths align with function name boundaries (e.g., pkg="a/b", fn="c" vs pkg="a", fn="b/c"). In practice impossible — Go function names cannot contain /, and external analyzer function names are identifiers by protocol convention. The existing buildContractLookup uses a struct key (funcKey) which is collision-free. Consider aligning for consistency.
LOW: DetectionConfidence cannot distinguish "no data" from "0% confidence"
File: internal/adapter/contract.go — DetectionConfidence method
Returns 0 for both "function not in map" and "all assertions unrecognized." Current callers do not need this distinction, but a (int, bool) return would match the Go convention used by the contract coverage lookup.
LOW: Rounding consistency (intentional)
Per-function confidence uses integer truncation (recognized * 100 / total), summary uses round-half-up (int(float64(totalDetectionConf)/n + 0.5)). This exactly matches the existing Go-native path — computeDetectionConfidence truncates, BuildPackageSummary rounds. Consistent with codebase convention.
Verdict: APPROVE
No CRITICAL or HIGH findings. The MEDIUM finding (redundant iteration) is a minor efficiency concern that does not affect correctness. All LOW findings are informational. The fix is well-scoped, well-tested, and aligned with the stated intent and constitution principles.
ad2a6a4 to
48c547e
Compare
|
🤖 Finished Review · ✅ Success · Started 10:07 AM UTC · Completed 10:20 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.77 |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsMedium
Low
Next steps:
Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsMedium
Low
|
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 12:18 PM UTC · Completed 12:30 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.49 |
8282e6b to
5a50633
Compare
|
🤖 Finished Review · ✅ Success · Started 2:53 PM UTC · Completed 3:13 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.70 |
|
Risk Assessment: moderate (2/5) DetailsRoutine metric bug fix in the adapter layer. File count and line count inflated by learnings, openspec, and docs artifacts that carry no runtime risk. Core changes are 9 source/test files fixing a hardcoded-zero confidence metric. High churn in the adapter area is expected given active external-analyzer feature development. No security, CI, or dependency concerns. Prior score of 2 confirmed. Previous runRisk Assessment: moderate (2/5) DetailsRoutine metric bug fix with well-scoped code changes in adapter/protocol layers (contract.go +75, main.go +34/-5). Large file count and blast radius are dominated by learnings, openspec artifacts, and documentation rather than risky production code spread. No security, CI, or dependency concerns. Prior score of 2 preserved. Previous run (2)Risk Assessment: moderate (2/5) DetailsRoutine metric bug fix with well-scoped code changes in adapter/protocol layers, good test accompaniment, no security or CI concerns; large blast radius is dominated by documentation and learning files rather than risky code spread. Tier 1 signals unchanged from prior assessment, confirming prior score of 2. Previous run (3)Risk Assessment: moderate (2/5) DetailsRoutine metric bug fix with well-scoped code changes in adapter/protocol layers, good test accompaniment, no security or CI concerns; large blast radius is dominated by documentation and learning files rather than risky code spread. Anchored to prior score of 2 — no material signal changes between assessments. Previous run (4)Risk Assessment: moderate (2/5) DetailsRoutine metric bug fix with well-scoped code changes in adapter/protocol layers, good test accompaniment, no security or CI concerns; large blast radius is dominated by documentation files rather than risky code spread. Anchored to prior score of 2 — no material signal changes between assessments. Previous run (5)Risk Assessment: moderate (2/5) DetailsRoutine metric bug fix with well-scoped code changes in adapter/protocol layers, good test accompaniment, no security or CI concerns; large blast radius is dominated by documentation files rather than risky code spread. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
/fs-fix |
1 similar comment
|
/fs-fix |
Update test_mapping description, assertion_type field semantics, error handling consequences, and Building an Analyzer guidance to reflect that test_mapping also enables assertion detection confidence. Addresses PR unbound-force#253 review feedback from @fullsend-ai-review[bot]. Signed-off-by: jflowers <jflowers@users.noreply.github.com> Assisted-by: claude-opus-4-6
- proposal.md: change "per test function" to "per target function" (matches actual implementation which computes per target function) - design.md: change DetectionConfidenceByFunc to detectionConfidence (matches actual field/method naming in implementation) Addresses PR unbound-force#253 review feedback from @fullsend-ai-review[bot]. Signed-off-by: jflowers <jflowers@users.noreply.github.com> Assisted-by: claude-opus-4-6
|
🤖 Review · ❌ Terminated · Started 5:30 PM UTC · Ended 5:50 PM UTC Commit: |
|
Thanks for the thorough review. Addressing each finding: 1. AGENTS.md protected path — The AGENTS.md change is a standard bookkeeping entry documenting the PR in the Recent Changes section. This follows the project's existing pattern and doesn't modify any governance rules, conventions, or behavioral constraints. 2. 3. 4. Building an Analyzer step 3 — Addressed in 8. Optional method error table — Addressed in |
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 5:30 PM UTC · Completed 5:50 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.08 |
Update test_mapping description, assertion_type field semantics, error handling consequences, and Building an Analyzer guidance to reflect that test_mapping also enables assertion detection confidence. Addresses PR unbound-force#253 review feedback from @fullsend-ai-review[bot]. Signed-off-by: jflowers <jflowers@users.noreply.github.com> Assisted-by: claude-opus-4-6
- proposal.md: change "per test function" to "per target function" (matches actual implementation which computes per target function) - design.md: change DetectionConfidenceByFunc to detectionConfidence (matches actual field/method naming in implementation) Addresses PR unbound-force#253 review feedback from @fullsend-ai-review[bot]. Signed-off-by: jflowers <jflowers@users.noreply.github.com> Assisted-by: claude-opus-4-6
db2eb4f to
f146f3a
Compare
|
🤖 Finished Review · ✅ Success · Started 10:07 PM UTC · Completed 10:22 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.81 |
yvonnedevlinrh
left a comment
yvonnedevlinrh
left a comment
There was a problem hiding this comment.
I found three issues that should be addressed before merge:
- The new external-analyzer metric is not semantically equivalent to the native assertion detection confidence: it counts only mapping rows, excluding detected-but-unmapped assertions.
ExternalContractCoverageProvidercan retain stale confidence values after a failed or unsupported subsequentBuild.- Tests do not cover the report-construction path where the original hardcoded-zero bug lived.
The external analyzer path otherwise appears safe: the new type assertion and summary guard are reachable, and no regex or match-pattern reachability issues were introduced.
This review was generated by /uf.review-pr (AI-assisted).
…ings (unbound-force#251) - Add computeDetectionConfidenceFromMappings to internal/adapter/contract.go mirroring quality.computeDetectionConfidence semantics (integer truncation) - Add detectionConfidence map[string]int field and DetectionConfidence method on ExternalContractCoverageProvider with nil-map safety - Populate AssertionDetectionConfidence on each QualityReport and summary in buildExternalQualityReports via comma-ok type assertion - Extend fake analyzer test_mapping from 1 to 3 mappings for integration testing - Add 13 new tests (9 table-driven unit, 3 method, 1 integration) - Add OpenSpec artifacts (proposal, design, spec, tasks) Closes unbound-force#251 Assisted-by: claude-opus Generated with AI assistance (claude-opus)
Update test_mapping description, assertion_type field semantics, error handling consequences, and Building an Analyzer guidance to reflect that test_mapping also enables assertion detection confidence. Addresses PR unbound-force#253 review feedback from @fullsend-ai-review[bot]. Signed-off-by: jflowers <jflowers@users.noreply.github.com> Assisted-by: claude-opus-4-6
- proposal.md: change "per test function" to "per target function" (matches actual implementation which computes per target function) - design.md: change DetectionConfidenceByFunc to detectionConfidence (matches actual field/method naming in implementation) Addresses PR unbound-force#253 review feedback from @fullsend-ai-review[bot]. Signed-off-by: jflowers <jflowers@users.noreply.github.com> Assisted-by: claude-opus-4-6
f146f3a to
6cf9cf1
Compare
|
🤖 Finished Review · ✅ Success · Started 2:57 PM UTC · Completed 3:16 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.95 |
…ntics Rename the ExternalContractCoverageProvider detection-confidence surface to "mapping classification confidence" to reflect that the denominator is emitted mapping rows, not all detected assertion sites. Also clear the cached confidence map at the start of Build to prevent stale values from prior successful builds leaking into degraded build paths. Addresses PR unbound-force#253 review feedback from @yvonnedevlinrh. Signed-off-by: Jay Flowers <jay.flowers@gmail.com> Assisted-by: deepseek-v4-pro
Extend TestQualityWithExternalAnalyzer to assert both per-report and summary assertion_detection_confidence values, exercising the buildExternalQualityReports construction boundary so removing the assignment in main.go would fail the test. Addresses PR unbound-force#253 review feedback from @yvonnedevlinrh. Signed-off-by: Jay Flowers <jay.flowers@gmail.com> Assisted-by: deepseek-v4-pro
Addresses PR unbound-force#253 review-council feedback: AGENTS.md Recent Changes and the OpenSpec tasks.md still referenced the pre-rename identifiers (computeDetectionConfidenceFromMappings, detectionConfidence, DetectionConfidence). Updated to the final computeMappingClassificationConfidence / classificationConfidence / MappingClassificationConfidence names and documented the mapping-row proxy semantics plus the Build reset-to-nil behavior. Signed-off-by: Jay Flowers <jay.flowers@gmail.com> Assisted-by: deepseek-v4-pro
Addresses PR unbound-force#253 review-council feedback: the stale-state nil-reset at the top of Build had no regression coverage. TestBuild_ResetsClassificationConfidence seeds a populated classificationConfidence map, re-runs Build with TestMapping unsupported, and asserts MappingClassificationConfidence returns 0 instead of the stale value. Signed-off-by: Jay Flowers <jay.flowers@gmail.com> Assisted-by: deepseek-v4-pro
|
🤖 Finished Review · ✅ Success · Started 3:59 PM UTC · Completed 4:17 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.10 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
requires-manual-review
…detection-confidence # Conflicts: # AGENTS.md # cmd/gaze/external_analyzer_test.go # cmd/gaze/main.go # docs/protocol.md # internal/adapter/adapter_test.go
|
🤖 Finished Review · ✅ Success · Started 9:24 PM UTC · Completed 9:42 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.28 |
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Retro · ✅ Success · Started 9:57 PM UTC · Completed 10:07 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.60 |
Retro: PR #253 — fix: compute AssertionDetectionConfidence from external analyzer mappingsTimelineThis human-authored PR fixed a bug where
Review qualityReview agent strengths: The agent excelled at documentation accuracy — it caught 4 stale-doc findings in Review agent gaps: The agent identified a surface-level edge case (the literal string Impact: All 3 human findings drove code changes (4 commits). None of the agent's findings drove code changes — they were either informational LOW observations or the repeatedly-flagged AGENTS.md governance gate. CostThe review agent ran 8 times across the PR lifecycle at ~$55 total. Finding quality plateaued after run 1 — subsequent runs produced the same core findings (AGENTS.md protected-path, Existing issues with supporting evidence from this retro
Proposals skipped (target repo not allowed)File manually or update
|
4 participants
Summary
Fixes #251.
buildExternalQualityReportsincmd/gaze/main.gohardcodedAssertionDetectionConfidenceto 0 for all external analyzer quality reports,causing
gaze quality --analyzerto always show "Assertion detection confidence: 0%"even when assertions were correctly detected.
The fix adds gaze-side computation from existing
protocol.AssertionMappingDataentries, mirroring the Go-native
computeDetectionConfidencesemantics (sameinteger truncation, same ratio formula, same 0-when-empty behavior). No protocol
changes required — confidence is derived from the
AssertionTypefield thatexternal analyzers already provide.
How to Test
Run the unit tests for the computation function:
Run the method tests for the provider accessor:
Run the integration test via fake analyzer:
Verify with an external analyzer (if available):
"Assertion detection confidence" should now show accurate percentages instead of 0%.
How to Demo
Run
gaze quality --analyzer <analyzer-binary> <package>and observe that the"Assertion detection confidence" field now reflects the actual proportion of
assertion mappings with a recognized
AssertionType, rather than always showing 0%.Key Files Changed
cmd/gaze/main.go(+36/-0) — Comma-ok type assertion to accessDetectionConfidencefromExternalContractCoverageProvider; populates per-report and summary-levelAssertionDetectionConfidenceinbuildExternalQualityReportsinternal/adapter/contract.go(+59/-0) — NewcomputeDetectionConfidenceFromMappingsfunction,detectionConfidencemap field,DetectionConfidencemethod,Buildintegrationcontract_internal_test.go(+144/-0) — 9 table-driven unit tests + 3 method testsadapter_test.go(+49/-6) — 1 integration test, updated existing test expectationinternal/protocol/client_test.go(+3/-3) — Updated expected mapping count (1→3)testdata/fake_analyzer/main.go(+28/-6) — Extendedtest_mappingresponse from1 to 3 mappings with mixed
assertion_typevaluesopenspec/changes/fix-assertion-detection-confidence/This PR was generated by /uf.finale (AI-assisted).