NAS-144026 / 27.0.0-BETA.1 / Request a team review when a human review is needed - #19
william-gr wants to merge 3 commits into
Conversation
The needs-a-human verdict now asks the calling org's team (default ux-team) for a review; the team's auto-assignment picks the person. Once per PR: skipped when any reviewer is already requested, or when an earlier verdict recorded a request, so pushes do not reassign it. Best effort, and the body names the team without an @-mention so only the assigned reviewer is notified.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const team = `${repo.split('/')[0]}/${humanReviewTeam}`; | ||
| try { | ||
| const requested = await api(`/pulls/${number}/requested_reviewers`); | ||
| if (requested.users?.length || requested.teams?.length) { |
There was a problem hiding this comment.
LOW — This guard also disables the feature outright in any consumer with a CODEOWNERS file. GitHub auto-requests code owners when the PR opens, so requested.users/requested.teams is already non-empty on the first verdict run and the team is never asked. The comment above frames the guard as being about pushes not reassigning ("so each push does not assign someone new"), and README's own CODEOWNERS bullet a few lines below the new one documents that configuration as supported — so a reader of either would not expect the request to be skipped from the start.
Nothing produces a wrong result (someone is already requested, which is arguably the point), so this is LOW rather than a bug. Worth a sentence in the README bullet, or narrowing the guard to requested.teams plus reviewers the workflow itself requested, if the intent is that ux-team gets asked even when code owners already were.
| console.log('A reviewer is already requested; not requesting the team.'); | ||
| return []; | ||
| } | ||
| const reviews = await paginate(`/pulls/${number}/reviews`); |
There was a problem hiding this comment.
LOW — /pulls/{n}/reviews is now paginated twice on the needs-a-human path: here, and again in dismissOwnStateReviews a few statements later (line 232), which runs immediately after the submit('COMMENT', …) on the same path. Same endpoint, same PR, up to 30 pages each, and the second call is the only one that needs to be after the new review exists.
Fetching once before requestTeamReview and passing the list into both would halve it. Not a correctness problem — just duplicated API work on the busiest path.
|
Three findings: three LOW. Nothing blocks. A human should look at this one. The mechanism itself holds up.
Findings
Needs a human review. The unresolved thread at |
There was a problem hiding this comment.
Needs a human review. 2 finding(s): 2 LOW.
Nothing blocks, but this change is one a person should decide on:
- .github/workflows/claude-review.yml:131 sets default: 'ux-team', a caller-visible default that opts every @master consumer into requesting reviewers from real people with no change on their side
…uard The self-review run confirmed the job token cannot request team reviewers (422, "Could not resolve to a node"), so the request is now skipped with a log line unless a github-token is passed, rather than a warning on every needs-human run. The guard stays on any requested reviewer: truenas-connect/ui and webui name ux-team in CODEOWNERS, and after auto-assignment replaces the team with a user, a team-only check would assign a second person. README says so. Reviews are fetched once on the needs-human path.
| |---|---|---| | ||
| | Anything at MEDIUM or above | REQUEST_CHANGES | fails | | ||
| | Only LOW or none, a human must look | COMMENT listing the reasons | passes, no approval | | ||
| | Only LOW or none, a human must look | COMMENT listing the reasons, and a review request to `human-review-team` | passes, no approval | |
There was a problem hiding this comment.
LOW — This row promises the review request unconditionally, but the request only happens when a github-token is passed, no reviewer is already requested, and no earlier verdict recorded one. github-token is optional and defaults to empty, so for a caller on the default configuration this row describes something that never occurs — the log line "Not requesting …: the job token cannot request team reviewers" is the whole behaviour. The input table and the branch-protection prose both state the condition; only this summary row omits it. A qualifier such as "and, with a github-token, a review request to human-review-team" would make the three agree.
| return all; | ||
| }; | ||
|
|
||
| // Fetched before the verdict is posted; every consumer excludes that review by id. |
There was a problem hiding this comment.
LOW — Both halves of this comment are narrower than the code. The cache is fetched before the verdict only on the needs-a-human path, where requestTeamReview() primes it; on the approve-when-clean: false and refused-APPROVE paths dismissOwnStateReviews is the first caller and fetches after submit(). And "every consumer excludes that review by id" is not true of requestTeamReview, the consumer this cache was added for — it matches on the two body markers and never looks at id. It does not need to, precisely because it runs first.
Nothing misbehaves today; the hazard the comment guards against is real but stated as an invariant that the file does not hold. Something like "may be populated before the verdict is posted, so callers that must not see it exclude it by id" would describe what is actually here.
There was a problem hiding this comment.
Needs a human review. 2 finding(s): 2 LOW.
Nothing blocks, but this change is one a person should decide on:
- .github/workflows/claude-review.yml:129 defaults human-review-team to ux-team, so the four repos calling this at @master start requesting reviewers with no change on their side.
| * clean, but a human must look COMMENT naming why, team review | ||
| * requested once per PR, job passes |
There was a problem hiding this comment.
LOW — This outcome table now says the human-review path requests a team review "once per PR", unqualified, but requestTeamReview returns early without a github-token (line 269) — which is the default for every consumer, including this repo's own claude-review-self.yml, which maps no github-token. The README's equivalent table row was qualified with "and with a github-token" in d291001; this header comment is the same claim left unqualified.
| * clean, but a human must look COMMENT naming why, team review | |
| * requested once per PR, job passes | |
| * clean, but a human must look COMMENT naming why, and with a | |
| * github-token a team review | |
| * requested once per PR, job passes |
| // On the flag, not the reason count: `required: true` with no reasons is | ||
| // schema-valid and must not approve. | ||
| if (humanReview.required || reasons.length > 0) { | ||
| const requestLines = await requestTeamReview(); |
There was a problem hiding this comment.
LOW — The "once per PR" record is written by the review posted after the request, so a request that lands while the review that would record it does not leaves no trace of itself.
Sequence: run A requests the team, then submit('COMMENT') throws (a dropped connection, a 5xx, the 15s AbortSignal.timeout) and the outer catch at line 383 turns it into a warning — no review body carries REQUESTED_MARKER. Auto-assignment swaps the team for Alice, Alice submits a review, and GitHub clears her from requested_reviewers. The next push reaches run B with both guards open: the requested list is empty and no review carries the marker, so it requests the team again and a second person is assigned.
Narrow, and the feature is best effort by design, so this is not worth much — but it is the one case the marker exists to cover and does not. Requesting after the review is posted, with the marker in the body either way, would close it.
|
|
||
| Making the review count is branch protection, per repo: | ||
|
|
||
| - **Turn on code review auto-assignment for the team** named by |
There was a problem hiding this comment.
LOW — This bullet sits under "Making the review count is branch protection, per repo:" (line 327), but it is neither: code review auto-assignment is an org team setting, configured once per org rather than per repo, and it does not make the review count towards anything branch protection checks. Every other bullet in the list is a repository branch-protection or Actions setting. A reader following the section heading to their repo's protection rules will not find it there.
It reads as its own paragraph before the list — something like "The team request needs one setting outside this repo:" ahead of it, with the branch-protection list starting at Require 1 approval.
There was a problem hiding this comment.
Needs a human review. 3 finding(s): 3 LOW.
Nothing blocks, but this change is one a person should decide on:
- .github/workflows/claude-review.yml:131 - human-review-team defaults to ux-team, turning reviewer requests on for every consumer at @master with a github-token, with no opt-in
- review/submit-verdict.mjs:284 - adds a write the workflow never made before, POST /pulls/{n}/requested_reviewers, assigning work to people in three orgs
bugclerk
changed the title
3 participants
Summary
When the verdict is "needs a human review", the workflow now requests a review from a team in the calling repository's org. The team's code review auto-assignment picks who gets it.
human-review-team, defaultux-team. Empty disables the request.ux-teamexists in truenas-connect, iXsystems and truenas, with write on all five consumer repos.@-mention, which would notify every member.Verified against a mocked API: first request, skip when a reviewer is already requested, skip after an earlier request, refused request, empty input, and no request on the approve path.
Needs doing outside this PR
ux-team's settings in each of the three orgs. Without it the whole team stays requested.github-token. This PR's first self-review confirmed the job token cannot request team reviewers: GitHub answered 422, "Could not resolve to a node with the global id" of the team. Without agithub-tokenthe request is skipped with a log line. A fine-grained PAT may also need organisation Members read; that part is unverified.ux-teamfor every file, so GitHub requests the team on every PR and this request is skipped. It adds value in truenas-file-manager, truenas-ui-components and api-client-ts.No Jira ticket on hand for the title; add one so
PR ticketpasses.