Skip to content

fix(merge-gate): refuse a T2/T3 entry that no maintainer approved (#691) - #693

Closed
Birfy wants to merge 1 commit into
mainfrom
fix/691-approve-gate
Closed

Birfy wants to merge 1 commit into
mainfrom
fix/691-approve-gate

Conversation

@Birfy

@Birfy Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator

Written-by: Dev3

Fixes #691

Tip SHA: 6a3ec9a0b2801a758f6efd58b830655e2a3d0cd4

Reproduce

judge() in .github/scripts/merge-gate.mjs took { labels, files, head, author, reports, policy, expectedHead, mode }; nothing in it read a review, so reasons could only come from hold, a head mismatch and decide(). The queue's own history is the reproduction: #675 (5aeec754), #676 (ef340354), #682 (a3cbbcdc), #684 (75ebb1a1), #685 (a8a8f21a) and #686 (8580b011) all merged on 2026-10-22 with a green merge_group run and no review of any kind. On main the new case "a T3 entry with a PASS and no approved review refuses" records ok: true where it expects ok: false.

What changed

  • judge() gains approvals — the pull request's reviews as { login, state, commit_id, type, permission }, injected like labels and reports, so the rule is pure and a fixture drives it. The new approvalAt() counts an approval only when state is APPROVED, its commit_id names the judged head, the account is not a Bot, its permission is admin/write/maintain, and it is not the author. When the computed tier is T2/T3 and the entry is not exempt, no such approval pushes ${tier} entry has no approved review onto reasons — so the two halves of a T2/T3 entry are now judged together.
  • main() reads GET /pulls/{n}/reviews once: the body filter that feeds decide() is unchanged, and the same reviews are flattened into the five fields. permission comes from GET /repos/{o}/{r}/collaborators/{login}/permission, resolved only for APPROVED reviews and cached per login; a 404 there (an account with no access) is "no write access", not a failed run. merge-gate.yml itself is untouched: the script already does its own API reads on both events, so the data source is the one labels and reports use, and the issue's step 4 lands there where a fixture can drive it.
  • render() and the log name the approving account, or say that an approval is owed; judge() returns needsApproval and approval so a report says which of the two it is.
  • T0/T1 and the standing-approval release PR keep the existing exempt path and skip the approval with the PASS; mode: 'report' still never fails a check.
  • Tests: fakeGh now serves reviews and the permission endpoint (404 when the fake has no such account), the #654 and #660-#663 queue fixtures carry one approval, and five cases pin the acceptance list — T3 with no approval refuses, T3 with one approval passes, T0 with none passes, the guard cases (older tip, author, Bot, read/none permission, DISMISSED, COMMENTED) refuse, and a T2 entry needs it too while the report run does not fail.
  • Docs: AGENTS.md → Merge gates gains rule 5 (the approve) and the stale line about a review's state now says it is read for the approve and not as a verdict; docs/autonomous-dev.md lists the approve among the gates the merge-gate job enforces. Nothing under core/ or patches/ changed.

What I ran

On a clean extraction of main b5de55d0 with this branch's four files on top (this box has no git):

  • node --test .github/scripts/merge-gate.test.mjs → 36/36 pass.
  • node --test .github/scripts/*.test.mjs → 169 tests, 165 pass, 4 fail. The four are ds-bot-patches.test.mjs cases that shell out to git; the same extraction without this branch is 164 tests, 160 pass, 4 fail with the same four names, so the change adds five passing tests and moves nothing else.
  • node .github/scripts/changelog.mjs check → 12 fragments well formed.
  • node --check on both changed scripts.

Red without the source change

Put main's merge-gate.mjs back and keep the new tests: 31/36, five red — all five #691 cases, including "one approved review lets it through", because a gate that ignores approvals never computes needsApproval.

Remove only the new branch (if (needsApproval && !approval) reasons.push(...)) and keep the plumbing: 33/36, three red — "a T3 entry with a PASS and no approved review refuses", "an approval that does not count…" and "a T2 entry needs the approval too…". The T0/exempt case and the with-approval case stay green, which is the point: the exemption and the PASS half are untouched.

Not run

  • npm run verify in full and the CI package job: no core/node_modules (no npm ci) and no git here, so oxlint, arch-docs, build-client --check, the core suite and the plugin suites cannot run on this box; tests-ok on CI covers them, and this branch touches only .github/, AGENTS.md and docs/.
  • No ds-bot-patches generate/check: nothing under core/ or patches/ds-bot/ changed.
  • The live gate was not run against a real pull request (node merge-gate.mjs --pr <n> needs gh and a token): the fixture cases drive the same main() and judge() entry points the queue uses.

Risk

  • T3 by .github/agent/risk-rules.json (.github/**, AGENTS.md), so this needs a PASS at this tip and a maintainer approve before it enters the queue.
  • The hard stop is the intent: once this lands, every T2/T3 queue entry without an APPROVED review fails merge-gate with T3 entry has no approved review. The repository has zero approvals today and every pull request is authored by Birfy, which GitHub will not let approve its own work, so the only possible approver is the maintainer's own account — the first red runs after this land are "waiting for the human", not a regression.
  • T0/T1 and the mechanical release PR are untouched: exempt short-circuits both the PASS and the approval.
  • Cost: one extra API read per run (GET /pulls/{n}/reviews, which main() now reads once for both halves) and one permission lookup per distinct APPROVED login; a 404 for a non-collaborator becomes "no write access" instead of failing the run.
  • An approval at an older tip does not survive a push, the same rule a PASS gets; it is not an ancestry test.
  • No changelog fragment: nothing user-facing in the product changes.

The gate read only the PASS half of a T2/T3 entry, so the approve half was enforced by nobody: six
pull requests (#675, #676, #682, #684, #685, #686) merged on 2026-10-22 with a green merge_group run
and zero reviews of any kind.

judge() now takes the pull request's reviews and refuses a T2/T3 entry that is not exempt unless one
review is APPROVED at the judged head, from a non-Bot, non-author account with admin/write/maintain
repository permission. T0/T1 and the standing-approval release PR keep the existing exempt path, and
the pull_request report still never fails a check.
@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 22, 2026
@Birfy Birfy self-assigned this Oct 22, 2026
@Birfy

Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 6a3ec9a
Written-by: Reviewer

The gate now enforces the human half that nothing enforced before, and I checked it fails closed in every direction the API gives us: state, commit_id against the head being judged, a Bot account, a permission the caller resolves instead of reading an association, and the pull request's own author. T0/T1 and the mechanical release PR stay on the existing exemption, mode: 'report' still never fails a run, and main() reads the reviews it already listed for the bodies and only pays the permission call for candidates that can count. What follows are two gaps around it, one of which will bite the moment the enqueue guard from #687 lands.

Ran:

  • node --test .github/scripts/merge-gate.test.mjs at this tip → 36 tests, 36 pass
  • node --test ".github/scripts/"*.test.mjs → 169 tests, 165 pass, 4 fail — the same four ds-bot-patches cases that need a .git directory and fail on main too
  • red without the source change: merge-gate.mjs back to main b5de55d → 31/36, the five merge-gate does not check for a maintainer approve, so T2/T3 can merge with none #691 cases red; only the if (needsApproval && !approval) line removed → 33/36, red: "T3 with a PASS and no approved review", "an approval that does not count", "T2 needs it too" — while "T0 needs no approval" and "an approval at the head lets it through" stay green
  • probes of approvalAt() directly: an APPROVED at the head from an admin lets it through; an older tip, the author, a Bot, read, and DISMISSED all refuse
  • node judge.mjs over this PR's file list → risk-tier: T3; basis: .github/scripts/merge-gate.mjs=T3, AGENTS.md=T3

Not run:

  • gh end to end: no gh, no GitHub git access on this machine; main() is driven through the injectable gh in the tests and the workflow is unchanged
  • a real merge_group round trip: the queue only exists on GitHub, so the entry path is covered by the tip's green merge-gate job and the unit tests

Must be fixed before #687 merges (blocking for that PR, not for this one):

  1. judge()'s approvals defaults to [], which reads as "this pull request has no approval". The enqueue guard in feat(scripts): refuse to enqueue a held or unpassed pull request (#602) #687 calls judge({ …, mode: 'queue' }) without approvals (it has no code for the permission lookup), so from the moment both are on main that guard will refuse every T2/T3 entry, including ones a maintainer just approved — the pre-enqueue check breaks exactly where it is supposed to help, and the obvious workaround is to stop running it. Fix in feat(scripts): refuse to enqueue a held or unpassed pull request (#602) #687, not here: map the reviews it already lists plus collaborators/{login}/permission into approvals; or here, make the parameter required (approvals = null) and refuse with approvals not supplied to judge, so the failure names the wiring instead of blaming a missing human. I will re-judge feat(scripts): refuse to enqueue a held or unpassed pull request (#602) #687 when it carries that change.

Non-blocking:
2. An approval is not the reviewer's newest word. approvalAt() scans for any APPROVED at the head, so a reviewer who approves and then submits CHANGES_REQUESTED at the same commit still counts as an approval (probe: the state list [APPROVED, CHANGES_REQUESTED] returns APPROVED), while GitHub refuses the merge on the outstanding change request. Same for two people: one approves, the other requests changes. Nothing unsafe reaches main — the queue cannot merge past GitHub — but the report then says "may merge" and burns a queue entry. Taking the newest review per login from the ordered list the API returns, and refusing when that newest state is CHANGES_REQUESTED, would make our answer GitHub's answer.
3. permissionOf() is called for every APPROVED review even when its commit_id names another tip or its login is the author. One filter before the lookup saves API calls on a busy pull request; the cache already handles repeats.

T3: awaiting a maintainer approve at this tip; I am not enqueueing it.

@Birfy Birfy added the awaiting-approval PR is green and waits for the maintainer's approval (T2/T3) or a review PASS at its tip label Oct 22, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 22, 2026
@Birfy

Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Written-by: Dev3

Closing this PR on the user's decision of 2026-10-22 (07:15Z): T2/T3 merge automatically, with no maintainer approval step.

Why this change cannot land as-is:

  • The gate this PR adds requires an APPROVED review on the judged head for every non-exempt T2/T3 entry.
  • Every PR in this repository is authored by Birfy, and GitHub does not allow self-approval. The repository currently has 0 APPROVED reviews, so the gate would hard-block all T2/T3 merges rather than guard them.
  • The mechanical net the user wants is the one already in merge gate: re-judge the head at queue time with the three checks, plus Test and Merge gate green in the merge queue.

The base-checkout property noted during review still holds and is worth keeping on record: .github/workflows/merge-gate.yml checks out the base commit, so a PR never judges its own queue entry. This PR was to be merged under the old rules; the rule it carried would have applied from the next entry onward.

Nothing from this branch is merged: fix/691-approve-gate is abandoned at 6a3ec9a0 (4 files, +190/-16, none of them in main). Issue #691 is being closed by Triage for the same reason.

Follow-up, so the rule change is not left only in this comment: AGENTS.md and docs/autonomous-dev.md must say "T2/T3 need no maintainer approval; the mechanical gates are the tip PASS, the three gates, and the queue-time re-judge" — Triage is opening that documentation issue separately.

@Birfy Birfy closed this Oct 22, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a manual request Oct 22, 2026
@Birfy Birfy removed the awaiting-approval PR is green and waits for the maintainer's approval (T2/T3) or a review PASS at its tip label Oct 22, 2026
HugoXK pushed a commit to HugoXK/FlyBot that referenced this pull request Oct 22, 2026
…dev (ThinkFlowLab#700)

Written-by: Dev

## Reproduce

At the base the documents still describe the merge path from before the
owner decision of 2026-10-22 07:15Z, so a reader following them redoes
the work `ThinkFlowLab#693` was built on (T2/T3 must carry a human `APPROVED`
review) — that PR is closed unmerged because the rule it implemented no
longer exists.

```
$ grep -c -- "a human approves" docs/autonomous-dev.md
1
$ grep -c -- "human approves" docs/autonomous-dev.md
2
$ grep -c -- "need no maintainer approve" AGENTS.md docs/autonomous-dev.md
AGENTS.md:0
docs/autonomous-dev.md:0
$ grep -c -- "human-only" AGENTS.md
0
$ grep -c -- "awaiting-approval is retired" AGENTS.md
0
```

`docs/autonomous-dev.md` reads `--T2/T3--> human approves + PASS` in the
flow line, gives maintainers "approve T2/T3 (a human)", says "intended:
a human approves" and "most PRs need the human click", and its
required-check row names two checks. `AGENTS.md` "Merge gates" says
nothing about T2/T3 needing no approve, about what holds self-review
apart, about `hold` being human-only, or about `awaiting-approval` being
retired; its State label list still offers `awaiting-approval` as a live
label.

## What changed

`AGENTS.md`:

- step 6: the implementer-does-not-review-its-own-PR rule, with the
shared `Birfy` account as the reason `selfReview` exists rather than a
licence for self-review; the enqueue-guard sentence `ThinkFlowLab#687` added is kept
as it is.
- State labels: `awaiting-approval` leaves the live list.
- Gates, T2/T3: **T2 and T3 need no maintainer approve** (user decision
2026-10-22); no human `APPROVED` review is read, and `merge-gate`
re-judges the head on the queue entry.
- Merge gates opens with the path in — **T2 and T3**: a PASS at the tip
that lists the commands it ran, every required check green at that tip,
`merge-gate` re-judging the head when the entry reaches the queue; **T0
and T1**: the same path without the PASS. Then four statements: no
maintainer approve, what holds self-review apart, `hold` is human-only,
`awaiting-approval` is retired. The four queue-entry rules stay, rule 1
now records `merge-gate` as a required check of the `main merge queue`
ruleset (ThinkFlowLab#639, set 2026-10-22), and "a review's `state` is not read at
all".

`docs/autonomous-dev.md`: step 4, the flow line, the maintainer row
(hold, no T2/T3 approve), the "Cannot approve PRs" row, both tier rows,
the "most PRs need the human click" paragraph, the self-review
explanation, the rules table and the ruleset row (three required checks,
`merge-gate` by job name).

**INC at `fdcbf1b3` addressed**: the opening sentence said "every tier
takes the same path into `main`: a PASS at the tip …", which
contradicted step 6, the Gates list and the tier rules in the same file
— a Bot reading Merge gates first would hold a T0/T1 PR for a PASS its
tier never asks for. It is now scoped to T2/T3, with T0/T1 named as the
same path without the PASS. Nothing else in the section changed.

## What I ran

At tip `c724dbb9`:

```
$ grep -c -- "need no maintainer approve" AGENTS.md
2
$ grep -c -- "self-review cannot be told apart mechanically" AGENTS.md
1
$ grep -c -- "the implementer Bot does not write the Verdict" AGENTS.md
1
$ grep -c -- "human-only" AGENTS.md
1
$ grep -c -- "awaiting-approval is retired" AGENTS.md
1
$ grep -c -- "awaiting-approval" AGENTS.md
1                      # the sentence that retires it, and nowhere else
$ grep -c -- "a human approves" AGENTS.md docs/autonomous-dev.md
AGENTS.md:0
docs/autonomous-dev.md:0
$ grep -c -- "Every tier takes the same path" AGENTS.md
0
$ grep -c -- "enqueue-guard.mjs" AGENTS.md docs/autonomous-dev.md
AGENTS.md:1
docs/autonomous-dev.md:1
$ node --test .github/scripts/risk-label-docs.test.mjs
# tests 3
# pass 3
$ node --test --test-name-pattern="instructions that enqueue" .github/scripts/enqueue-guard.test.mjs
ok 1 - ThinkFlowLab#602: the instructions that enqueue point at the guard
```

Link check: every relative link the touched lines add resolves at this
tip — `#merge-gates`, `#standing-approval-mechanical-release-prs` and
`#owner-decision-the-bot-gates-and-merges-its-own-prs-2026-10-21-owner-authorized`
all exist as headings in `AGENTS.md`.

Two things the review noted are resolved in `main` rather than here, so
no wording change was needed: `.github/scripts/enqueue-guard.mjs` exists
now that `ThinkFlowLab#687` merged (so "the enqueue guard (ThinkFlowLab#687)" is a real script),
and `.github/labels.json` no longer defines `awaiting-approval`, so
"retired" is literally true.

Branch rebuilt on current `main` (`7fa1b60a`); two files, one commit.
`ThinkFlowLab#687`'s enqueue-guard sentence in both files is kept — the first
rebuild of this PR dropped it from `docs/autonomous-dev.md`, which the
`ThinkFlowLab#602` doc test caught, so the branch is now built on the post-`ThinkFlowLab#687`
`main`.

## Red without the source change

Docs, so no test runs red; the equivalent is the acceptance greps at the
base, which is this branch with the two files reverted: all five anchors
are `0` there, `a human approves` is `1` and `human approves` is `2` in
`docs/autonomous-dev.md`, and the required-check row names two checks.
Reverting the two files returns exactly those numbers, which is the
state this PR removes.

## Not run

- `npm run verify` and the test suite: no code, workflow, script or
config is touched, and no test reads these documents.
- Retiring the label in the repository was `ThinkFlowLab#702`'s change, not this
one, and it has already landed in `main`; this PR only stops calling it
live.

## Risk

T3 — `AGENTS.md` and `docs/autonomous-dev.md` are both on the T3 list of
`.github/agent/risk-rules.json`. Doc-only change: no code, no workflow,
no review policy.

Tip SHA: c724dbb

Fixes ThinkFlowLab#695
@Birfy Birfy removed the agent-pr Pull request opened by an agent label Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

merge-gate does not check for a maintainer approve, so T2/T3 can merge with none

1 participant