Skip to content

Read a stop request written on a review comment line #2091

Closed

Description

@HugoXK

Implementation: FlyBot (ours) — PR follows within 24h. Do not pick up without a pickup-ok comment on this issue.

Written-by: FlyBot Lead
Claimed-by: FlyBot Lead
Claim-Id: 3152749ab2e3458d91c4a97724d7cee9
Branch: dev/1-inline-review-stop
Base: fca35af
At: 2026-11-02T01:11:58Z

Implementation: FlyBot (ours) — PR follows within 24h. Do not pick up without a pickup-ok comment.

Written-by: FlyBot Lead

What happens

A stop request written on a review comment line ("please do not merge this tip") is silently ignored by both gates that call stopRequestAt. The reader already accepts such a row — the row is never fetched.

On live main fca35af6f29a10214f239b78a2087b82845ba4ce, the two builders that feed stopRequestAt collect only two surfaces each:

  • .github/scripts/enqueue-guard.mjs:68 — repos/${repo}/issues/${pr}/comments
  • .github/scripts/enqueue-guard.mjs:69 — repos/${repo}/pulls/${pr}/reviews
  • .github/scripts/merge-gate.mjs:218 — repos/${repo}/issues/${pr}/comments
  • .github/scripts/merge-gate.mjs:219 — repos/${repo}/pulls/${pr}/reviews

The third surface, repos/${repo}/pulls/${pr}/comments (the inline comments written on lines of the diff), appears nowhere in .github/scripts/: grep -n 'pulls/\${pr}/comments' .github/scripts/*.mjs on fca35af6 returns no hit.

The reader is not the gap. stopRequestAt (enqueue-rules.mjs:181) matches any row kind since #2085 dropped its kind === 'comment' filter.

Reproduction (live main fca35af6)

Same sentence, three placements:

  1. As a review body → ok=false, stop.id is that review's id (this is Read a stop request written in a review body #2085, working).
  2. As an issue comment → ok=false, stop.id is that comment's id (this is Refuse to enqueue when a comment asks not to merge this tip #2073, working).
  3. As an inline review comment on a diff line → stopRequestAt([that row]) returns { id, phrase: "Please do not merge" } (the reader would catch it), but reports as built by the two callers holds 0 rows from pulls/{pr}/comments, so guard() returns ok=true, stop=null, reasons=[].

Expected

An inline review comment that asks not to merge this tip must refuse:

  • the queue entry (enqueue-guard), and
  • the queue mode of the merge gate (merge-gate with mode: 'queue'),

with the decision naming that review comment's id, and the note saying the request was refused because of it. The existing rules must carry over unchanged for this surface: the since window (a stop older than the PASS at this tip is not this tip's hold), quote/code-block stripping (withoutQuotes), and the author/type filter (only the PR author or a member/maintainer type can hold it). A reply on an inline thread (in_reply_to_id) counts as the same request, read once.

Scope

  • .github/scripts/enqueue-guard.mjs and .github/scripts/merge-gate.mjs: add repos/${repo}/pulls/${pr}/comments to the report rows, with kind: 'review-comment', at: created_at, id, login, type, association, and the same empty-body filter.
  • .github/scripts/enqueue-rules.mjs: kind: 'review-comment' rows feed stopRequestAt only. A conclusion is never read from an inline comment line: the "record only" reader and the merge-gate judge skip that kind, so an inline Verdict: PASS at <sha> is neither a stop nor a verdict.
  • Tests: one red-first case per caller (enqueue-guard refuses and names the review-comment id; merge-gate's mode: 'queue' refuses through main()), plus the negative cases above. Both fake gh routes must be split: today they dispatch on path.includes('/comments') (enqueue-guard.test.mjs:92, merge-gate.test.mjs:170), so issues/N/comments and pulls/N/comments collide in one array and a new case can pass while reading the wrong surface.

Out of scope

  • .github/scripts/review-gate.mjs and its decide(): it has no stopRequestAt (guard() is the only caller), so this surface adds nothing to a hold there. "A conclusion may be written on a line of the diff" is a separate decision and is not made here.
  • An empty-body Request changes review whose text exists only on an inline thread.
  • Anything under core/, the client, or the docs.

Evidence at the fix (ready before this issue was opened)

Red first on live main source with the final test files, three runs identical:

  • enqueue-guard.test.mjs 104 / 102 / 2 — an inline review comment asking not to merge this tip refuses too, and names it; a reply on an inline thread is the same request, and it is read once
  • merge-gate.test.mjs 51 / 50 / 1 — an inline review comment asking not to merge refuses the queue entry, through main()

After the fix, three runs: 104/104/0 and 51/51/0. Mutation arms (each applied alone, then restored): drop the enqueue-guard surface → the two reds above; drop the merge-gate surface → 51/50/1; drop the since window → both new window cases plus the existing ones go red; let the judge read conclusions from inline rows → a conclusion written on one line of the diff is a request reader only, never a verdict; let the "record only" reader use them → a "record only" marker on one line of the diff does not hold the entry; read issues/{pr}/comments instead of pulls/{pr}/comments → the two enqueue-guard reds, which is what proves the route split bites.

npm run verify with ARCH_DOCS_BASE=fca35af6: exit 0 — core 2557, social 541, tree 127, timeline 135, workflow 336, .github/scripts 584, interop 37, live-e2e 24; lint 73/0 and 36/9/2/16; arch-docs: OK (5 changed file(s)).

Honest note: pulls/{pr}/comments is empty on #2073–#2085, our own reviews do not use inline comments. This surface is for the owner and future contributors, not a path our Bots walk. It still deserves the fix because the failure is silent: a hold written there is read by nothing.

Dedup before opening: on live main fca35af6, grep -n 'pulls/\${pr}/comments' .github/scripts/*.mjs → no hit, so the gap is open. Closed issues cover the other two surfaces only — #2073 (issue comments), #2084/#2085 (review bodies) — and no open or closed issue addresses inline review comments as a stop source.

Activity

  1. HugoXK commented on Nov 2, 2026

    @HugoXK
    ContributorAuthor

    Release

    Written-by: FlyBot Lead
    Released: 3152749ab2e3458d91c4a97724d7cee9
    Reason: Merged as 2998628 (#2092) at 2026-11-02T01:41:16Z; issue closed.
    At: 2026-11-02T01:44:01Z

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

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions