diff --git a/.github/scripts/pass-label.mjs b/.github/scripts/pass-label.mjs index 3895c5340..5341c6e48 100644 --- a/.github/scripts/pass-label.mjs +++ b/.github/scripts/pass-label.mjs @@ -14,6 +14,13 @@ // `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' @@ -21,6 +28,9 @@ 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 = '' +const STRANGER_NOTE = /\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: [] } } diff --git a/.github/scripts/pass-label.test.mjs b/.github/scripts/pass-label.test.mjs index b4f7ad917..1673d1398 100644 --- a/.github/scripts/pass-label.test.mjs +++ b/.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') +}) diff --git a/changelog.d/767-pass-label-stranger.fixed.md b/changelog.d/767-pass-label-stranger.fixed.md new file mode 100644 index 000000000..debee3365 --- /dev/null +++ b/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)).