fix(#2093): retry empty PR file list before refusing to approve

scripts/lib/github-review-ops.lib.sh

@@ -51,8 +51,18 @@ forge_get_pr_info() {
 }
 
 forge_get_pr_files() {
-  GH_TOKEN="${REVIEW_TOKEN}" gh pr view "${PR_NUMBER}" \
-    --repo "${REPO}" --json files --jq '.files[].path'
+  # Use the paginated /pulls/{n}/files REST endpoint rather than the
+  # `gh pr view --json files` summary field: issue #2093 found empty
+  # results correlated with recent merge-commit updates and hypothesized
+  # asynchronous diff computation, but GitHub does not document that as
+  # an API contract. The files endpoint reflects the computed diff more
+  # directly.
+  local files
+  if ! files=$(GH_TOKEN="${REVIEW_TOKEN}" gh api \
+    "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' 2>/dev/null); then
+    return 1
+  fi
+  [[ -n "${files}" ]] && printf '%s\n' "${files}"
 }
 
 # --- PR mutations ---

scripts/post-review-test.sh

@@ -442,10 +442,38 @@ if [[ "\$1" == "pr" ]] && [[ "\$2" == "view" ]] && [[ "\$*" == *"--json state"*
   exit 0
 fi
 
-# gh pr view ... --json files ... → configurable via MOCK_PR_FILES.
-# Uses \${VAR-default} (not \${VAR:-default}) so an explicitly-empty
+# gh api repos/.../pulls/{n}/files --paginate --jq '.[].filename'
+# → configurable via MOCK_PR_FILES (the mock emits the already-jq'd
+# filename list, matching what forge_get_pr_files consumes). Uses
+# \${VAR-default} (not \${VAR:-default}) so an explicitly-empty
 # MOCK_PR_FILES="" can simulate "no changed files" instead of falling
 # back to the default.
+#
+# MOCK_PR_FILES_ON_RETRY, when set, makes the FIRST call return an empty
+# list and later calls return its value — simulating the transient
+# forge data race in fullsend-ai/fullsend#2093 that the call-site retry
+# recovers from. MOCK_FILES_CALL_MARKER tracks whether the first call
+# has happened; the retry test resets it before running.
+if [[ "\$1" == "api" ]] && [[ "\$*" == *"/pulls/"* ]] && [[ "\$*" == *"/files"* ]]; then
+  if [[ -n "\${MOCK_PR_FILES_FAIL:-}" ]]; then
+    echo "src/partial-before-fetch-failure.go"
+    echo "mock gh api failure" >&2
+    exit 1
+  fi
+  if [[ -n "\${MOCK_PR_FILES_ON_RETRY:-}" ]]; then
+    if [[ -f "\${MOCK_FILES_CALL_MARKER:-${TMPDIR}/pr-files-call-marker}" ]]; then
+      echo "\${MOCK_PR_FILES_ON_RETRY}"
+    else
+      : > "\${MOCK_FILES_CALL_MARKER:-${TMPDIR}/pr-files-call-marker}"
+    fi
+    exit 0
+  fi
+  echo "\${MOCK_PR_FILES-src/main.go}"
+  exit 0
+fi
+
+# gh pr view ... --json files ... → legacy summary path, retained for any
+# caller still using it. Configurable via MOCK_PR_FILES (see above).
 if [[ "\$1" == "pr" ]] && [[ "\$2" == "view" ]] && [[ "\$*" == *"--json files"* ]]; then
   echo "\${MOCK_PR_FILES-src/main.go}"
   exit 0
@@ -474,6 +502,16 @@ echo "gh \$*" >> "${GH_LOG}"
 MOCKEOF
 chmod +x "${MOCK_BIN}/gh"
 
+# Mock sleep: no-op. The empty-PR-files retry branch in post-review.sh
+# calls `sleep 10` before re-fetching; without this mock the real sleep
+# runs in every empty-list integration test, adding ~10s each to a
+# serial suite run. The retry logic doesn't depend on real elapsed time.
+cat > "${MOCK_BIN}/sleep" <<'MOCKEOF'
+#!/usr/bin/env bash
+exit 0
+MOCKEOF
+chmod +x "${MOCK_BIN}/sleep"
+
 cat > "${MOCK_BIN}/fullsend" <<MOCKEOF
 #!/usr/bin/env bash
 # Mock fullsend: log the call, consume stdin if --result - is used,
@@ -536,6 +574,10 @@ fi
 
 # GET /merge_requests/:iid/changes → changed files
 if [[ "\${URL}" == *"/changes"* ]]; then
+  if [[ -n "\${MOCK_MR_FILES_FAIL:-}" ]]; then
+    echo "mock curl failure" >&2
+    exit 1
+  fi
   echo '{"changes":[{"new_path":"'"\${MOCK_MR_FILES:-src/main.go}"'"}]}'
   exit 0
 fi
@@ -660,6 +702,46 @@ run_gitlab_label_test "gitlab-no-label-actions-still-posts" \
   '{"action":"approve","pr_number":99,"repo":"test-group/test-project","head_sha":"abcdef0123456789abcdef0123456789abcdef01","body":"LGTM"}' \
   "fullsend post-review"
 
+run_gitlab_pr_files_fetch_error_fails_closed_test() {
+  local test_name="gitlab-pr-files-fetch-error-fails-closed"
+  local run_dir="${TMPDIR}/run-${test_name}"
+  mkdir -p "${run_dir}/iteration-1/output"
+  echo '{"action":"approve","pr_number":99,"repo":"test-group/test-project","head_sha":"abcdef0123456789abcdef0123456789abcdef01","body":"LGTM"}' > "${run_dir}/iteration-1/output/agent-result.json"
+  : > "${GH_LOG}"
+
+  local exit_code=0
+  # shellcheck disable=SC2030,SC2031
+  (
+    cd "${run_dir}"
+    export PATH="${MOCK_BIN}:${PATH}"
+    export REVIEW_TOKEN="fake-gitlab-token"
+    export PR_NUMBER="99"
+    export REPO_FULL_NAME="test-group/test-project"
+    export PR_URL="https://gitlab.com/test-group/test-project/-/merge_requests/99"
+    export CI_SERVER_HOST="gitlab.com"
+    export FULLSEND_FORGE="gitlab"
+    export REVIEW_FINDING_SEVERITY_THRESHOLD="low"
+    export MOCK_MR_FILES_FAIL="1"
+    bash "${POST_SCRIPT}"
+  ) > "${TMPDIR}/stdout-${test_name}.log" 2>&1 || exit_code=$?
+
+  if [[ ${exit_code} -eq 0 ]]; then
+    echo "FAIL: ${test_name} — expected non-zero exit"
+    cat "${TMPDIR}/stdout-${test_name}.log"
+    FAILURES=$((FAILURES + 1))
+    return
+  fi
+  if ! grep -qF "retrying once in case of a transient forge data race" "${TMPDIR}/stdout-${test_name}.log" || \
+     ! grep -qF "Failed to fetch PR files or PR has no changed files" "${TMPDIR}/stdout-${test_name}.log"; then
+    echo "FAIL: ${test_name} — expected retry and fail-closed messages"
+    cat "${TMPDIR}/stdout-${test_name}.log"
+    FAILURES=$((FAILURES + 1))
+    return
+  fi
+  echo "PASS: ${test_name}"
+}
+run_gitlab_pr_files_fetch_error_fails_closed_test
+
 run_label_test() {
   local test_name="$1"
   local json_content="$2"
@@ -1661,6 +1743,152 @@ run_empty_pr_files_with_protection_disabled_test() {
 }
 run_empty_pr_files_with_protection_disabled_test
 
+run_github_pr_files_fetch_error_fails_closed_test() {
+  local test_name="github-pr-files-fetch-error-fails-closed"
+  local run_dir="${TMPDIR}/run-${test_name}"
+  mkdir -p "${run_dir}/iteration-1/output"
+  echo "${APPROVE_JSON}" > "${run_dir}/iteration-1/output/agent-result.json"
+  : > "${GH_LOG}"
+
+  local exit_code=0
+  # shellcheck disable=SC2030,SC2031
+  (
+    cd "${run_dir}"
+    export PATH="${MOCK_BIN}:${PATH}"
+    export REVIEW_TOKEN="fake-token"
+    export PR_NUMBER="99"
+    export REPO_FULL_NAME="test-org/test-repo"
+    export PR_URL="https://github.com/test-org/test-repo/pull/99"
+    export FULLSEND_FORGE="github"
+    export REVIEW_FINDING_SEVERITY_THRESHOLD="low"
+    export REVIEW_PROTECTED_PATHS=""
+    export MOCK_PR_FILES_FAIL="1"
+    bash "${POST_SCRIPT}"
+  ) > "${TMPDIR}/stdout-${test_name}.log" 2>&1 || exit_code=$?
+
+  if [[ ${exit_code} -eq 0 ]]; then
+    echo "FAIL: ${test_name} — expected non-zero exit"
+    cat "${TMPDIR}/stdout-${test_name}.log"
+    FAILURES=$((FAILURES + 1))
+    return
+  fi
+  if ! grep -qF "retrying once in case of a transient forge data race" "${TMPDIR}/stdout-${test_name}.log" || \
+     ! grep -qF "Failed to fetch PR files or PR has no changed files" "${TMPDIR}/stdout-${test_name}.log"; then
+    echo "FAIL: ${test_name} — expected retry and fail-closed messages"
+    cat "${TMPDIR}/stdout-${test_name}.log"
+    FAILURES=$((FAILURES + 1))
+    return
+  fi
+  echo "PASS: ${test_name}"
+}
+run_github_pr_files_fetch_error_fails_closed_test
+
+# forge_get_pr_files can transiently return an empty list right after a
+# merge-commit update (fullsend-ai/fullsend#2093). The call site retries
+# once before refusing to approve: a first-empty-then-populated response
+# must recover and proceed rather than abort.
+run_empty_pr_files_retry_recovers_test() {
+  local test_name="empty-pr-files-retry-recovers"
+  local run_dir="${TMPDIR}/run-${test_name}"
+  mkdir -p "${run_dir}/iteration-1/output"
+  echo "${APPROVE_JSON}" > "${run_dir}/iteration-1/output/agent-result.json"
+  : > "${GH_LOG}"
+  rm -f "${TMPDIR}/marker-${test_name}"
+
+  local exit_code=0
+  # shellcheck disable=SC2030,SC2031
+  (
+    cd "${run_dir}"
+    export PATH="${MOCK_BIN}:${PATH}"
+    export REVIEW_TOKEN="fake-token"
+    export PR_NUMBER="99"
+    export REPO_FULL_NAME="test-org/test-repo"
+    export PR_URL="https://github.com/test-org/test-repo/pull/99"
+    export FULLSEND_FORGE="github"
+    export REVIEW_FINDING_SEVERITY_THRESHOLD="low"
+    export REVIEW_PROTECTED_PATHS=""
+    # First files call returns empty, the retry returns a real file.
+    export MOCK_PR_FILES_ON_RETRY="src/main.go"
+    export MOCK_FILES_CALL_MARKER="${TMPDIR}/marker-${test_name}"
+    bash "${POST_SCRIPT}"
+  ) > "${TMPDIR}/stdout-${test_name}.log" 2>&1 || exit_code=$?
+
+  if [[ ${exit_code} -ne 0 ]]; then
+    echo "FAIL: ${test_name} — expected success after retry recovered the file list"
+    cat "${TMPDIR}/stdout-${test_name}.log"
+    FAILURES=$((FAILURES + 1))
+    return
+  fi
+
+  if ! grep -qF "retrying once in case of a transient forge data race" "${TMPDIR}/stdout-${test_name}.log"; then
+    echo "FAIL: ${test_name} — expected retry notice in output"
+    cat "${TMPDIR}/stdout-${test_name}.log"
+    FAILURES=$((FAILURES + 1))
+    return
+  fi
+
+  if grep -qF "Failed to fetch PR files or PR has no changed files" "${TMPDIR}/stdout-${test_name}.log"; then
+    echo "FAIL: ${test_name} — should not abort once the retry returned files"
+    cat "${TMPDIR}/stdout-${test_name}.log"
+    FAILURES=$((FAILURES + 1))
+    return
+  fi
+
+  echo "PASS: ${test_name}"
+}
+run_empty_pr_files_retry_recovers_test
+
+# When both the initial fetch and the retry come back empty, the safety
+# net must still refuse to approve — the retry loosens the guard for
+# transient races only, not for genuinely empty results.
+run_empty_pr_files_retry_still_fails_test() {
+  local test_name="empty-pr-files-retry-still-fails"
+  local run_dir="${TMPDIR}/run-${test_name}"
+  mkdir -p "${run_dir}/iteration-1/output"
+  echo "${APPROVE_JSON}" > "${run_dir}/iteration-1/output/agent-result.json"
+  : > "${GH_LOG}"
+
+  local exit_code=0
+  # shellcheck disable=SC2030,SC2031
+  (
+    cd "${run_dir}"
+    export PATH="${MOCK_BIN}:${PATH}"
+    export REVIEW_TOKEN="fake-token"
+    export PR_NUMBER="99"
+    export REPO_FULL_NAME="test-org/test-repo"
+    export PR_URL="https://github.com/test-org/test-repo/pull/99"
+    export FULLSEND_FORGE="github"
+    export REVIEW_FINDING_SEVERITY_THRESHOLD="low"
+    export REVIEW_PROTECTED_PATHS=""
+    export MOCK_PR_FILES=""
+    bash "${POST_SCRIPT}"
+  ) > "${TMPDIR}/stdout-${test_name}.log" 2>&1 || exit_code=$?
+
+  if [[ ${exit_code} -eq 0 ]]; then
+    echo "FAIL: ${test_name} — expected non-zero exit when both attempts are empty"
+    cat "${TMPDIR}/stdout-${test_name}.log"
+    FAILURES=$((FAILURES + 1))
+    return
+  fi
+
+  if ! grep -qF "retrying once in case of a transient forge data race" "${TMPDIR}/stdout-${test_name}.log"; then
+    echo "FAIL: ${test_name} — expected the retry to be attempted before aborting"
+    cat "${TMPDIR}/stdout-${test_name}.log"
+    FAILURES=$((FAILURES + 1))
+    return
+  fi
+
+  if ! grep -qF "Failed to fetch PR files or PR has no changed files" "${TMPDIR}/stdout-${test_name}.log"; then
+    echo "FAIL: ${test_name} — expected empty-PR-files abort message after retry"
+    cat "${TMPDIR}/stdout-${test_name}.log"
+    FAILURES=$((FAILURES + 1))
+    return
+  fi
+
+  echo "PASS: ${test_name}"
+}
+run_empty_pr_files_retry_still_fails_test
+
 # The REVIEW_PROTECTED_PATHS default above is duplicated verbatim in
 # harness/review.yaml's env.runner/env.sandbox (there's no single structural
 # source of truth since env/default-review-protected-paths.txt was removed).

scripts/post-review.sh

@@ -101,8 +101,18 @@ forge_get_pr_info() {
 }
 
 forge_get_pr_files() {
-  GH_TOKEN="${REVIEW_TOKEN}" gh pr view "${PR_NUMBER}" \
-    --repo "${REPO}" --json files --jq '.files[].path'
+  # Use the paginated /pulls/{n}/files REST endpoint rather than the
+  # `gh pr view --json files` summary field: issue #2093 found empty
+  # results correlated with recent merge-commit updates and hypothesized
+  # asynchronous diff computation, but GitHub does not document that as
+  # an API contract. The files endpoint reflects the computed diff more
+  # directly.
+  local files
+  if ! files=$(GH_TOKEN="${REVIEW_TOKEN}" gh api \
+    "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' 2>/dev/null); then
+    return 1
+  fi
+  [[ -n "${files}" ]] && printf '%s\n' "${files}"
 }
 
 # --- PR mutations ---
@@ -642,8 +652,28 @@ if [ "${ACTION}" = "approve" ]; then
   # run regardless of whether protected-path enforcement itself is
   # enabled — only the pattern-matching loop below is gated on a
   # non-empty REVIEW_ACTIVE_PROTECTED_PATHS.
-  PR_FILES=$(forge_get_pr_files)
-  if [ -z "${PR_FILES}" ]; then
+  if PR_FILES=$(forge_get_pr_files); then
+    PR_FILES_FETCH_FAILED=false
+  else
+    PR_FILES_FETCH_FAILED=true
+    PR_FILES=""
+  fi
+  if [ "${PR_FILES_FETCH_FAILED}" = true ] || [ -z "${PR_FILES}" ]; then
+    # An empty file list may be a transient forge data race. Issue #2093
+    # found empty results correlated with recent merge-commit updates and
+    # hypothesized asynchronous diff computation, but the exact mechanism
+    # is not an established forge API contract. Retry once before refusing
+    # to approve, so we don't fail a genuinely non-empty PR.
+    echo "::notice::PR files came back empty; retrying once in case of a transient forge data race (forge_get_pr_files)" >&2
+    sleep 10
+    if PR_FILES=$(forge_get_pr_files); then
+      PR_FILES_FETCH_FAILED=false
+    else
+      PR_FILES_FETCH_FAILED=true
+      PR_FILES=""
+    fi
+  fi
+  if [ "${PR_FILES_FETCH_FAILED}" = true ] || [ -z "${PR_FILES}" ]; then
     echo "::error::Failed to fetch PR files or PR has no changed files — refusing to approve (forge_get_pr_files)" >&2
     exit 1
   fi

scripts/post-review.src.sh

@@ -232,8 +232,28 @@ if [ "${ACTION}" = "approve" ]; then
   # run regardless of whether protected-path enforcement itself is
   # enabled — only the pattern-matching loop below is gated on a
   # non-empty REVIEW_ACTIVE_PROTECTED_PATHS.
-  PR_FILES=$(forge_get_pr_files)
-  if [ -z "${PR_FILES}" ]; then
+  if PR_FILES=$(forge_get_pr_files); then
+    PR_FILES_FETCH_FAILED=false
+  else
+    PR_FILES_FETCH_FAILED=true
+    PR_FILES=""
+  fi
+  if [ "${PR_FILES_FETCH_FAILED}" = true ] || [ -z "${PR_FILES}" ]; then
+    # An empty file list may be a transient forge data race. Issue #2093
+    # found empty results correlated with recent merge-commit updates and
+    # hypothesized asynchronous diff computation, but the exact mechanism
+    # is not an established forge API contract. Retry once before refusing
+    # to approve, so we don't fail a genuinely non-empty PR.
+    echo "::notice::PR files came back empty; retrying once in case of a transient forge data race (forge_get_pr_files)" >&2
+    sleep 10
+    if PR_FILES=$(forge_get_pr_files); then
+      PR_FILES_FETCH_FAILED=false
+    else
+      PR_FILES_FETCH_FAILED=true
+      PR_FILES=""
+    fi
+  fi
+  if [ "${PR_FILES_FETCH_FAILED}" = true ] || [ -z "${PR_FILES}" ]; then
     echo "::error::Failed to fetch PR files or PR has no changed files — refusing to approve (forge_get_pr_files)" >&2
     exit 1
   fi

scripts/pre-review.sh

@@ -86,8 +86,18 @@ forge_get_pr_info() {
 }
 
 forge_get_pr_files() {
-  GH_TOKEN="${REVIEW_TOKEN}" gh pr view "${PR_NUMBER}" \
-    --repo "${REPO}" --json files --jq '.files[].path'
+  # Use the paginated /pulls/{n}/files REST endpoint rather than the
+  # `gh pr view --json files` summary field: issue #2093 found empty
+  # results correlated with recent merge-commit updates and hypothesized
+  # asynchronous diff computation, but GitHub does not document that as
+  # an API contract. The files endpoint reflects the computed diff more
+  # directly.
+  local files
+  if ! files=$(GH_TOKEN="${REVIEW_TOKEN}" gh api \
+    "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' 2>/dev/null); then
+    return 1
+  fi
+  [[ -n "${files}" ]] && printf '%s\n' "${files}"
 }
 
 # --- PR mutations ---