fix(pass-label): tell a reviewer whose PASS the policy does not count

.github/scripts/pass-label.mjs

@@ -14,13 +14,23 @@
 //     `parseReport` would happily find the `Verdict:` it quotes;
 //   - the label is only ever added. A later INC or a moved tip does not remove it: it records that a
 //     PASS was given, and `hold` stays the one manual stop.
+//
+// And one rule that keeps the refusal from being silent (#767): a PASS whose `Written-by:` names someone
+// the policy does not list is not a conclusion — `review-gate` and `merge-gate` both refuse it — but the
+// author who wrote it is told nothing, so the pull request simply never reaches the queue. This job knows
+// that answer (it is the same `writers` check the gates make) and throws it away, so it now leaves ONE
+// comment naming the author, the tip and the file to fix, and adds no label. The comment carries a marker
+// per tip, so a second run adds nothing and a moved tip may speak once about the new one.
 import { execFileSync } from 'node:child_process'
 import { readFileSync } from 'node:fs'
 import { fileURLToPath } from 'node:url'
 import { parseReport, loadPolicy } from './review-gate.mjs'
 
 export const PASS_LABEL = 'pass'
 export const ASSOCIATIONS = new Set(['OWNER', 'MEMBER', 'COLLABORATOR'])
+// The marker that makes the note idempotent: one per tip, so a second run on the same tip adds nothing.
+export const STRANGER_MARK = '<!-- pass-label: writer-not-listed -->'
+const STRANGER_NOTE = /<!-- pass-label: writer-not-listed -->\s+tip:([0-9a-f]{7,40})/i
 const POLICY = '.github/agent/review-policy.json'
 const SHORT = 12
 
@@ -52,6 +62,57 @@ export function planComment({ pr, body, association, writers, labels = [] }) {
   return { add: true, pr, tip: report.tip }
 }
 
+// The gates' refusal seen from the other side: a PASS at a tip whose `Written-by:` names someone the
+// policy does not list. `verdictOf` answers `null` for it — that is right for a label and useless for the
+// author — so this keeps the stranger's PASS as a value of its own. A missing `Written-by:` line is not
+// this case: nothing names a Bot to tell, and the gates' message already covers it.
+export function strangerPassOf(body, { writers = [], association } = {}) {
+  if (!ASSOCIATIONS.has(String(association ?? '').toUpperCase())) return null
+  if (/^Correction\b/i.test(firstLine(body))) return null
+  const report = parseReport(body)
+  if (report.verdict !== 'PASS' || !report.tip || !report.writer) return null
+  const listed = new Set(writers.map((w) => String(w).toLowerCase()))
+  if (listed.has(String(report.writer).toLowerCase())) return null
+  return { writer: report.writer, tip: report.tip }
+}
+
+// The note for one stranger's PASS: who wrote it, which tip it names, and the one edit that makes the
+// next one count. It never claims the pull request is unmergeable — a listed writer's PASS still is.
+export function strangerCommentBody({ writer, tip }) {
+  return `${STRANGER_MARK} tip:${tip}
+A PASS at \`${tip}\` was posted with \`Written-by: ${writer}\`, and **${writer} is not in the \`writers\` list** of \`.github/agent/review-policy.json\`, so this conclusion does not count: \`review-gate\` and \`merge-gate\` both refuse it, and a T2/T3 pull request waits for a PASS that cannot arrive while every other check is green.
+
+What makes the next one count: add the name to \`writers\` in \`.github/agent/review-policy.json\`. That file is T3 — the edit goes with the roster test and the owner's approval, not into the reviewing pull request.
+
+This note is left once per tip by the \`pass\` label job, which only records verdicts: it adds no label here and never turns this pull request red.
+`
+}
+
+// One note per tip, and no note at all unless this really is a stranger's PASS.
+export function planStrangerComment({ pr, body, association, writers, comments = [] }) {
+  const stranger = strangerPassOf(body, { writers, association })
+  if (!stranger) return { add: false, reason: 'not a PASS from a writer the policy does not list' }
+  // The same commit may be named by a 12-character tip in one run and by 40 in the next: compare the
+  // tips the way `namesHead` compares a report's tip with a head, not as whole strings.
+  const noted = comments.some((comment) => {
+    const match = STRANGER_NOTE.exec(String(comment?.body ?? comment ?? ''))
+    return match !== null && namesHead(match[1], stranger.tip)
+  })
+  if (noted) return { add: false, reason: `already noted at ${stranger.tip}` }
+  return { add: true, pr, tip: stranger.tip, writer: stranger.writer, body: strangerCommentBody(stranger) }
+}
+
+// What one `--comment` run does, before any write: the label, else the note, else nothing. `comments`
+// is a thunk so the thread is read only when a stranger's PASS actually needs it.
+export function planCommentRun({ pr, body, association, writers, labels = [], comments = () => [] }) {
+  const label = planComment({ pr, body, association, writers, labels })
+  if (label.add) return { write: 'label', pr, tip: label.tip }
+  if (!strangerPassOf(body, { writers, association })) return { write: 'none', reason: label.reason }
+  const note = planStrangerComment({ pr, body, association, writers, comments: comments() })
+  if (note.add) return { write: 'comment', pr, tip: note.tip, writer: note.writer, body: note.body }
+  return { write: 'none', reason: `${label.reason}; ${note.reason}` }
+}
+
 // Merged pull requests whose history carries a PASS at the head that merged, and which do not have the
 // label yet. `pages` is the raw API data so the whole decision is testable without the network.
 export function planBackfill({ pulls, historyOf, writers }) {
@@ -95,12 +156,26 @@ export function main({ gh = realGh, env = process.env } = {}) {
   if (!repo) throw new Error('set GH_REPO')
   const writers = loadPolicy(POLICY).writers ?? []
   const add = (pr) => gh('api', '-X', 'POST', `repos/${repo}/issues/${pr}/labels`, '-f', `labels[]=${PASS_LABEL}`)
+  const comment = (pr, text) => gh('api', '-X', 'POST', `repos/${repo}/issues/${pr}/comments`, '-f', `body=${text}`)
 
   if (process.argv.includes('--comment')) {
     const pr = Number(env.PR)
     if (!pr) throw new Error('set PR')
-    const plan = planComment({ pr, body: env.COMMENT_BODY ?? '', association: env.ASSOCIATION, writers })
-    if (!plan.add) {
+    const plan = planCommentRun({
+      pr,
+      body: env.COMMENT_BODY ?? '',
+      association: env.ASSOCIATION,
+      writers,
+      comments: () => list(gh, `repos/${repo}/issues/${pr}/comments?per_page=100`),
+    })
+    if (plan.write === 'comment') {
+      // The one refusal worth a comment (#767): the gates refuse a stranger's PASS too, and until now
+      // only a log line said so. It is left once per tip, and no label is ever added for it.
+      comment(pr, plan.body)
+      console.log(`pass-label: #${pr} told that ${plan.writer} is not in writers (PASS at ${plan.tip})`)
+      return { ok: true, added: [], noted: [pr] }
+    }
+    if (plan.write === 'none') {
       console.log(`pass-label: #${pr} not labelled — ${plan.reason}`)
       return { ok: true, added: [] }
     }

.github/scripts/pass-label.test.mjs

@@ -5,7 +5,7 @@ import { test } from 'node:test'
 import assert from 'node:assert/strict'
 import { readFileSync } from 'node:fs'
 import { join } from 'node:path'
-import { PASS_LABEL, namesHead, planBackfill, planComment, verdictOf, historyFrom } from './pass-label.mjs'
+import { PASS_LABEL, STRANGER_MARK, historyFrom, namesHead, planBackfill, planComment, planCommentRun, planStrangerComment, verdictOf } from './pass-label.mjs'
 import { loadPolicy } from './review-gate.mjs'
 
 const repo = join(import.meta.dirname, '..', '..')
@@ -89,3 +89,75 @@ test('#739: a 12-character tip names the head, a different one does not', () =>
   assert.equal(namesHead('aaaaaaaaaaaa', head), false)
   assert.equal(namesHead(null, head), false)
 })
+
+// #767: the gates refuse a PASS whose `Written-by:` names someone the policy does not list, and the one
+// place that already knew the answer said it in a log line. Now the author is told, once per tip.
+const stranger = (writer = 'Reviewer3') => conclusion('PASS', head, writer)
+
+test('#767: a PASS from someone outside writers leaves one note naming the author, the tip and the file', () => {
+  const plan = planStrangerComment({ pr: 7, body: stranger(), association: 'MEMBER', writers })
+  assert.equal(plan.add, true)
+  assert.equal(plan.writer, 'Reviewer3')
+  assert.equal(plan.tip, head)
+  assert.ok(plan.body.startsWith(`${STRANGER_MARK} tip:${head}`), 'the marker carries the tip')
+  assert.match(plan.body, /Written-by: Reviewer3/)
+  assert.match(plan.body, /\.github\/agent\/review-policy\.json/, 'names the file to edit')
+  assert.match(plan.body, /`writers`/, 'names the key to edit')
+  // The label path is unchanged: no label for this comment, ever.
+  assert.equal(planComment({ pr: 7, body: stranger(), association: 'MEMBER', writers }).add, false)
+})
+
+test('#767: the note is idempotent per tip, and a moved tip may speak once', () => {
+  const first = planStrangerComment({ pr: 7, body: stranger(), association: 'MEMBER', writers })
+  const again = planStrangerComment({ pr: 7, body: stranger(), association: 'MEMBER', writers, comments: [{ body: first.body }] })
+  assert.equal(again.add, false)
+  assert.match(again.reason, /already noted/)
+  const moved = 'f'.repeat(40)
+  const next = planStrangerComment({ pr: 7, body: conclusion('PASS', moved, 'Reviewer3'), association: 'MEMBER', writers, comments: [{ body: first.body }] })
+  assert.equal(next.add, true, 'a note about another tip is not this tip')
+  assert.equal(next.tip, moved)
+})
+
+test('#767: everything that is not a stranger PASS stays silent', () => {
+  const plan = (body, extra = {}) => planStrangerComment({ pr: 7, body, association: 'MEMBER', writers, ...extra })
+  assert.equal(plan(conclusion()).add, false, 'a listed writer labels, and is not told anything')
+  assert.equal(plan(conclusion('INC', head, 'Reviewer3')).add, false, 'an INC is not the case, whoever writes it')
+  assert.equal(plan(conclusion('PASS', head, 'Reviewer3'), { association: 'NONE' }).add, false, 'a passer-by is not a conclusion')
+  assert.equal(plan(`Written-by: Reviewer3\n\nWhat I ran: nothing.`).add, false, 'no verdict line, nothing to say')
+  assert.equal(plan(`Verdict: PASS at ${head}`).add, false, 'a PASS with no Written-by names no one to tell')
+  assert.equal(plan(`Correction from Reviewer: the PASS at ${head} was written by Reviewer3.`).add, false, 'a correction is not a conclusion')
+})
+
+test('#767: one run picks the label, the note, or silence — and never both', () => {
+  const run = (body, extra = {}) => planCommentRun({ pr: 7, body, association: 'MEMBER', writers, ...extra })
+  const labelled = run(conclusion())
+  assert.equal(labelled.write, 'label', 'a listed writer still just labels')
+  const noted = run(stranger())
+  assert.equal(noted.write, 'comment')
+  assert.equal(noted.writer, 'Reviewer3')
+  assert.ok(noted.body.startsWith(`${STRANGER_MARK} tip:${head}`))
+  assert.equal(noted.labelled, undefined, 'the note path never labels')
+  const again = run(stranger(), { comments: () => [{ body: noted.body }] })
+  assert.equal(again.write, 'none', 'a second run on the same tip writes nothing')
+  const silent = run(conclusion('INC', head, 'Reviewer3'))
+  assert.equal(silent.write, 'none')
+  // The thread is read only when a stranger's PASS is really on the table.
+  let reads = 0
+  run(conclusion(), { comments: () => { reads += 1; return [] } })
+  assert.equal(reads, 0, 'a plain PASS does not read the thread')
+  run(stranger(), { comments: () => { reads += 1; return [] } })
+  assert.equal(reads, 1)
+})
+
+test('#767: one note per commit, whether the tip is written 12 characters or 40', () => {
+  const long = 'a'.repeat(40)
+  assert.equal(long.length, 40)
+  const short = long.slice(0, 12)
+  const first = planStrangerComment({ pr: 7, body: conclusion('PASS', short, 'Reviewer3'), association: 'MEMBER', writers })
+  assert.equal(first.add, true)
+  const again = planStrangerComment({ pr: 7, body: conclusion('PASS', long, 'Reviewer3'), association: 'MEMBER', writers, comments: [{ body: first.body }] })
+  assert.equal(again.add, false, 'the same commit named two ways is one note')
+  const other = 'b'.repeat(40)
+  const next = planStrangerComment({ pr: 7, body: conclusion('PASS', other, 'Reviewer3'), association: 'MEMBER', writers, comments: [{ body: first.body }] })
+  assert.equal(next.add, true, 'a different commit may speak once')
+})

changelog.d/767-pass-label-stranger.fixed.md

@@ -0,0 +1 @@
+A PASS whose `Written-by:` names someone outside `writers` in `.github/agent/review-policy.json` is refused by `review-gate` and `merge-gate`, but until now only a log line said so: the reviewer assumed a conclusion that counted, nobody enqueued, and the pull request waited with every check green. The `pass` label job now leaves one comment per tip naming the author, the tip and the file to edit, and still adds no label for it ([#767](https://github.com/ThinkFlowLab/FlyBot/issues/767)).