Skip to content

Skip test_data_found when test-result attestations exist - #1805

Merged
robnester-rh merged 5 commits into
conforma:mainfrom
robnester-rh:EC-1952
Aug 13, 2026
Merged

Skip test_data_found when test-result attestations exist#1805
robnester-rh merged 5 commits into
conforma:mainfrom
robnester-rh:EC-1952

Conversation

@robnester-rh

Copy link
Copy Markdown
Contributor

Summary

When verified test-result attestations are present via OCI referrers, test_data_found no longer requires TEST_OUTPUT in the build pipeline provenance. This supports the migration of tests to their own pipeline where results are delivered as attestations rather than task results.

  • Modified test_data_found deny rule — added condition: count(intoto.verified_statements_by_predicate(intoto.predicate_test_result)) == 0. When verified test-result attestations exist, enforcement is delegated to the test_attestation package (same collections).

  • Updated METADATA — title, description, and solution now mention test-result attestations as an alternative to TEST_OUTPUT.

  • Added 4 test cases:

    • Verified attestations present, no TEST_OUTPUT → no denial
    • No attestations, no TEST_OUTPUT → denial fires (backward compat)
    • Attestations present but unverified → denial fires (security property)
    • Both TEST_OUTPUT and attestations present → no denial

Test plan

  • make test — 1056/1056 pass, 100% coverage
  • make lint — 0 violations
  • make fmt — clean
  • make generate-docs — Antora docs regenerated

Resolves: EC-1952

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 263d690e-d79f-424c-81c1-5b89da6d8dcc

📥 Commits

Reviewing files that changed from the base of the PR and between 894ac11 and c4926a4.

📒 Files selected for processing (1)
  • policy/release/test/test_test.rego
🚧 Files skipped from review as they are similar to previous changes (1)
  • policy/release/test/test_test.rego

📝 Walkthrough

Walkthrough

The release test policy accepts verified OCI test-result attestations as test data. Tests cover verification outcomes and combined data sources. Policy collections and documentation include the updated test rules.

Changes

Release test attestation support

Layer / File(s) Summary
Attestation-aware test data rule
policy/release/test/test.rego
test_data_found recognizes verified test-result attestations through OCI referrers. It denies only when both supported test-data sources are absent.
Attestation fixtures and behavior tests
policy/release/test/test_test.rego
Fixtures and tests cover verified, missing, unverified, and combined test-result sources.
Policy metadata and documentation
policy/release/test_attestation/test_attestation.rego, antora/docs/modules/ROOT/pages/release_policy.adoc, antora/docs/modules/ROOT/pages/packages/*, antora/docs/modules/ROOT/partials/release_policy_nav.adoc
Policy collections include test-attestation rules. Four attestation rules include the redhat_security collection. Documentation updates labels, descriptions, navigation text, and source references.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Mergeability Score: ⚪ Minimal · up to c4926

The change allows verified test-result attestations to satisfy test-data enforcement while preserving denial when attestations or TEST_OUTPUT are absent or unverified; no actionable merge-blocking risk remains after normal checks.

Sequence Diagram(s)

sequenceDiagram
  participant ReleasePolicy
  participant TEST_OUTPUT
  participant OCIReferrers
  participant IntotoVerification
  ReleasePolicy->>TEST_OUTPUT: Check pipeline test results
  ReleasePolicy->>OCIReferrers: Find test-result attestations
  OCIReferrers->>IntotoVerification: Verify matching attestation
  IntotoVerification-->>ReleasePolicy: Return verification result
  ReleasePolicy-->>ReleasePolicy: Deny when both sources are absent
Loading

Suggested reviewers: joejstuart, st3penta

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly states that test_data_found skips enforcement when test-result attestations exist.
Description check ✅ Passed The description explains the change, its context, related ticket, and validation results, although it uses different section headings than the template.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 12, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 12:46 PM UTC · Ended 12:51 PM UTC

Commit: 87c4a29 · View workflow run →

@qodo-for-conforma

Copy link
Copy Markdown

PR Summary by Qodo

Skip test_data_found when verified test-result attestations are present

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Allow test data satisfaction via verified test-result attestations attached as OCI referrers.
• Keep backward-compatible denial when neither TEST_OUTPUT nor verified attestations exist.
• Update docs and add regression tests covering verified/unverified/mixed scenarios.
Diagram

graph TD
  A["Policy input"] --> B["intoto verified statements"] --> C{"Any test-result attestation?"}
  C -->|"Yes"| D["test_attestation pkg"] --> E["Policy result"]
  C -->|"No"| F["test_data_found rule"] --> G{"TEST_OUTPUT present?"} -->|"Yes"| E
  G -->|"No"| H["Deny: No test data"] --> E
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Fold attestation presence into lib.results_from_tests
  • ➕ Single source of truth for “test data exists” checks; fewer rules need special-casing
  • ➕ Could simplify future policy rules that only care about existence/count
  • ➖ Risks changing semantics for other rules that assume results_from_tests only reflects pipeline task results
  • ➖ Harder to keep clear separation between provenance-derived results and attestation-derived results
2. Add an explicit feature flag for attestation-based satisfaction
  • ➕ Safer rollout; teams can opt-in per collection/tenant
  • ➕ Reduces surprise behavior change for existing adopters
  • ➖ Adds configuration surface area and migration complexity
  • ➖ Still needs eventual cleanup once migration completes
3. Deprecate/disable test_data_found and rely solely on test_attestation
  • ➕ Clear ownership: test result enforcement lives in one package
  • ➕ Avoids duplicated enforcement paths long-term
  • ➖ Breaks backward compatibility for pipelines that only emit TEST_OUTPUT
  • ➖ Requires a deprecation window and communication/versioning strategy

Recommendation: The chosen approach (skip test_data_found only when verified test-result attestations exist, and delegate enforcement to test_attestation) is the best near-term balance: it preserves backward compatibility, maintains the security property by requiring verification, and avoids double-enforcement during migration. Consider a later refactor to centralize “test data exists” logic (or a deprecation plan) once the migration is complete.

Files changed (5) +217 / -21

Enhancement (1) +14 / -5
test.regoSkip test_data_found denial when verified test-result attestations exist +14/-5

Skip test_data_found denial when verified test-result attestations exist

• Imports the intoto library and updates rule metadata to describe attestations as an alternative test-data source. Modifies the deny rule to only fire when there are no test results and there are zero verified test-result attestations, delegating attestation enforcement to the test_attestation package.

policy/release/test/test.rego

Tests (1) +187 / -0
test_test.regoAdd coverage for verified/unverified test-result attestation scenarios +187/-0

Add coverage for verified/unverified test-result attestation scenarios

• Adds mock infrastructure for OCI referrers, blob retrieval, and attestation verification, plus four new tests covering: verified attestations without TEST_OUTPUT (no deny), no attestations (deny), unverified attestations (deny), and both sources present (no deny).

policy/release/test/test_test.rego

Documentation (3) +16 / -16
release_test.adocDocument test_data_found accepting verified test-result attestations +13/-13

Document test_data_found accepting verified test-result attestations

• Renames the rule section to “Test data found” and updates the description/solution to allow verified test-result attestations via OCI referrers as an alternative to TEST_OUTPUT. Adjusts source links to match updated line offsets in the policy file.

antora/docs/modules/ROOT/pages/packages/release_test.adoc

release_policy.adocUpdate release policy rule list label for test_data_found +2/-2

Update release policy rule list label for test_data_found

• Updates references in the rule list from “Test data found in task results” to “Test data found” to match the updated rule semantics and documentation.

antora/docs/modules/ROOT/pages/release_policy.adoc

release_policy_nav.adocRename nav entry for test_data_found +1/-1

Rename nav entry for test_data_found

• Updates the navigation entry label to “Test data found” for consistency with the revised rule documentation.

antora/docs/modules/ROOT/partials/release_policy_nav.adoc

@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

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/release/test/test.rego 100.00% <100.00%> (ø)
policy/release/test/test_test.rego 100.00% <100.00%> (ø)
...icy/release/test_attestation/test_attestation.rego 100.00% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 12, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:52 PM UTC · Completed 1:05 PM UTC

Commit: 87c4a29 · View workflow run →

@qodo-for-conforma

qodo-for-conforma Bot commented Aug 12, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Attestation bypass without digest ✗ Dismissed 🐞 Bug ⛨ Security
Description
test.test_data_found now skips its denial whenever any verified test-result statement exists, but
the bypass does not require the statement subject to match the evaluated image. If input.image.ref
has no digest (tag-only), the test_attestation subject-mismatch guard is also skipped, so an
unrelated verified test-result statement can satisfy the “test data found” requirement even when the
build pipeline has no TEST_OUTPUT for this image.
Code

policy/release/test/test.rego[R92-95]

+	# When verified test-result attestations exist, enforcement is handled
+	# by the test_attestation package (same collections). Skip this denial
+	# to avoid requiring both TEST_OUTPUT and attestations simultaneously.
+	count(intoto.verified_statements_by_predicate(intoto.predicate_test_result)) == 0
Relevance

●●● Strong

Team has accepted fixes for digest/tag edge cases; subject-mismatch bypass is security-critical.

PR-#1407

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new test.test_data_found logic skips the denial when the count of verified test-result
statements is non-zero, but the intoto verification filter only checks predicateType and does not
enforce subject matching. The only subject-matching enforcement in test_attestation is gated on
the evaluated image having a non-empty digest, and image.parse explicitly produces an empty digest
when the input ref has no @digest part; this combination means tag-based refs can satisfy
test_data_found with attestations that are not bound to the evaluated image digest.

policy/release/test/test.rego[87-98]
policy/lib/intoto/trust.rego[51-63]
policy/release/test_attestation/test_attestation.rego[289-300]
policy/lib/image/image.rego[7-41]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`test.test_data_found` skips denial when *any* verified test-result attestation exists, but it does not ensure the attestation is for the evaluated image.

Because `test_attestation` only enforces subject matching when `input.image.ref` includes a digest, a tag-only `input.image.ref` can allow unrelated verified test-result statements to satisfy `test_data_found`.

## Issue Context
- `lib.intoto.verified_statements_by_predicate` filters only on `predicateType`, not on statement subject.
- `test_attestation`’s subject-mismatch deny rule is gated on `img_digest != ""`.

## Fix Focus Areas
- policy/release/test/test.rego[87-98]
- policy/lib/intoto/trust.rego[51-63]
- policy/release/test_attestation/test_attestation.rego[289-300]
- policy/lib/image/image.rego[7-41]

## Proposed fix
Update the `test.test_data_found` denial-bypass condition to require a verified test-result statement whose subject digest matches the evaluated image digest.

One safe approach:
1. Compute `img := image.parse(input.image.ref)`.
2. Only allow the attestation-based bypass when `img.digest != ""` *and* there exists a `statement` in `intoto.verified_statements_by_predicate(intoto.predicate_test_result)` such that `img.digest` is present in `intoto.subject_digests(subject)` for some `subject` in `statement.subject`.
3. If `img.digest == ""`, keep the legacy behavior (require TEST_OUTPUT) unless you can resolve the digest via an internal mechanism and then enforce the subject match.

Add a regression test covering a tag-only `input.image.ref` with a verified test-result statement whose subject digest does NOT match (it should still deny under `test.test_data_found` after the fix).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Manual SLSA attestation JSON ✗ Dismissed 📜 Skill insight ⚙ Maintainability
Description
The new tests manually construct SLSA provenance/attestation JSON
(_slsa_v1_task/_slsa_v1_provenance) instead of using tekton_test helpers. This violates the
requirement to use tekton_test helpers for SLSA attestations in tests.
Code

policy/release/test/test_test.rego[R797-800]

+_slsa_v1_task := {
+	"name": "pipelineTask",
+	"content": base64.encode(json.marshal({
+		"metadata": {"labels": {
Relevance

●● Moderate

No clear precedent on enforcing tekton_test SLSA helpers vs manual JSON; could be seen as acceptable
test scaffolding.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 3884 requires using tekton_test helpers rather than manually constructing SLSA
attestation/provenance JSON. The added test infrastructure defines _slsa_v1_task and
_slsa_v1_provenance as hand-built JSON objects for attestation/provenance handling.

policy/release/test/test_test.rego[797-841]
Skill: write-tests

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tests are manually building SLSA provenance/attestation JSON structures instead of using `tekton_test` helpers.

## Issue Context
Compliance requires tests involving SLSA attestations to use `tekton_test.slsav1_task`, `tekton_test.with_results`, and `tekton_test.slsav1_attestation` helpers rather than hand-rolled JSON structures.

## Fix Focus Areas
- policy/release/test/test_test.rego[797-841]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
✅ Compliance rules (platform): 41 rules
✅ Skills: 4 invoked
  add-policy-rule
  write-tests
  lint-and-conventions
  pr-checklist

Grey Divider

Tip of the day
💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread policy/release/test/test_test.rego
Comment thread policy/release/test/test.rego
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [enforcement gap analysis] policy/release/test/test.rego:95 — When test_data_found is bypassed (verified test-result attestations exist), test_all_images (verifying image digest in IMAGES_PROCESSED) has no direct counterpart in test_attestation. The subject_mismatch rule checks attestation subjects against the image digest but uses a different verification model (in-toto subject binding vs. task result). This is a design difference inherent to the attestation approach rather than an enforcement gap.

  • [scope] policy/release/test_attestation/test_attestation.rego — The PR adds redhat_security collection membership to four test_attestation deny rules (no_erred_test_attestations, no_skipped_test_attestations, subject_mismatch, rule_data_provided). These additions are architecturally necessary for enforcement parity when test_data_found defers to attestation-based checks, but are not mentioned in the PR description.

  • [architectural coherence] policy/release/test/test.rego:95 — The test_data_found rule now has an implicit coupling with the test_attestation package. When verified attestations exist, it silently defers enforcement. This cross-package dependency is documented via an inline code comment but not formalized in METADATA annotations.

  • [code duplication] policy/release/test/test_test.rego:773 — The new mock infrastructure block (~90 lines) duplicates helpers from test_attestation_test.rego (_image_ref, _statement_digest, _parse_digest, _slsa_v1_provenance, _mock_verify_success, etc.). No established pattern for cross-package test helper sharing exists in this codebase, so the duplication is pragmatic but worth noting for future consolidation.

  • [naming consistency] policy/release/test/test_test.rego:783 — The _referrer constant is defined as a bare object literal, while test_attestation_test.rego defines it as a parameterized helper _referrer(digest, artifact_type). The simpler form is adequate for the current single-referrer use case.

Previous run

Review

Findings

Low

  • [authorization/bypass logic] policy/release/test/test.rego:89 — The new bypass condition in test_data_found relies on verified_statements_by_predicate which requires Sigstore-verified provenance (fail-closed). Downstream rules declaring depends_on: test.test_data_found can still fire independently since depends_on is advisory metadata, not a runtime gate. This is inherent to the framework design and not a defect.

  • [mock-infrastructure inconsistency] policy/release/test/test_test.rego:779 — The _trusted_task_rules constant defines the value directly as the inner object, while test_attestation_test.rego wraps it in an outer key and accesses it via .trusted_task_rules. Both produce the identical value at the point of use — minor stylistic inconsistency with no functional impact.

  • [mock-infrastructure duplication] policy/release/test/test_test.rego:772 — Approximately 100 lines of mock infrastructure are near-verbatim duplicates of the same definitions in test_attestation_test.rego. This is a known trade-off in Rego test organization where test helpers are package-scoped.

  • [naming convention] policy/release/test/test_test.rego:870 — The new test names (test_data_found_skipped_with_test_result_attestations, etc.) are longer than the existing convention (test_needs_non_empty_data, test_success_data). The longer names are more descriptive but diverge from the file's established style.

Previous run (2)

Looks good to me


Labels: PR extends policy enforcement rule with new attestation-based compliance path

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge enhancement New feature or request labels Aug 12, 2026
Acepresso
Acepresso previously approved these changes Aug 13, 2026
Comment thread policy/release/test/test.rego
robnester-rh added a commit to robnester-rh/policy that referenced this pull request Aug 13, 2026
…(EC-1952)

When test.test_data_found skips its denial because verified test-result
attestations exist, enforcement shifts to the test_attestation package.
That compensating coverage was missing from @redhat_security: the rules
were only in @redhat. Add redhat_security to no_erred_test_attestations,
no_skipped_test_attestations, subject_mismatch, and rule_data_provided so
@redhat_security consumers retain enforcement when the skip path is taken.

Addresses review feedback from st3penta on PR conforma#1805.

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size: XL and removed size: L labels Aug 13, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 13, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 1:41 PM UTC · Ended 1:49 PM UTC

Commit: 87c4a29 · View workflow run →

robnester-rh and others added 4 commits August 13, 2026 09:47
When verified test-result attestations are present via OCI referrers,
test_data_found no longer requires TEST_OUTPUT in the build pipeline
provenance. This supports the migration of tests to their own pipeline
where results are delivered as attestations rather than task results.

Resolves: EC-1952

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…(EC-1952)

When test.test_data_found skips its denial because verified test-result
attestations exist, enforcement shifts to the test_attestation package.
That compensating coverage was missing from @redhat_security: the rules
were only in @redhat. Add redhat_security to no_erred_test_attestations,
no_skipped_test_attestations, subject_mismatch, and rule_data_provided so
@redhat_security consumers retain enforcement when the skip path is taken.

Addresses review feedback from st3penta on PR conforma#1805.

Co-Authored-By: Claude <noreply@anthropic.com>
EC-2031 removed data.trusted_task_rules in favor of
data.rule_data.trusted_task_rules. Two test_data_found skip-path tests
still mocked the removed location, so after rebasing onto main the
trusted-task lookup found nothing, attestation verification returned no
verified statements, and test_data_found denied instead of skipping.
Point the mocks at data.rule_data.trusted_task_rules, matching the
rename main applied to lib/intoto/trust_test.rego.

Co-Authored-By: Claude <noreply@anthropic.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 13, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:50 PM UTC · Completed 2:04 PM UTC

Commit: 87c4a29 · View workflow run →

@robnester-rh
robnester-rh requested a review from st3penta August 13, 2026 13:55
fullsend-ai-review[bot]

This comment was marked as outdated.

- Wrap _trusted_task_rules in the trusted_task_rules key and dot-index at
  the use sites, matching the shape in test_attestation_test.rego.
- Shorten the four new test_data_found test names to the file's
  convention while keeping the denies/skips outcome explicit.

Cosmetic only; no behavior change. 1070/1070 tests pass.

Co-Authored-By: Claude <noreply@anthropic.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 13, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:24 PM UTC · Completed 2:43 PM UTC

Commit: 87c4a29 · View workflow run →

@st3penta st3penta left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread policy/release/test/test.rego
Comment thread policy/release/test/test.rego
Comment thread policy/release/test/test_test.rego
Comment thread policy/release/test/test_test.rego
@robnester-rh
robnester-rh merged commit 57634e6 into conforma:main Aug 13, 2026
25 of 27 checks passed
@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 13, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 3:03 PM UTC · Completed 3:15 PM UTC

Commit: 87c4a29 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #1805 — Skip test_data_found when test-result attestations exist

Timeline

  1. Aug 12, 12:45 UTC — PR opened by robnester-rh (branch EC-1952). Adds a bypass condition to test.test_data_found: when verified test-result attestations exist via OCI referrers, skip the denial and delegate enforcement to the test_attestation package. 7 files changed, +228/−24.
  2. Aug 12, 12:50–13:05 — First fullsend-ai-review (run 31598428096, $5.81, 12 min, 5 sub-agents + challenger). Approved with zero inline comments. Challenger sub-agent read test_attestation.rego but focused on dependency semantics, not collection membership.
  3. Aug 12, 12:53 — qodo-for-conforma reviewed: raised two findings (manual mock JSON, attestation bypass without digest check). Author correctly explained both were inapplicable.
  4. Aug 13, 12:24Human reviewer st3penta found the critical bug: the inline comment (same collections) was factually incorrect for @redhat_security. Four rules in the test_attestation package (no_erred_test_attestations, no_skipped_test_attestations, subject_mismatch, rule_data_provided) lacked redhat_security collection membership, creating an enforcement gap for @redhat_security consumers.
  5. Aug 13, 13:40–13:48 — Author pushed fix (commit 89ded27) and rebased onto main.
  6. Aug 13, 14:04 — Second fullsend-ai-review (run 31706850329, $7.33). Approved with 4 low-severity inline comments (mock shape inconsistency, code duplication, naming convention, enforcement gap analysis).
  7. Aug 13, 14:23 — Author addressed review nits (commit c4926a4: mock shape + shorter test names).
  8. Aug 13, 14:43 — Third fullsend-ai-review (run 31709939640, $7.47). Approved. st3penta approved with "LGTM".
  9. Aug 13, 15:02 — PR merged.

Review quality assessment

The human reviewer caught the only correctness bug — a collection-membership gap that would have silently dropped enforcement for @redhat_security consumers. The agent found 2 actionable cosmetic issues (mock shape, naming) and 6 informational observations across 3 runs ($20.61 total), but never identified the collection-parity problem despite its challenger sub-agent reading the relevant file.

The bug was mechanically detectable: the diff's inline comment (same collections) is a verifiable factual claim, and checking it required comparing collections: annotations between test.test_data_found (which belongs to redhat + redhat_security) and the four test_attestation rules (which only belonged to redhat). The challenger sub-agent had the file open but lacked instructions to perform this specific comparison.

Evidence for existing issues

  • Add cross-file consistency review guidance to AGENTS.md #1801 (cross-file consistency review guidance): This retro provides additional evidence. The collection-parity bug is a cross-file consistency issue — the agent verified patterns within the PR's own files but did not verify consistency with the unchanged destination package.
  • Document dual test-result architecture and trust chain in AGENTS.md #1772 (document dual test-result architecture): This retro demonstrates the cost of not documenting the test/test_attestation dual-path architecture. Had the agent known about the feature-parity requirement between these packages, it might have checked collection membership more carefully.

Proposals filed

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

Labels

enhancement New feature or request ready-for-merge All reviewers approved — ready to merge size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants