diff --git a/.github/scripts/enqueue-guard.mjs b/.github/scripts/enqueue-guard.mjs index 541f674c0..1012d1050 100644 --- a/.github/scripts/enqueue-guard.mjs +++ b/.github/scripts/enqueue-guard.mjs @@ -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. // @@ -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 @@ -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 #` and `Closes #`, 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 @@ -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 #"') + 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. @@ -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 diff --git a/.github/scripts/enqueue-guard.test.mjs b/.github/scripts/enqueue-guard.test.mjs index 033ccdd5a..f2838ef78 100644 --- a/.github/scripts/enqueue-guard.test.mjs +++ b/.github/scripts/enqueue-guard.test.mjs @@ -29,7 +29,7 @@ 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] @@ -37,7 +37,7 @@ const fakeGh = ({ comments = [], reviews = [], labels = [], files = [], head = H 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) } @@ -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/) @@ -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) }) @@ -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 #"/) + 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', () => { diff --git a/AGENTS.md b/AGENTS.md index 93288d1e1..de3194a25 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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 #` / `Closes #`) 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.