feat(EC-1950): bring test_attestation to feature parity with test package

[robnester-rh]

Summary

Brings the test_attestation package to feature parity with the test package so teams adopting OCI-referrer test result attestations get the same policy rigor as those using embedded TEST_OUTPUT task results.

• AC-1: Configurable result values via rule data (predicate spec vocabulary: PASSED/FAILED/WARNED/ERROR/SKIPPED)
• AC-2: no_erred_test_attestations deny rule for ERROR results
• AC-3: no_skipped_test_attestations deny rule for SKIPPED results
• AC-4: Informative test support — failed informative tests warn instead of deny
• AC-5: Subject digest validation — deny if attestation subject doesn't match the image being evaluated (ANY-match semantics)
• AC-6: Rule data schema validation for all 6 new keys
• AC-7: 9 new tests covering all ACs

Predicate alignment

Updated to match the actual attest-test-result step action output which uses integer count fields (failures, warnings, successes) rather than string arrays (failedTests, warnedTests). The _has_result helper checks both predicate.result (string match) and count fields (predicate[key] > 0), following test.rego's _did_result pattern.

Test plan

• 996/996 tests pass (make quiet-test)
• Regal lint clean
• make conventions-check (needs CI)
• Review predicate format against step action with @joejstuart

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: cb283d7e-2082-423b-aad1-57a8990e33ef

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag	Coverage Δ
unit-tests	100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
policy/lib/intoto/intoto.rego	100.00% <100.00%> (ø)
policy/lib/intoto/intoto_test.rego	100.00% <100.00%> (ø)
policy/lib/intoto/trust.rego	100.00% <100.00%> (ø)
policy/lib/rule_data/rule_data.rego	100.00% <ø> (ø)
...uild_scripted_build/slsa_build_scripted_build.rego	100.00% <100.00%> (ø)
...icy/release/test_attestation/test_attestation.rego	100.00% <100.00%> (ø)
...elease/test_attestation/test_attestation_test.rego	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 2:03 PM UTC · Ended 2:09 PM UTC
Commit: 47d3320 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 2:12 PM UTC · Ended 2:20 PM UTC
Commit: 47d3320 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 2:23 PM UTC · Ended 2:30 PM UTC
Commit: 47d3320 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:32 PM UTC · Completed 2:51 PM UTC
Commit: 47d3320 · View workflow run →

[fullsend-ai-review]

Review — ✅ Approve

PR: feat(EC-1950): bring test_attestation to feature parity with test package
Status: Draft

Summary

This PR brings the test_attestation package to feature parity with the test package by adding new deny/warn rules for erred, skipped, and informative test results, configurable result vocabularies via rule data, subject digest validation, and comprehensive rule data schema validation. It also refactors subject_digest from private helpers in trust.rego and slsa_build_scripted_build.rego into a shared public helper in intoto.rego.

Dimensions Reviewed

Correctness ✅

The new rules faithfully replicate the patterns from test.rego:

• _has_result dual-trigger semantics correctly implement OR logic: fires on either predicate.result string match or count field > 0, matching test.rego's _did_result pattern. The "n/a" count_key for erred/skipped rules is a clean sentinel (since the predicate spec defines no count fields for those states), consistent with test.rego.
• _subject_matches correctly navigates the in-toto statement structure — verified_statements_by_predicate returns parsed in-toto statements where .subject is the top-level subject array. The object.get(statement, "subject", []) fallback handles missing subjects safely (tested by test_missing_subject_triggers_mismatch).
• Subject digest comparison is correct: image.parse(input.image.ref).digest returns "sha256:abc123" and intoto.subject_digests({"digest": {"sha256": "abc123"}}) returns {"sha256:abc123"}, so the in check works. The img_digest != "" guard correctly skips the check when the image ref has no digest.
• Rule data defaults correctly use the in-toto test-result predicate vocabulary (PASSED/FAILED/WARNED/ERROR/SKIPPED) rather than the test.rego vocabulary (SUCCESS/FAILURE/WARNING), and informative_test_attestations correctly defaults to [] via rule_data.get's fallback (no tests are informative by default).
• intoto.subject_digest multi-valued semantics are correct OPA: some digest in subject_digests(subject) produces all digest bindings, and the caller's equality check tries each. This is identical to the old _subject_digest behavior.
• Test coverage is thorough: 9+ new test cases cover all acceptance criteria, including edge cases (missing subject, count-based trigger without result string match, custom rule data overrides, informative vs non-informative distinction).

Security ✅

• The subject_mismatch rule is a security-positive addition — it prevents attestation reuse attacks where a test attestation produced for image A is applied to image B. The ANY-match semantics (deny if no subject digest matches) are correct for this use case.
• Rule data schema validation (rule_data_provided) prevents misconfigured rule data from silently disabling policy checks — invalid values like "INVALID_VALUE" are caught and reported.
• No new data exposure, privilege escalation, or injection vectors introduced. The PR body and commit messages contain no instruction-injection patterns.

Intent & Coherence ✅

The change aligns with the stated goal (EC-1950 feature parity) and is appropriately scoped:

• New rules mirror the test.rego structure (erred, skipped, informative, rule data validation, subject validation).
• The predicate format change from string arrays to integer counts aligns with the actual attest-test-result step action output.
• The subject_digest refactoring into intoto.rego is a clean deduplication — the same logic was duplicated in trust.rego and slsa_build_scripted_build.rego.
• All new rules correctly declare collections: - redhat (and - policy_data for rule_data_provided).

Style & Conventions ✅

• All rules have complete METADATA annotations with title, description, short_name, failure_msg, solution, collections, and depends_on — matching the project's conventions-check requirements.
• Import ordering is correct (alphabetical within the data.lib group).
• Helper naming follows the project's _private_helper convention.
• Error message format (failures: %s, warnings: %s) is consistent within the package.

Documentation ✅

• release_test_attestation.adoc updated with all 5 new rule sections and updated descriptions for existing rules.
• release_policy.adoc updated with new rule cross-references in the correct alphabetical position.
• release_policy_nav.adoc updated with nav entries for all new rules.
• release_slsa_build_scripted_build.adoc line numbers updated to account for the new import.
• All source line references in docs match the new file layout.

Cross-repo Contracts ✅

• The predicate format change (string arrays → integer counts) is a deliberate alignment with the upstream attest-test-result step action. Consumers of the old format (failedTests, warnedTests arrays) will see degraded detail in messages (counts instead of names) but rules still trigger correctly via predicate.result string matching.
• The subject_digest / subject_digests functions are now public API from lib.intoto. No external consumers are known to import these from trust.rego or slsa_build_scripted_build.rego directly (they were private helpers).

Findings

Low — Message detail regression for legacy predicates

Attribute	Value
Severity	Low
Category	Correctness
File	policy/release/test_attestation/test_attestation.rego
Line	~54

The switch from _test_list (which extracted individual test names from failedTests/warnedTests arrays) to _count_detail (which reports integer counts from failures/warnings fields) means that predicates using the old string-array format will produce messages like "failures: 0" instead of "failed tests CVE-2024-1234, CVE-2024-5678". While the deny/warn rules still trigger correctly (via _has_result string match on predicate.result), operators lose test-level detail in violation messages. This is intentional per the PR description (alignment with actual step action output) and the _has_result dual-trigger pattern handles backward compatibility for rule evaluation, but teams debugging failures from old-format attestations will see less actionable messages. No remediation needed if old-format attestations are being phased out; consider documenting the format transition in release notes.

Previous run

Review — approve

PR: feat(EC-1950): bring test_attestation to feature parity with test package
Status: Draft PR — reviewing code on its merits

Overview

This PR brings the test_attestation package to feature parity with the test package, adding configurable result values, new deny rules (ERROR/SKIPPED), informative test support, subject digest validation, and rule data schema validation. The implementation closely follows the patterns established in test.rego and is well-structured.

What was reviewed:

• test_attestation.rego — all new/modified rules, helpers (_has_result, _count_detail, _subject_matches)
• test_attestation_test.rego — 9+ new test cases covering all acceptance criteria
• intoto.rego / trust.rego — subject_digest/subject_digests extraction and refactoring
• slsa_build_scripted_build.rego — refactored to use shared intoto.subject_digest
• rule_data.rego — new defaults with correct vocabulary (PASSED/FAILED/WARNED vs SUCCESS/FAILURE/WARNING)
• Antora documentation updates for all new/modified rules
• Collection membership via METADATA annotations (collections: - redhat)

Strengths

1. Consistent patterns: The _has_result dual-path helper mirrors test.rego's _did_result faithfully — string-match OR count-field, same OR semantics, same rule_data-driven configurability.

2. Clean refactoring: Extracting subject_digest/subject_digests from trust.rego's private _subject_digest into public intoto.rego functions eliminates duplication across trust.rego, slsa_build_scripted_build.rego, and the new test_attestation.rego. Tests cover both single and multi-algorithm digest cases.

3. Defensive coding: _subject_matches uses object.get(statement, "subject", []) instead of direct access, correctly handling statements without a subject field (tested via test_missing_subject_triggers_mismatch).

4. Comprehensive tests: All acceptance criteria (AC-1 through AC-7) have dedicated tests. The test_informative_test_warns_instead_of_denies test correctly verifies both that deny is empty AND that warn fires with the right code. The test_count_triggers_deny test validates the count-path independently of the result-string path.

5. Rule data validation: Follows the exact pattern from test.rego — JSON Schema validation for all 6 new keys, with appropriate statuses and strings_array schemas.

Findings

All findings are low severity — none block merge.

1. [low] Contradictory messaging in edge cases

File: policy/release/test_attestation/test_attestation.rego (lines 46-51, 82-91, 141-151)

When _has_result triggers via the string-match path (e.g., result: "FAILED") but the count field is 0 or absent, the message reads: "has a failed result, failures: 0". Conversely, when _has_result triggers via the count path (e.g., result: "PASSED" but failures: 5), the message reads "has a failed result, failures: 5" even though predicate.result is actually "PASSED".

Both cases are functionally correct (the policy catches both signals), but the fixed message template "has a failed result" may confuse operators when the result string disagrees. The old code showed "(none listed)" for the zero-count case.

Suggestion: Consider a conditional message or noting the result field value, e.g., "has failures: 5 (result: PASSED)". This is a UX polish item, not a correctness issue — the test.rego package has the same behavior.

2. [low] "n/a" sentinel for unused count_key parameter

File: policy/release/test_attestation/test_attestation.rego (lines 227, 256)

The no_erred_test_attestations and no_skipped_test_attestations rules pass "n/a" as the count_key to _has_result. This works because object.get(predicate, "n/a", 0) returns 0 and the count path never fires. The intent is that these result types have no integer count field in the attest-test-result step action output.

While functionally safe, "n/a" is a non-obvious sentinel. A brief inline comment explaining the choice (or an alternative like introducing a single-path _has_result_string_only helper) would improve readability. In test.rego, all calls to _did_result pass real count field names.

3. [low] subject_digest non-determinism with multi-algorithm subjects

File: policy/lib/intoto/intoto.rego (lines 58-60)

subject_digest(subject) picks an arbitrary element from subject_digests(subject) via some digest in .... The docstring correctly notes "Safe when subjects have one digest algorithm", which is the common case. If the in-toto ecosystem ever produces multi-algorithm digest sets, _subject_matches in test_attestation.rego uses subject_digests (the set version) and would still be correct — it's only slsa_build_scripted_build.rego's collect_subjects that calls subject_digest (singular) and could pick an unexpected algorithm.

No action needed now, but worth noting for future maintainers.

Verdict

Approve. The code is well-structured, follows existing conventions, has comprehensive test coverage, and the documentation is up to date. The three low-severity findings above are polish items that don't affect correctness or security. The subject-mismatch validation (AC-5) is a meaningful security improvement that prevents attestation substitution.

The PR is currently marked as draft — the open test plan items (make conventions-check and predicate format review with @joejstuart) should be resolved before merge.

Previous run (2)

Review

Findings

Medium

• [logic-error] policy/lib/intoto/intoto.rego:53 — The subject_digest function uses := (complete rule assignment) with some algorithm, value in subject.digest. If a subject contains multiple digest algorithms (e.g., both sha256 and sha512), OPA will produce a runtime conflict error because a complete rule can only yield a single value per input. This is pre-existing behavior (identical to the former _subject_digest in trust.rego and slsa_build_scripted_build.rego), but the new subject_mismatch deny rule adds another consumer, making this a good time to address it.
  Remediation: Consider changing subject_digest to a partial rule that returns a set (subject_digests(subject) contains digest if { ... }), or document the single-algorithm-per-subject invariant and add a guard.

Low

• [edge-case] policy/release/test_attestation/test_attestation.rego — The _has_result function has two overloads: one matching on the result string field, the other on a numeric count key > 0. For no_erred_test_attestations and no_skipped_test_attestations, the count_key is "n/a" (disabling count matching), while for no_failed_tests and no_test_warnings, count matching is active. This means an attestation with result: "PASSED" but failures: 5 triggers deny. This is tested (test_count_triggers_deny) and appears intentional (fail-closed), but the rule metadata description could be more precise about the dual-trigger semantics.

• [missing-test] policy/release/test_attestation/test_attestation_test.rego — No tests verify the no_skipped_test_attestations or no_erred_test_attestations rules when a test is in the informative list. Unlike no_failed_tests which explicitly excludes informative tests via the not _test_name(statement) in {...} guard, these deny rules do not exclude informative tests. An informative test that errors or is skipped will still produce a deny violation. Tests should verify this asymmetry is intentional.

• [edge-case] policy/release/test_attestation/test_attestation.rego — The subject_mismatch rule calls image.parse(input.image.ref) and accesses .digest. If input.image.ref has no digest (e.g., registry.io/repo/image:tag), img_digest will be an empty string, causing a subject_mismatch violation for every test attestation since no subject digest can equal "". While conforma typically operates with digest-pinned refs, adding a guard img_digest != "" would be defensive.

• [fail-open] policy/release/test_attestation/test_attestation.rego — All result classification values (failed_test_attestation_results, etc.) are configurable via rule_data. Setting any to [] disables the corresponding deny rule. The schema validation checks valid enum members and uniqueItems but does not enforce minItems. This mirrors the existing pattern in test.rego and requires privileged access to rule_data to exploit, but adding minItems: 1 would prevent accidental policy disabling.

Previous run (3)

Review

Findings

Medium

• [breaking-api] policy/release/test_attestation/test_attestation.rego — The failure_msg for no_failed_tests and no_test_warnings changed format: "failed tests CVE-2024-1234, CVE-2024-5678" → "failures: 2" and "warned tests deprecated-api-v1" → "warnings: 1". Downstream systems that parse these human-readable violation messages will see different output. Consumers matching only on the violation code field are unaffected.
  Remediation: Document the message format change in release notes.

• [breaking-schema] policy/release/test_attestation/test_attestation.rego — The policy no longer reads failedTests/warnedTests string arrays from the attestation predicate. It now reads numeric failures/warnings count fields. The PR description acknowledges this aligns with actual attest-test-result step action output. Attestation producers still using the old schema will see degraded detail in messages ("failures: 0") but the result-string matching path still catches FAILED/WARNED results, so enforcement is preserved. See also: [scope-creep] finding below.
  Remediation: Coordinate with attestation producers to confirm migration to numeric count fields.

• [breaking-api] policy/release/test_attestation/test_attestation.rego — ERROR and SKIPPED result values previously triggered test_result_known (unsupported result). Now they are accepted as valid and caught by dedicated deny rules (no_erred_test_attestations, no_skipped_test_attestations) with different violation codes. The net enforcement effect is the same (deny), but consumers filtering on specific violation code strings will need updating.
  Remediation: Document the violation code changes in release notes.

• [breaking-api] policy/release/test_attestation/test_attestation.rego — The new subject_mismatch deny rule is net-new enforcement that will reject test attestations whose subject digest does not match the image being evaluated. This could cause previously-passing evaluations to fail if attestation subjects were not precisely matching.

• [breaking-api] policy/release/slsa_build_scripted_build/slsa_build_scripted_build.rego — Public function subject_digest removed from slsa_build_scripted_build and replaced by intoto.subject_digest. Any external consumer importing data.slsa_build_scripted_build.subject_digest will break.
  Remediation: Check if any external consumers reference the old path. If so, add a compatibility alias or coordinate the migration.

• [scope-creep] policy/release/test_attestation/test_attestation.rego — The predicate data model change from string arrays (failedTests, warnedTests) to numeric counts (failures, warnings) alters existing rule semantics. The PR description explicitly acknowledges this alignment with actual step action output. See also: [breaking-schema] finding above.

Low

• [logic-error] policy/release/test_attestation/test_attestation.rego:208 — no_erred_test_attestations and no_skipped_test_attestations pass "n/a" as count_key to _has_result. The count-based clause (object.get(predicate, "n/a", 0)) is dead code since no realistic predicate has a key named "n/a". The rules work correctly via the result-string clause, but a clearer approach would be a dedicated single-clause helper or an empty-string sentinel.

• [missing-test] policy/release/test_attestation/test_attestation_test.rego — No test for a statement with no subject field (malformed attestation), which would trigger subject_mismatch deny via the empty-list fallback in _subject_matches. A direct test would document this edge case behavior.

• [missing-effective-on] policy/release/test_attestation/test_attestation.rego — test.rego uses effective_on dates on some rules to phase them in. The new deny rules (no_erred_test_attestations, no_skipped_test_attestations, subject_mismatch) enforce immediately. Consider whether phased rollout is appropriate.

• [documentation-comment] policy/lib/intoto/intoto.rego — New exported subject_digest function has no documentation comment. Other exported functions in this file have at least a one-line comment.

---

Labels: PR adds new policy rules and configurable features (enhancement) with corresponding Antora documentation updates (documentation).

[robnester-rh]

Fullsend review findings — responses

Medium

• [breaking-api] Message format change — Intentional. Aligned with the actual attest-test-result step action output which produces integer counts (failures, warnings), not string arrays (failedTests, warnedTests). Will document in release notes.

• [breaking-schema] Predicate data model change — Intentional. The previous schema (failedTests/warnedTests arrays) came from a PoC spec example, not from the actual step action. Enforcement is preserved via dual-path _has_result (checks both predicate.result string and count fields).

• [breaking-api] ERROR/SKIPPED violation codes — Intentional per AC-2/AC-3. Operators now get specific short_names to waive (no_erred_test_attestations, no_skipped_test_attestations) instead of the generic test_result_known.

• [breaking-api] subject_mismatch is net-new enforcement — Intentional per AC-5. Closes a security gap where a valid-but-mismatched attestation (produced for image A but attached to image B) would pass. Will discuss phased rollout via effective_on with @joejstuart.

• [breaking-api] subject_digest moved — Moved from slsa_build_scripted_build to intoto.rego to avoid reimplementing digest format conversion. The slsa_build_scripted_build package was updated to use the new location in the same commit. No known external consumers of the old path.

• [scope-creep] — Acknowledged. The predicate model change is part of aligning with the real step action output, not incidental scope expansion.

Low — addressed in d414a97

• [documentation-comment] — Added doc comment on intoto.subject_digest. Fixed.

• [missing-test] — Added test_missing_subject_triggers_mismatch for the no-subject edge case. Fixed.

Low — acknowledged, no change

• [logic-error] "n/a" as count_key — Follows test.rego's _did_result pattern where ERROR and SKIPPED have no corresponding count field. The "n/a" sentinel makes object.get(predicate, "n/a", 0) return 0, so the count clause is a no-op. This is intentional — it keeps the helper signature uniform across all callers.

• [missing-effective-on] — Good point. Will discuss with @joejstuart whether phased rollout via effective_on is appropriate for the new deny rules.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:35 PM UTC · Completed 4:46 PM UTC
Commit: 47d3320 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:28 PM UTC · Completed 10:37 PM UTC
Commit: 7c8ccca · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 10:44 PM UTC · Ended 10:47 PM UTC
Commit: 7c8ccca · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 10:48 PM UTC · Ended 10:51 PM UTC
Commit: 7c8ccca · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 10:52 PM UTC · Ended 10:55 PM UTC
Commit: 7c8ccca · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:56 PM UTC · Completed 11:03 PM UTC
Commit: 7c8ccca · View workflow run →