docs(EC-1954): add effective_on review checklist to AGENTS.md

[cuipinghuo]

Summary

• Add a "Review Checklist for New Policy Rules" section to AGENTS.md
• Ensures AI review agents flag new deny/warn rules missing effective_on dates, which would otherwise enforce immediately on deployment
• Also includes checklist items for collection membership and test coverage

Ref: EC-1954
Upstream: #1760

Test plan

• Verify the next PRs adding new deny/warn rules get flagged by review agents if effective_on is missing

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e233b397-b6d2-4dcd-93d9-e3404437af77

📥 Commits

Reviewing files that changed from the base of the PR and between 892f5bd and 6a9036a.

📒 Files selected for processing (1)

• AGENTS.md

🚧 Files skipped from review as they are similar to previous changes (1)

• AGENTS.md

---

📝 Walkthrough

Walkthrough

Updated AGENTS.md with collection membership instructions and review requirements for new policy rules.

Changes

Policy rule review guidance

Layer / File(s)	Summary
Rule review requirements AGENTS.md	Documents the collections: METADATA key and requires future RFC 3339 effective_on dates, appropriate collection membership, and 100% _test.rego coverage for new deny and warn rules.

Estimated code review effort: 1 (Trivial) | ~2 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the documentation change and the specific effective_on review checklist.
Description check	✅ Passed	The description explains the change, its purpose, related tickets, and test plan, although it does not use the template headings.
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

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[qodo-for-conforma]

PR Summary by Qodo

Add new-policy review checklist (effective_on, collections, tests) to AGENTS.md

📝 Documentation 🕐 Less than 5 minutes

AI Description

• Add a review checklist for new policy rules to prevent immediate enforcement.
• Require effective_on for new deny/warn rules to ensure a migration window.
• Remind reviewers to verify collection membership and mandatory test coverage.

Diagram

graph TD
A([PR adds new policy rule]) --> B["AGENTS.md checklist"] --> C{Reviewer/AI agent}
C --> D["Verify effective_on"] --> E([Safe rollout window])
C --> F["Verify collections + tests"] --> G([Rule evaluated + covered])

subgraph Legend
direction LR
_db[(Database)] ~~~ _doc["Doc/Checklist"] ~~~ _dec{Decision} ~~~ _act([Action/Artifact])
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Automated lint/CI guard for missing `effective_on`

• ➕ Prevents merges regardless of reviewer attentiveness
• ➕ Scales better as policy rule volume grows
• ➕ Provides consistent, actionable failures
• ➖ Requires implementing/maintaining a rule that correctly detects new deny/warn rules
• ➖ May need exceptions/backward-compatibility handling for legacy rules

2. Introduce a policy-rule template/generator with required METADATA

• ➕ Makes the correct pattern the default for authors
• ➕ Reduces reviewer burden and repeated feedback
• ➖ Requires adoption and tooling/documentation work
• ➖ Does not fully prevent manual deviations without CI enforcement

Recommendation: The documentation checklist is a low-cost, immediate improvement and is appropriate for reducing review misses right away. If missing effective_on continues to be a recurring source of breakage, follow up with an automated CI/lint check to make the requirement enforceable and less dependent on human/agent vigilance.

Files changed (1) +13 / -0

Documentation (1) +13 / -0

AGENTS.mdAdd review checklist for new policy rules (effective_on, collections, tests) +13/-0

Add review checklist for new policy rules (effective_on, collections, tests)

• Introduces a dedicated checklist section instructing reviewers/AI agents to require 'effective_on' for new deny/warn rules, and to verify collection membership and test coverage. Clarifies that missing 'effective_on' causes immediate enforcement and potential build breakage.

AGENTS.md

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:08 AM UTC · Completed 3:21 AM UTC
Commit: 87c4a29 · View workflow run →

[qodo-for-conforma]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

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

Flags with carried forward coverage won't be shown. Click here to find out more.
see 6 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] AGENTS.md — This PR modifies AGENTS.md, which is a protected governance/infrastructure file. The PR links to issue Add effective_on review checklist item to AGENTS.md for new deny rules #1760 and the description explains the rationale (adding a review checklist for effective_on dates). Human approval is always required for protected-path changes, regardless of context.

Low

• [scope-creep] AGENTS.md — The checklist includes two items beyond the issue's primary ask (effective_on): collection membership and test coverage. These are minor additions and arguably necessary for a complete review checklist, but they duplicate guidance already present elsewhere in AGENTS.md (line 29: "100% test coverage is enforced"; line 62: "declare collections: in METADATA"). The duplication is low-risk for a documentation-only change and serves the stated goal of giving review agents a single consolidated checklist.

Previous run

Review

Findings

Medium

• [protected-path] AGENTS.md — This PR modifies AGENTS.md, which is a protected governance file. The PR links to issue Add effective_on review checklist item to AGENTS.md for new deny rules #1760 and provides clear rationale for the change (adding review checklist guidance based on a retrospective of PR feat: deny releases using experimental Hermeto backends #1736). Human approval is always required for protected-path changes, regardless of context.

Low

• [Technical documentation correctness] AGENTS.md:76 — The collection membership checklist item states "New rules must be added to the appropriate collection(s) in policy/*/collection/", which could mislead a developer into modifying collection directory files directly. Collection membership is actually declared via the collections: key in each rule's own METADATA annotation block (confirmed by existing rule files and .claude/rules/policy-rules.md). The correct procedure is already documented in the "Common Change Patterns" section ("declare collections: in METADATA"). The same ambiguous phrasing also exists in the pre-existing Architecture section. Consider rewording to: "New rules must declare membership in the appropriate collection(s) via the collections: key in their METADATA annotation. See existing rules in policy/release/ for the pattern."

Previous run (2)

Review

Findings

Medium

• [protected-path] AGENTS.md — This PR modifies AGENTS.md, which is a governance/infrastructure file. The PR is well-justified by issue Add effective_on review checklist item to AGENTS.md for new deny rules #1760 and the description clearly explains the rationale. Human approval is required for changes to protected paths regardless of context.

Low

• [technical accuracy] AGENTS.md:71 — The effective_on checklist item covers the METADATA annotation pattern (under custom:) used when adding new deny/warn rules in .rego files, which is the correct target for the scenario that triggered issue Add effective_on review checklist item to AGENTS.md for new deny rules #1760. However, effective_on dates are also used in rule data entries in example/data/ YAML files (e.g., trusted_tekton_tasks.yml, required_tasks.yml), where they serve a similar migration-window purpose for data-driven rules. These are a different change pattern from adding new policy rules, but a reviewer following only this checklist might miss the data-file case.
  Remediation: Consider adding a brief note that rule data entries in example/data/ also use effective_on dates, though that is a separate change pattern.

---

Labels: PR modifies documentation (AGENTS.md review checklist)

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:39 AM UTC · Completed 3:50 AM UTC
Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:14 PM UTC · Completed 1:29 PM UTC
Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 3:03 PM UTC · Completed 3:14 PM UTC
Commit: 87c4a29 · View workflow run →

[fullsend-ai-retro]

Retro: PR #1789 — docs(EC-1954): add effective_on review checklist to AGENTS.md

Context

This documentation PR implements issue #1760, which was itself filed by the retro agent after analyzing PR #1736. On that earlier PR, the review agent ran 4 iterations but never flagged missing effective_on dates on new deny rules — human reviewer st3penta caught the gap instead. The retro correctly identified the root cause (AGENTS.md lacked explicit review guidance for effective_on) and filed a well-scoped issue.

Timeline

1. Jun 24 14:34 — Retro agent files #1760 proposing an effective_on review checklist in AGENTS.md
2. Jun 24 14:39 — Triage agent labels ready-to-code
3. Jun 24 14:44 — Code agent opens PR #1761, but the proposed text contains a technical inaccuracy (directs reviewers to check example/data/ files instead of METADATA annotations in .rego files)
4. Jun 24 — Review agent correctly flags the inaccuracy on docs(#1760): add effective_on review checklist to AGENTS.md #1761, requests changes. PR stalls.
5. Aug 3 03:07 — Human author cuipinghuo opens PR #1789 with correct implementation
6. Aug 3 03:21 — Review run 1: flags technical gap (data-file effective_on not mentioned) — author incorporates
7. Aug 3 03:50 — Review run 2: flags misleading collection membership wording — author incorporates and also fixes pre-existing Architecture section
8. Aug 3 13:29 — Review run 3: flags scope-creep (extra checklist items beyond issue scope), but acknowledges it's acceptable
9. Aug 3 14:51 — Human st3penta approves
10. Aug 3 15:03 — Author merges

What went well

• Retro-to-implementation pipeline worked end-to-end: The retro agent correctly identified a real review gap, filed a well-scoped issue, and the fix was ultimately merged.
• Review agent quality on PR docs(EC-1954): add effective_on review checklist to AGENTS.md #1789: All 3 inline findings were factually accurate. Two of three led to real improvements in the documentation. The fullsend review agent was the only automated reviewer (of three: CodeRabbit, Qodo, fullsend) to provide substantive feedback.
• Review agent as quality gate: Correctly blocked the code agent's inaccurate PR docs(#1760): add effective_on review checklist to AGENTS.md #1761, preventing wrong documentation from merging.
• Author responsiveness: Incorporated review feedback within hours and went beyond the suggestions (also fixing the pre-existing Architecture section wording).

Existing issues with new supporting evidence

• Repeated protected-path flag (3 identical [medium] protected-path findings across 3 review iterations): Provides additional evidence for fullsend#2794, fullsend#1013, and fullsend#2816.
• Self-undermining review finding (review 3's scope-creep finding concluded the approach was acceptable): Supports agents#537.
• Issue Add effective_on review checklist item to AGENTS.md for new deny rules #1760 still open, PR docs(#1760): add effective_on review checklist to AGENTS.md #1761 still open: PR docs(EC-1954): add effective_on review checklist to AGENTS.md #1789 references Add effective_on review checklist item to AGENTS.md for new deny rules #1760 but lacks a closing keyword. The code agent's PR docs(#1760): add effective_on review checklist to AGENTS.md #1761 is abandoned. Supports fullsend#2292 and fullsend#2285.
• Three full review cycles on a 1-file docs change: Supports agents#373 (skip irrelevant review dimensions for docs-only PRs).

Proposals filed

• Retro agent should verify proposed_change text against codebase before including it (in fullsend-ai/agents)