From c27a028afec401c3043e6e1a2344287e6fe854a8 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Sun, 25 Oct 2026 16:33:42 +0800 Subject: [PATCH] Judge a conclusion against the whole head, and print the head that was judged The guard printed a PASS at a tip that no conclusion named yet, twice on #1106: a conclusion written `at abaf938` matched any head beginning with those seven characters, so the check went green on the newest tip before its own PASS existed, and a pull request that was PASSed once could be pushed again and merge with no conclusion at the new head (#1112). A conclusion now names the whole 40-character sha; a shorter tip names no head and is refused with its length in the reason. The guard reads the head, then the comments and the diff, then the head again: a push in the middle of a run is refused instead of judging one head and printing another. The report list carries each comment's id, the printed line carries the full sha, and the summary names the comment the decision used, so the decision is a snapshot a reader can go and look at. Fixes #1112 --- .claude/skills/review-pr/SKILL.md | 2 +- .github/scripts/enqueue-guard.mjs | 19 ++++++++-- .github/scripts/enqueue-guard.test.mjs | 51 +++++++++++++++++++++++++- .github/scripts/enqueue-rules.mjs | 2 +- .github/scripts/gate-check.mjs | 4 +- .github/scripts/review-gate.mjs | 20 ++++++---- .github/scripts/review-gate.test.mjs | 8 ++-- AGENTS.md | 2 +- 8 files changed, 89 insertions(+), 19 deletions(-) diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index 5acbf87dc..9e4d5465e 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -12,7 +12,7 @@ This file stays thin: Verifier process only. On each tip review, re-read these f ## Establish the tip 1. Confirm repo identity (`ThinkFlowLab/FlyBot` or the agreed fork) and the commit under review. -2. Record **tip SHA** (`git rev-parse HEAD` or the PR head SHA). If the tip moves, re-run from scratch. +2. Record **tip SHA** (`git rev-parse HEAD` or the PR head SHA) — all 40 characters. The gate compares the whole sha: a `Verdict: … at abaf938` names every head that starts that way, and the newest of them is the tip being judged, so a short sha never counts (#1112). If the tip moves, re-run from scratch. 3. Load the **latest rules from `origin/main`** before each review: `git fetch origin && git show origin/main:` for this skill, `docs/code-standards.md`, `AGENTS.md`, and every `docs/subsystems/` page the diff touches. Prefer the fetched text over a stale local copy. Do not restate those pages here. 4. Read [LESSONS.md](../../../LESSONS.md) and the hard rules in [CONTRIBUTING.md](../../../CONTRIBUTING.md). 5. Inventory changed files vs the merge base. Do not treat author narrative as evidence. diff --git a/.github/scripts/enqueue-guard.mjs b/.github/scripts/enqueue-guard.mjs index 610a877fc..eeae7f9c9 100644 --- a/.github/scripts/enqueue-guard.mjs +++ b/.github/scripts/enqueue-guard.mjs @@ -59,13 +59,22 @@ export function decidePull({ gh = realGh, env = process.env, repo: repoOption, p // #676: the association travels with the report, the way review-gate.mjs reads the same two APIs. Without // it every conclusion is refused as "not tied to this repository", so a PASS could never clear the guard. // #943: the account type travels too, for the same reason — a bot account writes no conclusion. + // #1112: each report carries the id GitHub gave it, so the summary can name the one the decision used. + // With one shared account the id is the only handle on a conclusion that is not a person or a word. const reports = [ - ...list(`repos/${repo}/issues/${pr}/comments?per_page=100`).map((c) => ({ kind: 'comment', login: c.user?.login, type: c.user?.type, body: c.body, at: c.created_at, association: c.author_association })), + ...list(`repos/${repo}/issues/${pr}/comments?per_page=100`).map((c) => ({ id: c.id, kind: 'comment', login: c.user?.login, type: c.user?.type, body: c.body, at: c.created_at, association: c.author_association })), ...list(`repos/${repo}/pulls/${pr}/reviews?per_page=100`) .filter((v) => (v.body ?? '').trim()) - .map((v) => ({ kind: 'review', login: v.user?.login, type: v.user?.type, body: v.body, at: v.submitted_at, association: v.author_association })), + .map((v) => ({ id: v.id, kind: 'review', login: v.user?.login, type: v.user?.type, body: v.body, at: v.submitted_at, association: v.author_association })), ] const files = list(`repos/${repo}/pulls/${pr}/files?per_page=100`) + // #1112: the head is read again after the comments and the diff. A push in the middle of this run + // would otherwise let the decision judge one head and print another — the shape in which a pull + // request that was PASSed once is pushed again and merges with no conclusion at the new tip. A + // moved head is refused rather than resolved: both reads are stale by the time the second lands. + const again = JSON.parse(gh('api', `repos/${repo}/pulls/${pr}`)) + const moved = again.head?.sha + if (moved && moved !== head) throw new Error(`the head moved during this run (${short(head)} to ${short(moved)}); run it again and judge the head it prints`) // The live head is judged, never a cached one: a push that moved the tip voids the PASS the caller // believes it has, and `--sha` lets a caller insist on the head it read a moment ago. @@ -85,7 +94,11 @@ export function decidePull({ gh = realGh, env = process.env, repo: repoOption, p export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prOption } = {}) { const { decision, pull, pr } = decidePull({ gh, env, repo: repoOption, pr: prOption }) const labels = (pull.labels ?? []).map((l) => l.name) - console.log(`enqueue-guard: PR #${pr} head ${short(decision.head)}, labels [${labels.join(', ')}]`) + // #1112: the full 40-character sha, from the value the decision consumed. The short form let a reader + // compare it against a summary that named a different head and still look right. + console.log(`enqueue-guard: PR #${pr} head ${decision.head}, labels [${labels.join(', ')}]`) + const used = decision.verdict?.id ? `${decision.verdict.kind ?? 'comment'} ${decision.verdict.id}` : 'no conclusion' + console.log(`enqueue-guard: conclusion used: ${used}${decision.verdict?.by ? ` by ${decision.verdict.by}` : ''}${decision.verdict?.tip ? ` naming ${decision.verdict.tip}` : ''}`) console.log(decision.ok ? `enqueue-guard: ALLOW — ${decision.message}; ${decision.exempt ? `a ${decision.tier ?? 'T0/T1'} tier or a mechanical release PR (AGENTS.md 6)` : `${decision.verdict?.by}'s PASS names this tip`}` : `enqueue-guard: REFUSE — ${decision.message}`) diff --git a/.github/scripts/enqueue-guard.test.mjs b/.github/scripts/enqueue-guard.test.mjs index 3334cc665..79294edc5 100644 --- a/.github/scripts/enqueue-guard.test.mjs +++ b/.github/scripts/enqueue-guard.test.mjs @@ -36,8 +36,9 @@ const report = ({ tip = HEAD, verdict = 'PASS', extra = [] } = {}) => const comment = (body = report(), at = '2026-10-21T10:31:00Z') => ({ user: { login: 'reviewer-agent' }, body, created_at: at, author_association: 'MEMBER' }) -const fakeGh = ({ body = HEADERS + 'Fixes #602\n', comments = [], reviews = [], labels = ['agent-pr'], files = [], head = HEAD, author = 'opener', headRepo = 'ThinkFlowLab/FlyBot', permission = undefined, draft = false, merged = false } = {}) => { +const fakeGh = ({ body = HEADERS + 'Fixes #602\n', comments = [], reviews = [], labels = ['agent-pr'], files = [], head = HEAD, heads = null, author = 'opener', headRepo = 'ThinkFlowLab/FlyBot', permission = undefined, draft = false, merged = false } = {}) => { const calls = [] + let pullReads = 0 const gh = (...args) => { const path = args[args.length - 1] calls.push(path) @@ -48,7 +49,11 @@ const fakeGh = ({ body = HEADERS + 'Fixes #602\n', comments = [], reviews = [], if (path.includes('/files')) return files.map((f) => JSON.stringify(typeof f === 'string' ? { filename: f } : f)).join('\n') if (path.includes('/comments')) return comments.map((c) => JSON.stringify(c)).join('\n') if (path.includes('/reviews')) return reviews.map((r) => JSON.stringify(r)).join('\n') - if (/\/pulls\/\d+$/.test(path)) return JSON.stringify({ head: { sha: head, repo: { full_name: headRepo } }, user: { login: author }, body, labels: labels.map((name) => ({ name })), draft, merged }) + if (/\/pulls\/\d+$/.test(path)) { + // #1112 (c): a run reads the head twice, so the fixture can answer differently the second time. + const sha = heads ? heads[Math.min(pullReads++, heads.length - 1)] : head + return JSON.stringify({ head: { sha, repo: { full_name: headRepo } }, user: { login: author }, body, labels: labels.map((name) => ({ name })), draft, merged }) + } // Anything else — `gh pr merge` above all — is the guard doing more than checking. throw new Error('fake gh: unexpected ' + path) } @@ -599,3 +604,45 @@ test('#962: Docs is registered in writers; Owner: Docs passes and an unknown own assert.equal(refused.ok, false) assert.ok(refused.reasons.includes('Owner names "somebody", who is not a writers Bot name'), refused.reasons.join('; ')) }) + + +// ---- #1112: the guard reported a PASS that had not been posted yet, and a re-push could go green again. +// The three cases below are one per failure mode the issue names; each is red on the code before it. + +test('#1112: a 7-character prefix is not this tip, even though the head starts with it', () => { + // The guard went green at 05:09 on a conclusion naming a head that was PASSed at 05:12: a tip that is + // only a prefix matches every head that begins with it, and the newest one is the tip being judged. + const r = run({ comments: [comment(report({ tip: HEAD.slice(0, 7) }))] }) + assert.equal(r.ok, false, 'a short tip names no head') + assert.match(r.reasons.join('; '), /7 characters/) +}) + +test('#1112: the summary names the comment id the decision used, and a later edit cannot change it', () => { + const reports = [{ id: 4242, user: { login: 'reviewer-agent' }, body: report(), created_at: '2026-10-21T10:31:00Z', author_association: 'MEMBER' }] + const r = run({ comments: reports }) + assert.equal(r.ok, true, r.reasons.join('; ')) + assert.equal(r.verdict.id, 4242) + assert.equal(r.verdict.kind, 'comment') + // The decision holds its own copy: an edit after the read is not a second opinion. + reports[0].body = report({ verdict: 'INC' }) + assert.equal(r.verdict.verdict, 'PASS') +}) + +test('#1112: the printed head is the decided head, all 40 characters, and it names the conclusion', () => { + const lines = [] + const log = console.log + console.log = (...a) => lines.push(a.join(' ')) + try { + run({ comments: [{ id: 77, user: { login: 'reviewer-agent' }, body: report(), created_at: '2026-10-21T10:31:00Z', author_association: 'MEMBER' }] }) + } finally { console.log = log } + assert.match(lines[0], new RegExp(`head ${HEAD},`), 'the head is printed whole, from the value judged') + assert.match(lines.join('\n'), /conclusion used: comment 77 by reviewer-agent naming [0-9a-f]{40}/) +}) + +test('#1112: a head that moves while the comments are read is refused, not judged', () => { + const later = 'c0ffee00'.repeat(5) + let thrown = null + try { run({ comments: [comment()], heads: [HEAD, later] }) } catch (e) { thrown = e } + assert.ok(thrown, 'the run refuses instead of judging a head nobody will merge') + assert.match(thrown.message, /head moved during this run/) +}) diff --git a/.github/scripts/enqueue-rules.mjs b/.github/scripts/enqueue-rules.mjs index bbb41b96b..58bfcb890 100644 --- a/.github/scripts/enqueue-rules.mjs +++ b/.github/scripts/enqueue-rules.mjs @@ -121,7 +121,7 @@ export function recordOnlyAt(reports, head) { if (!target) return null const at = (reports ?? []) .map((r, i) => ({ r, i, parsed: parseReport(r.body) })) - .filter(({ parsed }) => parsed.verdict && parsed.tip && target.startsWith(parsed.tip)) + .filter(({ parsed }) => parsed.verdict && parsed.tip && target === parsed.tip) .map((x) => ({ ...x, t: Date.parse(x.r.at ?? 0) || 0 })) .sort((a, b) => a.t - b.t || a.i - b.i) const last = at.at(-1) diff --git a/.github/scripts/gate-check.mjs b/.github/scripts/gate-check.mjs index 5f5f41c1e..13d8c88d7 100644 --- a/.github/scripts/gate-check.mjs +++ b/.github/scripts/gate-check.mjs @@ -87,7 +87,9 @@ export function summaryOf({ decision, head, pr, entry } = {}) { if (decision?.exempt) lines.push(`- no PASS needed: ${decision.tier ? `computed tier ${decision.tier}` : 'a T0/T1 tier or a mechanical release pull request'} (AGENTS.md → Merge gates)`) if (decision?.mechanicalRelease) lines.push('- a mechanical release pull request: it carries no issue and no `agent-pr` by design') if (decision?.fork) lines.push('- the head lives in a fork: the issue reference and `agent-pr` rules are not asked of it') - if (decision?.verdict?.by) lines.push(`- newest conclusion at this tip: ${decision.verdict.verdict ?? 'PASS'} by ${decision.verdict.by}`) + // #1112: the summary names the comment the decision read, so a reader can go and look at the text it + // used instead of trusting the sentence. The full sha above it is the value that was judged. + if (decision?.verdict?.by) lines.push(`- newest conclusion at this tip: ${decision.verdict.verdict ?? 'PASS'} by ${decision.verdict.by} (${decision.verdict.kind ?? 'comment'} ${decision.verdict.id ?? 'without an id'}), naming ${decision.verdict.tip ?? 'no tip'}`) if (decision?.recordOnly) lines.push('- the newest conclusion at this head says "record only — do not merge"') if (!ok) lines.push('', 'What refuses it:', ...(decision?.reasons?.length ? decision.reasons.map((r) => `- ${r}`) : ['- no reason was recorded — treat this as a bug in the gate itself'])) lines.push('', '_One decision, two readers: this check and `node .github/scripts/enqueue-guard.mjs --pr ` call the same function. Rules: [`AGENTS.md` → Merge gates](../../AGENTS.md#merge-gates). This check reads the repository and writes this one run; it never merges, labels or comments._') diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index 455ec3387..3fa1fa4bf 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -206,11 +206,17 @@ export function decide({ head, author, reports, policy, admins } = {}) { for (const r of reports ?? []) { const parsed = parseReport(r.body) if (!parsed.tip && !parsed.verdict) continue // not a Verifier report - const entry = { login: r.login, verdict: parsed.verdict, tip: parsed.tip, at: r.at, kind: r.kind, writer: parsed.writer, parsed, counts: false, why: null } + const entry = { id: r.id, login: r.login, verdict: parsed.verdict, tip: parsed.tip, at: r.at, kind: r.kind, writer: parsed.writer, parsed, counts: false, why: null } judged.push(entry) if (!parsed.tip) { reasons.push(`${where(r)}: no Tip SHA`); continue } - if (!headSha.startsWith(parsed.tip)) { - reasons.push(`${where(r)}: names ${parsed.tip.slice(0, SHORT)}, not the tip ${headSha.slice(0, SHORT)}`) + // #1112: the comparison is the whole sha, not a prefix. A conclusion written `at abaf938` is a + // mention of one head among every head that starts that way, and one of them is the newest tip — the + // guard went green at 05:09 on a PASS naming the tip that was PASSed at 05:12 because a short tip + // matched it. A tip that is not 40 characters names no head here; the refusal says how long it was. + if (parsed.tip !== headSha) { + reasons.push(parsed.tip.length < 40 + ? `${where(r)}: names ${parsed.tip.slice(0, SHORT)} (${parsed.tip.length} characters), not the full tip ${headSha} — a conclusion names the whole 40-character sha` + : `${where(r)}: names ${parsed.tip.slice(0, SHORT)}, not the tip ${headSha.slice(0, SHORT)}`) continue } if (!parsed.verdict) continue // a `Tip SHA` block with no verdict is not a conclusion @@ -264,15 +270,15 @@ export function decide({ head, author, reports, policy, admins } = {}) { .map((e, i) => ({ e, i, t: Date.parse(e.at ?? 0) || 0 })) .sort((a, b) => a.t - b.t || a.i - b.i) .at(-1).e - if (!last.counts) return { ok: false, verdict: last.verdict, by: null, tip: last.tip, reason: last.why, judged } + if (!last.counts) return { ok: false, verdict: last.verdict, by: null, tip: last.tip, id: last.id, kind: last.kind, reason: last.why, judged } const { parsed } = last if (last.verdict !== 'PASS') { - return { ok: false, verdict: last.verdict, by: last.login, tip: last.tip, reason: `${last.login} says ${last.verdict ?? 'nothing'} at this tip`, judged } + return { ok: false, verdict: last.verdict, by: last.login, tip: last.tip, id: last.id, kind: last.kind, reason: `${last.login} says ${last.verdict ?? 'nothing'} at this tip`, judged } } if (!parsed.ran.length) { - return { ok: false, verdict: 'PASS', by: last.login, tip: last.tip, reason: `${last.login}'s PASS lists no command it ran`, judged } + return { ok: false, verdict: 'PASS', by: last.login, tip: last.tip, id: last.id, kind: last.kind, reason: `${last.login}'s PASS lists no command it ran`, judged } } - return { ok: true, verdict: 'PASS', by: last.login, tip: last.tip, reason: `${last.login}'s PASS names this tip`, judged } + return { ok: true, verdict: 'PASS', by: last.login, tip: last.tip, id: last.id, kind: last.kind, reason: `${last.login}'s PASS names this tip`, judged } } export function render(result, { pr, head, author } = {}) { diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index fca836ed3..f37665922 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -257,8 +257,11 @@ test('#643: a quoted verdict later in the body does not overwrite the first line // report quoted in the body, used to move the tip, so `decide` dropped the INC as "about another // tip" and the PASS it refused stayed in force. test('#640: a quoted Tip SHA later in the body does not move the first line to another tip', () => { + // #1112 made a conclusion name the whole sha, so this fixture names the head it judges in full: the + // point of the case is which line names the tip, not how long the tip is. + const head = '1234567890abcdef000000000000000000000000' const body = [ - 'Verdict: INC at 1234567890abcdef', + `Verdict: INC at ${head}`, 'Written by: Reviewer', '', 'Ran: node --test .github/scripts/review-gate.test.mjs → 43 pass', @@ -274,12 +277,11 @@ test('#640: a quoted Tip SHA later in the body does not move the first line to a ].join('\n') const parsed = parseReport(body) assert.equal(parsed.verdict, 'INC') - assert.equal(parsed.tip, '1234567890abcdef', 'the first line names the tip being judged') + assert.equal(parsed.tip, head, 'the first line names the tip being judged') assert.deepEqual(parsed.ran, ['node --test .github/scripts/review-gate.test.mjs → 43 pass']) assert.equal(parsed.hasNotRun, true) // And the whole point: the INC is not dropped, so it still refuses the merge. - const head = '1234567890abcdef000000000000000000000000' const at = (report, when) => ({ kind: 'comment', login: 'reviewer-agent', body: report, at: when, association: 'MEMBER' }) const policy = { reviewers: ['reviewer-agent'], selfReview: [], writers: ['Reviewer'] } const pass = ['Tip SHA: ' + head, 'Ran:', ' - npm test → pass', 'Not run:', ' - smoke', 'Verdict: PASS'].join('\n') diff --git a/AGENTS.md b/AGENTS.md index c4bf136f2..a8eea7555 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -184,7 +184,7 @@ Four rules decide what a queue entry must pass. The mechanical judge runs where A review's `state` is not read at all: approvals are not a gate any more (user decision 2026-10-22), and the account cannot approve its own pull requests, so its reviews stay `COMMENTED` even for an INC — the INC on #610 was a comment rather than a review. -**One conclusion per tip.** A tip carries at most one conclusion: one `Verdict:` block naming one SHA. Two conclusions at the same tip is an incident, not a tie to be broken — nobody re-verdicts to win, the Reviewer that wrote the later one retracts it in a comment, and the pull request waits. A correction reads `Correction from : …` and carries no `Verdict:` line, so it never competes with the standing conclusion; the replacement PASS for a missing `Ran:` ([above](#gates)) is the only other body allowed at one tip. **A GitHub bot account's `Verdict:` is never a conclusion** — `user.type` is `Bot`, or the login ends `[bot]` — whatever its body says (#943, `cursor[bot]` at #940's tip); the rule is the account type and not a login list, because every Bot writes as the shared `Birfy` account and a list containing it would void the whole pipeline, so the real Reviewer voids such a report at that tip with `Correction from Reviewer:`. +**One conclusion per tip.** A tip carries at most one conclusion: one `Verdict:` block naming one SHA — the whole 40 characters, because a prefix names every head that starts that way and the newest of them is the tip being judged (#1112). Two conclusions at the same tip is an incident, not a tie to be broken — nobody re-verdicts to win, the Reviewer that wrote the later one retracts it in a comment, and the pull request waits. A correction reads `Correction from : …` and carries no `Verdict:` line, so it never competes with the standing conclusion; the replacement PASS for a missing `Ran:` ([above](#gates)) is the only other body allowed at one tip. **A GitHub bot account's `Verdict:` is never a conclusion** — `user.type` is `Bot`, or the login ends `[bot]` — whatever its body says (#943, `cursor[bot]` at #940's tip); the rule is the account type and not a login list, because every Bot writes as the shared `Birfy` account and a list containing it would void the whole pipeline, so the real Reviewer voids such a report at that tip with `Correction from Reviewer:`. **Judged on the merge tree, never on the pull request page.** The page answers "does this commit pass alone"; the queue answers "does this commit pass merged onto the current `main`", and the second one is the merge decision. #759 was green on every check, was kicked out of the queue twice, and went red on `main` at `core/test/follow-through.test.js:264`. A red queue entry that names files you did not touch is the stale base of [step 2](#pipeline-contract), not bad luck: fix the base, do not re-enqueue blindly.