From 6f03c2540ba9686c45dc0ff4ad7b31f8d05fe8f1 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Wed, 7 Oct 2026 10:16:23 -0700 Subject: [PATCH] Show whether a PR is approved in git pr An open PR that was not approved printed "Ready". It now prints "Review required" when the base branch requires an approval, and "Not approved" when it requires none. Without a review decision, the status comes from each reviewer's last approval, change request, or dismissal, so a comment after an approval no longer hides it. Claude-Session: https://claude.ai/code/session_01FLq8eNPjDPRau2RPMnVDTj --- ai/skills/address-pr-reviews/scripts/git-pr | 32 ++++++++---- ai/skills/wait-for-pr-reviews/scripts/git-pr | 32 ++++++++---- bin/git-pr | 32 ++++++++---- bin/lib/test-git-pr.sh | 55 ++++++++++++++++---- 4 files changed, 112 insertions(+), 39 deletions(-) diff --git a/ai/skills/address-pr-reviews/scripts/git-pr b/ai/skills/address-pr-reviews/scripts/git-pr index 85c71ac7..c49c167e 100755 --- a/ai/skills/address-pr-reviews/scripts/git-pr +++ b/ai/skills/address-pr-reviews/scripts/git-pr @@ -1,9 +1,13 @@ #!/usr/bin/env bash # Prints the URL of the PR for the current branch, or for an explicit # / argument (passed straight to `gh pr view` unchanged). -# On a terminal the URL is followed by the PR's status: Draft, Ready, Approved, -# Changes requested, Merged, or Closed. An open PR that a Trunk merge queue has -# taken also shows its queue status, such as "Approved, Submitted to Trunk Queue". +# On a terminal the script prints the PR's status after the URL: Draft, Review +# required, Not approved, Approved, Changes requested, Merged, or Closed. +# Review required means the base branch requires an approval that the PR lacks. +# Not approved means the base branch requires no approval and the PR has no +# current approval. +# An open PR that a Trunk merge queue has taken also shows its queue status, +# such as "Approved, Submitted to Trunk Queue". # When other open PRs target this PR's branch, or this PR targets another open # PR's branch, the stack follows on separate lines, top-down: the PRs stacked on # this one, this PR marked with ">", the PRs it sits on, and the bottom base @@ -34,22 +38,30 @@ set -e # Both field sets include state for that lookup. if [ -t 1 ]; then # GitHub leaves reviewDecision empty when the base branch requires no reviews. - # latestReviews still records approvals and change requests in that case. + # The status then comes from each reviewer's last review that approved, + # requested changes, or was dismissed. latestReviews would hide an approval + # that the same reviewer followed with a comment. + # `gh pr list` returns only the oldest 100 reviews of a PR. An approval after + # those 100 does not count. # Trunk edits one status comment in place. The first line of that comment # names the queue state. Trunk posts submission errors as separate comments. # The status comment is therefore the last one whose first line has a known # phrase. The un-submitted control comment and the test analytics comment # match no phrase. - format='def status: + format='def review_status: + reduce (.reviews[] | select(.state != "COMMENTED" and .state != "PENDING")) as $r + ({}; .[$r.author.login // ""] = $r.state) + | if any(.[]; . == "CHANGES_REQUESTED") then "Changes requested" + elif any(.[]; . == "APPROVED") then "Approved" + else "Not approved" end; + def status: (if .state == "MERGED" then "Merged" elif .state == "CLOSED" then "Closed" elif .isDraft then "Draft" elif .reviewDecision == "APPROVED" then "Approved" elif .reviewDecision == "CHANGES_REQUESTED" then "Changes requested" - elif (.reviewDecision // "") != "" then "Ready" - elif any(.latestReviews[]; .state == "CHANGES_REQUESTED") then "Changes requested" - elif any(.latestReviews[]; .state == "APPROVED") then "Approved" - else "Ready" end) + elif .reviewDecision == "REVIEW_REQUIRED" then "Review required" + else review_status end) + ([select(.state == "OPEN") | .comments[]? | select(.author.login == "trunk-io") | .body | split("\n")[0] | ascii_downcase | if contains("removed from the merge queue") then "Removed from Trunk Queue" @@ -63,7 +75,7 @@ if [ -t 1 ]; then # Branch names hold no spaces, so only the status in a format line can # contain one. # The retry below drops the trailing comments field, so it stays last. - fields=url,number,state,isDraft,reviewDecision,latestReviews,headRefName,baseRefName,isCrossRepository,comments + fields=url,number,state,isDraft,reviewDecision,reviews,headRefName,baseRefName,isCrossRepository,comments else fields=url,state format=.url diff --git a/ai/skills/wait-for-pr-reviews/scripts/git-pr b/ai/skills/wait-for-pr-reviews/scripts/git-pr index 85c71ac7..c49c167e 100755 --- a/ai/skills/wait-for-pr-reviews/scripts/git-pr +++ b/ai/skills/wait-for-pr-reviews/scripts/git-pr @@ -1,9 +1,13 @@ #!/usr/bin/env bash # Prints the URL of the PR for the current branch, or for an explicit # / argument (passed straight to `gh pr view` unchanged). -# On a terminal the URL is followed by the PR's status: Draft, Ready, Approved, -# Changes requested, Merged, or Closed. An open PR that a Trunk merge queue has -# taken also shows its queue status, such as "Approved, Submitted to Trunk Queue". +# On a terminal the script prints the PR's status after the URL: Draft, Review +# required, Not approved, Approved, Changes requested, Merged, or Closed. +# Review required means the base branch requires an approval that the PR lacks. +# Not approved means the base branch requires no approval and the PR has no +# current approval. +# An open PR that a Trunk merge queue has taken also shows its queue status, +# such as "Approved, Submitted to Trunk Queue". # When other open PRs target this PR's branch, or this PR targets another open # PR's branch, the stack follows on separate lines, top-down: the PRs stacked on # this one, this PR marked with ">", the PRs it sits on, and the bottom base @@ -34,22 +38,30 @@ set -e # Both field sets include state for that lookup. if [ -t 1 ]; then # GitHub leaves reviewDecision empty when the base branch requires no reviews. - # latestReviews still records approvals and change requests in that case. + # The status then comes from each reviewer's last review that approved, + # requested changes, or was dismissed. latestReviews would hide an approval + # that the same reviewer followed with a comment. + # `gh pr list` returns only the oldest 100 reviews of a PR. An approval after + # those 100 does not count. # Trunk edits one status comment in place. The first line of that comment # names the queue state. Trunk posts submission errors as separate comments. # The status comment is therefore the last one whose first line has a known # phrase. The un-submitted control comment and the test analytics comment # match no phrase. - format='def status: + format='def review_status: + reduce (.reviews[] | select(.state != "COMMENTED" and .state != "PENDING")) as $r + ({}; .[$r.author.login // ""] = $r.state) + | if any(.[]; . == "CHANGES_REQUESTED") then "Changes requested" + elif any(.[]; . == "APPROVED") then "Approved" + else "Not approved" end; + def status: (if .state == "MERGED" then "Merged" elif .state == "CLOSED" then "Closed" elif .isDraft then "Draft" elif .reviewDecision == "APPROVED" then "Approved" elif .reviewDecision == "CHANGES_REQUESTED" then "Changes requested" - elif (.reviewDecision // "") != "" then "Ready" - elif any(.latestReviews[]; .state == "CHANGES_REQUESTED") then "Changes requested" - elif any(.latestReviews[]; .state == "APPROVED") then "Approved" - else "Ready" end) + elif .reviewDecision == "REVIEW_REQUIRED" then "Review required" + else review_status end) + ([select(.state == "OPEN") | .comments[]? | select(.author.login == "trunk-io") | .body | split("\n")[0] | ascii_downcase | if contains("removed from the merge queue") then "Removed from Trunk Queue" @@ -63,7 +75,7 @@ if [ -t 1 ]; then # Branch names hold no spaces, so only the status in a format line can # contain one. # The retry below drops the trailing comments field, so it stays last. - fields=url,number,state,isDraft,reviewDecision,latestReviews,headRefName,baseRefName,isCrossRepository,comments + fields=url,number,state,isDraft,reviewDecision,reviews,headRefName,baseRefName,isCrossRepository,comments else fields=url,state format=.url diff --git a/bin/git-pr b/bin/git-pr index 85c71ac7..c49c167e 100755 --- a/bin/git-pr +++ b/bin/git-pr @@ -1,9 +1,13 @@ #!/usr/bin/env bash # Prints the URL of the PR for the current branch, or for an explicit # / argument (passed straight to `gh pr view` unchanged). -# On a terminal the URL is followed by the PR's status: Draft, Ready, Approved, -# Changes requested, Merged, or Closed. An open PR that a Trunk merge queue has -# taken also shows its queue status, such as "Approved, Submitted to Trunk Queue". +# On a terminal the script prints the PR's status after the URL: Draft, Review +# required, Not approved, Approved, Changes requested, Merged, or Closed. +# Review required means the base branch requires an approval that the PR lacks. +# Not approved means the base branch requires no approval and the PR has no +# current approval. +# An open PR that a Trunk merge queue has taken also shows its queue status, +# such as "Approved, Submitted to Trunk Queue". # When other open PRs target this PR's branch, or this PR targets another open # PR's branch, the stack follows on separate lines, top-down: the PRs stacked on # this one, this PR marked with ">", the PRs it sits on, and the bottom base @@ -34,22 +38,30 @@ set -e # Both field sets include state for that lookup. if [ -t 1 ]; then # GitHub leaves reviewDecision empty when the base branch requires no reviews. - # latestReviews still records approvals and change requests in that case. + # The status then comes from each reviewer's last review that approved, + # requested changes, or was dismissed. latestReviews would hide an approval + # that the same reviewer followed with a comment. + # `gh pr list` returns only the oldest 100 reviews of a PR. An approval after + # those 100 does not count. # Trunk edits one status comment in place. The first line of that comment # names the queue state. Trunk posts submission errors as separate comments. # The status comment is therefore the last one whose first line has a known # phrase. The un-submitted control comment and the test analytics comment # match no phrase. - format='def status: + format='def review_status: + reduce (.reviews[] | select(.state != "COMMENTED" and .state != "PENDING")) as $r + ({}; .[$r.author.login // ""] = $r.state) + | if any(.[]; . == "CHANGES_REQUESTED") then "Changes requested" + elif any(.[]; . == "APPROVED") then "Approved" + else "Not approved" end; + def status: (if .state == "MERGED" then "Merged" elif .state == "CLOSED" then "Closed" elif .isDraft then "Draft" elif .reviewDecision == "APPROVED" then "Approved" elif .reviewDecision == "CHANGES_REQUESTED" then "Changes requested" - elif (.reviewDecision // "") != "" then "Ready" - elif any(.latestReviews[]; .state == "CHANGES_REQUESTED") then "Changes requested" - elif any(.latestReviews[]; .state == "APPROVED") then "Approved" - else "Ready" end) + elif .reviewDecision == "REVIEW_REQUIRED" then "Review required" + else review_status end) + ([select(.state == "OPEN") | .comments[]? | select(.author.login == "trunk-io") | .body | split("\n")[0] | ascii_downcase | if contains("removed from the merge queue") then "Removed from Trunk Queue" @@ -63,7 +75,7 @@ if [ -t 1 ]; then # Branch names hold no spaces, so only the status in a format line can # contain one. # The retry below drops the trailing comments field, so it stays last. - fields=url,number,state,isDraft,reviewDecision,latestReviews,headRefName,baseRefName,isCrossRepository,comments + fields=url,number,state,isDraft,reviewDecision,reviews,headRefName,baseRefName,isCrossRepository,comments else fields=url,state format=.url diff --git a/bin/lib/test-git-pr.sh b/bin/lib/test-git-pr.sh index 3782fdd6..e49ce76e 100755 --- a/bin/lib/test-git-pr.sh +++ b/bin/lib/test-git-pr.sh @@ -190,7 +190,7 @@ stack_pr() { # stack_pr [] [ ...] jq -n -c --argjson number "$1" --arg head "$2" --arg base "$3" --arg review "${4-APPROVED}" '{ url: "https://github.com/haacked/dotfiles/pull/\($number)", number: $number, state: "OPEN", isDraft: ($review == "draft"), - reviewDecision: (if $review == "draft" then "" else $review end), latestReviews: [], + reviewDecision: (if $review == "draft" then "" else $review end), reviews: [], headRefName: $head, baseRefName: $base, isCrossRepository: false, headRepositoryOwner: {login: "haacked"}, comments: [$ARGS.positional[] | {author: {login: "trunk-io"}, body: .}] @@ -356,6 +356,43 @@ run_git_pr --tty GH_LIST_JSON="$(list_json OPEN)" assert "a terminal exits 0" test "$RC" -eq 0 assert "a terminal shows the review status" test "$OUT" = "$URL9 (Approved)" +# ── Test: each review state maps to a status ──────────────────────────────── + +assert_review_status() { # assert_review_status [: ...] + local pr + pr=$(stack_pr 9 haacked/topic main "$3" | jq -c \ + '.reviews = [$ARGS.positional[] | split(":") | {author: {login: .[0]}, state: .[1]}]' --args "${@:4}") + run_git_pr --tty GH_LIST_JSON="[$pr]" + assert "$1" test "$OUT" = "$URL9 ($2)" +} + +assert_review_status "a PR that needs a review shows Review required" "Review required" REVIEW_REQUIRED +assert_review_status "a PR with changes requested shows Changes requested" "Changes requested" CHANGES_REQUESTED +assert_review_status "a draft PR shows Draft" Draft draft +assert_review_status "a PR without a decision or reviews shows Not approved" "Not approved" '' +assert_review_status "a PR without a decision and only comments shows Not approved" "Not approved" '' \ + bot:COMMENTED +assert_review_status "a PR without a decision and an approval shows Approved" Approved '' alice:APPROVED +assert_review_status "a PR without a decision and a change request shows Changes requested" \ + "Changes requested" '' alice:CHANGES_REQUESTED +assert_review_status "a PR without a decision shows a change request over an approval" \ + "Changes requested" '' alice:APPROVED bob:CHANGES_REQUESTED +# GitHub lists a PR's reviews oldest first. +assert_review_status "a comment after an approval keeps the approval" Approved '' \ + alice:APPROVED alice:COMMENTED +assert_review_status "a comment after a change request keeps the change request" "Changes requested" '' \ + alice:CHANGES_REQUESTED alice:COMMENTED +assert_review_status "an approval after a change request replaces it" Approved '' \ + alice:CHANGES_REQUESTED alice:APPROVED +assert_review_status "a pending review after an approval keeps the approval" Approved '' \ + alice:APPROVED alice:PENDING +assert_review_status "a dismissed review after an approval replaces it" "Not approved" '' \ + alice:APPROVED alice:DISMISSED + +run_git_pr --tty GH_LIST_JSON="[$(stack_pr 9 haacked/topic main REVIEW_REQUIRED "$TRUNK_SUBMITTED")]" +assert "a queued PR that needs a review shows both statuses" \ + test "$OUT" = "$URL9 (Review required, Submitted to Trunk Queue)" + # ── Test: Trunk comments that name no queue state add nothing ─────────────── run_git_pr --tty GH_LIST_JSON="$(list_json OPEN "$TRUNK_CONTROL" "$TRUNK_ANALYTICS")" @@ -454,7 +491,7 @@ assert_not "no stack lookup omits the PR's repo" \ run_stack "$(stack_pr 9 haacked/topic main)" \ "$(stack_pr 11 haacked/child haacked/topic REVIEW_REQUIRED)" assert "a PR with a child shows the child above it" test "$OUT" = "$URL9 (Approved) - #11 Ready + #11 Review required > #9 Approved main" @@ -467,7 +504,7 @@ run_stack "$(stack_pr 9 haacked/topic haacked/b)" \ "$(stack_pr 20 haacked/other main)" \ "$(stack_pr 8 haacked/b haacked/a CHANGES_REQUESTED)" assert "a mid-stack PR shows two levels each way" test "$OUT" = "$URL9 (Approved) - #11 Ready + #11 Review required #10 Draft > #9 Approved #8 Changes requested @@ -482,8 +519,8 @@ run_stack "$(stack_pr 12 haacked/topic main)" \ "$(stack_pr 14 haacked/fourteen haacked/topic draft)" \ "$(stack_pr 15 haacked/fifteen haacked/thirteen REVIEW_REQUIRED)" assert "a PR whose base PR is not the line below names its base" test "$OUT" = "$URL12 (Approved) - #15 Ready - #13 Ready (on #12) + #15 Review required + #13 Review required (on #12) #14 Draft > #12 Approved main" @@ -494,7 +531,7 @@ run_stack "$(stack_pr 9 haacked/topic haacked/parent)" \ "$(stack_pr 30 haacked/parent main | fork)" \ "$(stack_pr 11 haacked/child haacked/topic REVIEW_REQUIRED)" assert "a fork's same-named PR leaves the base branch at the bottom" test "$OUT" = "$URL9 (Approved) - #11 Ready + #11 Review required > #9 Approved haacked/parent" @@ -516,7 +553,7 @@ assert "a PR on a branch whose PR merged, with no children, shows no stack" \ run_stack "$(stack_pr 9 haacked/topic haacked/gone)" \ "$(stack_pr 11 haacked/child haacked/topic REVIEW_REQUIRED)" assert "a stack over a branch without an open PR ends at that branch" test "$OUT" = "$URL9 (Approved) - #11 Ready + #11 Review required > #9 Approved haacked/gone" @@ -540,7 +577,7 @@ run_stack "$(stack_pr 9 haacked/topic main)" \ "$(stack_pr 32 patch-1 haacked/topic REVIEW_REQUIRED | fork)" \ "$(stack_pr 33 haacked/x patch-1)" assert "a child from a fork is listed without its own children" test "$OUT" = "$URL9 (Approved) - #32 Ready + #32 Review required > #9 Approved main" assert_not "a child from a fork never has its children looked up" \ @@ -607,7 +644,7 @@ run_git_pr --tty GH_VIEW_JSON="$TWELVE" \ GH_STACK_JSON="$(prs "$TWELVE" "$PARENT" "$(stack_pr 13 haacked/thirteen haacked/twelve REVIEW_REQUIRED)")" \ -- 12 assert "an explicit PR shows its stack" test "$OUT" = "$URL12 (Approved) - #13 Ready + #13 Review required > #12 Approved #10 Approved main"