Context
Follow-up from review of #226 (per-requirement test attribution). All findings are non-blocking and were deferred to keep the PR focused per Constitution Principle III.
Findings
1. EnrichWithTestResults / enrichWithTestResults naming collision (MEDIUM)
internal/coverage/coverage.go now contains two functions with near-identical names but completely different signatures and semantics:
| Function |
Line |
Visibility |
Signature |
Behavior |
enrichWithTestResults |
284 |
unexported |
(ctx, report, opts) error |
Runs tests, mutates report entries (gap-reporting engine) |
EnrichWithTestResults |
522 |
exported |
(mapping, results) map[...] |
Pure function, returns new requirement→status map (attribution engine) |
A reader will assume the exported version is a promoted refactor of the unexported one. In reality they serve different subsystems.
Suggested fix: Rename the exported function to ClassifyRequirementTestStatus or MapRequirementsToTestStatus to distinguish it from the gap-reporting function.
2. Regex [\da-z]+ admits _test as a valid sub-segment (LOW)
The requirementPattern regex at coverage.go:416:
^(a-z+)(\d+(?:\da-z+)*)$
After TrimSuffix(segment, "_test"), a pathological package name like data.policy.ac_2_1_test_test becomes ac_2_1_test, which the regex matches as ac-2.1.test. No real requirement ID uses .test as a sub-segment, so this is theoretical.
Suggested fix: Either restrict the character class (e.g., require digits-first in sub-segments) or add a post-match filter rejecting sub-segments that equal "test".
3. Details nil vs empty-slice inconsistency (NIT)
In internal/tester/runner.go:
- Empty files path (line 29): returns
Details: []testresult.Detail{} (non-nil empty slice)
- All-skipped path (line 66):
Details remains nil (zero value from &Results{})
Not a bug — all consumers use len() which handles both — but the inconsistency could surprise future callers checking == nil.
Suggested fix: Initialize Details to []testresult.Detail{} at line 66 as well.
Acceptance criteria
Context
Follow-up from review of #226 (per-requirement test attribution). All findings are non-blocking and were deferred to keep the PR focused per Constitution Principle III.
Findings
1.
EnrichWithTestResults/enrichWithTestResultsnaming collision (MEDIUM)internal/coverage/coverage.gonow contains two functions with near-identical names but completely different signatures and semantics:enrichWithTestResults(ctx, report, opts) errorEnrichWithTestResults(mapping, results) map[...]A reader will assume the exported version is a promoted refactor of the unexported one. In reality they serve different subsystems.
Suggested fix: Rename the exported function to
ClassifyRequirementTestStatusorMapRequirementsToTestStatusto distinguish it from the gap-reporting function.2. Regex
[\da-z]+admits_testas a valid sub-segment (LOW)The
requirementPatternregex atcoverage.go:416:^(a-z+)(\d+(?:\da-z+)*)$
After
TrimSuffix(segment, "_test"), a pathological package name likedata.policy.ac_2_1_test_testbecomesac_2_1_test, which the regex matches asac-2.1.test. No real requirement ID uses.testas a sub-segment, so this is theoretical.Suggested fix: Either restrict the character class (e.g., require digits-first in sub-segments) or add a post-match filter rejecting sub-segments that equal
"test".3.
Detailsnil vs empty-slice inconsistency (NIT)In
internal/tester/runner.go:Details: []testresult.Detail{}(non-nil empty slice)Detailsremainsnil(zero value from&Results{})Not a bug — all consumers use
len()which handles both — but the inconsistency could surprise future callers checking== nil.Suggested fix: Initialize
Detailsto[]testresult.Detail{}at line 66 as well.Acceptance criteria
EnrichWithTestResultsrenamed to eliminate collision with unexportedenrichWithTestResults