Repository navigation
feat(auto-approve-bot-prs): keep prs with a skip label for a person - #331
vcauesantos wants to merge 2 commits into
Conversation
loft-bot
left a comment
There was a problem hiding this comment.
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_downcaseon both sides)? GitHub treats label names case-insensitively, but hereskip-labels: Backport-AI-Resolvedagainst abackport-ai-resolvedlabel returnsproceed=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@tsvrow, for example by running the match inside the same--jqfilter with$ENV.SKIP_LABELS?@tsvescapes the backslashes thattojsonadds 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" ] |
There was a problem hiding this comment.
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:
| [ "$(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=nullwith the label andSKIP_LABELSset, expectingproceed=false:not ok(the script emitsproceed=truethrough 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.
There was a problem hiding this comment.
Fixed in 8c7c71d. Added the call-count check to this test, plus a test where mergeability never resolves.
|
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 |
Summary
skip-labelsinput 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.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.skip-labelsis set and the labels can't be read, the PR isn't approved.skip-labelsempty, nothing changes. This matters because the shared workflow checks out the action atmain, so it reaches every caller when it merges.Why
#330 labels backports
backport-ai-resolvedwhen the AI pass writes a commit.loft-enterprisealready auto-approves greenbackport/*PRs fromloft-bot, so without this an AI-written backport would be approved with no person looking at it. Theloft-enterprisecaller passesskip-labels: backport-ai-resolvedin 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
check-pr-after-ci.bats: a skip label blocks approval; any label from a list with spaces blocks it; other labels don't; withoutskip-labelsnothing 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.ghCLI 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 -xandmake check-docsare clean.main. I couldn't runwait-for-ci.batsbecause it needs GNUtimeout. CI runs all suites on Ubuntu.Related to DEVOPS-1648