diff --git a/.agents/skills/pr-review/SKILL.md b/.agents/skills/pr-review/SKILL.md index b76f0a524da..397eead1786 100644 --- a/.agents/skills/pr-review/SKILL.md +++ b/.agents/skills/pr-review/SKILL.md @@ -18,7 +18,7 @@ code is designed" never justifies a behavior on its own. ## 1. Collect the change The review target is `HEAD` unless you were given a commit. Set `TARGET` to it -and `BASE` to its merge base with main: +and `BASE` to its merge base with the PR's base branch: ```bash TARGET=HEAD @@ -27,26 +27,31 @@ git diff --stat "$BASE" "$TARGET" git diff "$BASE" "$TARGET" ``` +For a stacked PR (`gh pr view --json baseRefName` is not `main`), fetch that +branch and take `BASE` from `git merge-base "$TARGET" origin/` +instead, so the parent PR's hunks are not reviewed again here. + If the change has a PR, read its title and description to learn the intent. Do not read existing review comments before forming your own view. List the hunks the description does not explain: a fix riding along with the main change belongs in its own PR (`AGENTS.md`: one change per PR), so ask for the split. -Check how far the branch is from main with -`git rev-list --count "$BASE"..origin/main` and -`git merge-tree --write-tree "$TARGET" origin/main`. When the base is far -behind, files the change touches may have moved, split or been deleted on -main, and conventions it follows (changelog location, test matrix variables) -may have changed. Those are judged at merge time, not at `TARGET`, so name -the new location in the review body. +Judge the code at `TARGET`. Separately, check what a merge into the PR's +base branch would hit: `git rev-list --count "$BASE"..origin/main` and +`git merge-tree --write-tree "$TARGET" origin/main`, with +`origin/` in place of `origin/main` for a stacked PR, so a +conflict the parent PR has with main is reported once, on the parent. GitHub +flags the conflict too, but not where things went: if files the change +touches were moved, split or deleted there, or a convention it follows +changed (changelog location, test matrix variables), say so in the review +body with the new location, so the author rebases before the next round. +That is the only use of this check; do not re-review against main. Read every changed non-generated file in full at `TARGET`, not just the hunks (`git show "$TARGET":path`). Skim generated files (schemas, codegen output, acceptance test output) only to confirm they follow from the source change; -when the generator is cheap to run, regenerate into a temporary directory and -`diff -r` it against the committed tree, since stale generated files outside -the diff never show up in the hunks. +CI already fails on stale generated output, so do not regenerate locally. Read the repo's agent rules that apply to the changed files, under `.agents/rules/` (testing, comments, language style guides).