From 4bd04b222db2273138bfb1739bded028fdfbf58e Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Sat, 5 Sep 2026 22:14:05 +0000 Subject: [PATCH] fix(ci): retire superseded Ladon change requests --- .github/workflows/ai-review.yml | 28 +++++- .github/workflows/ci.yml | 2 + scripts/retire-superseded-ladon-reviews.cjs | 67 +++++++++++++ .../retire-superseded-ladon-reviews.test.cjs | 99 +++++++++++++++++++ 4 files changed, 195 insertions(+), 1 deletion(-) create mode 100644 scripts/retire-superseded-ladon-reviews.cjs create mode 100644 scripts/retire-superseded-ladon-reviews.test.cjs diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index 849908492..783794427 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -16,6 +16,7 @@ on: paths-ignore: - ".github/workflows/ai-review.yml" - "LADON.md" + - "scripts/retire-superseded-ladon-reviews.cjs" jobs: code_review: @@ -60,7 +61,7 @@ jobs: MODIFIED="" while IFS= read -r f; do case "$f" in - .github/workflows/ai-review.yml|LADON.md) + .github/workflows/ai-review.yml|LADON.md|scripts/retire-superseded-ladon-reviews.cjs) MODIFIED="${MODIFIED}${f}"$'\n' ;; esac done <<< "$CHANGED" @@ -104,3 +105,28 @@ jobs: skip-bot-authors: dependabot[bot],renovate[bot],github-actions[bot],aao-ipr-bot[bot] # Optional; defaults to the review action's pinned model. model: claude-opus-4-8 + + # GitHub keeps an earlier CHANGES_REQUESTED review blocking even after + # the same App approves a corrected head. Retire only Ladon's own older + # change requests, and only when its latest review approves this head. + - name: Retire superseded Ladon change requests + if: steps.workflow-mod.outputs.modified != 'true' + uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8 + env: + LADON_BOT_LOGIN: ${{ steps.app-token.outputs.app-slug }}[bot] + PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + with: + github-token: ${{ steps.app-token.outputs.token }} + script: | + const { retireSupersededLadonReviews } = require( + `${process.env.GITHUB_WORKSPACE}/scripts/retire-superseded-ladon-reviews.cjs` + ); + const dismissed = await retireSupersededLadonReviews({ + github, + owner: context.repo.owner, + repo: context.repo.repo, + pullNumber: context.issue.number, + botLogin: process.env.LADON_BOT_LOGIN, + headSha: process.env.PR_HEAD_SHA, + }); + core.info(`Dismissed ${dismissed.length} superseded Ladon review(s).`); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5e952e040..b6664bba7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,6 +37,8 @@ jobs: # catches up. flags: >- -ignore ^unexpected\s+key\s+\x22queue\x22\s+for\s+\x22concurrency\x22\s+section + - name: Test Ladon review-state helper + run: node --test scripts/retire-superseded-ladon-reviews.test.cjs - name: Audit workflows uses: zizmorcore/zizmor-action@3dc1ecc9bcb9e94e9b2c709687979e1298497054 # v0.6.2 with: diff --git a/scripts/retire-superseded-ladon-reviews.cjs b/scripts/retire-superseded-ladon-reviews.cjs new file mode 100644 index 000000000..6c505fb2e --- /dev/null +++ b/scripts/retire-superseded-ladon-reviews.cjs @@ -0,0 +1,67 @@ +#!/usr/bin/env node + +const APPROVED = 'APPROVED'; +const CHANGES_REQUESTED = 'CHANGES_REQUESTED'; + +function normalizeLogin(login) { + return String(login || '').toLowerCase(); +} + +function latestBotReview(reviews, botLogin) { + const normalizedBotLogin = normalizeLogin(botLogin); + return reviews + .filter((review) => normalizeLogin(review.user?.login) === normalizedBotLogin) + .sort((left, right) => Number(left.id) - Number(right.id)) + .at(-1); +} + +function supersededChangeRequests(reviews, botLogin, headSha) { + const latest = latestBotReview(reviews, botLogin); + if (!latest || latest.state !== APPROVED || latest.commit_id !== headSha) { + return []; + } + + const normalizedBotLogin = normalizeLogin(botLogin); + return reviews.filter( + (review) => + normalizeLogin(review.user?.login) === normalizedBotLogin && + review.state === CHANGES_REQUESTED && + Number(review.id) < Number(latest.id), + ); +} + +async function retireSupersededLadonReviews({ + github, + owner, + repo, + pullNumber, + botLogin, + headSha, +}) { + const reviews = await github.paginate(github.rest.pulls.listReviews, { + owner, + repo, + pull_number: pullNumber, + per_page: 100, + }); + const superseded = supersededChangeRequests(reviews, botLogin, headSha); + + for (const review of superseded) { + await github.rest.pulls.dismissReview({ + owner, + repo, + pull_number: pullNumber, + review_id: review.id, + message: `Superseded by Ladon approval of ${headSha}.`, + event: 'DISMISS', + }); + } + + return superseded.map((review) => review.id); +} + +module.exports = { + latestBotReview, + retireSupersededLadonReviews, + supersededChangeRequests, +}; diff --git a/scripts/retire-superseded-ladon-reviews.test.cjs b/scripts/retire-superseded-ladon-reviews.test.cjs new file mode 100644 index 000000000..347210c76 --- /dev/null +++ b/scripts/retire-superseded-ladon-reviews.test.cjs @@ -0,0 +1,99 @@ +#!/usr/bin/env node + +const assert = require('node:assert/strict'); +const test = require('node:test'); + +const { + retireSupersededLadonReviews, + supersededChangeRequests, +} = require('./retire-superseded-ladon-reviews.cjs'); + +const BOT = 'aao-secretariat[bot]'; +const HEAD = 'new-head'; + +function review(id, state, commitId = HEAD, login = BOT) { + return { id, state, commit_id: commitId, user: { login } }; +} + +test('selects only the bot change requests superseded by its final head approval', () => { + const reviews = [ + review(10, 'CHANGES_REQUESTED', 'old-head'), + review(11, 'CHANGES_REQUESTED'), + review(12, 'CHANGES_REQUESTED', 'old-head', 'human-reviewer'), + review(13, 'DISMISSED', 'old-head'), + review(14, 'APPROVED'), + ]; + + assert.deepEqual( + supersededChangeRequests(reviews, 'AAO-SECRETARIAT[BOT]', HEAD).map( + ({ id }) => id, + ), + [10, 11], + ); +}); + +test('keeps change requests when the latest bot review is not an approval', () => { + const reviews = [ + review(20, 'CHANGES_REQUESTED', 'old-head'), + review(21, 'APPROVED'), + review(22, 'COMMENTED'), + ]; + + assert.deepEqual(supersededChangeRequests(reviews, BOT, HEAD), []); +}); + +test('keeps change requests when the latest approval targets an older head', () => { + const reviews = [ + review(30, 'CHANGES_REQUESTED', 'older-head'), + review(31, 'APPROVED', 'previous-head'), + ]; + + assert.deepEqual(supersededChangeRequests(reviews, BOT, HEAD), []); +}); + +test('dismisses every selected review through the pull request API', async () => { + const dismissed = []; + const listReviews = Symbol('listReviews'); + const github = { + paginate: async (method, params) => { + assert.equal(method, listReviews); + assert.deepEqual(params, { + owner: 'adcontextprotocol', + repo: 'adcp-client-python', + pull_number: 1134, + per_page: 100, + }); + return [ + review(40, 'CHANGES_REQUESTED', 'old-head'), + review(41, 'APPROVED'), + ]; + }, + rest: { + pulls: { + listReviews, + dismissReview: async (params) => dismissed.push(params), + }, + }, + }; + + const ids = await retireSupersededLadonReviews({ + github, + owner: 'adcontextprotocol', + repo: 'adcp-client-python', + pullNumber: 1134, + botLogin: BOT, + headSha: HEAD, + }); + + assert.deepEqual(ids, [40]); + assert.deepEqual(dismissed, [ + { + owner: 'adcontextprotocol', + repo: 'adcp-client-python', + pull_number: 1134, + review_id: 40, + message: `Superseded by Ladon approval of ${HEAD}.`, + event: 'DISMISS', + }, + ]); +});