From 0356f5c4ae51830bf3874c24800f984185853182 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 11 Nov 2026 18:18:38 +0000 Subject: [PATCH 1/3] pr-review skill: judge at TARGET, leave stale generated files to CI Answers two questions on #7466: the main-distance check only tells the author what moved on main so they rebase, it does not move the review to the merge point; and regenerating generated files locally duplicates the deterministic CI check, so drop it. Co-authored-by: Isaac --- .agents/skills/pr-review/SKILL.md | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/.agents/skills/pr-review/SKILL.md b/.agents/skills/pr-review/SKILL.md index b76f0a524da..cb12906d920 100644 --- a/.agents/skills/pr-review/SKILL.md +++ b/.agents/skills/pr-review/SKILL.md @@ -33,20 +33,18 @@ 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 +Judge the code at `TARGET`. Separately, check what a merge would hit: `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. +`git merge-tree --write-tree "$TARGET" origin/main`. If files the change +touches were moved, split or deleted on main, or a convention it follows +changed there (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). From 9c69c4426673ae7be07617c9d0bf7096bec09e5c Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 11 Nov 2026 18:30:28 +0000 Subject: [PATCH 2/3] pr-review skill: take BASE from the stacked PR's base branch With BASE at the merge base with main, a PR stacked on another PR shows the parent's hunks too and they get reviewed twice (#7496 on #7006). Co-authored-by: Isaac --- .agents/skills/pr-review/SKILL.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.agents/skills/pr-review/SKILL.md b/.agents/skills/pr-review/SKILL.md index cb12906d920..64fa89e750b 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,6 +27,10 @@ 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 From 00b7b958497a6469fc664df822bfd55207acc93f Mon Sep 17 00:00:00 2001 From: "andrew.nester" Date: Thu, 12 Nov 2026 10:13:11 +0000 Subject: [PATCH 3/3] pr-review skill: run the merge check against the PR's base branch With the check against origin/main, a conflict the parent PR has with main shows up again in every PR stacked on it. Run it against origin/ for a stacked PR instead, and say what the check is for: GitHub flags the conflict, the review names where the file went. Co-authored-by: Isaac --- .agents/skills/pr-review/SKILL.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/.agents/skills/pr-review/SKILL.md b/.agents/skills/pr-review/SKILL.md index 64fa89e750b..397eead1786 100644 --- a/.agents/skills/pr-review/SKILL.md +++ b/.agents/skills/pr-review/SKILL.md @@ -37,13 +37,16 @@ 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. -Judge the code at `TARGET`. Separately, check what a merge would hit: -`git rev-list --count "$BASE"..origin/main` and -`git merge-tree --write-tree "$TARGET" origin/main`. If files the change -touches were moved, split or deleted on main, or a convention it follows -changed there (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. +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,