Skip to content

ci: restore rule review as merge gate with defense-in-depth quality enforcement#3725

Open
sanity wants to merge 1 commit intomainfrom
quality-gates
Open

ci: restore rule review as merge gate with defense-in-depth quality enforcement#3725
sanity wants to merge 1 commit intomainfrom
quality-gates

Conversation

@sanity
Copy link
Copy Markdown
Collaborator

@sanity sanity commented Mar 31, 2026

Problem

PR #3682 made the AI rule review fully advisory — findings are posted as a comment but nothing blocks merge. This led to PR #3703 merging with 4 unaddressed WARNING findings (magic numbers, stale docs, tautological assert). More broadly, there's insufficient edge-case test coverage and missing regression tests on bug fixes.

Approach

Defense-in-depth — four layers so no single failure lets bad code through:

Layer 1: Agent rules (prevent bad code from being written)

  • testing.md now requires edge-case tests and regression tests for all fix: PRs
  • Explicit guidance on boundary conditions (zero, overflow, error paths, concurrent scenarios)
  • AGENTS.md now has a "WHEN fixing a bug" section requiring reproduce-first testing

Layer 2: CI grep enforcement (catch missing tests mechanically)

  • rule_lint job now rejects fix: PRs that don't add at least one new #[test] function
  • test-exempt label available for legitimate exceptions (must include justification comment)

Layer 3: AI review gate (catch nuanced issues)

  • claude-pr-review.yml now sets a commit status (rule-review/warnings) based on WARNING findings
  • WARNINGs block merge — must fix or get review-override label from maintainer
  • Review prompt expanded to flag:
    • Missing edge-case tests
    • fix: PRs without regression tests
    • Stale documentation in .claude/rules/ or AGENTS.md
  • Graceful degradation: if review fails to run, status is set to success (don't block on tool breakage)

Layer 4: GitHub ruleset (enforce the gate)

  • rule-review/warnings added as required status check

Key difference from pre-#3682

No per-finding /ack ceremony, no re-acking on force-push. Instead: fix warnings OR maintainer adds review-override label. Lighter friction, same enforcement.

Testing

  • Workflow changes are testable on this PR itself (the rule review will run against these changes)
  • Grep-based regression test check can be verified by creating a fix: PR without tests
  • Override flow: add review-override label → workflow re-runs → sets status to success

[AI-assisted - Claude]

…nforcement

Addresses quality concerns raised by lead dev: insufficient edge-case
testing, missing regression tests on bug fixes, and review findings
being silently ignored after PR #3682 made the review advisory-only.

Defense-in-depth layers:
1. Agent rules: testing.md now requires edge-case tests and regression
   tests for all fix: PRs, with specific guidance on boundary conditions
2. CI grep enforcement: rule_lint job now rejects fix: PRs that don't
   add at least one new #[test] function (test-exempt label to bypass)
3. AI review gate: claude-pr-review.yml now sets a commit status
   (rule-review/warnings) that blocks merge when WARNINGs are found.
   Review prompt expanded to flag missing edge-case tests, missing
   regression tests, and stale documentation. review-override label
   provides escape hatch for legitimate exceptions.
4. GitHub ruleset updated: rule-review/warnings added as required check

Key difference from pre-#3682: no per-finding /ack ceremony, no
re-acking on force-push. Instead: fix warnings OR maintainer adds
review-override label. Lighter friction, same enforcement.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

Rule Review: Minor stale docs and a logic edge case

Rules checked: git-workflow.md
Files reviewed: 4 (.claude/rules/testing.md, .github/workflows/ci.yml, .github/workflows/claude-pr-review.yml, AGENTS.md)

Warnings

None.

Info

  • .claude/rules/git-workflow.md — The "Valid prefixes" list (feat, fix, docs, refactor, test, build) is incomplete. The actual CI check (amannn/action-semantic-pull-request in ci.yml:59-70) also accepts ci, style, perf, chore, revert. This PR's own ci: title is valid per CI but technically undocumented in git-workflow.md. The new rule being added in this PR flags stale documentation as a WARNING — this is a mild case of exactly that. (rule: git-workflow.md — stale docs)

  • .github/workflows/claude-pr-review.yml:373 — The warningCount regex (/^-\s+\[^]+\\s+—/gm) counts **all** bullet items in the report matching that format, including those under #### Info. If the AI generates a #### Warningssection heading (even with no items) alongside info-only findings,hasWarningscould evaluate totrueand incorrectly block merge. Scoping the count to lines between#### Warningsand the next####` would be more robust. (rule: no specific rule — implementation correctness)


Advisory review against .claude/rules/. Critical patterns are enforced by the Rule Lint CI job.

@sanity sanity requested a review from iduartgomez March 31, 2026 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant