joshtronic/igor

Watch

feat(automerge): handle require-up-to-date -- update behind branches instead of failing #265

Merged

joshtronic merged 1 commit from feat/automerge-update-behind into master

2026-06-26 16:39:52 +00:00

igor commented

2026-06-26 16:05:02 +00:00

Collaborator

Fixes the merge API failed warnings — and the underlying starve.

What was happening (verified live this morning): a ready PR that's behind its base passes every existing gate (approved, CI green, mergeable), but the merge API rejects it because require-up-to-date is on. So do_automerge_tick POSTed a doomed merge and logged merge API failed every single tick. And these do not self-heal — porksicle#82 sat 5-behind and failing for 14+ minutes (including 22 minutes with master perfectly stable); #81 only merged after I manually ran a branch update on it.

The fix — the gate now checks up-to-date-ness itself, and handles "behind" instead of fighting it:

automerge_behind_count (PR head vs base via the compare API): 0 = current, >0 = behind, -1 = can't tell.
current → merge (unchanged).
behind → automerge_update_branch (Forgejo's base-merge) so it satisfies the requirement; CI re-runs and it merges next cycle. The human approval survives the base-merge (verified on #81), and the shadow review's patch-id dedup skips the same-net-diff head.
inconclusive (-1) → skip the tick, never a blind update.

Anti-thrash for your fast-moving master: it prefers to merge a current PR each tick (master advances once), and updates only one behind PR when none is current. The deploy barrier already freezes master between deploys, which gives the update its window to land.

Net effect: no spurious warnings, and behind PRs self-unblock — your "perfect world."

Tests cover behind_count (count / up-to-date / API-error), update_branch, and the tick's merge-vs-update-vs-skip decision. bash -n + check-sync green. No reviewer.

Note: once this deploys, the stuck #82 (and any siblings) get picked up automatically — no manual bump.

**Fixes the `merge API failed` warnings** — and the underlying starve. **What was happening (verified live this morning):** a ready PR that's behind its base passes every existing gate (approved, CI green, mergeable), but the merge API rejects it because **require-up-to-date** is on. So `do_automerge_tick` POSTed a doomed merge and logged `merge API failed` every single tick. And these do **not** self-heal — porksicle#82 sat 5-behind and failing for 14+ minutes (including 22 minutes with master perfectly stable); #81 only merged after I manually ran a branch `update` on it. **The fix** — the gate now checks up-to-date-ness itself, and handles "behind" instead of fighting it: - `automerge_behind_count` (PR head vs base via the compare API): `0` = current, `>0` = behind, `-1` = can't tell. - **current** → merge (unchanged). - **behind** → `automerge_update_branch` (Forgejo's base-merge) so it satisfies the requirement; CI re-runs and it merges next cycle. The human **approval survives** the base-merge (verified on #81), and the shadow review's patch-id dedup skips the same-net-diff head. - **inconclusive** (`-1`) → skip the tick, never a blind update. **Anti-thrash for your fast-moving master:** it prefers to merge a *current* PR each tick (master advances once), and updates only **one** behind PR when none is current. The deploy barrier already freezes master between deploys, which gives the update its window to land. **Net effect: no spurious warnings, and behind PRs self-unblock** — your "perfect world." Tests cover `behind_count` (count / up-to-date / API-error), `update_branch`, and the tick's merge-vs-update-vs-skip decision. `bash -n` + check-sync green. No reviewer. Note: once this deploys, the stuck #82 (and any siblings) get picked up automatically — no manual bump.

igor added 1 commit

2026-06-26 16:05:02 +00:00

feat(automerge): handle require-up-to-date -- update behind branches instead of failing

Lint / check-sync (push) Successful in 5s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

5341e7c53b

A ready PR that's behind its base passes every existing gate (approved, CI green,
mergeable) but the merge API rejects it because require-up-to-date is on -- so
do_automerge_tick POSTed a doomed merge and logged 'merge API failed' every tick.
And behind PRs don't self-heal: porksicle#82 sat 5-behind and failing for 14+ min
(incl. 22 min of stable master); #81 only merged after a manual branch update.

The gate now checks up-to-date-ness itself (automerge_behind_count: PR head vs
base via the compare API; 0 = current, >0 = behind, -1 = can't tell -> skip):
- current -> merge (as before).
- behind  -> automerge_update_branch (Forgejo base-merge) so it satisfies the
  requirement; CI re-runs and it merges next cycle. The human APPROVAL survives
  the base-merge (verified live on #81), and the shadow review's patch-id dedup
  skips the same-net-diff head.
- To not thrash a fast-moving master: prefer merging a CURRENT pr each tick, and
  update only ONE behind pr when none is current. The deploy barrier freezes
  master between deploys, giving the update its window.

Result: no spurious 'merge API failed' warnings, and behind PRs self-unblock.
Tests cover behind_count (count / up-to-date / error), update_branch, and the
tick's merge-vs-update-vs-skip decision. bash -n + check-sync green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011KXPafSYJPY6XkWHHk1Lfs

igor force-pushed feat/automerge-update-behind from 5341e7c53b

Lint / check-sync (push) Successful in 5s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

to 9644019c53

Lint / check-sync (push) Successful in 5s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

2026-06-26 16:08:20 +00:00

Compare

igor commented

2026-06-26 16:11:11 +00:00

Author

Collaborator

🤖 Review — `APPROVE` (automated)

CI for 9644019c: success

Summary: Adds a require-up-to-date handler to do_automerge_tick: instead of POSTing a merge that the API will reject for a behind-base PR (the old merge API failed warning loop), the gate now measures behind-ness itself and updates the branch instead. Focused, tested, CI green. Approving.

What I verified:

Compare direction is correct (the crux). automerge_behind_count calls compare/${head}...${base}, i.e. base-side = PR head SHA, head-side = base ref, so total_commits = commits the base has that the PR head lacks = behind count. This matches the stated intent ("base-branch commits the PR head is missing"). Had it been base...head the feature would be inverted; it isn't.
jq fallbacks are sound. (.total_commits // (.commits | length) // 0) returns 0 for total_commits: 0 (jq // only falls through on null/false, not 0), and else -1 / || echo -1 cover non-object and parse-failure cases. Relies on _fj returning non-zero on API failure, which matches the existing convention in automerge_do_merge.
Tick decision flow. behind=0 → merge + return 0 (unchanged path); >0 → remember the first behind PR but keep scanning so a later CURRENT PR still wins (return 0 on merge); -1/other → log + continue. Only-one-update-per-tick anti-thrash is implemented as described. [ "$behind" -gt 0 ] 2>/dev/null safely treats non-numeric/-1 as the else branch.
Tests match the description's claims. behind_count count/up-to-date/API-error, update_branch POST path, and the tick's merge-vs-update-vs-skip (echo 3 → update-not-merge, no deploy; echo -1 → rc1, no update) are all present and meaningful. No CI-workflow files touched; docs/test/lib only; ~70 lines.

Non-blocking notes (for the human, no rework needed):

This makes every merge — including already-current PRs — depend on the compare API succeeding. A transient compare failure now returns -1 and skips a merge that previously would have gone through; it self-heals next tick, but it's a real new dependency worth being aware of.
The inconclusive (-1) log says "skipping this tick," but the code only skips that PR — a later current PR in the same stream still merges. The behavior is arguably better than the message implies; just a cosmetic mismatch.
The tick-decision tests mock automerge_behind_count wholesale, so the real compare logic and the tick are never exercised together. Each is unit-tested in isolation (standard), but there's no integration coverage of the URL the real function builds.

None of these rise to a blocker. Logic, scope, honesty, and CI all check out.

_{Independent review by the harness on claude-opus-4-8. The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `APPROVE` _(automated)_ CI for `9644019c`: **success** **Summary:** Adds a `require-up-to-date` handler to `do_automerge_tick`: instead of POSTing a merge that the API will reject for a behind-base PR (the old `merge API failed` warning loop), the gate now measures behind-ness itself and updates the branch instead. Focused, tested, CI green. Approving. **What I verified:** - **Compare direction is correct (the crux).** `automerge_behind_count` calls `compare/${head}...${base}`, i.e. base-side = PR head SHA, head-side = base ref, so `total_commits` = commits the base has that the PR head lacks = behind count. This matches the stated intent ("base-branch commits the PR head is missing"). Had it been `base...head` the feature would be inverted; it isn't. - **jq fallbacks are sound.** `(.total_commits // (.commits | length) // 0)` returns 0 for `total_commits: 0` (jq `//` only falls through on null/false, not 0), and `else -1` / `|| echo -1` cover non-object and parse-failure cases. Relies on `_fj` returning non-zero on API failure, which matches the existing convention in `automerge_do_merge`. - **Tick decision flow.** `behind=0` → merge + return 0 (unchanged path); `>0` → remember the *first* behind PR but keep scanning so a later CURRENT PR still wins (return 0 on merge); `-1`/other → log + continue. Only-one-update-per-tick anti-thrash is implemented as described. `[ "$behind" -gt 0 ] 2>/dev/null` safely treats non-numeric/`-1` as the else branch. - **Tests match the description's claims.** `behind_count` count/up-to-date/API-error, `update_branch` POST path, and the tick's merge-vs-update-vs-skip (`echo 3` → update-not-merge, no deploy; `echo -1` → rc1, no update) are all present and meaningful. No CI-workflow files touched; docs/test/lib only; ~70 lines. **Non-blocking notes (for the human, no rework needed):** - This makes **every** merge — including already-current PRs — depend on the compare API succeeding. A transient compare failure now returns `-1` and *skips* a merge that previously would have gone through; it self-heals next tick, but it's a real new dependency worth being aware of. - The inconclusive (`-1`) log says "skipping this tick," but the code only skips *that PR* — a later current PR in the same stream still merges. The behavior is arguably better than the message implies; just a cosmetic mismatch. - The tick-decision tests mock `automerge_behind_count` wholesale, so the real compare logic and the tick are never exercised together. Each is unit-tested in isolation (standard), but there's no integration coverage of the URL the real function builds. None of these rise to a blocker. Logic, scope, honesty, and CI all check out. --- <sub>Independent review by the harness on `claude-opus-4-8`. The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-06-26 16:11:11 +00:00

2 minutes 2 seconds

igor requested review from joshtronic

2026-06-26 16:11:11 +00:00