Reject a pull request that names one Bot as owner and reviewer

.github/scripts/enqueue-guard.mjs

@@ -78,18 +78,32 @@ const labelNames = (labels) => (labels ?? []).map((l) => String(typeof l === 'st
 // 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'
+// One line's names: split on commas, strip a leading `@`, compare case-insensitively. Both header lines
+// read their values this way so `@Dev`, `dev` and `DEV` are one name wherever they appear (#951).
+const headerNames = (line, pattern) => line.replace(pattern, '').split(',').map((value) => value.trim().replace(/^@/, '').toLowerCase()).filter(Boolean)
 function headerReasons(body, policy) {
   const lines = String(body ?? '').split(/\r?\n/).slice(0, HEADER_LINES)
   const reasons = []
-  if (!lines.some((line) => OWNER_LINE.test(line))) reasons.push('missing line: Owner:')
+  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())
+  // A line with no value names nobody, which is the same defect as a missing line — for either header.
+  // The owner speaks the same `writers` vocabulary: a name the policy does not know makes the line a
+  // claim with no name behind it, and an unknown name is also how "Owner: Somebody" cleared the
+  // duplicate check (#951 INC).
+  const owner = lines.find((line) => OWNER_LINE.test(line))
+  const ownerNames = owner ? [...new Set(headerNames(owner, OWNER_LINE))] : []
+  if (!ownerNames.length) reasons.push('missing line: Owner:')
+  for (const name of ownerNames) {
+    if (!bots.has(name)) reasons.push(`Owner names "${name}", who is not a writers Bot name`)
+  }
+  const owners = new Set(ownerNames)
   const reviewer = lines.find((line) => REVIEWER_LINE.test(line))
   if (!reviewer) reasons.push('missing line: Reviewer:')
   else {
-    const names = reviewer.replace(REVIEWER_LINE, '').split(',').map((value) => value.trim().replace(/^@/, '').toLowerCase()).filter(Boolean)
+    // One name twice is one name: `Reviewer: Dev, Dev` must not print the same refusal twice.
+    const names = [...new Set(headerNames(reviewer, REVIEWER_LINE))]
     if (!names.length) reasons.push('missing line: Reviewer:')
-    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)) {
@@ -105,6 +119,12 @@ function headerReasons(body, policy) {
       reasons.push(`Reviewer names "${name}", who is not a reviewers login`)
       reasons.push(`Reviewer names "${name}", who is not a writers Bot name`)
     }
+    // #951: the two lines are two roles. Reading them separately let one Bot own and review the same pull
+    // request — `Reviewer: Dev` passed on `Dev` being a `writers` name — which is the shape #940 had with
+    // no lines at all. The comparison is per name, so a list that overlaps in either direction refuses.
+    for (const name of names) {
+      if (owners.has(name)) reasons.push(`Reviewer names "${name}", who is also the Owner of this pull request`)
+    }
     // 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) {

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

@@ -17,7 +17,7 @@ const HEADERS = 'Owner: Dev\nReviewer: reviewer-agent\n'
 
 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', 'Reviewer2', 'Birfy'], ...overrides }))
+  writeFileSync(path, JSON.stringify({ reviewers: ['reviewer-agent', 'Birfy'], selfReview: ['Birfy'], writers: ['reviewer-agent', 'Reviewer', 'Reviewer2', 'Birfy', 'Dev', 'Dev2', 'Dev3'], ...overrides }))
   return path
 }
 const POLICY = policyFile()
@@ -470,3 +470,107 @@ test('#943 INC: a merged pull request is not re-judged when a comment arrives',
   assert.ok(live.reasons.includes('missing line: Owner:'), live.reasons.join('; '))
   assert.ok(live.reasons.includes('missing line: Reviewer:'), live.reasons.join('; '))
 })
+
+// #951: `Owner:` and `Reviewer:` were read as two independent lines (#943/#950), so a body could name the
+// same Bot in both and pass: `Reviewer: Dev` is accepted because `Dev` is a `writers` Bot name (:99-101),
+// and nothing compared the values (:84). One Bot owning and reviewing one pull request is the shape #940
+// had with no lines at all — the check exists so the queue knows who reviews, not so a word appears twice.
+// The comparison is the same normalisation the reviewer side already uses: split on commas, strip a
+// leading `@`, lowercase, then compare name by name, so a list that overlaps in either direction refuses.
+const selfReviewRefusals = (r) => r.reasons.filter((reason) => /who is also the Owner of this pull request/.test(reason))
+
+test('#951: Owner and Reviewer naming the same Bot is refused, and the reason says it is the Owner', () => {
+  const r = run({ body: 'Owner: Dev\nReviewer: Dev\nFixes #951\n', comments: [comment()] })
+  assert.equal(r.ok, false)
+  assert.deepEqual(selfReviewRefusals(r), ['Reviewer names "dev", who is also the Owner of this pull request'])
+})
+
+test('#951: a reviewer that is not the owner is accepted (the check compares the two lines)', () => {
+  const r = run({ body: 'Owner: Dev\nReviewer: Reviewer\nFixes #951\n', comments: [comment()] })
+  assert.equal(r.ok, true, r.reasons.join('; '))
+  assert.deepEqual(selfReviewRefusals(r), [])
+})
+
+test('#951: overlap in either direction, in a list, or through @ and case is refused', () => {
+  for (const header of [
+    'Owner: Dev, Dev2\nReviewer: Dev2\n',
+    'Owner: Dev\nReviewer: Dev, Reviewer\n',
+    'Owner: @Dev\nReviewer: dev\n',
+    'Owner: DEV\nReviewer: @dev\n',
+  ]) {
+    const r = run({ body: header + 'Fixes #951\n', comments: [comment()] })
+    assert.equal(r.ok, false, `${header}: ${r.reasons.join('; ')}`)
+    assert.equal(selfReviewRefusals(r).length, 1, `${header}: ${r.reasons.join('; ')}`)
+  }
+  // A list that shares no name with the owner still passes, so the comparison is per name and not per line.
+  const ok = run({ body: 'Owner: Dev\nReviewer: Reviewer2, Reviewer\nFixes #951\n', comments: [comment()] })
+  assert.equal(ok.ok, true, ok.reasons.join('; '))
+})
+
+// `Birfy` is both a `reviewers` login and the one identity every Bot writes as, so it is the sharpest
+// fixture: each line alone is perfectly valid, and only the comparison can refuse it.
+test('#951: a name that is both a login and a writers name is refused on both lines', () => {
+  const r = run({ body: 'Owner: Birfy\nReviewer: Birfy\nFixes #951\n', comments: [comment()] })
+  assert.equal(r.ok, false)
+  assert.deepEqual(selfReviewRefusals(r), ['Reviewer names "birfy", who is also the Owner of this pull request'])
+})
+
+// #943's messages and #950's vocabularies are the same check, not rewritten by this one.
+test('#951: the missing-line and vocabulary refusals are unchanged', () => {
+  const missing = run({ body: 'Fixes #951\n', comments: [comment()] })
+  assert.ok(missing.reasons.includes('missing line: Owner:'), missing.reasons.join('; '))
+  assert.ok(missing.reasons.includes('missing line: Reviewer:'), missing.reasons.join('; '))
+  const empty = run({ body: 'Owner: Dev\nReviewer:\nFixes #951\n', comments: [comment()] })
+  assert.ok(empty.reasons.includes('missing line: Reviewer:'), empty.reasons.join('; '))
+  const unknown = run({ body: 'Owner: Dev\nReviewer: Nonexistent\nFixes #951\n', comments: [comment()] })
+  assert.ok(unknown.reasons.includes('Reviewer names "nonexistent", who is not a reviewers login'), unknown.reasons.join('; '))
+  assert.ok(unknown.reasons.includes('Reviewer names "nonexistent", who is not a writers Bot name'), unknown.reasons.join('; '))
+  assert.deepEqual(selfReviewRefusals(unknown), [])
+})
+
+// History is not a request: this check runs on every comment, so a merged pull request that names one
+// Bot twice must stay exactly as it is (:150, #943 INC at 87b49961).
+test('#951: a merged pull request naming one Bot twice is not re-judged', () => {
+  const merged = run({ body: 'Owner: Dev\nReviewer: Dev\nFixes #951\n', merged: true, comments: [comment()] })
+  assert.equal(merged.ok, true, merged.reasons.join('; '))
+  assert.deepEqual(selfReviewRefusals(merged), [])
+})
+
+// #951 INC: refusing only an overlap can be switched off by leaving `Owner:` empty — an empty set has
+// nothing to overlap with, so `Owner:` + `Reviewer: Dev` was `ok: true`. The line names nobody, which is
+// the same defect as a missing line, and the vocabulary is the same `writers` list: an unknown owner name
+// made the duplicate check unpassable to reason about ("Owner: Somebody" cleared it too).
+test('#951 INC: an Owner: line with no value is the missing line, not an empty overlap', () => {
+  for (const body of ['Owner:\nReviewer: Dev\nFixes #951\n', 'Owner:   \nReviewer: Dev\nFixes #951\n', 'Owner: @\nReviewer: Dev\nFixes #951\n']) {
+    const r = run({ body, comments: [comment()] })
+    assert.equal(r.ok, false, body)
+    assert.ok(r.reasons.includes('missing line: Owner:'), `${body}: ${r.reasons.join('; ')}`)
+    assert.equal(r.reasons.some((reason) => /who is also the Owner/.test(reason)), false, `${body}: ${r.reasons.join('; ')}`)
+  }
+})
+
+test('#951 INC: an Owner: name outside the writers vocabulary is refused', () => {
+  const r = run({ body: 'Owner: Somebody\nReviewer: Dev\nFixes #951\n', comments: [comment()] })
+  assert.equal(r.ok, false)
+  assert.ok(r.reasons.includes('Owner names "somebody", who is not a writers Bot name'), r.reasons.join('; '))
+  // The negative control: a name the policy lists as a writers Bot is accepted on the same line.
+  const ok = run({ body: 'Owner: Dev3\nReviewer: Reviewer\nFixes #951\n', comments: [comment()] })
+  assert.equal(ok.ok, true, ok.reasons.join('; '))
+})
+
+// Two different Bots are the normal shape: the rule is "not the same Bot", not "the owner may not be a
+// writer" — `Dev` reviewing `Dev3`'s pull request is exactly what the pipeline does every day.
+test('#951 INC: a different Bot as reviewer is accepted, writers name or login', () => {
+  for (const line of ['Reviewer: Dev', 'Reviewer: Reviewer', 'Reviewer: reviewer-agent', 'Reviewer: Reviewer2, Dev']) {
+    const r = run({ body: `Owner: Dev3\n${line}\nFixes #951\n`, comments: [comment()] })
+    assert.equal(r.ok, true, `${line}: ${r.reasons.join('; ')}`)
+  }
+})
+
+test('#951 INC: Reviewer: Dev, Dev is one name and prints one refusal', () => {
+  const r = run({ body: 'Owner: Dev3\nReviewer: Dev, Dev\nFixes #951\n', comments: [comment()] })
+  assert.equal(r.ok, true, r.reasons.join('; '))
+  const same = run({ body: 'Owner: Dev\nReviewer: Dev, Dev\nFixes #951\n', comments: [comment()] })
+  assert.equal(same.ok, false)
+  assert.deepEqual(selfReviewRefusals(same), ['Reviewer names "dev", who is also the Owner of this pull request'])
+})

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

@@ -18,7 +18,7 @@ const HEADERS = 'Owner: Dev\nReviewer: reviewer-agent\n'
 
 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'] }))
+  writeFileSync(path, JSON.stringify({ reviewers: ['reviewer-agent', 'Birfy'], selfReview: ['Birfy'], writers: ['reviewer-agent', 'Reviewer', 'Birfy', 'Dev'] }))
   return path
 }
 const POLICY = policyFile()

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 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.
+- **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); a `Reviewer:` that names the same Bot as the `Owner:` is refused too, compared name by name after commas, a leading `@` and case are normalised, because one Bot owning and reviewing the same pull request is the shape #940 had with no lines at all (#951); both lines also need a value the policy knows — a `Reviewer:` outside both vocabularies and an `Owner:` outside `writers` are refused, and a line with no value is the missing line, so an empty `Owner:` cannot clear the comparison (#951); the refusal names the missing line, or the word and the list it missed (#943, #947, #951; #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

@@ -217,7 +217,10 @@ queue`; that is not a failure. Run `node .github/scripts/enqueue-guard.mjs --pr
 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:` 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
+Bot name, or a Bot name whose shared `Birfy` identity is not in `reviewers`, #947, or a `Reviewer:` naming the same Bot as
+the `Owner:`, compared per name after commas, `@` and case, #951; both lines need a value the policy knows — an `Owner:`
+outside `writers` is refused and a line with no value is the missing line, so an empty `Owner:` cannot clear the
+comparison, #951), 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