review-rocmlir-triton-pr

# rocmlirTriton 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/commits.json` | `Read('/tmp/pr/commits.json')` -- commit subjects, touched paths, and `diffPath` entries for commit-level checks. | | `/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-triton-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/commits.json` | Commit metadata for the same pinned PR range, as an array of `{sha, subject, paths, diffPath}`. Use it for commit-level rules such as `[EXTERNAL]` subject checks. Read the referenced `diffPath` file only when you need the exact diff for that commit. | | `/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. Azure and 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 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/commits.json')` to see commit subjects and touched paths. When a rule depends on the exact contents of one commit, read the listed `diffPath` for that commit instead of trying to run `git show`. - `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/`, `docs/PR_REVIEW_CHECKLIST.md`, or `docs/bump_triton_version.md` These paths are the workflow's "trust perimeter": their workspace contents have been **replaced** with the trusted default-branch versions by an overlay step that runs before this skill, because their semantics are what decide whether secrets are protected at runtime and what reviewers flag as findings (`.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; `docs/PR_REVIEW_CHECKLIST.md` is the **single source of truth** for the Critical / Major / Minor review tiers categorization you apply in Step 3; `docs/bump_triton_version.md` is the detailed Triton-bump guide you apply in Step 4). 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` | | `docs/bump_triton_version.md` | `/tmp/pr-source/docs/bump_triton_version.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 these 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` and `docs/bump_triton_version.md` are NOT in Layer 3's perimeter regex (they are docs files, not security-sensitive), so a label-trigger PR may legitimately diff them -- still review the PR-side versions under `/tmp/pr-source/docs/`; the workspace copies are the trusted versions your review criteria 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** for review tiers. The Triton bump guide reaches you through `docs/bump_triton_version.md`. The workflow loads both for you in two ways, sourced from the same default-branch ref: 1. The `snapshot_review_checklist` workflow step reads the overlaid, trusted files at runtime and substitutes their content into the prompt heredoc between the `<BEGIN/END docs/PR_REVIEW_CHECKLIST.md>` and `<BEGIN/END docs/bump_triton_version.md>` markers (the canonical reference sections above this skill in your conversation). You already have them in context -- no Read needed. 2. The same files are overlaid into the workspace (see the Special case section above), so `Read('docs/PR_REVIEW_CHECKLIST.md')` and `Read('docs/bump_triton_version.md')` return the same bytes if you want to confirm. Both channels come from the same files at the same workflow run, so they are byte-identical by construction. Categorize each finding against the **Critical / Major / Minor** tiers and the license-header template defined in the PR review checklist. 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 below applies the rocmlirTriton-specific sections of the same file (vendored Triton and Triton-pinned LLVM subtree updates, downstream patch records, `rock::*` hardware-feature detection, bridge passes, fat-library + MIGraphX, and the rocMLIR back-port check). --- ## Step 4 -- Apply the rocmlirTriton-specific review rules `docs/PR_REVIEW_CHECKLIST.md` (injected into the prompt above by the snapshot step and overlaid into the workspace; see Step 3) documents two rocmlirTriton-specific sections that don't fit the generic LLVM/MLIR tiers. `docs/bump_triton_version.md` is also injected and overlaid; apply it as the detailed source of truth when a PR imports new upstream Triton/LLVM revisions or changes downstream patch records: - **`## rocmlirTriton-specific checks`** -- vendored Triton and Triton-pinned LLVM subtree updates (delegating detailed steps to `docs/bump_triton_version.md`), downstream patch records, `[EXTERNAL]` commit boundaries for `external/triton` / `external/llvm-project` edits, `rock::*` hardware-feature detection (vs. `triton::AMD::TargetInfo`), bridge passes between Rock and Triton, and fat-library + downstream MIGraphX coordination. Each sub-rule documents its severity inline (Major / Minor). - **`## rocMLIR back-port check`** -- the path list of files shared with `ROCm/rocMLIR`, the rocmlirTriton-only path list, and the verdict logic for missing back-port notes (Major when none of options a/b/c is in the PR description). Compute the intersection of `meta.files[].path` against the path list there. Apply both alongside Step 3. Cite the matching section heading in the finding body (for example: "rocmlirTriton-specific -- Triton subtree updates: `librockcompiler_deps.cmake` not regenerated") so the author can look up the rationale. For vendored-subtree changes, always read `/tmp/pr/commits.json`: - A downstream commit whose `paths` includes a path under `external/triton/` or `external/llvm-project/` must have a subject beginning with `[EXTERNAL]`; otherwise raise a Major finding under "rocmlirTriton-specific checks -- Vendored Triton / LLVM subtrees". Do not flag upstream import / bump commits for lacking `[EXTERNAL]`; those are covered by the Triton / LLVM subtree update checks. - If the PR also changes `triton-patches/*.patch` or `llvm-patches/*.patch`, compare those patch-file contents with the relevant downstream `[EXTERNAL]` commit's `diffPath`. A patch record that does not match the corresponding `[EXTERNAL]` commit diff is a Major finding. --- ## Step 5 -- 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** --