feat(fix): cap automated review->fix cycles per PR - #6596
Conversation
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 <goron@redhat.com>
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
/fs-review is it possible that the interaction cap already exists, but in the fullsend-ai/agents repo? |
|
🤖 Finished Review · ✅ Success · Started 2:48 PM UTC · Completed 3:09 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.08 |
ReviewFindingsHigh
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| fi | ||
| fi | ||
|
|
||
| # Cap automated fix cycles: block further bot-triggered fixes once the |
There was a problem hiding this comment.
[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.
| '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" | ||
|
|
There was a problem hiding this comment.
[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.
| # 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 |
There was a problem hiding this comment.
[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.'
| echo "::warning::REVIEW_MAX_FIX_CYCLES is not a number (${REVIEW_MAX_FIX_CYCLES}) — using default of 3" | ||
| REVIEW_MAX_FIX_CYCLES=3 | ||
| fi | ||
|
|
There was a problem hiding this comment.
[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.
| # $5 — trigger source | ||
| # $6 — labels (optional, comma-separated) | ||
| # $7 — expected annotation substring (optional) | ||
| # $8 — reviews API response body (optional, default "[]"; "FAIL" |
There was a problem hiding this comment.
[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.
|
I think this PR may be superseded by existing functionality in the agents repo. The agents repo already has iteration caps implemented in Filed fullsend-ai/agents#1020 to document those variables. That might be a simpler solution than implementing a separate review-count-based cap here. |
|
Closing — this duplicates an existing mechanism I missed: the pre-fix script already enforces an iteration cap (ITERATION_CAP, bot-triggered default 5; ITERATION_CAP_HUMAN default 10), with the iteration derived from counting fullsend-fix commits on the PR, escalation messaging, and the needs-human label near the threshold (docs/agents/fix.md). The loop is already bounded; nothing further needed. |
|
🤖 Finished Retro · ✅ Success · Started 4:21 AM UTC · Completed 4:38 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.99 |
Retro: PR #6596 — feat(fix): cap automated review→fix cycles per PRTimeline
Review quality assessmentThe review agent produced 10 findings, all factually grounded. The GHA workflow command injection finding, dangling cross-reference to non-existent The review agent did not and could not answer ralphbean's strategic question about whether existing caps already solve this problem — the decisive insight that led to PR closure. What went well
Nuance on the "duplicate" assessmentThe PR's CI-level cap ( Evidence for existing open issues
Proposals filed
|
2 participants
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-fixwas never gated and remains the override.What changed
.github/scripts/check-fix-eligibility.sh: after the existing label/author gates, countsCHANGES_REQUESTEDreviews from the review bot (gh api .../pulls/{n}/reviews --paginate, aggregated the same way the neighboring fix-iteration count already does:--paginate | jq -s 'add | ...'). At or aboveREVIEW_MAX_FIX_CYCLES(default 3,0disables): emits::warning::, posts one idempotent comment (marker<!-- fullsend-fix-cycle-cap -->, searched via the same paginate+jq pattern before posting), and exits 1. A failed reviews-API call warns and proceeds — cap unavailable does not mean fix forbidden. A non-numericREVIEW_MAX_FIX_CYCLESwarns and falls back to the default of 3 (mirrorsroute-review-model.sh'sTRIVIAL_MAX_LINEShandling, but falls back to a default instead of bailing out, since an unenforceable cap must not silently become no cap)..github/workflows/reusable-dispatch.ymland.github/workflows/reusable-fix.yml: both existing "Check fix eligibility" steps now also passREVIEW_MAX_FIX_CYCLES: ${{ vars.REVIEW_MAX_FIX_CYCLES }}through to the script (both invocation sites;check-fix-eligibility.shis a single vendored source underinternal/scaffold/vendormanifest.go'svendoredDefaultsScripts, byte-copied to.defaults/.github/scripts/in consumer repos — no bundler step to keep in sync, unlike the agents repo's.src.sh/.shsplit)..github/scripts/check-fix-eligibility-test.sh: extends the existing stub-ghmock to also answerpulls/.../reviewsandissues/.../comments(GET + POST), with aposted-comment-body.txtsentinel file so tests can assert whether a comment was (not) posted. Seven new cases: under cap proceeds, at cap exits 1 and posts a comment, marker-present skips the second comment, human trigger ignores the cap,0disables, reviews-API failure proceeds with a warning, non-numeric cap value warns and falls back to the default.The review bot's REST login (
<org>-review[bot]) is documented indocs/contributing/bot-identities.md; rather than threading a new env var for it, it's derived fromSOURCE_REPO(already an input) the same way theREVIEW_BOTvar is built in this same workflow's "Pre-fetch review body" step a few lines below — so no new plumbing was needed for that part.Test plan
bash .github/scripts/check-fix-eligibility-test.sh— all 21 cases pass (14 pre-existing + 7 new)shellcheckon both touched scripts — cleanactionlinton both touched workflow files, diffed against pristineorigin/main(findings normalized for the line-number shift my one-line insertion causes) — identical 31 pre-existingjob.workflow_sha/job.workflow_repositoryfindings, zero newNote for reviewers
route-review-model.sh(the file this PR's non-numeric-handling style mirrors) does not exist onmainyet — it ships in still-open draft PR #6590. I read it there for the style reference only; this PR does not depend on it and branches frommaindirectly.