Skip to content

Three follow-ups from the unsupported-language review: an untested load-bearing skip, GradeNA ordinality, and a duplicated counter #45

Description

@iksnae

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions