From 57aef0df6ca759008c8b96013e6709f91ebbd376 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Fri, 23 Oct 2026 10:00:54 +0800 Subject: [PATCH 1/2] fix(pass-label): tell a reviewer whose PASS the policy does not count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `verdictOf()` refuses a PASS whose `Written-by:` names someone outside `writers` in `.github/agent/review-policy.json`, and `planComment()` turns that refusal into `{ add: false, reason: 'not a PASS by a writer the policy lists' }`: the reason is printed to a log and nothing else happens. So a reviewer added to the team writes a careful PASS, the gates refuse it, nobody enqueues a T2/T3 pull request that "has a PASS", and the pull request waits with every check green — the case that cost #646 an hour of review (#767). The job already holds `issues`/`pull-requests: write` (it adds the `pass` label) and it already reaches exactly this decision, so the note belongs here: - `strangerPassOf()` keeps the refused PASS as a value of its own — a `Verdict: PASS at ` from a repository member whose `Written-by:` is not in `writers`. A missing `Written-by:` line is not this case: there is no one to tell. - `strangerCommentBody()` names the author, the tip, the file to edit (`writers` in `.github/agent/review-policy.json`) and that the file is T3, so the edit does not go into the reviewing pull request. It says the conclusion does not count, not that the pull request cannot merge — a listed writer's PASS still can. - The marker ` tip:` makes it one note per tip: a second run adds nothing, and a moved tip may speak once about the new one. The thread is read only when a stranger's PASS is really on the table. - `planCommentRun()` is the whole decision for one `--comment` run — label, note, or nothing — and `main()` only executes it, so the choice is tested without a network. No label is ever added for this case, no label is removed, `reviewed tip` stays report-only (the workflow's `continue-on-error` is untouched), `review-policy.json` is not edited, and `tests-ok`'s `needs` are unchanged. Pinned by #767: a stranger's PASS leaves one note naming the author, the tip and the file; the note is idempotent per tip and a moved tip speaks once; everything that is not a stranger's PASS stays silent (a listed writer, an INC, a passer-by, a missing `Written-by:`, a correction); and one run picks label, note, or silence, never both. Red without the change, at this tip: returning `write: 'none'` instead of the note → 9/10, only red *one run picks the label, the note, or silence*; dropping the marker check → 8/10, red the idempotency case and that same one; treating a listed writer as a stranger → 9/10, only red *everything that is not a stranger PASS stays silent*. Part of #767. --- .github/scripts/pass-label.mjs | 75 +++++++++++++++++++- .github/scripts/pass-label.test.mjs | 61 +++++++++++++++- changelog.d/767-pass-label-stranger.fixed.md | 1 + 3 files changed, 134 insertions(+), 3 deletions(-) create mode 100644 changelog.d/767-pass-label-stranger.fixed.md diff --git a/.github/scripts/pass-label.mjs b/.github/scripts/pass-label.mjs index 3895c5340..8ba249f89 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,8 @@ 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 POLICY = '.github/agent/review-policy.json' const SHORT = 12 @@ -52,6 +61,54 @@ 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' } + const marker = `${STRANGER_MARK} tip:${stranger.tip}` + if (comments.some((comment) => String(comment?.body ?? comment ?? '').includes(marker))) { + 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 +152,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..c539a0aa2 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,62 @@ 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) +}) 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)). From 517be9bd6bdcff9e9fe43f81fe084759d213af08 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Fri, 23 Oct 2026 10:54:38 +0800 Subject: [PATCH 2/2] fix(pass-label): one note per commit, however the tip is written A report may name its tip with 12 characters or with 40, and the note's marker carried the tip verbatim, so the same commit described both ways left two comments. Compare the tip already in a marker with the passing one the way `namesHead()` compares a report's tip with a head, and pin it with a case that writes the note from a 12-character tip and then judges a 40-character one. Part of #767. --- .github/scripts/pass-label.mjs | 12 ++++++++---- .github/scripts/pass-label.test.mjs | 13 +++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/.github/scripts/pass-label.mjs b/.github/scripts/pass-label.mjs index 8ba249f89..5341c6e48 100644 --- a/.github/scripts/pass-label.mjs +++ b/.github/scripts/pass-label.mjs @@ -30,6 +30,7 @@ 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 @@ -91,10 +92,13 @@ This note is left once per tip by the \`pass\` label job, which only records ver 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' } - const marker = `${STRANGER_MARK} tip:${stranger.tip}` - if (comments.some((comment) => String(comment?.body ?? comment ?? '').includes(marker))) { - return { add: false, reason: `already noted at ${stranger.tip}` } - } + // 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) } } diff --git a/.github/scripts/pass-label.test.mjs b/.github/scripts/pass-label.test.mjs index c539a0aa2..1673d1398 100644 --- a/.github/scripts/pass-label.test.mjs +++ b/.github/scripts/pass-label.test.mjs @@ -148,3 +148,16 @@ test('#767: one run picks the label, the note, or silence — and never both', ( 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') +})