Update pytorchbot comment to have a PR status section - #8664
Conversation
meta-cla
Bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PytorchBotHandler.getApprovalStatus holds the app's only definition of "is this PR approved", and nothing else can reach it: pytorchBotHandler imports updateDrciComments from pages/api/drci/drci, so importing it back from the Dr.CI side would form a cycle. Move it to a leaf module so a second caller can share it. A move, not a rewrite -- the body is byte-identical apart from the two values that used to come off `this` and are now parameters (isPyTorchPyTorch(owner, repo) and ctx.log), plus one call prettier reflows at the shallower indentation. Comments and typos are carried across as-is so the diff reads as a move. Cleanups come next; the caller keeps the fetch and the "no_reviews" short-circuit. The rules had no unit tests. They do now.
Now that the rules stand alone, two problems in them are visible:
- the sort ran in place, mutating the caller's review array;
- a review whose `user` is null passed the association check and then
threw when its login was used as a map key.
Neither could bite under the merge command, which owns the array it
passes and runs inside error handling. Both would under a caller that
does not.
Split the verdict into its two steps and give the authorization check a
name while here, so the next commit can reuse the check rather than
restate it. Same rules, same result -- covered by the existing
mergeCommands tests and by new cases for both defects.
The first half of giving contributors one line that says what stage their PR is at. Pure: labels plus approval state in, markdown out. Nothing calls it yet, so this changes no comment. The stage comes from the workflow's three labels, with approval outranking them: triaged -> in pre-review, naming the assigned reviewers in progress -> in progress ready for review -> ready for maintainer review approved -> Approved, with the merge command The wording is contributor-facing policy quoted from the contributor workflow spec and should change only alongside CONTRIBUTING.md. A PR with none of the labels renders nothing, which is how the comment stays byte-identical for every PR outside the workflow. formDrciComment takes the section as a parameter, and splice/extract helpers let a caller that cannot rebuild the comment edit just this part of it. Those two must produce identical bytes for the same state or the sweep's no-op check stops firing and every sweep rewrites every comment, so a test pins them against the real formDrciComment. Reviewer names are the only interpolated value and are filtered to what GitHub can actually issue, so the section cannot carry markdown, HTML, or one of the literals the sweep's re-render query greps for.
The second half: the two things labels alone cannot give the renderer -- whether the PR is approved, and who is assigned to review it. Still no callers. Both come from the GitHub API rather than the ClickHouse mirror. The mirror carries this data and a batched read would be cheaper, but it lags GitHub, and a section that renders from stale state is the specific failure this is shaped to avoid. The cost is bounded: callers gate on the PR carrying a status label, so PRs outside the workflow pay nothing, and the reviewer read only happens for the one stage whose message names reviewers. Every input is one a webhook fires on, which is a constraint worth keeping. An earlier version named only reviewers who had not yet reacted to the PR description -- but GitHub emits no webhook for reactions, and the Dr.CI sweep only visits PRs with recent CI activity, so on a quiet PR that list could stay wrong indefinitely. Assigned reviewers are requested_reviewers unioned with people who have already reviewed, because GitHub drops a reviewer from that list the moment they submit anything, a plain comment included. The union reuses the approval authorization check and excludes the PR author: without the first, any account could leave one review comment to insert itself permanently into a bot-authored sentence about who must sign off; without the second, an author replying inline would be listed as owing agreement on their own change. Each read degrades on its own rather than failing the section, except that losing the author identity drops the reviewer list entirely -- an understated list beats one that names the author.
First commit with a user-visible effect: the 15-minute sweep now renders the section, so a PR carrying a status label gets the line. PRs without one are unaffected, and on pytorch/pytorch today that is all of them -- the bots that apply the labels are not part of this change. The sweep's own labels come from the ClickHouse mirror, which lags. Every other consumer tolerates that; this one cannot, because the sweep rebuilds the whole comment. A stale read in one direction deletes a line a webhook just wrote, and in the other resurrects one that was just removed -- either way the section flaps on and off across sweeps. So the render uses labels read live from GitHub, and the mirror's labels only decide whether that read is worth making. That cheap gate also opens when the existing comment already carries a section, which covers the just-labelled case the labels alone would miss. It narrows rather than closes that direction: the evidence comes from a different mirror, so it only helps when that one is fresher. The residue is the pre-existing transient, repaired by the next sweep. Wrapped so a GitHub error costs the status line rather than the comment.
The sweep runs every 15 minutes and only for PRs with recent CI activity, so on its own the status line can sit wrong for a long time on a quiet PR -- exactly the PRs whose contributors most need to know what is expected of them. Update the section from the events that actually move a PR between stages: label changes, review requests, and reviews. These handlers splice only the section into the existing comment. They have no CI classification of their own, so rebuilding the comment would blank out everything the sweep put there. A PR with no Dr.CI comment yet is left alone rather than given a resultless one that races the sweep. Guarded so this stays cheap on a shared installation token: pytorch/pytorch churns ciflow/* and module:* labels constantly, and only the events that can change what the section renders are worth a request. Also stop upsertDrCiComment dropping the section. It rebuilds on push with no status inputs, so without carrying the existing section across it would delete the line on every commit and leave it gone until the next sweep, undoing the point of maintaining it by webhook.
e7ce42a to
3285df6
Compare
izaitsevfb
left a comment
izaitsevfb
left a comment
There was a problem hiding this comment.
the approach is ok with me, but please take a look at the nits below
| // for the PRs -- the large majority -- that are not in the workflow yet. | ||
| export function hasPrStatusLabel(labels: string[]): boolean { | ||
| return PR_STATUS_LABELS.some((label) => labels.includes(label)); | ||
| } |
There was a problem hiding this comment.
please make sure the label name matches.
🔴 The ready-for-review stage can never match, because the label pytorch/pytorch has is spelled `Ready for Review`. (ai-generated section)
hasPrStatusLabel compares with Array.includes and getPrStatusStage with Set.has, both exact string comparisons against the constant "ready for review". pytorch/pytorch carries a label named Ready for Review, described "Automated review passed, ready for a final review by a maintainer", and no lowercase spelling of it exists in any repository isDrCIEnabled covers. A PR at that stage therefore falls through to whichever earlier label it still carries, or renders no note at all — silently, since there is nothing to log. Comparing lower-cased on both sides, or matching the label as it is actually spelled, closes it.
Reviewed by codex gpt-5.6-sol at xhigh effort, against 3285df6.
| return "preReview"; | ||
| } | ||
| return "none"; | ||
| } |
There was a problem hiding this comment.
please narrow the comment to pytorch/pytorch only!
🔴 `triaged` already means something else on these repositories, so this would put the pre-review note on 1,637 open pull requests that are not in this workflow. (ai-generated section)
getPrStatusStage returns the pre-review stage for any PR labelled triaged, and the comment then reads "PR Status: in pre-review. All assigned reviewers (...) must agree by reacting to the PR description that this change is worth pursuing before the PR will be marked in progress." A label named triaged already exists in eight of the repositories isDrCIEnabled covers. In pytorch/pytorch it is the module-triage label — its own description is "This issue has been looked at a team member, and triaged and prioritized into an appropriate module" — and 1,637 currently-open pull requests there carry it, against zero for either of the other two stage labels. If reusing that label is deliberate, that population is the rollout and is worth saying so in the description; if it is not, a label name unique to this workflow avoids it.
Reviewed by codex gpt-5.6-sol at xhigh effort, against 3285df6.
|
|
||
| const allowBots = isPyTorchPyTorch(owner, repo); | ||
| const isApproved = | ||
| getApprovalStatusFromReviews(reviews as any, allowBots) === PR_APPROVED; |
There was a problem hiding this comment.
looks like a minor bug.
🟡 A failed review lookup on the approval webhook rewrites a published "Approved" note as an earlier stage. (ai-generated section)
fetchPrStatusState treats a rejected listReviews as an empty review list, so isApproved becomes false and the stage falls back to the labels. The code documents that fallback as showing "the state the PR was in a moment ago", which is true of the sweep. It is not true on the pull_request_review.submitted path, where the approval that just arrived IS the state a moment ago: the handler writes the downgraded sentence into the comment, and it stays there until the next sweep or the next stage-changing event. Leaving the existing section untouched when the review read failed would keep an unknown from being published as a negative.
Reviewed by codex gpt-5.6-sol at xhigh effort, against 3285df6.
| ) | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
I don't think that's intended?
⚪ A single review comment from anyone who has previously contributed adds them to the list of reviewers the PR is said to be waiting on. (ai-generated section)
getReviewerLogins recovers reviewers GitHub dropped from requested_reviewers, keeping anyone whose author_association is COLLABORATOR, CONTRIBUTOR, MEMBER or OWNER, and it does not look at the review's state — a plain commented review counts. On GitHub, CONTRIBUTOR means only that the person has had a commit merged into the repository before. Their login is then rendered into "All assigned reviewers (@them) must agree by reacting to the PR description", which is a claim about who is blocking the PR. Filtering on review state is not the fix — a genuinely requested reviewer who leaves only a comment is exactly the case this recovery exists for — so the choice is in which associations qualify.
Reviewed by codex gpt-5.6-sol at xhigh effort, against 3285df6.
2 participants
No description provided.