From 8ca003801cbccef33cb223e0d72d94f4434a14f3 Mon Sep 17 00:00:00 2001 From: Sahil Aujla Date: Tue, 26 May 2026 14:37:29 -0400 Subject: [PATCH 1/2] Update docs agent self-approval guard Run the guard on PR updates and count historical non-requester approvals so pushes after review do not leave the required check stuck. Co-authored-by: Cursor --- .../workflows/no-originator-self-approval.yml | 110 +++++++++++------- 1 file changed, 68 insertions(+), 42 deletions(-) diff --git a/.github/workflows/no-originator-self-approval.yml b/.github/workflows/no-originator-self-approval.yml index 6f4fe296c..e9fade90b 100644 --- a/.github/workflows/no-originator-self-approval.yml +++ b/.github/workflows/no-originator-self-approval.yml @@ -2,9 +2,9 @@ name: Docs agent review guard # When the docs-agent (alchemy-bot) opens a PR on behalf of someone who # requested the change via Slack, it includes a "Requested-by: @" -# trailer in the commit message. This workflow watches for approvals on those -# PRs and dismisses any approval submitted by that user, so a docs-agent PR -# always requires a non-originator review before merging. +# trailer in the commit message. This workflow watches for approval submissions +# and PR updates on those PRs, dismisses any approval submitted by that user, +# and requires at least one non-originator approval in the PR review history. # # Note on shared bot identity: alchemy-bot is also used by other automation # (Daikon spec updates, weekly changelog, etc.). To avoid this workflow @@ -16,8 +16,8 @@ name: Docs agent review guard # # Branch protection on main needs both "Require 1 approval" and this workflow # as a required check. The review requirement supplies an approval, and this -# check ensures that approval did not come from the person who originated the -# docs-agent request. +# check ensures that the PR has been approved at least once by someone who did +# not originate the docs-agent request. # # Why commit message and not PR body: the PR body is editable by anyone with # write access (including the originator), who could remove their own @mention @@ -33,6 +33,8 @@ name: Docs agent review guard # the check can be required for every PR. on: + pull_request: + types: [opened, reopened, synchronize, ready_for_review] pull_request_review: types: [submitted] @@ -52,20 +54,24 @@ jobs: sparse-checkout: | .github/workflows/docs-agent-pubkey.asc sparse-checkout-cone-mode: false - - name: Compare approver to originator and dismiss if same + - name: Evaluate originator approval guard env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - APPROVER: ${{ github.event.review.user.login }} - REVIEW_STATE: ${{ github.event.review.state }} - REVIEW_ID: ${{ github.event.review.id }} + APPROVER: ${{ github.event.review.user.login || '' }} + REVIEW_STATE: ${{ github.event.review.state || '' }} + REVIEW_ID: ${{ github.event.review.id || '' }} PR_NUMBER: ${{ github.event.pull_request.number }} PR_AUTHOR: ${{ github.event.pull_request.user.login }} BASE_SHA: ${{ github.event.pull_request.base.sha }} - REVIEW_COMMIT_SHA: ${{ github.event.review.commit_id }} + TARGET_COMMIT_SHA: ${{ github.event.review.commit_id || github.event.pull_request.head.sha }} run: | set -euo pipefail REVIEW_STATE_LOWER="$(printf '%s' "$REVIEW_STATE" | tr '[:upper:]' '[:lower:]')" + IS_APPROVAL_REVIEW=false + if [ "$GITHUB_EVENT_NAME" = "pull_request_review" ] && [ "$REVIEW_STATE_LOWER" = "approved" ]; then + IS_APPROVAL_REVIEW=true + fi if [ "$PR_AUTHOR" != "alchemy-bot" ]; then echo "PR author is $PR_AUTHOR, not alchemy-bot. No docs-agent enforcement needed." @@ -97,8 +103,12 @@ jobs: # all fail-closed paths AND the originator-match dismissal so a # transient API hiccup doesn't leave the approval intact when our # logic says it should be dismissed. - dismiss_with_retry() { + dismiss_current_review_with_retry() { local message="$1" + if [ -z "${REVIEW_ID:-}" ]; then + echo "No submitted review to dismiss for $GITHUB_EVENT_NAME." + return 0 + fi for attempt in 1 2 3; do if gh api -X PUT "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews/$REVIEW_ID/dismissals" \ -f message="$message" \ @@ -131,7 +141,9 @@ jobs: # exactly what this rule is enforcing), fail closed. if [ ! -f "$PUBKEY_FILE" ]; then echo "ERROR: pubkey file $PUBKEY_FILE missing from checkout" >&2 - dismiss_with_retry ":warning: Originator self-approval check could not initialize (pinned pubkey file missing from repo). Approval dismissed by default per fail-closed policy." || true + if [ "$IS_APPROVAL_REVIEW" = true ]; then + dismiss_current_review_with_retry ":warning: Originator self-approval check could not initialize (pinned pubkey file missing from repo). Approval dismissed by default per fail-closed policy." || true + fi exit 1 fi @@ -139,7 +151,9 @@ jobs: if ! gpg --batch --quiet --import "$PUBKEY_FILE" 2>/tmp/gpg-import-err; then echo "ERROR: gpg --import failed:" >&2 cat /tmp/gpg-import-err >&2 || true - dismiss_with_retry ":warning: Originator self-approval check could not initialize (pubkey import failed). Approval dismissed by default per fail-closed policy." || true + if [ "$IS_APPROVAL_REVIEW" = true ]; then + dismiss_current_review_with_retry ":warning: Originator self-approval check could not initialize (pubkey import failed). Approval dismissed by default per fail-closed policy." || true + fi exit 1 fi @@ -147,13 +161,15 @@ jobs: # without keys would otherwise make every commit look untrusted. if ! gpg --list-keys "$EXPECTED_FPR" >/dev/null 2>&1; then echo "ERROR: pinned pubkey $EXPECTED_FPR not present in keyring after import" >&2 - dismiss_with_retry ":warning: Originator self-approval check could not initialize (pinned pubkey missing from imported keyring). Approval dismissed by default per fail-closed policy." || true + if [ "$IS_APPROVAL_REVIEW" = true ]; then + dismiss_current_review_with_retry ":warning: Originator self-approval check could not initialize (pinned pubkey missing from imported keyring). Approval dismissed by default per fail-closed policy." || true + fi exit 1 fi # Collect ALL Requested-by trailers from commits in this PR that # are BOTH: - # 1. In the exact commit range approved by this review, AND + # 1. In the exact commit range being evaluated, AND # 2. Cryptographically signed by the pinned agent GPG key # (verified locally in the workflow, not just trusting # GitHub's flag) @@ -188,32 +204,41 @@ jobs: # Fetch and inspect commits locally instead of using the REST pull # request commits endpoint. That endpoint is capped at 250 commits, # which is not acceptable for an enforcement decision. Local git - # history also lets us evaluate the exact commit SHA that the - # approval was submitted against, avoiding live-PR drift. - if [ -z "${BASE_SHA:-}" ] || [ -z "${REVIEW_COMMIT_SHA:-}" ]; then - echo "ERROR: missing base or reviewed commit SHA from event payload" >&2 - dismiss_with_retry ":warning: Originator self-approval check could not run (missing review commit metadata). Approval dismissed by default per fail-closed policy; please re-approve once the workflow check passes." || true + # history also lets us evaluate the exact commit SHA under review: + # the reviewed commit for approval events, or the current PR head + # for PR update events. + if [ -z "${BASE_SHA:-}" ] || [ -z "${TARGET_COMMIT_SHA:-}" ]; then + echo "ERROR: missing base or target commit SHA from event payload" >&2 + if [ "$IS_APPROVAL_REVIEW" = true ]; then + dismiss_current_review_with_retry ":warning: Originator self-approval check could not run (missing review commit metadata). Approval dismissed by default per fail-closed policy; please re-approve once the workflow check passes." || true + fi exit 1 fi - if ! git fetch --no-tags --filter=blob:none origin "$REVIEW_COMMIT_SHA" 2>/tmp/git-fetch-err; then - echo "ERROR: failed to fetch reviewed commit $REVIEW_COMMIT_SHA:" >&2 + if ! git fetch --no-tags --filter=blob:none origin "$TARGET_COMMIT_SHA" 2>/tmp/git-fetch-err; then + echo "ERROR: failed to fetch target commit $TARGET_COMMIT_SHA:" >&2 cat /tmp/git-fetch-err >&2 || true - dismiss_with_retry ":warning: Originator self-approval check could not fetch the reviewed commit. Approval dismissed by default per fail-closed policy; please re-approve once the workflow check passes." || true + if [ "$IS_APPROVAL_REVIEW" = true ]; then + dismiss_current_review_with_retry ":warning: Originator self-approval check could not fetch the reviewed commit. Approval dismissed by default per fail-closed policy; please re-approve once the workflow check passes." || true + fi exit 1 fi - if ! git cat-file -e "$REVIEW_COMMIT_SHA^{commit}" 2>/dev/null; then - echo "ERROR: reviewed commit $REVIEW_COMMIT_SHA is not available after fetch" >&2 - dismiss_with_retry ":warning: Originator self-approval check could not inspect the reviewed commit. Approval dismissed by default per fail-closed policy; please re-approve once the workflow check passes." || true + if ! git cat-file -e "$TARGET_COMMIT_SHA^{commit}" 2>/dev/null; then + echo "ERROR: target commit $TARGET_COMMIT_SHA is not available after fetch" >&2 + if [ "$IS_APPROVAL_REVIEW" = true ]; then + dismiss_current_review_with_retry ":warning: Originator self-approval check could not inspect the reviewed commit. Approval dismissed by default per fail-closed policy; please re-approve once the workflow check passes." || true + fi exit 1 fi COMMITS_FILE="$(mktemp)" - if ! git rev-list --reverse "$BASE_SHA..$REVIEW_COMMIT_SHA" > "$COMMITS_FILE" 2>/tmp/rev-list-err; then - echo "ERROR: failed to enumerate PR commits from $BASE_SHA to $REVIEW_COMMIT_SHA:" >&2 + if ! git rev-list --reverse "$BASE_SHA..$TARGET_COMMIT_SHA" > "$COMMITS_FILE" 2>/tmp/rev-list-err; then + echo "ERROR: failed to enumerate PR commits from $BASE_SHA to $TARGET_COMMIT_SHA:" >&2 cat /tmp/rev-list-err >&2 || true - dismiss_with_retry ":warning: Originator self-approval check could not enumerate PR commits. Approval dismissed by default per fail-closed policy; please re-approve once the workflow check passes." || true + if [ "$IS_APPROVAL_REVIEW" = true ]; then + dismiss_current_review_with_retry ":warning: Originator self-approval check could not enumerate PR commits. Approval dismissed by default per fail-closed policy; please re-approve once the workflow check passes." || true + fi exit 1 fi @@ -284,39 +309,40 @@ jobs: if ! REVIEWS_JSON="$(fetch_reviews)"; then echo "ERROR: could not fetch PR reviews after 3 retries." >&2 - if [ "$REVIEW_STATE_LOWER" = "approved" ] && printf '%s\n' "$ALL_REQUESTED_BY" | grep -qFx "$APPROVER_LOWER"; then - dismiss_with_retry ":warning: Originator self-approval check could not fetch current PR reviews. Approval dismissed by default because you are listed as the docs request originator." || true + if [ "$IS_APPROVAL_REVIEW" = true ] && printf '%s\n' "$ALL_REQUESTED_BY" | grep -qFx "$APPROVER_LOWER"; then + dismiss_current_review_with_retry ":warning: Originator self-approval check could not fetch current PR reviews. Approval dismissed by default because you are listed as the docs request originator." || true fi exit 1 fi - ACTIVE_APPROVERS="$( + HISTORICAL_APPROVERS="$( printf '%s' "$REVIEWS_JSON" \ - | jq -r 'map(select((.state | ascii_upcase) != "COMMENTED")) | sort_by(.submitted_at // "") | reduce .[] as $review ({}; .[$review.user.login | ascii_downcase] = ($review.state | ascii_upcase)) | to_entries[] | select(.value == "APPROVED") | .key' + | jq -r 'map(select((.state | ascii_upcase) == "APPROVED")) | .[] | .user.login | ascii_downcase' \ + | sort -u )" NON_REQUESTER_APPROVAL_COUNT=0 - while IFS= read -r active_approver; do - [ -n "$active_approver" ] || continue - if ! printf '%s\n' "$ALL_REQUESTED_BY" | grep -qFx "$active_approver"; then + while IFS= read -r historical_approver; do + [ -n "$historical_approver" ] || continue + if ! printf '%s\n' "$ALL_REQUESTED_BY" | grep -qFx "$historical_approver"; then NON_REQUESTER_APPROVAL_COUNT=$((NON_REQUESTER_APPROVAL_COUNT + 1)) fi - done <<< "$ACTIVE_APPROVERS" + done <<< "$HISTORICAL_APPROVERS" - echo "Approver=$APPROVER_LOWER, AllRequestedBy=$(printf '%s' "$ALL_REQUESTED_BY" | tr '\n' ',' | sed 's/,$//'), NonRequesterApprovals=$NON_REQUESTER_APPROVAL_COUNT" + echo "Approver=$APPROVER_LOWER, AllRequestedBy=$(printf '%s' "$ALL_REQUESTED_BY" | tr '\n' ',' | sed 's/,$//'), NonRequesterApprovalHistory=$NON_REQUESTER_APPROVAL_COUNT" - if [ "$REVIEW_STATE_LOWER" = "approved" ] && printf '%s\n' "$ALL_REQUESTED_BY" | grep -qFx "$APPROVER_LOWER"; then + if [ "$IS_APPROVAL_REVIEW" = true ] && printf '%s\n' "$ALL_REQUESTED_BY" | grep -qFx "$APPROVER_LOWER"; then echo "Approver matches a Requested-by trailer. Dismissing approval $REVIEW_ID." - if ! dismiss_with_retry "@$APPROVER you are listed as the originator of this docs request (via the Requested-by trailer on a docs-agent commit). Per the docs-agent self-review policy, the originator can't approve their own request. Please ask another team member to review."; then + if ! dismiss_current_review_with_retry "@$APPROVER you are listed as the originator of this docs request (via the Requested-by trailer on a docs-agent commit). Per the docs-agent self-review policy, the originator can't approve their own request. Please ask another team member to review."; then echo "ERROR: dismissal of originator approval failed after 3 retries, exiting 1 to surface the failure" >&2 exit 1 fi fi if [ "$NON_REQUESTER_APPROVAL_COUNT" -gt 0 ]; then - echo "$NON_REQUESTER_APPROVAL_COUNT non-requester approval(s) active. Check passes." + echo "$NON_REQUESTER_APPROVAL_COUNT non-requester approval(s) found in review history. Check passes." exit 0 fi - echo "No active non-requester approvals. Failing required check." + echo "No non-requester approvals found in review history. Failing required check." exit 1 From baa47c5503574ee4944af811b10f21dff2020420 Mon Sep 17 00:00:00 2001 From: Sahil Aujla Date: Tue, 26 May 2026 16:53:34 -0400 Subject: [PATCH 2/2] Remove ready-for-review guard trigger Keep the self-approval guard focused on PR creation and pushes while review submissions continue to handle approval dismissal. Co-authored-by: Cursor --- .github/workflows/no-originator-self-approval.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/no-originator-self-approval.yml b/.github/workflows/no-originator-self-approval.yml index e9fade90b..6034475ee 100644 --- a/.github/workflows/no-originator-self-approval.yml +++ b/.github/workflows/no-originator-self-approval.yml @@ -34,7 +34,7 @@ name: Docs agent review guard on: pull_request: - types: [opened, reopened, synchronize, ready_for_review] + types: [opened, reopened, synchronize] pull_request_review: types: [submitted]