Skip to content

feat(fix): cap automated review->fix cycles per PR #6596

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
guyoron1 wants to merge 1 commit into from
Closed

feat(fix): cap automated review->fix cycles per PR #6596

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
106 changes: 98 additions & 8 deletions .github/scripts/check-fix-eligibility-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,19 @@ trap 'rm -rf "${TMPDIR}"' EXIT
# $1 — is_bot value (true/false/null)
# $2 — login value
# $3 — comma-separated labels (optional)
# $4 — reviews API response body (optional, default "[]"); the literal
# string "FAIL" makes the mock reviews call exit 1, simulating an
# API error
# $5 — issues/comments API response body, for marker search (optional,
# default "[]")
build_mock() {
local is_bot="$1" login="$2" labels="${3:-}"
local reviews_json="${4:-[]}" comments_json="${5:-[]}"
local mock_bin="${TMPDIR}/bin"

rm -rf "${mock_bin}"
mkdir -p "${mock_bin}"
rm -f "${TMPDIR}/posted-comment-body.txt" "${TMPDIR}/reviews-fail"

local labels_json="[]"
if [[ -n "${labels}" ]]; then
Expand All @@ -39,6 +46,13 @@ build_mock() {
printf '%s' "${json}" > "${TMPDIR}/pr-json.txt"
printf '%s' "${TMPDIR}/pr-json.txt" > "${TMPDIR}/pr-json-path"

if [[ "${reviews_json}" == "FAIL" ]]; then
: > "${TMPDIR}/reviews-fail"
else
printf '%s' "${reviews_json}" > "${TMPDIR}/reviews-json.txt"
fi
printf '%s' "${comments_json}" > "${TMPDIR}/comments-json.txt"

cat > "${mock_bin}/gh" <<'MOCKEOF'
#!/usr/bin/env bash
MOCK_DIR="$(cd "$(dirname "$0")/.." && pwd)"
Expand Down Expand Up @@ -66,6 +80,25 @@ if [[ "$1" == "pr" && "$2" == "view" ]]; then
cat "${PR_JSON_FILE}"
exit 0
fi
if [[ "$1" == "api" ]]; then
if [[ "$2" == "-X" && "$3" == "POST" && "$4" == *"/comments" ]]; then
cat > "${MOCK_DIR}/posted-comment-body.txt"
echo '{"id": 999}'
exit 0
fi
if [[ "$2" == *"/reviews" ]]; then
if [[ -f "${MOCK_DIR}/reviews-fail" ]]; then
echo "mock gh: simulated reviews API failure" >&2
exit 1
fi
cat "${MOCK_DIR}/reviews-json.txt"
exit 0
fi
if [[ "$2" == *"/comments" ]]; then
cat "${MOCK_DIR}/comments-json.txt"
exit 0
fi
fi
echo "unexpected gh call: $*" >&2
exit 1
MOCKEOF
Expand All @@ -77,24 +110,33 @@ MOCKEOF

# run_test runs the eligibility script with mocked PR data and asserts exit code
# and (optionally) annotation text.
# $1 — test name
# $2 — expected exit code
# $3 — is_bot value
# $4 — login value
# $5 — trigger source
# $6 — labels (optional, comma-separated)
# $7 — expected annotation substring (optional)
# $1 — test name
# $2 — expected exit code
# $3 — is_bot value
# $4 — login value
# $5 — trigger source
# $6 — labels (optional, comma-separated)
# $7 — expected annotation substring (optional)
# $8 — reviews API response body (optional, default "[]"; "FAIL"

@fullsend-ai-review fullsend-ai-review Bot Aug 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] test-organization

run_test now takes 11 positional arguments. Other test files in the repo cap at 4-5 args using separate setup functions. Optional middle parameters require empty-string placeholders at call sites.

Suggested fix: Consider splitting cycle-cap test state into a separate setup step or environment variables.

# simulates an API error)
# $9 — issues/comments API response body (optional, default "[]")
# $10 — REVIEW_MAX_FIX_CYCLES value to set (optional; empty leaves the
# script's own default in effect)
# $11 — "yes"/"no" to assert whether a fix-cycle-cap comment was (not)
# posted (optional)
run_test() {
local name="$1" expected_exit="$2" is_bot="$3" login="$4" trigger="$5" labels="${6:-}" expected_annotation="${7:-}"
local reviews_json="${8:-[]}" comments_json="${9:-[]}" cap_env="${10:-}" expect_post="${11:-}"
local mock_bin
mock_bin=$(build_mock "${is_bot}" "${login}" "${labels}")
mock_bin=$(build_mock "${is_bot}" "${login}" "${labels}" "${reviews_json}" "${comments_json}")

local actual_exit=0 output
output=$(PATH="${mock_bin}:${PATH}" \
TRIGGER_SOURCE="${trigger}" \
PR_NUM="123" \
SOURCE_REPO="org/repo" \
GH_TOKEN="fake" \
REVIEW_MAX_FIX_CYCLES="${cap_env}" \
bash "${SCRIPT}" 2>&1) || actual_exit=$?

if [[ "${actual_exit}" -ne "${expected_exit}" ]]; then
Expand All @@ -109,6 +151,18 @@ run_test() {
return
fi

if [[ "${expect_post}" == "yes" && ! -f "${TMPDIR}/posted-comment-body.txt" ]]; then
echo "FAIL: ${name} — expected a fix-cycle-cap comment to be posted, but none was"
FAILURES=$((FAILURES + 1))
return
fi

if [[ "${expect_post}" == "no" && -f "${TMPDIR}/posted-comment-body.txt" ]]; then
echo "FAIL: ${name} — expected no fix-cycle-cap comment, but one was posted"
FAILURES=$((FAILURES + 1))
return
fi

echo "PASS: ${name}"
}

Expand Down Expand Up @@ -156,6 +210,42 @@ run_test "false is_bot coder login without label skipped" 1 "false" "app/fullsen
# is_bot=false with coder login and label proceeds (exit 0)
run_test "false is_bot coder login with label proceeds" 0 "false" "app/fullsend-ai-coder" "review-bot[bot]" "fullsend-fix"

echo ""
echo "=== fix-cycle cap tests ==="

REVIEWS_UNDER_CAP='[{"state":"CHANGES_REQUESTED","user":{"login":"org-review[bot]"}},{"state":"CHANGES_REQUESTED","user":{"login":"org-review[bot]"}}]'
REVIEWS_AT_CAP='[{"state":"CHANGES_REQUESTED","user":{"login":"org-review[bot]"}},{"state":"CHANGES_REQUESTED","user":{"login":"org-review[bot]"}},{"state":"CHANGES_REQUESTED","user":{"login":"org-review[bot]"}}]'
COMMENTS_WITH_MARKER='[{"id":55,"body":"<!-- fullsend-fix-cycle-cap -->\nOld message"}]'

# Under the default cap (3): proceeds, no comment
run_test "under cap proceeds" 0 "true" "app/fullsend-ai-coder" "review-bot[bot]" "" "" \
"${REVIEWS_UNDER_CAP}" "[]" "" "no"

# At the default cap: blocked, warning emitted, comment posted
run_test "at cap exits 1 and posts comment" 1 "true" "app/fullsend-ai-coder" "review-bot[bot]" "" \
"automated fix cycles" "${REVIEWS_AT_CAP}" "[]" "" "yes"

# Marker already present: still blocked, but no second comment
run_test "marker present skips second comment" 1 "true" "app/fullsend-ai-coder" "review-bot[bot]" "" \
"" "${REVIEWS_AT_CAP}" "${COMMENTS_WITH_MARKER}" "" "no"

# Human trigger bypasses the cap entirely (existing :16 early-exit)
run_test "human trigger ignores cap" 0 "false" "some-user" "human-user" "" \
"" "${REVIEWS_AT_CAP}" "[]" "" "no"

# 0 disables the cap even when the count would otherwise trip it
run_test "cap 0 disables" 0 "true" "app/fullsend-ai-coder" "review-bot[bot]" "" \
"" "${REVIEWS_AT_CAP}" "[]" "0" "no"

# Reviews API failure: proceed, warn, do not block
run_test "reviews API failure proceeds with warning" 0 "true" "app/fullsend-ai-coder" "review-bot[bot]" "" \
"Could not count review cycles" "FAIL" "[]" "" "no"

# Non-numeric cap value: warns and falls back to the default (3), which the
# at-cap review count above should still trip
run_test "non-numeric cap warns and uses default" 1 "true" "app/fullsend-ai-coder" "review-bot[bot]" "" \
"REVIEW_MAX_FIX_CYCLES is not a number" "${REVIEWS_AT_CAP}" "[]" "abc" "yes"

# gh pr view failure (network error / invalid token) emits ::error:: and exits 1
run_test_gh_failure() {
local mock_bin="${TMPDIR}/bin-fail"
Expand Down
64 changes: 60 additions & 4 deletions .github/scripts/check-fix-eligibility.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,15 @@
# check-fix-eligibility.sh — Determine if a bot-triggered fix should auto-run.
#
# Inputs (env vars):
# GH_TOKEN — GitHub token for API calls
# PR_NUM — Pull request number
# SOURCE_REPO — Repository in owner/repo format
# TRIGGER_SOURCE — Username that triggered the fix
# GH_TOKEN — GitHub token for API calls
# PR_NUM — Pull request number
# SOURCE_REPO — Repository in owner/repo format
# TRIGGER_SOURCE — Username that triggered the fix
# REVIEW_MAX_FIX_CYCLES — Cap on review-bot CHANGES_REQUESTED cycles before
# blocking further bot-triggered fixes. Default 3,
# 0 disables. Non-numeric values warn and fall back
# to the default. CI-runtime gating knob, not an
# agent behavior knob — see ADR 0081.
#
# Exits 0 if fix should proceed, 1 if it should be skipped.
# Emits GitHub Actions annotations (::warning::) for skip reasons.
Expand Down Expand Up @@ -59,3 +64,54 @@ if [[ "${PR_IS_BOT}" != "true" || "${PR_LOGIN}" != "app/fullsend-ai-coder" ]]; t
exit 1
fi
fi

# Cap automated fix cycles: block further bot-triggered fixes once the

@fullsend-ai-review fullsend-ai-review Bot Aug 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[medium] GHA workflow command injection

REVIEW_MAX_FIX_CYCLES is interpolated into a ::warning:: annotation before numeric validation succeeds. At this point it contains the raw invalid value. The existing code sanitizes PR_IS_BOT and PR_LOGIN via _sanitize_for_annotation for the same reason. Although the source is vars.REVIEW_MAX_FIX_CYCLES (settable only by repo admins), the new interpolation should follow the same defense-in-depth pattern.

Suggested fix: Pass the value through _sanitize_for_annotation before the ::warning:: emission on line 68.

# review bot has requested changes REVIEW_MAX_FIX_CYCLES times on this PR.
# Each CHANGES_REQUESTED review is one trip around the review->fix loop;
# uncapped, a standing disagreement between the review and fix agents can
# oscillate until someone notices the bill. Human /fs-fix is unaffected —
# it already exited at the TRIGGER_SOURCE check above, before this gate.
REVIEW_MAX_FIX_CYCLES="${REVIEW_MAX_FIX_CYCLES:-3}"
if [[ ! "${REVIEW_MAX_FIX_CYCLES}" =~ ^[0-9]+$ ]]; then
# Mirrors route-review-model.sh's TRIVIAL_MAX_LINES handling, but falls

@fullsend-ai-review fullsend-ai-review Bot Aug 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[medium] dangling cross-reference

The inline comment references route-review-model.sh's TRIVIAL_MAX_LINES handling as precedent, but neither exists in this repository. The PR body notes it ships in draft PR #6590; if that PR is abandoned or modified, this comment becomes misleading.

Suggested fix: Replace with self-contained rationale: 'An unenforceable cap must not silently become no cap, so we warn and fall back to the default.'

# back to the default instead of bailing out entirely — an unenforceable
# cap must not silently become "no cap".
echo "::warning::REVIEW_MAX_FIX_CYCLES is not a number (${REVIEW_MAX_FIX_CYCLES}) — using default of 3"
REVIEW_MAX_FIX_CYCLES=3
fi

@fullsend-ai-review fullsend-ai-review Bot Aug 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] edge-case

If gh api --paginate returns a 200 with a completely empty body, jq -s 'add' produces null and .[] fails. In practice, GitHub's reviews endpoint always returns an array, and the failure path falls through safely to the else branch.

if [[ "${REVIEW_MAX_FIX_CYCLES}" != "0" ]]; then
# The review bot's REST login is "<org>-review[bot]" (see
# docs/contributing/bot-identities.md). SOURCE_REPO already carries the
# org, so it's derived here rather than threaded in as a new input —
# the same construction as the REVIEW_BOT var in the "Pre-fetch review
# body" step of reusable-dispatch.yml.
REVIEW_BOT_LOGIN="${SOURCE_REPO%%/*}-review[bot]"

if CYCLE_COUNT=$(gh api "repos/${SOURCE_REPO}/pulls/${PR_NUM}/reviews" \
--paginate 2>/dev/null \
| jq -s --arg login "${REVIEW_BOT_LOGIN}" \
'add | [.[] | select(.state == "CHANGES_REQUESTED" and .user.login == $login)] | length'); then
if (( CYCLE_COUNT >= REVIEW_MAX_FIX_CYCLES )); then
echo "::warning::PR #${PR_NUM} has reached ${CYCLE_COUNT} automated fix cycles (cap ${REVIEW_MAX_FIX_CYCLES}) — a human needs to look"

@fullsend-ai-review fullsend-ai-review Bot Aug 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[medium] fail-open

When the reviews API call fails, the script proceeds without enforcing the cycle cap ('proceeding without the cap'). This is a fail-open pattern on a safety gate. A transient API failure silently bypasses the cap, undermining its purpose.

Suggested fix: Consider failing closed (exit 1) on API failure, or add an explicit comment documenting the fail-open tradeoff and confirming it is intentional.

MARKER='<!-- fullsend-fix-cycle-cap -->'
EXISTING_ID=$(gh api "repos/${SOURCE_REPO}/issues/${PR_NUM}/comments" \
--paginate 2>/dev/null \
| jq -r --arg marker "${MARKER}" '.[] | select(.body | contains($marker)) | .id' \
| head -n1 || true)

if [[ -z "${EXISTING_ID}" ]]; then
COMMENT_BODY="${MARKER}
${CYCLE_COUNT} automated fix cycles reached on this PR — a human needs to look. Trigger \`/fs-fix\` manually to run another cycle."
jq -n --arg body "${COMMENT_BODY}" '{body: $body}' \
| gh api -X POST "repos/${SOURCE_REPO}/issues/${PR_NUM}/comments" --input - >/dev/null \
|| echo "::warning::Could not post fix-cycle-cap comment on PR #${PR_NUM}"
fi

exit 1
fi
else
echo "::warning::Could not count review cycles for PR #${PR_NUM} — proceeding without the cap"
fi
fi
1 change: 1 addition & 0 deletions .github/workflows/reusable-dispatch.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,6 +1121,7 @@ jobs:
TRIGGER_SOURCE: ${{ needs.route.outputs.trigger_source }}
PR_NUM: ${{ steps.context.outputs.pr_number }}
SOURCE_REPO: ${{ github.repository }}
REVIEW_MAX_FIX_CYCLES: ${{ vars.REVIEW_MAX_FIX_CYCLES }}
run: bash .defaults/.github/scripts/check-fix-eligibility.sh

- name: Checkout target repository at PR HEAD
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/reusable-fix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ jobs:
TRIGGER_SOURCE: ${{ inputs.trigger_source }}
PR_NUM: ${{ steps.context.outputs.pr_number }}
SOURCE_REPO: ${{ inputs.source_repo }}
REVIEW_MAX_FIX_CYCLES: ${{ vars.REVIEW_MAX_FIX_CYCLES }}
run: bash .defaults/.github/scripts/check-fix-eligibility.sh

- name: Checkout target repository at PR HEAD
Expand Down
Loading