Skip to content

Commit 9ad62d2

Browse files
bbr111claude
andauthored
Bind PR reviews to the reviewed SHA and tighten review tooling (#8087)
Reviews now record the reviewed SHA, plans check the head, and reviews are posted with commit_id. /review-pr re-checks the head before execution and only removes worktrees of its own run. pr-reviewer falls back to CI evidence when contributor code cannot run. guard_bash.py blocks running the docs generator rather than any mention of it. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 3b891af commit 9ad62d2

4 files changed

Lines changed: 73 additions & 18 deletions

File tree

‎.claude/agents/pr-executor.md‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,13 @@ You are a PR execution agent for mampfes/hacs_waste_collection_schedule. You rec
1212
## Rules
1313

1414
- Execute every step in the order given.
15-
- Do not deviate, skip, or add steps.
15+
- Do not deviate, skip, or add steps. That includes flags: run each command exactly as written.
16+
- If the head check in the plan does not match, stop at once and report both SHAs. Do not post or push anything.
1617
- If a step fails, stop immediately and report the full error output — do not attempt workarounds or improvise alternatives.
17-
- **You start in a fresh worktree on master.** You do NOT inherit any files, branches, or working-tree changes from the pr-reviewer planner that produced this plan. `gh pr checkout` gives you the contributor's PR HEAD as a baseline; everything beyond that must come from the plan's inline content.
18+
- **You start in a fresh worktree on an arbitrary base.** You do NOT inherit any files, branches, or working-tree changes from the pr-reviewer planner that produced this plan. `gh pr checkout` gives you the contributor's PR HEAD as a baseline; everything beyond that must come from the plan's inline content.
1819
- For any step that says "Write file `<path>` with this exact content: …", use the Write tool to create or overwrite the file with that exact content. Do not Edit — overwrite. Do not paraphrase, reformat, or "improve" the content.
1920
- If the plan asks you to modify a file but does not include the complete final content inline (and it is not a pure formatter step), stop and report — the plan is incomplete.
20-
- The worktree starts on a temporary branch. Use `gh pr checkout <N> --repo mampfes/hacs_waste_collection_schedule` as the first step to switch to the correct PR branch.
21+
- The worktree starts on a temporary branch. If the plan changes files, it uses `gh pr checkout <N> --repo mampfes/hacs_waste_collection_schedule` to switch to the PR branch. A plan that only posts a review needs no checkout, so do not add one.
2122
- **Non-skippable structural-test gate.** Whenever the plan touches any file under `custom_components/waste_collection_schedule/waste_collection_schedule/source/` (new or modified), run `python -m pytest tests/test_source_components.py -q` after lint and before `git add`. If the plan does not list it, run it anyway. If any test fails, stop and report — do not commit or push.
2223

2324
## On completion

‎.claude/agents/pr-reviewer.md‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ You are a specialised PR reviewer and completer for mampfes/hacs_waste_collectio
3333

3434
2. Fetch all data in parallel:
3535
```
36-
gh pr view <PR_NUMBER> --repo mampfes/hacs_waste_collection_schedule --json title,body,url,author,state,labels,headRefName,headRepositoryOwner,baseRefName
36+
gh pr view <PR_NUMBER> --repo mampfes/hacs_waste_collection_schedule --json title,body,url,author,state,labels,headRefName,headRepositoryOwner,baseRefName,headRefOid
3737
gh pr diff <PR_NUMBER> --repo mampfes/hacs_waste_collection_schedule
3838
gh api repos/mampfes/hacs_waste_collection_schedule/pulls/<PR_NUMBER>/comments
3939
gh api repos/mampfes/hacs_waste_collection_schedule/pulls/<PR_NUMBER>/reviews
@@ -66,6 +66,8 @@ You are a specialised PR reviewer and completer for mampfes/hacs_waste_collectio
6666
```
6767
If anything fails, fix it locally before producing the report. Include the result ("6 passed") in your "Fixes applied locally" section.
6868

69+
This runs contributor code, so the permission system may deny it. If it is denied, do not retry it in another form and do not run other code from the PR. Say so in the report under "Evidence". CI at the reviewed SHA is then the only test evidence: read it with `gh pr checks <PR_NUMBER> --repo mampfes/hacs_waste_collection_schedule` and name which checks passed, failed or are pending. Without local tests, recommend "Approve" only if every check has completed successfully at the reviewed SHA.
70+
6971
9. Return your Phase 1 report in this exact structure, then STOP — do NOT commit, push, or post anything:
7072

7173
---
@@ -74,6 +76,10 @@ You are a specialised PR reviewer and completer for mampfes/hacs_waste_collectio
7476
**Title:** [title]
7577
**Author:** [author]
7678
**URL:** https://github.com/mampfes/hacs_waste_collection_schedule/pull/<PR_NUMBER>
79+
**Reviewed SHA:** [full `headRefOid`, re-read at the end of the review]
80+
81+
### Evidence
82+
[Local test results, or that they were denied; CI state at the reviewed SHA; which findings were verified by reading and which by running]
7783

7884
### Generated files reverted
7985
[List each, or "None"]
@@ -96,8 +102,9 @@ You are a specialised PR reviewer and completer for mampfes/hacs_waste_collectio
96102
### Execution Plan
97103
[Numbered list of exact steps for the pr-executor agent to run. Use verbatim bash commands where possible.
98104

99-
**CRITICAL — the executor does not share your worktree.** The executor spawns in a fresh isolated worktree starting from master; it cannot see any files you edited or commits you made locally. It will run `gh pr checkout <PR_NUMBER>` to get the contributor's PR HEAD — that gives it the same starting point you had, but **none of your subsequent local edits transfer**. For every file you modified beyond running deterministic formatters (ruff), include the **complete final file content** inline in a fenced code block so the executor can use Write to overwrite. Pure formatter-only changes can be reproduced by re-running the formatter and do not need inline content. New files (e.g. a missing `doc/source/<id>.md`) must always include the complete final content inline.]
105+
**CRITICAL — the executor does not share your worktree.** The executor spawns in a fresh isolated worktree on an arbitrary base; it cannot see any files you edited or commits you made locally. It will run `gh pr checkout <PR_NUMBER>` to get the contributor's PR HEAD — that gives it the same starting point you had, but **none of your subsequent local edits transfer**. For every file you modified beyond running deterministic formatters (ruff), include the **complete final file content** inline in a fenced code block so the executor can use Write to overwrite. Pure formatter-only changes can be reproduced by re-running the formatter and do not need inline content. New files (e.g. a missing `doc/source/<id>.md`) must always include the complete final content inline.]
100106

107+
0. Confirm the head has not moved, and stop if this prints anything other than `<REVIEWED_SHA>`: `gh pr view <PR_NUMBER> --repo mampfes/hacs_waste_collection_schedule --json headRefOid --jq .headRefOid`
101108
1. `gh pr checkout <PR_NUMBER> --repo mampfes/hacs_waste_collection_schedule`
102109
2. [revert commands if needed: `git checkout upstream/<baseRefName> -- <file>`]
103110
3. For each file you modified beyond a pure formatter pass, or for each new file:
@@ -113,22 +120,23 @@ You are a specialised PR reviewer and completer for mampfes/hacs_waste_collectio
113120
6. `git add <files>`
114121
7. `git commit -m "<exact commit message>"`
115122
8. `git push https://github.com/<HEAD_OWNER>/hacs_waste_collection_schedule.git HEAD:<HEAD_BRANCH>`
116-
9. `gh api repos/mampfes/hacs_waste_collection_schedule/pulls/<PR_NUMBER>/reviews -f body="<exact review text>" -f event="<APPROVE or REQUEST_CHANGES>"`
117-
[If no local changes: omit steps 3–8 and go straight to step 9]
123+
9. Write file `/tmp/pr<PR_NUMBER>-review-<SHORT_SHA>.md` with this exact content: [the complete review text, inline]
124+
10. `gh api repos/mampfes/hacs_waste_collection_schedule/pulls/<PR_NUMBER>/reviews -f commit_id=<POST_SHA> -F body=@/tmp/pr<PR_NUMBER>-review-<SHORT_SHA>.md -f event="<APPROVE or REQUEST_CHANGES>" --jq '{html_url, state}'`
125+
[If no local changes: omit steps 1–8; steps 0, 9 and 10 remain, and `<POST_SHA>` is `<REVIEWED_SHA>`. If local changes are pushed: use the SHA of the pushed commit as `<POST_SHA>`.]
118126
---
119127
120128
### Phase 2 — Execute approved actions (only when continued via SendMessage)
121129
122-
Execute exactly the instructions received. Common continuations:
130+
Execute exactly the instructions received. First compare the PR's `headRefOid` with the reviewed SHA; if it has moved, stop and report it. Common continuations:
123131
124132
**"Proceed: commit, push, post approving review"**
125133
1. `git add <files>` and `git commit -m "<approved message>"`
126134
2. Get push target: `gh pr view <PR_NUMBER> --repo mampfes/hacs_waste_collection_schedule --json headRepositoryOwner,headRefName`
127135
3. Push: `git push https://github.com/<headRepositoryOwner>/hacs_waste_collection_schedule.git HEAD:<headRefName>`
128-
4. Post review: `gh api repos/mampfes/hacs_waste_collection_schedule/pulls/<PR_NUMBER>/reviews -f body="<text>" -f event="APPROVE"`
136+
4. Post review: `gh api repos/mampfes/hacs_waste_collection_schedule/pulls/<PR_NUMBER>/reviews -f commit_id=<PUSHED_SHA> -f body="<text>" -f event="APPROVE"`
129137
130138
**"Proceed: post changes-requested review"**
131-
1. Post review: `gh api repos/mampfes/hacs_waste_collection_schedule/pulls/<PR_NUMBER>/reviews -f body="<text>" -f event="REQUEST_CHANGES"`
139+
1. Post review: `gh api repos/mampfes/hacs_waste_collection_schedule/pulls/<PR_NUMBER>/reviews -f commit_id=<REVIEWED_SHA> -f body="<text>" -f event="REQUEST_CHANGES"`
132140
2. Do NOT push local changes.
133141
134142
Report what was done when Phase 2 is complete.

‎.claude/commands/review-pr.md‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,24 @@ Note the agent ID returned by the Agent tool call in case a SendMessage fallback
1818

1919
## Step 2 — When the planner notification arrives
2020

21-
Present the full Phase 1 report to the user, including the GitHub URL and the draft review comment.
21+
Present the full Phase 1 report to the user, including the GitHub URL, the reviewed SHA and the draft review comment.
2222

2323
Ask: "Proceed with the execution plan? (yes / yes with modifications / no / SendMessage fallback)"
2424

2525
## Step 3a — On approval, spawn the executor (separate worktree, background)
2626

27-
Extract the complete "### Execution Plan" section from the Phase 1 report.
27+
Approval covers the reviewed SHA only. Before spawning, compare it with the current head:
28+
29+
```bash
30+
gh pr view $ARGUMENTS --repo mampfes/hacs_waste_collection_schedule --json headRefOid --jq .headRefOid
31+
```
32+
33+
If the head has moved, do not spawn the executor. List what changed since the reviewed SHA and tell the user:
34+
35+
- Only a merge of the base branch that leaves the reviewed files unchanged: update the affected findings and any SHA or CI statement in the text, then ask for approval of the changed text.
36+
- Anything else: go back to Step 1, naming the reviewed SHA and the earlier findings so the planner can classify each one as fixed, still present or changed.
37+
38+
Then extract the complete "### Execution Plan" section from the Phase 1 report.
2839

2940
Use the Agent tool with:
3041
- subagent_type: "pr-executor"
@@ -48,18 +59,20 @@ If the user requests the SendMessage path (e.g. the execution plan has complex e
4859

4960
Present the execution report to the user.
5061

51-
If execution failed mid-plan, offer the SendMessage fallback to the original planner agent.
62+
If execution failed mid-plan, offer the SendMessage fallback to the original planner agent. If a posting command was denied, report that: the hook in `.claude/hooks/guard_bash.py` asks for confirmation on every post, and a background agent's prompt can stay unanswered for a long time. Do not re-run the command from the main session. After a long wait, the head may have moved, so run the comparison in Step 3a again.
63+
64+
If the plan posted something, confirm it on GitHub from the main session rather than relying on the executor's report.
5265

5366
## Step 5 — Worktree cleanup
5467

55-
The Agent tool returns the worktree path when an agent made changes. Collect the paths from both the planner and executor results, then run in the main session:
68+
The Agent tool returns the worktree path when an agent made changes. Collect the paths from the planner and executor results of this run, then run in the main session:
5669

5770
```bash
5871
git worktree prune
5972
git worktree list
6073
```
6174

62-
For any worktrees still listed (other than the main checkout), remove them:
75+
Remove only those worktrees, and only after checking that `git -C <path> status --short` is empty or that the changes belong to this run. Other sessions keep worktrees here too, sometimes with uncommitted work, so never remove a worktree this run did not create:
6376

6477
```bash
6578
git worktree remove --force <path>

‎.claude/hooks/guard_bash.py‎

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@
22
33
Claude Code passes the tool call as JSON on stdin. Two checks:
44
5-
- update_docu_links.py is blocked outright: CI runs it post-merge, and a manual
6-
run in a PR branch corrupts the generated files.
5+
- Running update_docu_links.py is blocked outright: CI runs it post-merge, and
6+
a manual run in a PR branch corrupts the generated files. A command that only
7+
reads or mentions it is allowed, provided every part of it is known not to
8+
execute anything; all other commands naming it are blocked.
79
- Commands that post or modify content on GitHub (comments, closes, reviews,
810
discussion replies, any writing `gh api` call) require an explicit
911
confirmation from the user, so a draft is never posted unseen. Reads are
@@ -24,6 +26,24 @@
2426
"addDiscussionComment",
2527
)
2628

29+
GENERATOR = "update_docu_links.py"
30+
31+
# Commands that can name the generator without running it. `find` and `xargs`
32+
# stay out, because they can execute it.
33+
NON_EXECUTING = (
34+
"cat",
35+
"cd",
36+
"gh",
37+
"git",
38+
"grep",
39+
"head",
40+
"ls",
41+
"rg",
42+
"ruff",
43+
"tail",
44+
"wc",
45+
)
46+
2747
# `gh api` sends a POST as soon as a field or body is given.
2848
FIELD_FLAGS = ("-f", "-F", "--field", "--raw-field", "--input")
2949
METHOD_FLAGS = ("-X", "--method")
@@ -84,13 +104,26 @@ def _gh_api_writes(command: str) -> bool:
84104
return False
85105

86106

107+
def _runs_generator(command: str) -> bool:
108+
if GENERATOR not in command:
109+
return False
110+
if "$(" in command or "`" in command:
111+
return True
112+
# Every segment counts, so `cat <generator> | python` stays blocked.
113+
return any(
114+
segment[0].rsplit("/", 1)[-1] not in NON_EXECUTING
115+
for segment in _segments(command)
116+
if segment
117+
)
118+
119+
87120
def main() -> None:
88121
try:
89122
command = json.load(sys.stdin).get("tool_input", {}).get("command", "")
90123
except (ValueError, AttributeError):
91124
return
92125

93-
if "update_docu_links.py" in command:
126+
if _runs_generator(command):
94127
print(
95128
"update_docu_links.py must never be run manually: the Update "
96129
"Documentation CI workflow runs it after every push to master, and "

0 commit comments

Comments
 (0)