fix(dev-lead): hold auto-merge while working a PR + push defensively (#452)

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
-- <issue description>: <fix applied> [thread resolved]
+- <issue description>: <fix applied> [replied + thread resolved]
 - <issue description>: outdated thread resolved
 Files changed: <list of files>
 Skipped (informational): <count>

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** (`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.
 
 ### 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 <id>: <brief description of fix> [resolved]
+- Thread <id>: <brief description of fix> [replied + resolved]
 - Thread <id>: outdated — resolved without change
 - Thread <id>: skipped — <reason> [left open]
 Test verification: <pass/fail — paste output if relevant>

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 (`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
@@ -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 <author>: <brief description of change> [resolved]
+- Thread <author>: <brief description of change> [replied + resolved]
 - Thread <author>: outdated — resolved without change
 - Thread <author>: skipped — <reason> [left open]
 Test verification: <pass/fail — paste output if relevant>

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:-}"
@@ -192,6 +193,16 @@ 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
@@ -268,7 +279,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

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:-}"
@@ -26,17 +27,24 @@ 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).
 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
+  # 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
@@ -501,7 +509,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
@@ -523,8 +531,17 @@ 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 "${_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[@]}"
@@ -592,10 +609,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
 }
@@ -815,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 } } } }
             }
           }
         }
@@ -912,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 } } } }
             }
           }
         }