review-rocmlir-pr

# rocMLIR PR Review ## IMPORTANT: Do NOT post to GitHub This skill is **read-only**. Do NOT post any comments, reviews, or reactions. Do NOT use `gh pr comment`, `gh pr review`, `gh api ... -X POST/PUT/PATCH/DELETE`. Posting is the job of the workflow's post step, which runs in a separate job that does not have access to the LLM Gateway secrets. --- ## Tool budget — READ THIS BEFORE STEP 1 This skill runs in one of two modes; the available tools are different in each. Picking the wrong tool wastes turns on permission denials and can starve the review of budget before it reaches the final JSON output. **CI mode (default — this is your mode if you are reading this from `.github/workflows/claude_auto_review.yml`).** The workflow passes `--allowedTools "Skill,Read,Grep,Glob"` and `--json-schema '...'` to claude-code-action; the final JSON answer is captured as the action's `structured_output`. The pre-fetched context is already on disk under `/tmp/pr/` (the workflow has already done all the `gh`/`jq` work for you): | File | How to access | |---|---| | `/tmp/pr/meta.json` | `Read('/tmp/pr/meta.json')` -- a few KB, read it whole. | | `/tmp/pr/diff.patch` | `Read('/tmp/pr/diff.patch')` -- can be tens of KB; use `Read` offset/limit to page through it, or `Grep` it for a specific path. | | `/tmp/pr/checks.json` | `Read('/tmp/pr/checks.json')` then scan the array yourself for entries with `bucket == "fail"` or `bucket == "cancel"`. | | `/tmp/pr/prev_comments.json` | `Read('/tmp/pr/prev_comments.json')` then scan for entries authored by `rocmlir-pr-reviewer[bot]` with `in_reply_to_id == null` and the marker `<!-- claude-pr-review-marker:v1 -->` in the body. | | PR-head source files (any path in `meta.files`) | `Read('<path>')` directly from the working directory -- the PR head is checked out there. Use `Grep`/`Glob` to navigate. | In CI mode, **do not attempt** `Bash`, `jq <something>`, `head -200 file`, `gh api ...`, `cat`, `find`, `curl`, `wget`, `Write`, or any other shell-style or write-side tool. None of these are in the allowed list and every attempt returns a permission denial that counts against `--max-turns`. If you find yourself reasoning "I should run X to extract Y", stop and reformulate as "I should `Read` (or `Grep`, or `Glob`) Z". Examples in this file shown inside code fences are documentation for the *interactive* mode (see the appendix at the bottom); they are never to be executed in CI. **Interactive Stage-B mode (local dry-run only).** A maintainer runs the standalone Claude Code CLI with a broader tool set, e.g. claude --allowedTools "Skill,Read,Grep,Glob,Bash(gh *),Bash(jq *)" \ --skill review-rocmlir-pr <PR-number> In this mode `/tmp/pr/` may not be populated; pre-fetch it yourself with the commands in the [appendix](#appendix-interactive-stage-b-only-do-not-execute-in-ci). Everything in the appendix is **off-limits in CI mode**. --- ## Step 1 -- Load PR context The workflow has pre-fetched the PR data into `/tmp/pr/`. Note that several fields are deliberately derived from the LOCALLY checked-out PR HEAD instead of the live PR API, to defend against a force-push that lands during the review run -- everything in the table below describes the SAME pinned SHA the workspace is on (`headRefOid` in `meta.json`): | File | Contents | |------|----------| | `/tmp/pr/meta.json` | PR metadata: `title`, `body`, `author`, `baseRefName`, `headRefName`, plus two locally-injected fields. `headRefOid` is the SHA of the pinned checkout in the workspace (force-push defense), and `files` is an array of `{path}` objects describing the same set of changed paths that `diff.patch` covers. | | `/tmp/pr/diff.patch` | Unified diff between the merge-base with `baseRefName` and the pinned PR HEAD. Equivalent to GitHub's "Files changed" view for this SHA, but generated locally so it can never disagree with the workspace or with `meta.files` if a force-push lands mid-run. | | `/tmp/pr/checks.json` | CI status: an array of `{name, state, bucket}` covering both the modern Checks API (e.g. GitHub Actions) and the legacy Commit Statuses API (e.g. some Jenkins integrations), so neither category of red CI is silently missed. `bucket` is one of `pass`, `fail`, `pending`, `skipping`, `cancel`. | | `/tmp/pr/prev_comments.json` | All existing inline review comments on this PR, in the order the GitHub API returns them. | The PR head is checked out in the working directory, so you can `Read` source files directly to see them at their PR-state line numbers. In CI mode the four files in the table above are *already populated*; the only thing you need to do is `Read` them. Concretely: - Start by `Read('/tmp/pr/meta.json')`. Scan the JSON yourself for `title`, `headRefOid`, and `files[].path` -- the file is a few KB and `Read` returns the whole content. Do not try `jq` -- it is not in your tool set. - `Read('/tmp/pr/diff.patch')` to see the unified diff. If the file is large use `Read` with an `offset`/`limit`, or use `Grep` to jump to a specific path within the patch. - `Read('/tmp/pr/checks.json')` and scan the array for entries whose `bucket` is `"fail"` or `"cancel"`. Mention any such entries in your summary so the review reflects the PR's actual CI state. - `Read('/tmp/pr/prev_comments.json')` to discover previous Claude comments for the re-review path; see the Output section for the filter rule. ### Special case: changes under `.claude/`, `.github/scripts/`, or `docs/PR_REVIEW_CHECKLIST.md` These three paths are the workflow's [trust perimeter](../../../.github/workflows/CLAUDE_AUTO_REVIEW.md#17-glossary). Their workspace contents have been **replaced** with trusted default-branch versions by an overlay step that runs before this skill. `.claude/skills/` is what *you* are reading right now, `.github/scripts/sanitize_claude_actions.sh` is what gates your output before it leaves the runner, and `docs/PR_REVIEW_CHECKLIST.md` is the **single source of truth** for the Critical / Major / Minor tier categorization you apply in Step 3. If `diff.patch` shows changes under any of these paths, **the workspace copies are NOT the PR's proposed versions**. The PR-side versions are at: | Workspace path (overlaid -> develop's version) | PR-side version (what you should review) | |---|---| | `.claude/skills/foo/SKILL.md` | `/tmp/pr-source/.claude/skills/foo/SKILL.md` | | `.github/scripts/post_claude_review.sh` | `/tmp/pr-source/.github/scripts/post_claude_review.sh` | | `.github/scripts/sanitize_claude_actions.sh` | `/tmp/pr-source/.github/scripts/sanitize_claude_actions.sh` | | `docs/PR_REVIEW_CHECKLIST.md` | `/tmp/pr-source/docs/PR_REVIEW_CHECKLIST.md` | If `/tmp/pr-source/<path>` does not exist while `diff.patch` shows changes to `<path>`, the PR has deleted that file. Use `Read` on the snapshot path to see the PR's proposed file content; use the workspace path only if you explicitly want to see the trusted runtime version for comparison. **Files NOT under those three paths are unaffected** -- read them directly from the workspace as usual. This special case only applies on the workflow_dispatch path; PRs that touch `.claude/` or `.github/scripts/` under the label-trigger path are blocked by Layer 3 of the workflow and never reach this skill. `docs/PR_REVIEW_CHECKLIST.md` is NOT in Layer 3's perimeter regex (it's a docs file, not security-sensitive), so a label-trigger PR may legitimately diff it -- still review the PR-side version at `/tmp/pr-source/docs/PR_REVIEW_CHECKLIST.md`; the workspace copy is the trusted version your tier categorization actually used. Identify the changed `.cpp`, `.h`, `.td`, `.mlir`, `.py`, `CMakeLists.txt`, and `.cmake` files from `meta.json`. `Read` the ones with non-trivial diffs in full. > Interactive Stage-B (local dry-run only): if `/tmp/pr/` is not already populated > for you, the pre-fetch commands are in the [appendix at the bottom of this > file](#appendix-interactive-stage-b-only-do-not-execute-in-ci). **Do not run > them in CI** -- in CI the files are already there and your tool set does not > include `Bash`. --- ## Step 2 -- CRITICAL SCOPE RULE Only flag issues that exist in the PR diff itself -- lines added or modified by this PR. Do NOT flag pre-existing code that the PR did not touch, even if that code is in the same files. If a pre-existing problem is worth noting, mention it briefly in a `Pre-existing issues (out of scope)` section in the summary -- never as an inline finding against this PR. --- ## Step 3 -- Apply the PR review checklist The PR review checklist reaches you through `docs/PR_REVIEW_CHECKLIST.md` -- the **single source of truth**. The workflow loads it for you in two ways, both sourced from the same default-branch ref: 1. The `snapshot_review_checklist` workflow step reads the (overlaid, trusted) file at runtime and substitutes its content into the prompt heredoc between the `<BEGIN/END docs/PR_REVIEW_CHECKLIST.md>` markers (the "## PR review checklist (canonical reference)" section above this skill in your conversation). You already have it in context -- no Read needed. 2. The same file is overlaid into the workspace (see the Special case section above), so `Read('docs/PR_REVIEW_CHECKLIST.md')` returns the same content if you want to confirm. Both channels read from the same trusted file at the same workflow run, so the **Critical / Major / Minor** tiers and the license-header template you see in either view are authoritative. The two views are content-equivalent but not literally byte-identical: the injected BEGIN/END block intentionally omits the H1 + blank-line prelude (the prompt's "## PR review checklist" heading replaces it) and is LF-normalized, while `Read('docs/PR_REVIEW_CHECKLIST.md')` returns the full file as-is. Use either. Categorize each finding against those tiers. Each finding must: - Cite the exact `file:line` from the PR head (not diff-relative line numbers). - Include a concrete, actionable proposed fix in the `body`. - Reference the specific bullet in `docs/PR_REVIEW_CHECKLIST.md` that applies, so the author can look up the rationale (for example: "Critical: `using namespace std` at file scope" or "Major: `std::vector` for small local collections"). --- ## Step 4 -- Output Return a single JSON object with this exact shape AS YOUR FINAL RESPONSE. Do not write it to a file -- the workflow uses claude-code-action's `--json-schema` flag to validate your final message and capture it as the action's `structured_output`. Findings without a concrete `path` and `line` from the diff MUST be dropped (do not coerce them into the summary). ```json { "verdict": "APPROVE", "summary": "## Scope\n[1-2 sentences on what the PR does]\n\n## Findings\nNo blocking issues found.\n\n## Notes\n[optional observations]", "inline_comments": [ { "path": "mlir/lib/Dialect/Rock/Transforms/Foo.cpp", "line": 142, "side": "RIGHT", "severity": "Major", "body": "`std::vector<int64_t>` here is preferred as `SmallVector<int64_t, 4>` per LLVM coding standards. (Will also need `#include \"llvm/ADT/SmallVector.h\"`.)", "suggestion": " SmallVector<int64_t, 4> indices;" } ], "thread_updates": [] } ``` Field rules: - `verdict` -- one of `APPROVE`, `REQUEST_CHANGES`, `COMMENT`. Pick what a human reviewer would say about the PR's current state: any Critical finding -> `REQUEST_CHANGES`; zero findings -> `APPROVE`; otherwise `COMMENT` unless the findings materially affect correctness/security (then `REQUEST_CHANGES`). Justify the choice in `summary`. On a re-review the verdict reflects the PR's CURRENT state after the author's fixes -- a previously-REQUEST_CHANGES PR with everything resolved gets an `APPROVE`. The post job submits **every verdict** as `gh pr review --comment` (the rendered body header shows your verdict with a "submitted as COMMENT" annotation). **Do NOT change your verdict because of this** -- emit the honest verdict; full rationale in `CLAUDE_AUTO_REVIEW.md` §13. - `summary` -- Markdown body of the formal review. The post job prepends a one-line header (verdict + finding counts); EVERYTHING else is this field. Use `##` section headings; typical layout: ## Scope -- 1-2 sentences: what the PR does / files touched ## Findings -- one-sentence each, citing `path:line` (full bodies go in `inline_comments[]`). Zero findings: a single confirming line, e.g. "No blocking issues found." ## Notes -- (optional) spot-checks, out-of-scope observations, follow-up suggestions for a separate PR ## CI status -- (optional) non-self checks in `/tmp/pr/checks.json` with `bucket == "fail"` or `"cancel"`; the auto-review pipeline's own check is expected to be in-progress / failed and is NOT a CI failure Target ~2000 chars (sanitizer cap is 8 KiB). Do NOT restate individual findings (those go in `inline_comments[]`). Do NOT print "Verdict: ..." in the body -- the post job adds that header line. - `inline_comments[].path` -- repo-relative file path, must appear in the PR diff. - `inline_comments[].line` -- exact line number in the PR head (`headRefOid` in `/tmp/pr/meta.json`); do NOT use diff-relative line numbers. - `inline_comments[].side` -- MUST be `"RIGHT"`. We do not support `"LEFT"` comments (which would target deleted lines on the *base* file). GitHub's PR-review-comment API uses *base-file* line numbers for `LEFT` comments but *head-file* line numbers for `RIGHT` comments; this skill's "use the PR head line number" rule (and the diff context loaded into the prompt) is head-relative only, so a `LEFT` comment would either anchor to the wrong place or get a 422 from GitHub. If a deleted line is the right place to discuss something, comment instead on the nearest surviving RIGHT-side context line and reference the deletion in the body. - `inline_comments[].severity` -- one of `Critical`, `Major`, `Minor`. - `inline_comments[].body` -- a single short paragraph: what the issue is, why it matters, and a concrete proposed fix (in prose -- the verbatim replacement, if any, goes in `suggestion`). - `inline_comments[].suggestion` -- **OPTIONAL.** Verbatim replacement text for the single line at `line`. The post script wraps this in a fenced ` ```suggestion ` block at the bottom of the comment, which renders as a one-click "Commit suggestion" button in the GitHub UI. Strict rules: 1. **Single line only.** Multi-line ranges are not supported. The schema and sanitizer both reject any `suggestion` containing `\n`, `\r`, or ` ``` ` (which would close the wrapping fence early). 2. **Verbatim, with correct indentation.** GitHub commits the bytes as-is. Match the file's existing indentation (tabs vs spaces, depth) exactly. No trailing newline -- surrounding file lines already provide it. 3. **Self-contained.** Fully address the finding without requiring matching