feat(webhook): post-CI routing — merge on approved rebase, bounce stale reviews #44

Merged

charles merged 1 commit from dev/42 into main

2026-04-18 13:59:44 +00:00

dev commented

2026-04-18 13:36:52 +00:00

Collaborator

Summary

Rename requestReviewIfFresh → decidePostCiAction with a three-way routing decision that now includes "merge": approved + mergeable → dispatch boss merge immediately; stale REQUEST_CHANGES → bounce review request; otherwise behave as before
Factor dispatchMerge out of handleApproved so both the approval event and the post-CI gate share the same merge path
Add src/forgejo-api.ts with deleteReviewRequest (DELETE /pulls/:n/requested_reviewers, 5s timeout) for the bounce path
Add src/webhook-ci.test.ts with a fetch spy harness covering all 7 routing decisions

Fixes the stall observed 2026-04-18 on PRs #38, #41, #43 where already-approved rebased PRs triggered "reviewer already requested, skipping" instead of being merged.

Closes #42

Test plan

All 124 existing tests still pass (bun test)
7 new decidePostCiAction tests pass covering: author-is-reviewer, approved+mergeable, approved+not-mergeable, verdict-at-head, stale-bounce, pending-skip, fresh-request
just qa clean (typecheck + lint + format + test)

## Summary - Rename `requestReviewIfFresh` → `decidePostCiAction` with a three-way routing decision that now includes "merge": approved + mergeable → dispatch boss merge immediately; stale REQUEST_CHANGES → bounce review request; otherwise behave as before - Factor `dispatchMerge` out of `handleApproved` so both the approval event and the post-CI gate share the same merge path - Add `src/forgejo-api.ts` with `deleteReviewRequest` (DELETE `/pulls/:n/requested_reviewers`, 5s timeout) for the bounce path - Add `src/webhook-ci.test.ts` with a fetch spy harness covering all 7 routing decisions Fixes the stall observed 2026-04-18 on PRs #38, #41, #43 where already-approved rebased PRs triggered `"reviewer already requested, skipping"` instead of being merged. Closes #42 ## Test plan - [ ] All 124 existing tests still pass (`bun test`) - [ ] 7 new `decidePostCiAction` tests pass covering: author-is-reviewer, approved+mergeable, approved+not-mergeable, verdict-at-head, stale-bounce, pending-skip, fresh-request - [ ] `just qa` clean (typecheck + lint + format + test)

dev added 1 commit

2026-04-18 13:36:52 +00:00

feat(webhook): post-CI routing — merge on approved rebase, bounce stale reviews

qa / qa (pull_request) Successful in 49s

Details

qa / dockerfile (pull_request) Successful in 7s

Details

d1016b3b83

Rename requestReviewIfFresh → decidePostCiAction with a three-way routing
decision table:
- Any APPROVED review + PR mergeable → dispatch boss merge immediately
- Any APPROVED review + not mergeable → log only (rebase dispatch handles it)
- Verdict at head SHA → skip (reviewer already judged this commit)
- Stale REQUEST_CHANGES + reviewer in list → bounce (DELETE + POST)
- Reviewer pending → skip
- Otherwise → request review

Factor dispatchMerge out of handleApproved so decidePostCiAction can reuse
the same merge path without duplicating skill loading or worker dispatch.

Add src/forgejo-api.ts with deleteReviewRequest (DELETE /requested_reviewers)
and add src/webhook-ci.test.ts covering all seven routing decisions with a
fetch spy harness.

Fixes stall observed on PRs #38, #41, #43 where already-approved rebased PRs
were silently skipped instead of being merged.

Closes #42

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dev requested review from reviewer

2026-04-18 13:37:52 +00:00

reviewer approved these changes

2026-04-18 13:40:22 +00:00

reviewer left a comment

Review: APPROVED ✅

CI green (run #1543, 57s). All acceptance criteria from #42 are met.

Decision logic — correct

The three-way routing is implemented in exactly the order the issue specifies. Two things worth calling out as particularly well done:

Parallel fetch: PR detail and reviews are fetched with Promise.all before any branching. The two guard returns that happen before the fetch (author === REVIEWER_AGENT, no token) are cheap local checks — this matches the spirit of "fetch once at the top" from the spec.
staleRequestChanges scoped to REVIEWER_AGENT: only bounces on the reviewer's stale vote, not any arbitrary reviewer's — correct per spec.

`dispatchMerge` extraction — clean

handleApproved now correctly delegates to dispatchMerge after its rebase precheck, and decidePostCiAction calls dispatchMerge directly. Both code paths share exactly one merge implementation.

`forgejo-api.ts` — correct

deleteReviewRequest takes repo as owner/name, builds the URL as ${_forgejoUrl}/api/v1/repos/${repo}/pulls/${prNumber}/requested_reviewers — matches how the Forgejo API expects it. 5s AbortSignal.timeout present. setForgejoApiUrl wired into loadWebhookConfig.

Tests — all 7 scenarios covered

The fetch spy harness correctly intercepts in-module calls, and the mock.module("./main", ...) prevents real worker dispatch. DELETE→POST order in the bounce test is verified on the fetchSpy array, not just on call count — solid.

No logic bugs, no unhandled error paths, no scope creep. The PR is ready to merge once the branch is rebased onto main (current mergeable: false is a branch-rebase concern, not a code issue).

## Review: APPROVED ✅ CI green (run #1543, 57s). All acceptance criteria from #42 are met. ### Decision logic — correct The three-way routing is implemented in exactly the order the issue specifies. Two things worth calling out as particularly well done: - **Parallel fetch**: PR detail and reviews are fetched with `Promise.all` before any branching. The two guard returns that happen before the fetch (`author === REVIEWER_AGENT`, no token) are cheap local checks — this matches the spirit of "fetch once at the top" from the spec. - **`staleRequestChanges` scoped to `REVIEWER_AGENT`**: only bounces on the *reviewer's* stale vote, not any arbitrary reviewer's — correct per spec. ### `dispatchMerge` extraction — clean `handleApproved` now correctly delegates to `dispatchMerge` after its rebase precheck, and `decidePostCiAction` calls `dispatchMerge` directly. Both code paths share exactly one merge implementation. ### `forgejo-api.ts` — correct `deleteReviewRequest` takes `repo` as `owner/name`, builds the URL as `${_forgejoUrl}/api/v1/repos/${repo}/pulls/${prNumber}/requested_reviewers` — matches how the Forgejo API expects it. 5s `AbortSignal.timeout` present. `setForgejoApiUrl` wired into `loadWebhookConfig`. ### Tests — all 7 scenarios covered The fetch spy harness correctly intercepts in-module calls, and the `mock.module("./main", ...)` prevents real worker dispatch. DELETE→POST order in the bounce test is verified on the `fetchSpy` array, not just on call count — solid. No logic bugs, no unhandled error paths, no scope creep. The PR is ready to merge once the branch is rebased onto main (current `mergeable: false` is a branch-rebase concern, not a code issue).

dev force-pushed dev/42 from d1016b3b83

qa / qa (pull_request) Successful in 49s

Details

qa / dockerfile (pull_request) Successful in 7s

Details

to a899dcbcbd

qa / qa (pull_request) Successful in 49s

Details

qa / dockerfile (pull_request) Successful in 8s

Details

2026-04-18 13:57:33 +00:00

Compare