From cad0fc8ef3ae0597fbef4f2df2d1aff2b190207b Mon Sep 17 00:00:00 2001 From: guy oron Date: Tue, 25 Aug 2026 17:32:10 +0300 Subject: [PATCH] feat(fix): cap automated review->fix cycles per PR Nothing bounds the review->fix loop today; a standing disagreement between the review and fix agents oscillates until a human notices the cost. Bot-triggered fixes now stop after REVIEW_MAX_FIX_CYCLES (default 3) bot-requested-changes reviews, with one explanatory comment. Human /fs-fix was never gated and remains the override. Signed-off-by: guy oron --- .github/scripts/check-fix-eligibility-test.sh | 106 ++++++++++++++++-- .github/scripts/check-fix-eligibility.sh | 64 ++++++++++- .github/workflows/reusable-dispatch.yml | 1 + .github/workflows/reusable-fix.yml | 1 + 4 files changed, 160 insertions(+), 12 deletions(-) diff --git a/.github/scripts/check-fix-eligibility-test.sh b/.github/scripts/check-fix-eligibility-test.sh index 11a0e557c..562eeff7e 100755 --- a/.github/scripts/check-fix-eligibility-test.sh +++ b/.github/scripts/check-fix-eligibility-test.sh @@ -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 @@ -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)" @@ -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 @@ -77,17 +110,25 @@ 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" +# 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}" \ @@ -95,6 +136,7 @@ run_test() { 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 @@ -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}" } @@ -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":"\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" diff --git a/.github/scripts/check-fix-eligibility.sh b/.github/scripts/check-fix-eligibility.sh index 620ac05ac..0dc6c850e 100755 --- a/.github/scripts/check-fix-eligibility.sh +++ b/.github/scripts/check-fix-eligibility.sh @@ -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. @@ -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 +# 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 + # 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 + +if [[ "${REVIEW_MAX_FIX_CYCLES}" != "0" ]]; then + # The review bot's REST login is "-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" + + MARKER='' + 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 diff --git a/.github/workflows/reusable-dispatch.yml b/.github/workflows/reusable-dispatch.yml index 62b072bd4..8e9a603b3 100644 --- a/.github/workflows/reusable-dispatch.yml +++ b/.github/workflows/reusable-dispatch.yml @@ -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 diff --git a/.github/workflows/reusable-fix.yml b/.github/workflows/reusable-fix.yml index 1c70c54dd..9ed721db1 100644 --- a/.github/workflows/reusable-fix.yml +++ b/.github/workflows/reusable-fix.yml @@ -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