feat(automerge): shadow-review APPROVE gates default; require_human pins carve-outs

igor commented

2026-07-20 22:02:01 +00:00

Collaborator

Step ③ of the review-gate rollout (spec merged as igor#404). The carve-out flag PRs (igor.bot#335, joshing.you#146, porksicle.com#143) are already merged, so the flags-first prerequisite is satisfied — this is safe to land.

What changes

do_automerge_tick approval gate becomes dual-path:

Flagged repo (agent.json .automerge.require_human: true) → human FORGEJO_REVIEWER APPROVED, exactly as today.
Default repo → the shadow review verdict == APPROVE (APPROVE-only; a COMMENT routes to you, never auto-merges).
A live REQUEST_CHANGES vetoes either path — the shadow’s on the human path, the human’s on the default path (new automerge_reviewer_blocks, so you can still stop any PR by requesting changes).
igor unchanged: never-merge via no smoke.url + AUTOMERGE_SELF_REPO.
All existing gates (CI green, cleanly mergeable, not-behind) untouched.

Tests

New unit tests for automerge_require_human and automerge_reviewer_blocks; the end-to-end do_automerge_tick tests updated to the new default and now cover both the shadow path and the human-flagged path (plus the human-RC veto). make test green.

⚠️ Heads-up on merging

This activates shadow-gating fleet-wide the moment it self-deploys. Any open PR on a non-carve-out repo that already has a shadow APPROVE + green CI will auto-merge on the next tick. Merge when you’re ready for that.

Step ③ of the review-gate rollout (spec merged as igor#404). The carve-out flag PRs (igor.bot#335, joshing.you#146, porksicle.com#143) are already merged, so the flags-first prerequisite is satisfied — this is safe to land. ## What changes `do_automerge_tick` approval gate becomes dual-path: - **Flagged repo** (`agent.json` `.automerge.require_human: true`) → human `FORGEJO_REVIEWER` APPROVED, exactly as today. - **Default repo** → the **shadow review verdict == `APPROVE`** (APPROVE-only; a `COMMENT` routes to you, never auto-merges). - A live `REQUEST_CHANGES` vetoes **either** path — the shadow’s on the human path, the human’s on the default path (new `automerge_reviewer_blocks`, so you can still stop any PR by requesting changes). - **igor** unchanged: never-merge via no `smoke.url` + `AUTOMERGE_SELF_REPO`. - All existing gates (CI green, cleanly mergeable, not-behind) untouched. ## Tests New unit tests for `automerge_require_human` and `automerge_reviewer_blocks`; the end-to-end `do_automerge_tick` tests updated to the new default and now cover both the shadow path and the human-flagged path (plus the human-RC veto). `make test` green. ## ⚠️ Heads-up on merging This **activates shadow-gating fleet-wide the moment it self-deploys.** Any open PR on a non-carve-out repo that already has a shadow `APPROVE` + green CI will auto-merge on the next tick. Merge when you’re ready for that.

igor added 1 commit

2026-07-20 22:02:01 +00:00

feat(automerge): shadow-review APPROVE gates the default; require_human pins carve-outs

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

Details

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

Details

85d7481b53

do_automerge_tick approval gate is now dual-path (design: spec in
docs/superpowers/specs/2026-07-20-automerge-review-gate-and-ship-report-design.md):
- flagged repo (agent.json .automerge.require_human) -> human FORGEJO_REVIEWER
  APPROVED, as today;
- default repo -> the shadow reviews verdict == APPROVE (APPROVE only, never
  COMMENT).
A live REQUEST_CHANGES vetoes either path -- the shadow's on the human path, the
human's on the default path (new automerge_reviewer_blocks). igor stays
never-merge via no smoke.url + AUTOMERGE_SELF_REPO. All existing CI/mergeable/
behind gates unchanged. Adds automerge_require_human + automerge_reviewer_blocks
with unit tests; end-to-end tests updated to the new default and cover both paths.

igor commented

2026-07-20 22:04:43 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 85d7481b: success

Makes the automerge approval gate dual-path: human-flagged repos keep the FORGEJO_REVIEWER APPROVED gate, default repos now merge on a shadow-review APPROVE (with a human REQUEST_CHANGES vetoing either path). Scope is tight, checklist matches the diff, CI green. One correctness question I can't resolve from the diff keeps me off APPROVE.

Main concern — is the shadow `APPROVE` bound to the current head sha?

On the new default path the positive gate is the state-file verdict:

verdict=$(jq -r --arg k "$key" '.review[$k].verdict // ""' "$sf" ...)
...
if [ "$verdict" != "APPROVE" ]; then ... continue; fi

The key is ${repo}#${pr} — not keyed by, or checked against, the PR head sha (head=$(_fj ...) is fetched afterward but never compared to whatever commit the shadow reviewed).

Before this PR the shadow verdict was only ever used as a veto (REQUEST_CHANGES blocks). A stale veto fails safe (over-blocks). Now a stale APPROVE fails open: if the shadow approves commit A, the author pushes commit B, and the review-writer hasn't re-reviewed B yet, .review[repo#pr].verdict is still APPROVE and the tick will merge B — unreviewed code — as long as CI on B is green. The human path is protected from exactly this by Forgejo's own review-staleness/dismissal on new commits; the default path has no equivalent here.

I can't see the code that writes .review[$k].verdict, so I can't tell whether it's sha-gated (e.g. overwrites/clears on head change, or the verdict record carries the reviewed sha). If it is, this is a non-issue — please confirm where. If it isn't, this defeats the purpose of the gate and should block: bind the stored verdict to the head sha and require the shadow APPROVE to be for the current head before auto-merging.

Things that look correct

automerge_reviewer_blocks is a faithful inverted mirror of automerge_approved_by (latest decision-review by submitted_at, non-dismissed, non-stale). The five new unit tests cover live-RC/RC-then-approve/only-approve/stale/other-user and all behave as expected.
automerge_require_human defaults correctly (missing key / missing agent.json / explicit false → shadow gate), tested.
e2e tests exercise both paths incl. the human-RC-vetoes-shadow-APPROVE and flagged-no-human-approve-with-shadow-APPROVE-present cases. Good coverage of the branch matrix.
No secrets/injection; jq uses --arg throughout.

Note

The description's own "activates fleet-wide the moment it self-deploys" warning is accurate — this is the human's timing call, not a code defect. My hold is purely the sha-binding question above; resolve/confirm that and I'd be comfortable.

_{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 `85d7481b`: **success** Makes the automerge approval gate dual-path: human-flagged repos keep the `FORGEJO_REVIEWER` APPROVED gate, default repos now merge on a shadow-review `APPROVE` (with a human `REQUEST_CHANGES` vetoing either path). Scope is tight, checklist matches the diff, CI green. One correctness question I can't resolve from the diff keeps me off APPROVE. ## Main concern — is the shadow `APPROVE` bound to the current head sha? On the new default path the *positive* gate is the state-file verdict: ```sh verdict=$(jq -r --arg k "$key" '.review[$k].verdict // ""' "$sf" ...) ... if [ "$verdict" != "APPROVE" ]; then ... continue; fi ``` The key is `${repo}#${pr}` — **not** keyed by, or checked against, the PR head sha (`head=$(_fj ...)` is fetched afterward but never compared to whatever commit the shadow reviewed). Before this PR the shadow verdict was only ever used as a *veto* (`REQUEST_CHANGES` blocks). A stale veto fails safe (over-blocks). Now a stale `APPROVE` fails *open*: if the shadow approves commit A, the author pushes commit B, and the review-writer hasn't re-reviewed B yet, `.review[repo#pr].verdict` is still `APPROVE` and the tick will merge B — unreviewed code — as long as CI on B is green. The human path is protected from exactly this by Forgejo's own review-staleness/dismissal on new commits; the default path has no equivalent here. I can't see the code that *writes* `.review[$k].verdict`, so I can't tell whether it's sha-gated (e.g. overwrites/clears on head change, or the verdict record carries the reviewed sha). If it is, this is a non-issue — please confirm where. If it isn't, this defeats the purpose of the gate and should block: bind the stored verdict to the head sha and require the shadow `APPROVE` to be *for the current head* before auto-merging. ## Things that look correct - `automerge_reviewer_blocks` is a faithful inverted mirror of `automerge_approved_by` (latest decision-review by `submitted_at`, non-dismissed, non-stale). The five new unit tests cover live-RC/RC-then-approve/only-approve/stale/other-user and all behave as expected. - `automerge_require_human` defaults correctly (missing key / missing agent.json / explicit false → shadow gate), tested. - e2e tests exercise both paths incl. the human-RC-vetoes-shadow-APPROVE and flagged-no-human-approve-with-shadow-APPROVE-present cases. Good coverage of the branch matrix. - No secrets/injection; jq uses `--arg` throughout. ## Note The description's own "activates fleet-wide the moment it self-deploys" warning is accurate — this is the human's timing call, not a code defect. My hold is purely the sha-binding question above; resolve/confirm that and I'd be comfortable. --- <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-20 22:04:43 +00:00

1 minute 3 seconds

igor requested review from joshtronic

2026-07-20 22:04:43 +00:00

igor commented

2026-07-20 22:17:25 +00:00

Author

Collaborator

Fixed — the sha-binding concern is correct, and it was a real fail-open.

Verified: review_record (tick.sh:959) stamps .review[key] = {sha, verdict, ...} — the verdict is stored with the sha it reviewed — but the default path only checked verdict == "APPROVE", never that the stored sha matched the current head. And do_automerge_tick runs before do_review_tick in the cascade, so the window is concrete: shadow approves A → commit B lands → automerge merges B on the stale APPROVE before the re-review ever happens.

Fix: the default path now requires .review[key].sha == head in addition to verdict == "APPROVE" (head sha fetched up front). A base-merge keeps sha == head (review_update_sha bumps the sha for a same-content head), so those still auto-merge; a genuine new commit leaves the recorded sha pointing at the old commit until re-review, so it waits. The human path was already covered by Forgejo’s own staleness flag — this gives the default path the equivalent.

New test covers the stale case (APPROVE recorded for an older sha → no merge); make test green.

Fixed — the sha-binding concern is correct, and it was a real fail-open. Verified: `review_record` (tick.sh:959) stamps `.review[key] = {sha, verdict, ...}` — the verdict is stored **with** the sha it reviewed — but the default path only checked `verdict == "APPROVE"`, never that the stored sha matched the current head. And `do_automerge_tick` runs **before** `do_review_tick` in the cascade, so the window is concrete: shadow approves A → commit B lands → automerge merges B on the stale APPROVE before the re-review ever happens. **Fix:** the default path now requires `.review[key].sha == head` in addition to `verdict == "APPROVE"` (head sha fetched up front). A base-merge keeps `sha == head` (`review_update_sha` bumps the sha for a same-content head), so those still auto-merge; a genuine new commit leaves the recorded sha pointing at the old commit until re-review, so it waits. The human path was already covered by Forgejo’s own staleness flag — this gives the default path the equivalent. New test covers the stale case (APPROVE recorded for an older sha → no merge); `make test` green.

igor referenced this pull request from a commit

2026-07-20 22:17:26 +00:00

fix(automerge): bind the default-path shadow APPROVE to the current head sha

igor added 1 commit

2026-07-20 22:17:26 +00:00

fix(automerge): bind the default-path shadow APPROVE to the current head sha

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

Details

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

Details

7ad3b5fa82

Shadow-review catch on #405: the default path gated only on
.review[key].verdict == APPROVE, never checking that the stored verdict was
for the CURRENT head. review_record stamps the verdict WITH the reviewed sha,
so a stale APPROVE (shadow approved A, a real commit B then landed, not yet
re-reviewed) would merge B unreviewed -- and do_automerge_tick runs before
do_review_tick in the cascade, so the window is concrete, not theoretical.

Fix: the default path now also requires .review[key].sha == head. A base-merge
keeps sha == head (review_update_sha updates the sha for a same-content head),
so those still qualify; a genuine new commit leaves the recorded sha pointing
at the old commit until re-review, so it waits. Head sha is now fetched up
front so both the gate and the CI check use it. New test: APPROVE recorded for
a stale sha -> no merge.

igor commented

2026-07-20 22:19:13 +00:00

Author

Collaborator

🤖 Review — `APPROVE` (automated)

CI for 7ad3b5fa: success

Summary: Makes do_automerge_tick's approval gate dual-path — flagged repos (agent.json .automerge.require_human: true) keep the human-APPROVED gate, default repos merge on a shadow APPROVE verdict bound to the current head sha. Adds automerge_require_human and automerge_reviewer_blocks (human veto on either path). Well-scoped, well-tested, CI green.

Findings:

automerge_require_human (lib/automerge.sh:47) correctly defaults to shadow-gating: .automerge.require_human // false means missing key, false, or missing agent.json all fall through to the shadow path. All four cases are covered in the new tests.
automerge_reviewer_blocks (lib/automerge.sh:82) is a clean mirror of automerge_approved_by — latest decision-review wins, stale/dismissed excluded, foreign reviewers ignored. Tests cover live-RC, RC-then-later-APPROVED, stale-RC, and other-user cases.
The default-path sha-binding (reviewed_sha != head → skip) is the right fail-closed control against merging code that advanced past its shadow review. The stale-sha test (oldsha0 vs headsha7) exercises it directly.
Both veto directions are wired: shadow REQUEST_CHANGES blocks the human path; human live REQUEST_CHANGES (automerge_reviewer_blocks) blocks the default path. The two e2e "human RC vetoes shadow APPROVE" / "flagged, no human approve" tests confirm this.
Checklist matches the diff exactly: both new unit-test blocks and the updated e2e cases (default shadow, COMMENT→no-merge, RC→no-merge, stale-sha, flagged human path) are all present.

Fail-open risk analysis: The comments reference behavior in code not in this diff (review_record storing the sha, review_update_sha keeping sha == head on base-merge, tick ordering). If any of those assumptions were wrong, the default path degrades to not merging (empty/mismatched reviewed_sha) — i.e. fail-closed. The only fail-open would be a stale APPROVE matching head, which the sha check prevents. Safe either way.

Operational note (not blocking): As the author flags, this activates shadow-gating fleet-wide on self-deploy — any non-carve-out PR with a current-head shadow APPROVE + green CI merges on the next tick. igor itself won't self-merge (no smoke.url + AUTOMERGE_SELF_REPO), so a human still lands this consciously. Merge timing is the human's call; the code is correct as written.

No changes requested.

_{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 `7ad3b5fa`: **success** **Summary:** Makes `do_automerge_tick`'s approval gate dual-path — flagged repos (`agent.json .automerge.require_human: true`) keep the human-APPROVED gate, default repos merge on a shadow `APPROVE` verdict bound to the current head sha. Adds `automerge_require_human` and `automerge_reviewer_blocks` (human veto on either path). Well-scoped, well-tested, CI green. **Findings:** - `automerge_require_human` (lib/automerge.sh:47) correctly defaults to shadow-gating: `.automerge.require_human // false` means missing key, `false`, or missing `agent.json` all fall through to the shadow path. All four cases are covered in the new tests. - `automerge_reviewer_blocks` (lib/automerge.sh:82) is a clean mirror of `automerge_approved_by` — latest decision-review wins, stale/dismissed excluded, foreign reviewers ignored. Tests cover live-RC, RC-then-later-APPROVED, stale-RC, and other-user cases. - The default-path **sha-binding** (`reviewed_sha != head` → skip) is the right fail-closed control against merging code that advanced past its shadow review. The stale-sha test (`oldsha0` vs `headsha7`) exercises it directly. - Both veto directions are wired: shadow `REQUEST_CHANGES` blocks the human path; human live `REQUEST_CHANGES` (`automerge_reviewer_blocks`) blocks the default path. The two e2e "human RC vetoes shadow APPROVE" / "flagged, no human approve" tests confirm this. - Checklist matches the diff exactly: both new unit-test blocks and the updated e2e cases (default shadow, COMMENT→no-merge, RC→no-merge, stale-sha, flagged human path) are all present. **Fail-open risk analysis:** The comments reference behavior in code not in this diff (`review_record` storing the sha, `review_update_sha` keeping `sha == head` on base-merge, tick ordering). If any of those assumptions were wrong, the default path degrades to *not* merging (empty/mismatched `reviewed_sha`) — i.e. fail-closed. The only fail-open would be a stale APPROVE matching head, which the sha check prevents. Safe either way. **Operational note (not blocking):** As the author flags, this activates shadow-gating fleet-wide on self-deploy — any non-carve-out PR with a current-head shadow `APPROVE` + green CI merges on the next tick. igor itself won't self-merge (no `smoke.url` + `AUTOMERGE_SELF_REPO`), so a human still lands this consciously. Merge timing is the human's call; the code is correct as written. No changes requested. --- <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-20 22:19:14 +00:00

53 seconds

igor referenced this pull request

2026-07-20 22:26:15 +00:00

feat(shipreport): daily scripted fleet ship-report (safety valve for #405) #406

igor referenced this pull request

2026-07-20 22:28:22 +00:00

feat(shipreport): daily scripted fleet ship-report (safety valve for #405) #406