Post-CI routing: merge when already approved, skip re-review on rebase #42

New issue

Closed

opened 2026-04-18 11:35:22 +00:00 by claude-desktop · 0 comments

claude-desktop commented

2026-04-18 11:35:22 +00:00

Collaborator

User story

As a maintainer, when a previously-approved PR is rebased and CI re-goes green, I want the webhook to dispatch boss to merge immediately — not to re-request the reviewer. Re-reviewing an already-approved diff burns Claude tokens for zero value. If the reviewer previously left REQUEST_CHANGES on a different SHA and is still in requested_reviewers, bounce them so they re-review the new SHA. If there's no prior verdict, request review as today.

Context

Observed 2026-04-18, twice:

PR #38 (webhook split): reviewer REQUEST_CHANGES at old SHA → boss fixed → force-rebased → webhook logged "reviewer already requested, skipping" → stalled until manual bounce.
PR #41 & #43: reviewer APPROVED at old SHA → dev rebased onto new main → CI green → webhook logged "reviewer already requested, skipping" → stalled even though the PRs were already approved. Manual recovery: direct POST /merge via boss's token worked first try, no re-review needed. Forgejo does not require commit_id == head on the approval for a squash-merge.

Root cause in src/webhook-ci.ts:requestReviewIfFresh: the decision logic only knows one action — request a review — and its dedup is too aggressive (checks requested_reviewers before checking verdict state). The real "what should happen now" is a three-way routing decision that includes "merge":

State	Correct action
Any APPROVED review exists + PR mergeable	Dispatch boss to merge — no re-review
Verdict at head SHA (APPROVED or REQUEST_CHANGES)	Skip
Stale REQUEST_CHANGES + reviewer still in requested_reviewers	Bounce review request (fires `pull_request_review_request`)
Reviewer in requested_reviewers + no verdict at all	Skip (pending review)
Reviewer not in requested_reviewers + no verdict	POST `requested_reviewers`
PR author is reviewer	Skip

Acceptance criteria

Rename + rewire

Rename requestReviewIfFresh → decidePostCiAction (or similar — name reflects that the function now can dispatch a merge too). Keep the export shape thin so callers (handleActionRunEvent, handleStatusEvent, handlePullRequestOpened no-workflows path) stay one-line.
Function signature: (repo, pr, sha, reason) => Promise<void>. Returns void; all state changes are side effects (dispatch, fetch).

Decision logic

Fetch PR detail + reviews once at the top (not after early returns).
Compute:
- verdictAtHead — any APPROVED/REQUEST_CHANGES review with commit_id === sha.
- approvedAnywhere — any APPROVED review (any commit_id, not dismissed, by the REVIEWER_AGENT or any reviewer — prefer APPROVED by REVIEWER_AGENT specifically since that's the one our flow cares about).
- staleRequestChanges — REQUEST_CHANGES review by REVIEWER_AGENT with commit_id !== sha.
- reviewerInRequestedList — boolean.
Decision table (implement in this order):
1. PR author is REVIEWER_AGENT → skip, log "... author is reviewer, skipping".
2. approvedAnywhere AND PR mergeable === true → dispatch boss to merge (reuse the existing handleApproved path or a new dispatchMerge(repo, pr) helper). Log "<reason> on <repo>#<n>@<sha> — approved at <oldSha?>, dispatching boss merge".
3. verdictAtHead → skip, log "<reason> on <repo>#<n>@<sha> — verdict already at head, skipping".
4. staleRequestChanges AND reviewerInRequestedList → bounce (DELETE + POST requested_reviewers). Log "<reason> on <repo>#<n>@<sha> — stale REQUEST_CHANGES at <oldSha>, bouncing review request".
5. reviewerInRequestedList (and none of the above) → skip, log "<reason> on <repo>#<n>@<sha> — reviewer already requested and pending, skipping".
6. Otherwise → POST requested_reviewers. Log "<reason> on <repo>#<n>@<sha> — requested review from reviewer".

Merge dispatch helper

If the current pull_request_approved handler already has merge-dispatch logic, factor it into an exported dispatchMerge(repo, pr) and call it from both handleApproved and the new decision branch.
The merge dispatch sends to the boss worker with the existing merge skill and no stateless session — same as a normal approval.

API wiring

Add deleteReviewRequest(repo, prNumber, reviewer, token) to src/forgejo-api.ts (DELETE /pulls/:n/requested_reviewers with {reviewers: [...]}). 5s AbortSignal.timeout, shared auth.

Tests

In src/webhook-ci.test.ts (using the fetch harness from #37 now merged):

PR author is reviewer → no fetch, no dispatch.
Any APPROVED review + PR mergeable → boss merge dispatched, no requested_reviewers write.
Any APPROVED review + PR not mergeable → no boss dispatch (the mergeable: false path is handled elsewhere by the rebase dispatch), no review request either. Log only.
verdictAtHead (APPROVED on the current commit) → no dispatches, no fetches beyond the initial PR + reviews list.
Stale REQUEST_CHANGES + reviewer in requested_reviewers → DELETE then POST called in order.
Reviewer in requested_reviewers + no prior verdict → no API calls.
No reviewer in requested_reviewers + no prior verdict → single POST.

Manual verification

Test A (approved rebase): open PR, approve, force-rebase, wait for CI green. Log must show "dispatching boss merge" and the PR merges without any reviewer task running.
Test B (stale REQUEST_CHANGES): submit REQUEST_CHANGES, push fix, force-rebase. Log must show "stale REQUEST_CHANGES at <old>, bouncing review request" and the reviewer task runs exactly once.
Test C (fresh PR, no prior review): behaves as today — review requested, reviewer runs, approves, merges.

Out of scope

Changing merge.md or review.md skills. This is purely a webhook routing fix.
Handling external-reviewer approvals (non-REVIEWER_AGENT users). The decision should work the same regardless of who left the APPROVED.
Dismissing stale reviews (Forgejo stops counting them once head SHA moves; no need).
#40 auto-rebase remains a separate concern.

References

PR #38 (2026-04-18) — manual bounce recovery.
PRs #41 & #43 (2026-04-18) — manual direct-merge recovery after tokens were already burned on a canceled re-review.
Current buggy code: src/webhook-ci.ts:requestReviewIfFresh.

Dependencies

Not blocked by: anything (current main layout is fine).
Helped by: #37's fetch harness (now merged) — tests are easier to write.
Branch off: main

## User story As a **maintainer**, when a previously-approved PR is rebased and CI re-goes green, I want the webhook to dispatch boss to **merge** immediately — not to re-request the reviewer. Re-reviewing an already-approved diff burns Claude tokens for zero value. If the reviewer previously left `REQUEST_CHANGES` on a different SHA and is still in `requested_reviewers`, bounce them so they re-review the new SHA. If there's no prior verdict, request review as today. ## Context Observed 2026-04-18, twice: - PR #38 (webhook split): reviewer `REQUEST_CHANGES` at old SHA → boss fixed → force-rebased → webhook logged `"reviewer already requested, skipping"` → stalled until manual bounce. - PR #41 & #43: reviewer `APPROVED` at old SHA → dev rebased onto new main → CI green → webhook logged `"reviewer already requested, skipping"` → stalled **even though the PRs were already approved**. Manual recovery: direct `POST /merge` via boss's token worked first try, no re-review needed. Forgejo does not require commit_id == head on the approval for a squash-merge. Root cause in `src/webhook-ci.ts:requestReviewIfFresh`: the decision logic only knows one action — *request a review* — and its dedup is too aggressive (checks `requested_reviewers` before checking verdict state). The real "what should happen now" is a three-way routing decision that includes "merge": | State | Correct action | |---|---| | Any APPROVED review exists + PR mergeable | **Dispatch boss to merge** — no re-review | | Verdict at head SHA (APPROVED or REQUEST_CHANGES) | Skip | | Stale REQUEST_CHANGES + reviewer still in requested_reviewers | Bounce review request (fires `pull_request_review_request`) | | Reviewer in requested_reviewers + no verdict at all | Skip (pending review) | | Reviewer not in requested_reviewers + no verdict | POST `requested_reviewers` | | PR author is reviewer | Skip | ## Acceptance criteria ### Rename + rewire - [ ] Rename `requestReviewIfFresh` → `decidePostCiAction` (or similar — name reflects that the function now can dispatch a *merge* too). Keep the export shape thin so callers (`handleActionRunEvent`, `handleStatusEvent`, `handlePullRequestOpened` no-workflows path) stay one-line. - [ ] Function signature: `(repo, pr, sha, reason) => Promise<void>`. Returns void; all state changes are side effects (dispatch, fetch). ### Decision logic - [ ] Fetch PR detail + reviews **once** at the top (not after early returns). - [ ] Compute: - `verdictAtHead` — any APPROVED/REQUEST_CHANGES review with `commit_id === sha`. - `approvedAnywhere` — any APPROVED review (any commit_id, not dismissed, by the REVIEWER_AGENT or any reviewer — prefer APPROVED by REVIEWER_AGENT specifically since that's the one our flow cares about). - `staleRequestChanges` — REQUEST_CHANGES review by REVIEWER_AGENT with `commit_id !== sha`. - `reviewerInRequestedList` — boolean. - [ ] Decision table (implement in this order): 1. PR author is REVIEWER_AGENT → skip, log `"... author is reviewer, skipping"`. 2. `approvedAnywhere` AND PR `mergeable === true` → dispatch boss to merge (reuse the existing `handleApproved` path or a new `dispatchMerge(repo, pr)` helper). Log `"<reason> on <repo>#<n>@<sha> — approved at <oldSha?>, dispatching boss merge"`. 3. `verdictAtHead` → skip, log `"<reason> on <repo>#<n>@<sha> — verdict already at head, skipping"`. 4. `staleRequestChanges` AND `reviewerInRequestedList` → bounce (`DELETE` + `POST` `requested_reviewers`). Log `"<reason> on <repo>#<n>@<sha> — stale REQUEST_CHANGES at <oldSha>, bouncing review request"`. 5. `reviewerInRequestedList` (and none of the above) → skip, log `"<reason> on <repo>#<n>@<sha> — reviewer already requested and pending, skipping"`. 6. Otherwise → POST `requested_reviewers`. Log `"<reason> on <repo>#<n>@<sha> — requested review from reviewer"`. ### Merge dispatch helper - [ ] If the current `pull_request_approved` handler already has merge-dispatch logic, factor it into an exported `dispatchMerge(repo, pr)` and call it from both `handleApproved` and the new decision branch. - [ ] The merge dispatch sends to the **boss** worker with the existing merge skill and no stateless session — same as a normal approval. ### API wiring - [ ] Add `deleteReviewRequest(repo, prNumber, reviewer, token)` to `src/forgejo-api.ts` (DELETE `/pulls/:n/requested_reviewers` with `{reviewers: [...]}`). 5s `AbortSignal.timeout`, shared auth. ### Tests In `src/webhook-ci.test.ts` (using the fetch harness from #37 now merged): - [ ] PR author is reviewer → no fetch, no dispatch. - [ ] Any APPROVED review + PR mergeable → boss merge dispatched, no `requested_reviewers` write. - [ ] Any APPROVED review + PR not mergeable → **no** boss dispatch (the `mergeable: false` path is handled elsewhere by the rebase dispatch), no review request either. Log only. - [ ] `verdictAtHead` (APPROVED on the current commit) → no dispatches, no fetches beyond the initial PR + reviews list. - [ ] Stale REQUEST_CHANGES + reviewer in requested_reviewers → `DELETE` then `POST` called in order. - [ ] Reviewer in requested_reviewers + no prior verdict → no API calls. - [ ] No reviewer in requested_reviewers + no prior verdict → single `POST`. ### Manual verification - [ ] Test A (approved rebase): open PR, approve, force-rebase, wait for CI green. Log must show `"dispatching boss merge"` and the PR merges without any reviewer task running. - [ ] Test B (stale REQUEST_CHANGES): submit REQUEST_CHANGES, push fix, force-rebase. Log must show `"stale REQUEST_CHANGES at <old>, bouncing review request"` and the reviewer task runs exactly once. - [ ] Test C (fresh PR, no prior review): behaves as today — review requested, reviewer runs, approves, merges. ## Out of scope - Changing `merge.md` or `review.md` skills. This is purely a webhook routing fix. - Handling external-reviewer approvals (non-`REVIEWER_AGENT` users). The decision should work the same regardless of who left the APPROVED. - Dismissing stale reviews (Forgejo stops counting them once head SHA moves; no need). - `#40` auto-rebase remains a separate concern. ## References - PR #38 (2026-04-18) — manual bounce recovery. - PRs #41 & #43 (2026-04-18) — manual direct-merge recovery after tokens were already burned on a canceled re-review. - Current buggy code: `src/webhook-ci.ts:requestReviewIfFresh`. ## Dependencies - **Not blocked by:** anything (current `main` layout is fine). - **Helped by:** #37's fetch harness (now merged) — tests are easier to write. - **Branch off:** `main`

claude-desktop added the

area:webhook

type:user-story

labels

2026-04-18 11:35:27 +00:00

claude-desktop changed title from ~~Re-dispatch reviewer when head SHA changed and prior verdict is stale~~ to Post-CI routing: merge when already approved, skip re-review on rebase

2026-04-18 13:26:39 +00:00