feat(pr-watch): integrate reviews/comments into pr-watch
The pr-watch skill and script now cover reviews/comments, not just CI checks. An agent reading only the pr-watch skill now knows to track both. .agents/skills/pr-watch/SKILL.md: - Reframed: 'watching a PR' means checks AND reviews - Added 'Reviews and Comments' section with the three surfaces table - Added 'Review Obligations' subsection (answer feedback, identity line, etc.) - Documented --reviews (default) and --no-reviews flags - Cross-references AGENTS.md for full workflow details scripts/pr-watch: - Added --reviews (default) and --no-reviews flags - After exit 0 (green/mergeable), calls scripts/pr-reviews to show all reviews and comments. Does not change exit code — answering is agent's job. - Updated usage text to document the new behavior - Composes with existing pr-reviews (fixture mode passes through) scripts/pr-watch-test.sh: - Added mk_reviews_empty() and mk_reviews_with_review() helpers - Added review fixtures to all cases that exit 0 (pr-reviews needs them) - Added tests: 'green with reviews shows review output' and '--no-reviews skips review output' - All 56 tests pass AGENTS.md / context/cloud-agents.md: - Updated cross-references to point to pr-watch skill as agent-facing recipe - Simplified text now that pr-watch handles reviews automatically Co-authored-by: Ed Langley <fiddlerwoaroof@users.noreply.github.com>
This commit is contained in:
Cursor Agent
+41
-8
@@ -5,11 +5,13 @@ set -euo pipefail
|
||||
|
||||
usage() {
|
||||
cat >&2 <<'EOF'
|
||||
usage: pr-watch <pr-number> [--expect <sha>] [--timeout <sec>] [--interval <sec>]
|
||||
usage: pr-watch <pr-number> [--expect <sha>] [--timeout <sec>] [--interval <sec>] [--reviews|--no-reviews]
|
||||
|
||||
--expect <sha> fail if the PR head is not this (prefix ok) at any poll
|
||||
--timeout <sec> give up after this long (default 1800)
|
||||
--interval <sec> 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
|
||||
|
||||
@@ -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" <<EOF
|
||||
{"headRefOid":"$head","reviews":[{"submittedAt":"2026-09-25T12:00:00Z","state":"COMMENTED","author":{"login":"copilot"},"commit":{"oid":"$review_commit"},"body":"Test review body"}]}
|
||||
EOF
|
||||
echo '[]' > "$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"
|
||||
|
||||
Reference in New Issue
Block a user