github-actions[bot] commented on code in PR #40753:
URL: https://github.com/apache/beam/pull/40753#discussion_r4363028454
##########
scripts/ci/pr-bot/processPrUpdate.ts:
##########
@@ -220,6 +221,17 @@ async function processPrUpdate() {
await setNextActionReviewers(payload, pull, stateClient);
} else if (payload.action === "review_requested") {
console.log("Processing review_requested action");
Review Comment:
**🟠 High**: If a bot reviewer (such as GitHub Copilot, where
`payload.requested_reviewer.type === "Bot"`) is requested on a PR automatically
or via the UI, `payload.requested_reviewer?.login` is still truthy. Recording a
bot in `prState.reviewersAssignedForLabels` will cause `processNewPrs.ts` to
see an assigned reviewer and skip assigning a human reviewer after checks pass,
and `findPrsNeedingAttention.ts` will later ping the bot for slow reviews.
Guard against bot accounts here.
```suggestion
if (payload.requested_reviewer?.login &&
payload.requested_reviewer?.type !== "Bot") {
```
##########
scripts/ci/pr-bot/processPrUpdate.ts:
##########
@@ -220,6 +221,17 @@ async function processPrUpdate() {
await setNextActionReviewers(payload, pull, stateClient);
} else if (payload.action === "review_requested") {
console.log("Processing review_requested action");
+ if (payload.requested_reviewer?.login) {
+ let prState = await stateClient.getPrState(pull.number);
+ const reviewersForLabels = reviewerConfig.getReviewersForLabels(
+ pull.labels,
+ [pull.user.login]
+ );
+ const label = Object.keys(reviewersForLabels)[0] ||
NO_MATCHING_LABEL;
+ prState.reviewersAssignedForLabels[label] =
+ payload.requested_reviewer.login;
Review Comment:
**🟡 Medium**: `stateClient.writePrState` performs a full `git pull`, `git
commit`, and `git push` to the `pr-bot-state` branch, and
`setNextActionReviewers` on line 234 immediately reads `prState` and calls
`stateClient.writePrState` a second time for non-draft PRs. Also, if multiple
reviewers are requested for the same label,
`prState.reviewersAssignedForLabels[label]` will overwrite the previously
recorded reviewer so `getLabelForReviewer` will no longer recognize the earlier
reviewer.
##########
scripts/ci/pr-bot/processPrUpdate.ts:
##########
@@ -220,6 +221,17 @@ async function processPrUpdate() {
await setNextActionReviewers(payload, pull, stateClient);
} else if (payload.action === "review_requested") {
console.log("Processing review_requested action");
+ if (payload.requested_reviewer?.login) {
+ let prState = await stateClient.getPrState(pull.number);
+ const reviewersForLabels = reviewerConfig.getReviewersForLabels(
+ pull.labels,
+ [pull.user.login]
+ );
Review Comment:
**🟠 High**: When a reviewer is requested upfront during PR creation, the
`review_requested` event fires immediately—before the `LabelPrs` workflow has
had time to apply path-based component labels (`pull.labels` is `[]`). As a
result, `getReviewersForLabels` falls back to `NO_MATCHING_LABEL`
(`"no-matching-label"`), storing
`prState.reviewersAssignedForLabels["no-matching-label"] = reviewer`.
Later, if that reviewer is not a committer and approves the PR,
`processNewPrs.ts` calls `let labelOfReviewer =
prState.getLabelForReviewer(approver)`. Because `"no-matching-label"` is
truthy, `processNewPrs.ts` skips its `!labelOfReviewer` fallback that inspects
`pull.labels` and instead calls
`reviewerConfig.getReviewersForLabel("no-matching-label")` (which returns
`[]`), bypassing component-specific committers and always falling back to
`fallbackReviewers` (and similarly breaking `assign to next reviewer` in
`userCommand.ts`).
Consider either updating `processNewPrs.ts` (and `userCommand.ts`) to
re-resolve the label from `pull.labels` when `labelOfReviewer ===
NO_MATCHING_LABEL`, or checking if the reviewer belongs to one of the
configured labels in `reviewerConfig`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
