fix(automerge): a base-merge must not strand a human approval (unblocks ctj#59)

igor commented

2026-07-21 00:37:32 +00:00

Collaborator

The human-side twin of the sha-binding, surfaced by ctj#59 (which #408 could not merge).

The deadlock

When the auto-merge base-merges a behind PR (to satisfy require-up-to-date), the head advances → Forgejo marks your APPROVED review stale → automerge_approved_by (correctly) won't count it. For a shadow-COMMENT PR — where your approval is the only merge signal — that strands the PR. ctj#59: you approved it, the auto-merge base-merged it, and it's logged "no human approve" ever since.

The fix

automerge_stale_approval_is_basemerge: a stale APPROVED whose approved commit is a parent of a merge-commit head = a base-merge (same diff), so the approval stands. A stale approval on a real new commit (normal 1-parent head) does not qualify — unreviewed code never merges on a walked-back head. Mirrors review_update_sha's base-merge handling for the shadow verdict. Applied to both the require_human and default gate paths.

Verified against ctj#59's actual head: Merge branch 'master' into agent/52…, parents [b8787e70 (your approved commit), <master>] — exactly the base-merge this rescues.

Tests

5 new unit tests: base-merge → rescue; real new commit → no rescue; commit-not-a-parent → no; live approval → not-here; stale RC → no. make test green.

Merge this and ctj#59 auto-merges on the next tick — no re-approve. That's the proof.

The human-side twin of the sha-binding, surfaced by ctj#59 (which #408 could not merge). ## The deadlock When the auto-merge base-merges a behind PR (to satisfy require-up-to-date), the head advances → Forgejo marks your `APPROVED` review **stale** → `automerge_approved_by` (correctly) won't count it. For a shadow-`COMMENT` PR — where your approval is the *only* merge signal — that strands the PR. ctj#59: you approved it, the auto-merge base-merged it, and it's logged "no human approve" ever since. ## The fix `automerge_stale_approval_is_basemerge`: a **stale** `APPROVED` whose approved commit is a **parent of a merge-commit head** = a base-merge (same diff), so the approval stands. A stale approval on a **real new commit** (normal 1-parent head) does *not* qualify — unreviewed code never merges on a walked-back head. Mirrors `review_update_sha`'s base-merge handling for the shadow verdict. Applied to both the `require_human` and default gate paths. Verified against ctj#59's actual head: `Merge branch 'master' into agent/52…`, parents `[b8787e70 (your approved commit), <master>]` — exactly the base-merge this rescues. ## Tests 5 new unit tests: base-merge → rescue; real new commit → no rescue; commit-not-a-parent → no; live approval → not-here; stale RC → no. `make test` green. Merge this and **ctj#59 auto-merges on the next tick** — no re-approve. That's the proof.

igor added 1 commit

2026-07-21 00:37:32 +00:00

fix(automerge): a base-merge must not strand a human approval

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

Details

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

Details

7d1537075e

The auto-merge branch-update (base-merge to satisfy require-up-to-date) advances
the head, which makes Forgejo mark the human APPROVED review stale -- and
automerge_approved_by (correctly) wont count a stale approval. For a shadow-
COMMENT PR where the human approval is the ONLY merge signal, that deadlocks the
PR (ctj#59: approved, base-merged, then stuck logging "no human approve").

New automerge_stale_approval_is_basemerge rescues it: a STALE APPROVED whose
approved commit is a parent of a MERGE-commit head = a base-merge (same diff), so
the approval stands. A stale approval on a real new commit (1-parent head) does
NOT qualify, so unreviewed code never merges. Human-side twin of
review_update_sha. Applied to both the require_human and default gate paths.
5 new unit tests.

igor commented

2026-07-21 00:41:54 +00:00

Author

Collaborator

🤖 Review — `REQUEST_CHANGES` (automated)

CI for 7d153707: success

Summary: Adds automerge_stale_approval_is_basemerge to rescue a human APPROVED review that Forgejo staled when the auto-merge base-merged a behind PR, and wires it into both the require_human and default merge gates. CI is green, scope is tight (~one helper + tests), and the checklist claims match the diff. My block is a correctness/security gap in the rescue predicate itself.

Blocking concern — the "same diff" guarantee isn't actually checked (lib/automerge.sh, automerge_stale_approval_is_basemerge):

The whole safety claim is "the head is only a base-merge of the approved commit (same diff), so the approval should still stand." But the predicate only verifies:

(.parents | length) >= 2 and ([.parents[].sha] | any(. == $c))

i.e. "head is a merge commit AND the approved commit is one of its parents." It never verifies that the other parent(s) belong to the base branch. That is not equivalent to "same diff."

Concrete failure: head = merge(approved_commit, evil_branch) where evil_branch carries new, unreviewed files. This head has 2 parents, one of which is the approved commit, so it passes — yet the PR diff (head vs base) now includes evil_branch's changes, which no human ever approved. On the default path the rescue is OR'd in ahead of the reviewed_sha == head shadow check, so the shadow's approval-at-head requirement is bypassed too. The PR branch tip is controlled by the agent that opened the PR, so this shape is reachable by a misbehaving/rogue agent — exactly the class of "unreviewed code never merges on a walked-back head" the comment claims to prevent. The comment only reasons about the 1-parent case; the 2-parent-with-hostile-second-parent case is the hole.

A true base-merge always has its non-approved parent on the base branch. To actually honor the "same diff" claim, the predicate should additionally require that the non-approved parent is the base branch tip (or an ancestor of it) — e.g. fetch the PR's base sha and assert the other parent equals/descends from it, or compare the tree diffs. As written, "is a parent" is necessary but not sufficient.

Please either (a) tighten the check so the non-approved parent(s) are constrained to the base branch, or (b) if review_update_sha's mirrored handling already enforces this upstream and this helper is guaranteed to only see auto-merge-produced heads, explain why an agent-pushed merge commit can't reach this gate — I can't confirm that from the diff alone.

Test coverage note: The 5 tests exercise base-merge / real-new-commit / not-a-parent / live / stale-RC, but there is no test for the dangerous case above: a merge head whose approved parent is present but whose second parent is an unrelated (non-base) commit. That case currently returns "rescue," which is the bug. A regression test for it should accompany the fix.

Everything else (jq filtering, RC-veto preservation via last, dismissed-filtering, gate wiring) looks correct.

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

### 🤖 Review — `REQUEST_CHANGES` _(automated)_ CI for `7d153707`: **success** **Summary:** Adds `automerge_stale_approval_is_basemerge` to rescue a human `APPROVED` review that Forgejo staled when the auto-merge base-merged a behind PR, and wires it into both the `require_human` and default merge gates. CI is green, scope is tight (~one helper + tests), and the checklist claims match the diff. My block is a correctness/security gap in the rescue predicate itself. **Blocking concern — the "same diff" guarantee isn't actually checked (lib/automerge.sh, `automerge_stale_approval_is_basemerge`):** The whole safety claim is "the head is *only* a base-merge of the approved commit (same diff), so the approval should still stand." But the predicate only verifies: ``` (.parents | length) >= 2 and ([.parents[].sha] | any(. == $c)) ``` i.e. "head is a merge commit AND the approved commit is *one of* its parents." It never verifies that the **other** parent(s) belong to the base branch. That is not equivalent to "same diff." Concrete failure: head = `merge(approved_commit, evil_branch)` where `evil_branch` carries new, unreviewed files. This head has 2 parents, one of which is the approved commit, so it passes — yet the PR diff (head vs base) now includes `evil_branch`'s changes, which no human ever approved. On the default path the rescue is OR'd in ahead of the `reviewed_sha == head` shadow check, so the shadow's approval-at-head requirement is bypassed too. The PR branch tip is controlled by the agent that opened the PR, so this shape is reachable by a misbehaving/rogue agent — exactly the class of "unreviewed code never merges on a walked-back head" the comment claims to prevent. The comment only reasons about the 1-parent case; the 2-parent-with-hostile-second-parent case is the hole. A true base-merge always has its non-approved parent on the base branch. To actually honor the "same diff" claim, the predicate should additionally require that the non-approved parent is the base branch tip (or an ancestor of it) — e.g. fetch the PR's base sha and assert the *other* parent equals/descends from it, or compare the tree diffs. As written, "is a parent" is necessary but not sufficient. Please either (a) tighten the check so the non-approved parent(s) are constrained to the base branch, or (b) if `review_update_sha`'s mirrored handling already enforces this upstream and this helper is guaranteed to only see auto-merge-produced heads, explain why an agent-pushed merge commit can't reach this gate — I can't confirm that from the diff alone. **Test coverage note:** The 5 tests exercise base-merge / real-new-commit / not-a-parent / live / stale-RC, but there is no test for the dangerous case above: a merge head whose approved parent is present but whose *second* parent is an unrelated (non-base) commit. That case currently returns "rescue," which is the bug. A regression test for it should accompany the fix. Everything else (jq filtering, RC-veto preservation via `last`, dismissed-filtering, gate wiring) looks correct. --- <sub>Independent review by the harness on `claude-opus-4-8` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-21 00:41:55 +00:00

1 minute 18 seconds

igor self-assigned this

2026-07-21 00:41:55 +00:00

igor commented

2026-07-21 00:49:13 +00:00

Author

Collaborator

🔧 Rework — round 1 (automated)

Addressed the review on claude-opus-4-8 at effort high — 1 new commit(s).

### 🔧 Rework — round 1 _(automated)_ Addressed the review on `claude-opus-4-8` at **effort high** — 1 new commit(s).

igor removed their assignment

2026-07-21 00:49:40 +00:00

igor added spent time

2026-07-21 00:49:41 +00:00

4 minutes 51 seconds

igor added 1 commit

2026-07-21 00:49:41 +00:00

test: verify base-branch ancestry for basemerge rescue

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

Details

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

Details

c7767bb4c6

igor commented

2026-07-21 00:53:16 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for c7767bb4: success

Adds automerge_stale_approval_is_basemerge, a helper that lets a human APPROVED review survive being staled by the auto-merge's own base-merge (the ctj#59 deadlock). Focused (~110 lines), CI green, tests included. My one real reservation is a subtle gap in the security-critical check — surfacing it for you to weigh rather than blocking, since it mirrors the author's stated model of review_update_sha and I can't confirm exploitability from the diff alone.

Key concern — the parent-ancestry check is a proxy for "same net diff," and misses evil merges

lib/automerge.sh: the guard verifies (a) the approved commit is a parent of head, and (b) every other parent is an ancestor-or-equal of the current base tip (compare/base_tip...parent → total_commits == 0). That closes the obvious attack the author calls out — merge(approved_commit, evil_branch) where evil_branch carries unreviewed commits (it'd be ahead of base → rejected). Good.

But it does not verify the head merge commit's own tree. A merge commit can introduce changes present in neither parent (a classic "evil merge" / hostile conflict resolution). A head of shape merge(approved_commit, base_tip) whose tree injects unreviewed files passes both checks — both parents are legit ancestors — yet its net diff vs base now contains code no human approved. The author's own threat model ("the PR tip is agent-controlled, so a rogue/misbehaving agent could push exactly that shape") is precisely the scenario that defeats a parent-only check. Since this PR newly opens a merge path for stale approvals (previously they just stranded — fail-closed), the tightness of the condition matters.

The robust check is diff/tree equivalence: assert head's net diff vs base equals the approved commit's net diff vs base (or that head's tree equals a clean merge of its parents), not just parent ancestry.
If this gap is knowingly accepted as mirroring review_update_sha's existing behavior, that's a defensible call — but it deserves an explicit note so a human isn't surprised later. I've left it as COMMENT rather than a block because I can't confirm from the diff whether igor's actual post-approval flow ever exposes an agent-crafted head to this helper.

Smaller notes

Description says "5 new unit tests" but the diff adds 6 test cases (rescue, non-base-branch → no rescue, real new commit → no, not-a-parent → no, live approval → no, stale RC → no). Undercount, not a fabrication — every claimed test exists, plus one extra. Fine.
Failure/empty-response paths all return 1 (fail closed) — good hygiene throughout.
Latest-review selection (sort_by(.submitted_at) | last, filtering APPROVED|REQUEST_CHANGES) correctly lets a later RC override an earlier approval → return 1. Verified against the test cases.

Test coverage

The unit tests are well-constructed and cover the important branches (base-merge rescue, hostile non-base branch, normal new commit, commit-not-a-parent, live approval, stale RC). The one branch they don't exercise is the evil-merge-tree case above — because the helper has no logic for it. If you accept the diff-equivalence hardening, that'd want a test too.

Net: solid, in-scope fix for a real deadlock, honestly described. Withholding APPROVE only on the evil-merge question — worth a human's eyes before this merge gate ships.

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

### 🤖 Review — `COMMENT` _(automated)_ CI for `c7767bb4`: **success** Adds `automerge_stale_approval_is_basemerge`, a helper that lets a human `APPROVED` review survive being staled by the auto-merge's own base-merge (the ctj#59 deadlock). Focused (~110 lines), CI green, tests included. My one real reservation is a subtle gap in the security-critical check — surfacing it for you to weigh rather than blocking, since it mirrors the author's stated model of `review_update_sha` and I can't confirm exploitability from the diff alone. ## Key concern — the parent-ancestry check is a proxy for "same net diff," and misses evil merges `lib/automerge.sh`: the guard verifies (a) the approved commit is a parent of head, and (b) every *other* parent is an ancestor-or-equal of the current base tip (`compare/base_tip...parent` → `total_commits == 0`). That closes the obvious attack the author calls out — `merge(approved_commit, evil_branch)` where evil_branch carries unreviewed commits (it'd be ahead of base → rejected). Good. But it does **not** verify the head merge commit's *own tree*. A merge commit can introduce changes present in neither parent (a classic "evil merge" / hostile conflict resolution). A head of shape `merge(approved_commit, base_tip)` whose tree injects unreviewed files passes **both** checks — both parents are legit ancestors — yet its net diff vs base now contains code no human approved. The author's own threat model ("the PR tip is agent-controlled, so a rogue/misbehaving agent could push exactly that shape") is precisely the scenario that defeats a parent-only check. Since this PR newly *opens* a merge path for stale approvals (previously they just stranded — fail-closed), the tightness of the condition matters. - The robust check is diff/tree equivalence: assert head's net diff vs base equals the approved commit's net diff vs base (or that head's tree equals a clean merge of its parents), not just parent ancestry. - If this gap is knowingly accepted as mirroring `review_update_sha`'s existing behavior, that's a defensible call — but it deserves an explicit note so a human isn't surprised later. I've left it as COMMENT rather than a block because I can't confirm from the diff whether igor's actual post-approval flow ever exposes an agent-crafted head to this helper. ## Smaller notes - Description says "5 new unit tests" but the diff adds **6** test cases (rescue, non-base-branch → no rescue, real new commit → no, not-a-parent → no, live approval → no, stale RC → no). Undercount, not a fabrication — every claimed test exists, plus one extra. Fine. - Failure/empty-response paths all `return 1` (fail closed) — good hygiene throughout. - Latest-review selection (`sort_by(.submitted_at) | last`, filtering APPROVED|REQUEST_CHANGES) correctly lets a later RC override an earlier approval → `return 1`. Verified against the test cases. ## Test coverage The unit tests are well-constructed and cover the important branches (base-merge rescue, hostile non-base branch, normal new commit, commit-not-a-parent, live approval, stale RC). The one branch they don't exercise is the evil-merge-tree case above — because the helper has no logic for it. If you accept the diff-equivalence hardening, that'd want a test too. Net: solid, in-scope fix for a real deadlock, honestly described. Withholding APPROVE only on the evil-merge question — worth a human's eyes before this merge gate ships. --- <sub>Independent review by the harness on `claude-opus-4-8` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-21 00:53:16 +00:00

1 minute 57 seconds

igor requested review from joshtronic

2026-07-21 00:53:17 +00:00