feat(ci): block PR merge on Critical/High backend review findings

[dsilvam]

Summary

• Adds STEP 6 to the AI backend reviewer workflow (ai_claude-backend-reviewer.yml)
• After posting the review comment, the orchestrator now submits a formal GitHub PR review:
  • Critical or High findings → REQUEST_CHANGES (merge blocked until resolved)
  • Clean review → APPROVE (automatically lifts a previous block on the next clean push)

Motivation

PR #35345 had a 🔴 Critical cross-site content-bleed finding flagged by the automated reviewer — twice. It was ignored and the PR was approved and merged, leading to a production incident (Lennox ticket #36966: wrong brand content rendering on product pages after release 26.04.28-02).

The previous workflow only posted a comment. Comments are easy to ignore. A formal REQUEST_CHANGES review cannot be bypassed with a simple Approve click — it must be explicitly dismissed or resolved.

Setup required

To fully enforce the block, enable the following in Settings → Branches → Branch protection rules on main:

• ✅ Require a pull request before merging
• ✅ Dismiss stale pull request approvals when new commits are pushed

Test plan

• Open a PR with a Java change that triggers a Critical/High finding — verify the bot submits REQUEST_CHANGES and the merge button is blocked
• Push a fix — verify the bot submits APPROVE on the clean re-run and the block is lifted

Closes #35614

🤖 Generated with Claude Code

[claude]

Claude finished @dsilvam's task in 2m 24s —— View job

---

Review

• Read PR diff and workflow file
• Analyze STEP 6 logic, allowedTools, and blocking mechanism
• Post review

---

This change won't actually block merges as described. Several issues, ordered by severity.

🔴 Critical — grep and gh pr review are not in --allowedTools, so STEP 6 cannot execute

The orchestrator's allowlist at .github/workflows/ai_claude-backend-reviewer.yml:96 is:

Agent,Bash(git diff*),Bash(git log*),Bash(git show*),Bash(cat ...),
Bash(find ...),Bash(grep -rn dotCMS/src*),Bash(gh pr comment*),
Bash(gh api repos/.../issues/comments/*),Bash(jq*)

STEP 6 introduces two commands that are not allowed:

• grep -qE '🔴 Critical|🟠 High' /tmp/dotcms_review_body.md — the only grep permission is Bash(grep -rn dotCMS/src*), which won't match this invocation.
• gh pr review ... — only gh pr comment* and a narrow set of gh api calls are whitelisted.

Net effect: Claude will be denied permission, the orchestrator will skip or error, and no formal review will be submitted. The PR's stated goal silently does nothing. This needs to be added explicitly, e.g. Bash(grep*),Bash(gh pr review*).

Fix this →

🔴 Critical — REQUEST_CHANGES from this bot does not actually block merge

.github/CODEOWNERS only lists @dotCMS/dotDevelopers for /.github/. The Claude bot is not a CODEOWNER and is not configured as a required reviewer anywhere. Under standard GitHub branch protection, a REQUEST_CHANGES review from a non-required reviewer is informational — the merge button stays enabled. The setup steps in the PR description ("Require a pull request before merging" + "Dismiss stale approvals") do not enforce blocking on bot REQUEST_CHANGES.

To actually block, one of:

• Add the bot identity to CODEOWNERS for paths covered by this workflow (e.g. **/*.java), and enable "Require review from Code Owners" + "Require approval of the most recent reviewable push".
• Or convert this into a required status check (set the orchestrator's job to fail when Critical/High findings exist, then require that check in branch protection). Status checks are far more reliable for enforcement than synthetic PR reviews.

The status-check approach is also robust to the bot being unable to self-approve and to fork PRs with read-only tokens.

🟠 High — Sub-agent failures default to APPROVE (fails open)

STEP 4 instructs the orchestrator to insert a ⚠️ <Domain> review unavailable warning when a sub-agent crashes. STEP 6's grep only looks for 🔴 Critical|🟠 High, so a warning row does not trigger REQUEST_CHANGES — the bot will APPROVE despite incomplete coverage. Given the motivating incident (a missed Critical finding), failing open on sub-agent error reproduces exactly the failure mode this PR is trying to prevent. Either grep must also match ⚠️, or the orchestrator must exit non-zero when any agent failed.

🟠 High — Missing review file → APPROVE (fails open)

if grep -qE '🔴 Critical|🟠 High' /tmp/dotcms_review_body.md; then
  ... --request-changes ...
else
  ... --approve ...
fi

If /tmp/dotcms_review_body.md doesn't exist (STEP 5 errored, file path typo, etc.), grep exits 2 (treated as falsy), and the bot approves. Should be:

[ -f /tmp/dotcms_review_body.md ] || { echo "review body missing"; exit 1; }

…before the conditional, so a broken pipeline blocks rather than green-lights.

🟡 Medium — Stale REQUEST_CHANGES survives concurrency cancel

The job uses cancel-in-progress: true keyed by PR number. If push N is cancelled mid-run by push N+1, push N's REQUEST_CHANGES from a prior successful run is still active. Push N+1 will eventually post a fresh review — but if N+1 itself is cancelled, the stale block remains until a successful run completes. Not catastrophic, but worth a continue-on-error cleanup or final-state guarantee.

🟡 Medium — Approval semantics with GITHUB_TOKEN

gh pr review --approve submitted by GITHUB_TOKEN does not count as a "required approver" for branch protection's "Require N approvals" rule. Currently this is only used to override the bot's own prior REQUEST_CHANGES (which works, since the latest review state per reviewer is what's evaluated), so this is fine given the design — but it's a constraint to document so nobody adds "require 1 approval" thinking the bot satisfies it.

🟡 Medium — PR description claim is partially incorrect

✅ Dismiss stale pull request approvals when new commits are pushed

This setting only dismisses approvals, not REQUEST_CHANGES. It's not what lifts the block on a clean re-run — what lifts it is the bot's new APPROVE replacing its prior REQUEST_CHANGES (latest-review-per-reviewer semantics). The setup note as written misleads readers into thinking that toggle is the unblocking mechanism.

Nit

• gh pr review body strings contain literal emoji; fine on ubuntu-latest (UTF-8 locale), but the bot identity submitting the review is whatever the orchestrator workflow's token resolves to — worth confirming in a dry-run before relying on it as a merge gate.

---

Bottom line

The change is 17 lines but the surface area is large because it depends on (a) tool allowlist, (b) branch protection config, (c) CODEOWNERS, and (d) sub-agent failure semantics — all of which currently make this a no-op or fail-open. I'd recommend:

1. Add Bash(grep*) and Bash(gh pr review*) to allowedTools (otherwise nothing else matters).
2. Switch enforcement to a required status check (job exits non-zero on Critical/High) rather than relying on bot REQUEST_CHANGES + branch-protection alignment.
3. Fail closed on missing review file and on sub-agent failures.
   • feat/backend-reviewer-block-on-critical-v2