Skip to content

codex: verify review collaborators independently #31

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ttaylorr-oai wants to merge 1 commit into from
Closed

codex: verify review collaborators independently #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 42 additions & 32 deletions .github/workflows/codex-branch.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1065,22 +1065,8 @@ authenticate_pending_codex_merge () (
then
die "could not authenticate the merged Codex pull request"
fi
awk -F '\t' -v author="$author" -v head="$head" '
NF == 4 && $2 ~ /^(APPROVED|CHANGES_REQUESTED|DISMISSED)$/ {
state[$1] = $2
commit[$1] = $3
association[$1] = $4
}
END {
for (reviewer in state)
if (reviewer != author &&
state[reviewer] == "APPROVED" &&
commit[reviewer] == head &&
association[reviewer] ~ /^(OWNER|MEMBER|COLLABORATOR)$/)
approved++
exit !approved
}
' "$state/reviews" ||
has_qualifying_current_review "$state/reviews" "$author" "$head" \
openai/git "$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"
Expand Down Expand Up @@ -5912,6 +5898,44 @@ plan_trailer_optional () (
printf '%s\n' "$values" | sed -n '1p'
)

has_qualifying_current_review () {
reviews=$1
author=$2
head=$3
repository=$4
candidates=$5
awk -F '\t' -v author="$author" -v head="$head" '
NF == 4 && $2 ~ /^(APPROVED|CHANGES_REQUESTED|DISMISSED)$/ {
state[$1] = $2
commit[$1] = $3
association[$1] = $4
}
END {
for (reviewer in state)
if (reviewer != author &&
state[reviewer] == "APPROVED" &&
commit[reviewer] == head)
print reviewer "\t" association[reviewer]
}
' "$reviews" >"$candidates" || return 1
while IFS="$tab" read -r reviewer association
do
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
}

validate_topic_review () {
repository=openai/git
pull_number=
Expand Down Expand Up @@ -5979,22 +6003,8 @@ validate_topic_review () {
--jq '.[] | [.user.login, .state, (.commit_id // "-"),
.author_association] | @tsv' >"$tmp_dir/topic-reviews" ||
die "could not inspect reviews for topic pull request #$pull_number"
awk -F '\t' -v author="$author" -v head="$source_tip" '
NF == 4 && $2 ~ /^(APPROVED|CHANGES_REQUESTED|DISMISSED)$/ {
state[$1] = $2
commit[$1] = $3
association[$1] = $4
}
END {
for (reviewer in state)
if (reviewer != author &&
state[reviewer] == "APPROVED" &&
commit[reviewer] == head &&
association[reviewer] ~ /^(OWNER|MEMBER|COLLABORATOR)$/)
approved++
exit !approved
}
' "$tmp_dir/topic-reviews" ||
has_qualifying_current_review "$tmp_dir/topic-reviews" "$author" \
"$source_tip" "$repository" "$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"
}
Expand Down
24 changes: 22 additions & 2 deletions t/t9905-codex-branch.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
;;
*"pulls/42/reviews?"*)
printf "%s\t%s\t%s\t%s\n" reviewer APPROVED \
"$FAKE_REVIEW_HEAD" MEMBER
"$FAKE_REVIEW_HEAD" "${FAKE_ASSOCIATION:-MEMBER}"
exit 0
;;
*"pulls/42 "*)
Expand Down Expand Up @@ -8986,7 +8990,23 @@ test_expect_success 'topic review requires the exact approved head' '
validate-topic-review --pull-request 42 --lane codex \
--topic aa/codex/reviewed --source-tip "$source" \
>stale.out 2>stale.err &&
test_grep "has no current approval for $source" stale.err
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_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 &&
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_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
'

test_expect_success 'checked-in release recovery manifest is the bound incident' '
Expand Down