Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 33 additions & 5 deletions .github/scripts/enqueue-guard.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@
//
// So this is the step immediately before that call. It refuses — exit 1, the reason printed — when
// the pull request carries `hold`, when the newest conclusion at its head is not a PASS, or when that
// conclusion is itself marked "record only — do not merge". It reuses `judge({ mode: 'queue' })`, so
// conclusion is itself marked "record only — do not merge". It also refuses a pull request that names
// no issue in its body or carries no `agent-pr` label (#718; a mechanical release PR and a fork are
// exempt), so
// "one issue, one pull request" is checked rather than remembered. It reuses `judge({ mode: 'queue' })`, so
// "may this enter the queue" keeps one definition: a rule added to the merge gate is enforced here
// without being restated, unless someone deliberately changes both.
//
Expand All @@ -21,7 +24,7 @@
// routine's path rather than left to a check that reports after the queue is reached.
import { execFileSync } from 'node:child_process'
import { fileURLToPath } from 'node:url'
import { judge } from './merge-gate.mjs'
import { isMechanicalRelease, judge } from './merge-gate.mjs'
import { loadPolicy, parseReport } from './review-gate.mjs'

const SHORT = 12
Expand All @@ -34,7 +37,16 @@ const SHORT = 12
// pull request out` — must not freeze itself (#687). A bullet or a quote line is allowed, but nothing
// may follow the marker on it except its other language half.
const RECORD_ONLY = /^[\s>*+-]*(?:record only\s*[—–-]+\s*do not (?:merge|queue)(?:\s*[//]\s*仅记录[、,,]?\s*禁止入队)?|仅记录[、,,]?\s*禁止入队)\s*[.。]?\s*$/im
// AGENTS.md → Claim: a pull request says `Fixes #N` in its body, and one issue carries one pull request.
// #718: a pull request with no issue behind it, or with no `agent-pr` label, is not the routine's work at
// all — #711/#712/#713 arrived with neither and nothing refused them, because the guard only looked at
// `hold`, the conclusions and the diff. The reference is a line of its own, so prose about "fixes" does
// not pass; the spellings accepted are exactly `Fixes #<n>` and `Closes #<n>`, behind any `#`, `>` or `-`.
// The rules only hold where the branch is ours: a fork is exempt, because we cannot push to its head
// and the owner keeps their own workflow there (#711/#713).
const ISSUE_REF = /^[\s>#-]*(?:fixes|closes)\s*#\d+/im
const short = (s) => String(s ?? '').slice(0, SHORT)
const labelNames = (labels) => (labels ?? []).map((l) => String(typeof l === 'string' ? l : l?.name ?? '').toLowerCase())

// The newest conclusion that names this head, if it carries the record-only marker. Only a body that
// parses as a conclusion counts, so ordinary prose quoting the phrase is no marker; only the newest
Expand All @@ -52,16 +64,27 @@ export function recordOnlyAt(reports, head) {
}

// The decision, without the network: `result` is merge-gate's `judge` output for this head.
export function guard({ pr, head, labels, result, reports }) {
export function guard({ pr, head, labels, body, mechanicalRelease, fork, result, reports }) {
const quiet = recordOnlyAt(reports, result.head ?? head)
const reasons = [...result.reasons]
if (quiet) reasons.push('the newest conclusion at this head says "record only — do not merge"')
const ok = result.ok && !quiet
// Two exemptions, both about what the pull request is rather than how risky it is. A mechanical release
// PR carries neither an issue nor the label by design — it is the routine's own daily output under the
// standing approval (AGENTS.md 6). A fork is not ours: its head cannot be pushed by us, and the owner runs
// their own workflow there (#711/#713). `result.exempt` cannot stand in for either: it is true for T0/T1
// as well, and routine work still names its issue.
if (!mechanicalRelease && !fork) {
if (!ISSUE_REF.test(String(body ?? ''))) reasons.push('no issue reference in the pull request body: add "Fixes #<n>"')
if (!labelNames(labels).includes('agent-pr')) reasons.push('missing label: agent-pr')
}
const ok = result.ok && !quiet && reasons.length === 0
return {
...result,
ok,
reasons,
recordOnly: Boolean(quiet),
mechanicalRelease: Boolean(mechanicalRelease),
fork: Boolean(fork),
labels: labels ?? [],
// One loud sentence: a refusal says what to do ("do not enqueue") and why, so it cannot be read
// as a warning that some other step may override.
Expand Down Expand Up @@ -106,7 +129,12 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prO
// The live head is judged, never a cached one: a push that moved the tip voids the PASS the caller
// believes it has, and `--sha` lets a caller insist on the head it read a moment ago.
const result = judge({ labels, files, head, author: pull.user?.login, reports, policy: loadPolicy(policyPath), expectedHead: arg('--sha'), mode: 'queue' })
const decision = guard({ pr, head, labels, result, reports })
const mechanicalRelease = isMechanicalRelease(files.map((f) => f.filename))
// The branch this repository owns, not a contributor's: `head.repo` is null when the fork was deleted,
// which is not ours either. Both sides are lower-cased because the repository name in the API paths is
// case-insensitive: `--repo thinkflowlab/flybot` used to read our own branch as a fork and print ALLOW.
const fork = String(pull.head?.repo?.full_name ?? '').toLowerCase() !== String(repo).toLowerCase()
const decision = guard({ pr, head, labels, body: pull.body, mechanicalRelease, fork, result, reports })

console.log(`enqueue-guard: PR #${pr} head ${short(decision.head)}, labels [${labels.join(', ')}]`)
console.log(decision.ok
Expand Down
69 changes: 63 additions & 6 deletions .github/scripts/enqueue-guard.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,15 @@ const report = ({ tip = HEAD, verdict = 'PASS', extra = [] } = {}) =>
const comment = (body = report(), at = '2026-10-21T10:31:00Z') =>
({ user: { login: 'reviewer-agent' }, body, created_at: at, author_association: 'MEMBER' })

const fakeGh = ({ comments = [], reviews = [], labels = [], files = [], head = HEAD, author = 'opener' } = {}) => {
const fakeGh = ({ body = 'Fixes #602\n', comments = [], reviews = [], labels = ['agent-pr'], files = [], head = HEAD, author = 'opener', headRepo = 'ThinkFlowLab/FlyBot' } = {}) => {
const calls = []
const gh = (...args) => {
const path = args[args.length - 1]
calls.push(path)
if (path.includes('/files')) return files.map((f) => JSON.stringify(typeof f === 'string' ? { filename: f } : f)).join('\n')
if (path.includes('/comments')) return comments.map((c) => JSON.stringify(c)).join('\n')
if (path.includes('/reviews')) return reviews.map((r) => JSON.stringify(r)).join('\n')
if (/\/pulls\/\d+$/.test(path)) return JSON.stringify({ head: { sha: head }, user: { login: author }, labels: labels.map((name) => ({ name })) })
if (/\/pulls\/\d+$/.test(path)) return JSON.stringify({ head: { sha: head, repo: { full_name: headRepo } }, user: { login: author }, body, labels: labels.map((name) => ({ name })) })
// Anything else — `gh pr merge` above all — is the guard doing more than checking.
throw new Error('fake gh: unexpected ' + path)
}
Expand All @@ -47,13 +47,13 @@ const fakeGh = ({ comments = [], reviews = [], labels = [], files = [], head = H

const run = (fixture = {}) => main({
gh: fakeGh({ files: [HOST_FILE], ...fixture }),
env: { REVIEW_POLICY: POLICY, GH_REPO: 'ThinkFlowLab/FlyBot', PR: '566' },
repo: 'ThinkFlowLab/FlyBot',
env: { REVIEW_POLICY: POLICY, GH_REPO: fixture.repo ?? 'ThinkFlowLab/FlyBot', PR: '566' },
repo: fixture.repo ?? 'ThinkFlowLab/FlyBot',
pr: 566,
})

test('#602: a held pull request is not enqueued, whatever a PASS at its tip says', () => {
const r = run({ labels: ['hold'], comments: [comment()] })
const r = run({ labels: ['hold', 'agent-pr'], comments: [comment()] })
assert.equal(r.ok, false)
assert.equal(r.held, true)
assert.match(r.reasons.join('; '), /`hold` label/)
Expand Down Expand Up @@ -150,7 +150,7 @@ test('#602: a T0/T1 pull request still needs no PASS', () => {
})

test('#602: hold refuses a T0/T1 pull request too', () => {
const r = run({ files: [TEST_FILE], labels: ['hold'], comments: [] })
const r = run({ files: [TEST_FILE], labels: ['hold', 'agent-pr'], comments: [] })
assert.equal(r.ok, false)
assert.equal(r.held, true)
})
Expand All @@ -169,6 +169,63 @@ test('#602: the self-gate case (author Birfy, PASS by Birfy) is allowed', () =>
assert.equal(r.ok, true, r.reasons.join('; '))
})

// #718: a pull request with no issue behind it, or with no `agent-pr` label, is refused before the
// queue: #711/#712/#713 arrived with neither and the guard said nothing, because it only read `hold`,
// the conclusions and the diff. The release PR is the one exception — it has neither by design.
test('#718: a body that names no issue is not enqueued', () => {
const r = run({ body: '## Summary\n\nTidy up the dock.', comments: [comment()] })
assert.equal(r.ok, false)
assert.match(r.reasons.join('; '), /no issue reference in the pull request body: add "Fixes #<n>"/)
assert.match(r.message, /^do not enqueue #566: /)
})

test('#718: a pull request without the agent-pr label is not enqueued', () => {
const r = run({ body: 'Fixes #566\n\nthe detail', labels: [], comments: [comment()] })
assert.equal(r.ok, false)
assert.ok(r.reasons.includes('missing label: agent-pr'), r.reasons.join('; '))
})

test('#718: a Closes reference and the agent-pr label let it through', () => {
const gh = fakeGh({ body: 'Closes #714\n\nthe detail', labels: ['agent-pr'], files: [HOST_FILE], comments: [comment()] })
const r = main({ gh, env: { REVIEW_POLICY: POLICY, GH_REPO: 'ThinkFlowLab/FlyBot', PR: '566' }, repo: 'ThinkFlowLab/FlyBot', pr: 566 })
assert.equal(r.ok, true, r.reasons.join('; '))
assert.equal(r.command, `gh pr merge 566 --auto --squash --match-head-commit ${HEAD}`)
})

test('#718: a mechanical release pull request needs neither an issue nor the label', () => {
const r = run({ body: 'Version 0.2.1', labels: [], files: ['changelog.d/999-x.changed.md'], comments: [] })
assert.equal(r.ok, true, r.reasons.join('; '))
assert.equal(r.exempt, true)
assert.equal(r.mechanicalRelease, true)
})

test('#718: a fork is exempt — its head is not ours to ask for an issue', () => {
const r = run({ body: '## Summary\n\nTidy up the dock.', labels: [], files: [TEST_FILE], headRepo: 'HugoXK/FlyBot', comments: [] })
assert.equal(r.ok, true, r.reasons.join('; '))
assert.equal(r.fork, true)
// The same pull request on a branch this repository owns is refused, so the exemption is the fork and
// not a missing fixture field.
assert.equal(run({ body: '## Summary\n\nTidy up the dock.', labels: [], files: [TEST_FILE], comments: [] }).ok, false)
})

// The repository name in the API paths is case-insensitive, so the ownership test is too: spelling `--repo`
// in lower case used to turn our own branch into a "fork" and let both rules through (#718 INC).
test('#718: a lower-case --repo spelling does not turn our own branch into a fork', () => {
const r = run({ body: '## Summary', labels: [], files: [TEST_FILE], repo: 'thinkflowlab/flybot', comments: [] })
assert.equal(r.fork, false)
assert.equal(r.ok, false)
assert.match(r.reasons.join('; '), /no issue reference/)
assert.match(r.reasons.join('; '), /missing label: agent-pr/)
})

test('#718: a fork stays exempt under any --repo spelling', () => {
for (const repo of ['ThinkFlowLab/FlyBot', 'thinkflowlab/flybot']) {
const r = run({ body: '## Summary', labels: [], files: [TEST_FILE], headRepo: 'HugoXK/FlyBot', repo, comments: [] })
assert.equal(r.fork, true, repo)
assert.equal(r.ok, true, repo + ' → ' + r.reasons.join('; '))
}
})

// The guard is only in the path if the instructions that tell an agent to enqueue name it; otherwise
// the next agent runs `gh pr merge` from memory and #602 is back.
test('#602: the instructions that enqueue point at the guard', () => {
Expand Down
1 change: 1 addition & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ What this buys is presence and a name from a closed list, not truth: with one sh
- **No implementer writes the Verdict.** On the API side self-review cannot be told apart mechanically — every pull request is opened by the shared `Birfy` account, and `selfReview: ["Birfy"]` in [`review-policy.json`](.github/agent/review-policy.json) makes a PASS always look like Birfy judging Birfy — so what the team holds is the role split: the implementer Bot does not write the Verdict, the Bot that writes the Verdict neither changes the tip nor re-judges its own verdict, and `selfReview` stays as it is because clearing it would leave every pull request without a conclusion.
- **`hold` is human-only.** Only a person sets the label and only a person lifts it; automation refuses a held pull request and never removes the label itself. That is the one human input left on the merge path.
- **The label awaiting-approval is retired.** A PASS at the tip goes straight to the queue through the enqueue guard (#687); no pull request waits for an approval click and the label is not set again.
- **A pull request enters the queue with its issue and its `agent-pr` label.** On a branch this repository owns, the enqueue guard also refuses one whose body names no issue (`Fixes #<n>` / `Closes #<n>`) and one without the `agent-pr` label (#718), so "one issue, one pull request" is checked rather than remembered. Two exemptions: a mechanical release PR, which carries neither by design, and a **fork** (`pull.head.repo.full_name` is not this repository), whose head we cannot push to and where the contributor keeps their own workflow.

Four rules decide what a queue entry must pass. The mechanical judge runs where the merge actually happens: [`merge-gate.yml`](.github/workflows/merge-gate.yml) fails the `merge-gate` job on a `merge_group` entry, and [`merge-gate.mjs`](.github/scripts/merge-gate.mjs) does the judging.

Expand Down
Loading