What happened
The review agent's highest-severity finding on PR #1817 was that attestations without a valid predicate.timestamp are silently excluded from policy evaluation, creating a fail-open path (review workflow 32283843026). The human reviewer (st3penta) approved conditionally, noting this concern was valid unless all Konflux tests produce attestations in the expected format. The author confirmed that all test attestations produce the required fields and that a separate story exists to require certain test attestations. The agent's finding was theoretically valid but not a practical concern in this codebase — a distinction the agent could not make because AGENTS.md does not document the input contract for test attestations.
What could go better
The review agent rated the fail-open concern at high severity because it had no way to know that Konflux test attestation predicates always include configuration[0].name and a valid timestamp. This is domain-specific knowledge the author holds but that is not recorded anywhere in the repo's agent guidance. With this knowledge documented, the review agent could still flag the missing-timestamp edge case but calibrate its severity appropriately — acknowledging the theoretical risk while noting that the input contract makes it unlikely in practice. I am moderately confident in this proposal: the author's statement is clear evidence of the invariant, but I have not independently verified it against the Konflux attestation schema. Existing issue #1772 proposes documenting the dual test-result architecture, but its scope focuses on file-level change patterns (which packages need updating together) rather than on input contract guarantees that inform review calibration.
Proposed change
Add a subsection to AGENTS.md under the existing architecture or common change patterns section documenting the test attestation input contract. Specifically:
- All Konflux test-result attestations include
predicate.configuration[0].name (the test name) and predicate.timestamp (RFC 3339 with nanosecond precision).
- The
test_attestation.rego package relies on these fields for deduplication (grouping by name, selecting the latest timestamp). Attestations without a valid timestamp are excluded from evaluation by design.
- A separate enforcement story exists to require certain test attestations, providing a safety net if attestation format changes.
This belongs in AGENTS.md (not just code comments) because it informs review severity calibration — the review agent should treat missing-field edge cases in test_attestation.rego as low-severity theoretical concerns rather than high-severity fail-open paths.
Validation criteria
On the next review agent run on a PR modifying policy/release/test_attestation/test_attestation.rego, the agent should not rate missing-timestamp or missing-name edge cases at high severity if the code handles the expected input format correctly. If the agent references the AGENTS.md guidance when calibrating severity, the documentation is working as intended. Measure over the next 3 PRs touching this package.
Generated by retro agent from #1817
What happened
The review agent's highest-severity finding on PR #1817 was that attestations without a valid
predicate.timestampare silently excluded from policy evaluation, creating a fail-open path (review workflow 32283843026). The human reviewer (st3penta) approved conditionally, noting this concern was valid unless all Konflux tests produce attestations in the expected format. The author confirmed that all test attestations produce the required fields and that a separate story exists to require certain test attestations. The agent's finding was theoretically valid but not a practical concern in this codebase — a distinction the agent could not make because AGENTS.md does not document the input contract for test attestations.What could go better
The review agent rated the fail-open concern at high severity because it had no way to know that Konflux test attestation predicates always include
configuration[0].nameand a validtimestamp. This is domain-specific knowledge the author holds but that is not recorded anywhere in the repo's agent guidance. With this knowledge documented, the review agent could still flag the missing-timestamp edge case but calibrate its severity appropriately — acknowledging the theoretical risk while noting that the input contract makes it unlikely in practice. I am moderately confident in this proposal: the author's statement is clear evidence of the invariant, but I have not independently verified it against the Konflux attestation schema. Existing issue #1772 proposes documenting the dual test-result architecture, but its scope focuses on file-level change patterns (which packages need updating together) rather than on input contract guarantees that inform review calibration.Proposed change
Add a subsection to AGENTS.md under the existing architecture or common change patterns section documenting the test attestation input contract. Specifically:
predicate.configuration[0].name(the test name) andpredicate.timestamp(RFC 3339 with nanosecond precision).test_attestation.regopackage relies on these fields for deduplication (grouping by name, selecting the latest timestamp). Attestations without a valid timestamp are excluded from evaluation by design.This belongs in AGENTS.md (not just code comments) because it informs review severity calibration — the review agent should treat missing-field edge cases in
test_attestation.regoas low-severity theoretical concerns rather than high-severity fail-open paths.Validation criteria
On the next review agent run on a PR modifying
policy/release/test_attestation/test_attestation.rego, the agent should not rate missing-timestamp or missing-name edge cases at high severity if the code handles the expected input format correctly. If the agent references the AGENTS.md guidance when calibrating severity, the documentation is working as intended. Measure over the next 3 PRs touching this package.Generated by retro agent from #1817