From 9e60f7484beb08dfa0cb1bcbd071bbe117efb7a9 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Fri, 23 Oct 2026 16:33:36 +0800 Subject: [PATCH] fix(automation): read the check runs from the endpoint's envelope /commits//check-runs answers { total_count, check_runs }, so `gh api --jq '.[]'` printed a number and then the whole array. planCheck saw neither with a name, and every event POSTed another enqueue-guard run for the same head instead of PATCHing the one already there: the update branch was unreachable, and the step-1 proof in #812 would have shown a second run rather than a flip. The read is `.check_runs[]` now, the test fake answers the envelope and slices it the way --jq does, so the wrong filter is visible in the suite, and a test drives two judgments of one head into one POST and one PATCH. Two more from the same review. --rollback proves the ruleset is gone by reading the list again, and its test asserted the fake's own bookkeeping instead: a fixture that keeps the ruleset listed after the DELETE now fails the rollback. AGENTS.md says the ruleset is on the fork's main once --apply has run, not before, because this pull request deliberately does not apply it. Fixes ThinkFlowLab/FlyBot#812 --- .github/agent/merge-gate-ruleset.json | 36 ++++ .github/scripts/enqueue-guard.mjs | 17 +- .github/scripts/gate-check.mjs | 159 ++++++++++++++ .github/scripts/gate-check.test.mjs | 257 +++++++++++++++++++++++ .github/scripts/gate-ruleset.mjs | 149 +++++++++++++ .github/scripts/gate-ruleset.test.mjs | 186 ++++++++++++++++ .github/workflows/enqueue-guard.yml | 70 ++++++ AGENTS.md | 2 + changelog.d/812-fork-merge-gate.added.md | 1 + docs/autonomous-dev.md | 33 +++ docs/testing-layers.json | 9 + docs/testing-layers.md | 1 + 12 files changed, 916 insertions(+), 4 deletions(-) create mode 100644 .github/agent/merge-gate-ruleset.json create mode 100644 .github/scripts/gate-check.mjs create mode 100644 .github/scripts/gate-check.test.mjs create mode 100644 .github/scripts/gate-ruleset.mjs create mode 100644 .github/scripts/gate-ruleset.test.mjs create mode 100644 .github/workflows/enqueue-guard.yml create mode 100644 changelog.d/812-fork-merge-gate.added.md diff --git a/.github/agent/merge-gate-ruleset.json b/.github/agent/merge-gate-ruleset.json new file mode 100644 index 00000000..d9befc3f --- /dev/null +++ b/.github/agent/merge-gate-ruleset.json @@ -0,0 +1,36 @@ +{ + "name": "main merge gate", + "target": "branch", + "enforcement": "active", + "bypass_actors": [], + "conditions": { + "ref_name": { + "include": ["refs/heads/main"], + "exclude": [] + } + }, + "rules": [ + { "type": "deletion" }, + { "type": "non_fast_forward" }, + { + "type": "required_status_checks", + "parameters": { + "strict_required_status_checks_policy": false, + "required_status_checks": [ + { "context": "tests-ok" }, + { "context": "enqueue-guard" } + ] + } + }, + { + "type": "pull_request", + "parameters": { + "required_approving_review_count": 0, + "dismiss_stale_reviews_on_push": false, + "require_code_owner_review": false, + "require_last_push_approval": false, + "required_review_thread_resolution": false + } + } + ] +} diff --git a/.github/scripts/enqueue-guard.mjs b/.github/scripts/enqueue-guard.mjs index aa30e778..e6ca324b 100644 --- a/.github/scripts/enqueue-guard.mjs +++ b/.github/scripts/enqueue-guard.mjs @@ -107,9 +107,12 @@ function arg(name) { return i > 0 ? process.argv[i + 1] : undefined } -// `gh` and `env` are injectable so a test drives the whole check without the network, the way -// merge-gate.mjs does. -export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prOption } = {}) { +// The decision for one pull request, read live: the single place that reads the pull request, its +// conclusions and its diff, and calls `judge` + `guard`. `main` below prints it (the command an agent +// runs); `gate-check.mjs` turns the same object into the `enqueue-guard` check run the ruleset requires +// (#812). One function with two readers, so the check and the printed command cannot disagree — #812's +// kill condition is a second copy of these rules, and there is none. +export function decidePull({ gh = realGh, env = process.env, repo: repoOption, pr: prOption, expectedHead } = {}) { 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') @@ -133,14 +136,20 @@ 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 result = judge({ labels, files, head, author: pull.user?.login, reports, policy: loadPolicy(policyPath), expectedHead: expectedHead ?? arg('--sha'), mode: 'queue' }) 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 }) + return { decision, pull, repo, pr } +} +// The printed form: the guard's own ALLOW/REFUSE line, then the command to run. +export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prOption } = {}) { + const { decision, pull, pr } = decidePull({ gh, env, repo: repoOption, pr: prOption }) + const labels = (pull.labels ?? []).map((l) => l.name) console.log(`enqueue-guard: PR #${pr} head ${short(decision.head)}, labels [${labels.join(', ')}]`) console.log(decision.ok ? `enqueue-guard: ALLOW — ${decision.message}; ${decision.exempt ? `a ${decision.tier ?? 'T0/T1'} tier or a mechanical release PR (AGENTS.md 6)` : `${decision.verdict?.by}'s PASS names this tip`}` diff --git a/.github/scripts/gate-check.mjs b/.github/scripts/gate-check.mjs new file mode 100644 index 00000000..946b844b --- /dev/null +++ b/.github/scripts/gate-check.mjs @@ -0,0 +1,159 @@ +// The merge gate GitHub enforces, not just a script (#812). +// +// `enqueue-guard.mjs` decides whether a pull request may merge and prints the command; nothing ran it +// before a person merged, and on the code fork `Birfy/FlyBot`, where every routine pull request lands, +// `main` had no ruleset and no branch protection — so a PASS was advisory and #30 merged without +// `agent-pr`. This script writes that same decision onto the pull request head as a check run named +// `enqueue-guard`, the context the ruleset on `main` requires; with no bypass actor, an unreviewed +// pull request cannot be squash-merged through the API at all. +// +// It decides nothing itself: `decidePull` in enqueue-guard.mjs is called, and inside it the merge +// gate's `judge`. A second copy of the rules is #812's kill condition, so there is none — the check and +// the command the guard prints come from one function. +// +// The check is written through the API instead of being the job's own result because it has to flip on +// events no `pull_request` run follows: a verdict arrives as a comment (`issue_comment`), a label is +// added or removed (`labeled`/`unlabeled`), and neither starts a run for the head being judged (#639). +// `external_id` is the head, so a re-judgement PATCHes that one run instead of piling runs up, and a +// check written for an older head is never reused: a stale green must not answer for a new tip. +// +// `enqueue-guard.yml` decides when this runs, and takes the code it runs from `main` +// (`pull_request_target` + `issue_comment` + `workflow_dispatch`); a `pull_request` job from a fork is +// read-only and could not write the check, which is why that trigger is not used. Nothing here +// checks out or runs pull request code: the head is read through the API only. +// +// Exit code: 0 whenever the judgement was written, whatever it concluded — a red `enqueue-guard` check +// is the refusal, and a red job on top of it would report the same thing twice. 1 only when the head +// could not be judged or the check could not be written. `--dry-run` (or `DRY_RUN=1`) judges the live +// head, prints the payload and writes nothing: it is how a person checks the decision against a real +// pull request before the workflow exists on `main`, and it cannot leave a context behind. +import { appendFileSync } from 'node:fs' +import { execFileSync } from 'node:child_process' +import { fileURLToPath } from 'node:url' +import { decidePull } from './enqueue-guard.mjs' + +export const CHECK_NAME = 'enqueue-guard' +// `output.title` is capped by the API at 255 characters; a refusal that names several defects is +// longer than that, and the full text is in the summary either way. +const TITLE_MAX = 250 +const SHORT = 12 +const short = (s) => String(s ?? '').slice(0, SHORT) + +// The check run's body, for `POST /check-runs` and for the fields `PATCH` may change. `conclusion` is +// the decision: `success` only when the guard allows the merge, so every refusal — a `hold`, a tip with +// no PASS, a missing `agent-pr`, a PASS that lists no `Ran:` — is one red required check. +export function payloadFor({ decision, head }) { + const refused = !decision?.ok + const message = decision?.message ?? (refused ? 'the guard refused this head' : 'the guard allows this head') + return { + name: CHECK_NAME, + head_sha: String(head ?? ''), + status: 'completed', + conclusion: refused ? 'failure' : 'success', + external_id: String(head ?? ''), + output: { + title: message.length > TITLE_MAX ? message.slice(0, TITLE_MAX - 1) + '…' : message, + summary: summaryOf({ decision, head }), + }, + } +} + +// The check run this head already carries, if any. One per head, by `external_id`: a label or a comment +// re-judges the same head, and a second run for one context on one commit is a state nobody can read. +export function planCheck({ runs, head } = {}) { + const target = String(head ?? '') + const mine = (runs ?? []).find((run) => + run?.name === CHECK_NAME && + String(run?.external_id ?? '') === target && + String(run?.app?.slug ?? 'github-actions') === 'github-actions') + return mine?.id != null ? { action: 'update', id: mine.id } : { action: 'create' } +} + +// The evidence under the check: same decision, same words, one place to read why it is red. +export function summaryOf({ decision, head, pr } = {}) { + const ok = Boolean(decision?.ok) + const labels = (decision?.labels ?? []).map((l) => `\`${l}\``).join(', ') || '(none)' + const lines = [ + `### \`${CHECK_NAME}\`: ${ok ? 'this head may merge' : 'this head must not merge'}`, + '', + `Pull request${pr ? ` #${pr}` : ''}, head \`${head ?? ''}\`. ${ok + ? 'The ruleset on `main` lets this merge once `tests-ok` is green here too.' + : 'While this check is red, a merge of this head through the API is refused.'}`, + '', + `- decision: \`${decision?.message ?? 'no decision'}\``, + `- labels: ${labels}`, + ] + if (decision?.exempt) lines.push(`- no PASS needed: ${decision.tier ? `computed tier ${decision.tier}` : 'a T0/T1 tier or a mechanical release pull request'} (AGENTS.md → Merge gates)`) + if (decision?.mechanicalRelease) lines.push('- a mechanical release pull request: it carries no issue and no `agent-pr` by design') + if (decision?.fork) lines.push('- the head lives in a fork: the issue reference and `agent-pr` rules are not asked of it') + if (decision?.verdict?.by) lines.push(`- newest conclusion at this tip: ${decision.verdict.verdict ?? 'PASS'} by ${decision.verdict.by}`) + if (decision?.recordOnly) lines.push('- the newest conclusion at this head says "record only — do not merge"') + if (!ok) lines.push('', 'What refuses it:', ...(decision?.reasons?.length ? decision.reasons.map((r) => `- ${r}`) : ['- no reason was recorded — treat this as a bug in the gate itself'])) + lines.push('', '_One decision, two readers: this check and `node .github/scripts/enqueue-guard.mjs --pr ` call the same function. Rules: [`AGENTS.md` → Merge gates](../../AGENTS.md#merge-gates). This check reads the repository and writes this one run; it never merges, labels or comments._') + return lines.join('\n') +} + +// One CLI shape for both callers: the guard's reads are `gh('api', …, path)`, and a write has to carry a +// JSON body, so it is `gh([args], JSON.stringify(body))` (the way controller.mjs calls `ghRaw`). Both +// arrive here, and getting this wrong is invisible to a fake that accepts either shape — a live +// `--dry-run` is what caught it once, which is why that run is part of the verification of this file. +const realGh = (...a) => { + const [args, input] = Array.isArray(a[0]) ? [a[0], a[1]] : [a.filter((x) => x !== undefined), undefined] + return execFileSync('gh', args, { encoding: 'utf8', maxBuffer: 256 << 20, input, stdio: ['pipe', 'pipe', 'pipe'] }) +} + +// `gh` is injectable so a test drives the whole write path without the network: it is called as +// `gh(args, input)`, the way controller.mjs calls `ghRaw`, because the checks API takes a JSON body. +export function applyCheck({ gh = realGh, repo, head, decision } = {}) { + if (!repo) throw new Error('set GH_REPO or pass --repo owner/name') + const runs = gh(['api', '--paginate', '--jq', '.check_runs[]', `repos/${repo}/commits/${head}/check-runs?filter=all&per_page=100`]) + .split('\n').filter(Boolean).map((line) => JSON.parse(line)) + const plan = planCheck({ runs, head }) + const body = payloadFor({ decision, head }) + if (plan.action === 'update') { + // `status` goes with `conclusion`: the API rejects a conclusion that is not paired with a completed + // run, and this run always is one. + const patch = { status: 'completed', conclusion: body.conclusion, output: body.output } + gh(['api', '-X', 'PATCH', `repos/${repo}/check-runs/${plan.id}`, '--input', '-'], JSON.stringify(patch)) + return { action: 'update', id: plan.id, conclusion: body.conclusion } + } + const created = gh(['api', '-X', 'POST', `repos/${repo}/check-runs`, '--input', '-'], JSON.stringify(body)) + let id = null + try { id = JSON.parse(created)?.id ?? null } catch { /* the write succeeded; the id is only for the log */ } + return { action: 'create', id, conclusion: body.conclusion } +} + +export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prOption, dryRun } = {}) { + const repo = repoOption || env.GH_REPO + const pr = prOption || env.PR + if (!repo) throw new Error('set GH_REPO or pass --repo owner/name') + if (!pr) throw new Error('set PR or pass --pr N') + const dry = dryRun ?? (Boolean(env.DRY_RUN) || process.argv.includes('--dry-run')) + let decided + try { + decided = decidePull({ gh, env, repo, pr }) + } catch (error) { + // `issue_comment` fires for a plain issue too, and the workflow filters those out; a pull request + // that no longer exists is not a head to judge. Leave no check behind: a context nothing will ever + // complete would hold a merge that cannot happen anyway, and hide the real reason. + if (/404|Not Found/.test(String(error?.message ?? ''))) { + console.log(`gate-check: pull request ${pr} is gone; nothing to judge`) + return { ok: true, skipped: true } + } + throw error + } + const { decision, pull } = decided + const head = pull.head?.sha + if (!head) throw new Error(`pull request ${pr} has no head sha`) + const applied = dry ? { action: 'dry-run', id: null, conclusion: payloadFor({ decision, head }).conclusion } : applyCheck({ gh, repo, head, decision }) + const summary = summaryOf({ decision, head, pr }) + console.log(`gate-check: PR #${pr} head ${short(head)} — ${CHECK_NAME}: ${applied.conclusion} (${applied.action}${applied.id != null ? ` run ${applied.id}` : ''})`) + console.log(`gate-check: ${decision.message}`) + if (env.GITHUB_STEP_SUMMARY) appendFileSync(env.GITHUB_STEP_SUMMARY, summary + '\n') + else console.log('\n' + summary) + return { ...decision, applied } +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + try { main(); process.exit(0) } catch (e) { console.error(`gate-check: ${e.message}`); process.exit(1) } +} diff --git a/.github/scripts/gate-check.test.mjs b/.github/scripts/gate-check.test.mjs new file mode 100644 index 00000000..00a41fbf --- /dev/null +++ b/.github/scripts/gate-check.test.mjs @@ -0,0 +1,257 @@ +// gate-check (#812): the guard's decision, written onto the pull request head as the `enqueue-guard` +// check run that the ruleset on `main` requires. +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { mkdtempSync, readFileSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { CHECK_NAME, applyCheck, main, payloadFor, planCheck, summaryOf } from './gate-check.mjs' + +const HEAD = 'abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f' +const NEXT = 'cd12ef34050617283940a1b2c3d4e5f60718293a' +const HOST_FILE = 'core/src/host/turns.js' // T2: needs a PASS +const TEST_FILE = 'core/test/gate-check.test.js' // T1: needs none +const REPO = 'Birfy/FlyBot' + +const policyFile = () => { + const path = join(mkdtempSync(join(tmpdir(), 'gate-check-policy-')), 'review-policy.json') + writeFileSync(path, JSON.stringify({ reviewers: ['reviewer-agent', 'Birfy'], selfReview: ['Birfy'], writers: ['reviewer-agent', 'Reviewer', 'Birfy'] })) + return path +} +const POLICY = policyFile() + +const report = ({ tip = HEAD, verdict = 'PASS' } = {}) => + [`Verdict: ${verdict} at ${tip}`, 'Written-by: reviewer-agent', '', 'Ran: npm run verify → pass'].join('\n') +const comment = (body = report(), at = '2026-10-23T07:00:00Z') => + ({ user: { login: 'reviewer-agent' }, body, created_at: at, author_association: 'MEMBER' }) + +// A repository with one pull request and the check runs written on it so far. `gh` answers both call +// shapes the scripts use: variadic reads (`gh('api', …, path)`) and writes (`gh([args], json)`). +const world = ({ head = HEAD, labels = ['agent-pr'], body = 'Fixes #812\n', comments = [], files = [HOST_FILE], author = 'opener', headRepo = REPO } = {}) => { + const state = { head, labels, body, comments, files, author, headRepo, runs: [], writes: [], nextId: 100 } + const pull = () => ({ head: { sha: state.head, repo: { full_name: state.headRepo } }, user: { login: state.author }, body: state.body, labels: state.labels.map((name) => ({ name })) }) + const handle = (args, input) => { + // A write ends with `--input -`, so the API path is the `repos/…` argument, not the last one. + const path = String(args.find((a) => String(a).startsWith('repos/')) ?? args[args.length - 1]) + const methodAt = args.indexOf('-X') + const method = methodAt >= 0 ? args[methodAt + 1] : 'GET' + if (method === 'GET') { + if (/\/pulls\/\d+$/.test(path)) return JSON.stringify(pull()) + if (path.includes('/files')) return state.files.map((f) => JSON.stringify(typeof f === 'string' ? { filename: f } : f)).join('\n') + if (path.includes('/comments')) return state.comments.map((c) => JSON.stringify(c)).join('\n') + if (path.includes('/reviews')) return '' + const sha = /\/commits\/([0-9a-f]+)\/check-runs/.exec(path)?.[1] + if (sha) { + // The endpoint answers an envelope, not an array, and `--jq` slices that answer. Answering with + // runs directly is what let a `.[]` filter pass here and POST a second run against the real API. + const runs = state.runs.filter((r) => r.head_sha === sha) + const jqAt = args.indexOf('--jq') + const filter = jqAt >= 0 ? args[jqAt + 1] : undefined + if (filter === '.check_runs[]') return runs.map((r) => JSON.stringify(r)).join('\n') + if (filter === '.[]') return [String(runs.length), JSON.stringify(runs)].join('\n') + throw new Error('fake gh: unexpected --jq filter ' + filter) + } + throw new Error('fake gh: unexpected read ' + path) + } + const target = path + const sent = JSON.parse(input) + state.writes.push({ method, target, sent }) + if (method === 'POST' && /\/check-runs$/.test(target)) { + const run = { id: state.nextId++, name: sent.name, head_sha: sent.head_sha, external_id: sent.external_id, status: sent.status, conclusion: sent.conclusion, output: sent.output, app: { slug: 'github-actions' } } + state.runs.push(run) + return JSON.stringify(run) + } + if (method === 'PATCH' && /\/check-runs\/\d+$/.test(target)) { + const id = Number(target.split('/').at(-1)) + const run = state.runs.find((r) => r.id === id) + if (!run) throw new Error('fake gh: no such check run ' + id) + Object.assign(run, { status: sent.status, conclusion: sent.conclusion, output: sent.output }) + return JSON.stringify(run) + } + throw new Error('fake gh: unexpected write ' + method + ' ' + target) + } + state.gh = (...a) => { + if (Array.isArray(a[0])) return handle(a[0], a[1]) + return handle(a.filter((x) => x !== undefined), undefined) + } + return state +} + +const run = (state, pr = 812) => main({ gh: state.gh, env: { REVIEW_POLICY: POLICY, GH_REPO: REPO, PR: String(pr) }, repo: REPO, pr }) + +const ALLOWED = { ok: true, head: HEAD, reasons: [], labels: ['agent-pr'], message: `enqueue #812: no \`hold\` and the gate passes` } +const REFUSED = { ok: false, head: HEAD, reasons: ['missing label: agent-pr'], labels: [], message: 'do not enqueue #812: missing label: agent-pr' } + +test('#812: an allowed head becomes a green check named enqueue-guard, tied to that head', () => { + const body = payloadFor({ decision: ALLOWED, head: HEAD }) + assert.equal(body.name, CHECK_NAME) + assert.equal(body.conclusion, 'success') + assert.equal(body.head_sha, HEAD) + // `external_id` is the head, so a re-judgement finds this run instead of piling another one up. + assert.equal(body.external_id, HEAD) + assert.equal(body.status, 'completed') + assert.match(body.output.title, /enqueue #812/) +}) + +test('#812: a refusal becomes a red check whose title is the guard\u2019s own REFUSE sentence', () => { + const body = payloadFor({ decision: REFUSED, head: HEAD }) + assert.equal(body.conclusion, 'failure') + assert.equal(body.output.title, REFUSED.message) + assert.match(body.output.summary, /must not merge/) + assert.match(body.output.summary, /missing label: agent-pr/) +}) + +test('#812: a refusal that names several defects keeps every reason in the summary', () => { + const many = { ok: false, head: HEAD, labels: ['hold'], reasons: ['the pull request carries the `hold` label', 'no Verifier report names the tip abaf938b0c1d', 'missing label: agent-pr'], message: 'do not enqueue #812: the `hold` label' } + const summary = summaryOf({ decision: many, head: HEAD, pr: 812 }) + for (const reason of many.reasons) assert.ok(summary.includes(reason), reason) + // A title is capped by the API; the reasons are not lost, they are in the summary. + assert.ok(payloadFor({ decision: { ...many, message: 'x'.repeat(400) }, head: HEAD }).output.title.length <= 250) +}) + +test('#812: one run per head — a run for an older head is never reused', () => { + const runs = [ + { id: 7, name: CHECK_NAME, head_sha: NEXT, external_id: NEXT, app: { slug: 'github-actions' } }, + { id: 8, name: 'tests-ok', head_sha: HEAD, external_id: HEAD, app: { slug: 'github-actions' } }, + { id: 9, name: CHECK_NAME, head_sha: HEAD, external_id: 'stale-tip', app: { slug: 'github-actions' } }, + ] + assert.deepEqual(planCheck({ runs, head: HEAD }), { action: 'create' }) + const mine = { id: 11, name: CHECK_NAME, head_sha: HEAD, external_id: HEAD, app: { slug: 'github-actions' } } + assert.deepEqual(planCheck({ runs: [mine], head: HEAD }), { action: 'update', id: 11 }) +}) + +test('#812: the same head flips red then green on one check run — a label or a comment re-judges it', () => { + const state = world({ labels: [], comments: [comment()] }) // a PASS at the tip, but no agent-pr yet + const first = run(state) + assert.equal(first.applied.conclusion, 'failure') + assert.match(first.message, /missing label: agent-pr/) + assert.equal(state.runs.length, 1) + + // The label is added and nothing else changes: same head, no new commit. + state.labels = ['agent-pr'] + const second = run(state) + assert.equal(second.applied.conclusion, 'success') + assert.equal(second.applied.action, 'update') + // One run for this head, not two: the flip is the same run changing state. + assert.equal(state.runs.length, 1) + assert.equal(state.runs[0].conclusion, 'success') + assert.equal(state.writes.filter((w) => w.method === 'POST').length, 1) +}) + +test('#812: the check-run read takes the envelope, so one head is re-judged, never given a second run', () => { + const state = world() + const seen = [] + const spy = (...a) => { seen.push(Array.isArray(a[0]) ? a[0] : a.filter((x) => x !== undefined)); return state.gh(...a) } + const first = applyCheck({ gh: spy, repo: REPO, head: HEAD, decision: ALLOWED }) + const second = applyCheck({ gh: spy, repo: REPO, head: HEAD, decision: REFUSED }) + // With `.[]` on the envelope both calls would answer `create` and this head would carry two runs. + assert.equal(first.action, 'create') + assert.equal(second.action, 'update') + assert.ok(seen.some((args) => args.includes('.check_runs[]')), 'reads the array inside the envelope, not the envelope’s values') + assert.equal(state.runs.length, 1) + assert.deepEqual(state.writes.map((w) => w.method), ['POST', 'PATCH']) +}) + +test('#812: a push returns the check to red even with agent-pr present — a PASS is bound to a tip', () => { + const state = world({ comments: [comment()] }) + assert.equal(run(state).applied.conclusion, 'success') + state.head = NEXT // a new commit: the PASS above names the old head + const moved = run(state) + assert.equal(moved.applied.conclusion, 'failure') + assert.equal(moved.applied.action, 'create') + assert.match(moved.message, /names abaf938b0c1d, not the tip cd12ef340506/) + // Two heads, two runs: the old green stays as the record of the old head. + assert.equal(state.runs.length, 2) +}) + +test('#812: hold is the guard\u2019s rule, not the gate\u2019s own — a PASS does not clear it', () => { + const state = world({ labels: ['hold', 'agent-pr'], comments: [comment()] }) + const r = run(state) + assert.equal(r.applied.conclusion, 'failure') + assert.match(r.message, /`hold` label/) +}) + +test('#812: a comment arriving after a refusal PATCHes the same run green', () => { + const state = world({ comments: [] }) // agent-pr is there, the PASS is not yet + assert.equal(run(state).applied.conclusion, 'failure') + const before = state.runs[0].id + state.comments = [comment()] + const after = run(state) + assert.equal(after.applied.conclusion, 'success') + assert.equal(after.applied.id, before) + assert.equal(state.runs.length, 1) +}) + +test('#812: a T0/T1 head is green without a PASS, and the summary says why', () => { + const state = world({ body: 'Fixes #812\n', files: [TEST_FILE], comments: [] }) + const r = run(state) + assert.equal(r.applied.conclusion, 'success') + assert.equal(state.runs[0].conclusion, 'success') + assert.match(state.runs[0].output.summary, /T1|T0|no PASS needed/) +}) + +test('#812: a mechanical release pull request is green without an issue or agent-pr', () => { + const state = world({ body: '', labels: [], files: ['CHANGELOG.md', 'changelog.d/812-x.added.md', 'core/package.json'], comments: [] }) + const r = run(state) + assert.equal(r.applied.conclusion, 'success') + assert.match(state.runs[0].output.summary, /mechanical release/) +}) + +test('#812: a judged head always writes exactly one run; nothing else is written', () => { + const state = world() + run(state) + for (const write of state.writes) { + assert.ok(/\/check-runs(\/\d+)?$/.test(write.target), write.target) + assert.ok(write.method === 'POST' || write.method === 'PATCH', write.method) + } +}) + +test('#812: a plain-issue comment is not a pull request and leaves no check behind', () => { + const state = world() + const gh = (...a) => { + const args = Array.isArray(a[0]) ? a[0] : a + if (/\/pulls\/\d+$/.test(String(args[args.length - 1]))) throw new Error('gh: Not Found (HTTP 404)') + return '' + } + const r = main({ gh, env: { REVIEW_POLICY: POLICY, GH_REPO: REPO, PR: '77' }, repo: REPO, pr: 77 }) + assert.equal(r.skipped, true) + assert.equal(state.writes.length, 0) +}) + +// The gate is only in the path if the ruleset requires the context this script writes, and if the +// workflow that runs it can actually write a check. +test('#812: the ruleset requires the context this script writes, with no one able to bypass it', () => { + const ruleset = JSON.parse(readFileSync(new URL('../agent/merge-gate-ruleset.json', import.meta.url), 'utf8')) + const status = ruleset.rules.find((r) => r.type === 'required_status_checks').parameters + const contexts = status.required_status_checks.map((c) => c.context) + assert.ok(contexts.includes(CHECK_NAME), contexts.join(', ')) + assert.ok(contexts.includes('tests-ok'), contexts.join(', ')) + assert.equal(status.strict_required_status_checks_policy, false) + assert.deepEqual(ruleset.bypass_actors, []) + assert.equal(ruleset.enforcement, 'active') + assert.deepEqual(ruleset.conditions.ref_name.include, ['refs/heads/main']) +}) + +test('#812: the workflow can write the check and never runs pull request code', () => { + const workflow = readFileSync(new URL('../workflows/enqueue-guard.yml', import.meta.url), 'utf8') + assert.match(workflow, /^\s+checks: write$/m) + // `pull_request_target` for the write token, `issue_comment` for a verdict, a manual run for a backfill. + assert.match(workflow, /^\s+pull_request_target:$/m) + assert.match(workflow, /^\s+issue_comment:$/m) + assert.match(workflow, /^\s+workflow_dispatch:$/m) + // A `pull_request` run from a fork is read-only, so that trigger cannot be the one that writes the check. + assert.doesNotMatch(workflow, /^\s{2}pull_request:$/m) + // The code that judges comes from main; no head ref is ever checked out. + assert.match(workflow, /ref: main/) + assert.doesNotMatch(workflow, /github\.event\.pull_request\.head\.(sha|ref)/) +}) + +test('#812: --dry-run judges the live head and writes nothing', () => { + const state = world({ labels: [], comments: [comment()] }) + const r = main({ gh: state.gh, env: { REVIEW_POLICY: POLICY, GH_REPO: REPO, PR: '812', DRY_RUN: '1' }, repo: REPO, pr: 812 }) + assert.equal(r.applied.action, 'dry-run') + assert.equal(r.applied.conclusion, 'failure') + assert.match(r.applied && r.message, /missing label: agent-pr/) + assert.equal(state.writes.length, 0) + assert.equal(state.runs.length, 0) +}) diff --git a/.github/scripts/gate-ruleset.mjs b/.github/scripts/gate-ruleset.mjs new file mode 100644 index 00000000..8b903110 --- /dev/null +++ b/.github/scripts/gate-ruleset.mjs @@ -0,0 +1,149 @@ +// The ruleset that makes the check a gate (#812). +// +// The check run is only half of #812: `enqueue-guard.yml` writes it, and a ruleset on `main` has to +// require it, or the refusal is a red badge a merge endpoint never reads. `.github/agent/ +// merge-gate-ruleset.json` is that ruleset verbatim — branch ruleset, `enforcement: active`, +// `bypass_actors: []`, `refs/heads/main`, `tests-ok` and `enqueue-guard` required, +// `strict_required_status_checks_policy: false` (main moves on every merge, and a strict rule would make +// every open pull request rebase before it could merge). +// +// node .github/scripts/gate-ruleset.mjs --check # read-only: is main gated, and by what +// node .github/scripts/gate-ruleset.mjs --apply # create (or converge) it — an owner action +// node .github/scripts/gate-ruleset.mjs --rollback # delete it and prove it is gone +// +// Changing a repository's ruleset is not a Bot's call (AGENTS.md → Hard rules): `--apply` prints what it +// would do and needs the owner's word, which is why the merge is sequenced in the pull request and in +// #812 rather than performed here. `--check` writes nothing and is safe to run at any time. +// +// Order matters: the check must exist on the open pull requests *before* the ruleset does, or every one of +// them reads "Expected — waiting for status" and cannot merge (#639). `--apply` therefore reads the +// current state back and names any pull request that already carries the check, so the switch is a +// decision made with the evidence in front of it. The rollback is one DELETE by id: no state is kept +// anywhere but GitHub. +import { readFileSync } from 'node:fs' +import { execFileSync } from 'node:child_process' +import { fileURLToPath } from 'node:url' + +export const BODY_PATH = fileURLToPath(new URL('../agent/merge-gate-ruleset.json', import.meta.url)) +export const RULESET_NAME = 'main merge gate' +export const BASE_REF = 'refs/heads/main' +export const REQUIRED_CONTEXTS = ['tests-ok', 'enqueue-guard'] + +function arg(name) { + const i = process.argv.indexOf(name) + return i > 0 ? process.argv[i + 1] : undefined +} + +const realGh = (args, input) => execFileSync('gh', args, { encoding: 'utf8', maxBuffer: 64 << 20, input, stdio: ['pipe', 'pipe', 'pipe'] }) +const realGet = (path) => JSON.parse(realGh(['api', path])) + +export function loadBody(path = BODY_PATH) { + return JSON.parse(readFileSync(path, 'utf8')) +} + +// The rules the check depends on, read out of a ruleset the API returned. `current_user_can_bypass` only +// appears on the single-ruleset GET, which is why `--check` fetches each one instead of trusting the list. +export function summarize(row) { + const status = (row?.rules ?? []).find((r) => r?.type === 'required_status_checks') + const pull = (row?.rules ?? []).find((r) => r?.type === 'pull_request') + return { + id: row?.id ?? null, + name: row?.name ?? '', + target: row?.target ?? '', + enforcement: row?.enforcement ?? '', + refs: row?.conditions?.ref_name?.include ?? [], + bypass: (row?.bypass_actors ?? []).map((a) => `${a?.actor_type ?? '?'}:${a?.actor_id ?? '?'}`), + canBypass: row?.current_user_can_bypass ?? null, + strict: status?.parameters?.strict_required_status_checks_policy ?? null, + contexts: (status?.parameters?.required_status_checks ?? []).map((c) => c?.context).filter(Boolean), + approvals: pull?.parameters?.required_approving_review_count ?? null, + } +} + +// Everything that makes the gate real, as problems: an empty list is "main is gated". Each one is a +// separate line because they fail for different reasons and are fixed in different places. +export function problems(row) { + if (!row) return [`no ruleset named "${RULESET_NAME}" on ${BASE_REF}: main is unprotected`] + const s = summarize(row) + const out = [] + if (s.enforcement !== 'active') out.push(`the ruleset is "${s.enforcement || 'missing'}", not active`) + if (!s.refs.includes(BASE_REF)) out.push(`the ruleset does not include ${BASE_REF} (includes ${s.refs.join(', ') || 'nothing'})`) + if (s.bypass.length) out.push(`the ruleset has bypass actors (${s.bypass.join(', ')}): the check can be skipped`) + if (s.canBypass !== 'never') out.push(`current_user_can_bypass is "${s.canBypass ?? 'not reported'}", not "never"`) + if (s.strict !== false) out.push(`strict_required_status_checks_policy is ${JSON.stringify(s.strict)}, not false: every open pull request would have to rebase first`) + for (const context of REQUIRED_CONTEXTS) if (!s.contexts.includes(context)) out.push(`the required checks do not include "${context}" (they are ${s.contexts.join(', ') || 'none'})`) + return out +} + +// The body that will be sent: the file, unmodified. A ruleset body carries no comment fields, so the +// file is the request — everything explained lives here, not in JSON the API would reject. +export function bodyFor(path = BODY_PATH) { + return loadBody(path) +} + +export function findByRow(rows, name = RULESET_NAME) { + return (rows ?? []).find((row) => String(row?.name ?? '') === name) ?? null +} + +// `gh`/`get` are injectable so a test drives every mode without the network (`gh(args, input)`, the way +// controller.mjs calls `ghRaw`). +export function main({ mode = 'check', repo, gh = realGh, get = realGet, bodyPath = BODY_PATH, current = null } = {}) { + if (!repo) throw new Error('set GH_REPO or pass --repo owner/name') + if (mode === 'apply') { + const body = bodyFor(bodyPath) + const existing = findByRow(listRulesets(repo, get)) + const saved = existing + // PUT, not PATCH: the rulesets API answers 404 to a PATCH on a repository ruleset. + ? JSON.parse(gh(['api', '-X', 'PUT', `repos/${repo}/rulesets/${existing.id}`, '--input', '-'], JSON.stringify(body))) + : JSON.parse(gh(['api', '-X', 'POST', `repos/${repo}/rulesets`, '--input', '-'], JSON.stringify(body))) + const readBack = get(`repos/${repo}/rulesets/${saved.id}`) + const found = problems(readBack) + return { ok: found.length === 0, mode, action: existing ? 'update' : 'create', id: saved.id, summary: summarize(readBack), problems: found } + } + if (mode === 'rollback') { + const existing = findByRow(listRulesets(repo, get)) + if (!existing) return { ok: true, mode, action: 'none', problems: [] } + gh(['api', '-X', 'DELETE', `repos/${repo}/rulesets/${existing.id}`]) + const left = findByRow(listRulesets(repo, get)) + return { ok: !left, mode, action: 'delete', id: existing.id, problems: left ? [`the ruleset ${existing.id} is still listed after the delete`] : [] } + } + const rows = listRulesets(repo, get) + const hit = findByRow(rows) + if (!hit) return { ok: false, mode: 'check', problems: problems(null), summary: null } + const row = get(`repos/${repo}/rulesets/${hit.id}`) + const found = problems(row) + return { ok: found.length === 0, mode: 'check', id: hit.id, summary: summarize(row), problems: found } +} + +export function listRulesets(repo, get = realGet) { + return get(`repos/${repo}/rulesets`) +} + +export function render(result) { + const lines = [] + if (result.mode === 'apply') lines.push(`gate-ruleset: ${result.action}d "${RULESET_NAME}" (id ${result.id})`) + else if (result.mode === 'rollback') lines.push(result.action === 'delete' ? `gate-ruleset: deleted "${RULESET_NAME}" (id ${result.id})` : `gate-ruleset: no ruleset named "${RULESET_NAME}" to roll back`) + if (result.summary) { + const s = result.summary + lines.push(`gate-ruleset: ${s.enforcement}, ${s.target} ${s.refs.join(', ')}, bypass [${s.bypass.join(', ')}], can-bypass ${s.canBypass ?? 'not reported'}, strict ${JSON.stringify(s.strict)}, requires [${s.contexts.join(', ')}], approvals ${s.approvals ?? '—'}`) + } + if (result.problems?.length) { + lines.push(result.ok ? 'gate-ruleset: OK' : 'gate-ruleset: NOT GATED') + for (const problem of result.problems) lines.push(`gate-ruleset: ${problem}`) + } else { + lines.push('gate-ruleset: OK — main requires the checks and nobody can bypass them') + } + return lines.join('\n') +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + const mode = process.argv.includes('--apply') ? 'apply' : process.argv.includes('--rollback') ? 'rollback' : 'check' + try { + const result = main({ mode, repo: arg('--repo') || process.env.GH_REPO }) + console.log(render(result)) + process.exit(result.ok ? 0 : 1) + } catch (e) { + console.error(`gate-ruleset: ${e.message}`) + process.exit(1) + } +} diff --git a/.github/scripts/gate-ruleset.test.mjs b/.github/scripts/gate-ruleset.test.mjs new file mode 100644 index 00000000..26de36d1 --- /dev/null +++ b/.github/scripts/gate-ruleset.test.mjs @@ -0,0 +1,186 @@ +// gate-ruleset (#812): the ruleset that turns the `enqueue-guard` check into a gate, and the switches +// that would quietly turn it back into a report. +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { BODY_PATH, REQUIRED_CONTEXTS, RULESET_NAME, bodyFor, main, problems, render, summarize } from './gate-ruleset.mjs' + +const REPO = 'Birfy/FlyBot' + +// The ruleset the API returns for `.github/agent/merge-gate-ruleset.json` once it has been created, with +// the read-only fields GitHub adds on the single-ruleset GET. +const goodRow = () => { + const body = bodyFor() + return { id: 7, ...body, current_user_can_bypass: 'never', source_type: 'Repository', source: REPO } +} + +const fakeRepo = ({ rulesets = [], ignoreDelete = false } = {}) => { + const state = { + list: rulesets.map((r) => ({ id: r.id, name: r.name, target: r.target, enforcement: r.enforcement })), + details: new Map(rulesets.map((r) => [r.id, r])), + writes: [], + nextId: 50, + } + const get = (path) => { + if (/\/rulesets$/.test(path)) return state.list.map(({ id, name }) => ({ id, name })) + const hit = /\/rulesets\/(\d+)$/.exec(path) + if (hit) { + const row = state.details.get(Number(hit[1])) + if (!row) throw new Error('gh: Not Found (HTTP 404)') + return row + } + throw new Error('fake get: unexpected ' + path) + } + const gh = (args, input) => { + const path = String(args.find((a) => String(a).startsWith('repos/')) ?? args.at(-1)) + const method = args[args.indexOf('-X') + 1] + state.writes.push({ method, path, sent: input ? JSON.parse(input) : null }) + if (method === 'POST') { + const row = { id: state.nextId++, ...JSON.parse(input), current_user_can_bypass: 'never' } + state.list.push({ id: row.id, name: row.name, target: row.target, enforcement: row.enforcement }) + state.details.set(row.id, row) + return JSON.stringify(row) + } + if (method === 'PUT') { + const id = Number(path.split('/').at(-1)) + const row = { id, ...JSON.parse(input), current_user_can_bypass: 'never' } + state.details.set(id, row) + return JSON.stringify(row) + } + if (method === 'DELETE') { + const id = Number(path.split('/').at(-1)) + // `ignoreDelete` keeps the row listed the way a ruleset GitHub did not remove still would be: the + // point is the read after the DELETE, not the fake's bookkeeping. + if (!ignoreDelete) { + state.list = state.list.filter((r) => r.id !== id) + state.details.delete(id) + } + return '' + } + throw new Error('fake gh: unexpected write ' + method) + } + return { state, gh, get } +} + +test('#812: the ruleset requires both checks, on main, with nobody able to bypass it', () => { + const body = bodyFor(BODY_PATH) + assert.equal(body.name, RULESET_NAME) + assert.equal(body.target, 'branch') + assert.equal(body.enforcement, 'active') + assert.deepEqual(body.bypass_actors, []) + assert.deepEqual(body.conditions.ref_name.include, ['refs/heads/main']) + const status = body.rules.find((r) => r.type === 'required_status_checks').parameters + assert.deepEqual(status.required_status_checks.map((c) => c.context).sort(), [...REQUIRED_CONTEXTS].sort()) + assert.equal(status.strict_required_status_checks_policy, false) + // T2/T3 need no approve (owner decision 2026-10-22): the rule requires a pull request, not a signature. + const pull = body.rules.find((r) => r.type === 'pull_request').parameters + assert.equal(pull.required_approving_review_count, 0) + // Nobody force-pushes or deletes main to get around the check. + const types = body.rules.map((r) => r.type) + assert.ok(types.includes('non_fast_forward'), types.join(', ')) + assert.ok(types.includes('deletion'), types.join(', ')) +}) + +test('#812: a correctly built ruleset has no problems', () => { + assert.deepEqual(problems(goodRow()), []) + const s = summarize(goodRow()) + assert.deepEqual(s.contexts.sort(), [...REQUIRED_CONTEXTS].sort()) + assert.equal(s.canBypass, 'never') + assert.deepEqual(s.bypass, []) +}) + +test('#812: every way the gate could be bypassed is a separate problem', () => { + const cases = [ + ['a bypass actor', (row) => { row.bypass_actors = [{ actor_id: 1, actor_type: 'RepositoryRole', bypass_mode: 'always' }] }, /bypass actors/], + ['an admin escape hatch', (row) => { row.current_user_can_bypass = 'always' }, /current_user_can_bypass is "always"/], + ['a bypass nobody reported', (row) => { delete row.current_user_can_bypass }, /not reported/], + ['strict checks', (row) => { row.rules.find((r) => r.type === 'required_status_checks').parameters.strict_required_status_checks_policy = true }, /strict_required_status_checks_policy is true/], + ['a missing context', (row) => { row.rules.find((r) => r.type === 'required_status_checks').parameters.required_status_checks = [{ context: 'tests-ok' }] }, /do not include "enqueue-guard"/], + ['an inactive ruleset', (row) => { row.enforcement = 'evaluate' }, /not active/], + ['another ref', (row) => { row.conditions.ref_name.include = ['refs/heads/release'] }, /does not include refs\/heads\/main/], + ] + for (const [name, mutate, pattern] of cases) { + const row = goodRow() + mutate(row) + const found = problems(row) + assert.ok(found.some((p) => pattern.test(p)), `${name}: ${found.join(' | ')}`) + } + // A missing ruleset is the state #812 was filed about: no gate at all. + assert.match(problems(null)[0], /main is unprotected/) +}) + +test('#812: --check is read-only and says what is missing', () => { + const { state, gh, get } = fakeRepo() + const r = main({ mode: 'check', repo: REPO, gh, get }) + assert.equal(r.ok, false) + assert.match(r.problems[0], /main is unprotected/) + assert.equal(state.writes.length, 0) +}) + +test('#812: --check reads the detail, not the list, because only the detail reports who can bypass', () => { + const seen = [] + const get = (path) => { + seen.push(path) + if (/\/rulesets$/.test(path)) return [{ id: 7, name: RULESET_NAME }] + return goodRow() + } + const r = main({ mode: 'check', repo: REPO, get }) + assert.equal(r.ok, true, r.problems.join('; ')) + assert.ok(seen.includes(`repos/${REPO}/rulesets/7`), seen.join(', ')) + assert.equal(r.summary.canBypass, 'never') +}) + +test('#812: --apply posts the file itself and reads the result back', () => { + const { state, gh, get } = fakeRepo() + const r = main({ mode: 'apply', repo: REPO, gh, get }) + assert.equal(r.ok, true, r.problems.join('; ')) + assert.equal(r.action, 'create') + const post = state.writes.find((w) => w.method === 'POST') + assert.ok(post, 'no POST was made') + assert.deepEqual(post.sent, bodyFor()) + // Read back, not trusted: the summary is what GitHub now holds. + assert.deepEqual(r.summary.contexts.sort(), [...REQUIRED_CONTEXTS].sort()) +}) + +test('#812: --apply converges an existing ruleset with PUT, not PATCH', () => { + const { state, gh, get } = fakeRepo({ rulesets: [goodRow()] }) + const r = main({ mode: 'apply', repo: REPO, gh, get }) + assert.equal(r.ok, true, r.problems.join('; ')) + assert.equal(r.action, 'update') + const put = state.writes.find((w) => w.method === 'PUT') + assert.ok(put, 'no PUT was made') + assert.match(put.path, /\/rulesets\/7$/) + assert.deepEqual(put.sent, bodyFor()) +}) + +test('#812: --rollback deletes the ruleset and proves it is gone', () => { + const { state, gh, get } = fakeRepo({ rulesets: [goodRow()] }) + const r = main({ mode: 'rollback', repo: REPO, gh, get }) + assert.equal(r.ok, true, r.problems.join('; ')) + assert.equal(r.action, 'delete') + assert.equal(state.list.length, 0) + assert.equal(state.writes.filter((w) => w.method === 'DELETE').length, 1) +}) + +test('#812: --rollback reads the list again — a ruleset still listed after the delete is not gone', () => { + const { gh, get } = fakeRepo({ rulesets: [goodRow()], ignoreDelete: true }) + const r = main({ mode: 'rollback', repo: REPO, gh, get }) + assert.equal(r.ok, false, 'a ruleset still listed must not be reported as deleted') + assert.equal(r.action, 'delete') + assert.match(r.problems.join('; '), /still listed after the delete/) +}) + +test('#812: --rollback with nothing to roll back is not an error and writes nothing', () => { + const { state, gh, get } = fakeRepo() + const r = main({ mode: 'rollback', repo: REPO, gh, get }) + assert.equal(r.ok, true) + assert.equal(r.action, 'none') + assert.equal(state.writes.length, 0) +}) + +test('#812: the printed report names one problem per line, and nothing when it is gated', () => { + const bad = render({ ok: false, mode: 'check', problems: ['main is unprotected'], summary: null }) + assert.match(bad, /NOT GATED/) + assert.match(bad, /main is unprotected/) + const good = render({ ok: true, mode: 'check', problems: [], summary: summarize(goodRow()) }) + assert.match(good, /OK — main requires the checks and nobody can bypass them/) +}) diff --git a/.github/workflows/enqueue-guard.yml b/.github/workflows/enqueue-guard.yml new file mode 100644 index 00000000..43b058cd --- /dev/null +++ b/.github/workflows/enqueue-guard.yml @@ -0,0 +1,70 @@ +name: Enqueue guard + +# The required check on the code fork's `main` (#812). `enqueue-guard.mjs` was only a script an agent ran +# before merging; this writes its decision onto the pull request head as a check run named +# `enqueue-guard`, and the ruleset on `main` (`bypass_actors: []`) requires it — so an unreviewed pull +# request cannot be squash-merged through the API at all, instead of being refused only by a script +# somebody remembered to run. +# +# Why `pull_request_target` and not `pull_request`: a `pull_request` job from a fork gets a read-only +# `GITHUB_TOKEN` and could not write the check for the head it judges, while a required context that is +# never produced leaves that pull request on "Expected" forever (#639). `pull_request_target` runs with the +# write token this workflow declares, and it takes its code from `main` — `ref: main` below is the only +# checkout, no pull request code is ever fetched or run, and the head is read through the API +# (docs/autonomous-dev.md → Limits of GITHUB_TOKEN). The comment body a verdict arrives in is only parsed. +# +# The events are the ones that can change the answer for a fixed head: a push (a new head, whose old PASS +# no longer names it), a label added or removed (`agent-pr`, `hold`), a comment or a review body (a +# `Verdict:`), and a manual run. Each one re-judges the live head and PATCHes the same check run, so the +# check turns green without a new commit and back red when the tip moves. +on: + pull_request_target: + types: [opened, synchronize, reopened, labeled, unlabeled, ready_for_review] + pull_request_review: + types: [submitted, edited, dismissed] + issue_comment: + types: [created, edited, deleted] + workflow_dispatch: + inputs: + pr: + description: 'Pull request number to judge' + required: true + type: string + +# One judgement per pull request at a time: a label and a comment can arrive together, and two runs +# alternating the same check run is a state no one can read. +concurrency: + group: enqueue-guard-${{ github.event.pull_request.number || github.event.issue.number || github.run_id }} + cancel-in-progress: true + +# `checks: write` is what creates the required context — the same write token `pass-label.yml` already +# uses for labels on `issue_comment`. Everything else is read-only, and nothing here needs a secret. +permissions: + contents: read + issues: read + pull-requests: read + checks: write + +jobs: + judge: + # A comment on a plain issue is not a pull request; the manual run names the pull request itself. + name: judge + if: github.event_name != 'issue_comment' || github.event.issue.pull_request != null + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: main + persist-credentials: false + - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 + with: + node-version: 24 + # The judgement is `enqueue-guard.mjs`'s own; this only writes it. A refusal is a red check run, not + # a red job: the gate is the check, and a failing job would report the same refusal twice. + - name: Judge the head and write its enqueue-guard check run + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + PR: ${{ github.event.pull_request.number || github.event.issue.number || inputs.pr }} + run: node .github/scripts/gate-check.mjs diff --git a/AGENTS.md b/AGENTS.md index 28eb8da1..3aba326a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -146,6 +146,8 @@ What this buys is presence and a name from a closed list, not truth: with one sh - **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. +**On the code fork `Birfy/FlyBot`, where this team's pull requests actually merge, the gate is a ruleset, not a queue** (#812). Once the ruleset from `--apply` is on that repository, its `main` requires two checks, `tests-ok` and `enqueue-guard`, with `bypass_actors: []`, so `PUT /pulls//merge` is refused — 405/422 — while the head carries a `hold`, no PASS naming that exact tip, a PASS that lists no `Ran:`, no `agent-pr`, or a body naming no issue. The required check is written by [`enqueue-guard.yml`](.github/workflows/enqueue-guard.yml), from code taken from `main` and never from the pull request, on every event that can change the answer for a fixed head: a push, a label added or removed, a comment, a review body, or a manual run. It calls [`enqueue-guard.mjs`](.github/scripts/enqueue-guard.mjs)'s own decision — the same function that prints the command — so the check and the command cannot disagree; a new commit is a new head and a new check run, and a green check never carries over. The ruleset is [`merge-gate-ruleset.json`](.github/agent/merge-gate-ruleset.json): `node .github/scripts/gate-ruleset.mjs --check` reads the live state and writes nothing, while `--apply` and `--rollback` change repository protection and are the owner's call, not a Bot's. The order is not optional: the check must already report on the open pull requests **before** the ruleset goes on, or every one of them reads "Expected — waiting for status" and cannot merge. + 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. 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, and it is a required check of the `main merge queue` ruleset (#639, set 2026-10-22), so the refusal holds the entry. diff --git a/changelog.d/812-fork-merge-gate.added.md b/changelog.d/812-fork-merge-gate.added.md new file mode 100644 index 00000000..a65cf2f0 --- /dev/null +++ b/changelog.d/812-fork-merge-gate.added.md @@ -0,0 +1 @@ +Merging on the code fork is now refused by GitHub, not by a script somebody remembered to run: a new required check, `enqueue-guard`, is written onto every pull request head from the same decision `node .github/scripts/enqueue-guard.mjs --pr ` prints — a `hold`, a tip with no PASS naming it, a PASS that lists no `Ran:`, a missing `agent-pr` or a body that names no issue all turn it red, and it flips green without a new commit when the label or the conclusion arrives. `.github/agent/merge-gate-ruleset.json` is the ruleset that requires it next to `tests-ok` (branch `refs/heads/main`, `bypass_actors: []`, strict checks off so every open pull request does not have to rebase); `node .github/scripts/gate-ruleset.mjs --check` reports the live state and `--rollback` removes it ([#812](https://github.com/ThinkFlowLab/FlyBot/issues/812)). diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index 44fc6f63..b1c5d707 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -207,6 +207,39 @@ skipped the queue prints as `bypassed queue:` and fails the run while an older o not — the watermark lives in the Actions API and nothing is committed. Its first run passes `--baseline`, so the first green run is the baseline and only a newer bypass turns it red ([#697](https://github.com/ThinkFlowLab/FlyBot/issues/697)). +### The gate on the code fork: a ruleset and a written check (#812) + +The fork `Birfy/FlyBot` is where this team's pull requests are actually merged, and it had neither a ruleset nor branch +protection: `merge-gate.yml` reports on `pull_request` and never fails there, `pass-label` records a PASS and is not a +gate, and `enqueue-guard.mjs` only refused in a script somebody remembered to run. So a PASS was advisory, and #30 +merged without `agent-pr`. + +`.github/workflows/enqueue-guard.yml` writes the guard's own decision onto the pull request head as a check run named +`enqueue-guard`, and `.github/agent/merge-gate-ruleset.json` is the ruleset that requires it next to `tests-ok` on +`refs/heads/main`, with `bypass_actors: []`. Two properties make it a gate rather than a badge: + +- **One definition.** The script calls `decidePull` in [`enqueue-guard.mjs`](../.github/scripts/enqueue-guard.mjs) — the + same function behind the command the guard prints — so a rule added to the merge gate is enforced by the check without + being restated. Green means exactly "the printed command would say ALLOW". +- **It flips without a commit.** A verdict is a comment and a label is a label: neither starts a `pull_request` run for + the head being judged, which is why the check is written through the API instead of being a job's own result. + `external_id` is the head, so a re-judgement PATCHes that one run; a new commit is a new head and therefore a new run, + and a green check never carries over. + +`pull_request_target` is the trigger, not `pull_request`: a `pull_request` job from a fork is read-only and could not write +the check, while a required context that is never produced leaves that pull request on "Expected" forever (#639). The code +that judges is checked out from `main` (`ref: main`), no pull request code is fetched or run, and the only thing read from +the event is the number. + +**Turning it on is sequenced, and it is the owner's call.** `node .github/scripts/gate-ruleset.mjs --check` reads the live +state and writes nothing; `--apply` creates or converges the ruleset and `--rollback` deletes it and proves it is gone — +both change repository protection, which no Bot does on its own (AGENTS.md → Hard rules). The order is: merge the +workflow, prove the check goes red on a pull request that is missing `agent-pr` and green on one that is complete, and +only then apply the ruleset. Applied first, every open pull request reads "Expected — waiting for status" and none can +merge. `--apply` runs `--check`'s assertions on what GitHub read back (`bypass_actors`, `current_user_can_bypass: never`, +both contexts, `strict_required_status_checks_policy: false` so one merge on `main` does not force every open pull +request to rebase). + **The patrol that keeps the labels honest.** A status label says what is happening to an item *now*, so it must not outlive the item: on 2026-10-22 the board counted 11 closed issues and 43 closed pull requests as in flight, because nothing removed `agent-working`/`agent-pr` when they closed. `.github/workflows/label-hygiene.yml` runs diff --git a/docs/testing-layers.json b/docs/testing-layers.json index 5f2d775d..8ec026ba 100644 --- a/docs/testing-layers.json +++ b/docs/testing-layers.json @@ -126,6 +126,15 @@ "runs": "merge_group (reports on pull_request)", "role": "Re-reads `hold` and the newest conclusion at the entry's head, and fails the queue entry when either refuses it." }, + { + "id": "enqueue-guard", + "workflow": ".github/workflows/enqueue-guard.yml", + "kind": "gate", + "layer": "L2", + "gate": true, + "runs": "pull_request_target, pull_request_review, issue_comment, workflow_dispatch", + "role": "Writes the `enqueue-guard` check run a merge on the fork requires: the decision of `enqueue-guard.mjs`, on the live head, from code taken from `main`. The ruleset refuses a merge while it is red (#812)." + }, { "id": "docker", "workflow": ".github/workflows/docker.yml", diff --git a/docs/testing-layers.md b/docs/testing-layers.md index b61c50ff..fd0c98c1 100644 --- a/docs/testing-layers.md +++ b/docs/testing-layers.md @@ -91,6 +91,7 @@ The tables below are generated: edit `docs/testing-layers.json`, then run | --- | --- | --- | --- | --- | --- | | `test.yml` | gate | L2 | yes | push to main, pull_request, merge_group | Runs every L1 suite, the packaging and patch-pin checks, the strict typecheck, the changelog check and the workflow lint on `push`, `pull_request` and `merge_group`, and aggregates them as `tests-ok`, which `main` requires. | | `merge-gate.yml` | gate | L2 | yes | merge_group (reports on pull_request) | Re-reads `hold` and the newest conclusion at the entry's head, and fails the queue entry when either refuses it. | +| `enqueue-guard.yml` | gate | L2 | yes | pull_request_target, pull_request_review, issue_comment, workflow_dispatch | Writes the `enqueue-guard` check run a merge on the fork requires: the decision of `enqueue-guard.mjs`, on the live head, from code taken from `main`. The ruleset refuses a merge while it is red (#812). | | `docker.yml` | workflow-only | L2 | yes | pull_request that touches the image | Builds the container image and starts it, so a packaging change cannot merge without the image proving it starts. | | `live-e2e.yml` | workflow-only | L3 | no | daily on main | Drives the real end-to-end suite with a real model against `main`; it is the automation behind the self-test layer, and it never gates a merge. | | `review-gate.yml` | not-a-layer | none | no | — | Validates and posts the conclusion on a pull request. It gates nothing by itself: `merge-gate` re-judges the entry at merge time. |