Skip to content

A PASS posted as a review body is still silent: refuse it in a write-less pull_request_review job, never in pass-label #801

Closed
Birfy/FlyBot
#23

Description

@Birfy

The gap

#767 closes one door and leaves the other open. A Verdict: PASS at <sha> whose Written-by: names someone outside writers is refused by review-gate and merge-gate, and #797 makes pass-label say so — but only for a verdict posted as a comment. A verdict posted as a review body still gets nothing: no label, and no note.

What main (fd205c40) does today:

  • .github/workflows/pass-label.yml fires on issue_comment (types [created]) and workflow_dispatch only; the job is gated on github.event.issue.pull_request != null and passes PR / COMMENT_BODY / ASSOCIATION from the comment payload.
  • .github/scripts/pass-label.mjs:99-109 (--comment) reads exactly those three values. The review body is read only by historyFrom() (:85-91), which the backfill path uses (:112-118) — and backfill only looks at pull requests with merged_at. So an open pull request whose PASS lives in a review body produces no pass label and, after #797, no refusal note either.

Why the obvious fix is refused

"Add pull_request_review to pass-label.yml" is not a two-line change; it is a permission change, and it hands the workflow file to the pull request:

  • For pull_request_review, GITHUB_REF is refs/pull/<n>/merge and GITHUB_SHA is the last merge commit — the workflow definition runs from the merge ref, not from main. A pull request that edits the workflow file changes what runs on its own review events.
  • pass-label.yml holds issues: write and pull-requests: write. Our Bots push branches inside this repository, so that token is write-capable (a fork pull request gets a read-only token; ours do not). A write-capable job whose steps come from the merge ref is a hijack path, not a trigger.
  • The workflow's own comment already states the rule for the comment path: "The comment body is untrusted text: it is only parsed, never run, and the label is added from base-branch code. pull_request on a comment would run PR code, so this stays an issue_comment workflow." review-gate.yml says the same from the other side: "pull_request (never pull_request_target), read-only token, no secrets."

So the write-capable version is an owner decision (it widens the security surface), and this issue does not take it.

Scope: a write-less door, in its own workflow

.github/workflows/review-verdict.yml, separate from pass-label.yml on purpose:

  • on: pull_request_review: types: [submitted] (plus workflow_dispatch for a manual check of one pull request, optional).
  • permissions: contents: read. No issues:, no pull-requests:, no secrets. Then a pull request editing this file can only change what its own review job prints.
  • Checkout main (not the merge ref) and run .github/scripts/review-verdict.mjs, so the logic is always main's, even though the workflow file may not be.
  • The script reads github.event.review.body, github.event.review.author_association, github.event.pull_request.number; refuses a Correction from … first line; parses with the shared parseReport and the writers list from .github/agent/review-policy.json; when the verdict is a PASS at a tip and the writer is not listed, it emits ::warning title=… naming the writer, the tip and the file to edit, and appends the same sentence to the run summary. Otherwise it prints nothing and exits 0.
  • Always exits 0: this is a report, like review-gate; a job that turns red is a job people learn to ignore.
  • Empty review body (an approve with no text), a non-PASS verdict, a listed writer and an outsider's review with no Verdict: line all end in silence.

Out of scope

  • Any write from a review-triggered workflow: no label, no comment, no check run with a conclusion. If the owner later wants the label or the note on review bodies too, the safe shape is a two-step relay: this no-permission workflow runs, then a workflow_run workflow — whose definition comes from main and which may hold write — does the commenting. That is a separate decision, and the docs' warning about artifacts applies ("do not extract in the workspace dir that may contain executable scripts").
  • pull_request_target, and any repository_dispatch from GITHUB_TOKEN (an event created with GITHUB_TOKEN does not start another workflow).
  • Changing review-gate (it must stay continue-on-error, out of branch protection and out of tests-ok's needs), and changing #797.
  • Inline review comments (pull_request_review_comment): those are the line-level discussion, not a conclusion.

Done when

  • .github/workflows/review-verdict.yml exists with permissions: contents: read and nothing writable.
  • .github/scripts/review-verdict.mjs + .github/scripts/review-verdict.test.mjs (picked up by the existing node --test ".github/scripts/*.test.mjs" in npm run verify) cover: stranger PASS → warning text names the writer, the tip and .github/agent/review-policy.json; listed writer → silence; Correction from Reviewer: … quoting a PASS → silence; empty body → silence; outsider association → silence; comment-path behaviour of pass-label.mjs unchanged.
  • A new test asserts the workflow file grants no scope other than contents: read (parse the YAML, fail on any : write).
  • A run against a pull request whose review body carries a stranger PASS shows the warning in its job summary with the writer and the tip; the job is green.

Kill conditions (each must turn a test red)

  • Drop the association check → an outsider's review warns → test red.
  • Drop the writers check → a listed writer warns → test red.
  • Add issues: write to the new workflow → the permissions test red.
  • Let the job exit non-zero on a stranger PASS → the "always green" test red.
  • Check out the merge ref instead of main → a pull request can change the script that runs → the checkout assertion red.

Relation to #767

Same defect, different door: #767 is the comment path (merged by #797), this is the review path. The boundary was recorded by Dev in #767.

Activity

  1. Birfy commented on Oct 23, 2026

    @Birfy
    CollaboratorAuthor

    agent-ready, but scoped to the write-less half on purpose.

    The naive fix — adding pull_request_review to pass-label.yml — is refused here: that event runs the workflow file from refs/pull/<n>/merge, so a pull request edits what runs on its own review, while pass-label holds issues: write. Our Bots push inside this repository, so the token is write-capable (a fork PR is read-only; ours are not).

    So the accepted shape is a separate workflow with permissions: contents: read that only emits a ::warning and a summary line naming the writer, the tip and .github/agent/review-policy.json, and always exits 0. If you want the label or the comment on review bodies too, that needs the workflow_run relay described in the body, and it is an owner decision about the security surface — say so and it becomes its own issue.

  2. added
    enhancementNew feature or request
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    area:automationScheduled tasks and automation
    on Oct 23, 2026
  3. Birfy commented on Oct 23, 2026

    @Birfy
    CollaboratorAuthor

    Written-by: Dev

    Claimed. Branch fix/801-review-verdict on Birfy/FlyBot, pull request Birfy/FlyBot#23 at bda6334f.

    Implemented in the shape this issue fixed:

    • .github/workflows/review-verdict.yml — on: pull_request_review: types: [submitted], permissions: contents: read only (no issues:, no pull-requests:, no secrets), checkout main, one ::warning plus the same sentence in the run summary, exit 0 on every path.
    • .github/scripts/review-verdict.mjs — reuses strangerPassOf from pass-label.mjs and loadPolicy from review-gate.mjs, so the comment door (fix(pass-label): tell a reviewer whose PASS the policy does not count #797) and this one apply one rule.
    • .github/scripts/review-verdict.test.mjs — 11 cases, one per kill condition.
    • docs/testing-layers.json + regenerated .md — the workflow gets its row; 319 test files / 13 workflows, all classified.

    pass-label.yml and review-gate are untouched, per the scope here.

  4. added
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    and removed
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    on Oct 23, 2026
  5. Birfy commented on Oct 23, 2026

    @Birfy
    CollaboratorAuthor

    Landed in the fork as 25876943 (#23, squash). Closed by the cross-repository Fixes line when that pull request merged; the state labels agent-working/agent-pr were cleared here because a closed issue carries history, not workflow state.

  6. removed
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    on Oct 23, 2026
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 automationenhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions