Three findings from the review of the unsupported-language correction (dd8c6239b). None blocked that change; all are real and were measured.
1. The old-side Unsupported skip in ComputeTrustDelta is load-bearing and untested
internal/github/pr.go:100-102. Removing it, measured on an unsupported→supported transition with matching UnitIDs:
skip PRESENT: NewlyCertified=1 ScoreImproved=0 AvgDelta=0.0000
skip REMOVED: NewlyCertified=0 ScoreImproved=1 AvgDelta=0.9000
It drops a real certification and fabricates a +0.90 average delta. The code is correct; nothing pins it. The trigger is the engine gaining a language tier — a foreseeable event, not a hypothetical, since internal/language_tiers exists precisely to be extended.
Worth recording how this was nearly cleared as dead code. The first probe showed identical output with and without the skip, which would have made the line removable. That probe was broken — it built UnitID from language and passed swift on one side and go on the other, so the map lookup missed either way and both arms returned the same answer. An all-negative probe and a genuinely inert line are indistinguishable without a control.
Fix: add a ComputeTrustDelta case for an unsupported→supported transition with matching UnitIDs, and confirm it reddens when the skip is removed.
2. GradeNA = -1 sorts better than GradeA under ordinal comparison
internal/domain/dimension.go:116. GradeA..GradeF are 0..6; GradeNA is -1. Any grade < x or grade > x treats an unassessed unit as better than an A.
No production hit today — every aggregation goes through domain.GradeFromScore(score) (score-based) or GradeDistribution maps keyed by Grade.String(). But the exact comparison shape already exists in this repo, in tests:
internal/engine/pipeline_test.go:37 — if record.Grade > domain.GradeC
internal/engine/pipeline_unsupported_test.go:82 — if record.Grade > domain.GradeB
Neither is in a parseGrade path, so no corrupt record is being promoted. The relevance is that a future production > is plausible rather than hypothetical — and parseGrade now routes every unrecognised grade string to GradeNA, so the population carrying that value is larger than it was.
Fix: an explicit Grade.IsAssessed() predicate, or at minimum a doc-comment on the constant warning that it is not ordinally comparable.
3. runParams has two sources of truth for one number
cmd/certify/certify_cmd.go:634-635 carries both tally runTally and processed int, and the call site at :145 passes processed: tally.processed. Cosmetic — but the runTally refactor existed specifically to remove this class of duplication (four positional int returns threaded through three call sites, which is what made the unsupported miscount invisible in the first place). Leaving one behind undercuts the reason for the change.
Fix: drop processed from runParams and read it from the tally.
Not in this list, deliberately
Report-side aggregate exclusion (detailed.go, full.go, card.go) is tracked on #32. Predicate consolidation is #43. The enforcing exit-code contract is #44. The three-state Status refactor remains unfiled pending the #44 decision, since the two interact.
Three findings from the review of the unsupported-language correction (
dd8c6239b). None blocked that change; all are real and were measured.1. The old-side
Unsupportedskip inComputeTrustDeltais load-bearing and untestedinternal/github/pr.go:100-102. Removing it, measured on an unsupported→supported transition with matchingUnitIDs:It drops a real certification and fabricates a +0.90 average delta. The code is correct; nothing pins it. The trigger is the engine gaining a language tier — a foreseeable event, not a hypothetical, since
internal/language_tiersexists precisely to be extended.Worth recording how this was nearly cleared as dead code. The first probe showed identical output with and without the skip, which would have made the line removable. That probe was broken — it built
UnitIDfrom language and passedswifton one side andgoon the other, so the map lookup missed either way and both arms returned the same answer. An all-negative probe and a genuinely inert line are indistinguishable without a control.Fix: add a
ComputeTrustDeltacase for an unsupported→supported transition with matchingUnitIDs, and confirm it reddens when the skip is removed.2.
GradeNA = -1sorts better thanGradeAunder ordinal comparisoninternal/domain/dimension.go:116.GradeA..GradeFare0..6;GradeNAis-1. Anygrade < xorgrade > xtreats an unassessed unit as better than an A.No production hit today — every aggregation goes through
domain.GradeFromScore(score)(score-based) orGradeDistributionmaps keyed byGrade.String(). But the exact comparison shape already exists in this repo, in tests:internal/engine/pipeline_test.go:37—if record.Grade > domain.GradeCinternal/engine/pipeline_unsupported_test.go:82—if record.Grade > domain.GradeBNeither is in a
parseGradepath, so no corrupt record is being promoted. The relevance is that a future production>is plausible rather than hypothetical — andparseGradenow routes every unrecognised grade string toGradeNA, so the population carrying that value is larger than it was.Fix: an explicit
Grade.IsAssessed()predicate, or at minimum a doc-comment on the constant warning that it is not ordinally comparable.3.
runParamshas two sources of truth for one numbercmd/certify/certify_cmd.go:634-635carries bothtally runTallyandprocessed int, and the call site at:145passesprocessed: tally.processed. Cosmetic — but therunTallyrefactor existed specifically to remove this class of duplication (four positionalintreturns threaded through three call sites, which is what made the unsupported miscount invisible in the first place). Leaving one behind undercuts the reason for the change.Fix: drop
processedfromrunParamsand read it from the tally.Not in this list, deliberately
Report-side aggregate exclusion (
detailed.go,full.go,card.go) is tracked on #32. Predicate consolidation is #43. The enforcing exit-code contract is #44. The three-stateStatusrefactor remains unfiled pending the #44 decision, since the two interact.