gate: find main's ruleset by the ref it governs, and refuse a downgrade before writing (#2314)

.github/scripts/gate-ruleset.mjs

@@ -1,8 +1,13 @@
 // The ruleset that makes the check a gate (#812).
 //
+// A ruleset is identified by what it governs, never by its display name: the live one is called
+// `main merge queue` while this file is called `main merge gate`, and a name-based lookup read that as
+// "main is unprotected" (#2314). `.github/agent/merge-gate-ruleset.json` is the ruleset this file
+// converges to.
+//
 // 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`,
+// require it, or the refusal is a red badge a merge endpoint never reads. That file is the ruleset
+// verbatim — branch ruleset, `enforcement: active`,
 // `bypass_actors: []`, `refs/heads/main`, `tests-ok`, `enqueue-guard` and `policy-sign` 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).
@@ -62,8 +67,12 @@ export function summarize(row) {
 
 // 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.
+//
+// Drift is read in both directions: a context this file wants and the ruleset lacks, and a context the
+// ruleset requires and this file does not carry. The second is the downgrade `--apply` must refuse, and
+// on the live ruleset it is the only evidence of the drift a person can see without writing anything.
 export function problems(row) {
-  if (!row) return [`no ruleset named "${RULESET_NAME}" on ${BASE_REF}: main is unprotected`]
+  if (!row) return [`no active ruleset governs ${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`)
@@ -72,6 +81,7 @@ export function problems(row) {
   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'})`)
+  for (const context of s.contexts) if (!REQUIRED_CONTEXTS.includes(context)) out.push(`the ruleset also requires "${context}", which the body file does not carry: applying it would drop that context`)
   return out
 }
 
@@ -81,8 +91,29 @@ 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
+// The rulesets that govern `main`: a branch ruleset whose `conditions.ref_name.include` names
+// `refs/heads/main`. The list endpoint reports no `conditions`, so each row is read in full — which is
+// what `--check` needs anyway for `current_user_can_bypass`.
+export function findGoverning(repo, get = realGet) {
+  return (listRulesets(repo, get) ?? [])
+    .map((row) => (row?.id === undefined ? null : get(`repos/${repo}/rulesets/${row.id}`)))
+    .filter((row) => String(row?.target ?? '') === 'branch' && (row?.conditions?.ref_name?.include ?? []).includes(BASE_REF))
+}
+
+// What a body would remove from the ruleset it is applied to: every context the live ruleset requires
+// and the body does not carry, and the `merge_queue` rule. The body file in this repository lacks
+// `merge-gate`, the Node 24 context and the queue rule, so applying it to the live `main merge queue`
+// would weaken the gate it is supposed to describe.
+export function downgrade(live, body) {
+  const s = summarize(live)
+  const wanted = summarize({ rules: body?.rules ?? [] })
+  const out = []
+  for (const context of s.contexts) {
+    if (!wanted.contexts.includes(context)) out.push(`the body drops "${context}", which the live ruleset requires: refusing to apply a downgrade`)
+  }
+  const hasQueue = (rules) => (rules ?? []).some((rule) => rule?.type === 'merge_queue')
+  if (hasQueue(live?.rules) && !hasQueue(body?.rules)) out.push('the body drops the merge_queue rule, which the live ruleset requires: refusing to apply a downgrade')
+  return out
 }
 
 // `gh`/`get` are injectable so a test drives every mode without the network (`gh(args, input)`, the way
@@ -91,28 +122,31 @@ export function main({ mode = 'check', repo, gh = realGh, get = realGet, bodyPat
   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 [existing] = findGoverning(repo, get)
+    // Decided before the first request: a downgrade that is discovered after the PUT is already written.
+    const refused = existing ? downgrade(existing, body) : []
+    if (refused.length) {
+      return { ok: false, mode, action: 'refused', id: existing.id, name: existing.name ?? '', summary: summarize(existing), problems: refused }
+    }
     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 }
+    return { ok: found.length === 0, mode, action: existing ? 'update' : 'create', id: saved.id, name: readBack?.name ?? body.name ?? '', summary: summarize(readBack), problems: found }
   }
   if (mode === 'rollback') {
-    const existing = findByRow(listRulesets(repo, get))
+    const [existing] = findGoverning(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 left = findGoverning(repo, get)[0]
+    return { ok: !left, mode, action: 'delete', id: existing.id, name: existing.name ?? '', problems: left ? [`the ruleset ${left.id} is still listed after the delete`] : [] }
   }
-  const rows = listRulesets(repo, get)
-  const hit = findByRow(rows)
+  const [hit] = findGoverning(repo, get)
   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 }
+  const found = problems(hit)
+  return { ok: found.length === 0, mode: 'check', id: hit.id, summary: summarize(hit), problems: found }
 }
 
 export function listRulesets(repo, get = realGet) {
@@ -121,8 +155,14 @@ export function listRulesets(repo, get = realGet) {
 
 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`)
+  const label = result.name || result.summary?.name || RULESET_NAME
+  if (result.mode === 'apply') {
+    lines.push(result.action === 'refused'
+      ? `gate-ruleset: refused to write "${label}" (id ${result.id})`
+      : `gate-ruleset: ${result.action}d "${label}" (id ${result.id})`)
+  } else if (result.mode === 'rollback') {
+    lines.push(result.action === 'delete' ? `gate-ruleset: deleted "${label}" (id ${result.id})` : `gate-ruleset: nothing governs ${BASE_REF} 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 ?? '—'}`)

.github/scripts/gate-ruleset.test.mjs

@@ -2,6 +2,9 @@
 // that would quietly turn it back into a report.
 import { test } from 'node:test'
 import assert from 'node:assert/strict'
+import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
 import { BODY_PATH, REQUIRED_CONTEXTS, RULESET_NAME, bodyFor, main, problems, render, summarize } from './gate-ruleset.mjs'
 
 const REPO = 'Birfy/FlyBot'
@@ -193,3 +196,97 @@ test('#812: the printed report names one problem per line, and nothing when it i
   const good = render({ ok: true, mode: 'check', problems: [], summary: summarize(goodRow()) })
   assert.match(good, /OK — main requires the checks and nobody can bypass them/)
 })
+
+// #2314: the ruleset is identified by what it governs, not by its display name. On a11983c5 the only
+// active ruleset is `main merge queue` (id 25173372): it requires the Node 24 context, `tests-ok` and
+// `merge-gate`, and it carries the `merge_queue` rule. These are the two responses the issue quotes.
+const LIVE_ID = 25173372
+const liveRules = () => [
+  { type: 'deletion' },
+  { type: 'non_fast_forward' },
+  {
+    type: 'required_status_checks',
+    parameters: {
+      strict_required_status_checks_policy: false,
+      required_status_checks: [
+        { context: 'ubuntu-latest · Node 24 · DSH 0.2.0-rc.2' },
+        { context: 'tests-ok' },
+        { context: 'merge-gate' },
+      ],
+    },
+  },
+  {
+    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,
+    },
+  },
+  { type: 'merge_queue', parameters: { merge_method: 'SQUASH', max_entries_to_merge: 1 } },
+]
+const liveRow = (name = 'main merge queue') => ({
+  id: LIVE_ID, name, target: 'branch', enforcement: 'active',
+  bypass_actors: [], current_user_can_bypass: 'never',
+  conditions: { ref_name: { include: ['refs/heads/main'], exclude: [] } },
+  rules: liveRules(),
+})
+
+test('#2314: --check finds the gate by the ref it governs, not by its display name', () => {
+  const { state, gh, get } = fakeRepo({ rulesets: [liveRow()] })
+  const r = main({ mode: 'check', repo: REPO, gh, get })
+  assert.equal(state.writes.length, 0, 'check is still read-only')
+  assert.equal(r.id, LIVE_ID, 'the ruleset the check reads is the live one, named by its id')
+  const why = r.problems.join('\n')
+  assert.doesNotMatch(why, /unprotected/, 'main is gated under another name, not unprotected')
+  assert.match(why, /merge-gate/, 'a context the live gate requires and the file lacks is drift')
+  assert.match(why, /Node 24|ubuntu-latest/, 'the live Node 24 context is drift too')
+  assert.match(why, /enqueue-guard|policy-sign/, 'a context the file wants and the live gate lacks is drift as well')
+  assert.equal(r.ok, false, 'drift in either direction is not OK')
+})
+
+test('#2314: --apply updates the ruleset that governs main even under another name', (t) => {
+  // The body that converges the live ruleset: same rules, only the display name still differs.
+  const body = liveRow('main merge gate')
+  delete body.id
+  delete body.current_user_can_bypass
+  const dir = mkdtempSync(join(tmpdir(), 'gate-ruleset-body-'))
+  t.after(() => rmSync(dir, { recursive: true, force: true }))
+  const bodyPath = join(dir, 'body.json')
+  writeFileSync(bodyPath, JSON.stringify(body))
+  const { state, gh, get } = fakeRepo({ rulesets: [liveRow()] })
+  const r = main({ mode: 'apply', repo: REPO, gh, get, bodyPath })
+  assert.equal(r.action, 'update', 'a differently named ruleset on refs/heads/main is the one to update')
+  assert.equal(r.id, LIVE_ID)
+  assert.equal(state.writes.filter((w) => w.method === 'POST').length, 0, 'no second ruleset is created')
+  const put = state.writes.find((w) => w.method === 'PUT')
+  assert.ok(put, 'the live ruleset is updated in place')
+  assert.match(put.path, new RegExp(`/rulesets/${LIVE_ID}$`))
+})
+
+test('#2314: --apply refuses a body that would drop a live-required context or the merge queue', () => {
+  // The repository's own body file: no `merge-gate`, no Node 24 context, and no `merge_queue` rule.
+  const { state, gh, get } = fakeRepo({ rulesets: [liveRow()] })
+  let ghCalls = 0
+  const counting = (args, input) => { ghCalls += 1; return gh(args, input) }
+  const r = main({ mode: 'apply', repo: REPO, gh: counting, get })
+  assert.equal(r.ok, false, 'a downgrade is not a convergence')
+  const why = r.problems.join('\n')
+  assert.match(why, /merge-gate/, 'the live verdict gate it would drop is named')
+  assert.match(why, /Node 24|ubuntu-latest/, 'and the live Node 24 context')
+  assert.match(why, /merge_queue/, 'and the merge-queue rule it would remove')
+  assert.equal(ghCalls, 0, 'the refusal is decided before the first write request, not after one')
+  assert.equal(state.writes.length, 0, 'nothing is written when the body is a downgrade')
+})
+
+test('#2314: --rollback deletes the ruleset that governs main, whatever it is called', () => {
+  // Only the live `main merge queue` exists: a name-based lookup finds nothing, so the old code
+  // reported "nothing to roll back" and left the gate in place.
+  const { state, gh, get } = fakeRepo({ rulesets: [liveRow()] })
+  const r = main({ mode: 'rollback', repo: REPO, gh, get })
+  assert.equal(r.action, 'delete', 'a differently named ruleset that governs main is the one to roll back')
+  assert.equal(r.id, LIVE_ID)
+  const deletes = state.writes.filter((w) => w.method === 'DELETE')
+  assert.equal(deletes.length, 1)
+  assert.match(deletes[0].path, new RegExp(`/rulesets/${LIVE_ID}$`))
+  assert.equal(r.ok, true, r.problems.join('; '))
+})

.github/scripts/policy-sign.mjs

@@ -49,14 +49,42 @@ export function parseSignature(body = '') {
   return url === null ? value : url[1]
 }
 
+// A comment signs a tip when it names it. Twelve characters are enough: the comment quotes a sha a
+// person pasted, and the comparison is a prefix of the tip, the way the gate reads a verdict.
+function namesTip(comment, tip) {
+  const tokens = String(comment?.body ?? '').toLowerCase().match(/[0-9a-f]{12,40}/g) ?? []
+  return tokens.some(value => tip.startsWith(value))
+}
+
+// The three signability criteria, in one place: the refusal refuses with them, and lists the comments
+// that would satisfy them (#2314). A gate change needs another person, not the author and not a bot,
+// and the comment has to name the tip it signs.
+function canSign(comment, { author, tip }) {
+  if (tip === '') return false
+  const login = String(comment?.user?.login ?? '')
+  if (login !== '' && login === String(author)) return false
+  if (comment?.user?.type !== 'User') return false
+  return namesTip(comment, tip)
+}
+
+// At most three: a refusal is a remedy, not a list of every comment on the pull request.
+export function signable(comments = [], { author = '', tip = '', limit = 3 } = {}) {
+  return comments
+    .filter(comment => canSign(comment, { author, tip }))
+    .slice(0, limit)
+    .map(comment => ({ id: comment.id, login: String(comment.user?.login ?? '') }))
+}
+
 export function judge({ files = [], body = '', author = '', comments = [], head = '', policy = loadPolicy() }) {
   const touched = gateFiles(files, policy)
   if (touched.length === 0) return { ok: true, reason: 'no gate file changed, so no signature is needed' }
 
   const named = touched.length === 1 ? touched[0] : `${touched.length} gate files (${touched.join(', ')})`
+  const tip = String(head ?? '').trim().toLowerCase()
+  const candidates = signable(comments, { author, tip })
   const token = parseSignature(body)
   if (token === undefined) {
-    return { ok: false, reason: `a gate file changed (${named}), and the body carries no \`Policy-sign:\` line naming a comment` }
+    return { ok: false, reason: `a gate file changed (${named}), and the body carries no \`Policy-sign:\` line naming a comment`, signable: candidates }
   }
   const comment = comments.find(entry => String(entry?.id ?? '') === String(token))
   if (comment === undefined) {
@@ -71,13 +99,11 @@ export function judge({ files = [], body = '', author = '', comments = [], head
   }
   // The comment has to name the tip it signs. A comment reviewed one change set; without this, the
   // author can sign a harmless tip and then push the change the signature was supposed to cover.
-  const tip = String(head ?? '').trim().toLowerCase()
   if (tip === '') {
     return { ok: false, reason: `\`Policy-sign:\` names comment ${token}, but this run carries no head sha to bind it to — the check fails closed` }
   }
-  const tokens = String(comment.body ?? '').toLowerCase().match(/[0-9a-f]{12,40}/g) ?? []
-  if (!tokens.some(value => tip.startsWith(value))) {
-    return { ok: false, reason: `\`Policy-sign:\` names comment ${token}, which does not name this tip ${String(head).trim()} — a signature binds the tip it was written for, so a new tip needs a new signature` }
+  if (!namesTip(comment, tip)) {
+    return { ok: false, reason: `\`Policy-sign:\` names comment ${token}, which does not name this tip ${String(head).trim()} — a signature binds the tip it was written for, so a new tip needs a new signature`, signable: candidates }
   }
   return { ok: true, reason: `signed by ${login} in comment ${token}`, signer: login }
 }
@@ -149,6 +175,9 @@ export async function main(argv = process.argv.slice(2), env = process.env) {
     return 0
   }
   process.stdout.write(`policy-sign refused (#${number}): ${verdict.reason}\n`)
+  for (const candidate of verdict.signable ?? []) {
+    process.stdout.write(`policy-sign: comment ${candidate.id} by ${candidate.login} signs this tip — add \`Policy-sign: ${candidate.id}\` to the body\n`)
+  }
   process.stdout.write(`::error::${verdict.reason}\n`)
   return 1
 }