diff --git a/.agents/skills/pr-watch/SKILL.md b/.agents/skills/pr-watch/SKILL.md index c39a5254..d54afa71 100644 --- a/.agents/skills/pr-watch/SKILL.md +++ b/.agents/skills/pr-watch/SKILL.md @@ -5,8 +5,14 @@ description: Wait for a GitHub PR's checks to finish before merging or reporting # Waiting on a PR +"Watching a PR" means tracking **both** CI checks **and** GitHub comments/reviews. +A PR is not ready to merge just because checks are green — review feedback must be +read and answered first. + +## CI Checks + ``` -scripts/pr-watch [--expect ] [--timeout ] [--interval ] +scripts/pr-watch [--expect ] [--timeout ] [--interval ] [--reviews] ``` ``` @@ -23,19 +29,54 @@ exit 5 the PR is MERGED or CLOSED, at the head the watch pinned; it ends on the first poll that sees it closed, which may be mid-wait ``` +**`--reviews`** (default): after checks complete, print all reviews and comments +via `scripts/pr-reviews`. Use `--no-reviews` to skip this. The reviews are shown +but do not change the exit code — answering them is your responsibility. + **Exit 0 now means mergeable, not only green.** GitHub computes mergeability lazily and reports `UNKNOWN` for a while after the base moves. It also reports `BLOCKED` for the moment before a required check that `needs:` another job has registered. The watch waits out both, so you do not need to re-check `mergeable` by hand after it. **Exit 3 is not a pass.** It may be a required -check that never ran. Why each -state counts the way it does: `context/code/tooling.md`, "pr-watch and -mergeability" (improvise-edpl). +check that never ran. Why each state counts the way it does: +`context/code/tooling.md`, "pr-watch and mergeability" (improvise-edpl). **Pass `--expect ` whenever you know which commit you are asking about.** It is the difference between "this PR is green" and "the thing I pushed is green", and a PR here was once merged at a head that predated its own fix. +## Reviews and Comments + +Reviews are a **separate API surface** from the comment thread. Codex/ChatGPT +reviews arrive as *reviews*, not comments — checking only the comment thread +misses them entirely. `pr-watch --reviews` (the default) shows all three: + +| Surface | What it is | +|---|---| +| `[REVIEW]` | The review's own body (pulls/{n}/reviews) | +| `[INLINE]` | A comment on a line of the diff (pulls/{n}/comments) | +| `[COMMENT]` | A comment on the PR thread (issues/{n}/comments) | + +To fetch reviews directly: `./scripts/pr-reviews ` — **do not pipe it**. The +script's `$?` must be read directly; piping or chaining loses the exit code. +Redirect to a file if you need to process it. + +### Review Obligations + +Before treating a PR as done or merging: + +1. **Read all review feedback** — reviews against an earlier head may still apply +2. **Answer substantive feedback** on the PR with an identity line naming the + model (the repo owner's account is shared) +3. **Re-check after each push** that new reviews target the current head SHA +4. **Don't wait indefinitely on Copilot/Codex** — reviews arrive as quota allows; + an absent review and an unrequested one look identical. The adversarial review + that gates a merge is dispatched by the orchestrator; external reviews are + read *in addition*, not instead. + +See AGENTS.md "Shipping a wave" and "Read the PR's reviews before merging" for +the full workflow, Copilot request mechanics, and how to answer declined findings. + ## Do not write the loop yourself Four signals look like "finished" and are not: diff --git a/AGENTS.md b/AGENTS.md index e614641a..a207a172 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -68,26 +68,22 @@ See `context/cloud-agents.md` for environment-specific notes (nix, network, etc. ### PR Tracking: Checks AND Reviews For any PR an agent owns or is working on, **track both CI checks and GitHub comments/reviews**. -The `pr-watch` skill handles check status; reviews require separate attention. +The `pr-watch` skill (`.agents/skills/pr-watch/SKILL.md`) is the agent-facing recipe. -1. **Fetch reviews and comments** — reviews are a separate API surface from the comment thread. - Codex/ChatGPT reviews arrive as *reviews* (not comments), so checking only the comment - thread misses them. Use `./scripts/pr-reviews ` to see bodies, inline comments, and - issue comments together. +**`scripts/pr-watch`** now prints reviews after checks are green (use `--no-reviews` to skip). +Reviews are a separate API surface from comments — Codex/ChatGPT reviews arrive as *reviews*, +not comments, and are easy to miss if you only read the comment thread. -2. **Answer substantive review feedback** on the PR before treating the work as done or - merging. Every comment you post must open with an identity line naming the model and - that it is an agent posting (the repo owner's account is shared). +1. **Run `scripts/pr-watch `** — waits for checks, then prints all reviews/comments +2. **Read all review feedback** — reviews against an earlier head may still apply +3. **Answer substantive feedback** with an identity line naming the model before merging +4. **Re-check after each push** that new reviews target the current head SHA +5. **Don't wait indefinitely on Copilot/Codex** — reviews arrive as quota allows -3. **Re-check after each push** that new comments/reviews target the current head SHA. - A review may target an earlier commit; verify rather than assume. +To fetch reviews directly: `./scripts/pr-reviews ` — **do not pipe it** (exit code trap). -4. **Don't wait indefinitely on Copilot/Codex.** Reviews arrive as quota allows; an absent - review and an unrequested one look identical. The adversarial review that gates a merge - is dispatched by the orchestrator. External reviews are read *in addition*, not instead. - -See "Shipping a wave" and "Read the PR's reviews before merging it" below for the full -review workflow, Copilot request mechanics, and how to answer declined findings. +See the `pr-watch` skill for the full recipe, and "Shipping a wave" below for Copilot +request mechanics and how to answer declined findings. ## Quick Reference diff --git a/context/cloud-agents.md b/context/cloud-agents.md index d41bca55..e0ce73c6 100644 --- a/context/cloud-agents.md +++ b/context/cloud-agents.md @@ -170,12 +170,12 @@ environment, where egress leaves through your own network boundary. For any PR you own, **track both CI checks and GitHub comments/reviews**: -- **CI checks**: Use the `pr-watch` skill or `scripts/pr-watch`. -- **Reviews and comments**: Use `./scripts/pr-reviews ` — reviews are a separate API - surface from comments, and Codex/ChatGPT reviews arrive as *reviews* not comments. +- **`scripts/pr-watch `**: Waits for checks, then prints all reviews/comments (default). + Reviews are a separate API surface — Codex/ChatGPT reviews arrive as *reviews* not comments. - **Answer feedback** with an identity line naming the model before marking work done. - **Re-check after push** that reviews target the current head SHA. +The `pr-watch` skill (`.agents/skills/pr-watch/SKILL.md`) is the agent-facing recipe. See AGENTS.md "PR Tracking: Checks AND Reviews" and "Shipping a wave" for the full workflow. --- diff --git a/scripts/pr-watch b/scripts/pr-watch index 67222362..cc5759cf 100755 --- a/scripts/pr-watch +++ b/scripts/pr-watch @@ -5,11 +5,13 @@ set -euo pipefail usage() { cat >&2 <<'EOF' -usage: pr-watch [--expect ] [--timeout ] [--interval ] +usage: pr-watch [--expect ] [--timeout ] [--interval ] [--reviews|--no-reviews] --expect fail if the PR head is not this (prefix ok) at any poll --timeout give up after this long (default 1800) --interval seconds between polls (default 20) + --reviews after checks complete, print reviews/comments (default) + --no-reviews skip the reviews/comments printout exit 0 every check COMPLETED, none failed, and GitHub says MERGEABLE with a state of CLEAN, UNSTABLE or HAS_HOOKS @@ -19,22 +21,31 @@ exit 3 timed out with checks still running, or mergeability still UNKNOWN, or still BLOCKED with every registered check green exit 4 usage or environment error exit 5 the PR is MERGED or CLOSED, so there is nothing left to wait for + +After exit 0, reviews and comments are printed (unless --no-reviews). These do +not change the exit code -- answering them is your responsibility. Reviews are +a separate API surface from comments; Codex/ChatGPT reviews arrive as *reviews* +and are easy to miss if you only read the comment thread. EOF exit 4 } -PR=""; EXPECT=""; TIMEOUT=1800; INTERVAL=20 +PR=""; EXPECT=""; TIMEOUT=1800; INTERVAL=20; SHOW_REVIEWS=1 while [ $# -gt 0 ]; do case "$1" in - --expect) EXPECT="${2:-}"; shift 2 ;; - --timeout) TIMEOUT="${2:-}"; shift 2 ;; - --interval) INTERVAL="${2:-}"; shift 2 ;; - -h|--help) usage ;; - *) [ -z "$PR" ] || usage; PR="$1"; shift ;; + --expect) EXPECT="${2:-}"; shift 2 ;; + --timeout) TIMEOUT="${2:-}"; shift 2 ;; + --interval) INTERVAL="${2:-}"; shift 2 ;; + --reviews) SHOW_REVIEWS=1; shift ;; + --no-reviews) SHOW_REVIEWS=0; shift ;; + -h|--help) usage ;; + *) [ -z "$PR" ] || usage; PR="$1"; shift ;; esac done [ -n "$PR" ] || usage +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + # `gh` is shell-aliased to a different tool in this repo's shells, so the name # is never trusted: a real gh is located explicitly. fetch() { @@ -130,4 +141,26 @@ echo "pr-watch: head=$head $mergeable/$state $total checks, $failed failed" # An allowlist, so a state GitHub adds later fails closed. DIRTY and BLOCKED # can wear a MERGEABLE flag: context/code/tooling.md, "pr-watch and mergeability". [ "$mergeable" = MERGEABLE ] || exit 1 -case "$state" in CLEAN|UNSTABLE|HAS_HOOKS) exit 0 ;; *) exit 1 ;; esac +case "$state" in CLEAN|UNSTABLE|HAS_HOOKS) ;; *) exit 1 ;; esac + +# ── Reviews and comments ─────────────────────────────────────────────────── +# Checks are green and mergeable. Show reviews/comments so the agent sees any +# feedback that needs answering before merge. This does not change the exit +# code -- answering reviews is your responsibility. +if [ "$SHOW_REVIEWS" -eq 1 ]; then + echo "" + echo "pr-watch: fetching reviews and comments (--no-reviews to skip)…" + # pr-reviews' exit code is not propagated: reviews being present or absent + # does not change whether the PR is green/mergeable. The agent must read + # and answer them, but that is a human-in-the-loop obligation, not a gate. + # Fixture mode: pass through the fixture dir if set. + if [ -n "${PR_WATCH_FIXTURE_DIR:-}" ]; then + PR_REVIEWS_FIXTURE_DIR="$PR_WATCH_FIXTURE_DIR" "$SCRIPT_DIR/pr-reviews" "$PR" || true + else + "$SCRIPT_DIR/pr-reviews" "$PR" || true + fi + echo "" + echo "pr-watch: reviews shown above. Answer substantive feedback before merging." +fi + +exit 0 diff --git a/scripts/pr-watch-test.sh b/scripts/pr-watch-test.sh index 59dd6531..3e64a714 100755 --- a/scripts/pr-watch-test.sh +++ b/scripts/pr-watch-test.sh @@ -20,8 +20,39 @@ run_case() { [ "$got" = "$want" ] && ok "$name" || bad "$name" "$got" "$want" } +# Like run_case but captures stdout for content checks +run_case_capture() { + local name="$1" want="$2" dir="$3"; shift 3 + local out="$work/output.txt" + PR_WATCH_FIXTURE_DIR="$dir" "$WATCH" 1 --interval 0 --timeout 1 "$@" >"$out" 2>&1 + local got=$? + [ "$got" = "$want" ] && ok "$name" || bad "$name" "$got" "$want" + cat "$out" +} + mk() { mkdir -p "$1"; cat > "$1/$2"; } +# Create empty pr-reviews fixtures so pr-watch can call pr-reviews in fixture mode. +# $1 dir, $2 head (must match the pr-watch fixture's headRefOid) +mk_reviews_empty() { + local dir="$1" head="$2" + mkdir -p "$dir" + printf '{"headRefOid":"%s","reviews":[]}\n' "$head" > "$dir/last.pr.json" + echo '[]' > "$dir/last.inline.json" + echo '[]' > "$dir/last.issue.json" +} + +# Create pr-reviews fixtures with a review. $1 dir, $2 head, $3 review commit +mk_reviews_with_review() { + local dir="$1" head="$2" review_commit="$3" + mkdir -p "$dir" + cat > "$dir/last.pr.json" < "$dir/last.inline.json" + echo '[]' > "$dir/last.issue.json" +} + # The state a fresh push is in: the PR exists, no check has registered yet. # A watcher keyed on "nothing pending" calls this finished and green. mk "$work/none" last.json <<'EOF' @@ -43,8 +74,31 @@ mk "$work/green" last.json <<'EOF' {"name":"build","status":"COMPLETED","conclusion":"SUCCESS"}, {"name":"skipped","status":"COMPLETED","conclusion":"SKIPPED"}]} EOF +mk_reviews_empty "$work/green" "aaaaaaaaaaaa" run_case "all completed and green" 0 "$work/green" +# ── Reviews integration ──────────────────────────────────────────────────── +# pr-watch now calls pr-reviews after checks are green. Test that it works. +mk "$work/green-reviews" last.json <<'EOF' +{"headRefOid":"aaaaaaaaaaaa","mergeable":"MERGEABLE","mergeStateStatus":"CLEAN","statusCheckRollup":[ + {"name":"build","status":"COMPLETED","conclusion":"SUCCESS"}]} +EOF +mk_reviews_with_review "$work/green-reviews" "aaaaaaaaaaaa" "aaaaaaaaaaaa" +out=$(run_case_capture "green with reviews shows review output" 0 "$work/green-reviews") +if echo "$out" | grep -q "pr-reviews:"; then + ok "green with reviews prints pr-reviews output" +else + bad "green with reviews prints pr-reviews output" "no pr-reviews output" "pr-reviews: in output" +fi + +# --no-reviews skips the review printout +out=$(run_case_capture "--no-reviews skips review output" 0 "$work/green-reviews" --no-reviews) +if echo "$out" | grep -q "pr-reviews:"; then + bad "--no-reviews skips review output" "pr-reviews in output" "no pr-reviews" +else + ok "--no-reviews actually skips review output" +fi + mk "$work/red" last.json <<'EOF' {"headRefOid":"aaaaaaaaaaaa","mergeable":"MERGEABLE","mergeStateStatus":"UNSTABLE","statusCheckRollup":[ {"name":"build","status":"COMPLETED","conclusion":"FAILURE"}]} @@ -112,6 +166,10 @@ for m in MERGEABLE CONFLICTING UNKNOWN; do fi sv="\"$s\""; [ "$s" = null ] && sv=null state "$work/table-$m-$s" last.json "\"$m\"" "$sv" "$green_rollup" + # Add review fixtures for cases that exit 0 (pr-watch calls pr-reviews) + if [ "$want" = 0 ]; then + mk_reviews_empty "$work/table-$m-$s" "aaaaaaaaaaaa" + fi run_case "table: $m/$s" "$want" "$work/table-$m-$s" --timeout 0 done done @@ -148,6 +206,7 @@ gap_state "$work/gap" 1.json MERGEABLE BLOCKED "$not_yet | $build_running" gap_state "$work/gap" 2.json MERGEABLE BLOCKED "$not_yet" gap_state "$work/gap" 3.json MERGEABLE BLOCKED "$harness_running" gap_state "$work/gap" last.json MERGEABLE CLEAN '.' +mk_reviews_empty "$work/gap" "aaaaaaaaaaaa" run_case "the needs-gap before the required check registers is not a failure" 0 "$work/gap" --timeout 60 gap_state "$work/gap-never" last.json MERGEABLE BLOCKED "$not_yet"