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.

[yvonnedevlinrh]

Summary

This PR correctly implements per-requirement test attribution end-to-end across testresult, tester, evaluator, coverage, CLI, and MCP layers, with thorough test coverage (427 new test lines) and clean architecture. All prior review findings have been addressed. Two documentation accuracy issues in the new coverage-mapping.example.yaml require correction before merge.

Findings

[MEDIUM] Example YAML keys use incorrect format — overrides will silently fail

File: coverage-mapping.example.yaml:28-38

The example mapping keys use bare names (my_custom_rbac_tests, shared_access_control_tests, network_segmentation_suite), but EnrichWithTestResults matches against d.Package, which OPA's runner populates as full data.-prefixed paths (e.g., data.policy.my_custom_rbac_tests). A user following the example verbatim will get zero attribution matches with no error or warning.

The test suite correctly uses data.policy.custom_check as keys (coverage_test.go:790), contradicting the example.

Fix: Update all three example keys to use full OPA package paths:

mappings:
  # Map a non-standard test package to a requirement
  data.policy.my_custom_rbac_tests:
    - ac-3.1

  # Map one test package to multiple requirements
  data.policy.shared_access_control_tests:
    - ac-2.1
    - ac-3.2

  # Map a test package whose name does not follow the convention
  data.policy.network_segmentation_suite:
    - sc-7
    - sc-7.5

Also update the format comment at line 21 from to (e.g., data.policy.).

MEDIUM Non-existent --coverage-mapping CLI flag documented

File: coverage-mapping.example.yaml:15-16

Lines 15-16 instruct users to pass --coverage-mapping ./coverage-mapping.yaml to complypack pack. This flag does not exist — pack.go:239 hardcodes "" as mappingPath, relying entirely on auto-discovery of coverage-mapping.yaml in contentDir (coverage.go:606-607). Users who follow the example will receive an "unknown flag" error.

Fix: Replace lines 14-16 with documentation of the actual auto-discovery mechanism:

Place this file in your content directory (alongside your policies and tests).

It will be discovered automatically during complypack pack.

[em-redhat]

Thanks @yvonnedevlinrh — both findings fixed in c4e4f7e:

[MEDIUM] Example YAML keys use incorrect format — Updated all three mapping keys to use full OPA package paths (data.policy.my_custom_rbac_tests, etc.) and changed the format comment from <test-package-name> to <opa-package-path> (e.g., data.policy.<name>). This matches how d.Package is populated by the OPA runner and aligns with the test suite (coverage_test.go:790).

[MEDIUM] Non-existent --coverage-mapping CLI flag — Replaced lines 14-16 with auto-discovery documentation: "Place this file in your content directory (alongside your policies and tests). It will be discovered automatically during complypack pack." This correctly reflects the implementation at coverage.go:606-607.

[yvonnedevlinrh]

LGTM

[hbraswelrh]

LGTM.