fix: address three Codex P2 review findings
1. Shebang (Codex P2): scripts/agent-bootstrap.sh and .claude/hooks/session-start.sh now use #!/usr/bin/env bash instead of #!/bin/bash, matching repo convention enforced by checkers.sh. 2. Fail-closed bd pin (Codex P2): .claude/hooks/session-start.sh local path now exits 2 with a warning when pinned or running version cannot be parsed, instead of silently succeeding. An unparseable version is not a pass. 3. pr-reviews failure (Codex P2): scripts/pr-watch no longer uses '|| true' to swallow pr-reviews failures. If pr-reviews fails (fetch error, malformed response), pr-watch now exits 6 with a warning. Empty successful fetch (no reviews) is still exit 0. - Added exit 6 to usage text and skill documentation - Added test case 'pr-reviews failure exits 6' using malformed JSON fixture - All 57 pr-watch tests pass Co-authored-by: Ed Langley <fiddlerwoaroof@users.noreply.github.com>
This commit is contained in:
@@ -16,8 +16,8 @@ scripts/pr-watch <pr-number> [--expect <sha>] [--timeout <sec>] [--interval <sec
|
||||
```
|
||||
|
||||
```
|
||||
exit 0 every check COMPLETED, none failed, and MERGEABLE with a state of
|
||||
CLEAN, UNSTABLE or HAS_HOOKS
|
||||
exit 0 every check COMPLETED, none failed, MERGEABLE with a state of
|
||||
CLEAN, UNSTABLE or HAS_HOOKS, and reviews fetched (if --reviews)
|
||||
exit 1 a check failed (under any mergeability, BLOCKED included), or the
|
||||
PR is not mergeable (CONFLICTING, DIRTY, BEHIND, DRAFT, or a state
|
||||
it does not know)
|
||||
@@ -27,6 +27,7 @@ exit 3 timed out with checks still running, mergeability still UNKNOWN,
|
||||
exit 4 usage or environment error
|
||||
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
|
||||
exit 6 checks green/mergeable, but pr-reviews failed (fetch error, not "no reviews")
|
||||
```
|
||||
|
||||
**`--reviews`** (default): after checks complete, print all reviews and comments
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
#!/bin/bash
|
||||
#!/usr/bin/env bash
|
||||
# Prepare a Claude Code on the web container for work on this repository.
|
||||
#
|
||||
# This hook delegates to the shared `scripts/agent-bootstrap.sh`, which works
|
||||
@@ -33,7 +33,19 @@ if [ "${CLAUDE_CODE_REMOTE:-}" != "true" ]; then
|
||||
if command -v bd >/dev/null 2>&1; then
|
||||
pinned=$(beads_pin)
|
||||
running=$(bd_running_version)
|
||||
if [ -n "$pinned" ] && [ -n "$running" ] && [ "$running" != "$pinned" ]; then
|
||||
# Fail closed: if we can't parse either version, warn rather than silently pass
|
||||
if [ -z "$pinned" ]; then
|
||||
printf '[session-start] WARNING: cannot parse beads pin from flake.nix\n' >&2
|
||||
printf '[session-start] The pin check is skipped, but mutating bd commands may be unsafe.\n' >&2
|
||||
exit 2
|
||||
fi
|
||||
if [ -z "$running" ]; then
|
||||
printf '[session-start] WARNING: cannot parse bd version from "bd version"\n' >&2
|
||||
printf '[session-start] bd: %s\n' "$(command -v bd)" >&2
|
||||
printf '[session-start] The pin check is skipped, but mutating bd commands may be unsafe.\n' >&2
|
||||
exit 2
|
||||
fi
|
||||
if [ "$running" != "$pinned" ]; then
|
||||
printf '\n[session-start] ===== BEADS VERSION SKEW =====\n' >&2
|
||||
printf '[session-start] pinned (flake.nix): %s\n' "$pinned" >&2
|
||||
printf '[session-start] running: %s (%s)\n' "$running" "$(command -v bd)" >&2
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
#!/bin/bash
|
||||
#!/usr/bin/env bash
|
||||
# Stack-agnostic agent bootstrap for this repository.
|
||||
#
|
||||
# This script prepares any coding agent (Claude Code, Cursor, etc.) to work
|
||||
|
||||
+14
-10
@@ -14,16 +14,16 @@ usage: pr-watch <pr-number> [--expect <sha>] [--timeout <sec>] [--interval <sec>
|
||||
--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
|
||||
state of CLEAN, UNSTABLE or HAS_HOOKS (and reviews fetched if --reviews)
|
||||
exit 1 a check failed, or the PR is not mergeable (any other known state)
|
||||
exit 2 the head moved during the wait, or is not --expect
|
||||
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
|
||||
exit 6 checks green/mergeable, but pr-reviews failed (fetch error, not "no reviews")
|
||||
|
||||
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
|
||||
After exit 0, reviews and comments are printed (unless --no-reviews). 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
|
||||
@@ -146,21 +146,25 @@ 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.
|
||||
# code for success -- answering reviews is your responsibility. However, if
|
||||
# pr-reviews itself fails (usage error, network, etc.), we warn and exit 6.
|
||||
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.
|
||||
reviews_exit=0
|
||||
if [ -n "${PR_WATCH_FIXTURE_DIR:-}" ]; then
|
||||
PR_REVIEWS_FIXTURE_DIR="$PR_WATCH_FIXTURE_DIR" "$SCRIPT_DIR/pr-reviews" "$PR" || true
|
||||
PR_REVIEWS_FIXTURE_DIR="$PR_WATCH_FIXTURE_DIR" "$SCRIPT_DIR/pr-reviews" "$PR" || reviews_exit=$?
|
||||
else
|
||||
"$SCRIPT_DIR/pr-reviews" "$PR" || true
|
||||
"$SCRIPT_DIR/pr-reviews" "$PR" || reviews_exit=$?
|
||||
fi
|
||||
echo ""
|
||||
if [ "$reviews_exit" -eq 0 ]; then
|
||||
echo "pr-watch: reviews shown above. Answer substantive feedback before merging."
|
||||
else
|
||||
echo "pr-watch: WARNING: pr-reviews failed (exit $reviews_exit). Reviews may not be shown." >&2
|
||||
echo "pr-watch: Run './scripts/pr-reviews $PR' manually to fetch reviews." >&2
|
||||
exit 6
|
||||
fi
|
||||
fi
|
||||
|
||||
exit 0
|
||||
|
||||
@@ -99,6 +99,18 @@ else
|
||||
ok "--no-reviews actually skips review output"
|
||||
fi
|
||||
|
||||
# pr-reviews failure: if pr-reviews fails, pr-watch exits 6 (not 0)
|
||||
# Create a green fixture with MALFORMED review fixtures so pr-reviews fails
|
||||
mk "$work/green-bad-reviews" last.json <<'EOF'
|
||||
{"headRefOid":"aaaaaaaaaaaa","mergeable":"MERGEABLE","mergeStateStatus":"CLEAN","statusCheckRollup":[
|
||||
{"name":"build","status":"COMPLETED","conclusion":"SUCCESS"}]}
|
||||
EOF
|
||||
# Create malformed JSON that will cause jq to fail in pr-reviews
|
||||
echo 'NOT VALID JSON' > "$work/green-bad-reviews/last.pr.json"
|
||||
echo '[]' > "$work/green-bad-reviews/last.inline.json"
|
||||
echo '[]' > "$work/green-bad-reviews/last.issue.json"
|
||||
run_case "pr-reviews failure exits 6" 6 "$work/green-bad-reviews"
|
||||
|
||||
mk "$work/red" last.json <<'EOF'
|
||||
{"headRefOid":"aaaaaaaaaaaa","mergeable":"MERGEABLE","mergeStateStatus":"UNSTABLE","statusCheckRollup":[
|
||||
{"name":"build","status":"COMPLETED","conclusion":"FAILURE"}]}
|
||||
|
||||
Reference in New Issue
Block a user