Skip to content

feat(auto-approve-bot-prs): keep prs with a skip label for a person - #331

Open
vcauesantos wants to merge 2 commits into
mainfrom
devops-1648/auto-approve-skip-labels
Open

vcauesantos wants to merge 2 commits into
mainfrom
devops-1648/auto-approve-skip-labels

Conversation

@vcauesantos

@vcauesantos vcauesantos commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Parked until the script-only rollout (#329, #332) has run on real backports. Then this goes ahead with the AI pass.

Summary

  • Adds an optional skip-labels input to the shared auto-approve action and workflow. A PR carrying any of those labels is never approved. The run logs a notice and leaves the PR for a person.
  • The check is in check-pr-after-ci.sh, the last step before approving. It reads the labels from the PR read that step already makes, so there's no new API call or step.
  • If skip-labels is set and the labels can't be read, the PR isn't approved.
  • With skip-labels empty, nothing changes. This matters because the shared workflow checks out the action at main, so it reaches every caller when it merges.

Why

#330 labels backports backport-ai-resolved when the AI pass writes a commit. loft-enterprise already auto-approves green backport/* PRs from loft-bot, so without this an AI-written backport would be approved with no person looking at it. The loft-enterprise caller passes skip-labels: backport-ai-resolved in the rollout PR.

There's no race. #330 adds the label before pushing the AI commit, so the run for that push always sees it. Runs for older commits already stop at the existing "head changed" check.

Test plan

  • 9 new bats tests in check-pr-after-ci.bats: a skip label blocks approval; any label from a list with spaces blocks it; other labels don't; without skip-labels nothing changes; a moved head is still reported first; the label is decided on the first read while mergeability is pending; it still blocks when mergeability never resolves; matching ignores case; labels with quotes or backslashes don't break the read.
  • The label filter also ran through the real gh CLI against a real PR: it matched regardless of case and with spaces in the list, and found nothing for an empty list.
  • actionlint, zizmor, shellcheck -x and make check-docs are clean.
  • Locally on macOS, 16 tests in other auto-approve suites fail, and the same 16 fail on an untouched main. I couldn't run wait-for-ci.bats because it needs GNU timeout. CI runs all suites on Ubuntu.

Related to DEVOPS-1648

@vcauesantos
vcauesantos marked this pull request as ready for review October 23, 2026 19:29
@vcauesantos
vcauesantos requested a review from a team as a code owner October 23, 2026 19:29

@loft-bot loft-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Panel review: 1 blocking, 2 quality notes, 7 lanes.

Blocking concerns

  • .github/actions/auto-approve-bot-prs/test/check-pr-after-ci.bats:132: the pending-mergeability test stays green if the label check moves after mergeability resolves.

What was checked: Checked: correctness, security, test-quality, operability, infra, typos, PR metadata. Skipped: architecture, reuse, dead code, e2e, coverage-gap analysis, entry point, layout (no structural moves, new helpers, deletions, e2e files or qualifying linked issue).

Quality notes (non-blocking)

  • consider: .github/actions/auto-approve-bot-prs/src/check-pr-after-ci.sh:64. Could we compare labels case-insensitively (ascii_downcase on both sides)? GitHub treats label names case-insensitively, but here skip-labels: Backport-AI-Resolved against a backport-ai-resolved label returns proceed=true (reproduced). This becomes blocking if a caller's configured casing differs from the label's.
  • consider: .github/actions/auto-approve-bot-prs/src/check-pr-after-ci.sh:43. Could we keep the label list out of the @tsv row, for example by running the match inside the same --jq filter with $ENV.SKIP_LABELS? @tsv escapes the backslashes that tojson adds a second time. So a PR carrying any label with " or \ in its name is refused with "could not read the labels", even when none of its labels is a skip label (reproduced). This becomes blocking if a caller repo puts such a label on bot PRs.

printf 'null\ntrue\n' > "$BATS_TEST_TMPDIR/pulls"
GH_MOCK_PULL_SEQ="$BATS_TEST_TMPDIR/pulls" GH_MOCK_LABELS='["backport-ai-resolved"]' SKIP_LABELS="backport-ai-resolved" run "$SCRIPT"
[ "$status" -eq 0 ]
[ "$(kv proceed)" = "proceed=false" ]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking — Could we make this test prove the label is checked on the first read, while mergeability is still pending? As written it also passes if the label check only runs once mergeability resolves. With that change, a PR whose mergeability never resolves gets approved even though it carries the skip label. Pinning the call count to one catches it:

Suggested change
[ "$(kv proceed)" = "proceed=false" ]
[ "$(kv proceed)" = "proceed=false" ]
[ "$(grep -c 'pulls/42' "$GH_MOCK_CALLS")" -eq 1 ]
Mutation run (bats 1.11, this branch)

I changed the guard in check-pr-after-ci.sh to if [ -n "${SKIP_LABELS:-}" ] && [ "$mergeable" != "null" ]; then:

  • this test: still ok
  • the same test plus the call-count line above: not ok
  • GH_MOCK_MERGEABLE=null with the label and SKIP_LABELS set, expecting proceed=false: not ok (the script emits proceed=true through the "did not report mergeability" path)

On the unmodified branch, both added cases pass. The last one is also worth keeping as its own test, because it covers the path where a missed label actually leads to an approval.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8c7c71d. Added the call-count check to this test, plus a test where mergeability never resolves.

@vcauesantos

Copy link
Copy Markdown
Contributor Author

Both notes are fixed in 8c7c71d. Labels are now compared without case, and the match runs inside the same PR read, so no label name goes through @tsv as JSON. Tests cover both, and I checked the filter with the real gh CLI too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants