Skip to content

fix(#193): apply outcome labels even when review submission fails #195

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fullsend-ai-coder wants to merge 1 commit into from
Closed

fix(#193): apply outcome labels even when review submission fails #195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
207 changes: 207 additions & 0 deletions scripts/post-review-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,213 @@ run_label_test_with_env_stdout "severity-filter-downgrade-log-message" \
"All findings removed by severity filter" \
"REVIEW_FINDING_SEVERITY_THRESHOLD" "medium"

# ---------------------------------------------------------------------------
# Partial-failure tests: fullsend post-review exits non-zero (e.g. 422 from
# inline comment outside diff hunk) but outcome labels should still be applied.
# Mirrors the fix in post-review.sh — keep in sync.
# ---------------------------------------------------------------------------

run_partial_failure_test() {
local test_name="$1"
local json_content="$2"
local fullsend_exit="$3"
local expected_pattern="$4"

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}"

# Custom mock bin with a fullsend that exits non-zero
local fail_bin="${TMPDIR}/fail-bin-${test_name}"
mkdir -p "${fail_bin}"
cp "${MOCK_BIN}/gh" "${fail_bin}/gh"
cat > "${fail_bin}/fullsend" <<FAILEOF
#!/usr/bin/env bash
echo "fullsend \$*" >> "${GH_LOG}"
exit ${fullsend_exit}
FAILEOF
chmod +x "${fail_bin}/fullsend"

local exit_code=0
# shellcheck disable=SC2030,SC2031
(
cd "${run_dir}"
export PATH="${fail_bin}:${PATH}"
export REVIEW_TOKEN="fake-token"
export PR_NUMBER="99"
export REPO_FULL_NAME="test-org/test-repo"
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 but got 0"
cat "${TMPDIR}/stdout-${test_name}.log"
FAILURES=$((FAILURES + 1))
return
fi

if ! grep -qF -- "${expected_pattern}" "${GH_LOG}"; then
echo "FAIL: ${test_name} — expected pattern '${expected_pattern}' not found in gh calls"
echo "Actual calls:"
cat "${GH_LOG}"
FAILURES=$((FAILURES + 1))
return
fi

echo "PASS: ${test_name}"
}

run_partial_failure_test_stdout() {
local test_name="$1"
local json_content="$2"
local fullsend_exit="$3"
local expected_stdout="$4"

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 fail_bin="${TMPDIR}/fail-bin-${test_name}"
mkdir -p "${fail_bin}"
cp "${MOCK_BIN}/gh" "${fail_bin}/gh"
cat > "${fail_bin}/fullsend" <<FAILEOF
#!/usr/bin/env bash
echo "fullsend \$*" >> "${GH_LOG}"
exit ${fullsend_exit}
FAILEOF
chmod +x "${fail_bin}/fullsend"

local exit_code=0
# shellcheck disable=SC2030,SC2031
(
cd "${run_dir}"
export PATH="${fail_bin}:${PATH}"
export REVIEW_TOKEN="fake-token"
export PR_NUMBER="99"
export REPO_FULL_NAME="test-org/test-repo"
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 but got 0"
cat "${TMPDIR}/stdout-${test_name}.log"
FAILURES=$((FAILURES + 1))
return
fi

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

echo "PASS: ${test_name}"
}

run_partial_failure_test_no_pattern() {
local test_name="$1"
local json_content="$2"
local fullsend_exit="$3"
local forbidden_pattern="$4"

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 fail_bin="${TMPDIR}/fail-bin-${test_name}"
mkdir -p "${fail_bin}"
cp "${MOCK_BIN}/gh" "${fail_bin}/gh"
cat > "${fail_bin}/fullsend" <<FAILEOF
#!/usr/bin/env bash
echo "fullsend \$*" >> "${GH_LOG}"
exit ${fullsend_exit}
FAILEOF
chmod +x "${fail_bin}/fullsend"

local exit_code=0
# shellcheck disable=SC2030,SC2031
(
cd "${run_dir}"
export PATH="${fail_bin}:${PATH}"
export REVIEW_TOKEN="fake-token"
export PR_NUMBER="99"
export REPO_FULL_NAME="test-org/test-repo"
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 but got 0"
cat "${TMPDIR}/stdout-${test_name}.log"
FAILURES=$((FAILURES + 1))
return
fi

if grep -qF -- "${forbidden_pattern}" "${GH_LOG}"; then
echo "FAIL: ${test_name} — forbidden pattern '${forbidden_pattern}' was found in gh calls"
echo "Actual calls:"
cat "${GH_LOG}"
FAILURES=$((FAILURES + 1))
return
fi

echo "PASS: ${test_name}"
}

# --- Partial-failure test cases ---

# approve + fullsend failure → downgrade to requires-manual-review (no false ready-for-merge)
run_partial_failure_test "partial-failure-approve-downgrade-label" \
'{"action":"approve","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"LGTM"}' \
1 \
"--add-label requires-manual-review"

# approve + fullsend failure → ready-for-merge must NOT be applied
run_partial_failure_test_no_pattern "partial-failure-approve-no-ready-for-merge" \
'{"action":"approve","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"LGTM"}' \
1 \
"--add-label ready-for-merge"

# comment + fullsend failure → requires-manual-review label still applied
run_partial_failure_test "partial-failure-comment-labels-applied" \
'{"action":"comment","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"Mixed review"}' \
1 \
"--add-label requires-manual-review"

# Error annotation is logged on partial failure
run_partial_failure_test_stdout "partial-failure-error-logged" \
'{"action":"approve","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"LGTM"}' \
1 \
"::error::fullsend post-review failed"

# reject + fullsend failure → PR must NOT be closed
run_partial_failure_test_no_pattern "partial-failure-reject-no-close" \
'{"action":"reject","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"Approach rejected"}' \
1 \
"pr close"

# reject + fullsend failure → downgrade to requires-manual-review
run_partial_failure_test "partial-failure-reject-downgrade-label" \
'{"action":"reject","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"Approach rejected"}' \
1 \
"--add-label requires-manual-review"

# reject + fullsend failure → stdout confirms downgrade
run_partial_failure_test_stdout "partial-failure-reject-downgrade-logged" \
'{"action":"reject","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"Approach rejected"}' \
1 \
"downgrading to requires-manual-review"

# reject + fullsend failure → rejected label must NOT be applied
run_partial_failure_test_no_pattern "partial-failure-reject-no-rejected-label" \
'{"action":"reject","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"Approach rejected"}' \
1 \
"--add-label rejected"

# --- Summary ---

echo ""
Expand Down
63 changes: 47 additions & 16 deletions scripts/post-review.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,16 @@ ${REDISPATCH_MARKER}" || echo "::warning::Failed to post re-dispatch comment"
# appear as a failure.
exit 0
elif [ "${POST_REVIEW_EXIT}" -ne 0 ]; then

@waynesun09 waynesun09 Jul 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM — premature-decision: unverified assumption that all non-zero/non-10 fullsend post-review exit codes mean "safe to continue"

No exit-code contract exists (in this repo or fullsend-ai/fullsend) distinguishing "partial success" (something was posted, e.g. the 422 case in #193) from "total failure" (nothing was posted, e.g. auth/network failure). The fix generalizes from one specific reproduction to all failure modes without confirming this distinction is safe to collapse.

Suggestion: Confirm the fullsend CLI's actual failure contract, or note the assumption explicitly as a code comment / follow-up ticket to have fullsend return a distinct code for "partial post" vs. "nothing posted."

[verified] · flagged by 3/3 agents (full consensus)

@rh-hemartin rh-hemartin Jul 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@waynesun09 waynesun09 Jul 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM — ::error:: downgraded to ::warning:: for a condition that still fails the job

GitHub Actions workflow-command severity (::error:: vs ::warning::) only affects UI annotation prominence, not control flow — exit is what terminates the script, and that was already removed here. Downgrading to ::warning:: doesn't change behavior but does reduce visibility of a genuine failure in the Actions UI, and the failure is now logged twice (here, and again at the bottom of the script) with less prominence than before.

Suggestion: Keep ::error:: here (it doesn't terminate execution on its own), and drop or merge the duplicate message at the bottom of the script.

[verified] · flagged by 1/3 agents (Grok)

@rh-hemartin rh-hemartin Jul 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

echo "::error::fullsend post-review failed with exit code ${POST_REVIEW_EXIT} (PR #${PR_NUMBER} in ${REPO_FULL_NAME})" >&2
exit "${POST_REVIEW_EXIT}"
# ASSUMPTION: non-zero/non-10 exits are treated uniformly as partial
# failures where the sticky comment may have posted but the formal
# review submission failed (e.g. 422 from inline comments outside the
# diff hunk — see #193). The fullsend CLI does not currently distinguish
# partial success from total failure (auth/network). Destructive side
# effects (reject→close, approve→ready-for-merge) are guarded below,
# but a distinct exit code for "nothing posted" would be safer.
# TODO(fullsend-cli): add a distinct exit code for total failure vs
# partial post — see #193 follow-up.
echo "::error::fullsend post-review failed with exit code ${POST_REVIEW_EXIT} (PR #${PR_NUMBER} in ${REPO_FULL_NAME}) — applying outcome labels then exiting 1" >&2
fi

# ---------------------------------------------------------------------------
Expand All @@ -363,32 +371,43 @@ for stale_label in "ready-for-merge" "requires-manual-review" "rejected"; do
--remove-label "${stale_label}" 2>/dev/null || true
done

if [ "${ACTION}" = "approve" ] && [ "${DOWNGRADED}" = "false" ]; then
LABEL_FAILED=false

if [ "${ACTION}" = "approve" ] && [ "${DOWNGRADED}" = "false" ] && [ "${POST_REVIEW_EXIT}" -eq 0 ]; then
echo "Approve disposition — applying ready-for-merge label"
gh label create "ready-for-merge" --repo "${REPO_FULL_NAME}" \
--description "All reviewers approved — ready to merge" --color "0E8A16" \
2>/dev/null || true
gh pr edit "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" \
--add-label "ready-for-merge" || true
elif { [ "${ACTION}" = "approve" ] && [ "${DOWNGRADED}" = "true" ]; } || \
--add-label "ready-for-merge" || LABEL_FAILED=true
elif { [ "${ACTION}" = "approve" ] && { [ "${DOWNGRADED}" = "true" ] || [ "${POST_REVIEW_EXIT}" -ne 0 ]; }; } || \
[ "${ACTION}" = "comment" ]; then
echo "Review requires human judgment — applying requires-manual-review label"
gh label create "requires-manual-review" --repo "${REPO_FULL_NAME}" \
--description "Review requires human judgment" --color "FBCA04" \
2>/dev/null || true
gh pr edit "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" \
--add-label "requires-manual-review" || true
--add-label "requires-manual-review" || LABEL_FAILED=true
elif [ "${ACTION}" = "reject" ]; then
echo "Reject disposition — closing PR and applying label"
gh label create "rejected" --repo "${REPO_FULL_NAME}" \
--description "Approach rejected by review agent" --color "B60205" \
2>/dev/null || true
gh pr close "${PR_NUMBER}" \
--repo "${REPO_FULL_NAME}" \
--comment "Closed by review agent: approach rejected." || true
gh pr edit "${PR_NUMBER}" \
--repo "${REPO_FULL_NAME}" \
--add-label "rejected" || true
if [ "${POST_REVIEW_EXIT}" -ne 0 ]; then
echo "Reject disposition but post-review failed — downgrading to requires-manual-review"
gh label create "requires-manual-review" --repo "${REPO_FULL_NAME}" \
--description "Review requires human judgment" --color "FBCA04" \
2>/dev/null || true
gh pr edit "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" \
--add-label "requires-manual-review" || LABEL_FAILED=true
else
echo "Reject disposition — closing PR and applying label"
gh label create "rejected" --repo "${REPO_FULL_NAME}" \
--description "Approach rejected by review agent" --color "B60205" \
2>/dev/null || true
gh pr close "${PR_NUMBER}" \
--repo "${REPO_FULL_NAME}" \
--comment "Closed by review agent: approach rejected." || LABEL_FAILED=true
gh pr edit "${PR_NUMBER}" \
--repo "${REPO_FULL_NAME}" \
--add-label "rejected" || LABEL_FAILED=true
fi
elif [ "${ACTION}" = "request-changes" ]; then
echo "Request-changes disposition — no outcome label (fix agent triggers on event)"
fi
Expand All @@ -410,4 +429,16 @@ for label in "${VALIDATED_LABEL_REMOVES[@]}"; do
-X DELETE --silent 2>/dev/null || true
done

if [ "${LABEL_FAILED}" = "true" ]; then
echo "::error::Outcome label application failed on ${REPO_FULL_NAME}#${PR_NUMBER}" >&2
fi

if [ "${POST_REVIEW_EXIT}" -ne 0 ]; then
exit 1
fi

if [ "${LABEL_FAILED}" = "true" ]; then
exit 1
fi

echo "Review posted on ${REPO_FULL_NAME}#${PR_NUMBER}"
Loading