From bb2af8db10f5fa4ea9e8960671c06347852ceacd Mon Sep 17 00:00:00 2001
From: openhands <openhands@all-hands.dev>
Date: Wed, 18 Mar 2026 19:53:26 +0000
Subject: [PATCH] =?UTF-8?q?fix:=20address=20review=20feedback=20=E2=80=94?=
 =?UTF-8?q?=20set=20-e=20bug,=20sentinel=20path,=20fragile=20grep,=20stale?=
 =?UTF-8?q?=20comment=20(#171)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Fix set -e bug: use `_merge_rc=0; do_merge ... || _merge_rc=$?` so non-zero
  returns don't kill the agent before _merge_rc is captured
- Fix sentinel path: skip sentinel break for APPROVE so do_merge() always runs,
  even when review-poll.sh injected the verdict first
- Fix fragile grep: match HTTP 405 alone instead of `grep -qi "not enough"` —
  any 405 from the merge endpoint is a structural block (approvals, branch
  protection), not a transient error
- Fix stale comment/status in PHASE:done handler: "orchestrator or Claude"
  instead of "agent"

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---
 dev/phase-handler.sh | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/dev/phase-handler.sh b/dev/phase-handler.sh
index 17b9331..926deb6 100644
--- a/dev/phase-handler.sh
+++ b/dev/phase-handler.sh
@@ -42,11 +42,11 @@ do_merge() {
     return 0
   fi
 
-  # HTTP 405 "not enough approvals" — structural, not transient — escalate immediately
-  if [ "$merge_http_code" = "405" ] && echo "$merge_body" | grep -qi "not enough"; then
-    log "do_merge: PR #${pr_num} blocked — not enough approvals (HTTP 405): ${merge_body:0:200}"
+  # HTTP 405 — merge requirements not met (approvals, branch protection); structural, not transient
+  if [ "$merge_http_code" = "405" ]; then
+    log "do_merge: PR #${pr_num} blocked — merge requirements not met (HTTP 405): ${merge_body:0:200}"
     printf 'PHASE:needs_human\nReason: %s\n' \
-      "PR #${pr_num} merge blocked — not enough approvals (HTTP 405): ${merge_body:0:200}" \
+      "PR #${pr_num} merge blocked — merge requirements not met (HTTP 405): ${merge_body:0:200}" \
       > "$PHASE_FILE"
     return 2
   fi
@@ -366,19 +366,22 @@ Instructions:
           [ -n "$VERDICT" ] && log "verdict from formal review: $VERDICT"
         fi
 
-        # Skip injection if review-poll.sh already injected (sentinel present)
+        # Skip injection if review-poll.sh already injected (sentinel present).
+        # Exception: APPROVE always falls through so do_merge() runs even when
+        # review-poll injected first — prevents Claude writing PHASE:done on a
+        # failed merge without the orchestrator detecting the error.
         REVIEW_SENTINEL="/tmp/review-injected-${PROJECT_NAME}-${PR_NUMBER}"
-        if [ -n "$VERDICT" ] && [ -f "$REVIEW_SENTINEL" ]; then
+        if [ -n "$VERDICT" ] && [ -f "$REVIEW_SENTINEL" ] && [ "$VERDICT" != "APPROVE" ]; then
           log "review already injected by review-poll (sentinel exists) — skipping"
           rm -f "$REVIEW_SENTINEL"
           REVIEW_FOUND=true
           break
         fi
+        rm -f "$REVIEW_SENTINEL"  # consume sentinel before APPROVE handling below
 
         if [ "$VERDICT" = "APPROVE" ]; then
           REVIEW_FOUND=true
-          do_merge "$PR_NUMBER"
-          _merge_rc=$?
+          _merge_rc=0; do_merge "$PR_NUMBER" || _merge_rc=$?
           if [ "$_merge_rc" -eq 0 ]; then
             # Merge succeeded — close issue and signal done
             curl -sf -X PATCH \
@@ -499,11 +502,11 @@ Instructions:
     # Don't inject anything — supervisor-poll.sh (#81) injects human replies, gardener-poll.sh as backup
 
   # ── PHASE: done ─────────────────────────────────────────────────────────────
-  # The agent already merged the PR and closed the issue. Just clean up local state.
+  # PR merged and issue closed (by orchestrator or Claude). Just clean up local state.
   elif [ "$phase" = "PHASE:done" ]; then
-    status "phase done — agent merged PR #${PR_NUMBER:-?}, cleaning up"
+    status "phase done — PR #${PR_NUMBER:-?} merged, cleaning up"
 
-    # Notify Matrix (agent already closed the issue and removed labels via API)
+    # Notify Matrix (issue already closed and labels removed via API)
     notify_ctx \
       "✅ PR #${PR_NUMBER:-?} merged! Issue #${ISSUE} done." \
       "✅ PR <a href='${CODEBERG_WEB}/pulls/${PR_NUMBER:-?}'>#${PR_NUMBER:-?}</a> merged! <a href='${CODEBERG_WEB}/issues/${ISSUE}'>Issue #${ISSUE}</a> done."