charles/claude-hooks

Fork 0

feat(review-loop): auto-force-merge when MAX_ROUNDS cap is hit #137

Merged

code-lead merged 2 commits from feat/force-merge-on-max-rounds into main

2026-04-20 12:48:24 +00:00

claude-desktop commented

2026-04-20 12:23:26 +00:00

Collaborator

Behaviour change

When guardAuthorDispatch sees reviewer has submitted ≥ MAX_ROUNDS (3) REQUEST_CHANGES reviews on a PR, the round cap no longer halts and pages the operator. It now dispatches a force-merge to boss instead.

What "force" means

dispatchMerge(repo, pr, { force: true }) prepends an override block to the task text: skip the "latest review state is APPROVED" check in step 2 of skills/merge.md.
Every other guard stays: PR open, mergeable: true (no conflicts), CI green. If any of those fail, boss stops and posts an explanatory comment — no hard bypass.
The skill file itself is unchanged — the override is task-text-scoped.

Wiring

Layer	Change
`src/review-loop.ts`	`GuardDecision` grows `forceMerge?: boolean`. `guardAuthorDispatch` sets it + rewrites the PR comment to "review loop capped — auto-merging". No import of webhook-ci — decision returned, handler dispatches.
`src/webhook-ci.ts`	`dispatchMerge(repo, pr, opts: { force?: boolean })`. Force prepends a 9-line override block.
`src/webhook-handlers.ts`	`handleChangesRequested` routes `cap.forceMerge` → `dispatchMerge(repo, pr, { force: true })`.
`src/review-loop.test.ts`	Updated `${MAX_ROUNDS} rounds` test asserts `forceMerge: true`, `reason.includes("force-merging")`, comment body contains "auto-merging".

Comment on PR (before the merge dispatch)

## 🤖 Review loop capped — auto-merging

Reviewer `reviewer` submitted **3 REQUEST_CHANGES rounds** on this PR
against author `boss`. Per the MAX_ROUNDS=3 policy, the review cycle
is halted and boss will squash-merge the PR now.

### What still applies
- PR must be open, mergeable (no conflicts), CI green. If any fail,
  the force-merge dispatch stops and posts an explanatory comment —
  no hard bypass.
- The `latest review state is APPROVED` check is waived for this merge.
  The review will be REQUEST_CHANGES, and that's by design.

### Rationale
Each round costs ~5 min × 2 agents × 1M-context; past round 3 findings
are usually nitpick spiral or reviewer non-determinism rather than real
correctness issues.

_Cap is `MAX_ROUNDS=3` in `src/review-loop.ts`. …_

Revert path

If auto-merge turns out wrong in practice (e.g. reviewer catches a real bug on round 3 that should have blocked), the old operator-handoff behaviour is a 3-line revert in guardAuthorDispatch + handleChangesRequested — the postForceMergeComment body can swap back to options-list copy, and the forceMerge flag becomes a no-op.

Tests

bun test src/review-loop.test.ts — 17 pass.
bun test src/webhook-handlers.test.ts — 26 pass.
Full suite: pre-existing failures only (happy-dom tsc from #121, DELETE wipe=true env-dep, 3 sweeper-JSONL tests from a just-landed PR — all reproduce on main).

Closes nothing — this is a behaviour change requested by operator.

## Behaviour change When `guardAuthorDispatch` sees `reviewer` has submitted **≥ MAX_ROUNDS (3)** REQUEST_CHANGES reviews on a PR, the round cap no longer halts and pages the operator. It now dispatches a **force-merge** to boss instead. ### What "force" means - `dispatchMerge(repo, pr, { force: true })` prepends an override block to the task text: skip the "latest review state is APPROVED" check in step 2 of `skills/merge.md`. - Every other guard stays: PR open, `mergeable: true` (no conflicts), CI green. If any of those fail, boss stops and posts an explanatory comment — **no hard bypass**. - The skill file itself is unchanged — the override is task-text-scoped. ### Wiring | Layer | Change | |---|---| | `src/review-loop.ts` | `GuardDecision` grows `forceMerge?: boolean`. `guardAuthorDispatch` sets it + rewrites the PR comment to "review loop capped — auto-merging". No import of webhook-ci — decision returned, handler dispatches. | | `src/webhook-ci.ts` | `dispatchMerge(repo, pr, opts: { force?: boolean })`. Force prepends a 9-line override block. | | `src/webhook-handlers.ts` | `handleChangesRequested` routes `cap.forceMerge` → `dispatchMerge(repo, pr, { force: true })`. | | `src/review-loop.test.ts` | Updated `${MAX_ROUNDS} rounds` test asserts `forceMerge: true`, `reason.includes("force-merging")`, comment body contains "auto-merging". | ### Comment on PR (before the merge dispatch) ``` ## 🤖 Review loop capped — auto-merging Reviewer `reviewer` submitted **3 REQUEST_CHANGES rounds** on this PR against author `boss`. Per the MAX_ROUNDS=3 policy, the review cycle is halted and boss will squash-merge the PR now. ### What still applies - PR must be open, mergeable (no conflicts), CI green. If any fail, the force-merge dispatch stops and posts an explanatory comment — no hard bypass. - The `latest review state is APPROVED` check is waived for this merge. The review will be REQUEST_CHANGES, and that's by design. ### Rationale Each round costs ~5 min × 2 agents × 1M-context; past round 3 findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues. _Cap is `MAX_ROUNDS=3` in `src/review-loop.ts`. …_ ``` ### Revert path If auto-merge turns out wrong in practice (e.g. reviewer catches a real bug on round 3 that should have blocked), the old operator-handoff behaviour is a 3-line revert in `guardAuthorDispatch` + `handleChangesRequested` — the `postForceMergeComment` body can swap back to options-list copy, and the `forceMerge` flag becomes a no-op. ### Tests - `bun test src/review-loop.test.ts` — 17 pass. - `bun test src/webhook-handlers.test.ts` — 26 pass. - Full suite: pre-existing failures only (`happy-dom` tsc from #121, `DELETE wipe=true` env-dep, 3 sweeper-JSONL tests from a just-landed PR — all reproduce on `main`). Closes nothing — this is a behaviour change requested by operator.

claude-desktop added 1 commit

2026-04-20 12:23:26 +00:00

feat(review-loop): auto-force-merge when MAX_ROUNDS cap is hit

qa / qa (pull_request) Failing after 2m45s

Details

qa / dockerfile (pull_request) Successful in 9s

Details

64136f2d87

Instead of halting the address-review dispatch and paging the operator
with a handoff comment, the round cap now triggers a force-merge via
boss. The existing `dispatchMerge` grew an opt-in `force: true` flag
that prepends an override block to the task text; every other guard in
`skills/merge.md` (PR open, mergeable, CI green) still applies —
only the "latest review state is APPROVED" check is waived, which is
exactly the state the cap fires in.

Wiring:

- `guardAuthorDispatch` sets `forceMerge: true` on the `GuardDecision`
  and renames its PR comment to "review loop capped — auto-merging"
  (drops the operator-options block, keeps the rationale + cap pointer).
- `handleChangesRequested` routes `cap.forceMerge` into
  `dispatchMerge(repo, pr, { force: true })` in lieu of the skipped
  address-review.
- `review-loop.ts` stays free of the webhook-ci import chain — the
  decision is returned, the handler does the dispatch.

Tests updated for the new shape (`forceMerge: true`, comment copy);
skills/merge.md unchanged — the override is task-text-scoped.

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

charles force-pushed feat/force-merge-on-max-rounds from 64136f2d87

qa / qa (pull_request) Failing after 2m45s

Details

qa / dockerfile (pull_request) Successful in 9s

Details

to a1f4248876

qa / qa (pull_request) Failing after 2m43s

Details

qa / dockerfile (pull_request) Successful in 11s

Details

2026-04-20 12:32:29 +00:00

Compare

charles added 1 commit

2026-04-20 12:38:23 +00:00

style(webhook-ci): inline dispatchMerge signature per biome

qa / qa (pull_request) Successful in 3m2s

Details

qa / dockerfile (pull_request) Successful in 9s

Details

9a2ae95f94

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

charles requested review from reviewer

2026-04-20 12:42:03 +00:00

reviewer approved these changes

2026-04-20 12:46:47 +00:00

reviewer left a comment

pending

reviewer approved these changes

2026-04-20 12:47:05 +00:00

reviewer left a comment

✅ APPROVED

CI: Green on head 9a2ae95 (run #1715, 3m11s). ✅

What I checked

src/review-loop.ts — guardAuthorDispatch correctly trips at rounds >= MAX_ROUNDS, posts the "auto-merging" comment, and returns { skip: true, forceMerge: true }. The module stays pure: it returns a decision, the handler dispatches. Swallowing createIssueComment errors is documented and correct — the decision is the important effect, not the comment.

src/webhook-ci.ts — dispatchMerge(repo, pr, { force: true }) prepends a clear 9-line override block instructing boss to skip the approval check. The three remaining guards (PR open, mergeable: true, CI green) are still enforced by the boss merge skill — no hard bypass.

src/webhook-handlers.ts — handleChangesRequested routes cap.forceMerge → dispatchMerge(…, { force: true }). The return null below it is the documented revert path (dead code today, not a bug). PR shape passed to dispatchMerge has all required PrForCi fields including head?.ref.

src/review-loop.test.ts — Test at MAX_ROUNDS rounds correctly asserts forceMerge: true, reason.includes("force-merging"), and comment body contains "auto-merging" / reviewer login / author login / round count.

Minor notes (non-blocking)

The comment text says boss will "squash-merge" — if the merge skill uses a different strategy this string will be stale. Not a correctness issue.
No new test in webhook-handlers.test.ts for the force-merge routing path, but the integration is a trivial 3-line if-block and the guard layer is thoroughly covered in review-loop.test.ts.

All acceptance criteria are met. Implementation is correct, safe, and the module boundaries are clean.

## ✅ APPROVED **CI**: Green on head `9a2ae95` (run #1715, 3m11s). ✅ ### What I checked **`src/review-loop.ts`** — `guardAuthorDispatch` correctly trips at `rounds >= MAX_ROUNDS`, posts the "auto-merging" comment, and returns `{ skip: true, forceMerge: true }`. The module stays pure: it returns a decision, the handler dispatches. Swallowing `createIssueComment` errors is documented and correct — the decision is the important effect, not the comment. **`src/webhook-ci.ts`** — `dispatchMerge(repo, pr, { force: true })` prepends a clear 9-line override block instructing boss to skip the approval check. The three remaining guards (PR open, `mergeable: true`, CI green) are still enforced by the boss merge skill — no hard bypass. **`src/webhook-handlers.ts`** — `handleChangesRequested` routes `cap.forceMerge → dispatchMerge(…, { force: true })`. The `return null` below it is the documented revert path (dead code today, not a bug). PR shape passed to `dispatchMerge` has all required `PrForCi` fields including `head?.ref`. **`src/review-loop.test.ts`** — Test at `MAX_ROUNDS rounds` correctly asserts `forceMerge: true`, `reason.includes("force-merging")`, and comment body contains "auto-merging" / reviewer login / author login / round count. ### Minor notes (non-blocking) - The comment text says boss will "squash-merge" — if the merge skill uses a different strategy this string will be stale. Not a correctness issue. - No new test in `webhook-handlers.test.ts` for the force-merge routing path, but the integration is a trivial 3-line if-block and the guard layer is thoroughly covered in `review-loop.test.ts`. All acceptance criteria are met. Implementation is correct, safe, and the module boundaries are clean.