diff --git a/github/hooks/closeout-stop.sh b/github/hooks/closeout-stop.sh index cc9b2d03..1cbd6c6a 100755 --- a/github/hooks/closeout-stop.sh +++ b/github/hooks/closeout-stop.sh @@ -8,16 +8,21 @@ # by a hallucinated or premature stop. The skill arms a state file # (.git/review-pr-closeout.json) the moment Phase 4's stop conditions hold and # clears it once the user's merge choice is in (or the --auto-merge closeout ran -# or aborted). While the file exists, this hook blocks turn-end with a message -# naming the PR and the missing step — the ask cannot silently vanish. +# or aborted). While the file exists, this hook blocks one turn-end per user +# turn with a message naming the PR and the missing step — the ask cannot +# silently vanish, and it does not loop: after the single block the turn ends. # # Guard rails (this hook fires on EVERY turn end — Stop has no matcher): # - Fails open: no jq, no git repo, no state file, or a subagent stop # (agent_id present in the input — a subagent cannot ask the user) all pass. +# - Fires at most once per user turn: stop_hook_active is true when the turn +# is already continuing from a stop-hook block — pass through so the turn +# can end. The single block injects the reminder once; resolving the closeout +# (the merge ask or clear-closeout) is what removes the state file. # - Blocks via hookSpecificOutput.additionalContext (exit 0 + JSON): the # message is fed back to the model as "Stop hook feedback" and the turn -# continues — same loop protections (stop_hook_active, 8-block cap) as a -# decision:block, without the hook-error label. +# continues — same loop protections as a decision:block, without the +# hook-error label. # # Input (stdin JSON): Stop common fields + stop_hook_active, # last_assistant_message, background_tasks, session_crons. @@ -35,6 +40,15 @@ command -v jq >/dev/null 2>&1 || exit 0 agent_id=$(printf '%s' "$input" | jq -r '.agent_id // empty') [ -z "$agent_id" ] || exit 0 +# At most one reminder per user turn. stop_hook_active is true when the turn is +# already continuing from a stop-hook block — passing through lets the turn end +# instead of re-blocking every end attempt. The reminder was already injected +# once; resolving the closeout (the merge ask, or clear-closeout) is what clears +# the state file on its own schedule. Official anti-loop pattern per +# https://code.claude.com/docs/en/hooks-guide.md. +stop_hook_active=$(printf '%s' "$input" | jq -r '.stop_hook_active // false') +[ "$stop_hook_active" = "true" ] && exit 0 + # The state file lives in the repo's gitdir; resolve it from the hook cwd # (the session's working directory). Outside a git repo there is no closeout. GITDIR=$(git rev-parse --git-dir 2>/dev/null) || exit 0 @@ -64,12 +78,5 @@ else fi msg="${msg} Full verification procedure: ${PLUGIN_ROOT}/skills/review-pr/references/closeout.md (When the hook fires)." -# Repeated blocks mean the condition is not resolving (e.g. the user declined -# the required action) — escalate to the last-resort escape hatch. -stop_hook_active=$(printf '%s' "$input" | jq -r '.stop_hook_active // false') -if [ "$stop_hook_active" = "true" ]; then - msg="${msg} You have been blocked repeatedly: if this closeout cannot be resolved, tell the user they can delete the repo's .git/review-pr-closeout.json to release it." -fi - jq -n --arg c "$msg" '{hookSpecificOutput:{hookEventName:"Stop",additionalContext:$c}}' exit 0 diff --git a/github/skills/create-pr/SKILL.md b/github/skills/create-pr/SKILL.md index 1644680a..409e88ff 100644 --- a/github/skills/create-pr/SKILL.md +++ b/github/skills/create-pr/SKILL.md @@ -78,7 +78,7 @@ See `references/repository-templates.md` for template detection and compliance d **Goal**: Delegate CI monitoring and reviewer-comment triage to the dedicated skill. -**Action**: After the PR is created, invoke `Skill("github:review-pr", "")` to run the baseline review and launch the persistent CI + comment watch. The review-pr skill owns the Monitor script, the skeptical triage agent, the review → fix → commit+push → wait-for-review loop, through to the merge decision and the post-merge branch hygiene (remote + local head cleanup, `fetch --prune`, fast-forward `main`/`develop`). **Once CI is green and every comment is triaged, review-pr asks the user whether to merge via `AskUserQuestion` (merge commit/squash/rebase/don't) BEFORE its closeout ceremony** — the summary comment and body rewrite run only on a merge choice. The merge ask is enforced by the plugin's Stop hook: review-pr arms a closeout state the moment the stop conditions hold and turn-end stays blocked until the decision resolves (bounded by the platform's 8-consecutive-block cap) — the handoff cannot silently skip the ask. See `references/pr-creation-handoff.md` for the handoff contract including post-merge hygiene. This skill does not duplicate that cleanup; it is the handoff target's responsibility. +**Action**: After the PR is created, invoke `Skill("github:review-pr", "")` to run the baseline review and launch the persistent CI + comment watch. The review-pr skill owns the Monitor script, the skeptical triage agent, the review → fix → commit+push → wait-for-review loop, through to the merge decision and the post-merge branch hygiene (remote + local head cleanup, `fetch --prune`, fast-forward `main`/`develop`). **Once CI is green and every comment is triaged, review-pr asks the user whether to merge via `AskUserQuestion` (merge commit/squash/rebase/don't) BEFORE its closeout ceremony** — the summary comment and body rewrite run only on a merge choice. The merge ask is enforced by the plugin's Stop hook: review-pr arms a closeout state the moment the stop conditions hold and one turn-end per user turn is blocked until the decision resolves — the handoff cannot silently skip the ask. See `references/pr-creation-handoff.md` for the handoff contract including post-merge hygiene. This skill does not duplicate that cleanup; it is the handoff target's responsibility. **`--auto-merge` passthrough**: If `$ARGUMENTS` carried `--auto-merge`, pass it through to the review-pr invocation as `Skill("github:review-pr", " --auto-merge")`. It instructs review-pr to skip the merge `AskUserQuestion` — the closeout ceremony (summary comment + body rewrite) still runs first — and auto-merge with `gh pr merge --merge` once CI is green and every non-escalate comment is triaged — see `references/pr-creation-handoff.md` for the contract and the escalate fallback. Pass it through **only** when the user explicitly set it; never infer it. diff --git a/github/skills/review-pr/SKILL.md b/github/skills/review-pr/SKILL.md index 75f1fc78..2de109be 100644 --- a/github/skills/review-pr/SKILL.md +++ b/github/skills/review-pr/SKILL.md @@ -60,7 +60,7 @@ Stop the Monitor with `TaskStop` when EITHER holds — full conditions in `refer **Goal**: Once Phase 4 holds, ask the user whether to merge FIRST — before any ceremony. The summary comment and body rewrite run only on a merge choice; "Don't merge" skips the ceremony and goes straight to `TaskStop`. On a merge choice, post-merge hygiene (linked-worktree removal, switch to `main`, sync with origin) runs unconditionally. Full templates and ordered steps in `references/closeout.md`. **CRITICAL constraints (hold even when detail is delegated to L3)**: -1. **Arm the closeout state the moment Phase 4 holds — before anything else**: `bash ${CLAUDE_PLUGIN_ROOT}/skills/review-pr/scripts/arm-closeout.sh "$PR"` (append `--auto-merge` when the opt-in was parsed in Phase 1). This writes the repo's `.git/review-pr-closeout.json`, arming the plugin's Stop hook: while the file exists, every turn end is blocked with a message naming the missing merge decision — the Phase 5 ask cannot be skipped by a premature stop (bounded by Claude Code's 8-consecutive-block cap per turn; a user interrupt also bypasses it). **When the hook blocks, first verify the pending closeout is real** — a stale state file (ask already answered, summary posted, or PR merged without clearing) is a false alarm: judge simple checks directly (`gh pr view --json state,mergedAt`, the `` marker lookup), spawn an independent subagent with clean context for complex or ambiguous situations — see `references/closeout.md` (When the hook fires). A verified-stale state is cleared, not re-asked. Clear it the moment the decision is resolved — `bash ${CLAUDE_PLUGIN_ROOT}/skills/review-pr/scripts/clear-closeout.sh "$PR"`: after the user answers (any choice, including "Don't merge"), after the auto-merge completes, or after the opt-in aborts. A stale file blocks the next stop; its message repeats the clear path. +1. **Arm the closeout state the moment Phase 4 holds — before anything else**: `bash ${CLAUDE_PLUGIN_ROOT}/skills/review-pr/scripts/arm-closeout.sh "$PR"` (append `--auto-merge` when the opt-in was parsed in Phase 1). This writes the repo's `.git/review-pr-closeout.json`, arming the plugin's Stop hook: while the file exists, one turn-end per user turn is blocked with a message naming the missing merge decision — the Phase 5 ask cannot be skipped by a premature stop, and the hook passes through (`stop_hook_active`) on later end-attempts of the same turn so it does not loop (a user interrupt also bypasses it). **When the hook blocks, first verify the pending closeout is real** — a stale state file (ask already answered, summary posted, or PR merged without clearing) is a false alarm: judge simple checks directly (`gh pr view --json state,mergedAt`, the `` marker lookup), spawn an independent subagent with clean context for complex or ambiguous situations — see `references/closeout.md` (When the hook fires). A verified-stale state is cleared, not re-asked. Clear it the moment the decision is resolved — `bash ${CLAUDE_PLUGIN_ROOT}/skills/review-pr/scripts/clear-closeout.sh "$PR"`: after the user answers (any choice, including "Don't merge"), after the auto-merge completes, or after the opt-in aborts. A stale file blocks the next stop; its message repeats the clear path. 2. **Ask the merge question the moment Phase 4 holds.** Do not post the summary or rewrite the body first — the user's choice gates the ceremony. When everything is settled, the closeout ask is the next step, not more polling. 3. The ceremony runs ONLY on a merge choice: capture the summary comment URL from `gh pr comment` stdout (`SUMMARY_URL=$(gh pr comment …)`). 4. The Review-cycle line in the rewritten body MUST contain that literal URL — a count with no link is not a pointer, and the quoted heredoc will not expand `$SUMMARY_URL`, so paste it. diff --git a/github/skills/review-pr/references/closeout.md b/github/skills/review-pr/references/closeout.md index 22022a3f..5d4e913f 100644 --- a/github/skills/review-pr/references/closeout.md +++ b/github/skills/review-pr/references/closeout.md @@ -28,11 +28,12 @@ bash ${CLAUDE_PLUGIN_ROOT}/skills/review-pr/scripts/arm-closeout.sh "$PR" --auto Arming writes `.git/review-pr-closeout.json` (resolved via `git rev-parse --git-dir`, so it works from any cwd in the repo). While the file exists, the plugin's Stop hook -(`hooks/closeout-stop.sh`) blocks every turn end with a message naming the PR and the missing -step — the merge ask cannot be skipped by a premature or hallucinated stop. This is the -enforcement net for the whole closeout: turn-end stays blocked until the decision resolves -(bounded by Claude Code's 8-consecutive-block cap per turn; a user interrupt also bypasses -it — the hook's message always names the clear script as the escape hatch). +(`hooks/closeout-stop.sh`) blocks one turn-end per user turn with a message naming the PR and +the missing step — the merge ask cannot be skipped by a premature or hallucinated stop, and +the reminder does not loop: the hook passes through (`stop_hook_active`) on the second +end-attempt of the same turn, so the turn ends once the reminder is injected. A user +interrupt also bypasses it — the hook's message always names the clear script as the escape +hatch. Clear it the moment the decision is resolved: @@ -266,8 +267,8 @@ stop — do not retry with different flags, do not force-push, and do not re-arm the user decides next. If the user interrupts and you resume, and the gate no longer holds (a new comment arrived, CI re-ran red), do not auto-merge — re-check the gate, and if escalate items now exist, fall back to the explicit question. Either way, clear the closeout state -once the opt-in is resolved (completed or aborted) — while it stays armed, every turn end is -blocked until the decision is settled. +once the opt-in is resolved (completed or aborted) — while it stays armed, one turn-end per +user turn is blocked until the decision is settled. **On a successful auto-merge**, proceed to "After a successful merge" hygiene exactly as the explicit-choice path would. `TaskStop` the Monitor. @@ -379,16 +380,16 @@ holding). If the user interrupts and you resume, skip steps already completed; i either (the ask gates it). Under `--auto-merge`, if the gate no longer holds on resume (new comment or CI re-ran red), do not auto-merge — re-check and fall back to the explicit question if escalate items now exist. If an interrupt left the closeout state armed, clear -it when the closeout is resolved (ask answered or opt-in consumed); while armed, every -turn end is blocked until then. +it when the closeout is resolved (ask answered or opt-in consumed); while armed, one turn-end +per user turn is blocked until then. ## Do not - Do not ask to merge or post the summary while comments are still open or CI is still red — the gate must hold first, and the summary would claim a merge-ready state that is not true. - Do not end the turn with the closeout state armed — the Stop hook blocks it. Clear it - (`clear-closeout.sh "$PR"`) as soon as the decision resolves; while it stays armed, every - turn end is blocked with a message naming the PR and the missing step. + (`clear-closeout.sh "$PR"`) as soon as the decision resolves; while it stays armed, one + turn-end per user turn is blocked with a message naming the PR and the missing step. - Do not run the ceremony (summary comment + body rewrite) before the user's merge choice — the ask comes first, and "Don't merge" skips the ceremony entirely. - Do not rewrite the title/body to claim something the diff does not deliver.