From 387b13cc196b18898542445932fd73928f8515e3 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Thu, 22 Oct 2026 12:37:20 +0800 Subject: [PATCH 1/4] fix(agents): refuse a report that does not say who wrote it (#627) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every state-changing write now carries `Written-by: ` as its first line, and a conclusion puts it on the second line under the fixed `Verdict: ... at ` line (#637). review-gate's `decide` — the rule merge-gate reuses at the queue — refuses a report with no such line and, separately, one whose name is not in the policy's new `writers` list. The gate guarantees presence and a name from a closed list, not truth: with one shared GitHub identity the line is a self-report (#633). The issue named `reviewer-queue.mjs`, which does not exist; the verdict reader is `.github/scripts/review-gate.mjs`, so the checks live there. --- .claude/skills/review-pr/SKILL.md | 7 ++- .github/agent/review-policy.json | 17 +++++++- .github/scripts/merge-gate.test.mjs | 12 ++--- .github/scripts/review-gate.mjs | 34 +++++++++++++-- .github/scripts/review-gate.test.mjs | 65 +++++++++++++++++++++++++--- AGENTS.md | 14 +++++- docs/autonomous-dev.md | 12 +++-- 7 files changed, 138 insertions(+), 23 deletions(-) diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index b27adbf57..c899f7e7f 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -35,7 +35,8 @@ Always include: ```text Verdict: PASS | FAIL | INC at -Written by: Reviewer. Posted by: . +Written-by: Reviewer +Posted-by: Ran: - → pass|fail @@ -45,7 +46,9 @@ Not run: ``` The first line is the whole conclusion (#637): nothing may come before it, because the gates read the -first line and treat any other shape as no conclusion. +first line and treat any other shape as no conclusion. The `Written-by:` line is the attribution every write carries (#627): it names a Bot from the policy's +`writers`, the gates refuse a report without it, and it is a self-report, not proof — see +[AGENTS.md](../../../AGENTS.md#every-state-changing-write-says-who-wrote-it-627), which is the single source. What the gates read as evidence (`review-gate.mjs` `parseReport`; #660–#663 refused at the queue for the first point): diff --git a/.github/agent/review-policy.json b/.github/agent/review-policy.json index e66348c65..648a8cb9d 100644 --- a/.github/agent/review-policy.json +++ b/.github/agent/review-policy.json @@ -1,6 +1,19 @@ { "version": 1, - "note": "Who may record a PASS that counts, for the `reviewed tip` report (Phase 1: report only; see docs/autonomous-dev.md). A login listed here is not a merge right: the report is read when a tip is judged, and a PASS is required only for T2/T3 (at the tip, with green CI); T0/T1 merge on green CI without one (AGENTS.md step 6). A pull request's own author does not count unless the author is listed in `selfReview` (owner decision 2026-10-19: the owner's Bot/agent, which acts as Birfy, may PASS its own pull requests). A self-PASS never replaces green CI: the agent that gated a T2/T3 pull request merges it itself once a PASS sits at its tip (owner decision 2026-10-21). Editing this file is itself T3.", + "note": "Who may record a PASS that counts, for the `reviewed tip` report (Phase 1: report only; see docs/autonomous-dev.md). A login listed here is not a merge right: the report is read when a tip is judged, and a PASS is required only for T2/T3 (at the tip, with green CI); T0/T1 merge on green CI without one (AGENTS.md step 6). A pull request's own author does not count unless the author is listed in `selfReview` (owner decision 2026-10-19: the owner's Bot/agent, which acts as Birfy, may PASS its own pull requests). A self-PASS never replaces green CI: the agent that gated a T2/T3 pull request merges it itself once a PASS sits at its tip (owner decision 2026-10-21). Editing this file is itself T3. `writers` lists the names a report's `Written-by:` line may carry (#627): every Bot shares one GitHub identity, so that line is a claim, and the gate refuses a write whose line is missing or names something outside this list.", "reviewers": ["hsliuustc0106", "Birfy"], - "selfReview": ["Birfy"] + "selfReview": ["Birfy"], + "writers": [ + "Chief", + "Triage", + "Reviewer", + "Release", + "SelfTest", + "Maintainer", + "Dev", + "Dev2", + "Dev3", + "Dev4", + "Birfy" + ] } diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index 0005b27ad..7df31bd69 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -8,10 +8,10 @@ import { fileURLToPath } from 'node:url' const HEAD = 'abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f' const OLD = '175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90' const ENTRY = `gh-readonly-queue/main/pr-566-${HEAD}` -const policy = { reviewers: ['reviewer-agent'], selfReview: [] } +const policy = { reviewers: ['reviewer-agent'], selfReview: [], writers: ['Reviewer'] } const report = ({ tip = HEAD, verdict = 'PASS' } = {}) => - [`Tip SHA: ${tip}`, 'Ran:', ' - npm run verify → pass', 'Not run:', ' - smoke — host code unchanged', `Verdict: ${verdict}`].join('\n') + [`Tip SHA: ${tip}`, 'Written-by: Reviewer', 'Ran:', ' - npm run verify → pass', 'Not run:', ' - smoke — host code unchanged', `Verdict: ${verdict}`].join('\n') const say = (body = report(), at = '2026-10-21T10:31:00Z', kind = 'comment') => ({ login: 'reviewer-agent', body, at, kind }) @@ -142,7 +142,7 @@ const fakeGh = ({ comments, reviews = [], labels = [], files = [], head = HEAD } const runGate = (fixture) => { const policy = join(mkdtempSync(join(tmpdir(), 'merge-gate-policy-')), 'review-policy.json') - writeFileSync(policy, JSON.stringify({ reviewers: ['reviewer-agent'], selfReview: [] })) + writeFileSync(policy, JSON.stringify({ reviewers: ['reviewer-agent'], selfReview: [], writers: ['Reviewer'] })) return main({ gh: fakeGh(fixture), env: { GITHUB_STEP_SUMMARY: '', REVIEW_POLICY: policy }, @@ -218,7 +218,7 @@ test('#639: a report names what the entry would refuse and still passes', () => test('#639: the pull_request entry point reports without a head_ref and does not throw', () => { const policy = join(mkdtempSync(join(tmpdir(), 'merge-gate-policy-')), 'review-policy.json') - writeFileSync(policy, JSON.stringify({ reviewers: ['reviewer-agent'], selfReview: [] })) + writeFileSync(policy, JSON.stringify({ reviewers: ['reviewer-agent'], selfReview: [], writers: ['Reviewer'] })) const result = main({ gh: fakeGh({ comments: [apiComment(report({ verdict: 'INC' }), '2026-10-21T12:49:00Z')] }), env: { GITHUB_STEP_SUMMARY: '', REVIEW_POLICY: policy, MODE: 'report', PR: '639' }, @@ -273,7 +273,7 @@ const RUN_FILES = ['core/src/host/turns.js', 'changelog.d/651.fixed.md'] const runQueue = (fixture) => { const policyFile = join(mkdtempSync(join(tmpdir(), 'merge-gate-policy-')), 'review-policy.json') - writeFileSync(policyFile, JSON.stringify({ reviewers: ['reviewer-agent'], selfReview: [] })) + writeFileSync(policyFile, JSON.stringify({ reviewers: ['reviewer-agent'], selfReview: [], writers: ['Reviewer'] })) // Exactly what merge-gate.yml hands the script on a merge_group run, and no PR number. return main({ gh: fakeGh({ head: RUN_HEAD, files: RUN_FILES, ...fixture }), @@ -324,7 +324,7 @@ test('#654: a T1 queue entry passes without a PASS, as the pull_request report s assert.equal(queue.ok, true, queue.reasons.join('; ')) assert.equal(queue.tier, 'T1') const policyFile = join(mkdtempSync(join(tmpdir(), 'merge-gate-policy-')), 'review-policy.json') - writeFileSync(policyFile, JSON.stringify({ reviewers: ['reviewer-agent'], selfReview: [] })) + writeFileSync(policyFile, JSON.stringify({ reviewers: ['reviewer-agent'], selfReview: [], writers: ['Reviewer'] })) const report = main({ gh: fakeGh({ head: RUN_HEAD, files, comments: [] }), env: { GITHUB_STEP_SUMMARY: '', REVIEW_POLICY: policyFile, GH_REPO: 'ThinkFlowLab/FlyBot', PR: '654', MODE: 'report' } }) assert.deepEqual(report.reasons, queue.reasons, 'both events judge the same head the same way') assert.equal(report.head, queue.head) diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index 871bd6ac9..e41133e5a 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -13,6 +13,9 @@ // its own pull requests; any other author still needs someone else); // - it carries the Verifier block review-pr already writes, with a `Tip SHA:` that is the // head being judged and a `Verdict:`; +// - it names its writer in the header: `Written-by: ` (the older prose `Written by: …` reads +// the same way) with a name from the policy's `writers`, or it refuses — a missing line and an +// unknown name are separate refusals (#627); // - the verdict is PASS and the report lists a command it `Ran:` (a `- ` bullet, or a `$ ` line in a // fenced block under a `Ran …:` heading): a report that ran nothing is not evidence. // - `Not run:` is recommended but not checked (owner decision 2026-10-22 09:57 Asia/Shanghai, #667): @@ -35,7 +38,7 @@ export function parseReport(body) { // markdown emphasis, line for line, for everything else. const raw = String(body ?? '').split('\n') const lines = raw.map((line) => line.replace(/[*`]/g, '')) - const out = { tip: null, verdict: null, ran: [], hasNotRun: false } + const out = { tip: null, verdict: null, ran: [], hasNotRun: false, writer: null } // The fixed format (#637): the first non-empty line is the whole conclusion, // `Verdict: PASS at ` (or INC / REQUEST_CHANGES). The attribution line and the evidence go // below it, so a verdict Chief posts for a Reviewer with no write channel parses the same way. @@ -80,9 +83,27 @@ export function parseReport(body) { } if (/^Not run\b[^:\n]*:/i.test(line)) out.hasNotRun = true } + out.writer = parseWriter(lines) return out } +// #627: the attribution line, `Written-by: ` (the older prose `Written by: Reviewer. Posted by: +// …` reads the same way). Only the header is read — the first few non-empty lines — so a report that +// arrives without one does not borrow a writer from a block it quotes further down. +const HEADER_LINES = 3 +function parseWriter(lines) { + let seen = 0 + for (const line of lines) { + const text = line.trim() + if (!text) continue + if (++seen > HEADER_LINES) break + const written = /^Written[- ]by\s*:\s*(.+)$/i.exec(text) + // The first clause is the name: `Reviewer. Posted by: Chief (proxy…)` names Reviewer. + if (written) return written[1].split(/[.,(;]/)[0].trim() || null + } + return null +} + // The commands under a `Ran` heading at line `at`: the inline text of a bare `Ran: ` (never // an empty one or `-`), then either the `- ` bullets that follow, or the `$ ` lines of the fenced // block that follows. Output lines in the block are not commands, so a block with no `$ ` line, or @@ -117,19 +138,26 @@ export function decide({ head, author, reports, policy }) { const authorLogin = String(author ?? '').toLowerCase() const reviewers = new Set((policy?.reviewers ?? []).map((l) => String(l).toLowerCase())) const selfReview = new Set((policy?.selfReview ?? []).map((l) => String(l).toLowerCase())) + const writers = new Set((policy?.writers ?? []).map((w) => String(w).toLowerCase())) const reasons = [] const judged = [] 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, parsed, counts: false } + const entry = { login: r.login, verdict: parsed.verdict, tip: parsed.tip, at: r.at, kind: r.kind, writer: parsed.writer, parsed, counts: false } 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)}`) continue } + // #627: who wrote it, refused on two separate grounds — a report with no attribution line and one + // that names a writer the policy does not list are different defects, and the log has to say which + // one fired. Both fail closed. With one shared GitHub identity the line is a self-report, so this + // buys presence and a name from a closed list, not proof (#633). + if (!parsed.writer) { reasons.push(`${where(r)}: no Written-by line naming its writer`); continue } + if (!writers.has(parsed.writer.toLowerCase())) { reasons.push(`${where(r)}: Written-by names "${parsed.writer}", who is not in the writers list of .github/agent/review-policy.json`); continue } if (login(r) === authorLogin && !selfReview.has(authorLogin)) { reasons.push(`${where(r)}: the pull request's own author is not a reviewer (not in selfReview)`); continue } if (!reviewers.has(login(r))) { reasons.push(`${where(r)}: not a reviewer listed in .github/agent/review-policy.json`); continue } entry.counts = true @@ -170,7 +198,7 @@ export function render(result, { pr, head, author } = {}) { `Pull request #${pr}, author ${author}, head \`${String(head).slice(0, SHORT)}\`.`, ] const seen = result.judged.filter((e) => e.counts || e.verdict) - if (seen.length) out.push('', 'Verifier reports read:', ...seen.slice(0, 10).map((e) => `- ${e.login} (${e.kind}): ${e.verdict ?? 'no verdict'} at ${(e.tip ?? 'no tip').slice(0, SHORT)}${e.counts ? '' : ' (not counted)'}`)) + if (seen.length) out.push('', 'Verifier reports read:', ...seen.slice(0, 10).map((e) => `- ${e.login} (${e.kind}): ${e.verdict ?? 'no verdict'} at ${(e.tip ?? 'no tip').slice(0, SHORT)}, written-by ${e.writer ?? 'nothing'}${e.counts ? '' : ' (not counted)'}`)) out.push('', '_Report only (Phase 1): this does not block, label, approve or merge anything. Rules: `.github/agent/review-policy.json`; see [`docs/autonomous-dev.md`](../../docs/autonomous-dev.md)._') return out.join('\n') } diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index dace7e0bd..a1429caef 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -6,11 +6,12 @@ import { decide, loadPolicy, parseReport, render } from './review-gate.mjs' const HEAD = 'a4d506a1b2c3d4e5f60718293a4b5c6d7e8f9012' const OLD = '175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90' -const policy = { reviewers: ['reviewer-agent', 'hsliuustc0106'] } +const policy = { reviewers: ['reviewer-agent', 'hsliuustc0106'], writers: ['Reviewer', 'Chief'] } const report = ({ tip = HEAD, verdict = 'PASS', ran = [' - npm run verify → pass'], notRun = true } = {}) => [ `Tip SHA: ${tip}`, + 'Written-by: Reviewer', 'Ran:', ...ran, ...(notRun ? ['Not run:', ' - smoke — host code unchanged'] : []), @@ -46,7 +47,7 @@ test('the pull request author does not review their own tip unless listed in sel }) test('self-review: a listed author may PASS their own pull request at the tip (owner decision 2026-10-19)', () => { - const selfPolicy = { reviewers: ['reviewer-agent', 'Birfy'], selfReview: ['Birfy'] } + const selfPolicy = { reviewers: ['reviewer-agent', 'Birfy'], selfReview: ['Birfy'], writers: ['Reviewer', 'Birfy'] } const r = decide({ head: HEAD, author: 'Birfy', reports: [comment('Birfy')], policy: selfPolicy }) assert.equal(r.ok, true) assert.equal(r.by, 'Birfy') @@ -54,7 +55,7 @@ test('self-review: a listed author may PASS their own pull request at the tip (o }) test('self-review keeps every other rule: tip, evidence, listed reviewer, newest wins', () => { - const selfPolicy = { reviewers: ['reviewer-agent', 'Birfy'], selfReview: ['Birfy'] } + const selfPolicy = { reviewers: ['reviewer-agent', 'Birfy'], selfReview: ['Birfy'], writers: ['Reviewer', 'Birfy'] } const own = (body, at) => comment('Birfy', body, at) const run = (reports, policy = selfPolicy) => decide({ head: HEAD, author: 'Birfy', reports, policy }) assert.equal(run([own(report({ tip: OLD }))]).ok, false, 'a new commit voids the self-PASS') @@ -62,7 +63,7 @@ test('self-review keeps every other rule: tip, evidence, listed reviewer, newest assert.equal(run([own(report({ notRun: false }))]).ok, true, 'no Not run: line is no longer checked (owner decision 2026-10-22)') assert.equal(run([own(report({ verdict: 'FAIL' }))]).verdict, 'FAIL') assert.equal(run([own(report(), '2026-10-18T01:00:00Z'), own(report({ verdict: 'FAIL' }), '2026-10-18T02:00:00Z')]).ok, false, 'a later FAIL vetoes a self-PASS') - assert.equal(run([own(report())], { reviewers: ['reviewer-agent'], selfReview: ['Birfy'] }).ok, false, 'selfReview does not make an unlisted login a reviewer') + assert.equal(run([own(report())], { reviewers: ['reviewer-agent'], selfReview: ['Birfy'], writers: ['Reviewer', 'Birfy'] }).ok, false, 'selfReview does not make an unlisted login a reviewer') assert.equal(run([comment('reviewer-agent')]).ok, true, 'another listed reviewer still counts on that pull request') }) @@ -258,7 +259,7 @@ test('#640: a quoted Tip SHA later in the body does not move the first line to a // 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 }) - const policy = { reviewers: ['reviewer-agent'], selfReview: [] } + const policy = { reviewers: ['reviewer-agent'], selfReview: [], writers: ['Reviewer'] } const pass = ['Tip SHA: ' + head, 'Ran:', ' - npm test → pass', 'Not run:', ' - smoke', 'Verdict: PASS'].join('\n') const result = decide({ head, @@ -321,3 +322,57 @@ test('#640 still holds with the heading form: a quoted Ran block below does not test('a Not run heading with words before its colon counts as the Not run section', () => { assert.equal(parseReport([`Verdict: PASS at ${HEAD}`, 'Ran: npm test', 'Not run at this tip: smoke'].join('\n')).hasNotRun, true) }) + +// ---- #627: every state-changing write says who wrote it. The gate buys presence and a name from a +// closed list (the shared identity means the line is a self-report, not proof — #633), and it refuses +// on two separate grounds so the log says which defect fired. +const unattributed = ({ tip = HEAD, verdict = 'PASS' } = {}) => + [`Tip SHA: ${tip}`, 'Ran:', ' - npm run verify → pass', 'Not run:', ' - smoke', `Verdict: ${verdict}`].join('\n') + +test('#627: a report with no Written-by line is refused, and the refusal says so', () => { + const r = decide({ head: HEAD, author: 'opencode-agent', reports: [comment('reviewer-agent', unattributed())], policy }) + assert.equal(r.ok, false) + assert.match(r.reason, /no Written-by line naming its writer/) +}) + +test('#627: a Written-by naming a writer outside the policy is refused on its own ground', () => { + const body = [unattributed().split('\n')[0], 'Written-by: Some Other Agent', ...unattributed().split('\n').slice(1)].join('\n') + const r = decide({ head: HEAD, author: 'opencode-agent', reports: [comment('reviewer-agent', body)], policy }) + assert.equal(r.ok, false) + assert.match(r.reason, /Written-by names "Some Other Agent", who is not in the writers list/) + assert.doesNotMatch(r.reason, /no Written-by line/, 'the two refusals do not collapse into one another') +}) + +test('#627: the line the relayed conclusion format uses parses to the writer, not to the relay', () => { + const body = [ + `Verdict: PASS at ${HEAD}`, + 'Written-by: Reviewer', + 'Posted-by: Chief (verbatim relay; Reviewer has no write channel this turn)', + '', + 'Ran: npm run verify → pass', + 'Not run: smoke', + ].join('\n') + const parsed = parseReport(body) + assert.equal(parsed.writer, 'Reviewer') + assert.equal(decide({ head: HEAD, author: 'opener', reports: [comment('reviewer-agent', body)], policy }).ok, true) + + // The older prose the repository wrote until #627 reads the same way. + assert.equal(parseReport(['Verdict: INC at ' + HEAD, 'Written by: Reviewer. Posted by: Chief (proxy).'].join('\n')).writer, 'Reviewer') + assert.equal(parseReport(['Written-by: reviewer'].join('\n')).writer, 'reviewer', 'the name is returned as written; decide compares case-insensitively') + assert.equal(decide({ head: HEAD, author: 'opener', reports: [comment('reviewer-agent', [`Tip SHA: ${HEAD}`, 'written-by: reviewer', 'Ran:', '- npm test', 'Verdict: PASS'].join('\n'))], policy }).ok, true) +}) + +test('#627: a writer named further down does not attribute this report', () => { + const body = [ + `Verdict: PASS at ${HEAD}`, + 'Ran: npm test → pass', + 'Not run: smoke', + '', + 'For comparison, the report this replaces began:', + 'Written-by: Reviewer', + ].join('\n') + assert.equal(parseReport(body).writer, null) + const r = decide({ head: HEAD, author: 'opener', reports: [comment('reviewer-agent', body)], policy }) + assert.equal(r.ok, false) + assert.match(r.reason, /no Written-by line/) +}) diff --git a/AGENTS.md b/AGENTS.md index cc431b7ca..15e5da41a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,7 +71,7 @@ If `topics.json` conflicts, keep both sides' entries (one patch per edited upstr ## Agent operating model -Decided by the owner (Danyang, GitHub `Birfy`) on 2026-10-18/19. Four scheduled routines run on the Grok Bot box, each with one job. A routine does only its own job. +Decided by the owner (Danyang, GitHub `Birfy`) on 2026-10-18/19. Four scheduled routines run on the Grok Bot box, each with one job. A routine does only its own job. Every write a routine makes carries the `Written-by:` line ([rule](#every-state-changing-write-says-who-wrote-it-627)). ### Routines @@ -110,12 +110,22 @@ Label `agent-ready`, with a triage comment, only when all three hold: Otherwise label `needs-human` or `needs-info` and name the blocking conflict or question in a comment. Agents develop **only** `agent-ready` issues. +While checking labels and state, a write that carries no `Written-by:` line gets one comment naming it; the audit never repairs someone else's text by editing it ([rule](#every-state-changing-write-says-who-wrote-it-627)). + ### Claim, PR, release of a claim - Before work: skip the issue if someone else is assigned. Otherwise assign it to `Birfy`, add `agent-working`, and post a claim comment that names the branch. - Stopping without a PR: unassign, remove `agent-working`, and comment why. - The PR says `Fixes #N`, is assigned to `Birfy` and labeled `agent-pr`. +### Every state-changing write says who wrote it (#627) + +The owner kept one shared GitHub identity for every Bot (2026-10-21), so attribution is text and only text. Every state-changing write — an issue comment, a PR body, an issue body, a merge-queue note — carries **`Written-by: `** as its first line, e.g. `Written-by: Reviewer`. A **conclusion** is the one exception, because [`parseReport`](.github/scripts/review-gate.mjs) reads the first non-empty line as the verdict (#637): its first line is `Verdict: … at `, `Written-by: ` is the second line, and a relay adds `Posted-by: ` third without changing anything else. A scheduled routine quotes this rule and does not restate it. + +The gate refuses what it can check, and fails closed. [`review-gate.mjs`](.github/scripts/review-gate.mjs) — and [`merge-gate.mjs`](.github/scripts/merge-gate.mjs), which imports its `decide`, so a queue entry is judged by the same rule — refuses a report that carries **no** `Written-by:` line, and refuses one whose name is **not** in the policy's `writers` list. Two separate refusals, so the log says which defect fired. Triage names a write that carries no such line in one comment and never repairs it by editing someone else's text. + +What this buys is presence and a name from a closed list, not truth: with one shared identity the line is a self-report that can be left out (refused) and can name the wrong Bot (undetectable here). The write layer knowing the writer is the DSH-side half, #633. + ### Gates - **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. @@ -130,7 +140,7 @@ Four rules decide whether a pull request may enter the queue. The mechanical one 1. **`hold` blocks mechanically.** A pull request labelled `hold` fails `merge-gate` on its queue entry. The label is only a marker; the job is the gate. It blocks the queue once `merge-gate` is listed among the required checks of the `main merge queue` ruleset — a ruleset change for the maintainer, never an agent; until then the job reports without blocking. 2. **Labelling a pull request `hold` means adding a record.** Add the label **and** post a new comment saying **Record only — do not merge / 仅记录、禁止入队**. The pair is what holds it. Add that comment; never edit or delete an earlier PASS report to change what it said — the history is the record, and rewriting it is the thing that reads as an incident afterwards. 3. **An authorisation is not a queue permit.** The owner or the Chief saying "let it through", "claim it" or "handle it along with the rest" licenses no specific pull request for the queue. Entering the queue is an explicit action on that one pull request, after its own gate passed, while it carries no `hold`. #566 is the case this exists for: "handle it along with the rest" was read as a permit, the tip was PASSed at 09:48:18Z, enqueued at 09:48:28Z and merged at 09:50:40Z as `abaf938b`. -4. **The newest conclusion at the head being merged decides.** Read the pull request's comments and review bodies, live, for the exact head SHA being merged — the pull request's own head, on a queue entry too (the `` in `gh-readonly-queue/main/pr--` is the entry's base, not its head; #654) — and take the newest text that carries `Verdict:` or a `## Review —` heading: INC or REQUEST_CHANGES refuses, PASS allows, no conclusion refuses too. Since #637 the format is fixed: the **first line** of the conclusion is exactly `Verdict: PASS at ` / `Verdict: INC at ` / `Verdict: REQUEST_CHANGES at `, with the attribution on the second line (`Written by: Reviewer. Posted by: Chief (proxy…).`) and the evidence below it — a body that carries a verdict anywhere else is no conclusion, and no conclusion refuses. A result from `pull_request` time never stands in — on #610 the tip report ran at 10:31:08Z and the INC landed at 11:24:37Z, 54 minutes later. The gating agent checks this before enqueueing; `merge-gate.yml` checks it again at `merge_group`, and the stricter answer wins. That job runs on two events: at `merge_group` it judges and a refusal fails it, and at `pull_request` it only reports what the queue entry would refuse — a required check that never reports on the pull request head leaves every pull request waiting for a status that never comes and unable to enter the queue at all (#639). The required status check is the job name `merge-gate`; the workflow name `Merge gate` is not a context any run produces. +4. **The newest conclusion at the head being merged decides.** Read the pull request's comments and review bodies, live, for the exact head SHA being merged — the pull request's own head, on a queue entry too (the `` in `gh-readonly-queue/main/pr--` is the entry's base, not its head; #654) — and take the newest text that carries `Verdict:` or a `## Review —` heading: INC or REQUEST_CHANGES refuses, PASS allows, no conclusion refuses too, and so does a report with no `Written-by:` line or one that names a writer the policy does not list (#627). Since #637 the format is fixed: the **first line** of the conclusion is exactly `Verdict: PASS at ` / `Verdict: INC at ` / `Verdict: REQUEST_CHANGES at `, with the attribution on the second line (`Written-by: Reviewer`, and a relay adds `Posted-by: Chief (proxy…)`; the older prose `Written by: …` reads the same way) and the evidence below it — a body that carries a verdict anywhere else is no conclusion, and no conclusion refuses. A result from `pull_request` time never stands in — on #610 the tip report ran at 10:31:08Z and the INC landed at 11:24:37Z, 54 minutes later. The gating agent checks this before enqueueing; `merge-gate.yml` checks it again at `merge_group`, and the stricter answer wins. That job runs on two events: at `merge_group` it judges and a refusal fails it, and at `pull_request` it only reports what the queue entry would refuse — a required check that never reports on the pull request head leaves every pull request waiting for a status that never comes and unable to enter the queue at all (#639). The required status check is the job name `merge-gate`; the workflow name `Merge gate` is not a context any run produces. A review's `state` is not what is read: the account cannot approve its own pull requests, so its reviews stay `COMMENTED` even for an INC, and the INC on #610 was a comment rather than a review. diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index 6add6c7f6..a3fd30481 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -144,17 +144,23 @@ What counts as a reviewed tip, all of it: Owner decision 2026-10-19: a PR opened by the owner's Bot/agent may be PASSed by the owner's own reviewer Bot/agent, so no independent reviewer is required. Any other author still needs someone else. A self-PASS does not replace green CI, and a new commit voids it. -3. The conclusion format is fixed (#637): the **first line** is `Verdict: PASS at ` (or `INC` / `REQUEST_CHANGES`), the second is the attribution (`Written by: Reviewer. Posted by: Chief (proxy…).`), then the evidence — `Ran:` (or a `Ran …:` heading over a fenced block of `$ ` commands, #660–#663) with at least one command; `Not run:` naming any gap is recommended but not checked (owner decision 2026-10-22 09:57 Asia/Shanghai). A body whose first line is not the verdict is no conclusion. +3. The conclusion format is fixed (#637): the **first line** is `Verdict: PASS at ` (or `INC` / `REQUEST_CHANGES`), the second is the attribution (`Written-by: Reviewer`; a relay adds `Posted-by: Chief (proxy…)` on the third line, and the older prose `Written by: …` reads the same way), then the evidence — `Ran:` (or a `Ran …:` heading over a fenced block of `$ ` commands, #660–#663) with at least one command; `Not run:` naming any gap is recommended but not checked (owner decision 2026-10-22 09:57 Asia/Shanghai). A body whose first line is not the verdict is no conclusion. 4. The sha on that first line **is the head being judged**. A later commit moves the tip and the old PASS stops counting; that report is then listed as "not counted", with the sha it named. 5. The **newest** report that names this tip decides, so a later FAIL at the same tip vetoes an earlier PASS, and a later PASS at the same tip replaces a FAIL. +**Attribution (#627).** Every state-changing write says who wrote it: `Written-by: ` is its first +line, and a conclusion puts it on the second, under the fixed verdict line. `decide` refuses a report +with no such line and, separately, one whose name is not in the policy's `writers`; Triage reports a +write without one instead of editing it. The rule itself lives in AGENTS.md — one shared GitHub identity +makes the line a self-report, so the gate guarantees presence, not truth (#633). + | File | Purpose | | --- | --- | -| `.github/agent/review-policy.json` | the logins whose PASS counts, and `selfReview`, the authors who may PASS their own PR; editing it is T3 | +| `.github/agent/review-policy.json` | the logins whose PASS counts, `selfReview`, the authors who may PASS their own PR, and `writers`, the Bot names a `Written-by:` line may carry (#627); editing it is T3 | | `.github/scripts/review-gate.mjs` | the rule above, for one pull request at a time | -| `.github/scripts/review-gate.test.mjs` | stale tip, self-review (allowed only for a `selfReview` author), unlisted login, FAIL after PASS, missing `Ran:` (a missing `Not run:` no longer refuses), and the workflow's shape | +| `.github/scripts/review-gate.test.mjs` | stale tip, self-review (allowed only for a `selfReview` author), unlisted login, FAIL after PASS, missing `Ran:` (a missing `Not run:` no longer refuses), a missing `Written-by:` line and one naming an unlisted writer (#627), and the workflow's shape | | `.github/workflows/review-gate.yml` | reports it in the job summary; `continue-on-error`, never required | Run it by hand (read-only API calls with your `gh` login): From 6798cda24c92f2eff0acb6524ef4f38f5daae697 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Thu, 22 Oct 2026 12:41:57 +0800 Subject: [PATCH 2/4] fix(agents): require the attribution line on conclusions only (#627) An ordinary comment is not a conclusion: a missing `Written-by:` line there must not be held against it. `decide` now demands the line and the listed writer only when the parsed report carries a verdict, and a new case pins that a plain comment, and a `Tip SHA:` block with no verdict, are not refused for it. --- .github/scripts/review-gate.mjs | 15 +++++++++------ .github/scripts/review-gate.test.mjs | 20 ++++++++++++++++++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index e41133e5a..b49bfba41 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -152,12 +152,15 @@ export function decide({ head, author, reports, policy }) { reasons.push(`${where(r)}: names ${parsed.tip.slice(0, SHORT)}, not the tip ${headSha.slice(0, SHORT)}`) continue } - // #627: who wrote it, refused on two separate grounds — a report with no attribution line and one - // that names a writer the policy does not list are different defects, and the log has to say which - // one fired. Both fail closed. With one shared GitHub identity the line is a self-report, so this - // buys presence and a name from a closed list, not proof (#633). - if (!parsed.writer) { reasons.push(`${where(r)}: no Written-by line naming its writer`); continue } - if (!writers.has(parsed.writer.toLowerCase())) { reasons.push(`${where(r)}: Written-by names "${parsed.writer}", who is not in the writers list of .github/agent/review-policy.json`); continue } + // #627: a *conclusion* has to say who wrote it, refused on two separate grounds — a report with no + // attribution line and one that names a writer the policy does not list are different defects, and + // the log has to say which one fired. Both fail closed. An ordinary comment is not a conclusion, so + // a missing line is not held against it. With one shared GitHub identity the line is a self-report, + // so this buys presence and a name from a closed list, not proof (#633). + if (parsed.verdict) { + if (!parsed.writer) { reasons.push(`${where(r)}: no Written-by line naming its writer`); continue } + if (!writers.has(parsed.writer.toLowerCase())) { reasons.push(`${where(r)}: Written-by names "${parsed.writer}", who is not in the writers list of .github/agent/review-policy.json`); continue } + } if (login(r) === authorLogin && !selfReview.has(authorLogin)) { reasons.push(`${where(r)}: the pull request's own author is not a reviewer (not in selfReview)`); continue } if (!reviewers.has(login(r))) { reasons.push(`${where(r)}: not a reviewer listed in .github/agent/review-policy.json`); continue } entry.counts = true diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index a1429caef..2c5e7869f 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -376,3 +376,23 @@ test('#627: a writer named further down does not attribute this report', () => { assert.equal(r.ok, false) assert.match(r.reason, /no Written-by line/) }) + +test('#627: an ordinary comment is not a conclusion, so a missing Written-by line is not held against it', () => { + const r = decide({ head: HEAD, author: 'opener', reports: [comment('reviewer-agent', 'LGTM, merging this after CI')], policy }) + assert.equal(r.ok, false) + assert.match(r.reason, /no Verifier report names the tip/) + assert.doesNotMatch(r.reason, /Written-by/) + + // A `Tip SHA:` block with no verdict is no conclusion either, so it demands no attribution. + const noVerdict = decide({ head: HEAD, author: 'opener', reports: [comment('reviewer-agent', [`Tip SHA: ${HEAD}`, 'Ran: npm test → pass'].join('\n'))], policy }) + assert.doesNotMatch(noVerdict.reason, /Written-by/) + + // And an attributed PASS beside an unattributed ordinary comment still counts. + const both = decide({ + head: HEAD, + author: 'opener', + reports: [comment('reviewer-agent', 'LGTM', '2026-10-18T00:00:00Z'), comment('reviewer-agent', report(), '2026-10-18T01:00:00Z')], + policy, + }) + assert.equal(both.ok, true, both.reason) +}) From 172dc1763d21b5f339e06c28375acb14bae73721 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Thu, 22 Oct 2026 13:47:17 +0800 Subject: [PATCH 3/4] fix(agents): an invalid conclusion still counts as the newest word (#676) A conclusion that failed a check was pushed to `reasons` and skipped, so it never entered the newest-word question: an INC with no `Written-by:` line left an earlier PASS in force, and a human REQUEST_CHANGES from an unlisted login was swallowed the same way. Every conclusion that names the tip now joins `atTip` carrying its reason, and the newest one decides: if the checks refused it, the report is refused with that reason. An older invalid conclusion followed by a properly attributed PASS still counts, so a formatting mistake does not block the tip forever. --- .github/scripts/review-gate.mjs | 43 +++++++++++---------- .github/scripts/review-gate.test.mjs | 57 ++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 19 deletions(-) diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index b49bfba41..2d5eb1dc5 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -139,54 +139,59 @@ export function decide({ head, author, reports, policy }) { const reviewers = new Set((policy?.reviewers ?? []).map((l) => String(l).toLowerCase())) const selfReview = new Set((policy?.selfReview ?? []).map((l) => String(l).toLowerCase())) const writers = new Set((policy?.writers ?? []).map((w) => String(w).toLowerCase())) - const reasons = [] + const reasons = [] // reports that do not name this tip at all const judged = [] + const atTip = [] // conclusions that name this tip, valid or not, in the order the API returned them 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 } + 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 } 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)}`) continue } + if (!parsed.verdict) continue // a `Tip SHA` block with no verdict is not a conclusion + // #676: a conclusion at this tip belongs to the newest-word question even when it fails a check + // below, so it joins `atTip` carrying its reason instead of being dropped. Dropping it made a + // newer refusal invisible: an INC with no `Written-by:` line left an older PASS in force, and a + // human REQUEST_CHANGES from an unlisted login did the same. + atTip.push(entry) // #627: a *conclusion* has to say who wrote it, refused on two separate grounds — a report with no // attribution line and one that names a writer the policy does not list are different defects, and // the log has to say which one fired. Both fail closed. An ordinary comment is not a conclusion, so // a missing line is not held against it. With one shared GitHub identity the line is a self-report, // so this buys presence and a name from a closed list, not proof (#633). - if (parsed.verdict) { - if (!parsed.writer) { reasons.push(`${where(r)}: no Written-by line naming its writer`); continue } - if (!writers.has(parsed.writer.toLowerCase())) { reasons.push(`${where(r)}: Written-by names "${parsed.writer}", who is not in the writers list of .github/agent/review-policy.json`); continue } - } - if (login(r) === authorLogin && !selfReview.has(authorLogin)) { reasons.push(`${where(r)}: the pull request's own author is not a reviewer (not in selfReview)`); continue } - if (!reviewers.has(login(r))) { reasons.push(`${where(r)}: not a reviewer listed in .github/agent/review-policy.json`); continue } + if (!parsed.writer) { entry.why = `${where(r)}: no Written-by line naming its writer`; continue } + if (!writers.has(parsed.writer.toLowerCase())) { entry.why = `${where(r)}: Written-by names "${parsed.writer}", who is not in the writers list of .github/agent/review-policy.json`; continue } + if (login(r) === authorLogin && !selfReview.has(authorLogin)) { entry.why = `${where(r)}: the pull request's own author is not a reviewer (not in selfReview)`; continue } + if (!reviewers.has(login(r))) { entry.why = `${where(r)}: not a reviewer listed in .github/agent/review-policy.json`; continue } entry.counts = true } - const counting = judged.filter((e) => e.counts) - if (!counting.length) { - const reason = reviewers.size - ? reasons.length ? reasons.join('; ') : `no Verifier report names the tip ${headSha.slice(0, SHORT)}` - : 'no reviewer is listed in .github/agent/review-policy.json' + if (!reviewers.size) return { ok: false, verdict: null, by: null, tip: null, reason: 'no reviewer is listed in .github/agent/review-policy.json', judged } + if (!atTip.length) { + const reason = reasons.length ? reasons.join('; ') : `no Verifier report names the tip ${headSha.slice(0, SHORT)}` return { ok: false, verdict: null, by: null, tip: null, reason, judged } } - // Newest first: the reviewer's latest word at this tip decides. Equal timestamps fall back to - // the order the API returned them in (oldest first), so the later comment still wins. - const last = counting + // 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 .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 } const { parsed } = last - if (last.verdict === 'PASS' && !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 } - } 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 } } + 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: true, verdict: 'PASS', by: last.login, tip: last.tip, reason: `${last.login}'s PASS names this tip`, judged } } diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index 2c5e7869f..2ace60ffd 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -396,3 +396,60 @@ test('#627: an ordinary comment is not a conclusion, so a missing Written-by lin }) assert.equal(both.ok, true, both.reason) }) + +// ---- #676: an invalid conclusion at the tip is still the reviewer's word. Dropping it from the +// newest-word question made a later refusal invisible and left an older PASS in force. +test('#676: a newer INC with no Written-by line vetoes an earlier PASS instead of being skipped', () => { + const inc = [`Verdict: INC at ${HEAD}`, 'Ran: npm run verify → fail', 'Not run: smoke'].join('\n') + const r = decide({ + head: HEAD, + author: 'opener', + reports: [comment('reviewer-agent', report(), '2026-10-18T01:00:00Z'), comment('reviewer-agent', inc, '2026-10-18T02:00:00Z')], + policy, + }) + assert.equal(r.ok, false, 'the newer INC refuses; the PASS it replaced must not survive') + assert.equal(r.verdict, 'INC') + assert.match(r.reason, /no Written-by line naming its writer/) +}) + +test('#676: a newer REQUEST_CHANGES from a login outside the policy still refuses', () => { + const human = [ + `Verdict: REQUEST_CHANGES at ${HEAD}`, + 'Written-by: Reviewer', + '', + 'Ran: npm test → pass', + 'Not run: smoke', + ].join('\n') + const r = decide({ + head: HEAD, + author: 'opener', + reports: [comment('reviewer-agent', report(), '2026-10-18T01:00:00Z'), comment('outside-login', human, '2026-10-18T02:00:00Z')], + policy, + }) + assert.equal(r.ok, false, 'a refusal is not swallowed because its author is not a listed reviewer') + assert.equal(r.verdict, 'REQUEST_CHANGES') + assert.match(r.reason, /not a reviewer listed/) +}) + +test('#676: a fixed format after an older invalid conclusion counts — the newest word decides', () => { + const inc = [`Verdict: INC at ${HEAD}`, 'Ran: npm test → fail'].join('\n') + const r = decide({ + head: HEAD, + author: 'opener', + reports: [comment('reviewer-agent', inc, '2026-10-18T01:00:00Z'), comment('reviewer-agent', report(), '2026-10-18T02:00:00Z')], + policy, + }) + assert.equal(r.ok, true, r.reason) + assert.equal(r.verdict, 'PASS') +}) + +test('#676: a newer PASS with no command it ran still refuses over an older good PASS', () => { + const r = decide({ + head: HEAD, + author: 'opener', + reports: [comment('reviewer-agent', report(), '2026-10-18T01:00:00Z'), comment('reviewer-agent', report({ ran: [] }), '2026-10-18T02:00:00Z')], + policy, + }) + assert.equal(r.ok, false) + assert.match(r.reason, /lists no command it ran/) +}) From ef340354e2b8f3a98413060e6ddab1ee2b286854 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Thu, 22 Oct 2026 14:08:12 +0800 Subject: [PATCH 4/4] fix(agents): a conclusion counts only from an OWNER, MEMBER or COLLABORATOR (#676) The fail-closed newest-word rule made any comment that starts with `Verdict:` able to hold a merge. GitHub already says who is tied to the repository, so `decide` now reads a conclusion only when `author_association` is OWNER, MEMBER or COLLABORATOR; anything else is not a conclusion and counts for nothing, and an association the API did not send is not assumed. `merge-gate.mjs` passes the same field, so the queue entry judges identically. --- .github/scripts/merge-gate.mjs | 4 ++-- .github/scripts/merge-gate.test.mjs | 8 ++++---- .github/scripts/review-gate.mjs | 13 +++++++++++-- .github/scripts/review-gate.test.mjs | 22 ++++++++++++++++++++-- AGENTS.md | 2 ++ docs/autonomous-dev.md | 2 +- 6 files changed, 40 insertions(+), 11 deletions(-) diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index baa4d3890..39cb6788e 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -170,10 +170,10 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prO const author = pull.user?.login const labels = (pull.labels ?? []).map((l) => l.name) const reports = [ - ...list(`repos/${repo}/issues/${pr}/comments?per_page=100`).map((c) => ({ kind: 'comment', login: c.user?.login, body: c.body, at: c.created_at })), + ...list(`repos/${repo}/issues/${pr}/comments?per_page=100`).map((c) => ({ kind: 'comment', login: c.user?.login, 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, body: v.body, at: v.submitted_at })), + .map((v) => ({ kind: 'review', login: v.user?.login, body: v.body, at: v.submitted_at, association: v.author_association })), ] // The workflow passes both events through this same entry point: `merge_group` judges, `pull_request` diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index 7df31bd69..f3198b581 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -13,7 +13,7 @@ const policy = { reviewers: ['reviewer-agent'], selfReview: [], writers: ['Revie const report = ({ tip = HEAD, verdict = 'PASS' } = {}) => [`Tip SHA: ${tip}`, 'Written-by: Reviewer', 'Ran:', ' - npm run verify → pass', 'Not run:', ' - smoke — host code unchanged', `Verdict: ${verdict}`].join('\n') -const say = (body = report(), at = '2026-10-21T10:31:00Z', kind = 'comment') => ({ login: 'reviewer-agent', body, at, kind }) +const say = (body = report(), at = '2026-10-21T10:31:00Z', kind = 'comment') => ({ login: 'reviewer-agent', body, at, kind, association: 'MEMBER' }) const judgeAt = (over = {}) => judge({ labels: [], head: HEAD, author: 'opener', reports: [say()], policy, ...over }) @@ -152,8 +152,8 @@ const runGate = (fixture) => { }) } -const apiComment = (body, at) => ({ user: { login: 'reviewer-agent' }, body, created_at: at }) -const apiReview = (body, at) => ({ user: { login: 'reviewer-agent' }, body, submitted_at: at }) +const apiComment = (body, at) => ({ user: { login: 'reviewer-agent' }, body, created_at: at, author_association: 'MEMBER' }) +const apiReview = (body, at) => ({ user: { login: 'reviewer-agent' }, body, submitted_at: at, author_association: 'MEMBER' }) const firstLine = (verdict, tip = HEAD) => [ `Verdict: ${verdict} at ${tip}`, @@ -347,7 +347,7 @@ const queueReal = (fx, body = fx.comment.body) => main({ gh: (...args) => { const path = args[args.length - 1] if (path.includes('/files')) return fx.files.map((filename) => JSON.stringify({ filename })).join('\n') - if (path.includes('/comments')) return JSON.stringify({ user: { login: fx.comment.login }, body, created_at: fx.comment.created_at }) + if (path.includes('/comments')) return JSON.stringify({ user: { login: fx.comment.login }, body, created_at: fx.comment.created_at, author_association: 'OWNER' }) if (path.includes('/reviews')) return '' if (/\/pulls\/\d+$/.test(path)) return JSON.stringify({ head: { sha: fx.head }, user: { login: fx.author }, labels: [] }) throw new Error('fake gh: unexpected ' + path) diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index 2d5eb1dc5..3dc517d96 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -30,6 +30,10 @@ import { execFileSync } from 'node:child_process' import { fileURLToPath } from 'node:url' const SHORT = 12 +// #676: a conclusion is read only from someone GitHub ties to this repository. The fail-closed rule +// above makes any newest conclusion refuse, so without this an outside comment saying `Verdict: INC` +// could hold a merge. A comment from anyone else is not a conclusion: it can neither pass nor block. +const ASSOCIATIONS = new Set(['OWNER', 'MEMBER', 'COLLABORATOR']) // The last Verifier block in one body: a comment may hold an earlier review that a later // one replaced, and only the last block is the verdict the reviewer stands behind. @@ -154,6 +158,11 @@ export function decide({ head, author, reports, policy }) { continue } if (!parsed.verdict) continue // a `Tip SHA` block with no verdict is not a conclusion + const association = String(r.association ?? '').toUpperCase() + if (!ASSOCIATIONS.has(association)) { + reasons.push(`${where(r)}: author association ${association || 'missing'} is not OWNER, MEMBER or COLLABORATOR, so it is not a conclusion`) + continue + } // #676: a conclusion at this tip belongs to the newest-word question even when it fails a check // below, so it joins `atTip` carrying its reason instead of being dropped. Dropping it made a // newer refusal invisible: an INC with no `Written-by:` line left an older PASS in force, and a @@ -236,10 +245,10 @@ function main() { if (!head) throw new Error(`pull request ${pr} has no head sha`) const reports = [ - ...list(`repos/${repo}/issues/${pr}/comments?per_page=100`).map((c) => ({ kind: 'comment', login: c.user?.login, body: c.body, at: c.created_at, url: c.html_url })), + ...list(`repos/${repo}/issues/${pr}/comments?per_page=100`).map((c) => ({ kind: 'comment', login: c.user?.login, body: c.body, at: c.created_at, association: c.author_association, url: c.html_url })), ...list(`repos/${repo}/pulls/${pr}/reviews?per_page=100`) .filter((v) => (v.body ?? '').trim()) - .map((v) => ({ kind: 'review', login: v.user?.login, body: v.body, at: v.submitted_at, url: v.html_url })), + .map((v) => ({ kind: 'review', login: v.user?.login, body: v.body, at: v.submitted_at, association: v.author_association, url: v.html_url })), ] const result = decide({ head, author, reports, policy: loadPolicy(policyPath) }) diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index 2ace60ffd..75efa5ac5 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -18,7 +18,7 @@ const report = ({ tip = HEAD, verdict = 'PASS', ran = [' - npm run verify → p `Verdict: ${verdict}`, ].join('\n') -const comment = (login, body = report(), at = '2026-10-18T01:00:00Z', kind = 'comment') => ({ login, body, at, kind }) +const comment = (login, body = report(), at = '2026-10-18T01:00:00Z', kind = 'comment', association = 'MEMBER') => ({ login, body, at, kind, association }) test('a PASS by a listed reviewer that names the tip counts', () => { const r = decide({ head: HEAD, author: 'opencode-agent', reports: [comment('reviewer-agent')], policy }) @@ -258,7 +258,7 @@ test('#640: a quoted Tip SHA later in the body does not move the first line to a // 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 }) + 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') const result = decide({ @@ -453,3 +453,21 @@ test('#676: a newer PASS with no command it ran still refuses over an older good assert.equal(r.ok, false) assert.match(r.reason, /lists no command it ran/) }) + +// #676, as narrowed: the fail-closed newest-word rule is for people GitHub ties to this repository. +// Anyone else's `Verdict:` line is not a conclusion, so it can neither pass nor hold a merge. +test('#676: a conclusion from outside the repository counts for nothing', () => { + const inc = [`Verdict: INC at ${HEAD}`, 'Written-by: Reviewer', '', 'Ran: npm test → fail'].join('\n') + const outsider = { login: 'passer-by', body: inc, at: '2026-10-18T02:00:00Z', kind: 'comment', association: 'NONE' } + const r = decide({ head: HEAD, author: 'opener', reports: [comment('reviewer-agent', report(), '2026-10-18T01:00:00Z'), outsider], policy }) + assert.equal(r.ok, true, 'the listed reviewer\'s PASS stands; an outside comment cannot block it') + assert.equal(r.verdict, 'PASS') + + const solo = decide({ head: HEAD, author: 'opener', reports: [outsider], policy }) + assert.equal(solo.ok, false, 'nor can it pass a tip') + assert.match(solo.reason, /author association NONE is not OWNER, MEMBER or COLLABORATOR/) + + const missing = decide({ head: HEAD, author: 'opener', reports: [{ login: 'reviewer-agent', body: report(), at: '2026-10-18T01:00:00Z', kind: 'comment' }], policy }) + assert.equal(missing.ok, false, 'an association the API did not send is not assumed') + assert.match(missing.reason, /author association missing/) +}) diff --git a/AGENTS.md b/AGENTS.md index 15e5da41a..1cf433a90 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -124,6 +124,8 @@ The owner kept one shared GitHub identity for every Bot (2026-10-21), so attribu The gate refuses what it can check, and fails closed. [`review-gate.mjs`](.github/scripts/review-gate.mjs) — and [`merge-gate.mjs`](.github/scripts/merge-gate.mjs), which imports its `decide`, so a queue entry is judged by the same rule — refuses a report that carries **no** `Written-by:` line, and refuses one whose name is **not** in the policy's `writers` list. Two separate refusals, so the log says which defect fired. Triage names a write that carries no such line in one comment and never repairs it by editing someone else's text. +The gate reads a conclusion only from someone GitHub ties to this repository: the comment's `author_association` must be `OWNER`, `MEMBER` or `COLLABORATOR`, or it is not a conclusion and counts for nothing — otherwise any passer-by could hold a merge with one `Verdict: INC` line, because the newest conclusion decides (#676). An association the API did not send is not assumed. + What this buys is presence and a name from a closed list, not truth: with one shared identity the line is a self-report that can be left out (refused) and can name the wrong Bot (undetectable here). The write layer knowing the writer is the DSH-side half, #633. ### Gates diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index a3fd30481..1a3857538 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -144,7 +144,7 @@ What counts as a reviewed tip, all of it: Owner decision 2026-10-19: a PR opened by the owner's Bot/agent may be PASSed by the owner's own reviewer Bot/agent, so no independent reviewer is required. Any other author still needs someone else. A self-PASS does not replace green CI, and a new commit voids it. -3. The conclusion format is fixed (#637): the **first line** is `Verdict: PASS at ` (or `INC` / `REQUEST_CHANGES`), the second is the attribution (`Written-by: Reviewer`; a relay adds `Posted-by: Chief (proxy…)` on the third line, and the older prose `Written by: …` reads the same way), then the evidence — `Ran:` (or a `Ran …:` heading over a fenced block of `$ ` commands, #660–#663) with at least one command; `Not run:` naming any gap is recommended but not checked (owner decision 2026-10-22 09:57 Asia/Shanghai). A body whose first line is not the verdict is no conclusion. +3. The conclusion format is fixed (#637): the **first line** is `Verdict: PASS at ` (or `INC` / `REQUEST_CHANGES`), the second is the attribution (`Written-by: Reviewer`; a relay adds `Posted-by: Chief (proxy…)` on the third line, and the older prose `Written by: …` reads the same way), then the evidence — `Ran:` (or a `Ran …:` heading over a fenced block of `$ ` commands, #660–#663) with at least one command; `Not run:` naming any gap is recommended but not checked (owner decision 2026-10-22 09:57 Asia/Shanghai). A body whose first line is not the verdict is no conclusion. The comment's `author_association` must be `OWNER`, `MEMBER` or `COLLABORATOR`: a conclusion from anyone else counts for nothing, so it can neither pass a tip nor hold one (#676). 4. The sha on that first line **is the head being judged**. A later commit moves the tip and the old PASS stops counting; that report is then listed as "not counted", with the sha it named. 5. The **newest** report that names this tip decides, so a later FAIL at the same tip vetoes an