feat(skill): add triage-pr-review Claude Code skill (#35828)

[dsilvam]

Summary

• Adds .claude/commands/review-pr.md, a new Claude Code skill (/review-pr) that fetches ALL GitHub PR review feedback from every source and surfaces only Critical 🔴 and High 🟠 findings
• Skill queries all three GitHub endpoints: inline thread comments (pulls/{n}/comments), review summaries (pulls/{n}/reviews), and top-level bot/human comments (issues/{n}/comments)
• Reads the PR diff and description to evaluate each finding for relevance and necessity — assigns a Must fix / Nice to have / Out of scope — skip recommendation so developers don't waste time on pre-existing or cosmetic issues
• Automatically switches to the PR branch before reading files

Closes

Closes #35828

Test plan

• Run /review-pr <any-pr-number> and verify output includes findings from all three GitHub sources
• Verify the skill switches to the PR branch automatically
• Verify each finding shows a Recommendation label and Relevance note
• Verify Medium/Low findings are skipped with a count shown

🤖 Generated with Claude Code

[claude]

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

---

PR Review

The skill is well-iterated and the structure is sound. A few things worth a second look — most are correctness/clarity issues, not blockers.

---

1. PR description is stale vs. the skill it ships

The PR body says:

Automatically switches to the PR branch before reading files

But the skill (Step 1, line 28) explicitly says the opposite, which is the better design:

The local working tree is never touched. All file content is fetched via the GitHub Contents API…

This was clearly changed during review iterations. Update the PR body or future readers will be confused about what the skill does. Also worth dropping the matching test-plan item: "Verify the skill switches to the PR branch automatically".

---

2. allowed-tools may block the dead-code check

Frontmatter restricts to:

allowed-tools: Bash(gh api:*), Bash(gh pr view:*)

But Step 3 instructs:

For dead code / never called: only infer HIGH if you can confirm with Grep that no other file references the method…

If the harness honors allowed-tools as an allowlist for all tools (not just Bash patterns), Grep won't run and the only safe path is to skip — which makes the dead-code rule effectively dead itself. Either add Grep and Read to allowed-tools, or replace the Grep step with Bash(gh search code:*) / gh api .../search/code. Worth verifying the actual harness behavior before merging.

---

3. URL-encoding instruction in Step 5 is misleading

Step 5 says:

gh api repos/dotCMS/core/contents/<URL-encoded-path>?ref=<HEAD_SHA>

GitHub's Contents API takes the path as raw segments (slashes preserved, not %2F). URL-encoding / will 404. The only chars that need encoding are oddities like #, ?, spaces, which are vanishingly rare in this repo. Suggest changing to: <path> (slashes kept as-is; encode only #, ?, spaces if present).

---

4. No size/limits guidance for large PRs

• gh api .../pulls/<PR> with v3.diff returns the entire unified diff. Large refactors can exceed the model's context budget; nothing in the skill says "if diff > N lines, truncate / fetch per-file via .files[].patch".
• The Contents API caps files at ~1 MB; bigger files come back with content: "" and require the blobs API. Edge case, but the skill currently has no fallback.

At minimum, add a note: "if the file fetch returns empty content, fall back to gh api repos/dotCMS/core/git/blobs/<sha>."

---

5. Source C will pick up noise the skill doesn't filter

/issues/<n>/comments includes triggering @claude calls, this bot's own progress comments, "" placeholders, etc. The severity-marker rules in Step 3 will filter most of it, but a human top-level comment containing the word "NPE" in passing will still get matched by the implicit-severity rules. Two cheap mitigations:

• Skip comments authored by github-actions[bot], claude[bot], and dotcms-bot unless the body contains an explicit severity marker.
• Only run implicit-severity inference on inline comments (Source A) — Source C bots already use explicit markers, and Source C humans rarely write formal findings.

---

6. .original_line fallback can also be null

Step 2 / Step 5 fall back to .original_line when .line is null, but for some outdated comments (e.g., the file was deleted and re-added), both are null. The skill has no third fallback — instruction should be: "if both are null, mark [unable to locate] and still surface the finding with the reviewer's quoted text."

---

7. Minor: deduplication tiebreaker

Step 3 says "merge into one finding" when the same file+line shows up across sources, but doesn't say which source wins for the displayed Source: field. Suggest: prefer inline > bot comment > review summary (most specific wins).

---

8. Minor: HEAD_SHA race

Steps 1–5 use a single HEAD_SHA captured at the start. If the PR is force-pushed mid-run, file fetches still work (the SHA is immutable), but newly added findings or commit messages won't be picked up. A one-line caveat ("snapshot is taken at Step 1; re-run if the PR has been pushed since") would be honest.

---

Verdict

Logic is reasonable, severity inference has appropriate guardrails, the API-only design (no working-tree mutation) is the right call. Items 1 (stale PR body), 2 (allowed-tools vs. Grep), and 3 (URL-encoding) are the ones worth fixing before merge. The rest are polish.

• Read the new skill file
• Check git history / prior reviewer findings
• Analyze for correctness, edge cases, design issues
• Post review
  • Branch: feat/review-pr-skill-35828