From d71a88af1bce4f32fab24b427c8843e4c3ecf617 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Tue, 27 Oct 2026 11:44:49 +0800 Subject: [PATCH] fix(gate): keep a PASS when the tip moved without changing the change set (#1317) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Written-by: Dev3 Fixes #1317 Owner: Dev3 Reviewer: Reviewer2 Docs-not-needed: no page under docs/subsystems/ owns .github/scripts/ or AGENTS.md (arch-docs reads 0 owned files for this diff); the rule text is AGENTS.md and it moves with the code. ## Reproduce At `2c02cd15` any new commit voids a PASS, including the one `PUT /pulls//update-branch` writes to bring a branch up to date. `decide` compares the whole 40-character tip and nothing else: ``` reviewer-agent: names 175328d0c1b2, not the tip abaf938b0c1d ``` That answer is right when the code changed and wrong when only `main` moved: with a PASS at `175328d0c1b2…` and a head `abaf938b0c1d…` that merged `main` in, every changed path can carry the same blob at both tips and the pull request still pays a whole second review (#1304 was PASSed three times for one change set, once per update-branch). The same `decide` arms `enqueue-guard` and `merge-gate`, so all three paths answer alike — and all three were wrong alike. ## What changed - **New `.github/scripts/change-set.mjs`**: `sameChangeSet({ gh, repo, reviewed, head, paths })` reads `git/trees/?recursive=1` for both tips and compares the blob sha of every changed path. `{ok:true}` only when every path exists at both tips with the same blob. Fail closed everywhere else — no path list, no reader, a 404, `truncated:true`, no `tree` in the answer, a path missing on either side — and each refusal says which read failed (`git tree of 175328d0c1b2: 404`, `…: truncated`) or which path differs, so "the code changed" and "I could not look" never read the same. - **The comparison rejects a 100-entry list.** Each caller reads one page (`pulls//files?per_page=100`), so a list that fills the page may be cut short and file 101 could be the file that moved: that refuses as `the changed-file list stops at 100 entries, so it may be cut short` instead of comparing a part of the change set. A 99-entry list compares normally. - **`review-gate.mjs#decide`**: three optional inputs (`changedPaths`, `sameChangeSet`, `gh`). In the `parsed.tip !== headSha` branch — the one comparison point #1317 names — the conclusion is now compared before it is dropped: a match marks the entry `carriedFrom` and keeps it, a mismatch or an unreadable tree refuses with the reason appended. Without a path list the old refusal stands unchanged. - **A conclusion written at the head always decides.** `const exact = atTip.filter((e) => !e.carriedFrom)` and only carried ones are read when the head carries none. This is a new rule, and #1317 needs it: a carried PASS written after an INC at the head would otherwise win on timestamp alone, and the INC would stop vetoing. - **The allow sentence names both full shas and the comparison**: `reviewer-agent's PASS at still describes this head : the change set is identical (every changed path carries the same blob at both tips)`, and the result carries `carried: true` so every renderer can say which PASS it read. - **The three readers stay one cut.** `merge-gate.mjs#judge` passes `changedPaths: fileNames(files)` from the entry's own file list and accepts an injected comparison; `enqueue-guard.mjs#decidePull` forwards it; the ALLOW tails in `enqueue-guard.mjs` and `enqueue-rules.mjs` say why (`… the change set is the one that PASS names, so the tip moved without changing it`). `review-gate.mjs#main` reads the changed-file list itself. - **`AGENTS.md` three places**, all the same rule: `:34` (the pipeline contract's one-conclusion-per-tip), `:174` (Gates — where the queue reader and the `risk-tier` summary point, so leaving it would keep a contradicting sentence two screens below the new one, and the rule now also carries the exception's CI/queue obligation), and `:203` (Merge gates: the mechanism, the fail-closed list, and the precedence rule). - **Fixture**: `.github/scripts/fixtures/workflow-cases.json` gains a top-level `changeSetCases` array — `tip-moved-same-change-set` (allow, with the exact sentence), `tip-moved-one-blob-changed` (refuse, `the change set differs at core/package.json`), `tip-moved-tree-unreadable` (refuse, `the change set could not be read (git tree of 175328d0c1b2: 404)`). It is a new key rather than entries in `cases`, because `core/test/workflow-repo-dev.test.js` consumes `cases` one by one and adding there would mean editing `core/`, which #1317's boundary excludes. ## What I ran At the tree of this commit, `TZ=UTC`, Node v22.23.3: - `node --test ".github/scripts/*.test.mjs"` → `# tests 524 / # pass 522 / # fail 0 / # skipped 2`. - `node --test .github/scripts/change-set.test.mjs` → `# tests 20 / # pass 20 / # fail 0` (the 19 acceptance cases plus the page-limit one). - `node scripts/ds-bot-patches.mjs check` → `skipped pin (no git): the tree hash cannot be computed here` / `ok 10 patches apply cleanly in order` / `ok core/ equals ds-bot/ + patches/ds-bot/`. `core/` and `patches/ds-bot/` are untouched by this diff, so no `generate` was run. ## Red without the source change Two knives, both run at this tip and both restored afterwards (`diff` against the backup empty, suite `524 / 522 / 0 / 2` again): 1. **The comparison deleted** (`const moved = false && changedPaths…`): `not ok 8` (acceptance 1 — a repaired tip passes), `not ok 10` (acceptance 2 — one changed blob refuses), `not ok 11` (acceptance 3 — an unreadable tree refuses with the read), `not ok 13` (acceptance 5 — a newer matching conclusion wins), `not ok 14` (acceptance 7 — a carried PASS with no `Ran:` still refuses), `not ok 18/19/20` (acceptance 6, all three change-set cases); `# tests 20 / # pass 12 / # fail 8`. 2. **`sameChangeSet` always reports identical** (`return { ok: true, paths }` before the first tree read — the "another base sha and still green" failure): `not ok 2` (one changed blob is a different change set), `not ok 3` (a path missing on one side), `not ok 4` (truncated tree), `not ok 5` (a tree that 404s), `not ok 19/20` (acceptance 6 for "one blob changed" and "tree unreadable"); `# tests 20 / # pass 14 / # fail 6`. Acceptance 6 is not a unit test of the helper: it drives **`decidePull`** (`enqueue-guard`'s own decision), the guard's check line, and `judge` (`merge-gate`) over the same three change sets, so the three readers cannot drift apart while each one's own test stays green — that is the issue's core, not a spare test. ## Not run - `npm run verify` at the root: it needs `npm ci` and `npm run harness` in `core/`, which this box does not have; CI's `tests-ok` runs the same chain. `core/test/workflow-repo-dev.test.js` reads `cases` only, and the new key is top-level, so it is unaffected — but I did not run it here. - `node scripts/build-client.mjs --check`, smoke, typecheck and actionlint: nothing under `core/` changed. - A real `update-branch` on this pull request: the carried path is exercised through injected tree readers, not by merging `main` into a reviewed tip and re-running the guard against the live API. ## Risk - A carried PASS is a claim about **blobs, not about tests**. It does not stand in for CI: the head still needs its own green `tests-ok`, and the queue still rebuilds on the newest `main`. What it buys is that the reviewer's reading of the same content is not thrown away when `main` moves. - The comparison uses the changed-file list the caller read, one page of 100, and refuses at the page limit rather than comparing a subset. A pull request larger than 100 files therefore always pays a second review — the answer it already had. - `git/trees/?recursive=1` is truncated for very large trees; that refuses (fail closed) as well, so the rule downgrades to "review again", never to "assume unchanged". - A rebased-away tip is not reachable, so `reviewed` can 404 where the reviewer truly reviewed a force-pushed commit. It refuses, as before. --- .github/scripts/change-set.mjs | 78 +++++ .github/scripts/change-set.test.mjs | 289 +++++++++++++++++++ .github/scripts/enqueue-guard.mjs | 6 +- .github/scripts/enqueue-rules.mjs | 8 +- .github/scripts/fixtures/workflow-cases.json | 168 ++++++++++- .github/scripts/merge-gate.mjs | 10 +- .github/scripts/review-gate.mjs | 68 ++++- AGENTS.md | 6 +- 8 files changed, 595 insertions(+), 38 deletions(-) create mode 100644 .github/scripts/change-set.mjs create mode 100644 .github/scripts/change-set.test.mjs diff --git a/.github/scripts/change-set.mjs b/.github/scripts/change-set.mjs new file mode 100644 index 000000000..095b9cf9e --- /dev/null +++ b/.github/scripts/change-set.mjs @@ -0,0 +1,78 @@ +// #1317: the identity a conclusion was written against, so a tip that moved without changing it keeps +// the PASS. +// +// A PASS names one tip, and the gate refused every other tip (#1112) — which is right when the code +// changed and wrong when only `main` moved: #1304 was PASSed three times for one change set, once per +// `PUT /pulls/1304/update-branch`, and every PR that has to be brought up to date pays the same review. +// +// The identity is a fingerprint over `(path → blob sha)` at the pull request's changed paths, not a +// shell `git patch-id`: the scripts have `gh` and nothing else (`enqueue-guard.mjs` is `execFileSync`), +// a rebased-away tip is not reachable by ref anyway, and a patch-id of one file answers a different +// question than "is the content a reviewer judged still the content here". Comparing path by path also +// catches the case the rule must still refuse: at #1304's `d7bd1c1d` an update-branch left +// `core/package.json` at 0.2.9 while main had 0.2.10, and that blob differed. +// +// Fail closed, always. Any read that does not answer — an API error, a 404, a tree the API cut short +// (`truncated: true`), a path missing on either side, a changed-file list the caller could not read — +// is "could not be read", and a caller that could not read it refuses the tip exactly as it did before. +// This helper never decides whether a PASS counts; `review-gate.mjs`'s `decide` does, and it calls this +// only when a conclusion names a different tip (#1317 keeps one cut: `decidePull`, the `enqueue-guard` +// check and `merge-gate`'s `judge` all read its answer). +const SHORT = 12 +const short = (s) => String(s ?? '').slice(0, SHORT) + +// What a failed `gh` call says, in one word when the answer is "no such object" and one short line +// otherwise: the refusal has to name the read that failed, not just "an error". +function describe(error) { + const text = String(error?.stderr ?? '') || String(error?.message ?? '') + if (/\b404\b|not found/i.test(text)) return '404' + const line = text.split('\n').map((l) => l.trim()).filter(Boolean)[0] ?? 'unknown error' + return line.length > 120 ? line.slice(0, 119) + '…' : line +} + +// The blobs of one tree, or the reason it could not be read. `recursive=1` is one call for the whole +// tree; `truncated` is the API saying it stopped early, which is not a tree to compare against. +function blobsAt({ gh, repo, sha }) { + let answer + try { + answer = JSON.parse(gh('api', `repos/${repo}/git/trees/${sha}?recursive=1`)) + } catch (error) { + return { error: `git tree of ${short(sha)}: ${describe(error)}` } + } + if (!answer || !Array.isArray(answer.tree)) return { error: `git tree of ${short(sha)}: the answer carries no tree` } + if (answer.truncated) return { error: `git tree of ${short(sha)}: truncated` } + const blobs = new Map() + for (const entry of answer.tree) { + if (entry && entry.type === 'blob' && typeof entry.path === 'string') blobs.set(entry.path, entry.sha) + } + return { blobs } +} + +// `paths` is the pull request's changed-file list, read by the caller (`GET /pulls/{n}/files`, one page — +// the callers in `review-gate.mjs`, `enqueue-guard.mjs` and `merge-gate.mjs` all use `per_page=100` and +// read one page); `reviewed` and `head` are the two tips. Identical means every changed path carries the +// same blob at both — a path missing on either side is a difference, not a pass. +const FILE_PAGE = 100 + +export function sameChangeSet({ gh, repo, reviewed, head, paths } = {}) { + const wanted = (paths ?? []).map((path) => String(path ?? '').trim()).filter(Boolean) + if (!wanted.length) return { ok: false, read: 'the pull request names no changed path to compare' } + // A list that fills the page may be cut short, and a comparison over a part of the change set is the + // one thing this must never report as identical: file 101 could be the file that moved. Refusing here + // costs a second review on a pull request that large, which is the answer the gate already gave. + if (wanted.length >= FILE_PAGE) return { ok: false, read: `the changed-file list stops at ${FILE_PAGE} entries, so it may be cut short` } + if (!gh || !repo) return { ok: false, read: 'no repository reader was given' } + const atReviewed = blobsAt({ gh, repo, sha: reviewed }) + if (atReviewed.error) return { ok: false, read: atReviewed.error } + const atHead = blobsAt({ gh, repo, sha: head }) + if (atHead.error) return { ok: false, read: atHead.error } + for (const path of wanted) { + const before = atReviewed.blobs.get(path) + const now = atHead.blobs.get(path) + // A path the tree does not carry on one side is not "unchanged": a file that only exists at one of + // the two tips changes the change set. The refusal names it like any other difference. + if (before === undefined || now === undefined) return { ok: false, path } + if (before !== now) return { ok: false, path } + } + return { ok: true, paths: wanted.length } +} diff --git a/.github/scripts/change-set.test.mjs b/.github/scripts/change-set.test.mjs new file mode 100644 index 000000000..9e1223136 --- /dev/null +++ b/.github/scripts/change-set.test.mjs @@ -0,0 +1,289 @@ +// #1317: a tip that moved without changing the reviewed change set must not void the PASS. +// +// The identity is `(path → blob sha)` over the pull request's changed paths, compared at the reviewed +// tip and at the head — content a reviewer actually judged, not the commits that carried it. Everything +// here drives the comparison through injected readers, so the rules run without the network: +// `sameChangeSet` takes `gh`, `decide` takes the helper, and the three-reader test drives `decidePull` +// over a fake repository built from `.github/scripts/fixtures/workflow-cases.json`. +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { mkdtempSync, readFileSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { fileURLToPath } from 'node:url' +import { sameChangeSet } from './change-set.mjs' +import { decide, loadPolicy } from './review-gate.mjs' +import { judge } from './merge-gate.mjs' +import { decidePull } from './enqueue-guard.mjs' +import { payloadFor } from './gate-check.mjs' + +const OLD = '175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90' +const HEAD = 'abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f' +const OLDER = '0f1e2d3c4b5a69788796a5b4c3d2e1f009182736' +const PATHS = ['core/src/host/turns.js', 'core/test/gate-check.test.js'] +const REPO = 'ThinkFlowLab/FlyBot' +const PR = 1317 + +// A `gh` that answers trees only, by sha: `trees[sha]` is `{path: blob}`. A sha missing from the map is +// the 404 the real API gives for a rebased-away tip; `bad` marks a tree the API cut short. +const treeGh = (trees, { truncatedAt = null, fail = null } = {}) => (...args) => { + const path = String(args[args.length - 1]) + const sha = /\/git\/trees\/([0-9a-f]+)/.exec(path)?.[1] + if (!sha) throw new Error(`fake gh: unexpected read ${path}`) + if (sha === fail) { + const error = new Error('Command failed: gh api') + error.stderr = 'gh: Not Found (HTTP 404)' + throw error + } + if (!trees[sha]) throw new Error(`fake gh: no tree for ${sha}`) + if (sha === truncatedAt) return JSON.stringify({ truncated: true, tree: [] }) + return JSON.stringify({ truncated: false, tree: Object.entries(trees[sha]).map(([path_, blob]) => ({ path: path_, type: 'blob', sha: blob })) }) +} + +const identicalTrees = () => ({ + [OLD]: { 'core/src/host/turns.js': 'blob-host', 'core/test/gate-check.test.js': 'blob-test' }, + [HEAD]: { 'core/src/host/turns.js': 'blob-host', 'core/test/gate-check.test.js': 'blob-test' }, +}) + +const say = (txt) => (txt) + +const report = ({ tip = OLD, verdict = 'PASS', ran = 'Ran: npm run verify → pass', writtenBy = 'Reviewer', at = '2026-10-25T10:00:00Z' } = {}) => ({ + kind: 'comment', + login: 'reviewer-agent', + type: 'User', + association: 'MEMBER', + at, + body: [`Verdict: ${verdict} at ${tip}`, `Written-by: ${writtenBy}`, '', ran].filter((line) => line !== undefined).join('\n'), +}) + +const policy = { reviewers: ['reviewer-agent'], selfReview: [], writers: ['Reviewer', 'reviewer-agent', 'Dev3'] } +const matching = () => ({ ok: true, paths: PATHS.length }) + +// --- the helper itself --------------------------------------------------------------------------- + +test('#1317: every changed path carrying the same blob is one change set, whatever the commits', () => { + assert.deepEqual(sameChangeSet({ gh: treeGh(identicalTrees()), repo: REPO, reviewed: OLD, head: HEAD, paths: PATHS }), { ok: true, paths: 2 }) +}) + +test('#1317: one changed blob is a different change set, and the refusal names the first path', () => { + const trees = identicalTrees() + trees[HEAD]['core/src/host/turns.js'] = 'blob-host-edited' + assert.deepEqual(sameChangeSet({ gh: treeGh(trees), repo: REPO, reviewed: OLD, head: HEAD, paths: PATHS }), { ok: false, path: 'core/src/host/turns.js' }) +}) + +test('#1317: a path the tree does not carry on one side is a difference, not a pass', () => { + const trees = identicalTrees() + delete trees[HEAD]['core/test/gate-check.test.js'] + assert.deepEqual(sameChangeSet({ gh: treeGh(trees), repo: REPO, reviewed: OLD, head: HEAD, paths: PATHS }), { ok: false, path: 'core/test/gate-check.test.js' }) +}) + +test('#1317: a truncated tree reads as neither equal nor different — it refuses with the read', () => { + assert.deepEqual(sameChangeSet({ gh: treeGh(identicalTrees(), { truncatedAt: HEAD }), repo: REPO, reviewed: OLD, head: HEAD, paths: PATHS }), { ok: false, read: `git tree of ${HEAD.slice(0, 12)}: truncated` }) +}) + +test('#1317: a tree that 404s refuses and names the read, never "assume unchanged"', () => { + assert.deepEqual(sameChangeSet({ gh: treeGh(identicalTrees(), { fail: OLD }), repo: REPO, reviewed: OLD, head: HEAD, paths: PATHS }), { ok: false, read: `git tree of ${OLD.slice(0, 12)}: 404` }) +}) + +test('#1317: no changed-path list is nothing to compare, so it refuses', () => { + assert.equal(sameChangeSet({ gh: treeGh(identicalTrees()), repo: REPO, reviewed: OLD, head: HEAD, paths: [] }).ok, false) +}) + +test('#1317: a changed-file list that fills the 100-entry page refuses as cut short, never as identical', () => { + const trees = identicalTrees() + const paths = Array.from({ length: 100 }, (_, i) => `core/src/host/file-${i}.js`) + for (const path of paths) { trees[OLD][path] = 'blob-same'; trees[HEAD][path] = 'blob-same' } + // Every path it was given is identical, so only the page limit can refuse this — which is the point: + // file 101 is the one that could have moved, and the answer is "could not be read", not "unchanged". + assert.deepEqual(sameChangeSet({ gh: treeGh(trees), repo: REPO, reviewed: OLD, head: HEAD, paths }), { ok: false, read: 'the changed-file list stops at 100 entries, so it may be cut short' }) + const shortList = paths.slice(0, 99) + assert.deepEqual(sameChangeSet({ gh: treeGh(trees), repo: REPO, reviewed: OLD, head: HEAD, paths: shortList }), { ok: true, paths: 99 }) +}) + +// --- acceptance 1-5, through `decide` ----------------------------------------------------------- + +test('#1317 acceptance 1: a repaired tip passes, and the allow sentence names both full shas', () => { + const r = decide({ head: HEAD, author: 'opener', reports: [report()], policy, changedPaths: PATHS, sameChangeSet: matching }) + assert.equal(r.ok, true) + assert.equal(r.carried, true) + assert.equal(r.reason, `reviewer-agent's PASS at ${OLD} still describes this head ${HEAD}: the change set is identical (every changed path carries the same blob at both tips)`) +}) + +test('#1317 acceptance 1 knife: deleting the comparison puts the refusal back', () => { + const r = decide({ head: HEAD, author: 'opener', reports: [report()], policy, changedPaths: PATHS, sameChangeSet: () => ({ ok: false, read: 'the changed files of this pull request were not read' }) }) + assert.equal(r.ok, false) + assert.match(r.reason, /names 175328d0c1b2, not the tip abaf938b0c1d — the change set could not be read/) +}) + +test('#1317 acceptance 2: one changed blob refuses, and the refusal names the path', () => { + const r = decide({ head: HEAD, author: 'opener', reports: [report()], policy, changedPaths: PATHS, sameChangeSet: () => ({ ok: false, path: 'core/package.json' }) }) + assert.equal(r.ok, false) + assert.match(r.reason, /names 175328d0c1b2, not the tip abaf938b0c1d/) + assert.match(r.reason, /the change set differs at core\/package\.json, so that conclusion does not describe this head/) +}) + +test('#1317 acceptance 3: an unreadable tree refuses with the read that failed', () => { + const r = decide({ head: HEAD, author: 'opener', reports: [report()], policy, changedPaths: PATHS, sameChangeSet: () => ({ ok: false, read: `git tree of ${OLD.slice(0, 12)}: 404` }) }) + assert.equal(r.ok, false) + assert.match(r.reason, /the change set could not be read \(git tree of 175328d0c1b2: 404\)/) +}) + +test('#1317 acceptance 4: an INC at the head vetoes, even when the head carries a matching old PASS', () => { + const r = decide({ + head: HEAD, + author: 'opener', + reports: [report(), report({ tip: HEAD, verdict: 'INC', at: '2026-10-25T11:00:00Z' })], + policy, + changedPaths: PATHS, + sameChangeSet: matching, + }) + assert.equal(r.ok, false) + assert.equal(r.verdict, 'INC') + assert.match(r.reason, /reviewer-agent says INC at this tip/) +}) + +test('#1317 acceptance 5: the newest matching conclusion wins — an older PASS never outranks a newer INC', () => { + const newerInc = decide({ + head: HEAD, + author: 'opener', + reports: [report({ tip: OLDER, at: '2026-10-25T09:00:00Z' }), report({ tip: OLD, verdict: 'INC', at: '2026-10-25T12:00:00Z' })], + policy, + changedPaths: PATHS, + sameChangeSet: matching, + }) + assert.equal(newerInc.ok, false) + assert.equal(newerInc.verdict, 'INC') + // …and the other way round: the newer of two carried conclusions is the one read. + const newerPass = decide({ + head: HEAD, + author: 'opener', + reports: [report({ tip: OLD, verdict: 'INC', at: '2026-10-25T09:00:00Z' }), report({ tip: OLDER, at: '2026-10-25T12:00:00Z' })], + policy, + changedPaths: PATHS, + sameChangeSet: matching, + }) + assert.equal(newerPass.ok, true) + assert.equal(newerPass.tip, OLDER) +}) + +test('#1317 acceptance 7: nothing else was loosened — a carried PASS with no Ran: still refuses', () => { + const r = decide({ head: HEAD, author: 'opener', reports: [report({ ran: '' })], policy, changedPaths: PATHS, sameChangeSet: matching }) + assert.equal(r.ok, false) + assert.match(r.reason, /lists no command it ran/) +}) + +test('#1317: a short tip names no head, so it is refused before any comparison', () => { + let called = false + const r = decide({ head: HEAD, author: 'opener', reports: [report({ tip: '175328d' })], policy, changedPaths: PATHS, sameChangeSet: () => { called = true; return { ok: true } } }) + assert.equal(r.ok, false) + assert.equal(called, false) + assert.match(r.reason, /7 characters/) +}) + +test('#1317: a caller that read no changed files gets the old refusal, never a pass', () => { + const r = decide({ head: HEAD, author: 'opener', reports: [report()], policy }) + assert.equal(r.ok, false) + assert.match(r.reason, /not the tip abaf938b0c1d/) +}) + +// --- acceptance 6: three readers, one answer, from the shared fixture --------------------------- + +const fixture = JSON.parse(readFileSync(fileURLToPath(new URL('./fixtures/workflow-cases.json', import.meta.url)), 'utf8')) +const policyFile = () => { + const path = join(mkdtempSync(join(tmpdir(), 'change-set-policy-')), 'review-policy.json') + writeFileSync(path, JSON.stringify(policy)) + return path +} +const POLICY_PATH = policyFile() +const loaded = loadPolicy(POLICY_PATH) + +// One repository built from a fixture entry: the pull request, its changed files, its PASS, and the two +// trees. The guard also reads the body rules and the checkpoint, so the body carries both. +const world = (entry) => { + const state = { head: entry.head, files: entry.paths, trees: entry.trees } + // The guard reads the API shapes, so the comment is one: `user.login`, `author_association`, + // `created_at`. `reports` is that same comment as `decide` reads it, for the third reader. + const comments = [{ id: 9001, user: { login: 'reviewer-agent', type: 'User' }, author_association: 'MEMBER', created_at: '2026-10-25T10:00:00Z', body: `Verdict: PASS at ${entry.reviewed}\nWritten-by: Reviewer\n\nRan: npm run verify → pass` }] + const reports = comments.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 })) + const body = [ + 'Owner: Dev3', + 'Reviewer: reviewer-agent', + `Fixes #${PR}`, + '', + '', + '```json', + JSON.stringify({ + pr: PR, + claim: 'Dev3', + issue: PR, + summary: 'The guard reads the change set before it voids a PASS. A repaired tip keeps the conclusion it was given. Everything else refuses exactly as before.', + files: [entry.paths[0]], + checks: { ran: 'node --test .github/scripts/*.test.mjs', result: 'green', at: entry.head }, + next: 'none — ready for review', + }, null, 2), + '```', + '', + ].join('\n') + const pull = { head: { sha: state.head, repo: { full_name: REPO } }, user: { login: 'opener' }, body, labels: [{ name: 'agent-pr' }], draft: false, merged: false } + const gh = (...args) => { + const listArgs = Array.isArray(args[0]) + const argv = listArgs ? args[0] : args.filter((x) => x !== undefined) + const path = String(argv.find((a) => String(a).startsWith('repos/')) ?? argv[argv.length - 1]) + const jq = argv.indexOf('--jq') >= 0 ? argv[argv.indexOf('--jq') + 1] : undefined + if (/\/pulls\/\d+$/.test(path)) return JSON.stringify(pull) + if (path.includes('/files')) return state.files.map((f) => JSON.stringify({ filename: f })).join('\n') + if (path.includes('/comments')) return comments.map((c) => JSON.stringify(c)).join('\n') + if (path.includes('/reviews')) return '' + const tree = /\/git\/trees\/([0-9a-f]+)/.exec(path) + if (tree) { + const sha = tree[1] + if (entry.unreadable === sha) { + const error = new Error('Command failed: gh api') + error.stderr = 'gh: Not Found (HTTP 404)' + throw error + } + const blobs = state.trees[sha] ?? {} + return JSON.stringify({ truncated: false, tree: Object.entries(blobs).map(([p, sha_]) => ({ path: p, type: 'blob', sha: sha_ })) }) + } + if (jq === '.[]') return '' + throw new Error(`fake gh: unexpected read ${path}`) + } + return { gh, pull, reports } +} + +const cases = (fixture.changeSetCases ?? []).filter((entry) => entry.readers === true) + +test('#1317 acceptance 6: the fixture carries the three-reader cases', () => { + assert.ok(cases.length >= 3, 'expected the allow, the changed-blob and the unreadable case in workflow-cases.json') +}) + +for (const entry of cases) { + test(`#1317 acceptance 6: decidePull, the enqueue-guard check and judge agree — ${entry.id}`, () => { + const { gh, reports } = world(entry) + const env = { GH_REPO: REPO, REVIEW_POLICY: POLICY_PATH } + // Reader 1: the guard's own decision (the printed command's source). + const decided = decidePull({ gh, env, repo: REPO, pr: PR }) + assert.equal(decided.decision.ok, entry.expect === 'allow') + assert.ok(decided.decision.message.includes(entry.sentence), `decidePull said ${decided.decision.message}`) + // Reader 2: the check run the ruleset sees — same decision object, same words. + const check = payloadFor({ decision: decided.decision, head: entry.head }) + assert.equal(check.conclusion, entry.expect === 'allow' ? 'success' : 'failure') + assert.ok(check.output.title.includes(entry.sentence), `check title said ${check.output.title}`) + // Reader 3: `judge`, called on its own the way merge-gate's queue entry calls it. + const judged = judge({ + labels: ['agent-pr'], + files: entry.paths, + head: entry.head, + author: 'opener', + reports, + policy: loaded, + gh, + repo: REPO, + pr: PR, + body: decided.pull.body, + }) + const words = judged.ok ? judged.verdict.reason : judged.reasons.join('; ') + assert.equal(judged.ok, entry.expect === 'allow') + assert.ok(words.includes(entry.sentence), `judge said ${words}`) + }) +} diff --git a/.github/scripts/enqueue-guard.mjs b/.github/scripts/enqueue-guard.mjs index 8d896c890..4c6f7af40 100644 --- a/.github/scripts/enqueue-guard.mjs +++ b/.github/scripts/enqueue-guard.mjs @@ -47,7 +47,7 @@ function arg(name) { // runs); `gate-check.mjs` turns the same object into the `enqueue-guard` check run the ruleset requires // (#812). One function with two readers, so the check and the printed command cannot disagree — #812's // kill condition is a second copy of these rules, and there is none. -export function decidePull({ gh = realGh, env = process.env, repo: repoOption, pr: prOption, expectedHead } = {}) { +export function decidePull({ gh = realGh, env = process.env, repo: repoOption, pr: prOption, expectedHead, sameChangeSet } = {}) { const list = (path) => gh('api', '--paginate', '--jq', '.[]', path).split('\n').filter(Boolean).map((line) => JSON.parse(line)) const repo = repoOption || env.GH_REPO || arg('--repo') if (!repo) throw new Error('set GH_REPO or pass --repo owner/name') @@ -85,7 +85,7 @@ export function decidePull({ gh = realGh, env = process.env, repo: repoOption, p const policy = loadPolicy(policyPath) // #1110: the same keys `merge-gate` reads, so the check and the printed command cannot disagree. const signers = loadSigners({ policy, path: arg('--signers') || env.VERDICT_SIGNERS || fileURLToPath(new URL('../agent/signers.json', import.meta.url)) }) - const result = judge({ labels, files, head, author: pull.user?.login, reports, policy, gh, repo, pr, signers, expectedHead: expectedHead ?? arg('--sha'), mode: 'queue', body: pull.body }) + const result = judge({ labels, files, head, author: pull.user?.login, reports, policy, gh, repo, pr, signers, expectedHead: expectedHead ?? arg('--sha'), mode: 'queue', body: pull.body, sameChangeSet }) const mechanicalRelease = isMechanicalRelease(files.map((f) => f.filename)) // The branch this repository owns, not a contributor's: `head.repo` is null when the fork was deleted, // which is not ours either. Both sides are lower-cased because the repository name in the API paths is @@ -105,7 +105,7 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prO 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: ALLOW — ${decision.message}; ${decision.exempt ? `a ${decision.tier ?? 'T0/T1'} tier or a mechanical release PR (AGENTS.md 6)` : decision.verdict?.carried ? 'the change set is the one that PASS names, so the tip moved without changing it' : `${decision.verdict?.by}'s PASS names this tip`}` : `enqueue-guard: REFUSE — ${decision.message}`) if (decision.ok) { console.log('enqueue-guard: this guard never enqueues; run the command yourself, now:') diff --git a/.github/scripts/enqueue-rules.mjs b/.github/scripts/enqueue-rules.mjs index 237c2a360..169ed8d22 100644 --- a/.github/scripts/enqueue-rules.mjs +++ b/.github/scripts/enqueue-rules.mjs @@ -307,9 +307,13 @@ export function guard({ pr, head, labels, body, files, mechanicalRelease, fork, merged: Boolean(merged), labels: labels ?? [], // One loud sentence: a refusal says what to do ("do not enqueue") and why, so it cannot be read - // as a warning that some other step may override. + // as a warning that some other step may override. #1317: when the PASS was written at an older tip + // whose change set is this head's, the allow line says so and names both shas — the reader has to + // be able to tell "the tree I reviewed" from "a tip that merely moved". message: ok - ? `enqueue #${pr}: no \`hold\` and the gate passes` + ? result.verdict?.carried + ? `enqueue #${pr}: no \`hold\` and the gate passes — ${result.verdict.by}'s PASS at ${result.verdict.tip} still describes this head ${result.head}: the change set is identical (the tip moved without changing a changed file)` + : `enqueue #${pr}: no \`hold\` and the gate passes` : `do not enqueue #${pr}: ${reasons.join('; ')}`, command: `gh pr merge ${pr} --auto --squash --match-head-commit ${result.head ?? head}`, } diff --git a/.github/scripts/fixtures/workflow-cases.json b/.github/scripts/fixtures/workflow-cases.json index 6e574bacd..693520a7c 100644 --- a/.github/scripts/fixtures/workflow-cases.json +++ b/.github/scripts/fixtures/workflow-cases.json @@ -8,20 +8,44 @@ "where": "merge-gate.mjs `judge`: the `hold` label refuses whatever the review says", "sentence": "the pull request carries the `hold` label", "gate": { - "labels": ["hold"], + "labels": [ + "hold" + ], "head": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", "expectedHead": null, - "reports": [{ "verdict": "PASS", "tip": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", "writtenBy": "Reviewer" }] + "reports": [ + { + "verdict": "PASS", + "tip": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", + "writtenBy": "Reviewer" + } + ] }, "preset": { "node": "gate", "guard": "no-hold", "present": [ - { "spec": "pr", "revision": "1274@abaf938b0c1d", "value": "hold" }, - { "spec": "verdict", "revision": "abaf938b0c1d", "value": "PASS", "by": "Reviewer" }, - { "spec": "check:tests-ok", "revision": "abaf938b0c1d" } + { + "spec": "pr", + "revision": "1274@abaf938b0c1d", + "value": "hold" + }, + { + "spec": "verdict", + "revision": "abaf938b0c1d", + "value": "PASS", + "by": "Reviewer" + }, + { + "spec": "check:tests-ok", + "revision": "abaf938b0c1d" + } ], - "current": { "pr": "1274@abaf938b0c1d", "verdict": "abaf938b0c1d", "check:tests-ok": "abaf938b0c1d" } + "current": { + "pr": "1274@abaf938b0c1d", + "verdict": "abaf938b0c1d", + "check:tests-ok": "abaf938b0c1d" + } } }, { @@ -32,17 +56,39 @@ "labels": [], "head": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", "expectedHead": "175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90", - "reports": [{ "verdict": "PASS", "tip": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", "writtenBy": "Reviewer" }] + "reports": [ + { + "verdict": "PASS", + "tip": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", + "writtenBy": "Reviewer" + } + ] }, "preset": { "node": "gate", "guard": "verdict-at-tip", "present": [ - { "spec": "pr", "revision": "1274@abaf938b0c1d", "value": "none" }, - { "spec": "verdict", "revision": "175328d0c1b2", "value": "PASS", "by": "Reviewer" }, - { "spec": "check:tests-ok", "revision": "abaf938b0c1d" } + { + "spec": "pr", + "revision": "1274@abaf938b0c1d", + "value": "none" + }, + { + "spec": "verdict", + "revision": "175328d0c1b2", + "value": "PASS", + "by": "Reviewer" + }, + { + "spec": "check:tests-ok", + "revision": "abaf938b0c1d" + } ], - "current": { "pr": "1274@abaf938b0c1d", "verdict": "abaf938b0c1d", "check:tests-ok": "abaf938b0c1d" } + "current": { + "pr": "1274@abaf938b0c1d", + "verdict": "abaf938b0c1d", + "check:tests-ok": "abaf938b0c1d" + } } }, { @@ -53,18 +99,108 @@ "labels": [], "head": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", "expectedHead": null, - "reports": [{ "verdict": "INC", "tip": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", "writtenBy": "Reviewer" }] + "reports": [ + { + "verdict": "INC", + "tip": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", + "writtenBy": "Reviewer" + } + ] }, "preset": { "node": "gate", "guard": "verdict-says-pass", "present": [ - { "spec": "pr", "revision": "1274@abaf938b0c1d", "value": "none" }, - { "spec": "verdict", "revision": "abaf938b0c1d", "value": "INC", "by": "reviewer-agent" }, - { "spec": "check:tests-ok", "revision": "abaf938b0c1d" } + { + "spec": "pr", + "revision": "1274@abaf938b0c1d", + "value": "none" + }, + { + "spec": "verdict", + "revision": "abaf938b0c1d", + "value": "INC", + "by": "reviewer-agent" + }, + { + "spec": "check:tests-ok", + "revision": "abaf938b0c1d" + } ], - "current": { "pr": "1274@abaf938b0c1d", "verdict": "abaf938b0c1d", "check:tests-ok": "abaf938b0c1d" } + "current": { + "pr": "1274@abaf938b0c1d", + "verdict": "abaf938b0c1d", + "check:tests-ok": "abaf938b0c1d" + } } } + ], + "changeSetCases": [ + { + "id": "tip-moved-same-change-set", + "why": "#1304's first and third PASS: the tip moved only because main moved, and every changed path carries the same blob at both tips. The conclusion written at the old tip still describes the head.", + "head": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", + "reviewed": "175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90", + "paths": [ + "core/src/host/turns.js", + "core/test/gate-check.test.js" + ], + "trees": { + "175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90": { + "core/src/host/turns.js": "blob-host", + "core/test/gate-check.test.js": "blob-test" + }, + "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f": { + "core/src/host/turns.js": "blob-host", + "core/test/gate-check.test.js": "blob-test" + } + }, + "expect": "allow", + "readers": true, + "sentence": "reviewer-agent's PASS at 175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90 still describes this head abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f" + }, + { + "id": "tip-moved-one-blob-changed", + "why": "#1304's second PASS (d7bd1c1d): an update-branch left core/package.json at 0.2.9 while main had 0.2.10. One blob differs, so the PASS does not describe the head — the case that proves the rule must stay.", + "head": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", + "reviewed": "175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90", + "paths": [ + "core/package.json", + "core/src/host/turns.js" + ], + "trees": { + "175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90": { + "core/package.json": "blob-package-0.2.9", + "core/src/host/turns.js": "blob-host" + }, + "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f": { + "core/package.json": "blob-package-0.2.10", + "core/src/host/turns.js": "blob-host" + } + }, + "expect": "refuse", + "readers": true, + "sentence": "the change set differs at core/package.json" + }, + { + "id": "tip-moved-tree-unreadable", + "why": "The old tip is gone (404) or its tree came back truncated: the comparison fails closed with the read that failed, never \"assume unchanged\".", + "head": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", + "reviewed": "175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90", + "unreadable": "175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90", + "paths": [ + "core/src/host/turns.js", + "core/test/gate-check.test.js" + ], + "trees": { + "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f": { + "core/src/host/turns.js": "blob-host", + "core/test/gate-check.test.js": "blob-test" + } + }, + "expect": "refuse", + "readers": true, + "sentence": "the change set could not be read (git tree of 175328d0c1b2: 404)" + } ] } diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index 363aacc16..d336ff9a2 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -101,7 +101,7 @@ const short = (s) => String(s ?? '').slice(0, SHORT) // expectedHead: optional, a head the caller insists on (`--sha` on a hand run); a different live head // refuses. The queue's base sha is never a head: a merge_group entry passes none here (see // prFromHeadRef). reports: its comments and review bodies, oldest first (review-gate main()). -export function judge({ labels, files, head, author, reports, policy, expectedHead, mode = 'queue', admins, gh, repo, body, owners, pr, signers } = {}) { +export function judge({ labels, files, head, author, reports, policy, expectedHead, mode = 'queue', admins, gh, repo, body, owners, pr, signers, sameChangeSet } = {}) { const reasons = [] const target = String(head ?? '').toLowerCase() if (!target) return { ok: false, head: '', verdict: null, reasons: ['no head sha to judge'] } @@ -121,7 +121,7 @@ export function judge({ labels, files, head, author, reports, policy, expectedHe // A tier that needs no PASS also needs no permission lookup. const verdict = exempt ? { ok: true, verdict: null, by: null, exempt: true } - : decide({ head: target, author, reports, policy, body, owners, admins: admins ?? adminsFor({ gh, repo, reports, policy, author }), repo, pr, signers }) + : decide({ head: target, author, reports, policy, body, owners, admins: admins ?? adminsFor({ gh, repo, reports, policy, author }), repo, pr, signers, gh, changedPaths: fileNames(files), sameChangeSet }) if (!verdict.ok) reasons.push(verdict.reason) // A report says what the queue entry would refuse; only the queue entry refuses. Fail a // `pull_request` run and it keeps failing after the INC it names is answered by a PASS, because a @@ -142,11 +142,13 @@ export function render(result, { pr, labels } = {}) { ? `A queue entry for this head would be refused: ${result.reasons.join('; ')}. This report never fails.` : result.exempt ? `A queue entry for this head would pass without a PASS: ${result.tier ? 'a ' + result.tier : 'a T0/T1'} tier or a mechanical release PR (AGENTS.md 6), computed from the changed files.` - : `A queue entry for this head would pass: ${result.verdict?.by}'s PASS names ${short(result.head)}.` + : `A queue entry for this head would pass: ${result.verdict?.by}'s PASS ${result.verdict?.carried ? `at ${result.verdict.tip} still describes ${short(result.head)} — the change set is identical` : `names ${short(result.head)}`}.` : result.ok && result.exempt ? `Queue entry for #${pr} may merge without a PASS: ${result.reasons.length ? 'no \`hold\`' : `${result.tier ? 'a ' + result.tier : 'a T0/T1'} tier or a mechanical release PR (AGENTS.md 6), computed from the changed files`}.` : result.ok - ? `Queue entry for #${pr} may merge: no \`hold\`, ${result.verdict?.by}'s PASS names ${short(result.head)}.` + ? result.verdict?.carried + ? `Queue entry for #${pr} may merge: no \`hold\`, ${result.verdict.by}'s PASS at ${result.verdict.tip} still describes ${short(result.head)} — the change set is identical (every changed path carries the same blob).` + : `Queue entry for #${pr} may merge: no \`hold\`, ${result.verdict?.by}'s PASS names ${short(result.head)}.` : `Queue entry for #${pr} must not merge: ${result.reasons.join('; ')}.`, '', `Pull request #${pr}, head \`${result.head}\`, labels: ${(labels ?? []).join(', ') || '(none)'}.`, diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index a6bb265e7..fa974b1ef 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -30,13 +30,18 @@ // `hasNotRun` is still parsed for the summary, and a PASS without it is not refused. // The newest qualifying report that names this tip decides, so a later FAIL at the same tip // vetoes an earlier PASS. A tip that moves past the report is not reviewed: the report names -// the whole 40-character sha (#1112), so an older head stops counting. +// the whole 40-character sha (#1112), so an older head stops counting — unless the tip moved +// without changing the change set (#1317): when no report names the head itself, a report whose +// `(path → blob sha)` fingerprint at the pull request's changed paths is identical at both tips +// still describes this head, and `change-set.mjs` answers that. It fails closed, so an unreadable +// tree or a changed blob is the same refusal as before. // // The pull request is read only through the GitHub API; no PR code is checked out or run. import { readFileSync, appendFileSync } from 'node:fs' import { execFileSync } from 'node:child_process' import { fileURLToPath } from 'node:url' import { SIGNERS_PATH, loadSigners, verifyReport } from './verdict-signature.mjs' +import { sameChangeSet as sameChangeSetOf } from './change-set.mjs' const SHORT = 12 // #676: a conclusion is read only from someone GitHub ties to this repository. The fail-closed rule @@ -300,7 +305,11 @@ function signingState({ policy, signers, pr }) { // is non-empty every conclusion must verify against it (#1110); an unverified one is not a conclusion and // never joins the newest-wins scan, so it can neither pass nor veto. `repo` and `pr` are signed into the // report, so a signature cannot move to another pull request or another tip. -export function decide({ head, author, reports, policy, admins, body, owners, repo, pr, signers } = {}) { +// #1317: `changedPaths` is the pull request's changed-file list, read by the caller (the guard and this +// script's own `main` both read it); `sameChangeSet` is the injected comparison, so a test drives it with +// a stub and no network. Both are optional: without the list there is nothing to compare, and the tip +// rule refuses exactly as it did before — the conservative answer. +export function decide({ head, author, reports, policy, admins, body, owners, repo, pr, signers, changedPaths, sameChangeSet = sameChangeSetOf, gh } = {}) { const headSha = String(head ?? '').toLowerCase() const authorLogin = String(author ?? '').toLowerCase() const ownersKnown = owners !== undefined || body !== undefined @@ -330,10 +339,25 @@ export function decide({ head, author, reports, policy, admins, body, owners, re // 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.tip.length < 40) { + reasons.push(`${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`) + continue + } + // #1317: a tip that moved without changing the reviewed change set keeps its conclusion. The + // comparison is the caller's changed-path list against the trees at both tips, and it fails + // closed: no list, an unreadable tree or one changed blob is the refusal this gate always gave, + // with the reason named so a reader can tell "the code changed" from "I could not look". + const moved = changedPaths && changedPaths.length + ? sameChangeSet({ gh, repo, pr, reviewed: parsed.tip, head: headSha, paths: changedPaths }) + : { ok: false, read: 'the changed files of this pull request were not read' } + if (!moved.ok) { + const why = moved.path + ? ` — the change set differs at ${moved.path}, so that conclusion does not describe this head` + : ` — the change set could not be read (${moved.read})` + reasons.push(`${where(r)}: names ${parsed.tip.slice(0, SHORT)}, not the tip ${headSha.slice(0, SHORT)}${why}`) + continue + } + entry.carriedFrom = parsed.tip } // #1239: an unreadable word is not "no verdict" — it is a conclusion at this tip that nobody can // read, and the newest one has to be able to veto an older PASS. @@ -408,22 +432,42 @@ export function decide({ head, author, reports, policy, admins, body, owners, re return { ok: false, verdict: null, by: null, tip: null, reason, judged, unverified } } + // #1317: a conclusion that names the head itself always decides, and the carried ones are read only + // when the head carries none. That is what keeps an INC at the head a veto: a carried PASS can never + // outrank a conclusion written at this tip, whatever the two timestamps say. + const exact = atTip.filter((e) => !e.carriedFrom) + const candidates = exact.length ? exact : atTip // The newest word at this tip decides, and that includes a conclusion the checks refused: an invalid // PASS is not evidence, and an invalid INC is still someone saying not to merge (#676). Equal // timestamps fall back to the order the API returned them in (oldest first), so the later one wins. - const last = atTip + const last = candidates .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, id: last.id, kind: last.kind, reason: last.why, judged, unverified } const { parsed } = last if (last.verdict !== 'PASS') { - 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, unverified } + return { ok: false, verdict: last.verdict, by: last.login, tip: last.tip, id: last.id, kind: last.kind, carried: Boolean(last.carriedFrom), reason: `${last.login} says ${last.verdict ?? 'nothing'} at ${last.carriedFrom ? `the change set of this tip (written at ${last.tip.slice(0, SHORT)})` : 'this tip'}`, judged, unverified } } if (!parsed.ran.length) { 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, unverified } } - 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, unverified } + // #1317: the allow sentence names both full shas and the fact that the comparison was made, so a reader + // never has to guess whether the guard looked at the trees or simply found a PASS. + return { + ok: true, + verdict: 'PASS', + by: last.login, + tip: last.tip, + id: last.id, + kind: last.kind, + carried: Boolean(last.carriedFrom), + reason: last.carriedFrom + ? `${last.login}'s PASS at ${last.tip} still describes this head ${headSha}: the change set is identical (every changed path carries the same blob at both tips)` + : `${last.login}'s PASS names this tip`, + judged, + unverified, + } } export function render(result, { pr, head, author } = {}) { @@ -491,11 +535,15 @@ function main() { ] const policy = loadPolicy(policyPath) + // #1317: the changed-file list, read here so a tip that moved only because `main` moved can be told + // apart from one whose code changed. `decide` compares `(path → blob sha)` at both tips and refuses + // when the list or a tree cannot be read, so this read failing is a refusal, not a shortcut. + const changedPaths = list(`repos/${repo}/pulls/${pr}/files?per_page=100`).map((f) => f.filename).filter(Boolean) // #1110: the keys come from the same checkout the policy does, so a pull request that edits either file // changes only what its own run reads. `loadSigners` is fail-closed on its own; `decide` refuses when the // policy lists a roster and this is missing. const signers = loadSigners({ policy, path: arg('--signers') || process.env.VERDICT_SIGNERS || fileURLToPath(new URL('../agent/signers.json', import.meta.url)) }) - const result = decide({ head, author, reports, policy, body: pull.body, admins: adminsFor({ gh, repo, reports, policy, author }), repo, pr, signers }) + const result = decide({ head, author, reports, policy, body: pull.body, admins: adminsFor({ gh, repo, reports, policy, author }), repo, pr, signers, gh, changedPaths }) console.log(`review-gate (report only): PR #${pr} tip ${String(head).slice(0, SHORT)} — ${result.ok ? `PASS by ${result.by}` : result.reason}`) const text = render(result, { pr, head, author }) if (process.env.GITHUB_STEP_SUMMARY) appendFileSync(process.env.GITHUB_STEP_SUMMARY, text + '\n') diff --git a/AGENTS.md b/AGENTS.md index 43c63190a..20128b1c3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -31,7 +31,7 @@ A person talks to the **Chief** (the Main Bot). The Chief hands the work to the 6. **Gate, then queue.** Five checks, in this order. Each one can refuse. 1. **Tier.** `node .github/scripts/risk-gate.mjs --pr `, or quote the `risk-tier:` line of the job summary. **T0/T1** need green `tests-ok` at the tip, a mergeable pull request and design-conformance — no PASS. **T2/T3** need a **PASS at the tip SHA** written by the Reviewer ([`review-pr`](.claude/skills/review-pr/SKILL.md): PASS / FAIL / INC), never by the implementer ([Merge gates](#merge-gates)). Neither a PASS nor green CI replaces the other. - 2. **One conclusion per tip.** A tip carries at most one `Verdict:` block. A second conclusion at the same tip is an incident, not a better answer; a new commit voids the PASS; a correction reads `Correction from : …` and is not a verdict. + 2. **One conclusion per tip.** A tip carries at most one `Verdict:` block. A second conclusion at the same tip is an incident, not a better answer; a new commit voids the PASS **unless it changes no file of the reviewed change set** — a tip that moved only because `main` was merged in keeps it, and the guard names the sha it accepted and why (#1317); a correction reads `Correction from : …` and is not a verdict. 3. **Enqueue.** `node .github/scripts/enqueue-guard.mjs --pr `, then `gh pr merge --auto --squash --match-head-commit `. The guard re-reads the live head and refuses (exit 1) on a `hold`, on a newest conclusion that is not a PASS, and on a report marked "record only — do not merge", so a PASS is never a queue permit on its own (#602). `main` requires a merge queue: the command enqueues the pull request (gh may answer `The merge strategy for main is set by the merge queue` or `already queued`), the queue performs the squash merge, and a plain `--squash` merge is refused — do not retry ([merge plumbing](docs/autonomous-dev.md#merge-plumbing-merge_group-merge-queue-auto-merge)). 4. **Judge on the merge tree.** Whether the pull request can merge is decided on the queue entry — the pull request merged onto the current `main`, built and tested again — never on the pull request page. The page's green is the head alone: #759 was 9/9 green, was kicked out of the queue twice, and went red on `main` at `core/test/follow-through.test.js:264`. 5. **Never bypass.** No admin-merge, no bypass, no ruleset change. A `Docs-not-needed:` line goes in the PR body as well as in the branch commit, and you check both at the tip ([why, and how](#docs-not-needed-why-both-places)). Run `verify` with `TZ=UTC`. @@ -171,7 +171,7 @@ What this buys is presence and a name from a closed list, not truth: with one sh - **T0 / T1:** merge through the merge queue once `tests-ok` is green at the tip, the PR is mergeable and design-conformant. No PASS is required. - **T2 / T3:** a PASS at the head commit from a reviewer in [`.github/agent/review-policy.json`](.github/agent/review-policy.json), with green CI, and the agent that gated it then enqueues it itself ([self-gate](#owner-decision-the-bot-gates-and-merges-its-own-prs-2026-10-21-owner-authorized)). **T2 and T3 need no maintainer approve** (user decision 2026-10-22): no human `APPROVED` review is read or required, and `merge-gate` re-judges the head on the queue entry. **Self-PASS across Bots counts:** a PR opened by the owner's Bot/agent (`Birfy`) may be PASSed by the owner's own reviewer Bot/agent (the policy's `selfReview` list) — the shared account is why that list exists, not a licence for an implementer to review its own pull request. Neither the PASS nor green CI replaces the other. When that PASS is posted by the pull request author's own GitHub login, it also carries the attestation in [Merge gates](#merge-gates) (#644). - **A PASS must list what it ran.** The `Ran:` line (or a `Ran …:` heading over `$ ` commands) carrying at least one real command is part of the gate, not a nicety: `merge-gate` refuses a PASS that lists none (#681, #574). When a PASS at a tip cannot be read for that reason, the reviewer may post **one replacement PASS at the same tip**: its body keeps the original conclusion, adds the `Ran:` lines of what was actually run, and carries `Replaces: the PASS at merge-gate rejected for having no Ran: line`. That is a replacement, not a second conclusion — apart from it, one tip still carries one. -- Any new commit voids a PASS. CI green, the merge queue and the no-bypass rule apply to every tier. +- Any new commit voids a PASS, unless it changes no file of the reviewed change set: a tip that moved only by merging `main` keeps the conclusion, and the gate names both shas and the comparison it made (#1317). CI green, the merge queue and the no-bypass rule apply to every tier. - Agents never bypass a gate and never admin-merge (the routines run as the maintainer account `Birfy`, but use it only for the actions above, never for admin merge, bypass or changing protection rules). Run `verify` with `TZ=UTC`. ### Merge gates @@ -200,7 +200,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 — 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:`. +**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). **A tip that moved without changing the reviewed change set keeps its conclusion** (#1317): when no conclusion names the head itself, the gate compares `(path → blob sha)` at the reviewed tip and at the head over the pull request's changed paths, and accepts the PASS only when every one matches — naming both shas and the comparison it made — while a changed blob, a path missing on either side, a truncated tree or an unreadable one refuses exactly as before. A conclusion that names the head always decides, so an INC written at the head still vetoes a carried PASS. 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.