fix(review): don't REQUEST_CHANGES on pending CI — pollutes MAX_ROUNDS counter #148

Merged

charles merged 1 commit from fix/review-skill-pending-ci into main

2026-04-20 14:05:53 +00:00

claude-desktop commented

2026-04-20 14:03:35 +00:00

Collaborator

Root cause (observed on PR #147)

Reviewer agent submitted REQUEST_CHANGES three times in a row on PR #147. The actual code feedback was resolved in round 1 (try/catch scope fix in efe9a83). Rounds 2 and 3 explicitly said the code was correct but flagged "CI was still waiting" as the reason for REQUEST_CHANGES. Three REQUEST_CHANGES → MAX_ROUNDS=3 tripped → force-merge from #137 fired → boss squash-merged the PR despite the (meaningless) outstanding REQUEST_CHANGES.

The outcome was correct — the PR was clean and shipped — but the signal was garbage. The force-merge terminator is supposed to break reviewer-author deadlocks on real disagreement, not on CI runner latency. If this stays in place, every PR where the reviewer arrives before CI will eat the same 3-round meat-grinder before landing.

Source of the behavior

skills/review.md step 5 instructed the reviewer to REQUEST_CHANGES on pending CI:

If CI is still pending/running: submit REQUEST_CHANGES with a
one-line note "CI was still running at review time — push any
trivial change (or wait) and I will re-review when it completes".
Do not guess.

That path treats infrastructure state as a code defect.

Fix

Pending-CI carve-out added to both skills/review.md (full review) and skills/review-delta.md (round 2+ resume path). On pending/running/waiting CI, the reviewer now:

Posts a create_issue_comment — informational only, does not advance the review workflow ("Stepping off the review request — will be re-dispatched automatically when CI completes").
Calls delete_review_requests to remove itself from the PR's requested_reviewers list.
Exits the skill with no create_pull_review call.

Why that's safe

When CI settles (action_run_success / action_run_failure webhook), decidePostCiAction runs the three-way routing:

Reviewer is not in requested_reviewers → case 6 "POST requested_reviewers" → re-request.
Pool scheduler (idle-preferred round-robin) picks a reviewer instance.
The skill runs again; CI is now decided → normal APPROVED / REQUEST_CHANGES.

Event-driven re-review, no polling, no MAX_ROUNDS slot wasted on CI flakiness.

What about the ticket that warned against COMMENT-type reviews (#111)?

#111's warning is about create_pull_review with state: "COMMENT" — that does not advance Forgejo's review lifecycle, so the dispatcher can't see it. This fix uses create_issue_comment (plain issue comment), not create_pull_review — different API, doesn't claim to be a verdict. The dispatcher correctly ignores it; Forgejo doesn't treat it as a review at all.

No code changes

Skills are runtime-loaded — takes effect on the next reviewer dispatch after merge. Skill files alone; TypeScript untouched.

Test plan

Merge + next reviewer dispatch on a PR with pending CI: verify an issue_comment is posted + reviewer is removed from requested_reviewers list (not a new review submission).
When CI completes on that PR: verify the webhook's action_run_success re-requests the reviewer (service log: "requested review from reviewer").
The force-merge at #137's MAX_ROUNDS no longer fires from pure-CI-waiting review loops.

Closes nothing directly — follow-up to #137 (force-merge) and #147 (force-merge firing on a clean PR).

## Root cause (observed on PR #147) Reviewer agent submitted `REQUEST_CHANGES` three times in a row on PR #147. The actual code feedback was resolved in **round 1** (try/catch scope fix in `efe9a83`). Rounds 2 and 3 explicitly said the code was correct but flagged "CI was still waiting" as the reason for REQUEST_CHANGES. Three REQUEST_CHANGES → `MAX_ROUNDS=3` tripped → force-merge from #137 fired → boss squash-merged the PR despite the (meaningless) outstanding REQUEST_CHANGES. The **outcome** was correct — the PR was clean and shipped — but the **signal** was garbage. The force-merge terminator is supposed to break reviewer-author deadlocks on real disagreement, not on CI runner latency. If this stays in place, every PR where the reviewer arrives before CI will eat the same 3-round meat-grinder before landing. ## Source of the behavior `skills/review.md` step 5 instructed the reviewer to `REQUEST_CHANGES` on pending CI: ``` If CI is still pending/running: submit REQUEST_CHANGES with a one-line note "CI was still running at review time — push any trivial change (or wait) and I will re-review when it completes". Do not guess. ``` That path treats infrastructure state as a code defect. ## Fix Pending-CI carve-out added to both `skills/review.md` (full review) and `skills/review-delta.md` (round 2+ resume path). On pending/running/waiting CI, the reviewer now: 1. Posts a `create_issue_comment` — informational only, does **not** advance the review workflow ("Stepping off the review request — will be re-dispatched automatically when CI completes"). 2. Calls `delete_review_requests` to remove itself from the PR's `requested_reviewers` list. 3. **Exits the skill** with no `create_pull_review` call. ### Why that's safe When CI settles (`action_run_success` / `action_run_failure` webhook), `decidePostCiAction` runs the three-way routing: - Reviewer is not in `requested_reviewers` → case 6 "POST requested_reviewers" → re-request. - Pool scheduler (idle-preferred round-robin) picks a reviewer instance. - The skill runs again; CI is now decided → normal APPROVED / REQUEST_CHANGES. Event-driven re-review, no polling, no MAX_ROUNDS slot wasted on CI flakiness. ### What about the ticket that warned against COMMENT-type reviews (#111)? `#111`'s warning is about `create_pull_review` with `state: "COMMENT"` — that does not advance Forgejo's review lifecycle, so the dispatcher can't see it. This fix uses `create_issue_comment` (plain issue comment), not `create_pull_review` — different API, doesn't claim to be a verdict. The dispatcher correctly ignores it; Forgejo doesn't treat it as a review at all. ## No code changes Skills are runtime-loaded — takes effect on the next reviewer dispatch after merge. Skill files alone; TypeScript untouched. ## Test plan - [ ] Merge + next reviewer dispatch on a PR with pending CI: verify an `issue_comment` is posted + reviewer is removed from `requested_reviewers` list (not a new review submission). - [ ] When CI completes on that PR: verify the webhook's `action_run_success` re-requests the reviewer (service log: `"requested review from reviewer"`). - [ ] The force-merge at #137's MAX_ROUNDS no longer fires from pure-CI-waiting review loops. Closes nothing directly — follow-up to #137 (force-merge) and #147 (force-merge firing on a clean PR).

claude-desktop added 1 commit

2026-04-20 14:03:35 +00:00

fix(review): don't REQUEST_CHANGES on pending CI — pollutes MAX_ROUNDS counter

qa / qa (pull_request) Successful in 3m8s

Details

qa / dockerfile (pull_request) Successful in 11s

Details

8c7f54466e

On PR #147 the reviewer agent submitted REQUEST_CHANGES three times in a
row purely because CI was still in `waiting` state at each review —
never a real code finding after Round 1's try/catch fix. The
three-rounds-of-REQUEST_CHANGES triggered the force-merge terminator
from #137, which squash-merged the PR despite the (meaningless)
outstanding REQUEST_CHANGES. The merge was correct in outcome, but the
signal that drove it was noise — the force-merge is meant for genuine
reviewer-author disagreement, not infrastructure latency.

Skill change:

- `skills/review.md` step 5: pending CI no longer maps to
  REQUEST_CHANGES. Reviewer posts a `create_issue_comment` that says
  "CI still pending — stepping off the review request", calls
  `delete_review_requests` to remove itself from the PR's
  `requested_reviewers` list, and exits without a verdict.
  `decidePostCiAction` fires on `action_run_success` / `_failure`,
  sees reviewer absent from the requested list, and re-dispatches via
  the "POST requested_reviewers" branch — event-driven re-review with
  no polling and no wasted MAX_ROUNDS slot.
- Same rule added to `skills/review-delta.md` so the delta path (round
  2+ resume) can't regress the carve-out.

No code changes; skills are runtime-loaded. Effective on the next
reviewer dispatch.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

charles merged commit 7cc71f0b63 into main

2026-04-20 14:05:53 +00:00

charles deleted branch fix/review-skill-pending-ci

2026-04-20 14:05:54 +00:00

charles referenced this pull request from a commit

2026-04-20 14:05:55 +00:00

fix(review): don't REQUEST_CHANGES on pending CI — pollutes MAX_ROUNDS counter (#148)

claude-desktop referenced this pull request

2026-04-20 14:24:09 +00:00

Tracking: agent pool + customization #47