Accept a writers Bot name on the Reviewer: line, and refuse one that cannot review

.github/scripts/enqueue-guard.mjs

@@ -64,8 +64,20 @@ const labelNames = (labels) => (labels ?? []).map((l) => String(typeof l === 'st
 
 // The refusal names the line, not just the defect, so the fix is "add this line" rather than "something
 // about the body". A `Reviewer:` line with no value names nobody, which is the same defect as a missing
-// line: the queue is meant to know who reviews, not to see the word. A value is one login, with or
-// without a leading `@`, compared case-insensitively against the policy's `reviewers`.
+// line: the queue is meant to know who reviews, not to see the word.
+//
+// #947: `Reviewer:` names a workstation, so it speaks the vocabulary of `Written-by:` — the policy's
+// `writers` Bot names, which is the only thing our pull requests ever wrote there — and the `reviewers`
+// logins stay accepted, because the shared account is a login too. Checking the login list alone refused
+// every pull request we open, #941 among them: `Reviewer: Reviewer2` came back "not in reviewers" while
+// `reviewers` holds accounts, not workstations. A value is one name from either list, with or without a
+// leading `@`, compared case-insensitively.
+//
+// Being listed is not the same as being able to review, and the line has to name at least one of the
+// second kind, or "a Bot name" becomes a way around the list. A login in `reviewers` can record a PASS.
+// A Bot name can only through the one identity every Bot writes as, `Birfy`, so it names a reviewer only
+// while that account is in `reviewers`; with it gone the name is vocabulary and the line is refused.
+const SHARED_WRITE_LOGIN = 'Birfy'
 function headerReasons(body, policy) {
   const lines = String(body ?? '').split(/\r?\n/).slice(0, HEADER_LINES)
   const reasons = []
@@ -75,8 +87,30 @@ function headerReasons(body, policy) {
   else {
     const names = reviewer.replace(REVIEWER_LINE, '').split(',').map((value) => value.trim().replace(/^@/, '').toLowerCase()).filter(Boolean)
     if (!names.length) reasons.push('missing line: Reviewer:')
-    const reviewers = new Set((policy?.reviewers ?? []).map((l) => String(l).toLowerCase()))
-    for (const name of names) if (!reviewers.has(name)) reasons.push(`Reviewer names "${name}", who is not in reviewers`)
+    const logins = new Set((policy?.reviewers ?? []).map((l) => String(l).toLowerCase()))
+    const bots = new Set((policy?.writers ?? []).map((w) => String(w).toLowerCase()))
+    const sharedLogin = logins.has(SHARED_WRITE_LOGIN.toLowerCase())
+    let passable = false
+    for (const name of names) {
+      if (logins.has(name)) {
+        passable = true
+        continue
+      }
+      if (bots.has(name)) {
+        if (sharedLogin) passable = true
+        continue
+      }
+      // Neither list: say both, so the reader knows which two vocabularies were tried rather than
+      // being sent to fix a spelling that was never the problem.
+      reasons.push(`Reviewer names "${name}", who is not a reviewers login`)
+      reasons.push(`Reviewer names "${name}", who is not a writers Bot name`)
+    }
+    // Every name is a known Bot name and none of them can pass: refused, because the line would
+    // otherwise read as reviewed.
+    if (names.length && !passable && !reasons.length) {
+      const named = names.map((name) => `"${name}"`).join(', ')
+      reasons.push(`Reviewer names ${named}, who ${names.length === 1 ? 'is a writers Bot name but cannot record a Verdict here' : 'are writers Bot names but cannot record a Verdict here'}: every Bot writes as "${SHARED_WRITE_LOGIN}", which is not in reviewers`)
+    }
   }
   return reasons
 }

.github/scripts/enqueue-guard.test.mjs

@@ -15,12 +15,15 @@ const TEST_FILE = 'core/test/enqueue-guard.test.js' // T1: needs none
 // #943: a pull request that may enter the queue says who owns it and who reviews it, in its body.
 const HEADERS = 'Owner: Dev\nReviewer: reviewer-agent\n'
 
-const policyFile = () => {
+const policyFile = (overrides = {}) => {
   const path = join(mkdtempSync(join(tmpdir(), 'enqueue-guard-policy-')), 'review-policy.json')
-  writeFileSync(path, JSON.stringify({ reviewers: ['reviewer-agent', 'Birfy'], selfReview: ['Birfy'], writers: ['reviewer-agent', 'Reviewer', 'Birfy'] }))
+  writeFileSync(path, JSON.stringify({ reviewers: ['reviewer-agent', 'Birfy'], selfReview: ['Birfy'], writers: ['reviewer-agent', 'Reviewer', 'Reviewer2', 'Birfy'], ...overrides }))
   return path
 }
 const POLICY = policyFile()
+// #947: `Birfy` is the one identity every Bot writes as, so it is the login a `writers` Bot name stands
+// for. This fixture takes it out of `reviewers`: a Bot name then names nobody who can record a PASS.
+const NO_SHARED_LOGIN = policyFile({ reviewers: ['reviewer-agent'] })
 
 // The fixed format (#637): the first line is the conclusion, the evidence below it. `Ran:` is what
 // makes a PASS evidence rather than a claim (review-gate's rule, reused here).
@@ -54,7 +57,7 @@ const fakeGh = ({ body = HEADERS + 'Fixes #602\n', comments = [], reviews = [],
 
 const run = (fixture = {}) => main({
   gh: fakeGh({ files: [HOST_FILE], ...fixture }),
-  env: { REVIEW_POLICY: POLICY, GH_REPO: fixture.repo ?? 'ThinkFlowLab/FlyBot', PR: '566' },
+  env: { REVIEW_POLICY: fixture.policy ?? POLICY, GH_REPO: fixture.repo ?? 'ThinkFlowLab/FlyBot', PR: '566' },
   repo: fixture.repo ?? 'ThinkFlowLab/FlyBot',
   pr: 566,
 })
@@ -349,17 +352,63 @@ test('#943: a body with no Reviewer: line is refused, and the refusal names that
   assert.equal(r.reasons.some((reason) => /missing line: Owner:/.test(reason)), false, 'only the line that is missing is named')
 })
 
-test('#943: a Reviewer: naming a login outside `reviewers` is refused, and the refusal names the login', () => {
+// #947 rewrote both refusal sentences: a value is now looked for in two vocabularies, so the refusal
+// says which one it missed instead of claiming `reviewers` is the only list.
+test('#943, reworded by #947: a Reviewer: outside both vocabularies is refused and names the word', () => {
   const r = run({ body: 'Owner: Dev\nReviewer: someone-else\nFixes #943\n', comments: [comment()] })
   assert.equal(r.ok, false)
-  assert.ok(r.reasons.includes('Reviewer names "someone-else", who is not in reviewers'), r.reasons.join('; '))
+  assert.ok(r.reasons.includes('Reviewer names "someone-else", who is not a reviewers login'), r.reasons.join('; '))
+  assert.ok(r.reasons.includes('Reviewer names "someone-else", who is not a writers Bot name'), r.reasons.join('; '))
 })
 
 test('#943: an @ and any case are fine on a login `reviewers` lists', () => {
   const r = run({ body: 'Owner: Dev\nReviewer: @Reviewer-Agent, BIRFY\nFixes #943\n', comments: [comment()] })
   assert.equal(r.ok, true, r.reasons.join('; '))
 })
 
+// #947: `Reviewer:` names a workstation, so it speaks the vocabulary of `Written-by:` — the policy's
+// `writers` Bot names, which is the only thing our pull requests ever wrote there. #943 compared the
+// value against `reviewers` (logins) alone, so `Reviewer: Reviewer2` was refused and #941 could not
+// enter the queue at all.
+test('#947: a Reviewer: naming a writers Bot name is accepted', () => {
+  const r = run({ body: 'Owner: Dev\nReviewer: Reviewer2\nFixes #947\n', comments: [comment()] })
+  assert.equal(r.ok, true, r.reasons.join('; '))
+})
+
+test('#947: either vocabulary is accepted, with an @ and in any case', () => {
+  for (const line of ['Reviewer: @Reviewer2', 'Reviewer: reviewer', 'Reviewer: Birfy', 'Reviewer: Reviewer2, Birfy', 'Reviewer: reviewer-agent, @REVIEWER2']) {
+    const r = run({ body: `Owner: Dev\n${line}\nFixes #947\n`, comments: [comment()] })
+    assert.equal(r.ok, true, `${line}: ${r.reasons.join('; ')}`)
+  }
+})
+
+test('#947: a name in neither vocabulary is refused, and the refusal says which two lists it missed', () => {
+  const r = run({ body: 'Owner: Dev\nReviewer: Nonexistent\nFixes #947\n', comments: [comment()] })
+  assert.equal(r.ok, false)
+  assert.ok(r.reasons.includes('Reviewer names "nonexistent", who is not a reviewers login'), r.reasons.join('; '))
+  assert.ok(r.reasons.includes('Reviewer names "nonexistent", who is not a writers Bot name'), r.reasons.join('; '))
+})
+
+// Chief's boundary on #947: accepting the Bot vocabulary must not become a way around the list of who
+// can record a PASS. A Bot name is the shared `Birfy` account, so it names a reviewer only while that
+// account is in `reviewers`; with it gone the name is vocabulary, not a reviewer.
+test('#947: a writers Bot name names nobody when the shared account is not a reviewer', () => {
+  const r = run({ body: 'Owner: Dev\nReviewer: Reviewer2\nFixes #947\n', comments: [comment()], policy: NO_SHARED_LOGIN })
+  assert.equal(r.ok, false)
+  assert.ok(r.reasons.some((reason) => /"reviewer2".*writers Bot name.*Birfy/.test(reason)), r.reasons.join('; '))
+  // The negative control: the same policy still accepts a real login, so the refusal is about the Bot
+  // name and not a fixture that refuses everything.
+  const login = run({ body: 'Owner: Dev\nReviewer: reviewer-agent\nFixes #947\n', comments: [comment()], policy: NO_SHARED_LOGIN })
+  assert.equal(login.ok, true, login.reasons.join('; '))
+})
+
+// One value has to be somebody who can pass, not every value: a Bot name beside a real login is still a
+// name the policy lists, and the line still names a reviewer who can record a Verdict.
+test('#947: a Bot name beside a real login is accepted even when it names nobody alone', () => {
+  const r = run({ body: 'Owner: Dev\nReviewer: Reviewer2, reviewer-agent\nFixes #947\n', comments: [comment()], policy: NO_SHARED_LOGIN })
+  assert.equal(r.ok, true, r.reasons.join('; '))
+})
+
 // #941's shape: the body starts with the agent's own HTML comment, so the lines sit on the 4th and 5th
 // line, not on the first two. Reading only the first two would refuse every agent pull request.
 test('#943: the lines are found in the first 20 lines, not only the first two', () => {

AGENTS.md

@@ -167,7 +167,7 @@ What this buys is presence and a name from a closed list, not truth: with one sh
 - **No implementer writes the Verdict.** On the API side self-review cannot be told apart mechanically — every pull request is opened by the shared `Birfy` account, and `selfReview: ["Birfy"]` in [`review-policy.json`](.github/agent/review-policy.json) makes a PASS always look like Birfy judging Birfy — so what the team holds is the role split: the implementer Bot does not write the Verdict, the Bot that writes the Verdict neither changes the tip nor re-judges its own verdict, and `selfReview` stays as it is because clearing it would leave every pull request without a conclusion.
 - **`hold` is human-only.** Only a person sets the label and only a person lifts it; automation refuses a held pull request and never removes the label itself. That is the one human input left on the merge path.
 - **The label awaiting-approval is retired.** A PASS at the tip goes straight to the queue through the enqueue guard (#687); no pull request waits for an approval click and the label is not set again.
-- **A pull request enters the queue with its issue and its `agent-pr` label.** On a branch this repository owns, the enqueue guard also refuses one whose body names no issue (`Fixes #<n>` / `Closes #<n>`) and one without the `agent-pr` label (#718), so "one issue, one pull request" is checked rather than remembered. It refuses a **live, non-draft** request whose body carries no `Owner:` line or no `Reviewer:` line in its first 20 lines, or names a `Reviewer:` who is not in `reviewers` — the refusal names the missing line or the login (#943; #940 merged in about seven minutes with the largest diff of the day and neither line). A **merged** pull request is never re-judged: this check also runs on every comment, and history is not a request. 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.
+- **A pull request enters the queue with its issue and its `agent-pr` label.** On a branch this repository owns, the enqueue guard also refuses one whose body names no issue (`Fixes #<n>` / `Closes #<n>`) and one without the `agent-pr` label (#718), so "one issue, one pull request" is checked rather than remembered. It refuses a **live, non-draft** request whose body carries no `Owner:` line or no `Reviewer:` line in its first 20 lines, or a `Reviewer:` that is neither a `reviewers` login nor a `writers` Bot name, or that names nobody who can record a PASS — a Bot name counts only through the one identity every Bot writes as, `Birfy`, so it names nobody while that account is outside `reviewers` (#947); the refusal names the missing line, or the word and the list it missed (#943, #947; #940 merged in about seven minutes with the largest diff of the day and neither line). A **merged** pull request is never re-judged: this check also runs on every comment, and history is not a request. 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/<n>/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.
 

docs/autonomous-dev.md

@@ -216,8 +216,9 @@ entry — are in [`AGENTS.md` → Merge gates](../AGENTS.md#merge-gates). This s
 queue`; that is not a failure. Run `node .github/scripts/enqueue-guard.mjs --pr <n>` first — it re-reads the live head and
 exits 1 on a `hold`, on a newest conclusion that is not a PASS, and on a report marked "record only — do not merge", so a
 PASS is never a queue permit on its own (#602); on a live, non-draft pull request of ours it also refuses a body with no
-`Owner:`/`Reviewer:` line in its first 20 lines (or a `Reviewer:` outside `reviewers`), naming the missing line or the
-login (#943) — a merged pull request is not re-judged, because this check also runs on every comment; on success it prints the command and never runs it. Enqueue with
+`Owner:`/`Reviewer:` line in its first 20 lines (or a `Reviewer:` that is neither a `reviewers` login nor a `writers`
+Bot name, or a Bot name whose shared `Birfy` identity is not in `reviewers`, #947), naming the missing line or the word
+that missed both lists (#943) — a merged pull request is not re-judged, because this check also runs on every comment; on success it prints the command and never runs it. Enqueue with
 `gh pr merge <n> --auto --squash --match-head-commit <sha>` (gh has no `--merge-queue` flag). gh may still print the
 strategy line, or `! Pull request #<n> is already queued to merge`; the PR is
 then in the queue (`gh api graphql` → `pullRequest.isInMergeQueue`). The queue, not the agent, performs the squash merge