diff --git a/.github/workflows/codex-branch.sh b/.github/workflows/codex-branch.sh index 4dfb0c4fee429b..12743516da525c 100755 --- a/.github/workflows/codex-branch.sh +++ b/.github/workflows/codex-branch.sh @@ -1066,7 +1066,7 @@ authenticate_pending_codex_merge () ( die "could not authenticate the merged Codex pull request" fi has_qualifying_current_review "$state/reviews" "$author" "$head" \ - openai/git "$state/review-candidates" || + openai/git "$number" "$state/review-candidates" || die "pending Codex pull request has no qualifying approval" printf '%s\t%s\t%s\t%s\n' "$name" "$head" "$number" "$merge" \ >"$output" || die "could not record the reviewed Codex admission" @@ -5903,7 +5903,8 @@ has_qualifying_current_review () { author=$2 head=$3 repository=$4 - candidates=$5 + pull_number=$5 + candidates=$6 awk -F '\t' -v author="$author" -v head="$head" ' NF == 4 && $2 ~ /^(APPROVED|CHANGES_REQUESTED|DISMISSED)$/ { state[$1] = $2 @@ -5923,17 +5924,24 @@ has_qualifying_current_review () { case "$association" in OWNER|MEMBER|COLLABORATOR) return 0 ;; esac - # author_association is relative to the API caller. The - # Actions token can see an organization member as NONE, so - # fall back to the repository-scoped collaborator check. - if gh api --hostname github.com \ - "repos/$repository/collaborators/$reviewer" \ - --silent >/dev/null 2>&1 - then - return 0 - fi done <"$candidates" - return 1 + # author_association is relative to the API caller. The Actions + # token can see an organization member as NONE, so ask GitHub for + # the latest reviews from writers instead of trusting that field. + owner=${repository%%/*} + name=${repository#*/} + gh api --hostname github.com graphql \ + -f 'query=query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){latestOpinionatedReviews(first:100,writersOnly:true){nodes{author{login}state commit{oid}}}}}}' \ + -F owner="$owner" -F name="$name" -F number="$pull_number" \ + --jq '.data.repository.pullRequest.latestOpinionatedReviews.nodes[] | + [(.author.login // "-"), .state, + (.commit.oid // "-")] | @tsv' \ + >"$candidates-writers" 2>/dev/null || return 1 + awk -F '\t' -v author="$author" -v head="$head" ' + NF == 3 && $1 != author && $2 == "APPROVED" && + $3 == head { approved++ } + END { exit !approved } + ' "$candidates-writers" } validate_topic_review () { @@ -6004,7 +6012,8 @@ validate_topic_review () { .author_association] | @tsv' >"$tmp_dir/topic-reviews" || die "could not inspect reviews for topic pull request #$pull_number" has_qualifying_current_review "$tmp_dir/topic-reviews" "$author" \ - "$source_tip" "$repository" "$tmp_dir/topic-review-candidates" || + "$source_tip" "$repository" "$pull_number" \ + "$tmp_dir/topic-review-candidates" || die "topic pull request #$pull_number has no current approval for $source_tip" say "validated reviewed topic pull request #$pull_number at $source_tip" } diff --git a/t/t9905-codex-branch.sh b/t/t9905-codex-branch.sh index 6a151d0e211b41..b9f9cb20c553c0 100755 --- a/t/t9905-codex-branch.sh +++ b/t/t9905-codex-branch.sh @@ -8940,9 +8940,13 @@ test_expect_success 'topic review requires the exact approved head' ' cat >topic-review-bin/gh <<-\EOF && #!/bin/sh case " $* " in - *"repos/openai/git/collaborators/reviewer "*) - test "$FAKE_COLLABORATOR" = yes - exit + *" graphql "*) + if test "$FAKE_WRITER" = yes + then + printf "%s\t%s\t%s\n" reviewer APPROVED \ + "$FAKE_WRITER_HEAD" + fi + exit 0 ;; *"pulls/42/reviews?"*) printf "%s\t%s\t%s\t%s\n" reviewer APPROVED \ @@ -8993,20 +8997,21 @@ test_expect_success 'topic review requires the exact approved head' ' test_grep "has no current approval for $source" stale.err && env PATH="$PWD/topic-review-bin:$PATH" \ FAKE_HEAD="$source" FAKE_REVIEW_HEAD="$source" \ - FAKE_ASSOCIATION=NONE FAKE_COLLABORATOR=yes \ + FAKE_ASSOCIATION=NONE FAKE_WRITER=yes \ + FAKE_WRITER_HEAD="$source" \ FAKE_DECISION=APPROVED sh "$codex_branch" \ validate-topic-review --pull-request 42 --lane codex \ --topic aa/codex/reviewed --source-tip "$source" \ - >collaborator.out && - test_grep "validated reviewed topic" collaborator.out && + >writer.out && + test_grep "validated reviewed topic" writer.out && test_expect_code 1 env PATH="$PWD/topic-review-bin:$PATH" \ FAKE_HEAD="$source" FAKE_REVIEW_HEAD="$source" \ - FAKE_ASSOCIATION=NONE FAKE_COLLABORATOR=no \ + FAKE_ASSOCIATION=NONE FAKE_WRITER=no \ FAKE_DECISION=APPROVED sh "$codex_branch" \ validate-topic-review --pull-request 42 --lane codex \ --topic aa/codex/reviewed --source-tip "$source" \ - >outsider.out 2>outsider.err && - test_grep "has no current approval for $source" outsider.err + >nonwriter.out 2>nonwriter.err && + test_grep "has no current approval for $source" nonwriter.err ' test_expect_success 'checked-in release recovery manifest is the bound incident' '