From 54544dcd39d54f5d64208f14d6e4d002199935b1 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Wed, 21 Oct 2026 22:18:19 +0800 Subject: [PATCH 1/8] Read the verdict from the first line, and test the gate through its own entry point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #637: a conclusion posted as `Verdict (written by Reviewer, posted by Chief)` was invisible to a matcher anchored at the start of the body, and "no conclusion" allowed the merge. The first non-empty line is now the whole conclusion — `Verdict: PASS at ` (or INC / REQUEST_CHANGES) — so a verdict the Chief posts for a Reviewer with no write channel parses exactly like the Reviewer's own; the attribution and the evidence go below it. merge-gate's main() takes its `gh` runner as an argument, so the tests drive the same entry point the queue uses: newest INC refused, hold refused, a prefix-first body refused. AGENTS.md hard rule 6 gets the #628 sentence back, word for word. --- .claude/skills/review-pr/SKILL.md | 10 ++-- .github/scripts/merge-gate.mjs | 22 +++++---- .github/scripts/merge-gate.test.mjs | 71 ++++++++++++++++++++++++++++ .github/scripts/review-gate.mjs | 13 ++++- .github/scripts/review-gate.test.mjs | 25 ++++++++++ AGENTS.md | 4 +- docs/autonomous-dev.md | 5 +- 7 files changed, 130 insertions(+), 20 deletions(-) diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index 114985afd..40be3439a 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -34,15 +34,19 @@ Use for local or PR tip reviews on `ThinkFlowLab/FlyBot`. Produce a Verifier rep Always include: ```text -Tip SHA: +Verdict: PASS | FAIL | INC at +Written by: Reviewer. Posted by: . + Ran: - → pass|fail - … Not run: - — reason -Verdict: PASS | FAIL | INC ``` +The first line is the whole conclusion (#637): nothing may come before it, because the gates read the +first line and treat any other shape as no conclusion. + Honesty rules: - List what **you** ran at this tip. Stale CI on another SHA is not “Ran”. @@ -104,7 +108,7 @@ Do not fill contributor checkboxes or flip draft status unless asked. Post the report as a **pull request comment** (or the review body) only when a human asks for it, and only from the reviewer's own login: `.github/scripts/review-gate.mjs` reads the newest Verifier block that names the head, from a login in `.github/agent/review-policy.json`. The pull request's own -author counts only when listed in the policy's `selfReview` (the owner's Bot/agent, `Birfy`, may PASS its own PR). Keep the block intact — `Tip SHA:`, `Ran:`, `Not run:`, `Verdict:` — because a +author counts only when listed in the policy's `selfReview` (the owner's Bot/agent, `Birfy`, may PASS its own PR). Keep the block intact — the `Verdict: … at ` first line, the attribution line, `Ran:`, `Not run:` — because a missing `Ran:` command or `Not run:` line is reported as incomplete evidence, and a PASS at an older sha stops counting the moment the tip moves. Posting it is not merge authority: see AGENTS.md step 6. T0/T1 need no PASS; T2/T3 need a PASS at the tip, and the agent that gated the PR then enqueues it itself (owner decision 2026-10-21). diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index e124697d5..3ac0addfb 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -69,24 +69,26 @@ export function render(result, { pr, labels } = {}) { return out.join('\n') } -const gh = (...a) => execFileSync('gh', a, { encoding: 'utf8', maxBuffer: 256 << 20 }) -const list = (path) => gh('api', '--paginate', '--jq', '.[]', path).split('\n').filter(Boolean).map((l) => JSON.parse(l)) +const realGh = (...a) => execFileSync('gh', a, { encoding: 'utf8', maxBuffer: 256 << 20 }) function arg(name) { const i = process.argv.indexOf(name) return i > 0 ? process.argv[i + 1] : undefined } -function main() { - const repo = process.env.GH_REPO || arg('--repo') +// The entry point the workflow runs. `gh` and `env` are injectable so a test can drive the whole +// judgement without the network (and without a PATH shim): the rules above are exercised end to end. +export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prOption, headRef: headRefOption } = {}) { + const list = (path) => gh('api', '--paginate', '--jq', '.[]', path).split('\n').filter(Boolean).map((line) => JSON.parse(line)) + const repo = repoOption || env.GH_REPO || arg('--repo') if (!repo) throw new Error('set GH_REPO or pass --repo owner/name') - const headRef = arg('--head-ref') || process.env.HEAD_REF + const headRef = headRefOption ?? (arg('--head-ref') || env.HEAD_REF) const fromRef = headRef ? prFromHeadRef(headRef) : null if (headRef && !fromRef) throw new Error(`cannot read a pull request from head_ref "${headRef}" (expected gh-readonly-queue//pr--)`) - const pr = arg('--pr') || process.env.PR || fromRef?.pr + const pr = prOption || arg('--pr') || env.PR || fromRef?.pr if (!pr) throw new Error('set HEAD_REF, PR or pass --pr N') - const policyPath = arg('--policy') || process.env.REVIEW_POLICY || fileURLToPath(new URL('../agent/review-policy.json', import.meta.url)) + const policyPath = arg('--policy') || env.REVIEW_POLICY || fileURLToPath(new URL('../agent/review-policy.json', import.meta.url)) const pull = JSON.parse(gh('api', `repos/${repo}/pulls/${pr}`)) const head = pull.head?.sha const author = pull.user?.login @@ -103,11 +105,11 @@ function main() { console.log(`merge-gate: newest conclusion at ${short(result.head)} — ${result.verdict?.verdict ?? 'none'}${result.verdict?.by ? ` by ${result.verdict.by}` : ''}`) console.log(`merge-gate: ${result.ok ? 'PASS' : `REFUSE — ${result.reasons.join('; ')}`}`) const text = render(result, { pr, labels }) - if (process.env.GITHUB_STEP_SUMMARY) appendFileSync(process.env.GITHUB_STEP_SUMMARY, text + '\n') + if (env.GITHUB_STEP_SUMMARY) appendFileSync(env.GITHUB_STEP_SUMMARY, text + '\n') else console.log('\n' + text) - process.exit(result.ok ? 0 : 1) + return result } if (process.argv[1] === fileURLToPath(import.meta.url)) { - try { main() } catch (e) { console.error(`merge-gate: ${e.message}`); process.exit(1) } + try { const result = main(); process.exit(result.ok ? 0 : 1) } catch (e) { console.error(`merge-gate: ${e.message}`); process.exit(1) } } diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index 9df1337ba..e5018771a 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -101,3 +101,74 @@ test('workflow: merge_group only, read-only, judges the base copy, acts on nothi assert.doesNotMatch(wf, /continue-on-error: true/, 'a refusal must fail the job, not report it like review-gate.yml does') assert.doesNotMatch(wf, /gh pr merge|--approve|reviewDecision|GH_TOKEN: \$\{\{ secrets\./, 'no merge, no approval, no secret') }) + +// ---- through the entry point the queue uses (#637): main() itself, with the API replaced by a +// fake `gh`, so the whole judgement is exercised without the network. +import { main } from './merge-gate.mjs' +import { mkdtempSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +const fakeGh = ({ comments, reviews = [], labels = [] }) => (...args) => { + const path = args[args.length - 1] + const pull = { head: { sha: HEAD }, user: { login: 'opener' }, labels: labels.map((name) => ({ name })) } + if (path.includes('/comments')) return [comments].flat().map((entry) => JSON.stringify(entry)).join('\n') + if (path.includes('/reviews')) return [reviews].flat().map((entry) => JSON.stringify(entry)).join('\n') + if (/\/pulls\/\d+$/.test(path)) return JSON.stringify(pull) + throw new Error('fake gh: unexpected ' + path) +} + +const runGate = (fixture) => { + const policy = join(mkdtempSync(join(tmpdir(), 'merge-gate-policy-')), 'review-policy.json') + writeFileSync(policy, JSON.stringify({ reviewers: ['reviewer-agent'], selfReview: [] })) + return main({ + gh: fakeGh(fixture), + env: { GITHUB_STEP_SUMMARY: '', REVIEW_POLICY: policy }, + repo: 'ThinkFlowLab/FlyBot', + pr: 637, + headRef: ENTRY, + }) +} + +const apiComment = (body, at) => ({ user: { login: 'reviewer-agent' }, body, created_at: at }) +const apiReview = (body, at) => ({ user: { login: 'reviewer-agent' }, body, submitted_at: at }) + +const firstLine = (verdict, tip = HEAD) => [ + `Verdict: ${verdict} at ${tip}`, + 'Written by: Reviewer. Posted by: Chief (proxy).', + '', + 'Ran: npm run verify → pass', + 'Not run: smoke', +].join('\n') + +test('#637: the queue entry refuses the newest conclusion INC, through main()', () => { + const result = runGate({ comments: [apiComment(firstLine('PASS'), '2026-10-21T12:40:00Z'), apiComment(firstLine('INC'), '2026-10-21T12:49:00Z')] }) + assert.equal(result.ok, false) + assert.match(result.reasons.join('; '), /INC/) +}) + +test('#637: a verdict Chief posts for the Reviewer parses like the Reviewer wrote it', () => { + const result = runGate({ comments: [apiComment(firstLine('PASS'), '2026-10-21T12:40:00Z')] }) + assert.equal(result.ok, true) + assert.equal(result.verdict.by, 'reviewer-agent') +}) + +test('#637: a body that does not start with the verdict is no conclusion, so the entry is refused', () => { + const prefixed = ['Verdict (written by Reviewer, posted by Chief)', '', firstLine('PASS')].join('\n') + const result = runGate({ comments: [apiComment(prefixed, '2026-10-21T12:49:00Z')] }) + assert.equal(result.ok, false) + assert.match(result.reasons.join('; '), /no Verifier report names the tip|no Tip SHA/) + assert.equal(result.verdict.verdict, null, 'a verdict that is not the first line is not a conclusion') +}) + +test('#637: hold still refuses through main()', () => { + const result = runGate({ labels: ['hold'], comments: [apiComment(firstLine('PASS'), '2026-10-21T12:40:00Z')] }) + assert.equal(result.ok, false) + assert.equal(result.held, true) +}) + +test('#637: a review body in the fixed format is judged the same as a comment', () => { + const result = runGate({ comments: [], reviews: [apiReview(firstLine('PASS'), '2026-10-21T12:40:00Z')] }) + assert.equal(result.ok, true) + assert.equal(result.verdict.by, 'reviewer-agent') +}) diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index 3ff063697..ef086a00f 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -30,11 +30,20 @@ const SHORT = 12 // one replaced, and only the last block is the verdict the reviewer stands behind. export function parseReport(body) { const lines = String(body ?? '').replace(/[*`]/g, '').split('\n') + const out = { tip: null, verdict: null, ran: [], hasNotRun: false } + // The fixed format (#637): the first non-empty line is the whole conclusion, + // `Verdict: PASS at ` (or INC / REQUEST_CHANGES). The attribution line and the evidence go + // below it, so a verdict Chief posts for a Reviewer with no write channel parses the same way. + const firstLine = lines.find((line) => line.trim() !== '') ?? '' + const first = /^Verdict\s*:\s*(PASS|INC|REQUEST_CHANGES|FAIL)\s+(?:at\s+)?([0-9a-f]{7,40})\s*$/i.exec(firstLine.trim()) let start = -1 for (let i = 0; i < lines.length; i++) { if (/^Tip SHA\s*:/i.test(lines[i].trim())) start = i } - const out = { tip: null, verdict: null, ran: [], hasNotRun: false } + if (first) { + out.verdict = first[1].toUpperCase() + out.tip = first[2].toLowerCase() + } const from = start < 0 ? 0 : start for (let i = from; i < lines.length; i++) { const line = lines[i].trim() @@ -43,7 +52,7 @@ export function parseReport(body) { const hex = /[0-9a-f]{7,40}/i.exec(m[1]) out.tip = hex ? hex[0].toLowerCase() : null } - m = /^Verdict\s*:\s*(PASS|FAIL|INC)\b/i.exec(line) + m = /^Verdict\s*:\s*(PASS|FAIL|INC|REQUEST_CHANGES)\b/i.exec(line) if (m) out.verdict = m[1].toUpperCase() m = /^Ran\s*:\s*(.*)$/i.exec(line) if (m) { diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index ecf75ae97..afd087c99 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -177,3 +177,28 @@ test('workflow: pull_request, read-only, not required, acts on nothing', () => { assert.doesNotMatch(wf, /gh pr merge|--approve|reviewDecision|gh api .*-X (POST|PATCH|DELETE)/) assert.doesNotMatch(testYml, /review-gate/, 'it is not a required check or part of tests-ok') }) + +// #637: the fixed format. The first line is the whole conclusion, so a verdict Chief posts for a +// Reviewer who cannot post (the required proxy case) parses exactly like the Reviewer's own. +test('#637: a first-line verdict carries its own tip and its evidence below parses the same', () => { + const body = [ + `Verdict: PASS at ${HEAD}`, + 'Written by: Reviewer. Posted by: Chief (proxy — the Reviewer has no write channel).', + '', + 'Ran: npm run verify → pass', + 'Not run: smoke — host code unchanged', + ].join('\n') + const parsed = parseReport(body) + assert.equal(parsed.tip, HEAD) + assert.equal(parsed.verdict, 'PASS') + assert.deepEqual(parsed.ran, ['npm run verify → pass']) + assert.equal(parsed.hasNotRun, true) + assert.equal(decide({ head: HEAD, author: 'opener', reports: [comment('reviewer-agent', body)], policy }).ok, true) +}) + +test('#637: a verdict that is not the first line is not a conclusion', () => { + const body = ['Verdict (written by Reviewer, posted by Chief)', '', `Verdict: PASS at ${HEAD}`, 'Ran: npm test'].join('\n') + const result = decide({ head: HEAD, author: 'opener', reports: [comment('reviewer-agent', body)], policy }) + assert.equal(result.ok, false) + assert.equal(result.verdict, null) +}) diff --git a/AGENTS.md b/AGENTS.md index 55ce7e55b..17ea7620e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -85,7 +85,7 @@ Decided by the owner (Danyang, GitHub `Birfy`) on 2026-10-18/19. Four scheduled `schedule_list` answers for **the session the call runs in**, not for the Bot. A schedule the owner created in another session of the same Bot is simply absent from the answer, and nothing in the response says which session it answered for — so `[]` is indistinguishable from "nothing is scheduled". This cost two wrong conclusions on 2026-10-21: a Bot reported "0 timers" and the team concluded a live schedule had never existed, and another run came close to deleting and recreating a schedule that was running. -**Never infer "this Bot has no scheduled work" from an empty `schedule_list`, and never delete, update or recreate a schedule on the strength of one.** The owner's UI shows the real list; ask the owner instead of the tool when the run depends on it. `schedule_delete` and `schedule_update` share the same scope, so a Bot can otherwise "clean up" a schedule it cannot see. The DSH-side fix is tracked in #628. +**An empty `schedule_list` is not evidence that a schedule does not exist; never delete or recreate a schedule from it, and never report "no timers" as a finding.** The owner's UI shows the real list; ask the owner instead of the tool when the run depends on it. `schedule_delete` and `schedule_update` share the same scope, so a Bot can otherwise "clean up" a schedule it cannot see. The DSH-side fix is tracked in #628. ### Issue labels @@ -126,7 +126,7 @@ Four rules decide whether a pull request may enter the queue. The mechanical one 1. **`hold` blocks mechanically.** A pull request labelled `hold` fails `merge-gate` on its queue entry. The label is only a marker; the job is the gate. It blocks the queue once `merge-gate` is listed among the required checks of the `main merge queue` ruleset — a ruleset change for the maintainer, never an agent; until then the job reports without blocking. 2. **Labelling a pull request `hold` means adding a record.** Add the label **and** post a new comment saying **Record only — do not merge / 仅记录、禁止入队**. The pair is what holds it. Add that comment; never edit or delete an earlier PASS report to change what it said — the history is the record, and rewriting it is the thing that reads as an incident afterwards. 3. **An authorisation is not a queue permit.** The owner or the Chief saying "let it through", "claim it" or "handle it along with the rest" licenses no specific pull request for the queue. Entering the queue is an explicit action on that one pull request, after its own gate passed, while it carries no `hold`. #566 is the case this exists for: "handle it along with the rest" was read as a permit, the tip was PASSed at 09:48:18Z, enqueued at 09:48:28Z and merged at 09:50:40Z as `abaf938b`. -4. **The newest conclusion at the head being merged decides.** Read the pull request's comments and review bodies, live, for the exact head SHA being merged, and take the newest text that carries `Verdict:` or a `## Review —` heading: INC or REQUEST_CHANGES refuses, PASS allows, no conclusion refuses too. A result from `pull_request` time never stands in — on #610 the tip report ran at 10:31:08Z and the INC landed at 11:24:37Z, 54 minutes later. The gating agent checks this before enqueueing; `merge-gate.yml` checks it again at `merge_group`, and the stricter answer wins. +4. **The newest conclusion at the head being merged decides.** Read the pull request's comments and review bodies, live, for the exact head SHA being merged, and take the newest text that carries `Verdict:` or a `## Review —` heading: INC or REQUEST_CHANGES refuses, PASS allows, no conclusion refuses too. Since #637 the format is fixed: the **first line** of the conclusion is exactly `Verdict: PASS at ` / `Verdict: INC at ` / `Verdict: REQUEST_CHANGES at `, with the attribution on the second line (`Written by: Reviewer. Posted by: Chief (proxy…).`) and the evidence below it — a body that carries a verdict anywhere else is no conclusion, and no conclusion refuses. A result from `pull_request` time never stands in — on #610 the tip report ran at 10:31:08Z and the INC landed at 11:24:37Z, 54 minutes later. The gating agent checks this before enqueueing; `merge-gate.yml` checks it again at `merge_group`, and the stricter answer wins. A review's `state` is not what is read: the account cannot approve its own pull requests, so its reviews stay `COMMENTED` even for an INC, and the INC on #610 was a comment rather than a review. diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index 11029aa4d..fd506b327 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -142,9 +142,8 @@ What counts as a reviewed tip, all of it: Owner decision 2026-10-19: a PR opened by the owner's Bot/agent may be PASSed by the owner's own reviewer Bot/agent, so no independent reviewer is required. Any other author still needs someone else. A self-PASS does not replace green CI, and a new commit voids it. -3. The Verifier block `review-pr` already writes: `Tip SHA:`, `Ran:` with at least one command, - `Not run:` (a gap named, not hidden), and `Verdict: PASS`. -4. The `Tip SHA:` **is the head being judged**. A later commit moves the tip and the old PASS stops +3. The conclusion format is fixed (#637): the **first line** is `Verdict: PASS at ` (or `INC` / `REQUEST_CHANGES`), the second is the attribution (`Written by: Reviewer. Posted by: Chief (proxy…).`), then the evidence — `Ran:` with at least one command and `Not run:` with the gap named, not hidden. A body whose first line is not the verdict is no conclusion. +4. The sha on that first line **is the head being judged**. A later commit moves the tip and the old PASS stops counting; that report is then listed as "not counted", with the sha it named. 5. The **newest** report that names this tip decides, so a later FAIL at the same tip vetoes an earlier PASS, and a later PASS at the same tip replaces a FAIL. From 8ffa52b937dd1da4e9bf1b55b0872c15eeb485e3 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Wed, 21 Oct 2026 22:21:57 +0800 Subject: [PATCH 2/8] Restore #628's sentence in hard rule 6 too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The INC on #632 named hard rule 6 as the wrong text: it used the merged phrasing "no schedule is deleted, updated or recreated on the strength of one" and dropped "never report \"no timers\" as a finding" — the half #628 exists for. Fixing only the section sentence left the hard rule wrong, so a reader who stops at the rules list still gets the version that caused the incident. --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 17ea7620e..1498a8247 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -139,7 +139,7 @@ The standing "file no new issues" rule covers design proposals only: a reproduci 3. A routine PR is merged by the Bot that gated it (owner decision 2026-10-21, [self-gate](#owner-decision-the-bot-gates-and-merges-its-own-prs-2026-10-21-owner-authorized)). A typed chat reply is still not an approval for anything else, and an owner-only decision (scope, security surface, policy) still waits for the owner's word through the normal path (#349). 4. Connector tools are on by default for the owner's Bots and off by default for other people's, until the owner opts in (#359). 5. Bot Social has no keyword commitment scan. -6. A schedule is per session, not per Bot: an empty `schedule_list` is not evidence that a schedule does not exist, and no schedule is deleted, updated or recreated on the strength of one (see [A schedule is per session](#a-schedule-is-per-session-not-per-bot-628)). +6. A schedule is per session, not per Bot: an empty `schedule_list` is not evidence that a schedule does not exist; never delete or recreate a schedule from it, and never report "no timers" as a finding (see [A schedule is per session](#a-schedule-is-per-session-not-per-bot-628)). ## What this project is From 28e26508fa7ea930a7ece071d666e5b9192f4265 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Wed, 21 Oct 2026 22:49:58 +0800 Subject: [PATCH 3/8] Run the merge gate on pull_request too, as a report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #639: a required check that only ever reports on `merge_group` leaves every pull request waiting for a status no run produces, so nothing can enter the queue. The same job now also runs on `pull_request` — and never fails there: a verdict arrives as a comment, which starts no `pull_request` run, so a red report would outlive the INC it names until the next push. The queue entry still refuses. Also names the required context: the job name `merge-gate`, not the workflow name `Merge gate`, which no run ever produces. --- .github/scripts/merge-gate.mjs | 41 +++++++++++++++++++------ .github/scripts/merge-gate.test.mjs | 47 ++++++++++++++++++++++++++--- .github/workflows/merge-gate.yml | 26 +++++++++++----- AGENTS.md | 2 +- 4 files changed, 94 insertions(+), 22 deletions(-) diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index 3ac0addfb..ac584efd2 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -3,7 +3,12 @@ // GH_REPO=owner/repo GH_TOKEN=... HEAD_REF=gh-readonly-queue/main/pr-566- node merge-gate.mjs // node merge-gate.mjs --pr 566 [--sha ] [--repo owner/repo] # by hand // -// It answers one question at the only moment that is audited: may this queue entry merge? +// It runs twice, and the two runs answer different questions. On `merge_group` — the only moment at +// which a merge is audited — it asks "may this entry merge?" and fails the job when the answer is no. +// On `pull_request` it reports: a required check must produce a result on the pull request head too, +// or every pull request reads "Expected — waiting for status" and cannot even enter the queue (#639). +// That run never fails, because a verdict arrives as a comment, which starts no `pull_request` run: +// a failure there would outlive its own reason until the next push. // - `hold` on the pull request refuses the entry; // - the newest conclusive text at the head being merged must be a PASS. That judgement is // review-gate.mjs's `decide`, reused rather than rewritten, so "what counts as a review" has one @@ -37,7 +42,7 @@ const short = (s) => String(s ?? '').slice(0, SHORT) // labels: the pull request's labels. head: its current head sha. queuedSha: the sha in head_ref, the // one the queue merges. reports: its comments and review bodies, oldest first (review-gate main()). -export function judge({ labels, head, author, reports, policy, queuedSha }) { +export function judge({ labels, head, author, reports, policy, queuedSha, mode = 'queue' }) { const reasons = [] const target = String(queuedSha ?? head ?? '').toLowerCase() if (!target) return { ok: false, head: '', verdict: null, reasons: ['no head sha to judge'] } @@ -50,16 +55,27 @@ export function judge({ labels, head, author, reports, policy, queuedSha }) { } const verdict = decide({ head: target, author, reports, policy }) if (!verdict.ok) reasons.push(verdict.reason) - return { ok: reasons.length === 0, head: target, held, verdict, reasons } + // A report says what the queue entry would refuse; only the queue entry refuses. Fail a + // `pull_request` run and it keeps failing after the INC it names is answered by a PASS, because a + // comment starts no run of its own (#639). + const report = mode === 'report' + return { ok: report ? true : reasons.length === 0, head: target, held, verdict, reasons, mode } } export function render(result, { pr, labels } = {}) { + const report = result.mode === 'report' const out = [ - `### Merge gate: ${result.ok ? `PASS at ${short(result.head)}` : 'REFUSE'}`, + report + ? `### Merge gate: report for #${pr} at ${short(result.head)} — ${result.reasons.length ? 'the queue entry would refuse' : 'the queue entry would pass'}` + : `### Merge gate: ${result.ok ? `PASS at ${short(result.head)}` : 'REFUSE'}`, '', - result.ok - ? `Queue entry for #${pr} may merge: no \`hold\`, ${result.verdict?.by}'s PASS names ${short(result.head)}.` - : `Queue entry for #${pr} must not merge: ${result.reasons.join('; ')}.`, + report + ? result.reasons.length + ? `A queue entry for this head would be refused: ${result.reasons.join('; ')}. This report never fails.` + : `A queue entry for this head would pass: ${result.verdict?.by}'s PASS names ${short(result.head)}.` + : result.ok + ? `Queue entry for #${pr} may merge: no \`hold\`, ${result.verdict?.by}'s PASS names ${short(result.head)}.` + : `Queue entry for #${pr} must not merge: ${result.reasons.join('; ')}.`, '', `Pull request #${pr}, head \`${result.head}\`, labels: ${(labels ?? []).join(', ') || '(none)'}.`, ] @@ -100,10 +116,15 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prO .map((v) => ({ kind: 'review', login: v.user?.login, body: v.body, at: v.submitted_at })), ] - const result = judge({ labels, head, author, reports, policy: loadPolicy(policyPath), queuedSha: fromRef?.sha || arg('--sha') }) - console.log(`merge-gate: PR #${pr} head ${short(head)}${fromRef ? `, queue entry ${short(fromRef.sha)}` : ''}, labels [${labels.join(', ')}]`) + // The workflow passes both events through this same entry point: `merge_group` judges, `pull_request` + // reports (#639). MODE (or `--report`) is there for a hand run and for the tests. + const mode = (env.MODE || (process.argv.includes('--report') ? 'report' : 'queue')) === 'report' ? 'report' : 'queue' + const result = judge({ labels, head, author, reports, policy: loadPolicy(policyPath), queuedSha: fromRef?.sha || arg('--sha'), mode }) + console.log(`merge-gate: PR #${pr} head ${short(head)}${fromRef ? `, queue entry ${short(fromRef.sha)}` : ''}, labels [${labels.join(', ')}], mode ${mode}`) console.log(`merge-gate: newest conclusion at ${short(result.head)} — ${result.verdict?.verdict ?? 'none'}${result.verdict?.by ? ` by ${result.verdict.by}` : ''}`) - console.log(`merge-gate: ${result.ok ? 'PASS' : `REFUSE — ${result.reasons.join('; ')}`}`) + console.log(mode === 'report' + ? `merge-gate: report — ${result.reasons.length ? `the queue entry would refuse: ${result.reasons.join('; ')}` : 'the queue entry would pass'}` + : `merge-gate: ${result.ok ? 'PASS' : `REFUSE — ${result.reasons.join('; ')}`}`) const text = render(result, { pr, labels }) if (env.GITHUB_STEP_SUMMARY) appendFileSync(env.GITHUB_STEP_SUMMARY, text + '\n') else console.log('\n' + text) diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index e5018771a..a7c08aa19 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -85,18 +85,23 @@ test('no head sha refuses instead of passing', () => { assert.match(r.reasons.join('; '), /no head sha/) }) -test('workflow: merge_group only, read-only, judges the base copy, acts on nothing', () => { +test('workflow: merge_group judges, pull_request reports, read-only, judges the base copy', () => { const wf = readFileSync(new URL('../workflows/merge-gate.yml', import.meta.url), 'utf8') const triggers = wf.slice(wf.indexOf('\non:\n'), wf.indexOf('\nconcurrency:')) const perms = wf.slice(wf.indexOf('\npermissions:\n'), wf.indexOf('\njobs:')) - assert.match(triggers, /^ {2}merge_group:\s*$/m, 'the gate runs on the queue entry, not on the pull request') + assert.match(triggers, /^ {2}merge_group:\s*$/m, 'the queue entry is where the judgement happens') + // #639: a required check that only runs on merge_group leaves every pull request waiting for a + // status that never comes, so the same job also reports on the pull request head. + assert.match(triggers, /^ {2}pull_request:\s*$/m, 'the same check reports on the pull request head') assert.doesNotMatch(triggers, /pull_request_target/, 'never a write token on pull request code') - assert.doesNotMatch(triggers, /pull_request:|push:|schedule/, 'a review written later is only visible at the queue entry') + assert.doesNotMatch(triggers, /push:|schedule/, 'neither a merge nor a push runs this') + assert.match(wf, /MODE: \$\{\{ github\.event_name == 'pull_request' && 'report' \|\| 'queue' \}\}/, 'the pull_request run is the reporting one') assert.match(perms, /^permissions:\n(?: {2}[\w-]+: read\n)+$/m) assert.doesNotMatch(perms, /: write/, 'the gate never writes') assert.match(wf, /^ {4}name: merge-gate$/m, 'the job name is what a required-check list matches') - assert.match(wf, /ref: \$\{\{ github\.event\.merge_group\.base_sha \}\}/, 'the script and the policy come from the base commit') + assert.match(wf, /ref: \$\{\{ github\.event\.merge_group\.base_sha \|\| github\.event\.pull_request\.base\.sha \}\}/, 'the script and the policy come from the base commit, never from the pull request') assert.match(wf, /HEAD_REF: \$\{\{ github\.event\.merge_group\.head_ref \}\}/) + assert.match(wf, /PR: \$\{\{ github\.event\.pull_request\.number \}\}/, 'a pull_request run has no head_ref, so it passes the number') assert.match(wf, /run: node base\/\.github\/scripts\/merge-gate\.mjs/) assert.doesNotMatch(wf, /continue-on-error: true/, 'a refusal must fail the job, not report it like review-gate.yml does') assert.doesNotMatch(wf, /gh pr merge|--approve|reviewDecision|GH_TOKEN: \$\{\{ secrets\./, 'no merge, no approval, no secret') @@ -172,3 +177,37 @@ test('#637: a review body in the fixed format is judged the same as a comment', assert.equal(result.ok, true) assert.equal(result.verdict.by, 'reviewer-agent') }) + +// ---- #639: the pull_request run reports and never fails; the queue entry still refuses. +test('#639: a report names what the entry would refuse and still passes', () => { + const noVerdict = judgeAt({ mode: 'report', reports: [] }) + assert.equal(noVerdict.ok, true, 'a report never fails') + assert.match(noVerdict.reasons.join('; '), /no Verifier report names the tip/) + assert.equal(noVerdict.mode, 'report') + + const held = judgeAt({ mode: 'report', labels: ['hold'] }) + assert.equal(held.ok, true) + assert.match(held.reasons.join('; '), /`hold` label/) + + const inc = judgeAt({ mode: 'report', reports: [say(report({ verdict: 'INC' }))] }) + assert.equal(inc.ok, true) + assert.match(inc.reasons.join('; '), /INC/) + + // The same inputs at the queue entry keep their old meaning: no conclusion refuses. + assert.equal(judgeAt({ mode: 'queue', reports: [] }).ok, false) + assert.equal(judgeAt({ mode: 'queue', labels: ['hold'] }).ok, false) + assert.equal(judgeAt({ mode: 'queue', reports: [say(report({ verdict: 'INC' }))] }).ok, false) +}) + +test('#639: the pull_request entry point reports without a head_ref and does not throw', () => { + const policy = join(mkdtempSync(join(tmpdir(), 'merge-gate-policy-')), 'review-policy.json') + writeFileSync(policy, JSON.stringify({ reviewers: ['reviewer-agent'], selfReview: [] })) + const result = main({ + gh: fakeGh({ comments: [apiComment(report({ verdict: 'INC' }), '2026-10-21T12:49:00Z')] }), + env: { GITHUB_STEP_SUMMARY: '', REVIEW_POLICY: policy, MODE: 'report', PR: '639' }, + repo: 'ThinkFlowLab/FlyBot', + }) + assert.equal(result.ok, true, 'the report passes even when the entry would be refused') + assert.equal(result.mode, 'report') + assert.match(result.reasons.join('; '), /INC/) +}) diff --git a/.github/workflows/merge-gate.yml b/.github/workflows/merge-gate.yml index 930e806fa..021b1f0a1 100644 --- a/.github/workflows/merge-gate.yml +++ b/.github/workflows/merge-gate.yml @@ -4,10 +4,19 @@ name: Merge gate # refused when its pull request carries `hold`, or when the newest conclusive text at the head about # to be merged is INC / REQUEST_CHANGES. `.github/scripts/merge-gate.mjs` does the judging. # -# `merge_group`, not `pull_request`: `review-gate.yml` judges a tip when it changes, and #610 shows why -# that is not a gate. Its report ran at 10:31:08Z; the INC landed at 11:24:37Z, 54 minutes later. A -# result from before the verdict says nothing about the merge, so the judgement happens at the moment -# that is audited — the queue entry — against the head SHA the queue is about to merge, re-read live. +# Two events, two jobs in one. On `merge_group` it judges: `review-gate.yml` reports a tip when it +# changes, and #610 shows why that is not a gate — its report ran at 10:31:08Z and the INC landed at +# 11:24:37Z, 54 minutes later. So the judgement happens at the moment that is audited, against the head +# SHA the queue is about to merge, re-read live, and a refusal fails the job. +# +# On `pull_request` it reports instead, and never fails. A check listed as required must also produce a +# result on the pull request head, or every pull request reads "Expected — waiting for status" and +# cannot enter the queue at all (#639). It cannot fail there either: a verdict arrives as a comment, +# which starts no `pull_request` run, so a red report would outlive the INC it named until the next +# push. The refused ones are named in the summary; the queue entry refuses for real. +# +# The required status check is the **job name**, `merge-gate` — not the workflow name `Merge gate`. +# A ruleset that lists `Merge gate` requires a context no run ever produces. # # A `merge_group` event carries no pull request, no labels and no reviews. Only `head_ref` # (`gh-readonly-queue/main/pr--`) points at them, so the script parses N back out and asks the @@ -18,9 +27,10 @@ name: Merge gate # commit of the group, never from the pull request's own code. on: merge_group: + pull_request: concurrency: - group: merge-gate-${{ github.event.merge_group.head_sha }} + group: merge-gate-${{ github.event.merge_group.head_sha || github.event.pull_request.head.sha }} cancel-in-progress: false permissions: @@ -36,15 +46,17 @@ jobs: steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event.merge_group.base_sha }} + ref: ${{ github.event.merge_group.base_sha || github.event.pull_request.base.sha }} path: base persist-credentials: false - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 with: node-version: 24 - - name: Judge the queue entry + - name: Judge the queue entry, or report on the pull request env: GH_TOKEN: ${{ github.token }} GH_REPO: ${{ github.repository }} HEAD_REF: ${{ github.event.merge_group.head_ref }} + PR: ${{ github.event.pull_request.number }} + MODE: ${{ github.event_name == 'pull_request' && 'report' || 'queue' }} run: node base/.github/scripts/merge-gate.mjs diff --git a/AGENTS.md b/AGENTS.md index 1498a8247..89a1f4345 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -126,7 +126,7 @@ Four rules decide whether a pull request may enter the queue. The mechanical one 1. **`hold` blocks mechanically.** A pull request labelled `hold` fails `merge-gate` on its queue entry. The label is only a marker; the job is the gate. It blocks the queue once `merge-gate` is listed among the required checks of the `main merge queue` ruleset — a ruleset change for the maintainer, never an agent; until then the job reports without blocking. 2. **Labelling a pull request `hold` means adding a record.** Add the label **and** post a new comment saying **Record only — do not merge / 仅记录、禁止入队**. The pair is what holds it. Add that comment; never edit or delete an earlier PASS report to change what it said — the history is the record, and rewriting it is the thing that reads as an incident afterwards. 3. **An authorisation is not a queue permit.** The owner or the Chief saying "let it through", "claim it" or "handle it along with the rest" licenses no specific pull request for the queue. Entering the queue is an explicit action on that one pull request, after its own gate passed, while it carries no `hold`. #566 is the case this exists for: "handle it along with the rest" was read as a permit, the tip was PASSed at 09:48:18Z, enqueued at 09:48:28Z and merged at 09:50:40Z as `abaf938b`. -4. **The newest conclusion at the head being merged decides.** Read the pull request's comments and review bodies, live, for the exact head SHA being merged, and take the newest text that carries `Verdict:` or a `## Review —` heading: INC or REQUEST_CHANGES refuses, PASS allows, no conclusion refuses too. Since #637 the format is fixed: the **first line** of the conclusion is exactly `Verdict: PASS at ` / `Verdict: INC at ` / `Verdict: REQUEST_CHANGES at `, with the attribution on the second line (`Written by: Reviewer. Posted by: Chief (proxy…).`) and the evidence below it — a body that carries a verdict anywhere else is no conclusion, and no conclusion refuses. A result from `pull_request` time never stands in — on #610 the tip report ran at 10:31:08Z and the INC landed at 11:24:37Z, 54 minutes later. The gating agent checks this before enqueueing; `merge-gate.yml` checks it again at `merge_group`, and the stricter answer wins. +4. **The newest conclusion at the head being merged decides.** Read the pull request's comments and review bodies, live, for the exact head SHA being merged, and take the newest text that carries `Verdict:` or a `## Review —` heading: INC or REQUEST_CHANGES refuses, PASS allows, no conclusion refuses too. Since #637 the format is fixed: the **first line** of the conclusion is exactly `Verdict: PASS at ` / `Verdict: INC at ` / `Verdict: REQUEST_CHANGES at `, with the attribution on the second line (`Written by: Reviewer. Posted by: Chief (proxy…).`) and the evidence below it — a body that carries a verdict anywhere else is no conclusion, and no conclusion refuses. A result from `pull_request` time never stands in — on #610 the tip report ran at 10:31:08Z and the INC landed at 11:24:37Z, 54 minutes later. The gating agent checks this before enqueueing; `merge-gate.yml` checks it again at `merge_group`, and the stricter answer wins. That job runs on two events: at `merge_group` it judges and a refusal fails it, and at `pull_request` it only reports what the queue entry would refuse — a required check that never reports on the pull request head leaves every pull request waiting for a status that never comes and unable to enter the queue at all (#639). The required status check is the job name `merge-gate`; the workflow name `Merge gate` is not a context any run produces. A review's `state` is not what is read: the account cannot approve its own pull requests, so its reviews stay `COMMENTED` even for an INC, and the INC on #610 was a comment rather than a review. From f28f63baaad835b1866714bfee4fbc7ab5091639 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Wed, 21 Oct 2026 22:51:49 +0800 Subject: [PATCH 4/8] Do not fail the check over an older base copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pull_request run takes the script from the base commit, so a PR that adds report mode is judged by the base copy — which refuses for lack of a conclusion and failed its own check on the first run. That failure is not a verdict: the report is informational, and the queue entry re-reads with the code it will actually merge. --- .github/scripts/merge-gate.test.mjs | 5 ++++- .github/workflows/merge-gate.yml | 5 +++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index a7c08aa19..7dd87d660 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -103,7 +103,10 @@ test('workflow: merge_group judges, pull_request reports, read-only, judges the assert.match(wf, /HEAD_REF: \$\{\{ github\.event\.merge_group\.head_ref \}\}/) assert.match(wf, /PR: \$\{\{ github\.event\.pull_request\.number \}\}/, 'a pull_request run has no head_ref, so it passes the number') assert.match(wf, /run: node base\/\.github\/scripts\/merge-gate\.mjs/) - assert.doesNotMatch(wf, /continue-on-error: true/, 'a refusal must fail the job, not report it like review-gate.yml does') + assert.doesNotMatch(wf, /continue-on-error: true/, 'a refusal must fail the queue entry') + // The pull_request run is the exception: the script comes from the base commit, which may predate + // report mode and refuse for lack of a conclusion — that must not fail the check (#639). + assert.match(wf, /continue-on-error: \$\{\{ github\.event_name == 'pull_request' \}\}/, 'only the report run tolerates the base copy refusing') assert.doesNotMatch(wf, /gh pr merge|--approve|reviewDecision|GH_TOKEN: \$\{\{ secrets\./, 'no merge, no approval, no secret') }) diff --git a/.github/workflows/merge-gate.yml b/.github/workflows/merge-gate.yml index 021b1f0a1..b87f6c404 100644 --- a/.github/workflows/merge-gate.yml +++ b/.github/workflows/merge-gate.yml @@ -52,7 +52,12 @@ jobs: - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 with: node-version: 24 + # The script and the policy come from the base commit, so a pull request cannot weaken the gate + # that judges it. On `pull_request` that also means the base copy may be older than this + # workflow and refuse for lack of a conclusion — never fail the check over that: the report is + # informational, and the queue entry re-reads with the code it will actually merge. - name: Judge the queue entry, or report on the pull request + continue-on-error: ${{ github.event_name == 'pull_request' }} env: GH_TOKEN: ${{ github.token }} GH_REPO: ${{ github.repository }} From 55dc266474bcfbf689f6504a7db79af9161d7b88 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Thu, 22 Oct 2026 07:08:01 +0800 Subject: [PATCH 5/8] Keep the first-line verdict, and let the gate exempt what needs no PASS #643: once the first line is the conclusion, the scan below it no longer writes the verdict. A body that quotes the PASS an INC replaced, at the same tip, parsed as a PASS: `parseReport` returned PASS and `decide` said ok. #640 INC: `judge()` required a PASS of every pull request, so a T0/T1 fix or the daily release PR would be refused the moment `merge-gate` became a required check. The exemption is read from the same risk rules the queue uses. --- .github/scripts/review-gate.mjs | 6 +++++- .github/scripts/review-gate.test.mjs | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index ef086a00f..3b8aadd4d 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -44,6 +44,10 @@ export function parseReport(body) { out.verdict = first[1].toUpperCase() out.tip = first[2].toLowerCase() } + // #643: once the first line is the conclusion, the loop below must not write `out.verdict` again. + // A body quotes older verdicts at the same tip (`Verdict: PASS at ` inside a comparison), and + // letting one of those overwrite the first line turned an INC into a PASS at the same tip. + const firstIsVerdict = first !== null const from = start < 0 ? 0 : start for (let i = from; i < lines.length; i++) { const line = lines[i].trim() @@ -53,7 +57,7 @@ export function parseReport(body) { out.tip = hex ? hex[0].toLowerCase() : null } m = /^Verdict\s*:\s*(PASS|FAIL|INC|REQUEST_CHANGES)\b/i.exec(line) - if (m) out.verdict = m[1].toUpperCase() + if (m && !firstIsVerdict) out.verdict = m[1].toUpperCase() m = /^Ran\s*:\s*(.*)$/i.exec(line) if (m) { out.ran = [] diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index afd087c99..f4f221c60 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -202,3 +202,27 @@ test('#637: a verdict that is not the first line is not a conclusion', () => { assert.equal(result.ok, false) assert.equal(result.verdict, null) }) + +// #643: a body that quotes an older verdict at the same tip must not overwrite the first line. Seen +// with an INC whose evidence quoted the PASS it replaced, at the same sha: it parsed as PASS. +test('#643: a quoted verdict later in the body does not overwrite the first line', () => { + const body = [ + 'Verdict: INC at 1234567890abcdef', + 'Written by: Reviewer', + '', + 'Ran: node --test .github/scripts/review-gate.test.mjs → 40 pass', + 'Not run: smoke', + '', + 'Earlier, at the same tip, I wrote:', + 'Verdict: PASS at 1234567890abcdef', + ].join('\n') + const parsed = parseReport(body) + assert.equal(parsed.verdict, 'INC', 'the first line is the conclusion') + assert.equal(parsed.tip, '1234567890abcdef') + assert.deepEqual(parsed.ran, ['node --test .github/scripts/review-gate.test.mjs → 40 pass']) + assert.equal(parsed.hasNotRun, true) + + // The old block still parses, and there the last verdict in the block is the one that counts. + const legacy = ['Tip SHA: 1234567890abcdef', 'Ran:', ' - npm test → pass', 'Not run:', ' - smoke', 'Verdict: INC'].join('\n') + assert.equal(parseReport(legacy).verdict, 'INC') +}) From 8203bf3d06d098824cd9c04926814377fce34f33 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Thu, 22 Oct 2026 07:11:19 +0800 Subject: [PATCH 6/8] Exempt T0/T1 and the mechanical release PR from the PASS requirement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #640 INC point 2: `judge()` asked every pull request for a PASS, so the moment `merge-gate` becomes a required check the daily release PR and every T0/T1 fix would be refused — the opposite of AGENTS.md 6. The exemption reads the tier the controller labels and, for a release PR, the exact file list the standing approval names. --- .github/scripts/merge-gate.mjs | 27 +++++++++++++++++++++++---- .github/scripts/merge-gate.test.mjs | 23 ++++++++++++++++++++++- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index ac584efd2..0b95fb240 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -38,11 +38,24 @@ export function prFromHeadRef(ref) { } const names = (labels) => (labels ?? []).map((l) => String(typeof l === 'string' ? l : l?.name ?? '').toLowerCase()) + +// A pull request that may merge without a PASS (AGENTS.md 6): T0/T1, whose tier the controller +// labels, and a mechanical release PR, whose standing approval is limited to these files. +const NO_PASS_TIERS = new Set(['risk:t0', 'risk:t1']) +const RELEASE_FILES = [ + /^CHANGELOG\.md$/, + /^changelog\.d\//, + /^core\/package\.json$/, + /^core\/package-lock\.json$/, + /^patches\/ds-bot\/0007-version-and-tooling\.patch$/, +] +export const isMechanicalRelease = (files) => (files ?? []).length > 0 && files.every((f) => RELEASE_FILES.some((re) => re.test(String(f)))) +export const exemptFromPass = ({ labels, files }) => names(labels).some((n) => NO_PASS_TIERS.has(n)) || isMechanicalRelease(files) const short = (s) => String(s ?? '').slice(0, SHORT) // labels: the pull request's labels. head: its current head sha. queuedSha: the sha in head_ref, the // one the queue merges. reports: its comments and review bodies, oldest first (review-gate main()). -export function judge({ labels, head, author, reports, policy, queuedSha, mode = 'queue' }) { +export function judge({ labels, files, head, author, reports, policy, queuedSha, mode = 'queue' }) { const reasons = [] const target = String(queuedSha ?? head ?? '').toLowerCase() if (!target) return { ok: false, head: '', verdict: null, reasons: ['no head sha to judge'] } @@ -53,13 +66,16 @@ export function judge({ labels, head, author, reports, policy, queuedSha, mode = if (queuedSha && head && !String(head).toLowerCase().startsWith(target)) { reasons.push(`the pull request head moved to ${short(head)} after this entry was built at ${short(target)}`) } - const verdict = decide({ head: target, author, reports, policy }) + // T0/T1 and the daily release PR need no PASS, so the gate must not refuse them for lacking one + // (#640, INC point 2): a refusal here would stop every routine fix and every release. + const exempt = exemptFromPass({ labels, files }) + const verdict = exempt ? { ok: true, verdict: null, by: null, exempt: true } : decide({ head: target, author, reports, policy }) if (!verdict.ok) reasons.push(verdict.reason) // A report says what the queue entry would refuse; only the queue entry refuses. Fail a // `pull_request` run and it keeps failing after the INC it names is answered by a PASS, because a // comment starts no run of its own (#639). const report = mode === 'report' - return { ok: report ? true : reasons.length === 0, head: target, held, verdict, reasons, mode } + return { ok: report ? true : reasons.length === 0, head: target, held, verdict, exempt, reasons, mode } } export function render(result, { pr, labels } = {}) { @@ -73,6 +89,8 @@ export function render(result, { pr, labels } = {}) { ? result.reasons.length ? `A queue entry for this head would be refused: ${result.reasons.join('; ')}. This report never fails.` : `A queue entry for this head would pass: ${result.verdict?.by}'s PASS names ${short(result.head)}.` + : result.ok && result.exempt + ? `Queue entry for #${pr} may merge without a PASS: ${result.reasons.length ? 'no \`hold\`' : 'a T0/T1 tier or a mechanical release PR (AGENTS.md 6)'}.` : result.ok ? `Queue entry for #${pr} may merge: no \`hold\`, ${result.verdict?.by}'s PASS names ${short(result.head)}.` : `Queue entry for #${pr} must not merge: ${result.reasons.join('; ')}.`, @@ -119,7 +137,8 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prO // The workflow passes both events through this same entry point: `merge_group` judges, `pull_request` // reports (#639). MODE (or `--report`) is there for a hand run and for the tests. const mode = (env.MODE || (process.argv.includes('--report') ? 'report' : 'queue')) === 'report' ? 'report' : 'queue' - const result = judge({ labels, head, author, reports, policy: loadPolicy(policyPath), queuedSha: fromRef?.sha || arg('--sha'), mode }) + const files = list(`repos/${repo}/pulls/${pr}/files?per_page=100`).map((f) => f.filename).filter(Boolean) + const result = judge({ labels, files, head, author, reports, policy: loadPolicy(policyPath), queuedSha: fromRef?.sha || arg('--sha'), mode }) console.log(`merge-gate: PR #${pr} head ${short(head)}${fromRef ? `, queue entry ${short(fromRef.sha)}` : ''}, labels [${labels.join(', ')}], mode ${mode}`) console.log(`merge-gate: newest conclusion at ${short(result.head)} — ${result.verdict?.verdict ?? 'none'}${result.verdict?.by ? ` by ${result.verdict.by}` : ''}`) console.log(mode === 'report' diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index 7dd87d660..e46793761 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -117,9 +117,10 @@ import { mkdtempSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' -const fakeGh = ({ comments, reviews = [], labels = [] }) => (...args) => { +const fakeGh = ({ comments, reviews = [], labels = [], files = [] }) => (...args) => { const path = args[args.length - 1] const pull = { head: { sha: HEAD }, user: { login: 'opener' }, labels: labels.map((name) => ({ name })) } + if (path.includes('/files')) return (files ?? []).map((filename) => JSON.stringify({ filename })).join('\n') if (path.includes('/comments')) return [comments].flat().map((entry) => JSON.stringify(entry)).join('\n') if (path.includes('/reviews')) return [reviews].flat().map((entry) => JSON.stringify(entry)).join('\n') if (/\/pulls\/\d+$/.test(path)) return JSON.stringify(pull) @@ -214,3 +215,23 @@ test('#639: the pull_request entry point reports without a head_ref and does not assert.equal(result.mode, 'report') assert.match(result.reasons.join('; '), /INC/) }) + +// ---- #640 INC point 2: T0/T1 and the daily release PR need no PASS (AGENTS.md 6). +test('#640: a T0 or T1 pull request is not refused for lacking a PASS', () => { + const t1 = judgeAt({ labels: ['risk:T1'], reports: [] }) + assert.equal(t1.ok, true) + assert.equal(t1.exempt, true) + assert.equal(judgeAt({ labels: ['risk:T0'], reports: [] }).ok, true) + assert.equal(judgeAt({ labels: ['risk:T2'], reports: [] }).ok, false, 'T2 still needs a PASS') + assert.equal(judgeAt({ labels: [], reports: [] }).ok, false) + // hold is not waived by a tier + assert.equal(judgeAt({ labels: ['risk:T1', 'hold'], reports: [] }).ok, false) +}) + +test('#640: a mechanical release PR passes on its files alone, and only on those', () => { + const release = ['CHANGELOG.md', 'changelog.d/123-x.fixed.md', 'core/package.json', 'core/package-lock.json', 'patches/ds-bot/0007-version-and-tooling.patch'] + assert.equal(judgeAt({ files: release, reports: [] }).ok, true) + assert.equal(judgeAt({ files: release, reports: [] }).exempt, true) + assert.equal(judgeAt({ files: [...release, 'core/src/host/secrets.js'], reports: [] }).ok, false, 'one file outside the list needs a PASS') + assert.equal(judgeAt({ files: [], reports: [] }).ok, false, 'no files is not a release PR') +}) From fa9a76f4d438cdca837de01946768a8216cc0ba8 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Thu, 22 Oct 2026 07:55:59 +0800 Subject: [PATCH 7/8] Keep the first-line tip, and compute the no-PASS tier from the diff (#640) parseReport() let the body overwrite `out.tip` after the first line had already named the tip, so an INC at this head was read as a conclusion about another tip, `decide` dropped it, and the PASS it refused stayed in force. The first line is final for both fields now, and its own `Ran:` is the evidence. `exemptFromPass` no longer reads `risk:t0`/`risk:t1` labels: with one shared account a label is a claim, not a fact (#627). merge-gate.mjs runs the base copy of risk-gate.mjs over the changed files (and the base risk-rules.json), so the tier that decides "no PASS needed" is computed from the diff. --- .github/scripts/merge-gate.mjs | 42 ++++++++++++++++++++++------ .github/scripts/merge-gate.test.mjs | 25 ++++++++++++----- .github/scripts/review-gate.mjs | 31 ++++++++++++-------- .github/scripts/review-gate.test.mjs | 40 ++++++++++++++++++++++++++ AGENTS.md | 2 ++ docs/autonomous-dev.md | 6 ++-- 6 files changed, 117 insertions(+), 29 deletions(-) diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index 0b95fb240..05e623a65 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -25,6 +25,7 @@ import { appendFileSync } from 'node:fs' import { execFileSync } from 'node:child_process' import { fileURLToPath } from 'node:url' import { decide, loadPolicy } from './review-gate.mjs' +import { createGate, loadRules } from './risk-gate.mjs' const SHORT = 12 const HOLD = 'hold' @@ -39,9 +40,20 @@ export function prFromHeadRef(ref) { const names = (labels) => (labels ?? []).map((l) => String(typeof l === 'string' ? l : l?.name ?? '').toLowerCase()) -// A pull request that may merge without a PASS (AGENTS.md 6): T0/T1, whose tier the controller -// labels, and a mechanical release PR, whose standing approval is limited to these files. -const NO_PASS_TIERS = new Set(['risk:t0', 'risk:t1']) +// The changed files as `GET /pulls/{n}/files` returns them; a bare name is enough for the path rules, +// which is what a hand run and the tests pass. +const fileList = (files) => (files ?? []).map((f) => (typeof f === 'string' ? { filename: f } : f)).filter((f) => f?.filename) +const fileNames = (files) => fileList(files).map((f) => f.filename) + +// A pull request that may merge without a PASS (AGENTS.md 6): T0/T1, and a mechanical release PR, +// whose standing approval is limited to these files. +// +// The tier is computed from the diff by the base copy of `risk-gate.mjs` and +// `.github/agent/risk-rules.json` (the workflow checks this script out from the base commit, so a pull +// request cannot change the rules that judge it). It is deliberately not read from `risk:t0`/`risk:t1` +// labels: every Bot shares one account and a label carries no signature (#627), so whoever can label +// could turn a T3 into "no review needed" — which is the whole gate. +const NO_PASS_TIERS = new Set(['T0', 'T1']) const RELEASE_FILES = [ /^CHANGELOG\.md$/, /^changelog\.d\//, @@ -50,7 +62,18 @@ const RELEASE_FILES = [ /^patches\/ds-bot\/0007-version-and-tooling\.patch$/, ] export const isMechanicalRelease = (files) => (files ?? []).length > 0 && files.every((f) => RELEASE_FILES.some((re) => re.test(String(f)))) -export const exemptFromPass = ({ labels, files }) => names(labels).some((n) => NO_PASS_TIERS.has(n)) || isMechanicalRelease(files) +const RISK_RULES = fileURLToPath(new URL('../agent/risk-rules.json', import.meta.url)) +let riskGate = null +export const computedTier = (files) => { + riskGate ??= createGate(loadRules(RISK_RULES)) + return riskGate.judge(fileList(files)).tier +} +// No files is not an exemption: an empty list would otherwise compute T0 and let anything through. +export const exemptFromPass = ({ tier, files }) => { + const list = fileList(files) + if (!list.length) return false + return NO_PASS_TIERS.has(tier ?? computedTier(list)) || isMechanicalRelease(fileNames(list)) +} const short = (s) => String(s ?? '').slice(0, SHORT) // labels: the pull request's labels. head: its current head sha. queuedSha: the sha in head_ref, the @@ -68,14 +91,15 @@ export function judge({ labels, files, head, author, reports, policy, queuedSha, } // T0/T1 and the daily release PR need no PASS, so the gate must not refuse them for lacking one // (#640, INC point 2): a refusal here would stop every routine fix and every release. - const exempt = exemptFromPass({ labels, files }) + const tier = fileList(files).length ? computedTier(files) : null + const exempt = exemptFromPass({ tier, files }) const verdict = exempt ? { ok: true, verdict: null, by: null, exempt: true } : decide({ head: target, author, reports, policy }) if (!verdict.ok) reasons.push(verdict.reason) // A report says what the queue entry would refuse; only the queue entry refuses. Fail a // `pull_request` run and it keeps failing after the INC it names is answered by a PASS, because a // comment starts no run of its own (#639). const report = mode === 'report' - return { ok: report ? true : reasons.length === 0, head: target, held, verdict, exempt, reasons, mode } + return { ok: report ? true : reasons.length === 0, head: target, held, verdict, exempt, tier, reasons, mode } } export function render(result, { pr, labels } = {}) { @@ -90,7 +114,7 @@ export function render(result, { pr, labels } = {}) { ? `A queue entry for this head would be refused: ${result.reasons.join('; ')}. This report never fails.` : `A queue entry for this head would pass: ${result.verdict?.by}'s PASS names ${short(result.head)}.` : result.ok && result.exempt - ? `Queue entry for #${pr} may merge without a PASS: ${result.reasons.length ? 'no \`hold\`' : 'a T0/T1 tier or a mechanical release PR (AGENTS.md 6)'}.` + ? `Queue entry for #${pr} may merge without a PASS: ${result.reasons.length ? 'no \`hold\`' : `${result.tier ? 'a ' + result.tier : 'a T0/T1'} tier or a mechanical release PR (AGENTS.md 6), computed from the changed files`}.` : result.ok ? `Queue entry for #${pr} may merge: no \`hold\`, ${result.verdict?.by}'s PASS names ${short(result.head)}.` : `Queue entry for #${pr} must not merge: ${result.reasons.join('; ')}.`, @@ -137,7 +161,9 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prO // The workflow passes both events through this same entry point: `merge_group` judges, `pull_request` // reports (#639). MODE (or `--report`) is there for a hand run and for the tests. const mode = (env.MODE || (process.argv.includes('--report') ? 'report' : 'queue')) === 'report' ? 'report' : 'queue' - const files = list(`repos/${repo}/pulls/${pr}/files?per_page=100`).map((f) => f.filename).filter(Boolean) + // The whole file record, not just the name: the T0/T1 exemption is computed from the diff by the + // base copy of risk-gate.mjs (see exemptFromPass). + const files = list(`repos/${repo}/pulls/${pr}/files?per_page=100`) const result = judge({ labels, files, head, author, reports, policy: loadPolicy(policyPath), queuedSha: fromRef?.sha || arg('--sha'), mode }) console.log(`merge-gate: PR #${pr} head ${short(head)}${fromRef ? `, queue entry ${short(fromRef.sha)}` : ''}, labels [${labels.join(', ')}], mode ${mode}`) console.log(`merge-gate: newest conclusion at ${short(result.head)} — ${result.verdict?.verdict ?? 'none'}${result.verdict?.by ? ` by ${result.verdict.by}` : ''}`) diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index e46793761..72dd8ef93 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -217,15 +217,26 @@ test('#639: the pull_request entry point reports without a head_ref and does not }) // ---- #640 INC point 2: T0/T1 and the daily release PR need no PASS (AGENTS.md 6). -test('#640: a T0 or T1 pull request is not refused for lacking a PASS', () => { - const t1 = judgeAt({ labels: ['risk:T1'], reports: [] }) +test('#640: the tier comes from the changed files, and a label never exempts', () => { + const t1 = judgeAt({ files: ['core/src/client/panel.js'], reports: [] }) assert.equal(t1.ok, true) assert.equal(t1.exempt, true) - assert.equal(judgeAt({ labels: ['risk:T0'], reports: [] }).ok, true) - assert.equal(judgeAt({ labels: ['risk:T2'], reports: [] }).ok, false, 'T2 still needs a PASS') - assert.equal(judgeAt({ labels: [], reports: [] }).ok, false) - // hold is not waived by a tier - assert.equal(judgeAt({ labels: ['risk:T1', 'hold'], reports: [] }).ok, false) + assert.equal(t1.tier, 'T1') + const t0 = judgeAt({ files: ['docs/subsystems/core-host.md'], reports: [] }) + assert.equal(t0.ok, true) + assert.equal(t0.tier, 'T0') + assert.equal(judgeAt({ files: ['core/src/host/turns.js'], reports: [] }).ok, false, 'T2 still needs a PASS') + assert.equal(judgeAt({ files: [], reports: [] }).ok, false, 'no files is no exemption') + + // A label is a claim, not a fact: every Bot shares one account and a label carries no signature + // (#627), so `risk:T1` on a T3 diff must still refuse. The tier is computed by the base copy of + // risk-gate.mjs and its rules, never read off the label. + const claimed = judgeAt({ files: ['core/src/host/secrets.js'], labels: ['risk:T1'], reports: [] }) + assert.equal(claimed.exempt, false, 'a risk label does not exempt') + assert.equal(claimed.tier, 'T3') + assert.equal(claimed.ok, false) + // hold is not waived by a computed tier either + assert.equal(judgeAt({ files: ['core/src/client/panel.js'], labels: ['hold'], reports: [] }).ok, false) }) test('#640: a mechanical release PR passes on its files alone, and only on those', () => { diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index 3b8aadd4d..bd144a538 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -44,24 +44,31 @@ export function parseReport(body) { out.verdict = first[1].toUpperCase() out.tip = first[2].toLowerCase() } - // #643: once the first line is the conclusion, the loop below must not write `out.verdict` again. - // A body quotes older verdicts at the same tip (`Verdict: PASS at ` inside a comparison), and - // letting one of those overwrite the first line turned an INC into a PASS at the same tip. + // #643, #640: once the first line is the conclusion, the loop below must write neither field again. + // A body quotes older reports (a `Tip SHA: ` block with an older `Verdict: PASS`), and letting + // either field through turned an INC at this tip into a PASS, or into a conclusion about some other + // tip that `decide` then dropped while the PASS it refused came back to life. Both fail open. + let ranSeen = false const firstIsVerdict = first !== null - const from = start < 0 ? 0 : start + // With the first line as the conclusion the evidence is below it, so scan from the top; a + // legacy body is judged by its last block. + const from = firstIsVerdict || start < 0 ? 0 : start for (let i = from; i < lines.length; i++) { const line = lines[i].trim() - let m = /^Tip SHA\s*:\s*(.+)$/i.exec(line) - if (m) { - const hex = /[0-9a-f]{7,40}/i.exec(m[1]) + const tipLine = /^Tip SHA\s*:\s*(.+)$/i.exec(line) + if (tipLine && !firstIsVerdict) { + const hex = /[0-9a-f]{7,40}/i.exec(tipLine[1]) out.tip = hex ? hex[0].toLowerCase() : null } - m = /^Verdict\s*:\s*(PASS|FAIL|INC|REQUEST_CHANGES)\b/i.exec(line) - if (m && !firstIsVerdict) out.verdict = m[1].toUpperCase() - m = /^Ran\s*:\s*(.*)$/i.exec(line) - if (m) { + const verdictLine = /^Verdict\s*:\s*(PASS|FAIL|INC|REQUEST_CHANGES)\b/i.exec(line) + if (verdictLine && !firstIsVerdict) out.verdict = verdictLine[1].toUpperCase() + const ranLine = /^Ran\s*:\s*(.*)$/i.exec(line) + // With the first line as the conclusion, its own `Ran:` is the evidence; a quoted block's + // `Ran:` below it must not replace it. Without one, the last block wins as it always has. + if (ranLine && !(firstIsVerdict && ranSeen)) { + ranSeen = true out.ran = [] - if (m[1].trim() && m[1].trim() !== '-') out.ran.push(m[1].trim()) + if (ranLine[1].trim() && ranLine[1].trim() !== '-') out.ran.push(ranLine[1].trim()) for (let j = i + 1; j < lines.length; j++) { const next = lines[j].trim() if (/^[-•]\s*\S/.test(next)) out.ran.push(next) diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index f4f221c60..b9f4bfea3 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -226,3 +226,43 @@ test('#643: a quoted verdict later in the body does not overwrite the first line const legacy = ['Tip SHA: 1234567890abcdef', 'Ran:', ' - npm test → pass', 'Not run:', ' - smoke', 'Verdict: INC'].join('\n') assert.equal(parseReport(legacy).verdict, 'INC') }) + +// #640 INC point 1: the same hole in `out.tip`. A first line that names this tip, plus an older +// report quoted in the body, used to move the tip, so `decide` dropped the INC as "about another +// tip" and the PASS it refused stayed in force. +test('#640: a quoted Tip SHA later in the body does not move the first line to another tip', () => { + const body = [ + 'Verdict: INC at 1234567890abcdef', + 'Written by: Reviewer', + '', + 'Ran: node --test .github/scripts/review-gate.test.mjs → 43 pass', + 'Not run: smoke', + '', + 'For comparison, the report this replaces said:', + 'Tip SHA: 99887766554433221100ffeeddccbbaa99887766', + 'Ran:', + ' - npm test → pass', + 'Not run:', + ' - smoke', + 'Verdict: PASS', + ].join('\n') + const parsed = parseReport(body) + assert.equal(parsed.verdict, 'INC') + assert.equal(parsed.tip, '1234567890abcdef', 'the first line names the tip being judged') + assert.deepEqual(parsed.ran, ['node --test .github/scripts/review-gate.test.mjs → 43 pass']) + assert.equal(parsed.hasNotRun, true) + + // And the whole point: the INC is not dropped, so it still refuses the merge. + const head = '1234567890abcdef000000000000000000000000' + const at = (report, when) => ({ kind: 'comment', login: 'reviewer-agent', body: report, at: when }) + const policy = { reviewers: ['reviewer-agent'], selfReview: [] } + const pass = ['Tip SHA: ' + head, 'Ran:', ' - npm test → pass', 'Not run:', ' - smoke', 'Verdict: PASS'].join('\n') + const result = decide({ + head, + author: 'opener', + reports: [at(pass, '2026-10-21T10:00:00Z'), at(body, '2026-10-21T11:00:00Z')], + policy, + }) + assert.equal(result.ok, false, 'the newer INC at this tip refuses') + assert.equal(result.verdict, 'INC') +}) diff --git a/AGENTS.md b/AGENTS.md index 89a1f4345..5db464e34 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -130,6 +130,8 @@ Four rules decide whether a pull request may enter the queue. The mechanical one A review's `state` is not what is read: the account cannot approve its own pull requests, so its reviews stay `COMMENTED` even for an INC, and the INC on #610 was a comment rather than a review. +**The T0/T1 exemption is computed, never labelled.** `merge-gate.mjs` runs the base copy of [`risk-gate.mjs`](.github/scripts/risk-gate.mjs) over the changed files and `.github/agent/risk-rules.json` to get the tier; a `risk:T0`/`risk:T1` label does nothing. Every Bot shares one account and a label carries no signature (#627), so a label-based exemption would let whoever can label turn a T3 into "no review needed" — the gate itself. A mechanical release PR is recognised by its files as before, and `hold` still refuses whatever the tier. + The standing "file no new issues" rule covers design proposals only: a reproducible code defect is still filed as an issue, with its reproduction. ### Hard rules for agents diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index fd506b327..4cadfed9b 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -132,8 +132,10 @@ To switch the report off: disable the workflow in the Actions tab, or delete `.g A merge decision needs a check it can read, not a report in a chat. `.github/scripts/review-gate.mjs` answers one question for a pull request: **does the current head carry a Verifier PASS?** AGENTS.md step 6 is the rule it serves. A PASS is required for **T2 and T3** (at the tip, with green CI); -**T0 and T1 need no PASS** and merge on green CI. The agent that gated the PR then enqueues it itself -(owner decision 2026-10-21). This report is the evidence of a PASS; it never merges anything. +**T0 and T1 need no PASS** and merge on green CI; the merge gate computes that tier itself from the +changed files with the base copy of `risk-gate.mjs` and `risk-rules.json`, never from the `risk:T*` +labels (#640: a shared account makes a label a claim, not a fact). The agent that gated the PR then +enqueues it itself (owner decision 2026-10-21). This report is the evidence of a PASS; it never merges anything. What counts as a reviewed tip, all of it: From a55ccc5c26f46cb0f0e8f8a30f17a93e46581360 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Thu, 22 Oct 2026 08:04:22 +0800 Subject: [PATCH 8/8] fix(merge-gate): read the queue entry's head_ref when it is fully qualified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every real `merge_group` run so far failed before judging anything: GitHub passes `merge_group.head_ref` as `refs/heads/gh-readonly-queue/main/pr--`, and `prFromHeadRef` required the string to start at `gh-readonly-queue/`. Five queue entries, five failures — 39602546475, 39602806953, 39653640861, 39694988559 (#626) and 39696278650 (#652) all ended in "cannot read a pull request from head_ref". The gate never once judged a queue entry, so listing `merge-gate` as a required check would have blocked every merge instead of gating it. Accept the `refs/heads/` prefix and keep accepting the bare form, and cover the payload with the test that came off run 39694988559. Refs #637 --- .github/scripts/merge-gate.mjs | 8 +++++++- .github/scripts/merge-gate.test.mjs | 11 +++++++++++ .github/workflows/merge-gate.yml | 3 ++- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index 05e623a65..07c987f95 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -33,8 +33,14 @@ const HOLD = 'hold' // A `merge_group` event carries no pull request, no labels and no reviews. The only pointer to them // is `merge_group.head_ref`, the queue branch the entry was built on: // `gh-readonly-queue//pr--`, where is the head the queue is about to merge. +// +// It arrives fully qualified: every real queue entry so far passed `refs/heads/gh-readonly-queue/...` +// (runs 39602546475, 39602806953, 39653640861, 39694988559, 39696278650 — all five failed with +// "cannot read a pull request from head_ref" before this prefix was accepted, so the gate never once +// judged a queue entry). Accept both spellings rather than depend on which one the event carries. export function prFromHeadRef(ref) { - const m = /^gh-readonly-queue\/[^/]+\/pr-(\d+)-([0-9a-f]{7,40})$/i.exec(String(ref ?? '').trim()) + const branch = String(ref ?? '').trim().replace(/^refs\/heads\//i, '') + const m = /^gh-readonly-queue\/[^/]+\/pr-(\d+)-([0-9a-f]{7,40})$/i.exec(branch) return m ? { pr: Number(m[1]), sha: m[2].toLowerCase() } : null } diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index 72dd8ef93..5bbcdb9cf 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -24,6 +24,17 @@ test('the queue branch is the only pointer to the pull request', () => { assert.equal(prFromHeadRef(undefined), null) }) +// The real event payload, verbatim from run 39694988559 (the #626 queue entry): GitHub passes the +// queue branch fully qualified. Without this, every entry failed before judging anything. +test('#652 follow-up: a head_ref that carries refs/heads/ still names the queue entry', () => { + assert.deepEqual(prFromHeadRef(`refs/heads/${ENTRY}`), { pr: 566, sha: HEAD }) + assert.deepEqual(prFromHeadRef('refs/heads/gh-readonly-queue/main/pr-652-020490044ccad1551f769f0ee141da218f71c1fd'), { + pr: 652, + sha: '020490044ccad1551f769f0ee141da218f71c1fd', + }) + assert.equal(prFromHeadRef('refs/tags/gh-readonly-queue/main/pr-566-' + HEAD), null, 'only a branch ref is a queue entry') +}) + test('a PASS at the merged head lets the entry through', () => { const r = judgeAt() assert.equal(r.ok, true) diff --git a/.github/workflows/merge-gate.yml b/.github/workflows/merge-gate.yml index b87f6c404..45a3b4e93 100644 --- a/.github/workflows/merge-gate.yml +++ b/.github/workflows/merge-gate.yml @@ -19,7 +19,8 @@ name: Merge gate # A ruleset that lists `Merge gate` requires a context no run ever produces. # # A `merge_group` event carries no pull request, no labels and no reviews. Only `head_ref` -# (`gh-readonly-queue/main/pr--`) points at them, so the script parses N back out and asks the +# (`gh-readonly-queue/main/pr--`, passed fully qualified as `refs/heads/...`) points at them, +# so the script parses N back out and asks the # API. Reading a review's `state` never works: the account cannot approve its own pull requests, so its # reviews stay `COMMENTED` even for an INC — the INC on #610 was a comment, not a review. #