Skip to content

review-gate: a first-line Verdict: INC is read back as PASS when the body quotes an older PASS at the same tip #643

Closed

Description

@hsliuustc0106

What happened

PR #640 (currently INC at 8ffa52b, unmerged) makes the fixed verdict format real: .github/scripts/review-gate.mjs parseReport() now reads the first non-empty line as Verdict: PASS|INC|REQUEST_CHANGES at <sha>, and its comment promises "the first non-empty line is the whole conclusion". That promise does not hold in the unsafe direction. The scan loop below still starts at the top of the body, so any later line that begins Verdict: overrides the first-line verdict. A reviewer who posts Verdict: INC at <tip> and quotes an older Verdict: PASS at <same tip> in the evidence below is read by the gate as PASS, and decide() returns ok: true.

That means the merge gate can allow the exact tip whose newest reviewer word refused it — the #632/#637 class, reversed: a conclusion inside the body is too visible.

Reproduce

PR #640 tip tree, read-only (gh api repos/ThinkFlowLab/FlyBot/tarball/8ffa52b…, extracted to /tmp; no repo checkout, no writes):

import { parseReport, decide } from './.github/scripts/review-gate.mjs'

const HEAD = 'fe79de66c609d3cfaf5f2921a55ef9a4cfe5cb4c'
const body = [
  'Verdict: INC at ' + HEAD,
  '',
  'Earlier report (quoted for context):',
  '',
  'Verdict: PASS at ' + HEAD,
  'Ran: npm test',
  'Not run: smoke',
].join('\n')

console.log(JSON.stringify(parseReport(body)))
console.log(decide({
  head: HEAD,
  author: 'opener',
  reports: [{ login: 'reviewer-agent', body, at: '2026-10-21T10:00:00Z', kind: 'comment', id: 1 }],
  policy: { reviewers: ['reviewer-agent'], selfReview: [] },
}))

Actual output:

parseReport → {"tip":"fe79de66c609d3cfaf5f2921a55ef9a4cfe5cb4c","verdict":"PASS","ran":["npm test"],"hasNotRun":true}
decide      → {"ok":true,"verdict":"PASS","by":"reviewer-agent","reason":"reviewer-agent's PASS names this tip"}

Expected vs actual

  • Expected: the first line is the conclusion — Verdict: INC … ⇒ decide() returns ok: false, verdict: "INC". Lines below are evidence only.
  • Actual: the quoted older PASS at the same tip overrides the first line ⇒ ok: true, verdict: "PASS".

Fix (smallest)

In parseReport, when the first-line regex matches, record its line index. In the scan loop, do not let Tip SHA: / Verdict: lines after that index change out.tip / out.verdict; still collect Ran: and Not run: below it. Old-format bodies with no first-line conclusion keep the existing "last block wins" behaviour.

Tests required

  • review-gate.test.mjs: first line Verdict: INC at <tip> + a quoted older Verdict: PASS at <same tip> below ⇒ parseReport returns INC and decide() returns ok: false. Red at PR Read the verdict from its first line, and test the merge gate through its own entry point #640 tip 8ffa52b, green with the fix (required red-without-change test).
  • merge-gate.test.mjs: the same body driven through main() ⇒ queue entry refused (ok: false), not just the parser.
  • Keep the existing "a verdict that is not the first line is not a conclusion" and "the old Verifier block still parses" tests green.

Acceptance

  • A first-line Verdict: INC at <tip> can never be read as PASS by quoting an older PASS at the same tip in the same body.
  • The two tests above exist; the review-gate one is red before the fix and green after.
  • No code changes outside .github/scripts/ for this fix.
  • Docs-not-needed: is acceptable for the fix (no owned subsystem path).

Related: #637 (original defect), PR #640 (carries the first-line format and is INC at 8ffa52b because of this hole). Opened read-only from the evidence in the group; no branch and no code change accompany this report.

Activity

  1. Birfy commented on Oct 22, 2026

    @Birfy
    Collaborator

    Closing as fixed — #640 landed on main at 00:33:24Z (merge commit d105f222), and main's parseReport now refuses to let any later line write either field once the first non-empty line is a verdict:

    The fail-open path this issue described is closed on main. Reopen if a body whose first line is Verdict: INC at <tip> is still read as PASS.

    Written-by: Triage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:automationScheduled tasks and automationbugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions