From f66fcd666cc5a0bb7a71aff6363af1db1e06fdf4 Mon Sep 17 00:00:00 2001
From: openhands <openhands@all-hands.dev>
Date: Fri, 20 Mar 2026 01:52:46 +0000
Subject: [PATCH] =?UTF-8?q?fix:=20address=20review=20=E2=80=94=20terminal?=
 =?UTF-8?q?=20phase=20guard,=20explicit=20marker=20var,=20test=20coverage?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Guard against overwriting terminal phases (PHASE:done, PHASE:merged)
  in on-stop-failure.sh to prevent false failures from same-turn race
- Declare sf_phase_marker explicitly in StopFailure block instead of
  relying on phase_marker from PostToolUse block
- Add authentication_failed test (10c) and terminal phase guard tests
  (10g, 10h)
- Fix fragile nested command substitution in test 10f fail() message

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 dev/phase-test.sh            | 45 ++++++++++++++++++++++++++++++++----
 lib/agent-session.sh         |  5 +++-
 lib/hooks/on-stop-failure.sh |  8 +++++++
 3 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/dev/phase-test.sh b/dev/phase-test.sh
index 6c07c6c..15c59b8 100755
--- a/dev/phase-test.sh
+++ b/dev/phase-test.sh
@@ -260,7 +260,18 @@ if [ -x "$STOP_FAILURE_HOOK" ]; then
   fi
   rm -f "$SF_MARKER" "$PHASE_FILE"
 
-  # 10c: missing phase_file arg → no-op (exit 0, no crash)
+  # 10c: authentication_failed stop reason
+  printf '{"stop_reason":"authentication_failed"}' \
+    | "$STOP_FAILURE_HOOK" "$PHASE_FILE" "$SF_MARKER"
+  sf_second=$(sed -n '2p' "$PHASE_FILE" 2>/dev/null)
+  if echo "$sf_second" | grep -q "api_error: authentication_failed"; then
+    ok "StopFailure hook writes api_error: authentication_failed"
+  else
+    fail "StopFailure hook authentication_failed: got '$sf_second'"
+  fi
+  rm -f "$SF_MARKER" "$PHASE_FILE"
+
+  # 10e: missing phase_file arg → no-op (exit 0, no crash)
   printf '{"stop_reason":"rate_limit"}' | "$STOP_FAILURE_HOOK" "" "$SF_MARKER"
   if [ ! -f "$PHASE_FILE" ]; then
     ok "StopFailure hook no-ops when phase_file is empty"
@@ -269,16 +280,42 @@ if [ -x "$STOP_FAILURE_HOOK" ]; then
   fi
   rm -f "$SF_MARKER"
 
-  # 10d: missing marker arg → phase file still written, no marker
+  # 10f: missing marker arg → phase file still written, no marker
   printf '{"stop_reason":"billing_error"}' \
     | "$STOP_FAILURE_HOOK" "$PHASE_FILE" ""
   sf_first=$(head -1 "$PHASE_FILE" 2>/dev/null)
-  if [ "$sf_first" = "PHASE:failed" ] && [ ! -f "$SF_MARKER" ]; then
+  sf_marker_exists="no"
+  [ -f "$SF_MARKER" ] && sf_marker_exists="yes"
+  if [ "$sf_first" = "PHASE:failed" ] && [ "$sf_marker_exists" = "no" ]; then
     ok "StopFailure hook writes phase without marker when marker arg is empty"
   else
-    fail "StopFailure hook: first='$sf_first' marker_exists=$([ -f "$SF_MARKER" ] && echo yes || echo no)"
+    fail "StopFailure hook: first='$sf_first' marker_exists=$sf_marker_exists"
   fi
   rm -f "$PHASE_FILE"
+
+  # 10g: terminal phase guard — does not overwrite PHASE:done
+  echo "PHASE:done" > "$PHASE_FILE"
+  printf '{"stop_reason":"rate_limit"}' \
+    | "$STOP_FAILURE_HOOK" "$PHASE_FILE" "$SF_MARKER"
+  sf_first=$(head -1 "$PHASE_FILE" 2>/dev/null)
+  if [ "$sf_first" = "PHASE:done" ] && [ ! -f "$SF_MARKER" ]; then
+    ok "StopFailure hook does not overwrite terminal PHASE:done"
+  else
+    fail "StopFailure hook overwrote PHASE:done: first='$sf_first'"
+  fi
+  rm -f "$SF_MARKER" "$PHASE_FILE"
+
+  # 10h: terminal phase guard — does not overwrite PHASE:merged
+  echo "PHASE:merged" > "$PHASE_FILE"
+  printf '{"stop_reason":"server_error"}' \
+    | "$STOP_FAILURE_HOOK" "$PHASE_FILE" "$SF_MARKER"
+  sf_first=$(head -1 "$PHASE_FILE" 2>/dev/null)
+  if [ "$sf_first" = "PHASE:merged" ] && [ ! -f "$SF_MARKER" ]; then
+    ok "StopFailure hook does not overwrite terminal PHASE:merged"
+  else
+    fail "StopFailure hook overwrote PHASE:merged: first='$sf_first'"
+  fi
+  rm -f "$SF_MARKER" "$PHASE_FILE"
 else
   fail "StopFailure hook script not found or not executable: $STOP_FAILURE_HOOK"
 fi
diff --git a/lib/agent-session.sh b/lib/agent-session.sh
index 04638bb..2377f7e 100644
--- a/lib/agent-session.sh
+++ b/lib/agent-session.sh
@@ -130,7 +130,10 @@ create_agent_session() {
   if [ -n "$phase_file" ]; then
     local stop_failure_hook_script="${FACTORY_ROOT}/lib/hooks/on-stop-failure.sh"
     if [ -x "$stop_failure_hook_script" ]; then
-      local stop_failure_hook_cmd="${stop_failure_hook_script} ${phase_file} ${phase_marker}"
+      # phase_marker is defined in the PostToolUse block above; redeclare so
+      # this block is self-contained if that block is ever removed.
+      local sf_phase_marker="/tmp/phase-changed-${session}.marker"
+      local stop_failure_hook_cmd="${stop_failure_hook_script} ${phase_file} ${sf_phase_marker}"
       if [ -f "$settings" ]; then
         jq --arg cmd "$stop_failure_hook_cmd" '
           if (.hooks.StopFailure // [] | any(.[]; .hooks[]?.command == $cmd))
diff --git a/lib/hooks/on-stop-failure.sh b/lib/hooks/on-stop-failure.sh
index 01fb484..9c61648 100755
--- a/lib/hooks/on-stop-failure.sh
+++ b/lib/hooks/on-stop-failure.sh
@@ -25,6 +25,14 @@ reason=$(printf '%s' "$input" | jq -r '
 ' 2>/dev/null)
 [ -z "$reason" ] && reason="unknown"
 
+# Guard: do not overwrite a terminal phase. If Claude wrote PHASE:done via a
+# tool call and then hit a rate limit while generating the rest of its response,
+# the PostToolUse hook already recorded the correct terminal phase.
+existing=$(head -1 "$phase_file" 2>/dev/null | tr -d '[:space:]')
+case "$existing" in
+  PHASE:done|PHASE:merged) exit 0 ;;
+esac
+
 # Write phase file immediately — orchestrator reads first line as phase sentinel
 printf 'PHASE:failed\nReason: api_error: %s\n' "$reason" > "$phase_file"