Skip to content

pr_review.py Reads unresolved=0 With an Open qodo Thread, Because qodo Posts Under a Second Login the Allowlist Does Not Know #1404

Description

@ptr727

scripts/pr_review.py tracks one qodo identity:

QODO_LOGIN = "qodo-code-review"

qodo posts under at least two. Measured across this fleet's own recent pull request history:

Repository Thread author logins seen
Utilities coderabbitai, qodo-code-review
PlexCleaner qodo-free-for-open-source-projects
ProjectTemplate copilot-pull-request-reviewer

unresolved is computed as:

unresolved = [t for t in threads if not t["isResolved"] and thread_author(t) in KNOWN_REVIEWERS]

so a thread opened by the second spelling is counted in threads= and dropped from
unresolved=.

Observed, on ptr727/PlexCleaner#954

threads=1 unresolved=0 suppressed=0 (on_head=0 earlier=0) merge=BLOCKED checks=17/18

The one thread is open. GraphQL reports isResolved: false for it, authored by
qodo-free-for-open-source-projects, carrying a Medium-severity finding. status read it as
zero unresolved, and wait exited 0 on the same reading.

wait returning clean is the part that matters. The digest is what a Merge Gate decision is
taken from, so this reports a pull request as having nothing outstanding while a finding sits
open on it, unanswered.

This is the regression #915 already fixed once

The constant carries its own warning:

# An open thread blocks a ruleset-gated merge whoever opened it, and `status`'s `unresolved=0` once silently hid a CodeRabbit/qodo thread that did block one (PR #915, ptr727/ProjectTemplate).
# Login spellings are read off this repository's own history (`gh pr view --json reviews,comments`) rather than guessed.

The generalization from "Copilot's own threads" to "every tracked reviewer's" held. What did not
hold is the membership test underneath it, because the spellings were read off this
repository's
history, and this repository has only ever seen Copilot. The two spellings that
matter appear in Utilities and PlexCleaner, which that reading never covered.

Why an allowlist keeps failing this way

Each new spelling is silent by construction: the thread lands, the count ignores it, and the digest
looks the same as a genuinely clean one. Nothing in the output distinguishes "no unresolved
threads" from "unresolved threads by an author I do not recognize", which is the property that
makes it a gate defect rather than a cosmetic miscount.

Two directions, both worth weighing rather than patching the one spelling and moving on:

  1. Count every unresolved thread, and attribute rather than filter. An open thread blocks the
    merge whoever opened it, which the comment above already says. The login set would then decide
    only how a thread is labelled in the breakdown, never whether it is counted.
  2. Keep the allowlist, and make an unrecognized author loud. Emit a distinct marker
    (unknown_authors=<login>) so the digest cannot read clean while carrying a thread it
    declined to count.

Direction 1 matches what the gate is for. Direction 2 is the smaller change and still removes the
silence. Adding qodo-free-for-open-source-projects to the tuple fixes today's instance and
leaves the mechanism exactly as fragile.

Found during the #1399 fleet resync pilot, on the review of ptr727/PlexCleaner#954.

Activity

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

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions