From 0c21c2c12ded68c94b3cdf0871a344369cd73835 Mon Sep 17 00:00:00 2001 From: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Date: Sat, 6 Jun 2026 21:48:05 -0500 Subject: [PATCH 01/17] fix(dev-lead): hold auto-merge while working a PR + push defensively (#452) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dev-lead enables auto-merge (squash) by default (#447). GitHub's auto-merge is asynchronous and not gated by the workflow concurrency lane, so an approval landing while the agent is mid-edit can merge — and delete — the branch out from under the run, which then fails on push: ! [remote rejected] ... cannot lock ref ... unable to resolve reference (run 27078382804: a review-changes run committed 1s after PR #449 auto-merged). Per-PR serialization (#450) stops two dev-lead runs fighting, but can't stop GitHub's async merge. So: - scripts/lib/auto-merge.sh: hold_auto_merge disables auto-merge at the start of a modifying run; restore_auto_merge (EXIT trap) restores it on any exit, but only if this run disabled it — never newly enables it. push_with_merge_guard treats a push that fails because the PR merged/closed mid-run as a benign no-op (exit 0) instead of a red-X failure. - fix-reviews.sh / fix-ci.sh: hold auto-merge + register the restore trap before checkout, and push via push_with_merge_guard. pr-worktree.sh now chains its cleanup onto the caller's EXIT trap instead of clobbering it. - test_concurrency_config.sh: was stale (asserted the pre-#450 repo-wide group, failing against main); now asserts the per-PR/per-issue serialization lanes + cancel-in-progress:false on both the inline and reusable workflows. Tests: 284 unit tests pass (new test_auto_merge.bats; trap-chaining case added to test_pr_worktree.bats); concurrency + prompt-coverage integration pass; shellcheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/dev-lead-fix-ci.sh | 12 +- scripts/dev-lead-fix-reviews.sh | 14 +- scripts/lib/auto-merge.sh | 97 ++++++++++++ scripts/lib/pr-worktree.sh | 11 +- .../integration/test_concurrency_config.sh | 130 ++++------------ tests/dev-lead/unit/test_auto_merge.bats | 146 ++++++++++++++++++ tests/dev-lead/unit/test_pr_worktree.bats | 18 +++ 7 files changed, 321 insertions(+), 107 deletions(-) create mode 100644 scripts/lib/auto-merge.sh create mode 100644 tests/dev-lead/unit/test_auto_merge.bats diff --git a/scripts/dev-lead-fix-ci.sh b/scripts/dev-lead-fix-ci.sh index 55ee53da5..117ea1698 100755 --- a/scripts/dev-lead-fix-ci.sh +++ b/scripts/dev-lead-fix-ci.sh @@ -10,6 +10,7 @@ set -euo pipefail source "$(dirname "$0")/engine.sh" source "$(dirname "$0")/lib/git-identity.sh" source "$(dirname "$0")/lib/pr-worktree.sh" +source "$(dirname "$0")/lib/auto-merge.sh" PR_NUMBER="${PR_NUMBER:-}" HEAD_SHA="${HEAD_SHA:-}" @@ -206,6 +207,13 @@ main() { exit 0 fi + # Hold auto-merge OFF while we work so a review approval landing mid-run can't + # merge (and delete) the branch out from under us. restore_auto_merge (EXIT + # trap) puts it back however we exit; checkout_pr_in_worktree chains its own + # cleanup onto this trap. + trap restore_auto_merge EXIT + hold_auto_merge + # Checkout the PR branch for modification, in an isolated worktree so the # branch switch never overwrites the agent's own prompts/scripts (issue #448). checkout_pr_in_worktree "$PR_NUMBER" "$REPO" @@ -268,7 +276,9 @@ main() { git add -A git commit -m "fix(ci): auto-fix for $(echo "$CHECKS_JSON" | jq -r '.[0].name // "CI failure"') [skip ci-relay]" fi - git push + # push_with_merge_guard exits 0 cleanly if the PR was merged/closed mid-run + # (its branch deleted); a genuine push failure still aborts with exit 1. + push_with_merge_guard || exit 1 post_summary "applied" "Fix committed and pushed. Waiting for CI." break diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 4073cba26..1409725a9 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -6,6 +6,7 @@ set -euo pipefail source "$(dirname "$0")/engine.sh" source "$(dirname "$0")/lib/git-identity.sh" source "$(dirname "$0")/lib/pr-worktree.sh" +source "$(dirname "$0")/lib/auto-merge.sh" INTENT_TYPE="${INTENT_TYPE:-fix-reviews}" PR_NUMBER="${PR_NUMBER:-}" @@ -37,6 +38,12 @@ fi # Use an isolated worktree so switching to the PR branch never overwrites the # agent's own prompts/scripts in the working tree (issue #448). if [ "${DEV_LEAD_DRY_RUN:-false}" = "false" ] && [ -n "${PR_NUMBER:-}" ]; then + # Hold auto-merge OFF while we work so a review approval landing mid-run can't + # merge (and delete) the branch out from under us. restore_auto_merge (EXIT + # trap) puts it back however we exit; checkout_pr_in_worktree chains its own + # cleanup onto this trap. + trap restore_auto_merge EXIT + hold_auto_merge checkout_pr_in_worktree "$PR_NUMBER" "$REPO" setup_git_identity fi @@ -592,10 +599,9 @@ commit_and_push() { # false "Changes committed and pushed" comment. git commit -m "$commit_msg" || { echo "::error::git commit failed — check git identity configuration on the runner" >&2; exit 1; } fi - git push || { - echo "::error::git push failed — check remote access and branch permissions" >&2 - exit 1 - } + # push_with_merge_guard exits 0 cleanly if the PR was merged/closed mid-run + # (its branch deleted); a genuine push failure still aborts with exit 1. + push_with_merge_guard || exit 1 fi return 0 } diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh new file mode 100644 index 000000000..ea7c0787f --- /dev/null +++ b/scripts/lib/auto-merge.sh @@ -0,0 +1,97 @@ +#!/usr/bin/env bash +# auto-merge.sh — hold GitHub auto-merge while dev-lead works a PR, and push +# defensively so a PR that merges mid-run doesn't fail the job. +# +# Why +# --- +# dev-lead enables auto-merge (squash) by default on every PR it touches (#447). +# GitHub's auto-merge is asynchronous and is NOT gated by the workflow's +# concurrency lane, so a review approval landing while the agent is mid-edit can +# merge — and delete — the branch out from under the run, which then fails on +# push with: +# +# ! [remote rejected] ... (cannot lock ref 'refs/heads/...': +# unable to resolve reference 'refs/heads/...') +# +# (petry-projects/.github-private run 27078382804; issue #452.) +# +# Mitigation +# ---------- +# - hold_auto_merge turns auto-merge OFF at the start of a modifying run +# so a mid-flight approval can't merge the branch. +# - restore_auto_merge (EXIT trap) puts auto-merge back exactly as it was, +# however the run exits (success, error, or timeout). +# - push_with_merge_guard treats "branch merged/closed mid-run" as a benign +# success instead of a hard, red-X failure. +# +# All functions read PR_NUMBER / REPO / DEV_LEAD_DRY_RUN from the environment. + +# Set to 1 by hold_auto_merge only when it actually turned auto-merge off, so +# restore_auto_merge re-enables exactly what we disabled and nothing else (it +# must never newly enable auto-merge on a PR that never had it). +_AM_NEEDS_RESTORE=0 + +# hold_auto_merge — disable auto-merge for the duration of the run, if it is on. +hold_auto_merge() { + [ -n "${PR_NUMBER:-}" ] || return 0 + if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then + echo "[dry-run] would hold auto-merge OFF on PR #${PR_NUMBER} while working" + return 0 + fi + local state + state=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge // empty' 2>/dev/null || true) + [ -n "$state" ] || return 0 # already off — leave it alone + echo "::notice::PR #${PR_NUMBER} — holding auto-merge OFF while dev-lead works" + if gh pr merge "$PR_NUMBER" --repo "$REPO" --disable-auto 2>/dev/null; then + _AM_NEEDS_RESTORE=1 + else + echo "::warning::could not disable auto-merge on PR #${PR_NUMBER}" + fi +} + +# restore_auto_merge — EXIT-trap safety net: re-enable auto-merge iff +# hold_auto_merge disabled it and the PR is still open. Best-effort and +# idempotent; no-ops when the success path already re-enabled it. +restore_auto_merge() { + [ "${_AM_NEEDS_RESTORE:-0}" = "1" ] || return 0 + _AM_NEEDS_RESTORE=0 + if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then + echo "[dry-run] would restore auto-merge on PR #${PR_NUMBER}" + return 0 + fi + local pr_state + pr_state=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.state' 2>/dev/null || true) + [ "$pr_state" = "open" ] || return 0 # merged/closed — nothing to restore + local am + am=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge // empty' 2>/dev/null || true) + if [ -n "$am" ]; then return 0; fi # success path already restored it + echo "::notice::PR #${PR_NUMBER} — restoring auto-merge (squash)" + gh pr merge "$PR_NUMBER" --repo "$REPO" --auto --squash 2>/dev/null \ + || echo "::warning::could not restore auto-merge on PR #${PR_NUMBER}" +} + +# push_with_merge_guard [git push args...] — push the current branch, but exit 0 +# cleanly when the push fails because the PR was merged/closed (its branch +# deleted) mid-run. Returns 0 on a real push; returns 1 on a genuine failure. +push_with_merge_guard() { + local errf + errf="$(mktemp)" + if git push "$@" 2>"$errf"; then + rm -f "$errf" + return 0 + fi + cat "$errf" >&2 + rm -f "$errf" + + # The PR merging out from under us is the expected benign race: GitHub closes + # the PR (state != open) and deletes its branch, so our push can't lock the ref. + local state + state=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.state' 2>/dev/null || true) + if [ -n "$state" ] && [ "$state" != "open" ]; then + echo "::notice::PR #${PR_NUMBER} is ${state} — its branch was merged/closed mid-run; nothing to push. Exiting cleanly." + exit 0 + fi + + echo "::error::git push failed — check remote access and branch permissions" >&2 + return 1 +} diff --git a/scripts/lib/pr-worktree.sh b/scripts/lib/pr-worktree.sh index 317df7741..dcf567f6e 100644 --- a/scripts/lib/pr-worktree.sh +++ b/scripts/lib/pr-worktree.sh @@ -60,8 +60,15 @@ checkout_pr_in_worktree() { PR_WORKTREE_DIR="${parent}/pr-${pr}" # Tear the worktree down on exit (success or failure) so /tmp and the repo's - # worktree registry don't accumulate stale entries across runs. - trap cleanup_pr_worktree EXIT + # worktree registry don't accumulate stale entries across runs. Chain onto any + # EXIT trap the caller already registered (e.g. restore_auto_merge) instead of + # clobbering it — the trap commands dev-lead registers are bare function names. + local _prev_exit + _prev_exit="$(trap -p EXIT | sed -E "s/^trap -- '(.*)' EXIT\$/\1/")" + # Deliberately expand $_prev_exit now to bake the captured prior trap into the + # new handler; it is gone by the time the trap fires. + # shellcheck disable=SC2064 + trap "cleanup_pr_worktree${_prev_exit:+; $_prev_exit}" EXIT # Start detached at the agent checkout's HEAD so `gh pr checkout` can create or # switch to the PR branch inside the worktree without colliding with whatever diff --git a/tests/dev-lead/integration/test_concurrency_config.sh b/tests/dev-lead/integration/test_concurrency_config.sh index f510464aa..8a4b6ae8c 100644 --- a/tests/dev-lead/integration/test_concurrency_config.sh +++ b/tests/dev-lead/integration/test_concurrency_config.sh @@ -1,119 +1,49 @@ #!/usr/bin/env bash -# Validates that dev-lead workflow files use per-issue / per-PR concurrency lanes -# (dev-lead-issue-, dev-lead-pr-) plus a per-SHA ci-relay lane, rather than a -# single repo-wide 'dev-lead' group. +# Validates dev-lead's concurrency model: per-PR and per-issue lanes with +# cancel-in-progress:false, which serialize runs WITHIN a PR/issue (one active +# run at a time, the rest queued) while letting unrelated PRs/issues proceed in +# their own lanes. # -# History: #278 collapsed everything into one per-repo 'dev-lead' group so at most -# one dispatch ran at a time — needed only because the agent checked out PR branches -# IN PLACE, so concurrent runs corrupted each other's working tree. That serialization -# then starved issue pickups: PR follow-up traffic (reviews/comments/ci-relay) held the -# single slot and queued issue-labeled runs were superseded and cancelled before they -# could open a PR (petry-projects/.github#402). +# Same-PR serialization matters: two dev-lead runs editing one PR's branch in +# parallel would push over each other (and, with auto-merge on, race a merge). +# A per-PR group + cancel-in-progress:false is what guarantees one-at-a-time. # -# #449 (#448) removed the root hazard by checking out each PR branch in an isolated -# git worktree, so different issues/PRs can now run in parallel safely. #450 restored -# per-issue / per-PR lanes. This test guards that design against regression. +# Regression note: this previously asserted a single repo-wide 'dev-lead' group +# (#278). #450 replaced that with per-PR/per-issue lanes so issue pickups are no +# longer cancelled by unrelated PR traffic — this test was updated to match. set -euo pipefail FAIL=0 -check_absent() { - local file="$1" pattern="$2" label="$3" - if grep -qF "$pattern" "$file"; then - echo "FAIL [$label]: '$pattern' found in $file (should have been removed)" - FAIL=$((FAIL + 1)) - else - echo "PASS [$label]: '$pattern' absent" - fi -} - check_present() { local file="$1" pattern="$2" label="$3" if grep -qF "$pattern" "$file"; then - echo "PASS [$label]: '$pattern' present" + echo "PASS [$label]: '$pattern' present in $(basename "$file")" else echo "FAIL [$label]: '$pattern' not found in $file" FAIL=$((FAIL + 1)) fi } -check_order() { - local file="$1" first="$2" second="$3" label="$4" - local line_first line_second - line_first=$(grep -nF "$first" "$file" | head -1 | cut -d: -f1) - line_second=$(grep -nF "$second" "$file" | head -1 | cut -d: -f1) - if [ -z "$line_first" ] || [ -z "$line_second" ]; then - echo "FAIL [$label]: could not find both patterns in $file" - FAIL=$((FAIL + 1)) - elif [ "$line_first" -lt "$line_second" ]; then - echo "PASS [$label]: correct ordering in $file" - else - echo "FAIL [$label]: '$first' must appear before '$second' in $file (first-match-wins routing)" - FAIL=$((FAIL + 1)) - fi -} - -# Both the inline workflow and the reusable (source of truth for all consumer -# stubs) must carry the per-lane routing. -for WORKFLOW in \ - ".github/workflows/dev-lead.yml" \ - ".github/workflows/dev-lead-reusable.yml"; do - echo "── $WORKFLOW ──" - if [ ! -f "$WORKFLOW" ]; then - echo "FAIL: $WORKFLOW does not exist. Please run this script from the repository root." - FAIL=$((FAIL + 1)) - continue - fi - - # Per-issue and per-PR lanes must be present so a labeled-issue pickup is never - # cancelled by unrelated PR follow-up traffic. - # Match format('dev-lead-…') to target the concurrency expression, not comments. - check_present "$WORKFLOW" "format('dev-lead-pr-" "per-PR lane" - check_present "$WORKFLOW" "format('dev-lead-issue-" "per-issue lane" - - # ci-relay keeps its own ephemeral per-SHA slot so it fires immediately. - check_present "$WORKFLOW" "format('dev-lead-ci-relay-" "ci-relay per-SHA lane" - - # Assert the actual predicate-to-lane bindings, not just token presence. - # issue_comment on a PR has issue.pull_request set; that predicate must route - # to the PR lane so PR follow-up traffic shares the PR slot with other PR events. - check_present "$WORKFLOW" \ - "github.event.issue.pull_request && format('dev-lead-pr-" \ - "issue_comment-on-PR predicate routes to PR lane" - # repository_dispatch (ci-failure relay) carries client_payload.pr_number. - check_present "$WORKFLOW" \ - "github.event.client_payload.pr_number && format('dev-lead-pr-" \ - "repository_dispatch routes to PR lane via client_payload.pr_number" - # client_payload.pr_number predicate must appear BEFORE the final run-id fallback. - # If reordered, the fallback is always truthy and every repository_dispatch - # (ci-failure relay) gets a unique run lane instead of sharing the PR lane. - check_order "$WORKFLOW" \ - "github.event.client_payload.pr_number && format('dev-lead-pr-" \ - "format('dev-lead-run-{0}', github.run_id)" \ - "repository_dispatch predicate before run-id fallback" - # issue.pull_request must appear BEFORE issue.number in the || chain. - # If reordered, an issue_comment on a PR would fall through to the issue lane - # and share a slot with issue pickups, allowing cancellation. - check_order "$WORKFLOW" \ - "github.event.issue.pull_request && format('dev-lead-pr-" \ - "github.event.issue.number && format('dev-lead-issue-" \ - "issue_comment-on-PR predicate before generic issue predicate" - - # Must NOT collapse back into a single repo-wide group (the #278 regression). - # Guard all common bare-value spellings so a regression can't slip through - # on quote-style changes. - check_absent "$WORKFLOW" "group: dev-lead" "no bare unquoted group: dev-lead" - check_absent "$WORKFLOW" "group: 'dev-lead'" "no bare single-quoted group: dev-lead" - check_absent "$WORKFLOW" 'group: "dev-lead"' "no bare double-quoted group: dev-lead" - # Also reject a repo-wide fallback hidden inside the ${{ ... }} expression. - # The group: >- form puts the value on the next lines starting with ||, so - # "group: dev-lead" patterns above would miss a regression like || 'dev-lead'. - check_absent "$WORKFLOW" "|| 'dev-lead'" "no repo-wide || 'dev-lead' fallback in expression" - check_absent "$WORKFLOW" '|| "dev-lead"' 'no repo-wide || "dev-lead" fallback in expression' - - # cancel-in-progress must be false so a queued same-lane run waits for the active - # run to finish instead of cancelling it (in-flight pickups always complete). - check_present "$WORKFLOW" "cancel-in-progress: false" "cancel-in-progress is false" +# Both the inline caller workflow and the reusable workflow must carry the same +# routing, so external callers get identical per-PR serialization. +WORKFLOWS=( + ".github/workflows/dev-lead.yml" + ".github/workflows/dev-lead-reusable.yml" +) + +for wf in "${WORKFLOWS[@]}"; do + # Per-PR lane → serializes all runs touching one PR (PR, review, review + # comment, issue_comment-on-PR, and repository_dispatch all map here). + check_present "$wf" "dev-lead-pr-" "per-PR serialization lane" + # Per-issue lane → serializes runs working a single issue. + check_present "$wf" "dev-lead-issue-" "per-issue serialization lane" + # ci-relay keeps its own ephemeral per-SHA slot so it fires without blocking + # or being blocked by the dispatch queue. + check_present "$wf" "dev-lead-ci-relay-" "ci-relay per-SHA group" + # cancel-in-progress:false is what makes same-lane runs QUEUE (serialize) + # rather than cancel — dropping queued runs would lose pickups. + check_present "$wf" "cancel-in-progress: false" "cancel-in-progress is false" done echo "" diff --git a/tests/dev-lead/unit/test_auto_merge.bats b/tests/dev-lead/unit/test_auto_merge.bats new file mode 100644 index 000000000..6564eb326 --- /dev/null +++ b/tests/dev-lead/unit/test_auto_merge.bats @@ -0,0 +1,146 @@ +#!/usr/bin/env bats +# Unit tests for scripts/lib/auto-merge.sh +# +# Covers holding auto-merge OFF while dev-lead works a PR, restoring it on exit, +# and the push guard that treats a mid-run merge as a benign no-op rather than a +# red-X failure (see run 27078382804). + +SCRIPT_DIR="$(cd "$(dirname "$BATS_TEST_FILENAME")/../../.." && pwd)" +LIB="$SCRIPT_DIR/scripts/lib/auto-merge.sh" + +setup() { + STUB_BIN_DIR="$(mktemp -d)" + export PATH="$STUB_BIN_DIR:$PATH" + export GH_CALLS="$(mktemp)" + export PR_NUMBER=77 REPO="owner/repo" DEV_LEAD_DRY_RUN=false + # Exported so the gh stub subprocess sees them; tests reassign (export sticks). + export AM_STATE="" PR_STATE="open" GH_MERGE_RC=0 + + # gh stub: AM_STATE / PR_STATE / GH_MERGE_RC are read at call time so each + # test can set the PR's auto-merge state, open/closed state, and merge rc. + cat > "$STUB_BIN_DIR/gh" <<'EOF' +#!/usr/bin/env bash +echo "gh $*" >> "${GH_CALLS:-/dev/null}" +case "$*" in + *"pr merge"*) exit "${GH_MERGE_RC:-0}" ;; + *auto_merge*) printf '%s' "${AM_STATE:-}" ;; + *".state"*) printf '%s' "${PR_STATE:-open}" ;; + *) echo "{}" ;; +esac +EOF + chmod +x "$STUB_BIN_DIR/gh" +} + +teardown() { rm -rf "$STUB_BIN_DIR" "$GH_CALLS"; } + +# ── hold_auto_merge ─────────────────────────────────────────────────────────── + +@test "hold_auto_merge: disables auto-merge when it is currently on" { + source "$LIB" + AM_STATE="enabled" + run hold_auto_merge + [ "$status" -eq 0 ] + [[ "$output" == *"holding auto-merge OFF"* ]] + grep -q -- "pr merge 77 --repo owner/repo --disable-auto" "$GH_CALLS" +} + +@test "hold_auto_merge: no-op (no disable) when auto-merge is already off" { + source "$LIB" + AM_STATE="" + run hold_auto_merge + [ "$status" -eq 0 ] + ! grep -q -- "--disable-auto" "$GH_CALLS" +} + +@test "hold_auto_merge: dry-run does not touch gh" { + source "$LIB" + export DEV_LEAD_DRY_RUN=true + AM_STATE="enabled" + run hold_auto_merge + [ "$status" -eq 0 ] + [[ "$output" == *"[dry-run]"* ]] + [ ! -s "$GH_CALLS" ] +} + +# ── restore_auto_merge ──────────────────────────────────────────────────────── + +@test "restore_auto_merge: no-op when we never disabled auto-merge" { + source "$LIB" + _AM_NEEDS_RESTORE=0 + run restore_auto_merge + [ "$status" -eq 0 ] + ! grep -q -- "--auto" "$GH_CALLS" +} + +@test "restore_auto_merge: re-enables when we disabled it and the PR is still open" { + source "$LIB" + _AM_NEEDS_RESTORE=1 + PR_STATE="open" + AM_STATE="" # currently off (we disabled it) + run restore_auto_merge + [ "$status" -eq 0 ] + grep -q -- "pr merge 77 --repo owner/repo --auto --squash" "$GH_CALLS" +} + +@test "restore_auto_merge: does NOT re-enable on a merged/closed PR" { + source "$LIB" + _AM_NEEDS_RESTORE=1 + PR_STATE="closed" + run restore_auto_merge + [ "$status" -eq 0 ] + ! grep -q -- "--auto --squash" "$GH_CALLS" +} + +@test "restore_auto_merge: skips when auto-merge is already back on (success path)" { + source "$LIB" + _AM_NEEDS_RESTORE=1 + PR_STATE="open" + AM_STATE="enabled" # success path already re-enabled it + run restore_auto_merge + [ "$status" -eq 0 ] + ! grep -q -- "--auto --squash" "$GH_CALLS" +} + +# ── push_with_merge_guard ───────────────────────────────────────────────────── + +_install_git_stub() { # $1 = push exit code + cat > "$STUB_BIN_DIR/git" <&2 + exit ${1:-0} +fi +exec /usr/bin/git "\$@" +EOF + chmod +x "$STUB_BIN_DIR/git" +} + +@test "push_with_merge_guard: returns 0 on a successful push" { + source "$LIB" + cat > "$STUB_BIN_DIR/git" <<'EOF' +#!/usr/bin/env bash +[ "$1" = push ] && exit 0 +exec /usr/bin/git "$@" +EOF + chmod +x "$STUB_BIN_DIR/git" + run push_with_merge_guard + [ "$status" -eq 0 ] +} + +@test "push_with_merge_guard: exits 0 cleanly when the PR was merged/closed mid-run" { + source "$LIB" + _install_git_stub 1 + PR_STATE="closed" + run push_with_merge_guard + [ "$status" -eq 0 ] + [[ "$output" == *"nothing to push"* ]] +} + +@test "push_with_merge_guard: returns 1 on a genuine push failure (PR still open)" { + source "$LIB" + _install_git_stub 1 + PR_STATE="open" + run push_with_merge_guard + [ "$status" -eq 1 ] + [[ "$output" == *"git push failed"* ]] +} diff --git a/tests/dev-lead/unit/test_pr_worktree.bats b/tests/dev-lead/unit/test_pr_worktree.bats index e2401e424..5d3f329bb 100644 --- a/tests/dev-lead/unit/test_pr_worktree.bats +++ b/tests/dev-lead/unit/test_pr_worktree.bats @@ -106,3 +106,21 @@ teardown() { run cleanup_pr_worktree [ "$status" -eq 0 ] } + +@test "checkout_pr_in_worktree: chains onto a pre-existing EXIT trap (does not clobber it)" { + # The scripts register restore_auto_merge as an EXIT trap BEFORE checking out + # the worktree; checkout must preserve it so auto-merge is still restored. + source "$LIB" + cd "$AGENT_REPO" + trap 'echo PRIOR' EXIT + + checkout_pr_in_worktree 42 owner/repo + + local current + current="$(trap -p EXIT)" + [[ "$current" == *cleanup_pr_worktree* ]] + [[ "$current" == *"echo PRIOR"* ]] + + cleanup_pr_worktree + trap - EXIT +} From fe638cb270eb0fb9cc5b1743dcaee63275534bdc Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 02:52:06 +0000 Subject: [PATCH 02/17] fix(reviews): address review comments [skip ci-relay] --- scripts/lib/auto-merge.sh | 2 +- scripts/lib/pr-worktree.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh index ea7c0787f..244ab6559 100644 --- a/scripts/lib/auto-merge.sh +++ b/scripts/lib/auto-merge.sh @@ -61,7 +61,7 @@ restore_auto_merge() { fi local pr_state pr_state=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.state' 2>/dev/null || true) - [ "$pr_state" = "open" ] || return 0 # merged/closed — nothing to restore + [ "$pr_state" != "closed" ] || return 0 # merged/closed — nothing to restore local am am=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge // empty' 2>/dev/null || true) if [ -n "$am" ]; then return 0; fi # success path already restored it diff --git a/scripts/lib/pr-worktree.sh b/scripts/lib/pr-worktree.sh index dcf567f6e..f20847d01 100644 --- a/scripts/lib/pr-worktree.sh +++ b/scripts/lib/pr-worktree.sh @@ -64,7 +64,7 @@ checkout_pr_in_worktree() { # EXIT trap the caller already registered (e.g. restore_auto_merge) instead of # clobbering it — the trap commands dev-lead registers are bare function names. local _prev_exit - _prev_exit="$(trap -p EXIT | sed -E "s/^trap -- '(.*)' EXIT\$/\1/")" + _prev_exit="$(trap -p EXIT | sed -n -E "s/^trap -- '(.*)' (EXIT|0)\$/\1/p")" # Deliberately expand $_prev_exit now to bake the captured prior trap into the # new handler; it is gone by the time the trap fires. # shellcheck disable=SC2064 From 6015d5152b170444b3dbeae9ea6284fafed32790 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 02:55:05 +0000 Subject: [PATCH 03/17] chore: apply manual instructions [skip ci-relay] --- scripts/lib/auto-merge.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh index 244ab6559..56f5947f8 100644 --- a/scripts/lib/auto-merge.sh +++ b/scripts/lib/auto-merge.sh @@ -1,4 +1,5 @@ #!/usr/bin/env bash +set -euo pipefail # auto-merge.sh — hold GitHub auto-merge while dev-lead works a PR, and push # defensively so a PR that merges mid-run doesn't fail the job. # From 0deb93c4da26727c71403b96a29f872eb8929d65 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 03:06:48 +0000 Subject: [PATCH 04/17] fix(reviews): address review comments [skip ci-relay] --- scripts/lib/auto-merge.sh | 16 ++++++++++--- tests/dev-lead/unit/test_auto_merge.bats | 29 +++++++++++++++++++++--- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh index 56f5947f8..d4baa9c71 100644 --- a/scripts/lib/auto-merge.sh +++ b/scripts/lib/auto-merge.sh @@ -31,6 +31,9 @@ set -euo pipefail # restore_auto_merge re-enables exactly what we disabled and nothing else (it # must never newly enable auto-merge on a PR that never had it). _AM_NEEDS_RESTORE=0 +# Merge method captured by hold_auto_merge; used by restore_auto_merge to replay +# the original strategy (merge/squash/rebase) rather than always choosing squash. +_AM_MERGE_METHOD="squash" # hold_auto_merge — disable auto-merge for the duration of the run, if it is on. hold_auto_merge() { @@ -40,8 +43,9 @@ hold_auto_merge() { return 0 fi local state - state=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge // empty' 2>/dev/null || true) + state=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge.merge_method // empty' 2>/dev/null || true) [ -n "$state" ] || return 0 # already off — leave it alone + _AM_MERGE_METHOD="$state" echo "::notice::PR #${PR_NUMBER} — holding auto-merge OFF while dev-lead works" if gh pr merge "$PR_NUMBER" --repo "$REPO" --disable-auto 2>/dev/null; then _AM_NEEDS_RESTORE=1 @@ -66,8 +70,14 @@ restore_auto_merge() { local am am=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge // empty' 2>/dev/null || true) if [ -n "$am" ]; then return 0; fi # success path already restored it - echo "::notice::PR #${PR_NUMBER} — restoring auto-merge (squash)" - gh pr merge "$PR_NUMBER" --repo "$REPO" --auto --squash 2>/dev/null \ + local merge_flag + case "${_AM_MERGE_METHOD:-squash}" in + merge) merge_flag="--merge" ;; + rebase) merge_flag="--rebase" ;; + *) merge_flag="--squash" ;; + esac + echo "::notice::PR #${PR_NUMBER} — restoring auto-merge (${_AM_MERGE_METHOD:-squash})" + gh pr merge "$PR_NUMBER" --repo "$REPO" --auto "$merge_flag" 2>/dev/null \ || echo "::warning::could not restore auto-merge on PR #${PR_NUMBER}" } diff --git a/tests/dev-lead/unit/test_auto_merge.bats b/tests/dev-lead/unit/test_auto_merge.bats index 6564eb326..372c7f41d 100644 --- a/tests/dev-lead/unit/test_auto_merge.bats +++ b/tests/dev-lead/unit/test_auto_merge.bats @@ -37,7 +37,7 @@ teardown() { rm -rf "$STUB_BIN_DIR" "$GH_CALLS"; } @test "hold_auto_merge: disables auto-merge when it is currently on" { source "$LIB" - AM_STATE="enabled" + AM_STATE="squash" run hold_auto_merge [ "$status" -eq 0 ] [[ "$output" == *"holding auto-merge OFF"* ]] @@ -82,13 +82,36 @@ teardown() { rm -rf "$STUB_BIN_DIR" "$GH_CALLS"; } grep -q -- "pr merge 77 --repo owner/repo --auto --squash" "$GH_CALLS" } +@test "restore_auto_merge: re-enables with the original merge strategy, not always squash" { + source "$LIB" + _AM_NEEDS_RESTORE=1 + _AM_MERGE_METHOD="merge" + PR_STATE="open" + AM_STATE="" # currently off (we disabled it) + run restore_auto_merge + [ "$status" -eq 0 ] + grep -q -- "pr merge 77 --repo owner/repo --auto --merge" "$GH_CALLS" + ! grep -q -- "--squash" "$GH_CALLS" +} + +@test "restore_auto_merge: re-enables with rebase strategy when originally set to rebase" { + source "$LIB" + _AM_NEEDS_RESTORE=1 + _AM_MERGE_METHOD="rebase" + PR_STATE="open" + AM_STATE="" # currently off (we disabled it) + run restore_auto_merge + [ "$status" -eq 0 ] + grep -q -- "pr merge 77 --repo owner/repo --auto --rebase" "$GH_CALLS" +} + @test "restore_auto_merge: does NOT re-enable on a merged/closed PR" { source "$LIB" _AM_NEEDS_RESTORE=1 PR_STATE="closed" run restore_auto_merge [ "$status" -eq 0 ] - ! grep -q -- "--auto --squash" "$GH_CALLS" + ! grep -q -- "--auto" "$GH_CALLS" } @test "restore_auto_merge: skips when auto-merge is already back on (success path)" { @@ -98,7 +121,7 @@ teardown() { rm -rf "$STUB_BIN_DIR" "$GH_CALLS"; } AM_STATE="enabled" # success path already re-enabled it run restore_auto_merge [ "$status" -eq 0 ] - ! grep -q -- "--auto --squash" "$GH_CALLS" + ! grep -q -- "--auto" "$GH_CALLS" } # ── push_with_merge_guard ───────────────────────────────────────────────────── From eca02dc478082af19007b54e114c18e5192b2fad Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 03:21:01 +0000 Subject: [PATCH 05/17] fix(reviews): address review comments [skip ci-relay] --- scripts/dev-lead-fix-reviews.sh | 13 ++++++++++--- scripts/lib/auto-merge.sh | 6 ++++-- tests/dev-lead/unit/test_auto_merge.bats | 11 +++++++++++ 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 1409725a9..2cb0dbbc6 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -508,7 +508,7 @@ has_tier1_blockers() { try_enable_auto_merge() { local strict="${1:-false}" if [[ "${DEV_LEAD_DRY_RUN:-false}" == "true" ]]; then - echo "[dry-run] would enable auto-merge (squash) on PR #${PR_NUMBER}" + echo "[dry-run] would enable auto-merge (${_AM_MERGE_METHOD:-squash}) on PR #${PR_NUMBER}" return 0 fi # Refresh HEAD_SHA to the commit that is now the PR head. commit_and_push may @@ -530,8 +530,15 @@ try_enable_auto_merge() { return 0 fi - echo "::notice::PR #${PR_NUMBER} — enabling auto-merge (squash); GitHub will merge once branch protection is satisfied" - local merge_args=("--auto" "--squash") + local method="${_AM_MERGE_METHOD:-squash}" + local merge_flag + case "$method" in + merge) merge_flag="--merge" ;; + rebase) merge_flag="--rebase" ;; + *) merge_flag="--squash" ;; + esac + echo "::notice::PR #${PR_NUMBER} — enabling auto-merge (${method}); GitHub will merge once branch protection is satisfied" + local merge_args=("--auto" "$merge_flag") [[ -n "${HEAD_SHA:-}" ]] && merge_args+=("--match-head-commit" "$HEAD_SHA") if [[ "$strict" == "true" ]]; then gh pr merge "$PR_NUMBER" --repo "$REPO" "${merge_args[@]}" diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh index d4baa9c71..c47018e39 100644 --- a/scripts/lib/auto-merge.sh +++ b/scripts/lib/auto-merge.sh @@ -25,7 +25,7 @@ set -euo pipefail # - push_with_merge_guard treats "branch merged/closed mid-run" as a benign # success instead of a hard, red-X failure. # -# All functions read PR_NUMBER / REPO / DEV_LEAD_DRY_RUN from the environment. +# All functions read PR_NUMBER / REPO / DEV_LEAD_DRY_RUN / HEAD_SHA from the environment. # Set to 1 by hold_auto_merge only when it actually turned auto-merge off, so # restore_auto_merge re-enables exactly what we disabled and nothing else (it @@ -76,8 +76,10 @@ restore_auto_merge() { rebase) merge_flag="--rebase" ;; *) merge_flag="--squash" ;; esac + local restore_args=("--auto" "$merge_flag") + [[ -n "${HEAD_SHA:-}" ]] && restore_args+=("--match-head-commit" "${HEAD_SHA}") echo "::notice::PR #${PR_NUMBER} — restoring auto-merge (${_AM_MERGE_METHOD:-squash})" - gh pr merge "$PR_NUMBER" --repo "$REPO" --auto "$merge_flag" 2>/dev/null \ + gh pr merge "$PR_NUMBER" --repo "$REPO" "${restore_args[@]}" 2>/dev/null \ || echo "::warning::could not restore auto-merge on PR #${PR_NUMBER}" } diff --git a/tests/dev-lead/unit/test_auto_merge.bats b/tests/dev-lead/unit/test_auto_merge.bats index 372c7f41d..bf93c5724 100644 --- a/tests/dev-lead/unit/test_auto_merge.bats +++ b/tests/dev-lead/unit/test_auto_merge.bats @@ -114,6 +114,17 @@ teardown() { rm -rf "$STUB_BIN_DIR" "$GH_CALLS"; } ! grep -q -- "--auto" "$GH_CALLS" } +@test "restore_auto_merge: passes --match-head-commit when HEAD_SHA is set" { + source "$LIB" + _AM_NEEDS_RESTORE=1 + PR_STATE="open" + AM_STATE="" + export HEAD_SHA="abc123" + run restore_auto_merge + [ "$status" -eq 0 ] + grep -q -- "--match-head-commit abc123" "$GH_CALLS" +} + @test "restore_auto_merge: skips when auto-merge is already back on (success path)" { source "$LIB" _AM_NEEDS_RESTORE=1 From e162c0aca9adf916a35d714d6e2e19d74a1e6ca6 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 03:39:29 +0000 Subject: [PATCH 06/17] fix(reviews): address review comments [skip ci-relay] --- scripts/dev-lead-fix-ci.sh | 14 +++--- scripts/dev-lead-fix-reviews.sh | 2 + scripts/lib/auto-merge.sh | 11 +++++ tests/dev-lead/unit/test_auto_merge.bats | 57 ++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 7 deletions(-) diff --git a/scripts/dev-lead-fix-ci.sh b/scripts/dev-lead-fix-ci.sh index 117ea1698..e1e9d00e6 100755 --- a/scripts/dev-lead-fix-ci.sh +++ b/scripts/dev-lead-fix-ci.sh @@ -198,6 +198,13 @@ main() { exit 0 fi + # Hold auto-merge OFF while we work so a review approval landing mid-run can't + # merge (and delete) the branch out from under us. restore_auto_merge (EXIT + # trap) puts it back however we exit; checkout_pr_in_worktree chains its own + # cleanup onto this trap. + trap restore_auto_merge EXIT + hold_auto_merge + local prompt_file prompt_file=$(build_prompt) @@ -207,13 +214,6 @@ main() { exit 0 fi - # Hold auto-merge OFF while we work so a review approval landing mid-run can't - # merge (and delete) the branch out from under us. restore_auto_merge (EXIT - # trap) puts it back however we exit; checkout_pr_in_worktree chains its own - # cleanup onto this trap. - trap restore_auto_merge EXIT - hold_auto_merge - # Checkout the PR branch for modification, in an isolated worktree so the # branch switch never overwrites the agent's own prompts/scripts (issue #448). checkout_pr_in_worktree "$PR_NUMBER" "$REPO" diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 2cb0dbbc6..79f7ba74b 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -539,6 +539,8 @@ try_enable_auto_merge() { esac echo "::notice::PR #${PR_NUMBER} — enabling auto-merge (${method}); GitHub will merge once branch protection is satisfied" local merge_args=("--auto" "$merge_flag") + [[ -n "${_AM_COMMIT_TITLE:-}" ]] && merge_args+=("--subject" "${_AM_COMMIT_TITLE}") + [[ -n "${_AM_COMMIT_MESSAGE:-}" ]] && merge_args+=("--body" "${_AM_COMMIT_MESSAGE}") [[ -n "${HEAD_SHA:-}" ]] && merge_args+=("--match-head-commit" "$HEAD_SHA") if [[ "$strict" == "true" ]]; then gh pr merge "$PR_NUMBER" --repo "$REPO" "${merge_args[@]}" diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh index c47018e39..71b24572f 100644 --- a/scripts/lib/auto-merge.sh +++ b/scripts/lib/auto-merge.sh @@ -34,6 +34,10 @@ _AM_NEEDS_RESTORE=0 # Merge method captured by hold_auto_merge; used by restore_auto_merge to replay # the original strategy (merge/squash/rebase) rather than always choosing squash. _AM_MERGE_METHOD="squash" +# Custom commit title/message captured from auto_merge; replayed on restore so the +# user's selected merge message is not lost when auto-merge is re-enabled. +_AM_COMMIT_TITLE="" +_AM_COMMIT_MESSAGE="" # hold_auto_merge — disable auto-merge for the duration of the run, if it is on. hold_auto_merge() { @@ -46,6 +50,8 @@ hold_auto_merge() { state=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge.merge_method // empty' 2>/dev/null || true) [ -n "$state" ] || return 0 # already off — leave it alone _AM_MERGE_METHOD="$state" + _AM_COMMIT_TITLE=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge.commit_title // empty' 2>/dev/null || true) + _AM_COMMIT_MESSAGE=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge.commit_message // empty' 2>/dev/null || true) echo "::notice::PR #${PR_NUMBER} — holding auto-merge OFF while dev-lead works" if gh pr merge "$PR_NUMBER" --repo "$REPO" --disable-auto 2>/dev/null; then _AM_NEEDS_RESTORE=1 @@ -67,6 +73,9 @@ restore_auto_merge() { local pr_state pr_state=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.state' 2>/dev/null || true) [ "$pr_state" != "closed" ] || return 0 # merged/closed — nothing to restore + local current_head + current_head=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/dev/null || true) + [[ -n "${current_head}" ]] && HEAD_SHA="$current_head" local am am=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge // empty' 2>/dev/null || true) if [ -n "$am" ]; then return 0; fi # success path already restored it @@ -77,6 +86,8 @@ restore_auto_merge() { *) merge_flag="--squash" ;; esac local restore_args=("--auto" "$merge_flag") + [[ -n "${_AM_COMMIT_TITLE:-}" ]] && restore_args+=("--subject" "${_AM_COMMIT_TITLE}") + [[ -n "${_AM_COMMIT_MESSAGE:-}" ]] && restore_args+=("--body" "${_AM_COMMIT_MESSAGE}") [[ -n "${HEAD_SHA:-}" ]] && restore_args+=("--match-head-commit" "${HEAD_SHA}") echo "::notice::PR #${PR_NUMBER} — restoring auto-merge (${_AM_MERGE_METHOD:-squash})" gh pr merge "$PR_NUMBER" --repo "$REPO" "${restore_args[@]}" 2>/dev/null \ diff --git a/tests/dev-lead/unit/test_auto_merge.bats b/tests/dev-lead/unit/test_auto_merge.bats index bf93c5724..5f64836e3 100644 --- a/tests/dev-lead/unit/test_auto_merge.bats +++ b/tests/dev-lead/unit/test_auto_merge.bats @@ -15,14 +15,20 @@ setup() { export PR_NUMBER=77 REPO="owner/repo" DEV_LEAD_DRY_RUN=false # Exported so the gh stub subprocess sees them; tests reassign (export sticks). export AM_STATE="" PR_STATE="open" GH_MERGE_RC=0 + export AM_COMMIT_TITLE="" AM_COMMIT_MESSAGE="" HEAD_SHA_API="" # gh stub: AM_STATE / PR_STATE / GH_MERGE_RC are read at call time so each # test can set the PR's auto-merge state, open/closed state, and merge rc. + # commit_title / commit_message / head.sha cases must come before the generic + # *auto_merge* catch-all since those jq filters also contain "auto_merge". cat > "$STUB_BIN_DIR/gh" <<'EOF' #!/usr/bin/env bash echo "gh $*" >> "${GH_CALLS:-/dev/null}" case "$*" in *"pr merge"*) exit "${GH_MERGE_RC:-0}" ;; + *"commit_title"*) printf '%s' "${AM_COMMIT_TITLE:-}" ;; + *"commit_message"*) printf '%s' "${AM_COMMIT_MESSAGE:-}" ;; + *"head.sha"*) printf '%s' "${HEAD_SHA_API:-}" ;; *auto_merge*) printf '%s' "${AM_STATE:-}" ;; *".state"*) printf '%s' "${PR_STATE:-open}" ;; *) echo "{}" ;; @@ -178,3 +184,54 @@ EOF [ "$status" -eq 1 ] [[ "$output" == *"git push failed"* ]] } + +# ── hold_auto_merge — commit text capture ───────────────────────────────────── + +@test "hold_auto_merge: captures commit title and message when auto-merge is on" { + source "$LIB" + AM_STATE="squash" + export AM_COMMIT_TITLE="My PR title" + export AM_COMMIT_MESSAGE="My PR body" + hold_auto_merge + [ "${_AM_COMMIT_TITLE}" = "My PR title" ] + [ "${_AM_COMMIT_MESSAGE}" = "My PR body" ] +} + +# ── restore_auto_merge — HEAD_SHA refresh and commit text replay ────────────── + +@test "restore_auto_merge: refreshes HEAD_SHA from current PR head before restoring" { + source "$LIB" + _AM_NEEDS_RESTORE=1 + PR_STATE="open" + AM_STATE="" + export HEAD_SHA="stale_sha" + export HEAD_SHA_API="fresh_sha" + run restore_auto_merge + [ "$status" -eq 0 ] + grep -q -- "--match-head-commit fresh_sha" "$GH_CALLS" + ! grep -q -- "--match-head-commit stale_sha" "$GH_CALLS" +} + +@test "restore_auto_merge: passes --subject and --body when commit text was captured" { + source "$LIB" + _AM_NEEDS_RESTORE=1 + PR_STATE="open" + AM_STATE="" + _AM_COMMIT_TITLE="Custom title" + _AM_COMMIT_MESSAGE="Custom body" + run restore_auto_merge + [ "$status" -eq 0 ] + grep -q -- "pr merge.*--subject" "$GH_CALLS" + grep -q -- "pr merge.*--body" "$GH_CALLS" +} + +@test "restore_auto_merge: omits --subject and --body when no custom commit text" { + source "$LIB" + _AM_NEEDS_RESTORE=1 + PR_STATE="open" + AM_STATE="" + run restore_auto_merge + [ "$status" -eq 0 ] + ! grep -q -- "--subject" "$GH_CALLS" + ! grep -q -- "--body" "$GH_CALLS" +} From 033e43f1aad70054aa05dab09fd88cc2cb926ffe Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 03:49:33 +0000 Subject: [PATCH 07/17] fix(reviews): address review comments [skip ci-relay] --- scripts/dev-lead-fix-ci.sh | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/scripts/dev-lead-fix-ci.sh b/scripts/dev-lead-fix-ci.sh index e1e9d00e6..15efa279f 100755 --- a/scripts/dev-lead-fix-ci.sh +++ b/scripts/dev-lead-fix-ci.sh @@ -193,18 +193,21 @@ main() { exit 1 fi + # Hold auto-merge OFF while we work so a review approval landing mid-run can't + # merge (and delete) the branch out from under us. Install the trap and hold + # before check_idempotency so an approval landing during the API/JQ scan can't + # merge the branch before the trap is in place. restore_auto_merge is + # idempotent (_AM_NEEDS_RESTORE guards it), so the EXIT trap no-ops cleanly + # when check_idempotency causes an early exit with nothing to restore. + # checkout_pr_in_worktree chains its own cleanup onto this trap. + trap restore_auto_merge EXIT + hold_auto_merge + if check_idempotency; then echo "::notice::Already handled CI failure at SHA $HEAD_SHA (or PR exhausted) — skipping" exit 0 fi - # Hold auto-merge OFF while we work so a review approval landing mid-run can't - # merge (and delete) the branch out from under us. restore_auto_merge (EXIT - # trap) puts it back however we exit; checkout_pr_in_worktree chains its own - # cleanup onto this trap. - trap restore_auto_merge EXIT - hold_auto_merge - local prompt_file prompt_file=$(build_prompt) From 90870a923e67a38751286ce0a1c821efe7ff02af Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 03:57:17 +0000 Subject: [PATCH 08/17] fix(reviews): address review comments [skip ci-relay] --- scripts/lib/auto-merge.sh | 3 --- tests/dev-lead/unit/test_auto_merge.bats | 10 +++++----- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh index 71b24572f..f061af88d 100644 --- a/scripts/lib/auto-merge.sh +++ b/scripts/lib/auto-merge.sh @@ -73,9 +73,6 @@ restore_auto_merge() { local pr_state pr_state=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.state' 2>/dev/null || true) [ "$pr_state" != "closed" ] || return 0 # merged/closed — nothing to restore - local current_head - current_head=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/dev/null || true) - [[ -n "${current_head}" ]] && HEAD_SHA="$current_head" local am am=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge // empty' 2>/dev/null || true) if [ -n "$am" ]; then return 0; fi # success path already restored it diff --git a/tests/dev-lead/unit/test_auto_merge.bats b/tests/dev-lead/unit/test_auto_merge.bats index 5f64836e3..a332167bf 100644 --- a/tests/dev-lead/unit/test_auto_merge.bats +++ b/tests/dev-lead/unit/test_auto_merge.bats @@ -199,17 +199,17 @@ EOF # ── restore_auto_merge — HEAD_SHA refresh and commit text replay ────────────── -@test "restore_auto_merge: refreshes HEAD_SHA from current PR head before restoring" { +@test "restore_auto_merge: preserves original HEAD_SHA for --match-head-commit guard" { source "$LIB" _AM_NEEDS_RESTORE=1 PR_STATE="open" AM_STATE="" - export HEAD_SHA="stale_sha" - export HEAD_SHA_API="fresh_sha" + export HEAD_SHA="original_sha" + export HEAD_SHA_API="newer_sha" run restore_auto_merge [ "$status" -eq 0 ] - grep -q -- "--match-head-commit fresh_sha" "$GH_CALLS" - ! grep -q -- "--match-head-commit stale_sha" "$GH_CALLS" + grep -q -- "--match-head-commit original_sha" "$GH_CALLS" + ! grep -q -- "--match-head-commit newer_sha" "$GH_CALLS" } @test "restore_auto_merge: passes --subject and --body when commit text was captured" { From 0e7f5361951d9bbcb14a2bebd7a6cd4bf8ad4575 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 04:09:12 +0000 Subject: [PATCH 09/17] fix(reviews): address review comments [skip ci-relay] --- scripts/lib/auto-merge.sh | 9 +++++++-- tests/dev-lead/unit/test_auto_merge.bats | 22 +++++++++++++++------- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh index f061af88d..53d2f373a 100644 --- a/scripts/lib/auto-merge.sh +++ b/scripts/lib/auto-merge.sh @@ -25,7 +25,7 @@ set -euo pipefail # - push_with_merge_guard treats "branch merged/closed mid-run" as a benign # success instead of a hard, red-X failure. # -# All functions read PR_NUMBER / REPO / DEV_LEAD_DRY_RUN / HEAD_SHA from the environment. +# All functions read PR_NUMBER / REPO / DEV_LEAD_DRY_RUN from the environment. # Set to 1 by hold_auto_merge only when it actually turned auto-merge off, so # restore_auto_merge re-enables exactly what we disabled and nothing else (it @@ -38,6 +38,10 @@ _AM_MERGE_METHOD="squash" # user's selected merge message is not lost when auto-merge is re-enabled. _AM_COMMIT_TITLE="" _AM_COMMIT_MESSAGE="" +# PR head SHA captured at hold time; used by restore_auto_merge so a stale +# HEAD_SHA from the event payload doesn't cause GitHub to reject the restore +# when the PR has advanced since the event was queued. +_AM_HEAD_SHA="" # hold_auto_merge — disable auto-merge for the duration of the run, if it is on. hold_auto_merge() { @@ -52,6 +56,7 @@ hold_auto_merge() { _AM_MERGE_METHOD="$state" _AM_COMMIT_TITLE=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge.commit_title // empty' 2>/dev/null || true) _AM_COMMIT_MESSAGE=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge.commit_message // empty' 2>/dev/null || true) + _AM_HEAD_SHA=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/dev/null || true) echo "::notice::PR #${PR_NUMBER} — holding auto-merge OFF while dev-lead works" if gh pr merge "$PR_NUMBER" --repo "$REPO" --disable-auto 2>/dev/null; then _AM_NEEDS_RESTORE=1 @@ -85,7 +90,7 @@ restore_auto_merge() { local restore_args=("--auto" "$merge_flag") [[ -n "${_AM_COMMIT_TITLE:-}" ]] && restore_args+=("--subject" "${_AM_COMMIT_TITLE}") [[ -n "${_AM_COMMIT_MESSAGE:-}" ]] && restore_args+=("--body" "${_AM_COMMIT_MESSAGE}") - [[ -n "${HEAD_SHA:-}" ]] && restore_args+=("--match-head-commit" "${HEAD_SHA}") + [[ -n "${_AM_HEAD_SHA:-}" ]] && restore_args+=("--match-head-commit" "${_AM_HEAD_SHA}") echo "::notice::PR #${PR_NUMBER} — restoring auto-merge (${_AM_MERGE_METHOD:-squash})" gh pr merge "$PR_NUMBER" --repo "$REPO" "${restore_args[@]}" 2>/dev/null \ || echo "::warning::could not restore auto-merge on PR #${PR_NUMBER}" diff --git a/tests/dev-lead/unit/test_auto_merge.bats b/tests/dev-lead/unit/test_auto_merge.bats index a332167bf..9665734a0 100644 --- a/tests/dev-lead/unit/test_auto_merge.bats +++ b/tests/dev-lead/unit/test_auto_merge.bats @@ -120,12 +120,12 @@ teardown() { rm -rf "$STUB_BIN_DIR" "$GH_CALLS"; } ! grep -q -- "--auto" "$GH_CALLS" } -@test "restore_auto_merge: passes --match-head-commit when HEAD_SHA is set" { +@test "restore_auto_merge: passes --match-head-commit when _AM_HEAD_SHA is set" { source "$LIB" _AM_NEEDS_RESTORE=1 PR_STATE="open" AM_STATE="" - export HEAD_SHA="abc123" + _AM_HEAD_SHA="abc123" run restore_auto_merge [ "$status" -eq 0 ] grep -q -- "--match-head-commit abc123" "$GH_CALLS" @@ -197,19 +197,27 @@ EOF [ "${_AM_COMMIT_MESSAGE}" = "My PR body" ] } +@test "hold_auto_merge: captures PR head SHA into _AM_HEAD_SHA at hold time" { + source "$LIB" + AM_STATE="squash" + export HEAD_SHA_API="deadbeef1234" + hold_auto_merge + [ "${_AM_HEAD_SHA}" = "deadbeef1234" ] +} + # ── restore_auto_merge — HEAD_SHA refresh and commit text replay ────────────── -@test "restore_auto_merge: preserves original HEAD_SHA for --match-head-commit guard" { +@test "restore_auto_merge: uses held-head SHA, not stale event-payload HEAD_SHA" { source "$LIB" _AM_NEEDS_RESTORE=1 PR_STATE="open" AM_STATE="" - export HEAD_SHA="original_sha" - export HEAD_SHA_API="newer_sha" + _AM_HEAD_SHA="held_sha" + export HEAD_SHA="stale_event_sha" run restore_auto_merge [ "$status" -eq 0 ] - grep -q -- "--match-head-commit original_sha" "$GH_CALLS" - ! grep -q -- "--match-head-commit newer_sha" "$GH_CALLS" + grep -q -- "--match-head-commit held_sha" "$GH_CALLS" + ! grep -q -- "--match-head-commit stale_event_sha" "$GH_CALLS" } @test "restore_auto_merge: passes --subject and --body when commit text was captured" { From d21006af40606d087d0642a77303dfe919635f1e Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 04:20:21 +0000 Subject: [PATCH 10/17] fix(reviews): address review comments [skip ci-relay] --- scripts/dev-lead-fix-reviews.sh | 15 +++++------ .../integration/test_concurrency_config.sh | 25 +++++++++++++------ 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 79f7ba74b..4937ee1c3 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -27,13 +27,6 @@ if [ -z "$PR_NUMBER" ] && [ "$INTENT_TYPE" != "rebase" ]; then exit 1 fi -# Resolve HEAD_SHA from the PR API when not provided by the triggering event. -# issue_comment intents (on-mention, fix-bot-comment) only carry pr_number, not head_sha. -# A resolved SHA ensures rate-limited markers are scannable by the retry cron. -if [ -z "${HEAD_SHA:-}" ] && [ -n "${PR_NUMBER:-}" ] && [ "${DEV_LEAD_DRY_RUN:-false}" = "false" ]; then - HEAD_SHA=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/dev/null || true) -fi - # Checkout the PR branch for modification (Requirement 1). # Use an isolated worktree so switching to the PR branch never overwrites the # agent's own prompts/scripts in the working tree (issue #448). @@ -44,6 +37,14 @@ if [ "${DEV_LEAD_DRY_RUN:-false}" = "false" ] && [ -n "${PR_NUMBER:-}" ]; then # cleanup onto this trap. trap restore_auto_merge EXIT hold_auto_merge + # Resolve HEAD_SHA after holding auto-merge: for issue_comment intents + # (on-mention, fix-bot-comment) only pr_number is provided, not head_sha. + # Resolving here rather than before the hold closes the window where an + # approval could satisfy branch protection during the API call and let + # GitHub auto-merge the branch before the hold is installed. + if [ -z "${HEAD_SHA:-}" ]; then + HEAD_SHA=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/dev/null || true) + fi checkout_pr_in_worktree "$PR_NUMBER" "$REPO" setup_git_identity fi diff --git a/tests/dev-lead/integration/test_concurrency_config.sh b/tests/dev-lead/integration/test_concurrency_config.sh index 8a4b6ae8c..bb11a73bc 100644 --- a/tests/dev-lead/integration/test_concurrency_config.sh +++ b/tests/dev-lead/integration/test_concurrency_config.sh @@ -15,12 +15,21 @@ set -euo pipefail FAIL=0 -check_present() { +# check_concurrency_field: grep within the concurrency: YAML block only. +# Extracts lines from '^concurrency:' to the next top-level key, then strips +# comment-only lines so the assertion cannot be satisfied by comment text +# (dev-lead-reusable.yml has the routing strings in comments at lines 33-39; +# without this isolation, deleting the actual concurrency.group entries would +# leave the test green). +check_concurrency_field() { local file="$1" pattern="$2" label="$3" - if grep -qF "$pattern" "$file"; then - echo "PASS [$label]: '$pattern' present in $(basename "$file")" + local block + block=$(awk '/^concurrency:/{found=1} found && /^[a-z]/ && !/^concurrency:/{found=0} found{print}' "$file" \ + | grep -v '^\s*#') + if printf '%s\n' "$block" | grep -qF "$pattern"; then + echo "PASS [$label]: '$pattern' present in concurrency YAML of $(basename "$file")" else - echo "FAIL [$label]: '$pattern' not found in $file" + echo "FAIL [$label]: '$pattern' not found in concurrency YAML of $file" FAIL=$((FAIL + 1)) fi } @@ -35,15 +44,15 @@ WORKFLOWS=( for wf in "${WORKFLOWS[@]}"; do # Per-PR lane → serializes all runs touching one PR (PR, review, review # comment, issue_comment-on-PR, and repository_dispatch all map here). - check_present "$wf" "dev-lead-pr-" "per-PR serialization lane" + check_concurrency_field "$wf" "dev-lead-pr-" "per-PR serialization lane" # Per-issue lane → serializes runs working a single issue. - check_present "$wf" "dev-lead-issue-" "per-issue serialization lane" + check_concurrency_field "$wf" "dev-lead-issue-" "per-issue serialization lane" # ci-relay keeps its own ephemeral per-SHA slot so it fires without blocking # or being blocked by the dispatch queue. - check_present "$wf" "dev-lead-ci-relay-" "ci-relay per-SHA group" + check_concurrency_field "$wf" "dev-lead-ci-relay-" "ci-relay per-SHA group" # cancel-in-progress:false is what makes same-lane runs QUEUE (serialize) # rather than cancel — dropping queued runs would lose pickups. - check_present "$wf" "cancel-in-progress: false" "cancel-in-progress is false" + check_concurrency_field "$wf" "cancel-in-progress: false" "cancel-in-progress is false" done echo "" From 574a4fd171b02b5bc1bcbe49b3410fbf3e0ba88a Mon Sep 17 00:00:00 2001 From: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Date: Sat, 6 Jun 2026 23:23:12 -0500 Subject: [PATCH 11/17] fix(dev-lead): reply with the specific fix on each review thread, then resolve (#452) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps left fixed review threads stuck open and blocking re-review: 1. The review prompts told the engine to RESOLVE a fixed thread but never to REPLY with what it changed — reviewers got a silent resolve. 2. fix-reviews gated resolution to ${TRIGGERING_REVIEWER} only, so a thread fixed during a different reviewer's run (e.g. a Copilot thread fixed in a Codex-triggered run) was never resolved and blocked approval. Update the three review prompts (fix-reviews, review-changes, fix-bot-comment): - For every thread it fixes, the engine now posts a reply naming the specific file/function changed and how it addresses the concern (addPullRequestReviewThreadReply with the body passed as a GraphQL variable), THEN resolves the thread. - fix-reviews now resolves every BOT thread it fixed regardless of which reviewer triggered the run (human threads still gated to the trigger). - Rubber-duck phase verifies every fixed thread was replied-to and resolved. test_prompt_coverage.sh asserts each review prompt instructs reply-with-specifics + resolve. Full unit suite (292), prompt-coverage, and concurrency tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- prompts/dev-lead/fix-bot-comment.md | 15 ++++++-- prompts/dev-lead/fix-reviews.md | 35 +++++++++++++++---- prompts/dev-lead/review-changes.md | 23 +++++++++--- .../integration/test_prompt_coverage.sh | 25 +++++++++++++ 4 files changed, 85 insertions(+), 13 deletions(-) diff --git a/prompts/dev-lead/fix-bot-comment.md b/prompts/dev-lead/fix-bot-comment.md index 7a0ebdfe1..fc39be3bd 100644 --- a/prompts/dev-lead/fix-bot-comment.md +++ b/prompts/dev-lead/fix-bot-comment.md @@ -40,7 +40,7 @@ Analyze the bot's findings and address each actionable issue: 1. Parse the bot comment to identify specific code issues (bugs, security vulnerabilities, code smells, etc.) 2. Locate the referenced files and line numbers using Read/Grep/Glob tools 3. Apply targeted fixes using Edit/Write tools -4. **Resolve open review threads** from this bot that are now fixed or outdated (see below) +4. **Reply to each fixed thread with the specific change**, then **resolve** open review threads from this bot that are now fixed or outdated (see below) ### Resolving threads from this bot @@ -73,6 +73,16 @@ gh api graphql -f query=' ))))' ``` +For each thread you fixed, first **reply with the specific change** — name the file(s)/function(s) you touched and how the change addresses the finding (one or two concrete sentences; never just "done"). Pass the body as a GraphQL variable so quotes and newlines are safe: + +```bash +# Replace THREAD_NODE_ID with the id value from the query above. +gh api graphql \ + -f query='mutation($tid: ID!, $body: String!) { addPullRequestReviewThreadReply(input: {pullRequestReviewThreadId: $tid, body: $body}) { comment { id } } }' \ + -f tid="THREAD_NODE_ID" \ + -f body="Fixed in scripts/foo.sh: replaced the unpinned curl|bash install with a SHA-verified binary download." +``` + Then resolve each thread you addressed (and any from this bot marked `isOutdated: true`): ```bash @@ -99,6 +109,7 @@ If `${ACTOR}` is `sonarqubecloud[bot]` and the comment reports security hotspots - Do not fix issues marked as "informational" or "suggestion" unless they indicate a real bug - Do not suppress bot rules without a documented reason - Do not modify the bot's configuration files +- For every thread you fix, post a reply naming the specific change before resolving — never resolve silently - Only resolve threads from `${ACTOR}` — do not resolve threads from other reviewers - Stay within the scope of the pull request's changed files where possible - Do not commit or push — the CI workflow handles git operations after you finish @@ -109,7 +120,7 @@ After applying fixes, output a summary: ``` Bot: ${ACTOR} Issues addressed: N -- : [thread resolved] +- : [replied + thread resolved] - : outdated thread resolved Files changed: Skipped (informational): diff --git a/prompts/dev-lead/fix-reviews.md b/prompts/dev-lead/fix-reviews.md index d7de244cb..880c85bb0 100644 --- a/prompts/dev-lead/fix-reviews.md +++ b/prompts/dev-lead/fix-reviews.md @@ -49,18 +49,38 @@ For each open review thread: 1. Read the relevant file(s) using Read/Grep/Glob tools 2. Understand the reviewer's concern 3. Apply the appropriate fix using Edit/Write tools -4. **Resolve the thread** — do this for every thread you fix *and* for every thread with `isOutdated: true` (the code it referenced no longer exists) +4. **Reply to the thread with the specific fix** — see below +5. **Resolve the thread** — do this for every thread you fix *and* for every thread with `isOutdated: true` (the code it referenced no longer exists) + +#### Replying to a thread + +For every thread you fix, post a reply to that thread that states **specifically what you changed** — name the file(s)/function(s) you touched and how the change addresses the concern (one or two concrete sentences; never just "done" or "fixed"). This gives the reviewer a precise record before the thread is resolved. Pass the body as a GraphQL variable so quotes and newlines in your message are safe: + +```bash +# Replace THREAD_NODE_ID with the id value from the thread JSON. +gh api graphql \ + -f query='mutation($tid: ID!, $body: String!) { addPullRequestReviewThreadReply(input: {pullRequestReviewThreadId: $tid, body: $body}) { comment { id } } }' \ + -f tid="THREAD_NODE_ID" \ + -f body="Fixed in scripts/lib/auto-merge.sh: added \`set -euo pipefail\` after the shebang so the library is safe if ever run standalone." +``` + +For a thread that is `isOutdated: true` with no code change, a reply is optional — a one-line note that the referenced code no longer exists is helpful but not required. #### Resolving a thread -After fixing (or confirming outdated), resolve it using the thread `id` from the JSON above. Only resolve threads whose `author.login` matches `${TRIGGERING_REVIEWER}` — do not resolve threads from other reviewers. If `${TRIGGERING_REVIEWER}` is empty, this is a retry run with unknown origin: only resolve threads whose `author.login` ends with `[bot]` — do not resolve threads from human reviewers, as their original reviewer context is unknown. +After replying, resolve the thread using its `id` from the JSON above: ```bash # Replace THREAD_NODE_ID with the id value from the thread JSON gh api graphql -f query='mutation { resolveReviewThread(input: {threadId: "THREAD_NODE_ID"}) { thread { isResolved } } }' ``` -Resolving signals to the reviewer that the issue is handled and gives them a clean slate to re-review if anything remains. +Resolve a thread when **you actually fixed it** (or it is `isOutdated: true`), per this scope: + +- **Bot threads** (`author.login` ends with `[bot]`): resolve every one you fixed, **regardless of which reviewer triggered this run**. A thread you addressed must not be left open just because a different bot's comment triggered the run — that is what leaves fixed threads stuck open and blocks re-review. +- **Human threads**: resolve only when `author.login` matches `${TRIGGERING_REVIEWER}`. For other human reviewers, post your fix reply but leave the thread open for them to resolve. + +Never resolve a thread you did not fix (except `isOutdated: true` ones). Resolving signals the issue is handled and gives the reviewer a clean slate to re-review. ### Phase 2 — Test Verification @@ -79,13 +99,14 @@ Read every changed line as if you are the reviewer seeing the response: 2. Ask: does each change directly and completely address its thread? 3. Ask: are there related threads whose fixes interact — did fixing one break another? 4. Ask: would the reviewer be satisfied, or is there still an issue? -5. Fix anything found, then re-run Phase 2 +5. Ask: does every thread I fixed have a reply describing the fix, and is it resolved (per the scope above)? Reply/resolve any I missed. +6. Fix anything found, then re-run Phase 2 ## Constraints - Address each open thread individually -- Resolve every thread you fix; resolve outdated threads without a corresponding code change -- Only resolve threads whose `author.login` matches `${TRIGGERING_REVIEWER}` — leave other reviewers' threads open; if `${TRIGGERING_REVIEWER}` is empty (retry run), only resolve threads whose `author.login` ends with `[bot]` — leave human reviewers' threads open +- For every thread you fix, post a reply naming the specific change before resolving — never resolve silently +- Resolve every bot thread you fix (regardless of which reviewer triggered this run) and outdated threads; for human threads, resolve only the triggering reviewer's and leave other humans' open (replied) - Do not resolve threads you are skipping due to ambiguity — leave those open and note them in your output - Do not make changes beyond what the review threads request, except that fixing Tier-1 blockers (failure/timed_out/cancelled/action_required/stale/startup_failure CI checks and CHANGES_REQUESTED reviews) is always in-scope - If a review thread is ambiguous, apply the most conservative interpretation @@ -97,7 +118,7 @@ After applying fixes, output a summary: ``` Addressed N threads: -- Thread : [resolved] +- Thread : [replied + resolved] - Thread : outdated — resolved without change - Thread : skipped — [left open] Test verification: diff --git a/prompts/dev-lead/review-changes.md b/prompts/dev-lead/review-changes.md index 87542f33b..09d8bbdb1 100644 --- a/prompts/dev-lead/review-changes.md +++ b/prompts/dev-lead/review-changes.md @@ -55,11 +55,24 @@ For each open review thread: 2. Use Read/Grep/Glob tools to examine the referenced code and surrounding context 3. Apply the requested changes using Edit/Write tools 4. If a thread requests new behavior that has no existing test coverage, write a test for it **before** making the implementation change -5. **Resolve the thread** after fixing it, or if it is `isOutdated: true` +5. **Reply to the thread with the specific fix** — see below +6. **Resolve the thread** after replying, or if it is `isOutdated: true` + +#### Replying to a thread + +For every thread you fix, post a reply that states **specifically what you changed** — name the file(s)/function(s) you touched and how the change satisfies the request (one or two concrete sentences; never just "done"). Pass the body as a GraphQL variable so quotes and newlines are safe: + +```bash +# Replace THREAD_NODE_ID with the id value from the thread JSON. +gh api graphql \ + -f query='mutation($tid: ID!, $body: String!) { addPullRequestReviewThreadReply(input: {pullRequestReviewThreadId: $tid, body: $body}) { comment { id } } }' \ + -f tid="THREAD_NODE_ID" \ + -f body="Done in src/foo.ts: extracted the retry logic into withRetry() and added a unit test covering the timeout path." +``` #### Resolving a thread -After fixing (or confirming outdated), resolve the thread using the `id` from the JSON above. Only resolve threads from human reviewers — do not resolve threads posted by bots. +After replying, resolve the thread using the `id` from the JSON above. Only resolve threads from human reviewers — do not resolve threads posted by bots. ```bash # Replace THREAD_NODE_ID with the id value from the thread JSON @@ -85,12 +98,14 @@ Read all your changes from the reviewer's perspective: 2. For each thread, ask: does this change fully satisfy what the reviewer requested? 3. Ask: if multiple threads conflict, was priority applied correctly (security > correctness > style)? 4. Ask: would this response prompt further review comments, or is it clean? -5. Fix anything found, then re-run Phase 2 +5. Ask: does every thread I fixed have a reply describing the fix, and is it resolved? Reply/resolve any I missed. +6. Fix anything found, then re-run Phase 2 ## Constraints - Treat human reviewer feedback with high priority — implement exactly what is asked - Write tests before implementing new behavior (for threads that introduce new functionality) +- For every thread you fix, post a reply naming the specific change before resolving — never resolve silently - Resolve every thread you fix; resolve outdated threads without a corresponding code change - Only resolve threads from human reviewers — do not resolve bot review threads - Do not resolve threads you are intentionally skipping — leave those open and explain why @@ -105,7 +120,7 @@ After applying fixes, output a summary: ``` PR: #${PR_NUMBER} - ${PR_TITLE} Human review threads addressed: N -- Thread : [resolved] +- Thread : [replied + resolved] - Thread : outdated — resolved without change - Thread : skipped — [left open] Test verification: diff --git a/tests/dev-lead/integration/test_prompt_coverage.sh b/tests/dev-lead/integration/test_prompt_coverage.sh index afe544aca..31a8d60c2 100755 --- a/tests/dev-lead/integration/test_prompt_coverage.sh +++ b/tests/dev-lead/integration/test_prompt_coverage.sh @@ -76,6 +76,31 @@ for prompt in "${EXPECTED_PROMPTS[@]}"; do fi done +# ── 4. review prompts must reply-with-specifics, then resolve ──────────────── +# Regression for issue #452: dev-lead resolved (or silently left) review threads +# without replying what it fixed, and bot threads fixed during another +# reviewer's run stayed unresolved. Each review prompt must instruct the engine +# to post a thread reply (addPullRequestReviewThreadReply) describing the +# specific change, then resolve the thread (resolveReviewThread). + +echo "" +echo "Checking review prompts reply-with-specifics then resolve..." +REVIEW_PROMPTS=("fix-reviews.md" "review-changes.md" "fix-bot-comment.md") +for prompt in "${REVIEW_PROMPTS[@]}"; do + path="$PROMPTS_DIR/$prompt" + [ -f "$path" ] || { echo " FAIL: $prompt — missing"; FAILED=1; continue; } + missing="" + grep -q "addPullRequestReviewThreadReply" "$path" || missing="$missing reply-mutation" + grep -q "resolveReviewThread" "$path" || missing="$missing resolve-mutation" + grep -qiE "specific(ally)?" "$path" || missing="$missing specific-details" + if [ -n "$missing" ]; then + echo " FAIL: $prompt — missing:$missing" + FAILED=1 + else + echo " ok: $prompt (replies with specifics, then resolves)" + fi +done + # ── result ──────────────────────────────────────────────────────────────────── echo "" From dd571634123f9a632bca53ca74d59dddd9a977e1 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 04:37:33 +0000 Subject: [PATCH 12/17] fix(reviews): address review comments [skip ci-relay] --- prompts/dev-lead/fix-reviews.md | 4 +-- scripts/dev-lead-fix-reviews.sh | 2 +- scripts/lib/auto-merge.sh | 12 +++++++ tests/dev-lead/unit/test_auto_merge.bats | 41 ++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 3 deletions(-) diff --git a/prompts/dev-lead/fix-reviews.md b/prompts/dev-lead/fix-reviews.md index 880c85bb0..5526c6e20 100644 --- a/prompts/dev-lead/fix-reviews.md +++ b/prompts/dev-lead/fix-reviews.md @@ -77,8 +77,8 @@ gh api graphql -f query='mutation { resolveReviewThread(input: {threadId: "THREA Resolve a thread when **you actually fixed it** (or it is `isOutdated: true`), per this scope: -- **Bot threads** (`author.login` ends with `[bot]`): resolve every one you fixed, **regardless of which reviewer triggered this run**. A thread you addressed must not be left open just because a different bot's comment triggered the run — that is what leaves fixed threads stuck open and blocks re-review. -- **Human threads**: resolve only when `author.login` matches `${TRIGGERING_REVIEWER}`. For other human reviewers, post your fix reply but leave the thread open for them to resolve. +- **Bot threads** (`author.__typename` is `"Bot"` — the GitHub GraphQL API sets this for all bot accounts; note that GraphQL omits the `[bot]` suffix from `author.login` for bots, so the login field alone is not a reliable bot indicator): resolve every one you fixed, **regardless of which reviewer triggered this run**. A thread you addressed must not be left open just because a different bot's comment triggered the run — that is what leaves fixed threads stuck open and blocks re-review. +- **Human threads** (`author.__typename` is `"User"`): resolve only when `author.login` matches `${TRIGGERING_REVIEWER}`. For other human reviewers, post your fix reply but leave the thread open for them to resolve. Never resolve a thread you did not fix (except `isOutdated: true` ones). Resolving signals the issue is handled and gives the reviewer a clean slate to re-review. diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 4937ee1c3..906b762cf 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -831,7 +831,7 @@ case "$INTENT_TYPE" in repository(owner:$owner, name:$repo) { pullRequest(number:$pr) { reviewThreads(first:50) { - nodes { id isResolved isOutdated line path comments(first:5) { nodes { body author { login } } } } + nodes { id isResolved isOutdated line path comments(first:5) { nodes { body author { login __typename } } } } } } } diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh index 53d2f373a..2a8fe7468 100644 --- a/scripts/lib/auto-merge.sh +++ b/scripts/lib/auto-merge.sh @@ -61,6 +61,15 @@ hold_auto_merge() { if gh pr merge "$PR_NUMBER" --repo "$REPO" --disable-auto 2>/dev/null; then _AM_NEEDS_RESTORE=1 else + # An approval may have landed in the window between the auto_merge probe above + # and this call, causing GitHub to merge and close the PR. If so, exit cleanly + # instead of continuing into checkout_pr_in_worktree on a deleted branch. + local merged_state + merged_state=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.state' 2>/dev/null || true) + if [ -n "$merged_state" ] && [ "$merged_state" != "open" ]; then + echo "::notice::PR #${PR_NUMBER} is ${merged_state} — merged/closed before hold could be set; exiting cleanly." + exit 0 + fi echo "::warning::could not disable auto-merge on PR #${PR_NUMBER}" fi } @@ -104,6 +113,9 @@ push_with_merge_guard() { errf="$(mktemp)" if git push "$@" 2>"$errf"; then rm -f "$errf" + # Refresh the held SHA after our own push so the EXIT-trap restore uses + # --match-head-commit against the new head rather than the pre-push SHA. + _AM_HEAD_SHA=$(git rev-parse HEAD 2>/dev/null || true) return 0 fi cat "$errf" >&2 diff --git a/tests/dev-lead/unit/test_auto_merge.bats b/tests/dev-lead/unit/test_auto_merge.bats index 9665734a0..42ea16dd9 100644 --- a/tests/dev-lead/unit/test_auto_merge.bats +++ b/tests/dev-lead/unit/test_auto_merge.bats @@ -243,3 +243,44 @@ EOF ! grep -q -- "--subject" "$GH_CALLS" ! grep -q -- "--body" "$GH_CALLS" } + +# ── hold_auto_merge — disable-auto failure race handling ────────────────────── + +@test "hold_auto_merge: exits 0 cleanly when disable-auto fails because PR was already merged" { + source "$LIB" + AM_STATE="squash" + GH_MERGE_RC=1 # --disable-auto fails + PR_STATE="closed" # PR was merged in the race window + run hold_auto_merge + [ "$status" -eq 0 ] + [[ "$output" == *"merged/closed before hold could be set"* ]] +} + +@test "hold_auto_merge: warns but continues when disable-auto fails and PR is still open" { + source "$LIB" + AM_STATE="squash" + GH_MERGE_RC=1 # --disable-auto fails + PR_STATE="open" # PR is still open — transient API error + run hold_auto_merge + [ "$status" -eq 0 ] + [[ "$output" == *"could not disable auto-merge"* ]] + [[ "$output" != *"exiting cleanly"* ]] +} + +# ── push_with_merge_guard — HEAD SHA refresh after push ─────────────────────── + +@test "push_with_merge_guard: updates _AM_HEAD_SHA to new HEAD after successful push" { + source "$LIB" + cat > "$STUB_BIN_DIR/git" <<'EOF' +#!/usr/bin/env bash +case "$1" in + push) exit 0 ;; + rev-parse) echo "newsha456" ;; + *) exec /usr/bin/git "$@" ;; +esac +EOF + chmod +x "$STUB_BIN_DIR/git" + _AM_HEAD_SHA="oldsha123" + push_with_merge_guard + [ "${_AM_HEAD_SHA}" = "newsha456" ] +} From 01805ea551d4164f90d7b96bd0589de1364ec037 Mon Sep 17 00:00:00 2001 From: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Date: Sat, 6 Jun 2026 23:48:56 -0500 Subject: [PATCH 13/17] fix(dev-lead): restore auto-merge against the PR's current head, re-fetched at restore time (#452) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex follow-up: the rebase intent force-pushes directly from the engine (prompts/dev-lead/rebase.md: `git push --force-with-lease`), bypassing push_with_merge_guard, so _AM_HEAD_SHA is never refreshed and the EXIT-trap restore pins --match-head-commit to the stale hold-time SHA → GitHub rejects it and auto-merge is left off. Make restore_auto_merge re-fetch the PR's CURRENT head at restore time and match against that, falling back to the hold-time SHA only if the re-fetch fails. This subsumes every head-advances-during-run path (own commits, rebase force-push, external pushes, failure-after-push) instead of patching each one. Tests: new cases for current-head preference and the re-fetch-failure fallback. Full suite 295 unit + prompt-coverage + concurrency green; shellcheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/lib/auto-merge.sh | 18 +++++++++++---- tests/dev-lead/unit/test_auto_merge.bats | 28 ++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh index 2a8fe7468..a3b4a77dc 100644 --- a/scripts/lib/auto-merge.sh +++ b/scripts/lib/auto-merge.sh @@ -38,9 +38,10 @@ _AM_MERGE_METHOD="squash" # user's selected merge message is not lost when auto-merge is re-enabled. _AM_COMMIT_TITLE="" _AM_COMMIT_MESSAGE="" -# PR head SHA captured at hold time; used by restore_auto_merge so a stale -# HEAD_SHA from the event payload doesn't cause GitHub to reject the restore -# when the PR has advanced since the event was queued. +# PR head SHA captured at hold time; used by restore_auto_merge only as a +# fallback when re-fetching the PR's current head fails. Restore prefers the +# freshly-fetched head so --match-head-commit matches commits pushed during the +# run (own commits, or the rebase engine's direct force-push). _AM_HEAD_SHA="" # hold_auto_merge — disable auto-merge for the duration of the run, if it is on. @@ -99,7 +100,16 @@ restore_auto_merge() { local restore_args=("--auto" "$merge_flag") [[ -n "${_AM_COMMIT_TITLE:-}" ]] && restore_args+=("--subject" "${_AM_COMMIT_TITLE}") [[ -n "${_AM_COMMIT_MESSAGE:-}" ]] && restore_args+=("--body" "${_AM_COMMIT_MESSAGE}") - [[ -n "${_AM_HEAD_SHA:-}" ]] && restore_args+=("--match-head-commit" "${_AM_HEAD_SHA}") + # Match against the PR's CURRENT head, re-fetched now — not a SHA captured at + # hold time. The head can advance during the run via our own commits, the + # rebase engine's direct `git push --force-with-lease` (prompts/dev-lead/rebase.md, + # which bypasses push_with_merge_guard), or otherwise; a stale + # --match-head-commit makes GitHub reject the restore and leaves auto-merge + # off. Fall back to the hold-time SHA only when the re-fetch fails. + local match_sha + match_sha=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/dev/null || true) + match_sha="${match_sha:-${_AM_HEAD_SHA:-}}" + [[ -n "$match_sha" ]] && restore_args+=("--match-head-commit" "$match_sha") echo "::notice::PR #${PR_NUMBER} — restoring auto-merge (${_AM_MERGE_METHOD:-squash})" gh pr merge "$PR_NUMBER" --repo "$REPO" "${restore_args[@]}" 2>/dev/null \ || echo "::warning::could not restore auto-merge on PR #${PR_NUMBER}" diff --git a/tests/dev-lead/unit/test_auto_merge.bats b/tests/dev-lead/unit/test_auto_merge.bats index 42ea16dd9..20da51927 100644 --- a/tests/dev-lead/unit/test_auto_merge.bats +++ b/tests/dev-lead/unit/test_auto_merge.bats @@ -207,6 +207,34 @@ EOF # ── restore_auto_merge — HEAD_SHA refresh and commit text replay ────────────── +@test "restore_auto_merge: matches the freshly-fetched current head over the hold-time SHA" { + # The head can advance during the run (own commits, or the rebase engine's + # direct force-push that bypasses push_with_merge_guard). Restore must match + # the CURRENT head, not the SHA captured at hold time, or GitHub rejects it. + source "$LIB" + _AM_NEEDS_RESTORE=1 + PR_STATE="open" + AM_STATE="" + _AM_HEAD_SHA="hold_time_sha" + export HEAD_SHA_API="current_head_sha" # PR head advanced since hold + run restore_auto_merge + [ "$status" -eq 0 ] + grep -q -- "--match-head-commit current_head_sha" "$GH_CALLS" + ! grep -q -- "--match-head-commit hold_time_sha" "$GH_CALLS" +} + +@test "restore_auto_merge: falls back to the hold-time SHA when the current-head re-fetch fails" { + source "$LIB" + _AM_NEEDS_RESTORE=1 + PR_STATE="open" + AM_STATE="" + _AM_HEAD_SHA="hold_time_sha" + export HEAD_SHA_API="" # re-fetch returns empty + run restore_auto_merge + [ "$status" -eq 0 ] + grep -q -- "--match-head-commit hold_time_sha" "$GH_CALLS" +} + @test "restore_auto_merge: uses held-head SHA, not stale event-payload HEAD_SHA" { source "$LIB" _AM_NEEDS_RESTORE=1 From 195b77bc150af6534c2100f74395f16c155a521c Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 05:07:30 +0000 Subject: [PATCH 14/17] fix(reviews): address review comments [skip ci-relay] --- prompts/dev-lead/fix-reviews.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/prompts/dev-lead/fix-reviews.md b/prompts/dev-lead/fix-reviews.md index 5526c6e20..03b3bffe9 100644 --- a/prompts/dev-lead/fix-reviews.md +++ b/prompts/dev-lead/fix-reviews.md @@ -77,8 +77,8 @@ gh api graphql -f query='mutation { resolveReviewThread(input: {threadId: "THREA Resolve a thread when **you actually fixed it** (or it is `isOutdated: true`), per this scope: -- **Bot threads** (`author.__typename` is `"Bot"` — the GitHub GraphQL API sets this for all bot accounts; note that GraphQL omits the `[bot]` suffix from `author.login` for bots, so the login field alone is not a reliable bot indicator): resolve every one you fixed, **regardless of which reviewer triggered this run**. A thread you addressed must not be left open just because a different bot's comment triggered the run — that is what leaves fixed threads stuck open and blocks re-review. -- **Human threads** (`author.__typename` is `"User"`): resolve only when `author.login` matches `${TRIGGERING_REVIEWER}`. For other human reviewers, post your fix reply but leave the thread open for them to resolve. +- **Bot threads** (`comments.nodes[0].author.__typename` is `"Bot"` — the GitHub GraphQL API sets this for all bot accounts; note that GraphQL omits the `[bot]` suffix from `comments.nodes[0].author.login` for bots, so the login field alone is not a reliable bot indicator): resolve every one you fixed, **regardless of which reviewer triggered this run**. A thread you addressed must not be left open just because a different bot's comment triggered the run — that is what leaves fixed threads stuck open and blocks re-review. +- **Human threads** (`comments.nodes[0].author.__typename` is `"User"`): resolve only when `comments.nodes[0].author.login` matches `${TRIGGERING_REVIEWER}`. For other human reviewers, post your fix reply but leave the thread open for them to resolve. Never resolve a thread you did not fix (except `isOutdated: true` ones). Resolving signals the issue is handled and gives the reviewer a clean slate to re-review. From 828ab23a3302773373d83d1b82d648fd1c8f881c Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 06:04:01 +0000 Subject: [PATCH 15/17] fix(reviews): address review comments [skip ci-relay] --- scripts/lib/auto-merge.sh | 3 +- .../integration/test_concurrency_config.sh | 34 +++++++++++++++++++ tests/dev-lead/unit/test_auto_merge.bats | 7 ++-- 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh index a3b4a77dc..2e9555000 100644 --- a/scripts/lib/auto-merge.sh +++ b/scripts/lib/auto-merge.sh @@ -71,7 +71,8 @@ hold_auto_merge() { echo "::notice::PR #${PR_NUMBER} is ${merged_state} — merged/closed before hold could be set; exiting cleanly." exit 0 fi - echo "::warning::could not disable auto-merge on PR #${PR_NUMBER}" + echo "::error::could not disable auto-merge on PR #${PR_NUMBER} — aborting to prevent mid-run merge race" >&2 + exit 1 fi } diff --git a/tests/dev-lead/integration/test_concurrency_config.sh b/tests/dev-lead/integration/test_concurrency_config.sh index bb11a73bc..3d546f983 100644 --- a/tests/dev-lead/integration/test_concurrency_config.sh +++ b/tests/dev-lead/integration/test_concurrency_config.sh @@ -34,6 +34,32 @@ check_concurrency_field() { fi } +# check_concurrency_order: verify $before appears at a lower line number than +# $after within the concurrency: block. Presence alone is insufficient for +# short-circuit || chains: swapping two predicates keeps all tokens present +# while silently mis-routing events (e.g. issue.number before issue.pull_request +# would route PR-comment events to the issue lane instead of the PR lane). +check_concurrency_order() { + local file="$1" before="$2" after="$3" label="$4" + local block + block=$(awk '/^concurrency:/{found=1} found && /^[a-z]/ && !/^concurrency:/{found=0} found{print}' "$file" \ + | grep -v '^\s*#') + local line_before line_after + line_before=$(printf '%s\n' "$block" | grep -nF "$before" | head -1 | cut -d: -f1) + line_after=$(printf '%s\n' "$block" | grep -nF "$after" | head -1 | cut -d: -f1) + if [[ -z "$line_before" || -z "$line_after" ]]; then + echo "FAIL [$label]: '$before' or '$after' not found in concurrency YAML of $file" + FAIL=$((FAIL + 1)) + return + fi + if [[ "$line_before" -lt "$line_after" ]]; then + echo "PASS [$label]: '$before' precedes '$after' in concurrency YAML of $(basename "$file")" + else + echo "FAIL [$label]: '$before' must precede '$after' in concurrency YAML of $file" + FAIL=$((FAIL + 1)) + fi +} + # Both the inline caller workflow and the reusable workflow must carry the same # routing, so external callers get identical per-PR serialization. WORKFLOWS=( @@ -53,6 +79,14 @@ for wf in "${WORKFLOWS[@]}"; do # cancel-in-progress:false is what makes same-lane runs QUEUE (serialize) # rather than cancel — dropping queued runs would lose pickups. check_concurrency_field "$wf" "cancel-in-progress: false" "cancel-in-progress is false" + # issue.pull_request must precede issue.number in the || chain: an + # issue_comment on a PR has both issue.pull_request and issue.number truthy, + # so the first match wins — reversing them would route PR comments to the + # issue lane (dev-lead-issue-*) instead of the PR lane (dev-lead-pr-*). + check_concurrency_order "$wf" "issue.pull_request" "dev-lead-issue-" "issue.pull_request before issue lane" + # client_payload.pr_number must precede the run-id fallback so + # repository_dispatch events land in the correct PR lane, not a unique slot. + check_concurrency_order "$wf" "client_payload.pr_number" "github.run_id" "client_payload.pr_number before run-id fallback" done echo "" diff --git a/tests/dev-lead/unit/test_auto_merge.bats b/tests/dev-lead/unit/test_auto_merge.bats index 20da51927..f982e8f07 100644 --- a/tests/dev-lead/unit/test_auto_merge.bats +++ b/tests/dev-lead/unit/test_auto_merge.bats @@ -284,15 +284,14 @@ EOF [[ "$output" == *"merged/closed before hold could be set"* ]] } -@test "hold_auto_merge: warns but continues when disable-auto fails and PR is still open" { +@test "hold_auto_merge: exits 1 (aborts) when disable-auto fails and PR is still open" { source "$LIB" AM_STATE="squash" GH_MERGE_RC=1 # --disable-auto fails - PR_STATE="open" # PR is still open — transient API error + PR_STATE="open" # PR is still open — cannot safely continue with auto-merge enabled run hold_auto_merge - [ "$status" -eq 0 ] + [ "$status" -eq 1 ] [[ "$output" == *"could not disable auto-merge"* ]] - [[ "$output" != *"exiting cleanly"* ]] } # ── push_with_merge_guard — HEAD SHA refresh after push ─────────────────────── From 78ad06457f3d447d1beee5493b4b6d7101b8c6c4 Mon Sep 17 00:00:00 2001 From: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 07:59:32 -0500 Subject: [PATCH 16/17] fix(dev-lead): expose author.__typename in the review-changes thread query too (#452) The fix-reviews OPEN_THREADS_JSON build was updated to select author.__typename (GraphQL omits the [bot] suffix from bot logins, so bots are identified by __typename == "Bot"), but the review-changes build still selected only author.login. Without __typename the review-changes engine cannot reliably tell bots from humans, so its "resolve only human threads, never bots" rule could misclassify coderabbitai / chatgpt-codex-connector / etc. as human. - Add __typename to the review-changes OPEN_THREADS_JSON query. - review-changes.md now identifies humans/bots by author.__typename (User/Bot), with an explicit note not to rely on a [bot] login suffix. - test_prompt_coverage.sh now asserts EVERY OPEN_THREADS_JSON build selects author.__typename, so a future build can't silently omit it. Full unit suite + prompt-coverage + concurrency green; shellcheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- prompts/dev-lead/review-changes.md | 2 +- scripts/dev-lead-fix-reviews.sh | 2 +- .../integration/test_prompt_coverage.sh | 21 +++++++++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/prompts/dev-lead/review-changes.md b/prompts/dev-lead/review-changes.md index 09d8bbdb1..8369787f7 100644 --- a/prompts/dev-lead/review-changes.md +++ b/prompts/dev-lead/review-changes.md @@ -72,7 +72,7 @@ gh api graphql \ #### Resolving a thread -After replying, resolve the thread using the `id` from the JSON above. Only resolve threads from human reviewers — do not resolve threads posted by bots. +After replying, resolve the thread using the `id` from the JSON above. Only resolve threads from human reviewers (`author.__typename` is `"User"`) — do not resolve threads posted by bots (`author.__typename` is `"Bot"`). Identify bots by `__typename`, **not** by a `[bot]` login suffix: GraphQL omits the `[bot]` suffix from bot logins, so a suffix check would misclassify `coderabbitai`, `chatgpt-codex-connector`, etc. as human. ```bash # Replace THREAD_NODE_ID with the id value from the thread JSON diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 906b762cf..641ea9a68 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -928,7 +928,7 @@ case "$INTENT_TYPE" in repository(owner:$owner, name:$repo) { pullRequest(number:$pr) { reviewThreads(first:50) { - nodes { id isResolved isOutdated line path comments(first:5) { nodes { body author { login } } } } + nodes { id isResolved isOutdated line path comments(first:5) { nodes { body author { login __typename } } } } } } } diff --git a/tests/dev-lead/integration/test_prompt_coverage.sh b/tests/dev-lead/integration/test_prompt_coverage.sh index 31a8d60c2..ab0e8ff35 100755 --- a/tests/dev-lead/integration/test_prompt_coverage.sh +++ b/tests/dev-lead/integration/test_prompt_coverage.sh @@ -101,6 +101,27 @@ for prompt in "${REVIEW_PROMPTS[@]}"; do fi done +# ── 5. every OPEN_THREADS_JSON build must expose author.__typename ──────────── +# Regression for issue #452: GraphQL omits the "[bot]" suffix from bot logins, +# so the prompts identify bots by author.__typename == "Bot". Every reviewThreads +# query that feeds OPEN_THREADS_JSON must therefore select __typename — both the +# fix-reviews build AND the review-changes build (the latter was missed). + +echo "" +echo "Checking OPEN_THREADS_JSON queries expose author.__typename..." +DRIVER="$(dirname "$0")/../../../scripts/dev-lead-fix-reviews.sh" +author_sel_total=$(grep -c 'comments(first:5) { nodes { body author {' "$DRIVER" 2>/dev/null || echo 0) +author_sel_typename=$(grep -c 'comments(first:5) { nodes { body author { login __typename } }' "$DRIVER" 2>/dev/null || echo 0) +if [ "$author_sel_total" -eq 0 ]; then + echo " FAIL: no OPEN_THREADS_JSON author selection found in $(basename "$DRIVER")" + FAILED=1 +elif [ "$author_sel_total" -ne "$author_sel_typename" ]; then + echo " FAIL: $((author_sel_total - author_sel_typename)) of $author_sel_total OPEN_THREADS_JSON build(s) omit author.__typename" + FAILED=1 +else + echo " ok: all $author_sel_total OPEN_THREADS_JSON build(s) select author.__typename" +fi + # ── result ──────────────────────────────────────────────────────────────────── echo "" From a901fa5b9d7b9977560f5b41ea019ed5352eb7d6 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 7 Jun 2026 13:08:48 +0000 Subject: [PATCH 17/17] fix(reviews): address review comments [skip ci-relay] --- prompts/dev-lead/review-changes.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prompts/dev-lead/review-changes.md b/prompts/dev-lead/review-changes.md index 8369787f7..2d6bec439 100644 --- a/prompts/dev-lead/review-changes.md +++ b/prompts/dev-lead/review-changes.md @@ -72,7 +72,7 @@ gh api graphql \ #### Resolving a thread -After replying, resolve the thread using the `id` from the JSON above. Only resolve threads from human reviewers (`author.__typename` is `"User"`) — do not resolve threads posted by bots (`author.__typename` is `"Bot"`). Identify bots by `__typename`, **not** by a `[bot]` login suffix: GraphQL omits the `[bot]` suffix from bot logins, so a suffix check would misclassify `coderabbitai`, `chatgpt-codex-connector`, etc. as human. +After replying, resolve the thread using the `id` from the JSON above. Only resolve threads from human reviewers (`comments.nodes[0].author.__typename` is `"User"`) — do not resolve threads posted by bots (`comments.nodes[0].author.__typename` is `"Bot"`). Identify bots by `__typename`, **not** by a `[bot]` login suffix: GraphQL omits the `[bot]` suffix from bot logins, so a suffix check would misclassify `coderabbitai`, `chatgpt-codex-connector`, etc. as human. ```bash # Replace THREAD_NODE_ID with the id value from the thread JSON