Skip to content

fix: harden sdk auto-review gate #7

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

Merged
mekilis merged 3 commits into from
Jul 20, 2026
+84 −24
Merged

fix: harden sdk auto-review gate #7

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bf6e472
fix: harden sdk auto-review gate
mekilis Jul 20, 2026
e2f54d0
fix: close validate-then-approve race in auto-review
mekilis Jul 20, 2026
664fcf5
fix: make review dismissal best-effort ahead of deny
mekilis Jul 20, 2026
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
108 changes: 84 additions & 24 deletions .github/workflows/sdk_generation.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,31 +150,73 @@ jobs:
GH_REPO: ${{ github.repository }}
BRANCH: ${{ steps.branch.outputs.name }}
run: |
# Failure policy: fail open to human review, never to merge. If the
# diff touches anything outside the generated paths the PR is left
# unapproved with the offending paths printed. Auto-merge completes
# only after the required status checks pass.
pr=$(GH_TOKEN="$APP_TOKEN" gh pr list --head "$BRANCH" --state open --json number --jq '.[0].number // empty')
if [ -z "$pr" ]; then
# Failure policy: fail open to human review, never to merge. Any
# gate miss below (wrong base, stale head, non-generated paths)
# leaves the PR unapproved for a human. Clearing a stale auto-merge
# is the one hard failure: if --disable-auto errors, this step goes
# red instead of leaving a rejected head silently mergeable.
set -euo pipefail

deny() {
# Auto-merge is cleared before commenting: under set -e a comment
# failure must not skip the disable, and a disable failure must
# fail the step, not be swallowed.
enabled=$(GH_TOKEN="$APP_TOKEN" gh pr view "$pr" --json autoMergeRequest --jq '.autoMergeRequest != null')
if [ "$enabled" = "true" ]; then
GH_TOKEN="$MERGE_TOKEN" gh pr merge "$pr" --disable-auto
fi
printf '%s\n' "$1" | GH_TOKEN="$APP_TOKEN" gh pr comment "$pr" --body-file - || true
exit 0
}

pr_json=$(GH_TOKEN="$APP_TOKEN" gh pr list --head "$BRANCH" --state open \
--json number,baseRefName,headRefOid --jq '.[0] // empty')
if [ -z "$pr_json" ]; then
echo "No open PR for $BRANCH; nothing to approve."
exit 0
fi
pr=$(echo "$pr_json" | jq -r .number)
base=$(echo "$pr_json" | jq -r .baseRefName)
head_sha=$(echo "$pr_json" | jq -r .headRefOid)

if [ "$base" != "main" ]; then
echo "Not approving PR #$pr: base is $base, not main."
deny "SDK reviewer app: not auto-approving; the PR base is \`$base\`, not \`main\`. Left for human review (policy: fail open to human review, never to merge)."
fi

# Only regen PRs authored by the bot app qualify for auto-review.
# REST is used because it returns the stable "convoy-sdk-bot[bot]"
# login for app-authored PRs.
author=$(GH_TOKEN="$APP_TOKEN" gh api "repos/$GH_REPO/pulls/$pr" --jq '.user.login')
if [ "$author" != "convoy-sdk-bot[bot]" ]; then
echo "Not approving PR #$pr: author is $author, not convoy-sdk-bot[bot]."
GH_TOKEN="$APP_TOKEN" gh pr comment "$pr" --body "SDK reviewer app: not auto-approving; PR author is not convoy-sdk-bot[bot]. Left for human review (policy: fail open to human review, never to merge)."
exit 0
deny "SDK reviewer app: not auto-approving; PR author is not convoy-sdk-bot[bot]. Left for human review (policy: fail open to human review, never to merge)."
fi

# Bind the approval to the commit this run pushed; a concurrent
# push to the regen branch means the diff is no longer this run's
# generated output.
pushed_sha=$(git rev-parse HEAD)
if [ "$head_sha" != "$pushed_sha" ]; then
echo "Not approving PR #$pr: head $head_sha is not the commit this run pushed."
deny "SDK reviewer app: not auto-approving; the PR head is not the commit this generation run pushed. Left for human review (policy: fail open to human review, never to merge)."
fi

# This run committed a real diff, so an empty changed-files listing
# is an API anomaly, not a clean PR; it must not count as an
# allowlist pass.
files=$(GH_TOKEN="$APP_TOKEN" gh api "repos/$GH_REPO/pulls/$pr/files" --paginate \
--jq '.[] | .filename, (.previous_filename // empty)')
if [ -z "$files" ]; then
echo "Not approving PR #$pr: changed-files listing came back empty."
deny "SDK reviewer app: not auto-approving; the changed-files listing came back empty for a non-empty regen commit. Left for human review (policy: fail open to human review, never to merge)."
fi

# The head branch is the regen branch this run pushed, so the
# branch condition holds by construction of the lookup above.
# Allowlist mirrors scripts/generate.sh: only the api, client, and
# models packages are generated; webhook/ is hand-written.
bad=$(GH_TOKEN="$APP_TOKEN" gh api "repos/$GH_REPO/pulls/$pr/files" --paginate --jq '.[].filename' \
# models packages are generated; webhook/ is hand-written. Renames
# are checked on both sides so a file cannot be moved into the
# generated tree from outside it.
bad=$(printf '%s\n' "$files" \
| while read -r f; do
case "$f" in
src/main/java/com/getconvoy/api/*) ;;
Expand All @@ -186,22 +228,40 @@ jobs:
if [ -n "$bad" ]; then
echo "Not approving PR #$pr: diff touches non-generated paths:"
echo "$bad"
{
echo "SDK reviewer app: not auto-approving; the diff touches paths outside the generated allowlist:"
echo
echo '```'
echo "$bad"
echo '```'
echo
echo "Left for human review (policy: fail open to human review, never to merge)."
} | GH_TOKEN="$APP_TOKEN" gh pr comment "$pr" --body-file -
exit 0
deny "$(printf '%s\n' \
"SDK reviewer app: not auto-approving; the diff touches paths outside the generated allowlist:" \
"" '```' "$bad" '```' "" \
"Left for human review (policy: fail open to human review, never to merge).")"
fi

GH_TOKEN="$APP_TOKEN" gh api -X POST "repos/$GH_REPO/pulls/$pr/reviews" \
review_id=$(GH_TOKEN="$APP_TOKEN" gh api -X POST "repos/$GH_REPO/pulls/$pr/reviews" \
-f event=APPROVE \
-f body="SDK reviewer app: approving a generated-paths-only diff on the regen branch. Auto-merge completes only after required status checks pass."
-f commit_id="$head_sha" \
-f body="SDK reviewer app: approving a generated-paths-only diff on the regen branch. Auto-merge completes only after required status checks pass." \
--jq '.id')

# Close the validate-then-approve race: a push landing between the
# allowlist check and the approval is not covered by
# dismiss_stale_reviews (that only dismisses on pushes after the
# review), so re-read the head and dismiss our own approval if it
# moved off the validated commit.
now_sha=$(GH_TOKEN="$APP_TOKEN" gh pr view "$pr" --json headRefOid --jq '.headRefOid')
if [ "$now_sha" != "$head_sha" ]; then
echo "Head moved from $head_sha to $now_sha during approval; dismissing review."
# Best-effort: dismiss_stale_reviews normally dismissed this
# approval already when the head moved. The hard gate is deny
# below, which clears auto-merge or fails the step; a dismiss
# error must not skip it.
GH_TOKEN="$APP_TOKEN" gh api -X PUT "repos/$GH_REPO/pulls/$pr/reviews/$review_id/dismissals" \
-f message="Head moved during approval; the approved commit is no longer the PR head." \
-f event=DISMISS || true
deny "SDK reviewer app: approval dismissed; the PR head changed while the review was being submitted. Left for human review (policy: fail open to human review, never to merge)."
Comment thread
mekilis marked this conversation as resolved.
fi

# Post-validation head drift is closed by branch protection, not
# here: the approval is pinned to the validated commit_id and
# dismiss_stale_reviews dismisses it on any later push, so
# auto-merge fail-closes back to human review.
enabled=$(GH_TOKEN="$APP_TOKEN" gh pr view "$pr" --json autoMergeRequest --jq '.autoMergeRequest != null')
if [ "$enabled" = "true" ]; then
echo "Auto-merge already enabled on PR #$pr."
Expand Down
Loading