fix(#159): retry fullsend post-review on transient API failures

docs/review.md

@@ -76,6 +76,7 @@ See [Customizing with AGENTS.md](https://fullsend.sh/docs/guides/user/customizin
 | Variable | Description | Default | Valid values |
 |----------|-------------|---------|--------------|
 | `REVIEW_FINDING_SEVERITY_THRESHOLD` | Minimum severity for findings to include in the review. Findings below this level are omitted from both the narrative body and the posted inline comments. | `low` | `info`, `low`, `medium`, `high`, `critical` |
+| `POST_REVIEW_RETRY_DELAY` | Override backoff seconds for all `fullsend post-review` retry attempts. When unset, the post-script uses progressive backoff (5 s on the first retry, 15 s on the second). Set to `0` in tests to skip sleep. | Progressive (5 s / 15 s) | Any non-negative integer |
 
 Set this in the CI workflow `env:` block. The env file passes it to the
 sandbox automatically, and the post-script reads it from the runner

scripts/post-review-test.sh

@@ -719,6 +719,167 @@ run_label_test_no_pattern "no-op-skip-ready-for-merge-removal" \
   '{"action":"approve","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"LGTM"}' \
   "--remove-label ready-for-merge"
 
+# ---------------------------------------------------------------------------
+# Retry logic tests
+#
+# These tests verify that fullsend post-review is retried on transient
+# failures and that the degraded-mode label fallback works when retries
+# are exhausted.
+# ---------------------------------------------------------------------------
+
+# Create retry-aware mock directory with a fullsend binary that tracks
+# call counts and returns configured exit codes per invocation.
+RETRY_MOCK_BIN="${TMPDIR}/retry-bin"
+mkdir -p "${RETRY_MOCK_BIN}"
+
+# Copy the gh mock (it has GH_LOG baked in from the heredoc expansion).
+cp "${MOCK_BIN}/gh" "${RETRY_MOCK_BIN}/gh"
+
+# Retry-aware fullsend mock. Uses:
+#   MOCK_FULLSEND_COUNTER    — path to a file tracking invocation count
+#   MOCK_FULLSEND_EXIT_CODES — comma-separated exit codes per invocation
+# The heredoc is unquoted so ${GH_LOG} is baked in at creation time;
+# runtime variables are escaped with \$.
+cat > "${RETRY_MOCK_BIN}/fullsend" <<RETRYMOCKEOF
+#!/usr/bin/env bash
+if [[ "\$1" == "post-review" ]]; then
+  if [ -n "\${MOCK_FULLSEND_COUNTER:-}" ]; then
+    COUNT=0
+    if [ -f "\${MOCK_FULLSEND_COUNTER}" ]; then
+      COUNT=\$(<"\${MOCK_FULLSEND_COUNTER}")
+    fi
+    COUNT=\$((COUNT + 1))
+    echo "\${COUNT}" > "\${MOCK_FULLSEND_COUNTER}"
+
+    IFS=',' read -ra EXIT_CODES <<< "\${MOCK_FULLSEND_EXIT_CODES:-0}"
+    IDX=\$((COUNT - 1))
+    if [ "\${IDX}" -lt "\${#EXIT_CODES[@]}" ]; then
+      exit "\${EXIT_CODES[\$IDX]}"
+    fi
+    exit "\${EXIT_CODES[\${#EXIT_CODES[@]}-1]}"
+  fi
+fi
+echo "fullsend \$*" >> "${GH_LOG}"
+RETRYMOCKEOF
+chmod +x "${RETRY_MOCK_BIN}/fullsend"
+
+run_retry_test() {
+  local test_name="$1"
+  local json_content="$2"
+  local exit_codes="$3"        # comma-separated fullsend exit codes per attempt
+  local expected_exit="$4"     # expected script exit code
+  local expected_calls="$5"    # expected number of fullsend post-review calls
+  local expected_stdout="${6:-}"  # optional: pattern expected in stdout
+  local expected_gh_log="${7:-}"  # optional: pattern expected in GH_LOG
+
+  local run_dir="${TMPDIR}/run-${test_name}"
+  mkdir -p "${run_dir}/iteration-1/output"
+  echo "${json_content}" > "${run_dir}/iteration-1/output/agent-result.json"
+  : > "${GH_LOG}"
+
+  local counter_file="${TMPDIR}/counter-${test_name}"
+  rm -f "${counter_file}"
+
+  local exit_code=0
+  # shellcheck disable=SC2030,SC2031
+  (
+    cd "${run_dir}"
+    export PATH="${RETRY_MOCK_BIN}:${PATH}"
+    export REVIEW_TOKEN="fake-token"
+    export PR_NUMBER="99"
+    export REPO_FULL_NAME="test-org/test-repo"
+    export POST_REVIEW_RETRY_DELAY=0
+    export MOCK_FULLSEND_COUNTER="${counter_file}"
+    export MOCK_FULLSEND_EXIT_CODES="${exit_codes}"
+    bash "${POST_SCRIPT}"
+  ) > "${TMPDIR}/stdout-${test_name}.log" 2>&1 || exit_code=$?
+
+  if [ "${exit_code}" -ne "${expected_exit}" ]; then
+    echo "FAIL: ${test_name} — expected exit ${expected_exit}, got ${exit_code}"
+    cat "${TMPDIR}/stdout-${test_name}.log"
+    FAILURES=$((FAILURES + 1))
+    return
+  fi
+
+  local actual_count=0
+  if [ -f "${counter_file}" ]; then
+    actual_count=$(<"${counter_file}")
+  fi
+
+  if [ "${actual_count}" -ne "${expected_calls}" ]; then
+    echo "FAIL: ${test_name} — expected ${expected_calls} fullsend calls, got ${actual_count}"
+    cat "${TMPDIR}/stdout-${test_name}.log"
+    FAILURES=$((FAILURES + 1))
+    return
+  fi
+
+  if [ -n "${expected_stdout}" ]; then
+    if ! grep -qF -- "${expected_stdout}" "${TMPDIR}/stdout-${test_name}.log"; then
+      echo "FAIL: ${test_name} — expected stdout '${expected_stdout}' not found"
+      echo "Actual stdout:"
+      cat "${TMPDIR}/stdout-${test_name}.log"
+      FAILURES=$((FAILURES + 1))
+      return
+    fi
+  fi
+
+  if [ -n "${expected_gh_log}" ]; then
+    if ! grep -qF -- "${expected_gh_log}" "${GH_LOG}"; then
+      echo "FAIL: ${test_name} — expected gh log '${expected_gh_log}' not found"
+      echo "Actual gh log:"
+      cat "${GH_LOG}"
+      FAILURES=$((FAILURES + 1))
+      return
+    fi
+  fi
+
+  echo "PASS: ${test_name}"
+}
+
+# --- Retry test cases ---
+
+# Retry then succeed: fails twice, succeeds on 3rd attempt → exit 0, label applied
+run_retry_test "retry-then-succeed" \
+  '{"action":"approve","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"LGTM"}' \
+  "1,1,0" "0" "3" \
+  "retrying in" "--add-label ready-for-merge"
+
+# Retries exhausted + fallback (approve): all 3 fail → exit 1, fallback label applied
+run_retry_test "retries-exhausted-fallback-approve" \
+  '{"action":"approve","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"LGTM"}' \
+  "1,1,1" "1" "3" \
+  "Degraded-mode fallback: applied ready-for-merge" "--add-label ready-for-merge"
+
+# Retries exhausted + fallback (comment): all 3 fail → requires-manual-review
+run_retry_test "retries-exhausted-fallback-comment" \
+  '{"action":"comment","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"Split review"}' \
+  "1,1,1" "1" "3" \
+  "Degraded-mode fallback: applied requires-manual-review" "--add-label requires-manual-review"
+
+# Retries exhausted (request-changes): no fallback label applicable
+run_retry_test "retries-exhausted-no-fallback-label" \
+  '{"action":"request-changes","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"Changes needed","findings":[{"severity":"high","category":"bug","file":"main.go","description":"nil deref"}]}' \
+  "1,1,1" "1" "3" \
+  "Degraded-mode label fallback not applicable"
+
+# Stale-head bypass: exit code 10 → no retry, stale-head handler runs
+run_retry_test "stale-head-no-retry" \
+  '{"action":"approve","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"LGTM"}' \
+  "10" "0" "1" \
+  "Stale-head detected"
+
+# Retry logging: fails once then succeeds → verify log format
+run_retry_test "retry-logging" \
+  '{"action":"approve","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"LGTM"}' \
+  "1,0" "0" "2" \
+  "attempt 1/3 failed (exit 1)"
+
+# All-retries-exhausted logging: verify exhaustion message
+run_retry_test "retry-exhaustion-logging" \
+  '{"action":"approve","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"LGTM"}' \
+  "1,1,1" "1" "3" \
+  "all retries exhausted"
+
 # --- Summary ---
 
 echo ""

scripts/post-review.sh

@@ -309,16 +309,57 @@ if [[ "${HAS_LABEL_ACTIONS}" == "true" ]]; then
 fi
 
 # ---------------------------------------------------------------------------
-# Post the review. Exit code 10 = stale-head: the PR HEAD moved after the
-# agent reviewed it. When this happens, post a /fs-review comment to
-# re-dispatch a fresh review for the current HEAD.
+# Post the review with retry logic. Transient GitHub API errors (e.g. 422
+# during PR transitional states) can cause fullsend post-review to fail even
+# though the review comment was posted successfully. Retry with backoff to
+# handle these transient failures.
+#
+# Exit code 10 = stale-head: bypasses retry, handled separately below.
+# Other non-zero exit codes are retried up to POST_REVIEW_MAX_ATTEMPTS times.
+# If all retries are exhausted, attempt degraded-mode label fallback so the
+# PR is not left without an outcome label.
+#
+# Environment:
+#   POST_REVIEW_RETRY_DELAY — override backoff seconds for all retries
+#                             (default: 5s first retry, 15s second retry;
+#                              set to 0 in tests to skip sleep)
 # ---------------------------------------------------------------------------
+POST_REVIEW_MAX_ATTEMPTS=3
+
 POST_REVIEW_EXIT=0
-fullsend post-review \
-  --repo "${REPO_FULL_NAME}" \
-  --pr "${PR_NUMBER}" \
-  --token "${REVIEW_TOKEN}" \
-  --result "${RESULT_FILE}" || POST_REVIEW_EXIT=$?
+for _pr_attempt in $(seq 1 "${POST_REVIEW_MAX_ATTEMPTS}"); do
+  POST_REVIEW_EXIT=0
+  fullsend post-review \
+    --repo "${REPO_FULL_NAME}" \
+    --pr "${PR_NUMBER}" \
+    --token "${REVIEW_TOKEN}" \
+    --result "${RESULT_FILE}" || POST_REVIEW_EXIT=$?
+
+  # Exit code 10 = stale-head: bypass retry, handle below
+  if [ "${POST_REVIEW_EXIT}" -eq 10 ]; then
+    break
+  fi
+
+  # Success: no retry needed
+  if [ "${POST_REVIEW_EXIT}" -eq 0 ]; then
+    break
+  fi
+
+  # Non-zero, non-stale-head: log and retry if attempts remain
+  if [ "${_pr_attempt}" -lt "${POST_REVIEW_MAX_ATTEMPTS}" ]; then
+    if [ -n "${POST_REVIEW_RETRY_DELAY+x}" ]; then
+      _backoff="${POST_REVIEW_RETRY_DELAY}"
+    elif [ "${_pr_attempt}" -eq 1 ]; then
+      _backoff=5
+    else
+      _backoff=15
+    fi
+    echo "::warning::fullsend post-review attempt ${_pr_attempt}/${POST_REVIEW_MAX_ATTEMPTS} failed (exit ${POST_REVIEW_EXIT}) — retrying in ${_backoff}s"
+    sleep "${_backoff}"
+  else
+    echo "::warning::fullsend post-review attempt ${_pr_attempt}/${POST_REVIEW_MAX_ATTEMPTS} failed (exit ${POST_REVIEW_EXIT}) — all retries exhausted"
+  fi
+done
 
 if [ "${POST_REVIEW_EXIT}" -eq 10 ]; then
   echo "Stale-head detected — checking whether to re-dispatch review"
@@ -347,8 +388,91 @@ ${REDISPATCH_MARKER}" || echo "::warning::Failed to post re-dispatch comment"
   # appear as a failure.
   exit 0
 elif [ "${POST_REVIEW_EXIT}" -ne 0 ]; then
-  echo "::error::fullsend post-review failed with exit code ${POST_REVIEW_EXIT} (PR #${PR_NUMBER} in ${REPO_FULL_NAME})" >&2
-  exit "${POST_REVIEW_EXIT}"
+  echo "::error::fullsend post-review failed after ${POST_REVIEW_MAX_ATTEMPTS} attempts (exit ${POST_REVIEW_EXIT}, PR #${PR_NUMBER} in ${REPO_FULL_NAME})" >&2
+
+  # Degraded-mode fallback: apply the outcome label directly so the PR is
+  # not left in limbo without a label.
+  #
+  # Assumption: fullsend post-review is a two-step operation (post comment,
+  # then submit formal review). A transient API failure may occur between
+  # the steps, leaving the comment posted but the formal review missing.
+  # We cannot distinguish "comment posted, review failed" from "nothing
+  # posted" because fullsend post-review does not emit distinct exit codes
+  # for each case (see issue #159 requirement 4 — HTTP status codes and
+  # response bodies are not exposed by the CLI). Applying the label here
+  # is a best-effort measure — exit 1 still signals CI failure so a human
+  # can verify.
+  echo "Attempting degraded-mode label fallback..."
+  _fallback_applied=false
+
+  # Determine the target fallback label so we can skip removing it
+  # (avoids a pointless unlabel/relabel cycle — mirrors normal path).
+  # Label logic mirrors the outcome-label block below — keep in sync.
+  _fallback_label=""
+  if [ "${ACTION}" = "approve" ] && [ "${DOWNGRADED}" = "false" ] && [ "${PR_IS_DRAFT}" != "true" ]; then
+    _fallback_label="ready-for-merge"
+  elif { [ "${ACTION}" = "approve" ] && { [ "${DOWNGRADED}" = "true" ] || [ "${PR_IS_DRAFT}" = "true" ]; }; } || \
+       [ "${ACTION}" = "comment" ]; then
+    _fallback_label="requires-manual-review"
+  elif [ "${ACTION}" = "reject" ]; then
+    _fallback_label="rejected"
+  fi
+
+  # Remove stale outcome labels, skipping the one about to be applied.
+  for _stale in "ready-for-merge" "requires-manual-review" "rejected"; do
+    [ "${_stale}" = "${_fallback_label}" ] && continue
+    gh pr edit "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" \
+      --remove-label "${_stale}" 2>/dev/null || true
+  done
+  if [ "${ACTION}" = "approve" ] && [ "${DOWNGRADED}" = "false" ] && [ "${PR_IS_DRAFT}" != "true" ]; then
+    gh label create "ready-for-merge" --repo "${REPO_FULL_NAME}" \
+      --description "All reviewers approved — ready to merge" --color "0E8A16" \
+      2>/dev/null || true
+    if gh pr edit "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" \
+      --add-label "ready-for-merge"; then
+      _fallback_applied=true
+      echo "Degraded-mode fallback: applied ready-for-merge label"
+    fi
+  elif { [ "${ACTION}" = "approve" ] && { [ "${DOWNGRADED}" = "true" ] || [ "${PR_IS_DRAFT}" = "true" ]; }; } || \
+       [ "${ACTION}" = "comment" ]; then
+    gh label create "requires-manual-review" --repo "${REPO_FULL_NAME}" \
+      --description "Review requires human judgment" --color "FBCA04" \
+      2>/dev/null || true
+    if gh pr edit "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" \
+      --add-label "requires-manual-review"; then
+      _fallback_applied=true
+      echo "Degraded-mode fallback: applied requires-manual-review label"
+    fi
+  elif [ "${ACTION}" = "reject" ]; then
+    # NOTE: The normal path closes the PR before applying the rejected
+    # label (gh pr close with a comment). In degraded mode, closing is
+    # intentionally omitted as a conservative measure — without
+    # confirmation that the formal review was posted, closing the PR
+    # would be a destructive action based on uncertain state. The exit 1
+    # ensures CI failure so a human can intervene.
+    gh label create "rejected" --repo "${REPO_FULL_NAME}" \
+      --description "Approach rejected by review agent" --color "B60205" \
+      2>/dev/null || true
+    if gh pr edit "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" \
+      --add-label "rejected"; then
+      _fallback_applied=true
+      echo "Degraded-mode fallback: applied rejected label"
+    fi
+  fi
+
+  # Sanitize ACTION for GHA workflow command output (defense-in-depth,
+  # matches the pattern used for label-action values above).
+  _safe_action="${ACTION//$'\n'/}"
+  _safe_action="${_safe_action//$'\r'/}"
+  _safe_action="${_safe_action//::/:}"
+
+  if [ "${_fallback_applied}" = "true" ]; then
+    echo "::warning::Formal review failed but outcome label applied via degraded-mode fallback"
+  else
+    echo "::warning::Degraded-mode label fallback not applicable (action=${_safe_action})"
+  fi
+
+  exit 1
 fi
 
 # ---------------------------------------------------------------------------