fix(automerge): a human APPROVED merges any repo (default path was shadow-only) #408

Merged

joshtronic merged 1 commit from fix/human-approve-merges-default-repo into master

2026-07-21 00:19:00 +00:00

igor commented

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

Collaborator

Gap surfaced by ctj#59/#61: you approved both, but they wouldn't merge.

The bug

On a default (shadow-gated) repo, the gate from #405 only checked the shadow verdict — a human FORGEJO_REVIEWER APPROVED review was ignored. So when the shadow gave a COMMENT (routing to you) and you then approved, the PR sat unmerged. That contradicts the design: "a human can always approve/merge any repo."

Fix

The default path now merges on either signal:

a human FORGEJO_REVIEWER APPROVED review (human override — merges any repo), or
the shadow's APPROVE for the current head (unchanged, incl. the sha-binding).

A REQUEST_CHANGES — shadow's or the human's — still blocks either path (overriding a flagged problem is a deliberate manual merge, not an auto-merge). The require_human carve-out path is unchanged.

Tests

Added: human APPROVED overrides a shadow COMMENT → merges; shadow RC blocks even a human approve. Existing shadow-path e2e tests retargeted to "no human review" so they still exercise the shadow path. make test green.

Once this self-deploys, your approval on any default-repo PR merges it — ctj#59/#61 will auto-merge on the next tick (your APPROVED + green CI are already there).

Gap surfaced by ctj#59/#61: you approved both, but they wouldn't merge. ## The bug On a default (shadow-gated) repo, the gate from #405 only checked the **shadow** verdict — a human `FORGEJO_REVIEWER` `APPROVED` review was **ignored**. So when the shadow gave a `COMMENT` (routing to you) and you then approved, the PR sat unmerged. That contradicts the design: "a human can always approve/merge any repo." ## Fix The default path now merges on **either** signal: - a human `FORGEJO_REVIEWER` `APPROVED` review (human override — merges any repo), **or** - the shadow's `APPROVE` for the current head (unchanged, incl. the sha-binding). A `REQUEST_CHANGES` — shadow's *or* the human's — still blocks either path (overriding a flagged problem is a deliberate manual merge, not an auto-merge). The `require_human` carve-out path is unchanged. ## Tests Added: human `APPROVED` overrides a shadow `COMMENT` → merges; shadow `RC` blocks even a human approve. Existing shadow-path e2e tests retargeted to "no human review" so they still exercise the shadow path. `make test` green. Once this self-deploys, your approval on any default-repo PR merges it — **ctj#59/#61 will auto-merge on the next tick** (your APPROVED + green CI are already there).

igor added 1 commit

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

fix(automerge): a human APPROVED merges any repo (default path was shadow-only)

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

Details

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

Details

425cfb004c

Gap in the review-gate rollout: on a default (shadow-gated) repo the gate only
consulted the shadow verdict -- a human FORGEJO_REVIEWER APPROVED review was
ignored. So when the shadow only COMMENTed and the human reviewed + approved
(ctj#59/#61), the PR sat unmerged despite the approval, contradicting the design
("a human can always approve/merge any repo").

Default path now merges on EITHER a human APPROVED OR the shadow current-head
APPROVE, and never over a REQUEST_CHANGES (shadow or human). Tests: human
APPROVED overrides a shadow COMMENT -> merges; shadow RC blocks even a human
approve; existing shadow-path tests retargeted to no-human so they still
exercise the shadow path.

igor commented

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

Author

Collaborator

🤖 Review — `APPROVE` (automated)

CI for 425cfb00: success

Small, well-scoped fix to lib/automerge.sh's default (shadow-gated) merge branch so a human FORGEJO_REVIEWER APPROVED review can merge even when the shadow only COMMENTed, while any REQUEST_CHANGES (shadow's or human's) still blocks. Logic and tests check out; CI green.

Correctness review of the rewritten else branch:

REQUEST_CHANGES (shadow) → continue. Then automerge_reviewer_blocks (human RC) → continue. Only after both block-checks does it merge on human-approve OR shadow-APPROVE-of-current-head. Order is right: a shadow RC blocks even a human approve, matching the stated design and the new test.
Stale-APPROVE safety preserved: the elif [ "$verdict" = "APPROVE" ] && [ "$reviewed_sha" = "$head" ] retains the sha-binding, so an APPROVE for an older sha still falls through to the else/continue. The existing oldsha0 test still exercises this.
CI gate is untouched — forgejo_commit_status still runs after this block, so the human override does not bypass green-CI.
require_human path (if branch) is unchanged, as claimed.

Test hygiene: The flip of the shared mock automerge_approved_by() { return 0 → 1 } is necessary and correct — the new code now calls approved_by on the shadow path, so leaving it at return 0 would have masked every shadow-path assertion via the human override. It's reset back before the multi-repo sections. New tests cover both the override-merges and RC-blocks-override cases.

Checklist honesty: Description's claims (human override merges over shadow COMMENT; shadow RC blocks; existing e2e retargeted to "no human review") all correspond to real diff hunks.

No blocking issues. Scope is tight (~30 lines), no drive-by changes, tests added, CI `success`.

_{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 — `APPROVE` _(automated)_ CI for `425cfb00`: **success** Small, well-scoped fix to `lib/automerge.sh`'s default (shadow-gated) merge branch so a human `FORGEJO_REVIEWER` `APPROVED` review can merge even when the shadow only `COMMENT`ed, while any `REQUEST_CHANGES` (shadow's or human's) still blocks. Logic and tests check out; CI green. **Correctness review of the rewritten `else` branch:** - `REQUEST_CHANGES` (shadow) → `continue`. Then `automerge_reviewer_blocks` (human RC) → `continue`. Only after both block-checks does it merge on human-approve OR shadow-APPROVE-of-current-head. Order is right: a shadow RC blocks even a human approve, matching the stated design and the new test. - Stale-APPROVE safety preserved: the `elif [ "$verdict" = "APPROVE" ] && [ "$reviewed_sha" = "$head" ]` retains the sha-binding, so an APPROVE for an older sha still falls through to the `else`/`continue`. The existing `oldsha0` test still exercises this. - CI gate is untouched — `forgejo_commit_status` still runs after this block, so the human override does not bypass green-CI. - `require_human` path (`if` branch) is unchanged, as claimed. **Test hygiene:** The flip of the shared mock `automerge_approved_by() { return 0 → 1 }` is necessary and correct — the new code now *calls* `approved_by` on the shadow path, so leaving it at `return 0` would have masked every shadow-path assertion via the human override. It's reset back before the multi-repo sections. New tests cover both the override-merges and RC-blocks-override cases. **Checklist honesty:** Description's claims (human override merges over shadow COMMENT; shadow RC blocks; existing e2e retargeted to "no human review") all correspond to real diff hunks. No blocking issues. Scope is tight (~30 lines), no drive-by changes, tests added, CI `success`. ==== --- <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:13:59 +00:00

37 seconds