Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 48 additions & 7 deletions .github/scripts/review-gate.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ const SHORT = 12
// 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'])
// #1239: the four words a `Verdict:` line may carry. Nothing else is read, and a fifth word is named
// in the refusal rather than added as an alias — an alias would leave "how many words do we read" a guess.
const VERDICT_TOKENS = new Set(['PASS', 'FAIL', 'INC', 'REQUEST_CHANGES'])

// A conclusion GitHub did not already tie to the repo (OWNER/MEMBER/COLLABORATOR), from a login
// in `reviewers`, asked once via the collaborator permission API. Anyone else can never pass the
Expand Down Expand Up @@ -96,26 +99,37 @@ 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, writer: null, attestation: null }
const out = { tip: null, verdict: null, unknownVerdict: null, ran: [], hasNotRun: false, writer: null, attestation: null }
// The fixed format (#637): the first non-empty line is the whole conclusion,
// `Verdict: PASS at <sha>` (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.
const firstLine = lines.find((line) => line.trim() !== '') ?? ''
const first = /^Verdict\s*:\s*(PASS|INC|REQUEST_CHANGES|FAIL)\s+(?:at\s+)?([0-9a-f]{7,40})\s*$/i.exec(firstLine.trim())
// #1239: the same header with a word the gate does not read. The `\b` above cannot match `Incorrect`
// (`INC` is followed by a letter, not a word boundary), so before this the whole line was not a
// conclusion at all: the PASS it meant to withdraw stayed the newest word and the merge went ahead.
// The tip is read here too, which is what makes it a conclusion at that tip rather than a stray
// sentence — a word with no tip below is still an ordinary comment.
const firstUnknown = first ? null : /^Verdict\s*:\s*(\S+?)\s+(?:at\s+)?([0-9a-f]{7,40})\s*$/i.exec(firstLine.trim())
let start = -1
for (let i = 0; i < lines.length; i++) {
if (/^Tip SHA\s*:/i.test(lines[i].trim())) start = i
}
if (first) {
out.verdict = first[1].toUpperCase()
out.tip = first[2].toLowerCase()
} else if (firstUnknown) {
out.unknownVerdict = firstUnknown[1]
out.tip = firstUnknown[2].toLowerCase()
}
// #643, #640: once the first line is the conclusion, the loop below must write neither field again.
// A body quotes older reports (a `Tip SHA: <sha>` block with an older `Verdict: PASS`), and letting
// either field through turned an INC at this tip into a PASS, or into a conclusion about some other
// tip that `decide` then dropped while the PASS it refused came back to life. Both fail open.
let ranSeen = false
const firstIsVerdict = first !== null
// An unreadable first-line conclusion is still the header: the quote rules below keep a later block
// from overwriting the first one (#643, #640), and they protect this one the same way.
const firstIsVerdict = first !== null || firstUnknown !== null
// With the first line as the conclusion the evidence is below it, so scan from the top; a
// legacy body is judged by its last block.
const from = firstIsVerdict || start < 0 ? 0 : start
Expand All @@ -128,6 +142,11 @@ export function parseReport(body) {
}
const verdictLine = /^Verdict\s*:\s*(PASS|FAIL|INC|REQUEST_CHANGES)\b/i.exec(line)
if (verdictLine && !firstIsVerdict) out.verdict = verdictLine[1].toUpperCase()
// #1239: the loop needs its own read. A `Tip SHA:` block whose `Verdict:` line says anything else is
// the same defect as the first-line form; without this the block reads as "no verdict" and an older
// PASS survives. A line that already gave a readable verdict is not read again for an unknown word.
const unknownLine = verdictLine || firstIsVerdict ? null : /^Verdict\s*:\s*(\S+)/i.exec(line)
if (unknownLine && !VERDICT_TOKENS.has(unknownLine[1].toUpperCase())) out.unknownVerdict = unknownLine[1]
// `Ran: <command>`, or a heading such as `Ran at this tip (tarball of the tip):` — the shape the
// reviewer actually writes (#660-#663: every one of those PASSes refused as "lists no command"
// because only a bare `Ran:` was read). A longer heading is only a heading: words after its colon
Expand All @@ -141,6 +160,9 @@ export function parseReport(body) {
}
if (/^Not run\b[^:\n]*:/i.test(line)) out.hasNotRun = true
}
// #1239: a readable verdict is this report's word; an unreadable `Verdict:` line below it is quoted
// text (the rule #643 and #640 keep a quote from overwriting the header), so it decides nothing here.
if (out.verdict) out.unknownVerdict = null
out.writer = parseWriter(lines)
// #644: the attestation is part of the conclusion header, the lines before the first blank. A copy
// quoted under `Ran:` sits after that blank and must not lend this report a marker it did not open with.
Expand Down Expand Up @@ -299,8 +321,8 @@ export function decide({ head, author, reports, policy, admins, body, owners, re

for (const r of reports ?? []) {
const parsed = parseReport(r.body)
if (!parsed.tip && !parsed.verdict) continue // not a Verifier report
const entry = { id: r.id, login: r.login, verdict: parsed.verdict, tip: parsed.tip, at: r.at, kind: r.kind, writer: parsed.writer, parsed, counts: false, why: null }
if (!parsed.tip && !parsed.verdict && !parsed.unknownVerdict) continue // not a Verifier report
const entry = { id: r.id, login: r.login, verdict: parsed.verdict, unknownVerdict: parsed.unknownVerdict, 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 }
// #1112: the comparison is the whole sha, not a prefix. A conclusion written `at abaf938` is a
Expand All @@ -313,7 +335,9 @@ export function decide({ head, author, reports, policy, admins, body, owners, re
: `${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
// #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.
if (!parsed.verdict && !parsed.unknownVerdict) continue // a `Tip SHA` block with no verdict is not a conclusion
// #943: refused before the association test, with its own reason: a bot account is not a
// conclusion for a reason unlisted logins cannot explain, and the log has to say which defect fired.
if (isBotAccount(r)) {
Expand Down Expand Up @@ -352,6 +376,13 @@ export function decide({ head, author, reports, policy, admins, body, owners, re
// 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)
// #1239: a word outside the four the gate reads counts for nothing, but it still takes its place in
// the newest-word scan, carrying a reason that names it. Dropping the line is the defect: an INC the
// reviewer meant became invisible and the older PASS it withdrew stayed in force on #1221.
if (parsed.unknownVerdict) {
entry.why = `${where(r)}: reads "${parsed.unknownVerdict}" as its verdict, which is not one of PASS, FAIL, INC or REQUEST_CHANGES, so this conclusion cannot be read`
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
Expand Down Expand Up @@ -412,8 +443,18 @@ export function render(result, { pr, head, author } = {}) {
out.push('', 'Unverified — not read as conclusions:')
for (const e of unverifiedReports.slice(0, 10)) out.push(`- unverified: ${e.login} (${e.kind}): ${e.verdict ?? 'no verdict'} at ${(e.tip ?? 'no tip').slice(0, SHORT)} — ${String(e.why ?? '').replace(/^[^:]+: unverified — /, '')}`)
}
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)}, written-by ${e.writer ?? 'nothing'}${e.counts ? '' : ' (not counted)'}`))
const seen = result.judged.filter((e) => e.counts || e.verdict || e.unknownVerdict)
if (seen.length) {
out.push('', 'Verifier reports read:')
for (const e of seen.slice(0, 10)) {
// #1239: an unreadable word is named with its reason, so the next reader does not have to rediscover
// that `Incorrect` was never read as `INC`.
const word = e.unknownVerdict ? `"${e.unknownVerdict}", which is not a verdict word` : (e.verdict ?? 'no verdict')
const why = e.unknownVerdict ? ` — ${String(e.why ?? '').replace(/^[^:]+:\s*/, '')}` : ''
out.push(`- ${e.login} (${e.kind}): ${word} at ${(e.tip ?? 'no tip').slice(0, SHORT)}, written-by ${e.writer ?? 'nothing'}${e.counts ? '' : ' (not counted)'}${why}`)
}
if (seen.some((e) => e.unknownVerdict)) out.push('- the four verdict words the gate reads: PASS, FAIL, INC, REQUEST_CHANGES — any other word is unreadable and refuses this tip (#1239)')
}
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')
}
Expand Down
80 changes: 80 additions & 0 deletions .github/scripts/review-gate.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,86 @@ test('INC is not PASS', () => {
assert.equal(r.verdict, 'INC')
})

// #1239: a misspelt verdict word was not refused, it was invisible. `Verdict: Incorrect at <tip>` fell out of
// `decide` as "not a Verifier report", so the PASS it meant to withdraw stayed the newest word and merged.
const misspelt = (word, tip = HEAD) =>
[`Verdict: ${word} at ${tip}`, 'Written-by: Reviewer', 'Ran:', ' - npm run verify → pass', 'Not run:', ' - smoke — no host change'].join('\n')

test('#1239: an unreadable verdict word at the tip vetoes the older PASS there', () => {
const reports = [
comment('reviewer-agent', report(), '2026-10-18T01:00:00Z'),
comment('reviewer-agent', misspelt('Incorrect'), '2026-10-18T02:00:00Z'),
]
const r = decide({ head: HEAD, author: 'opencode-agent', reports, policy })
assert.equal(r.ok, false, 'the newer unreadable conclusion must not leave the older PASS in force')
assert.match(r.reason, /Incorrect/)
assert.match(r.reason, /PASS, FAIL, INC or REQUEST_CHANGES/)
assert.equal(r.judged.length, 2, 'it is read as a conclusion, not skipped as an ordinary comment')
assert.equal(parseReport(misspelt('Incorrect')).unknownVerdict, 'Incorrect')
assert.equal(parseReport(misspelt('Incorrect')).tip, HEAD, 'the word carries the tip it names')
})

test('#1239: an unreadable word alone at the tip still refuses, and the reason names the word', () => {
const r = decide({ head: HEAD, author: 'opencode-agent', reports: [comment('reviewer-agent', misspelt('banana'))], policy })
assert.equal(r.ok, false)
assert.match(r.reason, /"banana"/)
})

test('#1239: an unreadable word with no tip is an ordinary comment and leaves the PASS alone', () => {
const reports = [
comment('reviewer-agent', report(), '2026-10-18T01:00:00Z'),
comment('reviewer-agent', ['Verdict: banana', 'Written-by: Reviewer'].join('\n'), '2026-10-18T02:00:00Z'),
]
const r = decide({ head: HEAD, author: 'opencode-agent', reports, policy })
assert.equal(r.ok, true, r.reason)
assert.equal(parseReport(['Verdict: banana'].join('\n')).tip, null)
})

test('#1239: the legacy Tip SHA block needs the same read as the first line', () => {
const body = ['Tip SHA: ' + HEAD, 'Written-by: Reviewer', 'Ran:', '- npm test', 'Verdict: Incorrect'].join('\n')
const parsed = parseReport(body)
assert.equal(parsed.unknownVerdict, 'Incorrect', 'the loop at the second Verdict: line reads it too')
assert.equal(parsed.tip, HEAD)
const r = decide({ head: HEAD, author: 'opencode-agent', reports: [comment('reviewer-agent', body)], policy })
assert.equal(r.ok, false)
assert.match(r.reason, /Incorrect/)
})

// #1239, second shape (Reviewer2 on #1242): the header itself is unreadable and a readable PASS is quoted
// right below it. The quote rules keep that PASS from becoming this report's word, so `firstIsVerdict` must
// cover an unreadable first line too: without that half the quoted PASS is read and the tip goes green.
test('#1239: a readable PASS quoted under an unreadable first line does not rescue it', () => {
const body = [
`Verdict: Incorrect at ${HEAD}`,
'',
'Written-by: Reviewer',
`Tip SHA: ${HEAD}`,
'Ran:',
' - npm run verify → pass',
'Verdict: PASS',
].join('\n')
const parsed = parseReport(body)
assert.equal(parsed.unknownVerdict, 'Incorrect', 'the header is the unreadable word')
assert.equal(parsed.verdict, null, "the quoted PASS below it is a quote, not this report's verdict")
assert.equal(parsed.tip, HEAD)
const r = decide({ head: HEAD, author: 'opencode-agent', reports: [comment('reviewer-agent', body)], policy })
assert.equal(r.ok, false, 'an unreadable header must not go green on the strength of a quoted PASS')
assert.equal(r.verdict, null)
assert.match(r.reason, /Incorrect/)
})

test('#1239: the four words the gate reads behave exactly as before', () => {
for (const word of ['PASS', 'FAIL', 'INC', 'REQUEST_CHANGES']) {
const parsed = parseReport(misspelt(word))
assert.equal(parsed.unknownVerdict, null, word)
assert.equal(parsed.verdict, word)
assert.equal(parsed.tip, HEAD)
}
const view = render(decide({ head: HEAD, author: 'opencode-agent', reports: [comment('reviewer-agent')], policy }), { pr: 6, head: HEAD, author: 'opencode-agent' })
assert.match(view, /Reviewed tip: PASS/)
assert.doesNotMatch(view, /the four verdict words the gate reads/)
})

test('a PASS that lists no command it ran is not evidence', () => {
const r = decide({ head: HEAD, author: 'opencode-agent', reports: [comment('reviewer-agent', report({ ran: [] }))], policy })
assert.equal(r.ok, false)
Expand Down
2 changes: 1 addition & 1 deletion docs/autonomous-dev.md
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ What counts as a reviewed tip, all of it:
role that is the Owner is the author's own GitHub identity and does not count. A conclusion from a different
GitHub login does not carry the line. `decide` parses it; a signature was the later slice of #644 and is shipped but not enabled (#1110, #1111). The
reviewed-tip job, `enqueue-guard` and `merge-gate` call that one function.
3. The conclusion format is fixed (#637): the **first line** is `Verdict: PASS at <sha>` (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`, or the report must be that pull request's own author listed in `selfReview` (#928), or the login must be shown as a repo admin by the permission API. Org repositories do not report `OWNER`. A `CONTRIBUTOR` who is none of those counts for nothing, so they can neither pass a tip nor hold one. An association the API did not send is not assumed, and a failed permission lookup is not admin. The permission API is for an admin GitHub did not tie to the repository: a `CONTRIBUTOR` stamp on someone else's pull request, asked only for a Tip+Verdict conclusion by a login in `reviewers`. A `selfReview` login is skipped only on that author's own report, which is the #928 carve-out and needs no lookup — skipping every `selfReview` login would never call, because the shipped policy lists every reviewer there. That lookup does not recover the own-pull-request PASS; #926 and #928 already do. The two qualifications do not replace each other: `selfReview` answers only whether an author may review their own pull request and gives that login no right on anyone else’s, while admin is read only through the permission API and only for a login already in `reviewers`. Every other check is unchanged.
3. The conclusion format is fixed (#637): the **first line** is `Verdict: PASS at <sha>` (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. A `Verdict:` line reads one of exactly four words — `PASS`, `FAIL`, `INC`, `REQUEST_CHANGES` — and a fifth such as `Incorrect` is unreadable, not invisible: the gate names the word it could not read, lists the four, and counts it as a conclusion at that tip, so it vetoes an older PASS there instead of leaving it in force (#1239); a `Verdict:` that names no tip is still an ordinary comment. The comment's `author_association` must be `OWNER`, `MEMBER` or `COLLABORATOR`, or the report must be that pull request's own author listed in `selfReview` (#928), or the login must be shown as a repo admin by the permission API. Org repositories do not report `OWNER`. A `CONTRIBUTOR` who is none of those counts for nothing, so they can neither pass a tip nor hold one. An association the API did not send is not assumed, and a failed permission lookup is not admin. The permission API is for an admin GitHub did not tie to the repository: a `CONTRIBUTOR` stamp on someone else's pull request, asked only for a Tip+Verdict conclusion by a login in `reviewers`. A `selfReview` login is skipped only on that author's own report, which is the #928 carve-out and needs no lookup — skipping every `selfReview` login would never call, because the shipped policy lists every reviewer there. That lookup does not recover the own-pull-request PASS; #926 and #928 already do. The two qualifications do not replace each other: `selfReview` answers only whether an author may review their own pull request and gives that login no right on anyone else’s, while admin is read only through the permission API and only for a login already in `reviewers`. Every other check is unchanged.
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
Expand Down
Loading