Skip to content

fix(ci): backend reviewer upserts a sticky comment and unblocks fixed PRs - #36485

Merged
dsilvam merged 3 commits into
mainfrom
fix-backend-reviewer-sticky-comment
Jul 10, 2026
Merged

fix(ci): backend reviewer upserts a sticky comment and unblocks fixed PRs#36485
dsilvam merged 3 commits into
mainfrom
fix-backend-reviewer-sticky-comment

Conversation

@dsilvam

@dsilvam dsilvam commented Jul 9, 2026

Copy link
Copy Markdown
Member

Proposed Changes

Fixes two problems with the Claude Backend Code Reviewer workflow (.github/workflows/ai_claude-backend-reviewer.yml) where the reviewer failed to reflect that reported issues had been fixed:

  • Sticky comment (STEP 5). The reviewer posted a brand-new comment on every push and never updated the previous one, so once a push resolved the findings the stale comment kept showing already-fixed 🟠/🔴 issues, and the clean "no issues found" state was posted unreliably. It now upserts a single comment identified by the hidden <!-- dotcms-backend-review --> marker — editing it in place (PATCH /issues/comments/{id}) and creating one only on the first run. The upsert always runs, including when the result is "no issues found", so the comment always reflects the current state of the branch. No allowlist change was needed for this (listing + PATCHing issue comments were already permitted).

  • Formal review could never run (STEP 6). STEP 6 already approves when no Critical/High findings remain — which supersedes a prior --request-changes and unblocks the PR — but gh pr review was never in the orchestrator's allowedTools, so the step was silently blocked and PRs stayed blocked even after fixes. Added Bash(gh pr review*) to allowedTools and clarified STEP 6 that the reviewer's latest review supersedes its previous one.

Root cause

The prompt instructed "post it as a new comment (each push gets its own comment for a historical record)" with the clean-path as a weak secondary clause, and gh pr review was omitted from the sandbox allowlist.

Checklist

  • Tests
  • Translations
  • Security Implications Contemplated (add notes if applicable)

Additional Info

Security note: no expansion of the sandbox beyond formal PR reviews. The comment upsert reuses the already-permitted gh api .../issues/comments/* scope; the only new capability is gh pr review, needed for the merge-gating that STEP 6 already intends to perform.

Caveat to verify: the STEP 6 --approve path also depends on the org/repo setting "Allow GitHub Actions to create and approve pull requests." If that setting is off, the bot's approve may still be rejected by GitHub regardless of the allowlist; the --request-changes path is unaffected. Worth confirming during rollout.

🤖 Generated with Claude Code

dsilvam and others added 2 commits July 9, 2026 19:51
…ppending

The Claude Backend Code Reviewer posted a brand-new comment on every push and
never updated the previous one, so once a push resolved the reported findings
the stale comment kept showing already-fixed 🟠/🔴 issues, and the clean
"no issues found" state was posted unreliably.

STEP 5 now upserts a single comment identified by the hidden
`<!-- dotcms-backend-review -->` marker: it edits the existing comment in place
(PATCH /issues/comments/{id}) and only creates one on the first run. The upsert
always runs, including when the result is "no issues found", so the comment
always reflects the current state of the branch. No allowlist change needed —
listing and PATCHing issue comments were already permitted.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… unblock

STEP 6 already approves when no Critical/High findings remain (which supersedes
a prior request-changes and unblocks the PR), but `gh pr review` was never in
the orchestrator's allowedTools, so the step could not run — the reviewer left
PRs blocked even after the findings were fixed.

Add `Bash(gh pr review*)` to allowedTools and clarify STEP 6 that the reviewer's
latest review supersedes its previous one, so a later approve clears an earlier
request-changes automatically.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

❌ Issue Linking Required

This PR could not be linked to an issue. All PRs must be linked to an issue for tracking purposes.

How to fix this:

Option 1: Add keyword to PR body (Recommended - auto-removes this comment)
Edit this PR description and add one of these lines:

  • This PR fixes #123 or Fixes: #123

  • This PR closes #123 or Closes: #123

  • This PR resolves #123 or Resolves: #123

  • Other supported keywords: fix, fixed, close, closed, resolve, resolved
    Option 2: Link via GitHub UI (Note: won't clear the failed check)

  1. Go to the PR → Development section (right sidebar)

  2. Click "Link issue" and select an existing issue

  3. Push a new commit or re-run the workflow to clear the failed check
    Option 3: Use branch naming
    Create a new branch with one of these patterns:

  • 123-feature-description (number at start)

  • issue-123-feature-description (issue-number at start)

  • feature-issue-123 (issue-number anywhere)

Why is this required?

Issue linking ensures proper tracking, documentation, and helps maintain project history. It connects your code changes to the problem they solve.---

This comment was automatically generated by the issue linking workflow

@github-actions github-actions Bot added the Area : CI/CD PR changes GitHub Actions/workflows label Jul 9, 2026
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dsilvam's task in 1m 36s —— View job


Code Review

  • Gather context (diff vs origin/main)
  • Review STEP 5 sticky-comment upsert logic
  • Review STEP 6 formal-review logic & allowlist change
  • Post review

I reviewed the full diff against origin/main. This is a focused, well-reasoned change to a single workflow file. The logic is sound and the tricky parts (pagination + per-page --jq) are handled correctly.

New Issues

  • 🟡 Medium: .github/workflows/ai_claude-backend-reviewer.yml:261Stale duplicate comments from the old append behavior are never reconciled. The lookup takes the newest matching comment (tail -n1) and edits only that one. On any PR that already accumulated multiple `` comments under the previous "new comment per push" behavior, the older ones stay on the PR unchanged — still showing already-fixed 🟠/🔴 findings, which is the exact symptom this PR sets out to eliminate. Fresh PRs are fine; only in-flight PRs are affected, and it self-heals as those merge. Non-blocking, but worth a one-time manual cleanup or a note in the rollout. Fix this →

Notes (non-blocking, verified correct)

  • STEP 6 gate is robust (:280). grep -qE '🔴 Critical|🟠 High' runs over the whole body. The only places those exact strings occur are real finding lines and the mandatory dotcms-review-findings machine block (which carries "sev":"🔴 Critical" / "sev":"🟠 High" for open findings only, never resolved ones). The static "Next steps" line (🔴 / 🟠 Fix locally) does not contain 🔴 Critical, so it won't cause a false --request-changes. The machine block also acts as a nice defense-in-depth fallback if the model ever bolds a header (🔴 **Critical**).
  • Pagination fix is correct (:256-261). Emitting one bare .id per line and using tail -n1 avoids the per-page aggregate-null trap the comment describes. Good catch and good comment.
  • gh api ... -F body=@/tmp/dotcms_review_body.md (:263) reads the file as the field value against the correct PATCH /issues/comments/{id} endpoint. Body always starts with `

@mergify

mergify Bot commented Jul 9, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

The comment lookup used an aggregate jq filter (`[...] | last | .id`) with
`gh --paginate`. gh runs `--jq` once per page and concatenates, so the aggregate
returned a per-page result and pages with no match emitted `null` — producing a
multi-line EXISTING_ID (e.g. "12345\nnull") on PRs with more than one page of
comments. That passed the `-n`/`!= "null"` guards, took the PATCH branch with a
malformed URL, and silently stopped updating the sticky comment — the exact
staleness this change is meant to prevent, just gated behind a comment count.

Emit one bare id per matching comment and take the global last with `tail -n1`
(non-matching pages emit nothing, not `null`); drop the now-redundant
`!= "null"` guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dsilvam
dsilvam added this pull request to the merge queue Jul 10, 2026
Merged via the queue into main with commit 4eaf393 Jul 10, 2026
37 of 38 checks passed
@dsilvam
dsilvam deleted the fix-backend-reviewer-sticky-comment branch July 10, 2026 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : CI/CD PR changes GitHub Actions/workflows

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants