diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index 5acbf87d..9e4d5465 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 610a877f..eeae7f9c 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 3334cc66..79294edc 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 bbb41b96..58bfcb89 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 5f5f41c1..13d8c88d 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 455ec338..3fa1fa4b 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 fca836ed..f3766592 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 c4bf136f..a8eea755 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.