feat: add per-requirement test attribution to coverage engine

[em-redhat]

Summary

Resolves #203 — the coverage engine previously used aggregate test results,
causing all implemented requirements to be marked implemented_failing when
any single test failed. This change enables per-requirement test attribution
so each requirement gets its own passing/failing/untested status.

Changes

• internal/tester/ — Capture per-test details (name, package, location,
  pass/fail, error) during OPA test execution via new TestDetail struct
• internal/evaluator/ — Propagate per-test details through the evaluator
  abstraction boundary with mirrored TestDetail struct
• internal/coverage/ (NEW) — Map Rego test packages to requirement IDs
  via naming convention (ac_2_1_test → ac-2.1) or optional
  coverage-mapping.yaml override file; classify each requirement independently
• internal/mcp/ — Include perRequirement field in test_policy JSON response
• cmd/complypack/cli/ — Display per-requirement test status in CLI output
• AGENTS.md — Document new internal/coverage/ domain package

Testing

• 26+ subtests in internal/coverage/coverage_test.go covering normalization,
  convention mapping, enrichment, override loading, and orchestration
• 6 subtests in internal/tester/runner_test.go for detail capture
• 2 subtests in internal/evaluator/opa_test.go for propagation
• 3 tests in internal/mcp/tools_test.go for response serialization
• All 16 packages pass with -race -count=1

Backward Compatible

All changes are additive — existing aggregate fields (total, passed,
failed, errors) remain unchanged. The perRequirement field is only
included when per-test details are available.

[yvonnedevlinrh]

Solid implementation that correctly solves the aggregate attribution problem described in #203. Architecture follows the thin-transport-layer pattern, test coverage is thorough (37+ subtests), and CI is fully green. See inline comments for specific findings.

Commit trailer (must fix): Commit a515d98 is missing both required trailers per the constitution's Contribution Workflow section:
Signed-off-by: — required for DCO compliance
Assisted-by: () — required if AI-assisted

[yvonnedevlinrh]

This is the second copy of TestDetail — see identical definition at internal/tester/runner.go:15. The field-by-field conversion in opa.go:58-67 is the only bridge between them. Please see the comment on runner.go for suggested resolution paths.

[em-redhat]

Resolved — local TestDetail struct removed. evaluator/evaluator.go:61 now uses testresult.Detail from the shared internal/testresult/ package.

[yvonnedevlinrh]

os.IsNotExist() instead of errors.Is() — LoadOverrideMapping uses deprecated os.IsNotExist(err) instead of errors.Is(err, os.ErrNotExist). Not a bug with os.ReadFile, but violates post-Go-1.13 idiom.

[em-redhat]

Resolved — coverage.go:486 now uses errors.Is(err, os.ErrNotExist).

[yvonnedevlinrh]

Test assertion style inconsistency — new attribution tests use raw t.Errorf while existing tests in the same file use testify (assert.Equal, require.NoError). File already imports both assert and require.

[em-redhat]

Resolved — all attribution tests now use testify. See coverage_test.go:603 (assert.Equal), :611 (assert.Empty), :630 (require.Len/assert.Equal), etc. No more raw t.Errorf in the attribution test section.

[yvonnedevlinrh]

Regex only matches lowercase alpha families — ^([a-z]+)_(\d+...)$ won't match uppercase (AC_2_1) or hyphenated families (cp-abc_1_2). Fine if the convention is documented, but worth noting for users.

[em-redhat]

Resolved — coverage.go:410-415 now documents the lowercase-only constraint and notes that a coverage-mapping.yaml override file should be used for non-standard naming (uppercase or hyphenated families).

[hbraswelrh]

Review Summary

Solid implementation solving the aggregate attribution problem from #203. Architecture follows thin-transport-layer pattern per AGENTS.md. Test coverage is thorough (26+ subtests).

Concurs with @yvonnedevlinrh's existing review. Two HIGH findings block merge:

1. Duplicated TestDetail struct across package boundary (Constitution Principle I)
2. Missing commit trailers (Signed-off-by, Assisted-by) — constitutional MUST

Additional notes not covered by inline comments:

• enrichWithTestResults at coverage.go:340 still emits stale warning "per-requirement attribution is not yet available" — this PR adds exactly that capability. Update warning text or integrate AttributeTests() into Run().
• Test assertion style mixes raw t.Errorf with file's existing testify style — pick one per file.
• Regex requirementPattern only matches lowercase families — document this constraint if intentional.

This review was generated by /uf.review-pr (AI-assisted).

[yvonnedevlinrh]

PR: #226
Action: Submit review → Approve
Base: main
Head: opsx/per-requirement-test-attribution

Prior Findings Resolution

All findings from previous reviews have been addressed in a4a55c04:

Finding	Status	Evidence
TestDetail duplication	✅ Resolved	Extracted to shared internal/testresult/testresult.go
os.IsNotExist → errors.Is	✅ Resolved	coverage.go:486
Dead append pattern	✅ Resolved	Direct assignment at coverage.go:467
Test assertion style	✅ Resolved	All tests use testify
MCP asymmetric error handling	✅ Resolved	Graceful degradation with documented rationale (tools.go:336-343)
Warning text	✅ Resolved	Updated in gap-reporting engine
Regex documentation	✅ Resolved	coverage.go:410-415 documents convention and limitations
Commit trailers	✅ Resolved	Signed-off-by and Assisted-by present

New Findings (Non-blocking)

LOW: Code duplication in internal/tester/runner.go:81-106

The pkg extraction logic is duplicated between the pass and fail branches (lines 81-84 and 96-99). It could be hoisted above the if/else since both branches are identical:

pkg := ""
if result.Location != nil {
    pkg = pkgByFile[result.Location.File]
}

Not blocking — the current code is readable and functional.

NIT: Discarded error in MCP handler (tools.go:344)

perReq, _ := coverage.AttributeTests("", "", results)

The comment block (lines 336-343) thoroughly documents the rationale. For full CLI/MCP parity, the error could be logged (the CLI logs it as a WARNING at pack.go:241). In practice this is cosmetic since the MCP path passes empty strings for both directory parameters, so LoadOverrideMapping is never reached.

Architecture & Constitution

• ✅ Thin transport layers: CLI (pack.go:238-252) and MCP (tools.go:336-347) are pure wiring over coverage.AttributeTests()
• ✅ CLI/MCP parity maintained
• ✅ Domain logic in internal/coverage/, internal/tester/, internal/evaluator/
• ✅ Shared type in internal/testresult/ eliminates cross-package duplication
• ✅ SPDX headers on all new files
• ✅ Conventional commit with required trailers
• ✅ All constitution principles satisfied (centralized constants, isolation, readability, composability, convention-over-config)

Test Coverage

Comprehensive across all four packages — 26+ subtests in coverage_test.go, 6 in runner_test.go, 2 in opa_test.go, 2 in tools_test.go. Positive, negative, and edge cases covered. All use testify.

Security

No concerns. Override file path is deterministic. MCP handler passes empty paths preventing filesystem interaction from the attribution engine. YAML parsing uses yaml.v3.

LGTM — well-structured implementation that correctly resolves #203.

[sonupreetam]

Non-duplicate findings from review council (3 items, all LOW/non-blocking)

1. AGENTS.md:14 — internal/testresult/ not listed in domain packages

New shared types package consumed by tester, evaluator, and coverage is not in the domain packages inventory. Since AGENTS.md is the authoritative guide for where code belongs, consider adding:

- `internal/testresult/` — shared per-test outcome types consumed by tester, evaluator, and coverage

2. coverage.go:497-499 — Override mapping version error lacks actionable guidance

The error unsupported coverage mapping version "2" (expected "1") tells the user what failed but not what to do. Consider:

unsupported coverage mapping version "2"; this version of complypack supports version "1" — upgrade complypack or use version "1"

3. coverage-mapping.yaml format undocumented in-repo

The override file schema (version, mappings structure) is only discoverable by reading source. No example file or README section exists. Consider adding a coverage-mapping.example.yaml or a brief format description in the README.

---

E2E verified: CLI pack with convention naming, mixed pass/fail attribution, MCP perRequirement field, and override file precedence all work correctly.

[em-redhat]

Thanks for the thorough review @sonupreetam — all 4 findings addressed in 3929052:

1. AGENTS.md:14 — internal/testresult/ not listed — Fixed. Added to domain packages list at AGENTS.md:14:

- `internal/testresult/` — shared per-test outcome types consumed by tester, evaluator, and coverage

2. coverage.go:498 — Error message lacks actionable guidance — Fixed. Now reads:

unsupported coverage mapping version "2"; this version of complypack supports version "1" — upgrade complypack or use version "1"

3. coverage-mapping.yaml format undocumented — Fixed. Added coverage-mapping.example.yaml with annotated examples covering: single-requirement mapping, multi-requirement mapping, non-convention package names, and field descriptions.

4. [MEDIUM] MCP observability gap — empty vs absent perRequirement — Fixed. buildTestResultsResponse (tools.go:199) now includes perRequirement: {} whenever attribution was attempted (non-nil map), even when no tests matched the convention. This lets MCP consumers distinguish three states:

• perRequirement absent → feature not available (older complypack)
• perRequirement: {} → attribution attempted, no tests matched convention
• perRequirement: {"ac-2.1": "passing"} → tests matched and attributed

Tests updated to verify the new semantics at tools_test.go:382-395 and tools_test.go:738-741.