From 4e1a7c3037e8d410152c5473a4aaf012d5d6294a Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Fri, 25 Sep 2026 12:07:34 -0400 Subject: [PATCH 01/23] refactor: rename example pre-review-pr workflow more general beyond just pre-commit --- .../{review-pre-commit-pr.yml => pre-review-pr.yml} | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) rename examples/{review-pre-commit-pr.yml => pre-review-pr.yml} (92%) diff --git a/examples/review-pre-commit-pr.yml b/examples/pre-review-pr.yml similarity index 92% rename from examples/review-pre-commit-pr.yml rename to examples/pre-review-pr.yml index b28bb7db..d913cce9 100644 --- a/examples/review-pre-commit-pr.yml +++ b/examples/pre-review-pr.yml @@ -1,4 +1,4 @@ -name: review-pre-commit-pr +name: pre-review-pr on: pull_request: @@ -24,7 +24,7 @@ concurrency: cancel-in-progress: true jobs: - find-prs: + find-prec-prs: # Only needed when manually dispatched; the pull_request event already has a single PR to act on if: github.event_name == 'workflow_dispatch' runs-on: ubuntu-latest @@ -44,7 +44,7 @@ jobs: echo "pr-numbers=${pr_numbers}" >> "$GITHUB_OUTPUT" review-pre-commit-pr: - needs: [find-prs] + needs: [find-prec-prs] runs-on: ubuntu-latest container: nciccbr/ccbr_actions:latest # Only run for pre-commit.ci autoupdate PRs or when ccbr-bot[bot] is requested as a reviewer @@ -59,12 +59,12 @@ jobs: (github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'ccbr-bot[bot]') ) ) - || (github.event_name == 'workflow_dispatch' && needs.find-prs.outputs.pr-numbers != '[]') + || (github.event_name == 'workflow_dispatch' && needs.find-prec-prs.outputs.pr-numbers != '[]') ) strategy: fail-fast: false matrix: - pr-number: ${{ github.event_name == 'workflow_dispatch' && fromJson(needs.find-prs.outputs.pr-numbers) || fromJson(format('[{0}]', github.event.pull_request.number)) }} + pr-number: ${{ github.event_name == 'workflow_dispatch' && fromJson(needs.find-prec-prs.outputs.pr-numbers) || fromJson(format('[{0}]', github.event.pull_request.number)) }} steps: - uses: actions/checkout@v7 @@ -78,7 +78,6 @@ jobs: permission-contents: write permission-issues: write permission-pull-requests: write - - uses: CCBR/actions/review-pre-commit-pr@v0.8.0 with: github-token: ${{ steps.generate-token.outputs.token }} From 0309b77424ebbbd5b0b07daaf502953f18300b09 Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Fri, 25 Sep 2026 12:15:26 -0400 Subject: [PATCH 02/23] fix(pre-commit-review): post failure as comment, do not request changes fixes #225 --- review-pre-commit-pr/README.md | 3 +- review-pre-commit-pr/README.qmd | 2 +- src/ccbr_actions/pre_commit.py | 13 +++-- tests/test_pre_commit.py | 92 ++++++++++++++++----------------- 4 files changed, 52 insertions(+), 58 deletions(-) diff --git a/review-pre-commit-pr/README.md b/review-pre-commit-pr/README.md index ca09c69b..b20306e4 100644 --- a/review-pre-commit-pr/README.md +++ b/review-pre-commit-pr/README.md @@ -28,8 +28,7 @@ When both conditions are satisfied the action: When either condition is **not** satisfied the action: -- Submits a `REQUEST_CHANGES` review listing which conditions were not - met. +- Posts a comment listing which conditions were not met. - Requests a human reviewer, resolved in this order: 1. the `reviewer` input, if provided; 2. otherwise, the owner(s) of `.pre-commit-config.yaml` per the diff --git a/review-pre-commit-pr/README.qmd b/review-pre-commit-pr/README.qmd index 7037a2d2..5a5170fc 100644 --- a/review-pre-commit-pr/README.qmd +++ b/review-pre-commit-pr/README.qmd @@ -37,7 +37,7 @@ When both conditions are satisfied the action: When either condition is **not** satisfied the action: -- Submits a `REQUEST_CHANGES` review listing which conditions were not met. +- Posts a comment listing which conditions were not met. - Requests a human reviewer, resolved in this order: 1. the `reviewer` input, if provided; 2. otherwise, the owner(s) of `.pre-commit-config.yaml` per the repo's diff --git a/src/ccbr_actions/pre_commit.py b/src/ccbr_actions/pre_commit.py index 7502bb81..db2fc14e 100644 --- a/src/ccbr_actions/pre_commit.py +++ b/src/ccbr_actions/pre_commit.py @@ -16,7 +16,6 @@ get_pr_files, is_pr_approved, post_pr_comment, - request_changes, request_reviewer, ) @@ -269,8 +268,8 @@ def review_pre_commit_pr( When both conditions are met the function approves the PR, attempts to enable squash auto-merge, and returns ``True``. If auto-merge cannot be enabled, it leaves a separate comment with the GitHub API error. Otherwise - it submits a *REQUEST_CHANGES* review explaining why the PR needs manual - review, requests a human reviewer, and returns ``False``. The reviewer is resolved via + it posts a comment explaining why the PR needs manual review, requests a + human reviewer, and returns ``False``. The reviewer is resolved via [](`~ccbr_actions.pr_review.determine_reviewer`): *reviewer* if given, otherwise the repo's ``CODEOWNERS`` entry for the config file, otherwise the most recent human committer to the config file. @@ -424,13 +423,13 @@ def review_pre_commit_pr( f"because the following conditions were not met:\n{reasons}" ) try: - response = request_changes( + response = post_pr_comment( repo, pr_number, comment, token=token, session=session ) - print(f"Submitted REQUEST_CHANGES review (HTTP {response.status_code})") + print(f"Posted human-review comment (HTTP {response.status_code})") except (requests.exceptions.RequestException, RuntimeError) as exc: - print(f"Could not submit REQUEST_CHANGES review: {exc}") - warnings.warn(f"Could not submit request-changes review: {exc}") + print(f"Could not post human-review comment: {exc}") + warnings.warn(f"Could not post human-review comment: {exc}") if resolved_reviewer: try: response = request_reviewer( diff --git a/tests/test_pre_commit.py b/tests/test_pre_commit.py index 3cd6b4bf..b72a0461 100644 --- a/tests/test_pre_commit.py +++ b/tests/test_pre_commit.py @@ -515,7 +515,7 @@ def test_review_pre_commit_pr_approves_sliding_tag_pointing_at_same_commit(): assert any(body.get("event") == "APPROVE" for body in review_bodies) -def test_review_pre_commit_pr_requests_human_review_for_sliding_tag_different_commit(): +def test_review_pre_commit_pr_comments_when_sliding_tag_has_different_commit(): # Same rev change as above, but the two tags point at different commits, # so it's a genuine downgrade and should still require human review. session = _make_review_session(patch=SLIDING_TAG_PATCH) @@ -527,10 +527,11 @@ def test_review_pre_commit_pr_requests_human_review_for_sliding_tag_different_co } result = review_pre_commit_pr("CCBR/repo", 7, "alice", token="tok", session=session) assert result is False - review_bodies = [ - c[2]["json"] for c in session.calls if c[0] == "POST" and "reviews" in c[1] + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/7/comments" in c[1] ] - assert any(body.get("event") == "REQUEST_CHANGES" for body in review_bodies) + assert comment_calls + assert "requires human review" in comment_calls[0][2]["json"]["body"] def test_review_pre_commit_pr_skips_when_current_review_is_approved(): @@ -568,29 +569,31 @@ def test_review_pre_commit_pr_does_not_skip_superseded_approval(): assert any(c[2]["json"].get("event") == "APPROVE" for c in review_calls) -def test_review_pre_commit_pr_requests_human_review_when_extra_file(): +def test_review_pre_commit_pr_comments_when_extra_file(): session = _make_review_session(extra_files=[{"filename": "README.md", "patch": ""}]) result = review_pre_commit_pr("CCBR/repo", 7, "alice", token="tok", session=session) assert result is False posted_urls = [c[1] for c in session.calls if c[0] == "POST"] - # Should submit a REQUEST_CHANGES review and request the given reviewer, but NOT approve + # Should post a comment and request the given reviewer, but NOT approve. review_bodies = [ c[2]["json"] for c in session.calls if c[0] == "POST" and "reviews" in c[1] ] assert not any(body.get("event") == "APPROVE" for body in review_bodies) - assert any(body.get("event") == "REQUEST_CHANGES" for body in review_bodies) + assert not review_bodies + assert any("issues/7/comments" in u for u in posted_urls) assert any("requested_reviewers" in u for u in posted_urls) -def test_review_pre_commit_pr_requests_human_review_for_non_rev_change(): +def test_review_pre_commit_pr_comments_for_non_rev_change(): session = _make_review_session(patch=INVALID_PATCH_NON_REV_CHANGE) result = review_pre_commit_pr("CCBR/repo", 7, "bob", token="tok", session=session) assert result is False - review_calls = [c for c in session.calls if c[0] == "POST" and "reviews" in c[1]] - assert review_calls - review_body = review_calls[0][2]["json"] - assert review_body["event"] == "REQUEST_CHANGES" - assert "@bob" in review_body["body"] + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/7/comments" in c[1] + ] + assert comment_calls + comment_body = comment_calls[0][2]["json"]["body"] + assert "@bob" in comment_body def test_review_pre_commit_pr_warns_when_reviewer_request_fails(monkeypatch): @@ -609,18 +612,16 @@ def _fail_request(*args, **kwargs): assert result is False -def test_review_pre_commit_pr_warns_when_request_changes_fails(monkeypatch): - """request-changes review error should emit a warning but not raise.""" +def test_review_pre_commit_pr_warns_when_human_review_comment_fails(monkeypatch): + """Human-review comment errors should emit a warning but not raise.""" session = _make_review_session(patch=DOWNGRADE_PATCH) - def _fail_request_changes(*args, **kwargs): - raise RuntimeError("not allowed to request changes") + def _fail_comment(*args, **kwargs): + raise RuntimeError("comment is not allowed") - monkeypatch.setattr( - "ccbr_actions.pre_commit.request_changes", _fail_request_changes - ) + monkeypatch.setattr("ccbr_actions.pre_commit.post_pr_comment", _fail_comment) - with pytest.warns(UserWarning, match="Could not submit request-changes review"): + with pytest.warns(UserWarning, match="Could not post human-review comment"): result = review_pre_commit_pr( "CCBR/repo", 7, "dave", token="tok", session=session ) @@ -654,7 +655,7 @@ def test_review_pre_commit_pr_keeps_approval_when_auto_merge_api_fails(): assert "GraphQL errors" in comment_body -def test_review_pre_commit_pr_requests_human_review_when_approval_fails(monkeypatch): +def test_review_pre_commit_pr_comments_when_approval_fails(monkeypatch): session = _make_review_session() def _fail_approval(*args, **kwargs): @@ -665,12 +666,12 @@ def _fail_approval(*args, **kwargs): result = review_pre_commit_pr("CCBR/repo", 7, "erin", token="tok", session=session) assert result is False - review_calls = [c for c in session.calls if c[0] == "POST" and "reviews" in c[1]] - review_bodies = [c[2]["json"] for c in review_calls] - request_changes_body = next( - body["body"] for body in review_bodies if body.get("event") == "REQUEST_CHANGES" - ) - assert "automatic approval could not be completed" in request_changes_body + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/7/comments" in c[1] + ] + assert comment_calls + comment_body = comment_calls[0][2]["json"]["body"] + assert "automatic approval could not be completed" in comment_body assert any("requested_reviewers" in c[1] for c in session.calls if c[0] == "POST") @@ -692,7 +693,7 @@ def _fail_comment(*args, **kwargs): assert result is True -def test_review_pre_commit_pr_requests_human_review_when_title_or_sender_mismatch(): +def test_review_pre_commit_pr_comments_when_title_or_sender_mismatch(): session = _make_review_session( pr_node_payload={ "node_id": "PR_NODE_7", @@ -705,12 +706,13 @@ def test_review_pre_commit_pr_requests_human_review_when_title_or_sender_mismatc review_calls = [c for c in session.calls if c[0] == "POST" and "reviews" in c[1]] review_bodies = [c[2]["json"] for c in review_calls] assert not any(body.get("event") == "APPROVE" for body in review_bodies) - request_changes_calls = [ - body for body in review_bodies if body.get("event") == "REQUEST_CHANGES" + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/7/comments" in c[1] ] - assert request_changes_calls - assert "@frank" in request_changes_calls[0]["body"] - assert "autoupdate bot pattern" in request_changes_calls[0]["body"] + assert comment_calls + comment_body = comment_calls[0][2]["json"]["body"] + assert "@frank" in comment_body + assert "autoupdate bot pattern" in comment_body def test_review_pre_commit_pr_works_without_reviewer_input(): @@ -724,14 +726,11 @@ def test_review_pre_commit_pr_works_without_reviewer_input(): ) result = review_pre_commit_pr("CCBR/repo", 7, token="tok", session=session) assert result is False - review_calls = [c for c in session.calls if c[0] == "POST" and "reviews" in c[1]] - request_changes_calls = [ - c[2]["json"] - for c in review_calls - if c[2]["json"].get("event") == "REQUEST_CHANGES" + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/7/comments" in c[1] ] - assert request_changes_calls - assert "@a-human" in request_changes_calls[0]["body"] + assert comment_calls + assert "@a-human" in comment_calls[0][2]["json"]["body"] reviewer_calls = [ c for c in session.calls if c[0] == "POST" and "requested_reviewers" in c[1] ] @@ -747,14 +746,11 @@ def test_review_pre_commit_pr_skips_reviewer_request_when_none_resolved(): ) result = review_pre_commit_pr("CCBR/repo", 7, token="tok", session=session) assert result is False - review_calls = [c for c in session.calls if c[0] == "POST" and "reviews" in c[1]] - request_changes_calls = [ - c[2]["json"] - for c in review_calls - if c[2]["json"].get("event") == "REQUEST_CHANGES" + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/7/comments" in c[1] ] - assert request_changes_calls - assert request_changes_calls[0]["body"].startswith("This pre-commit.ci") + assert comment_calls + assert comment_calls[0][2]["json"]["body"].startswith("This pre-commit.ci") reviewer_calls = [ c for c in session.calls if c[0] == "POST" and "requested_reviewers" in c[1] ] From 1cd3e95feab39d41e32fd4f46de9aba3716bf5e8 Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Fri, 25 Sep 2026 12:19:30 -0400 Subject: [PATCH 03/23] docs: update CHANGELOG.md --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fd6d264..a0215909 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## actions development version +- Fix `review-pre-commit-pr` to post a comment instead of requesting changes when a pre-commit.ci autoupdate PR needs human review. (#226, @kelly-sovacool, @copilot) + ## actions 0.8.0 ### New From 1eab3a8f2f7c7f0a5bef82f03eae4038611d8d1a Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Fri, 25 Sep 2026 12:37:48 -0400 Subject: [PATCH 04/23] chore: ignore uv.lock --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index efa57e05..d37337f0 100644 --- a/.gitignore +++ b/.gitignore @@ -177,6 +177,9 @@ cython_debug/ .pixi/* !.pixi/config.toml +# uv +uv.lock + # genAI .github/prompts/ .github/plans/ From a8af359ebbcb87a5bd5b94f7df3eeb1aa38d80d6 Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Fri, 25 Sep 2026 12:46:09 -0400 Subject: [PATCH 05/23] feat: new action review-post-release-pr resolves #209 --- examples/pre-review-pr.yml | 63 +++ review-post-release-pr/README.md | 122 +++++ review-post-release-pr/README.qmd | 105 ++++ review-post-release-pr/action.yml | 80 ++++ src/ccbr_actions/post_release_pr.py | 675 ++++++++++++++++++++++++++ src/ccbr_actions/pr_review.py | 62 +++ tests/test_action_test_coverage.py | 1 + tests/test_post_release_pr.py | 712 ++++++++++++++++++++++++++++ tests/test_pr_review.py | 99 +++- 9 files changed, 1918 insertions(+), 1 deletion(-) create mode 100644 review-post-release-pr/README.md create mode 100644 review-post-release-pr/README.qmd create mode 100644 review-post-release-pr/action.yml create mode 100644 src/ccbr_actions/post_release_pr.py create mode 100644 tests/test_post_release_pr.py diff --git a/examples/pre-review-pr.yml b/examples/pre-review-pr.yml index d913cce9..c95474c0 100644 --- a/examples/pre-review-pr.yml +++ b/examples/pre-review-pr.yml @@ -15,6 +15,7 @@ on: type: boolean permissions: + actions: write contents: write issues: write pull-requests: write @@ -85,3 +86,65 @@ jobs: repo: ${{ github.repository }} reviewer: CCBR/adminteam force-review: ${{ github.event.action == 'review_requested' || inputs.force-review || false }} + + find-post-release-prs: + # Only needed when manually dispatched; the pull_request event already has a single PR to act on + if: github.event_name == 'workflow_dispatch' + runs-on: ubuntu-latest + outputs: + pr-numbers: ${{ steps.search.outputs.pr-numbers }} + steps: + - name: Search for open post-release cleanup PRs + id: search + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + pr_numbers=$(gh pr list \ + --repo "${{ github.repository }}" \ + --state open \ + --json number,title,author \ + --jq '[.[] | select(.title | startswith("chore: post-release cleanup for")) | select(.author.is_bot)] | map(.number)') + echo "pr-numbers=${pr_numbers}" >> "$GITHUB_OUTPUT" + + review-post-release-pr: + needs: [find-post-release-prs] + runs-on: ubuntu-latest + container: nciccbr/ccbr_actions:latest + # Only run for post-release cleanup PRs or when ccbr-bot[bot] is requested as a reviewer + if: > + always() && + ( + ( + github.event_name == 'pull_request' && + startsWith(github.event.pull_request.title, 'chore: post-release cleanup for') && + ( + (github.event.action == 'opened' && github.event.sender.type == 'Bot') || + (github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'ccbr-bot[bot]') + ) + ) + || (github.event_name == 'workflow_dispatch' && needs.find-post-release-prs.outputs.pr-numbers != '[]') + ) + strategy: + fail-fast: false + matrix: + pr-number: ${{ github.event_name == 'workflow_dispatch' && fromJson(needs.find-post-release-prs.outputs.pr-numbers) || fromJson(format('[{0}]', github.event.pull_request.number)) }} + + steps: + - uses: actions/checkout@v7 + - name: Generate CCBR-bot token + id: generate-token + uses: actions/create-github-app-token@v3 + with: + client-id: ${{ vars.CCBR_BOT_APP_ID }} + private-key: ${{ secrets.CCBR_BOT_PRIVATE_KEY }} + owner: ${{ github.repository_owner }} + permission-actions: write + permission-contents: write + permission-issues: write + permission-pull-requests: write + - uses: CCBR/actions/review-post-release-pr@v0.8.0 + with: + github-token: ${{ steps.generate-token.outputs.token }} + pr-number: ${{ matrix.pr-number }} + repo: ${{ github.repository }} + force-review: ${{ github.event.action == 'review_requested' || inputs.force-review || false }} diff --git a/review-post-release-pr/README.md b/review-post-release-pr/README.md new file mode 100644 index 00000000..acdc9282 --- /dev/null +++ b/review-post-release-pr/README.md @@ -0,0 +1,122 @@ +# review-post-release-pr + +**`review-post-release-pr`** - Review a post-release cleanup PR: approve +pending workflow runs, approve, and enable auto-merge when only +version/date bumps matching a real release tag are present, otherwise +request a human reviewer + +This action automates the review of post-release cleanup pull requests +opened by the [post-release](./post-release) action, with titles of the +form `chore: post-release cleanup for `. + +The action verifies three conditions: + +1. **The PR matches the post-release cleanup pattern** – the title + matches `chore: post-release cleanup for `, the sender is a + GitHub App (Bot), and `` corresponds to an actual GitHub + release in the repo. +2. **Only allowed files were changed** – the version file, citation + file, `codemeta.json`, changelog file, news file, and/or readme + files. No other files should be modified. +3. **Only version/date bumps were made** – every change in those files + must be a version or date bump consistent with the release found in + condition + 1. A readme file’s suggested citation snippet may also be + re-rendered by the `auto-format` action in response to the + version/date bump; that type of change is accepted the same way. + +When all three conditions are satisfied the action: + +- Approves any workflow runs pending approval on the PR’s head branch. +- Approves the PR as the token’s actor (typically CCBR-bot). +- Attempts to enable squash auto-merge so the PR merges automatically + once all required checks pass. If auto-merge is unavailable, the + approval remains and the action posts a comment containing the GitHub + API error. + +When any condition is **not** satisfied the action: + +- Posts a comment listing which conditions were not met. +- Requests a human reviewer, resolved in this order: + 1. the `reviewer` input, if provided; + 2. otherwise, the actor that triggered the most recent + `draft-release.yml` workflow run; + 3. otherwise, the repo’s default (catch-all `*`) `CODEOWNERS` entry. + +Set `force-review` to `true` to re-submit the review and reviewer +request even when the PR already has an approval. It defaults to +`false`. + +## Usage + +You should call this action from a workflow that is triggered on +`pull_request` events. Generate a CCBR-bot token first (using +`actions/create-github-app-token`) so that the approval and merge +operations are performed as CCBR-bot. The workflow and GitHub App token +must allow `actions: write` (to approve pending workflow runs), +`contents: write`, `pull-requests: write`, and `issues: write`. The +`contents` permission is required to enable auto-merge; `issues` is used +for comments when auto-merge cannot be enabled. The GitHub App +installation must have these repository permissions as well. + +### Basic example + +```yaml +on: + pull_request: + types: + - opened + - review_requested + +permissions: + actions: write + contents: write + issues: write + pull-requests: write + +jobs: + review-post-release-pr: + runs-on: ubuntu-latest + container: nciccbr/ccbr_actions:latest + # Only run for post-release cleanup PRs + if: startsWith(github.event.pull_request.title, 'chore: post-release cleanup for') + steps: + - uses: actions/create-github-app-token@v3 + id: generate-token + with: + client-id: ${{ vars.CCBR_BOT_APP_ID }} + private-key: ${{ secrets.CCBR_BOT_PRIVATE_KEY }} + - uses: CCBR/actions/review-post-release-pr@main + with: + github-token: ${{ steps.generate-token.outputs.token }} + pr-number: ${{ github.event.pull_request.number }} +``` + +See also [pre-review-pr.yml](./examples/pre-review-pr.yml). + +## Inputs + +- `github-token`: GitHub API token (e.g. a CCBR-bot token generated with + actions/create-github-app-token). **Required.** +- `pr-number`: Pull request number to review. **Required.** +- `repo`: Repository full name (e.g. CCBR/actions). **Required.** + Default: `${{ github.repository }}`. +- `reviewer`: GitHub username to request as reviewer when the PR + requires human review. If omitted, it is resolved from the actor that + triggered the most recent draft-release workflow run, falling back to + the repo’s default CODEOWNERS entry. +- `force-review`: Re-submit the review and reviewer request even when + the PR already has an approval. Default: `false`. +- `version-filepath`: Path to the file containing the current version. + Default: `VERSION`. +- `citation-filepath`: Path to the citation file. Default: + `CITATION.cff`. +- `changelog-filepath`: Path to the changelog or news file. Default: + `CHANGELOG.md`. +- `description-filepath`: Path to the R DESCRIPTION file, used when an R + package is detected. Default: `DESCRIPTION`. +- `ccbr-actions-version`: The version of CCBR/actions to install when + running outside the ccbr_actions container (branch, tag, or ‘latest’). + **Required.** Default: `latest`. +- `python-version`: Python version to use. **Required.** Default: + `3.14`. diff --git a/review-post-release-pr/README.qmd b/review-post-release-pr/README.qmd new file mode 100644 index 00000000..6b5dc427 --- /dev/null +++ b/review-post-release-pr/README.qmd @@ -0,0 +1,105 @@ +--- +title: review-post-release-pr +format: gfm +execute: + echo: false + output: asis +--- + + +```{python} +import ccbr_actions.docs + +action = ccbr_actions.docs.parse_action_yaml('action.yml') +print(ccbr_actions.docs.action_markdown_desc(action)) +``` + +This action automates the review of post-release cleanup pull requests +opened by the [post-release](/post-release) action, with titles of the form +`chore: post-release cleanup for `. + +The action verifies three conditions: + +1. **The PR matches the post-release cleanup pattern** – the title matches + `chore: post-release cleanup for `, the sender is a GitHub App (Bot), + and `` corresponds to an actual GitHub release in the repo. +2. **Only allowed files were changed** – the version file, citation file, + `codemeta.json`, changelog file, news file, and/or readme files. No other + files should be modified. +3. **Only version/date bumps were made** – every change in those files must + be a version or date bump consistent with the release found in condition + 1. A readme file's suggested citation snippet may also be re-rendered by + the `auto-format` action in response to the version/date bump; that type + of change is accepted the same way. + +When all three conditions are satisfied the action: + +- Approves any workflow runs pending approval on the PR's head branch. +- Approves the PR as the token's actor (typically CCBR-bot). +- Attempts to enable squash auto-merge so the PR merges automatically once all + required checks pass. If auto-merge is unavailable, the approval remains and + the action posts a comment containing the GitHub API error. + +When any condition is **not** satisfied the action: + +- Posts a comment listing which conditions were not met. +- Requests a human reviewer, resolved in this order: + 1. the `reviewer` input, if provided; + 2. otherwise, the actor that triggered the most recent `draft-release.yml` + workflow run; + 3. otherwise, the repo's default (catch-all `*`) `CODEOWNERS` entry. + +Set `force-review` to `true` to re-submit the review and reviewer request even +when the PR already has an approval. It defaults to `false`. + +## Usage + +You should call this action from a workflow that is triggered on +`pull_request` events. Generate a CCBR-bot token first (using +`actions/create-github-app-token`) so that the approval and merge +operations are performed as CCBR-bot. The workflow and GitHub App token +must allow `actions: write` (to approve pending workflow runs), +`contents: write`, `pull-requests: write`, and `issues: write`. +The `contents` permission is required to enable auto-merge; `issues` is used +for comments when auto-merge cannot be enabled. The GitHub App installation +must have these repository permissions as well. + +### Basic example + +```yaml +on: + pull_request: + types: + - opened + - review_requested + +permissions: + actions: write + contents: write + issues: write + pull-requests: write + +jobs: + review-post-release-pr: + runs-on: ubuntu-latest + container: nciccbr/ccbr_actions:latest + # Only run for post-release cleanup PRs + if: startsWith(github.event.pull_request.title, 'chore: post-release cleanup for') + steps: + - uses: actions/create-github-app-token@v3 + id: generate-token + with: + client-id: ${{ vars.CCBR_BOT_APP_ID }} + private-key: ${{ secrets.CCBR_BOT_PRIVATE_KEY }} + - uses: CCBR/actions/review-post-release-pr@main + with: + github-token: ${{ steps.generate-token.outputs.token }} + pr-number: ${{ github.event.pull_request.number }} +``` + +See also [pre-review-pr.yml](/examples/pre-review-pr.yml). + + +```{python} +print(ccbr_actions.docs.action_markdown_io(action)) +``` diff --git a/review-post-release-pr/action.yml b/review-post-release-pr/action.yml new file mode 100644 index 00000000..f7978298 --- /dev/null +++ b/review-post-release-pr/action.yml @@ -0,0 +1,80 @@ +name: review-post-release-pr +description: "Review a post-release cleanup PR: approve pending workflow runs, approve, and enable auto-merge when only version/date bumps matching a real release tag are present, otherwise request a human reviewer" +author: "Kelly Sovacool" + +inputs: + github-token: + description: "GitHub API token (e.g. a CCBR-bot token generated with actions/create-github-app-token)." + required: true + pr-number: + description: "Pull request number to review." + required: true + repo: + description: "Repository full name (e.g. CCBR/actions)." + required: true + default: "${{ github.repository }}" + reviewer: + description: "GitHub username to request as reviewer when the PR requires human review. If omitted, it is resolved from the actor that triggered the most recent draft-release workflow run, falling back to the repo's default CODEOWNERS entry." + required: false + force-review: + description: "Re-submit the review and reviewer request even when the PR already has an approval." + required: false + default: "false" + version-filepath: + description: "Path to the file containing the current version." + default: "VERSION" + citation-filepath: + description: "Path to the citation file." + default: "CITATION.cff" + changelog-filepath: + description: "Path to the changelog or news file." + default: "CHANGELOG.md" + description-filepath: + description: "Path to the R DESCRIPTION file, used when an R package is detected." + default: "DESCRIPTION" + ccbr-actions-version: + description: "The version of CCBR/actions to install when running outside the ccbr_actions container (branch, tag, or 'latest')." + required: true + default: "latest" + python-version: + description: "Python version to use." + required: true + default: "3.14" + +runs: + using: "composite" + steps: + - name: Detect ccbr_actions container + id: detect-container + shell: bash + # env vars baked into the container image are not visible to the `env` context in `if:` expressions + run: echo "in-container=${CCBR_ACTIONS_DOCKER:-false}" >> "$GITHUB_OUTPUT" + - uses: actions/setup-python@v6 + if: ${{ steps.detect-container.outputs.in-container != 'true' }} + with: + python-version: "${{ inputs.python-version }}" + cache: "pip" + - name: Install ccbr_actions + if: ${{ steps.detect-container.outputs.in-container != 'true' }} + shell: bash + run: pip install --upgrade pip git+https://github.com/CCBR/actions.git@${{ inputs.ccbr-actions-version }} + - name: Review post-release cleanup PR + shell: python {0} + run: | + import os + + from ccbr_actions.post_release_pr import review_post_release_pr + result = review_post_release_pr( + repo="${{ inputs.repo }}", + pr_number="${{ inputs.pr-number }}", + reviewer="${{ inputs.reviewer }}", + force_review="${{ inputs.force-review }}".lower() == "true", + version_filepath="${{ inputs.version-filepath }}", + citation_filepath="${{ inputs.citation-filepath }}", + changelog_filepath="${{ inputs.changelog-filepath }}", + description_filepath="${{ inputs.description-filepath }}", + token=os.environ["GITHUB_TOKEN"], + ) + print(f"Action completed with automatic_approval={result}") + env: + GITHUB_TOKEN: ${{ inputs.github-token }} diff --git a/src/ccbr_actions/post_release_pr.py b/src/ccbr_actions/post_release_pr.py new file mode 100644 index 00000000..bd80e572 --- /dev/null +++ b/src/ccbr_actions/post_release_pr.py @@ -0,0 +1,675 @@ +""" +Helpers for reviewing post-release cleanup pull requests opened by the +``post-release`` action. +""" + +import os +import re +import warnings + +import requests + +from .github import GITHUB_API_URL, github_api_get +from .pr_review import ( + approve_pending_workflow_runs, + approve_pr, + enable_auto_merge, + get_codeowners_content, + get_last_workflow_run_actor, + get_pr_files, + is_pr_approved, + match_codeowners, + post_pr_comment, + request_reviewer, +) + +POST_RELEASE_PR_TITLE_PATTERN = re.compile(r"^chore: post-release cleanup for (\S+)$") +DRAFT_RELEASE_WORKFLOW_FILE = "draft-release.yml" +_TOKEN_PATTERN = re.compile(r"\d+(?:[.\-]\d+)*") + + +def is_post_release_pr(pr_title, pr_sender_type): + """ + Check whether a PR is a post-release cleanup PR. + + Args: + pr_title (str): Title of the pull request. + pr_sender_type (str): Type of the PR sender (e.g. ``"Bot"``). + + Returns: + bool: ``True`` if the PR title matches the post-release cleanup + pattern and the sender is a GitHub App (Bot). + """ + return ( + bool(POST_RELEASE_PR_TITLE_PATTERN.match(pr_title)) and pr_sender_type == "Bot" + ) + + +def extract_release_tag(pr_title): + """ + Extract the release tag from a post-release cleanup PR title. + + Args: + pr_title (str): Title of the pull request. + + Returns: + str | None: The release tag, or ``None`` if *pr_title* doesn't match + the expected pattern. + """ + match = POST_RELEASE_PR_TITLE_PATTERN.match(pr_title) + return match.group(1) if match else None + + +def get_release_by_tag(repo, tag, token=None, session=None): + """ + Fetch a GitHub release by its tag name. + + Args: + repo (str): Repository full name (e.g. ``"CCBR/actions"``). + tag (str): Release tag name (e.g. ``"v0.7.1"``). + token (str, optional): GitHub API token. + session: Requests-compatible session object for dependency injection. + + Returns: + dict | None: The release object, or ``None`` if no release with that + tag exists. + """ + url = f"{GITHUB_API_URL}/repos/{repo}/releases/tags/{tag}" + try: + release = github_api_get(url=url, token=token, session=session) + except requests.exceptions.RequestException: + release = None + return release + + +def _allowed_basenames( + version_filepath, citation_filepath, changelog_filepath, description_filepath +): + """ + Build the set of file basenames a post-release cleanup PR may change. + + Args: + version_filepath (str): Path to the version file. + citation_filepath (str): Path to the citation file. + changelog_filepath (str): Path to the changelog file. + description_filepath (str): Path to the R DESCRIPTION file. + + Returns: + set[str]: Allowed file basenames (README files are matched separately + by prefix). + """ + return { + os.path.basename(version_filepath), + os.path.basename(citation_filepath), + os.path.basename(changelog_filepath), + os.path.basename(description_filepath), + "codemeta.json", + "NEWS.md", + "NEWS", + } + + +def is_allowed_filename(filename, allowed_basenames): + """ + Check whether a changed file is allowed in a post-release cleanup PR. + + Args: + filename (str): Changed file's path, relative to the repo root. + allowed_basenames (set[str]): Allowed file basenames, from + [](`~ccbr_actions.post_release_pr._allowed_basenames`). + + Returns: + bool: ``True`` if *filename*'s basename is allowed, including any + README file (matched case-insensitively by prefix). + """ + basename = os.path.basename(filename) + return basename in allowed_basenames or basename.lower().startswith("readme") + + +def check_only_allowed_files_changed(pr_files, allowed_basenames): + """ + Check that every changed file in the PR is an allowed post-release file. + + Args: + pr_files (list[dict]): File objects as returned by + [](`~ccbr_actions.pr_review.get_pr_files`). + allowed_basenames (set[str]): Allowed file basenames, from + [](`~ccbr_actions.post_release_pr._allowed_basenames`). + + Returns: + bool: ``True`` if *pr_files* is non-empty and every file is allowed. + """ + return bool(pr_files) and all( + is_allowed_filename(f.get("filename", ""), allowed_basenames) for f in pr_files + ) + + +def _release_bump_values(release_tag, release): + """ + Build the set of token values allowed as a version/date bump target. + + Args: + release_tag (str): The release tag name (e.g. ``"v0.7.1"``). + release (dict): Release object from + [](`~ccbr_actions.post_release_pr.get_release_by_tag`). + + Returns: + set[str]: Acceptable new numeric/version tokens, derived from the + release's tag and its published/created date. + """ + version = release_tag.lstrip("v") + values = {release_tag, version, f"{version}-dev"} + for date_key in ("published_at", "created_at"): + date_part = (release.get(date_key) or "")[:10] + if len(date_part) == 10 and date_part[4] == "-" and date_part[7] == "-": + year, month, day = date_part.split("-") + values.update({date_part, str(int(year)), str(int(month)), str(int(day))}) + return values + + +def _split_tokens(line): + """ + Split a line into its non-numeric text segments and numeric tokens. + + Args: + line (str): A single diff line's content (without the leading +/-). + + Returns: + tuple[list[str], list[str]]: The text segments surrounding each + numeric/version-like token, and the tokens themselves. + """ + return _TOKEN_PATTERN.split(line), _TOKEN_PATTERN.findall(line) + + +def _is_valid_bump_line(old_line, new_line, valid_values): + """ + Check whether a modified line is only a version/date bump. + + Args: + old_line (str): The removed line's content. + new_line (str): The added line's content. + valid_values (set[str]): Acceptable new token values, from + [](`~ccbr_actions.post_release_pr._release_bump_values`). + + Returns: + bool: ``True`` if the surrounding text is unchanged and every + differing numeric token is an acceptable bump value. + """ + old_text, old_tokens = _split_tokens(old_line) + new_text, new_tokens = _split_tokens(new_line) + if old_text != new_text or len(old_tokens) != len(new_tokens): + result = False + else: + result = all( + old_tok == new_tok or new_tok in valid_values + for old_tok, new_tok in zip(old_tokens, new_tokens) + ) + return result + + +def _is_valid_insertion(new_line, release_version): + """ + Check whether a pure line insertion is an allowed post-release addition. + + Args: + new_line (str): The added line's content. + release_version (str): The release version (without leading ``v``). + + Returns: + bool: ``True`` if the line is blank or references the release + version (e.g. a new changelog heading). + """ + stripped = new_line.strip() + return not stripped or release_version in new_line + + +def _flush_diff_buffer(removed_buffer, added_buffer, valid_values, release_version): + """ + Validate a buffered block of consecutive removed/added diff lines. + + Removed and added lines are paired positionally. Extra removed lines must + be blank, and extra added lines must be blank or reference the release + version. + + Args: + removed_buffer (list[str]): Buffered removed line contents. + added_buffer (list[str]): Buffered added line contents. + valid_values (set[str]): Acceptable new token values, from + [](`~ccbr_actions.post_release_pr._release_bump_values`). + release_version (str): The release version (without leading ``v``). + + Returns: + bool: ``True`` if the buffered block only contains valid bumps and/or + allowed insertions. + """ + pair_count = min(len(removed_buffer), len(added_buffer)) + pairs_valid = all( + _is_valid_bump_line(removed_buffer[i], added_buffer[i], valid_values) + for i in range(pair_count) + ) + extra_removed_valid = all(not line.strip() for line in removed_buffer[pair_count:]) + extra_added_valid = all( + _is_valid_insertion(line, release_version) for line in added_buffer[pair_count:] + ) + return pairs_valid and extra_removed_valid and extra_added_valid + + +def check_patch_is_version_bump(patch, valid_values, release_version): + """ + Verify a unified diff patch only contains version/date bump changes. + + Args: + patch (str): Unified diff patch string for a single file. + valid_values (set[str]): Acceptable new token values, from + [](`~ccbr_actions.post_release_pr._release_bump_values`). + release_version (str): The release version (without leading ``v``), + allowed in pure line insertions such as a new changelog heading. + + Returns: + bool: ``True`` if every change in the patch is a valid version/date + bump or an allowed insertion. + """ + removed_buffer = [] + added_buffer = [] + is_valid = True + for line in patch.splitlines(): + if line.startswith(("---", "+++")): + pass + elif line.startswith("-"): + removed_buffer.append(line[1:]) + elif line.startswith("+"): + added_buffer.append(line[1:]) + else: + is_valid = ( + _flush_diff_buffer( + removed_buffer, added_buffer, valid_values, release_version + ) + and is_valid + ) + removed_buffer = [] + added_buffer = [] + is_valid = ( + _flush_diff_buffer(removed_buffer, added_buffer, valid_values, release_version) + and is_valid + ) + return is_valid + + +def check_version_date_bumps(pr_files, valid_values, release_version): + """ + Verify every changed file's patch is only a version/date bump. + + Args: + pr_files (list[dict]): File objects as returned by + [](`~ccbr_actions.pr_review.get_pr_files`). + valid_values (set[str]): Acceptable new token values, from + [](`~ccbr_actions.post_release_pr._release_bump_values`). + release_version (str): The release version (without leading ``v``). + + Returns: + bool: ``True`` if *pr_files* is non-empty and every file's patch only + contains valid version/date bumps. + """ + return bool(pr_files) and all( + isinstance(f.get("patch"), str) + and check_patch_is_version_bump(f["patch"], valid_values, release_version) + for f in pr_files + ) + + +def determine_post_release_reviewer( + repo, + reviewer=None, + workflow_file=DRAFT_RELEASE_WORKFLOW_FILE, + token=None, + session=None, +): + """ + Determine which GitHub user to request for human review. + + Resolution order: + + 1. Use *reviewer* if explicitly provided. + 2. Otherwise, use the actor that triggered the most recent + *workflow_file* run (typically ``draft-release.yml``). + 3. Otherwise, fall back to the repository's default (catch-all ``*``) + ``CODEOWNERS`` entry. + + Args: + repo (str): Repository full name (e.g. ``"CCBR/actions"``). + reviewer (str, optional): Explicit reviewer override. + workflow_file (str): Workflow filename used to resolve the triggering + actor. + token (str, optional): GitHub API token. + session: Requests-compatible session object for dependency injection. + + Returns: + str | None: GitHub username to request as reviewer, or ``None`` if + none could be determined. + """ + actor = ( + None + if reviewer + else get_last_workflow_run_actor( + repo, workflow_file, token=token, session=session + ) + ) + codeowner = None + if not reviewer and not actor: + codeowners_content = get_codeowners_content(repo, token=token, session=session) + owners = match_codeowners(codeowners_content, "") if codeowners_content else [] + codeowner = owners[0] if owners else None + return reviewer or actor or codeowner + + +def _collect_failure_reasons( + is_post_release, release, only_allowed_files, only_valid_bumps, approval_error +): + """ + Build a list of human-readable reasons a post-release PR needs review. + + Args: + is_post_release (bool): Whether the PR matches the post-release + cleanup pattern. + release (dict | None): The matching GitHub release, or ``None``. + only_allowed_files (bool): Whether only allowed files were changed. + only_valid_bumps (bool): Whether all changes were valid version/date + bumps. + approval_error (Exception | None): The error raised while attempting + automatic approval, if any. + + Returns: + list[str]: Human-readable reasons, in the order they should be + reported. + """ + reasons = [] + if not is_post_release: + reasons.append( + "the PR title/sender do not match the expected post-release cleanup pattern" + ) + if release is None: + reasons.append("no matching GitHub release tag could be found for this PR") + if release is not None and not only_allowed_files: + reasons.append( + "only the version, citation, codemeta, changelog, news, and " + "readme files should be changed, but other files were modified" + ) + if release is not None and only_allowed_files and not only_valid_bumps: + reasons.append( + "the changes must be limited to version/date bumps matching the " + "release tag, but other modifications were found" + ) + if approval_error is not None: + reasons.append( + "automatic approval could not be completed because of a GitHub " + f"API error: {approval_error}" + ) + return reasons + + +def _enable_merge_or_comment(repo, pr_number, reviewer, token=None, session=None): + """ + Enable auto-merge, or post a comment explaining why it could not be enabled. + + Args: + repo (str): Repository full name (e.g. ``"CCBR/actions"``). + pr_number (int | str): Pull request number. + reviewer (str, optional): Reviewer to mention in the failure comment. + token (str, optional): GitHub API token. + session: Requests-compatible session object for dependency injection. + """ + try: + enable_auto_merge(repo, pr_number, token=token, session=session) + print("Squash auto-merge enabled successfully") + except (KeyError, requests.exceptions.RequestException, RuntimeError) as exc: + reviewer_mention = f"@{reviewer} " if reviewer else "" + comment = ( + f"{reviewer_mention}CCBR-bot approved this post-release cleanup " + "PR, but could not enable auto-merge because of a GitHub API " + f"error: {exc}" + ) + try: + post_pr_comment(repo, pr_number, comment, token=token, session=session) + print("Posted a comment explaining that auto-merge could not be enabled") + except (requests.exceptions.RequestException, RuntimeError) as comment_exc: + print(f"Could not post the auto-merge failure comment: {comment_exc}") + warnings.warn(f"Could not post auto-merge failure comment: {comment_exc}") + + +def _request_human_review( + repo, pr_number, reviewer, failed_reasons, token=None, session=None +): + """ + Post a comment explaining failed conditions and request a human reviewer. + + Args: + repo (str): Repository full name (e.g. ``"CCBR/actions"``). + pr_number (int | str): Pull request number. + reviewer (str, optional): Explicit reviewer override. + failed_reasons (list[str]): Reasons the PR needs human review, from + [](`~ccbr_actions.post_release_pr._collect_failure_reasons`). + token (str, optional): GitHub API token. + session: Requests-compatible session object for dependency injection. + """ + reasons_text = "\n".join(f"- {r}" for r in failed_reasons) + print(f"Reasons requiring human review: {reasons_text.replace(chr(10), '; ')}") + resolved_reviewer = determine_post_release_reviewer( + repo, reviewer=reviewer, token=token, session=session + ) + reviewer_mention = f"@{resolved_reviewer} " if resolved_reviewer else "" + comment = ( + f"{reviewer_mention}This post-release cleanup PR requires human " + "review. The changes were too complex for CCBR-bot to automatically " + f"approve because the following conditions were not met:\n{reasons_text}" + ) + try: + response = post_pr_comment( + repo, pr_number, comment, token=token, session=session + ) + print(f"Posted human-review comment (HTTP {response.status_code})") + except (requests.exceptions.RequestException, RuntimeError) as exc: + print(f"Could not post human-review comment: {exc}") + warnings.warn(f"Could not post human-review comment: {exc}") + if resolved_reviewer: + try: + response = request_reviewer( + repo, pr_number, resolved_reviewer, token=token, session=session + ) + print( + f"Requested reviewer {resolved_reviewer!r} " + f"(HTTP {response.status_code})" + ) + except (requests.exceptions.RequestException, RuntimeError) as exc: + print(f"Could not request reviewer {resolved_reviewer!r}: {exc}") + warnings.warn(f"Could not request reviewer {resolved_reviewer!r}: {exc}") + else: + print("No reviewer could be resolved") + + +def _review_post_release_pr( + repo, + pr_number, + reviewer, + token, + session, + version_filepath, + citation_filepath, + changelog_filepath, + description_filepath, +): + """ + Evaluate and review a post-release cleanup PR that isn't already approved. + + See [](`~ccbr_actions.post_release_pr.review_post_release_pr`) for the + full policy description. + + Returns: + bool: ``True`` if the PR was automatically approved, ``False`` if + human review was requested. + """ + pr_data = github_api_get( + url=f"{GITHUB_API_URL}/repos/{repo}/pulls/{pr_number}", + token=token, + session=session, + ) + pr_title = pr_data.get("title", "") + pr_sender_type = pr_data.get("user", {}).get("type", "") + head_ref = pr_data.get("head", {}).get("ref", "") + is_post_release = is_post_release_pr(pr_title, pr_sender_type) + release_tag = extract_release_tag(pr_title) + release = ( + get_release_by_tag(repo, release_tag, token=token, session=session) + if release_tag + else None + ) + + pr_files = get_pr_files(repo, pr_number, token=token, session=session) + filenames = [f.get("filename", "") for f in pr_files] + print(f"PR title: {pr_title!r}") + print(f"PR sender type: {pr_sender_type!r}") + print(f"Changed files ({len(filenames)}): {', '.join(filenames) or ''}") + print(f"Matches post-release cleanup PR: {is_post_release}") + print(f"Release tag from title: {release_tag!r}") + print(f"Release found on GitHub: {release is not None}") + + allowed_basenames = _allowed_basenames( + version_filepath, citation_filepath, changelog_filepath, description_filepath + ) + only_allowed_files = check_only_allowed_files_changed(pr_files, allowed_basenames) + only_valid_bumps = False + if only_allowed_files and release is not None: + valid_values = _release_bump_values(release_tag, release) + only_valid_bumps = check_version_date_bumps( + pr_files, valid_values, release_tag.lstrip("v") + ) + print(f"Only allowed files changed: {only_allowed_files}") + print(f"Only version/date bumps found: {only_valid_bumps}") + + eligible = ( + is_post_release + and release is not None + and only_allowed_files + and only_valid_bumps + ) + approval_error = None + was_auto_approved = False + if eligible: + print("Policy result: eligible for automatic approval") + try: + approve_pending_workflow_runs(repo, head_ref, token=token, session=session) + print("Approved pending workflow runs (if any)") + except (KeyError, requests.exceptions.RequestException, RuntimeError) as exc: + print(f"Could not approve pending workflow runs: {exc}") + warnings.warn(f"Could not approve pending workflow runs: {exc}") + try: + response = approve_pr(repo, pr_number, token=token, session=session) + was_auto_approved = True + print(f"Approval submitted successfully (HTTP {response.status_code})") + except (KeyError, requests.exceptions.RequestException, RuntimeError) as exc: + approval_error = exc + print(f"Approval failed: {exc}") + if was_auto_approved: + _enable_merge_or_comment( + repo, pr_number, reviewer, token=token, session=session + ) + + if not was_auto_approved: + print("Policy result: automatic approval not performed") + failed_reasons = _collect_failure_reasons( + is_post_release, + release, + only_allowed_files, + only_valid_bumps, + approval_error, + ) + _request_human_review( + repo, pr_number, reviewer, failed_reasons, token=token, session=session + ) + print("Result: human review requested") + else: + print("Result: automatically approved") + + return was_auto_approved + + +def review_post_release_pr( + repo, + pr_number, + reviewer=None, + token=None, + session=None, + force_review=False, + version_filepath="VERSION", + citation_filepath="CITATION.cff", + changelog_filepath="CHANGELOG.md", + description_filepath="DESCRIPTION", +): + """ + Evaluate and review a post-release cleanup pull request. + + Checks whether the PR satisfies three conditions: + + - **Condition 1** – the PR title matches the post-release cleanup + pattern (``chore: post-release cleanup for ``) and ** + corresponds to an actual GitHub release. + - **Condition 2** – only the version file, citation file, ``codemeta.json``, + changelog file, news file, and/or readme files were changed. + - **Condition 3** – every change in those files is a version/date bump + matching the release tag (or a re-rendered citation snippet in a + readme file, which is validated the same way). + + When all conditions are met the function approves any pending workflow + runs on the PR's head branch, approves the PR, and attempts to enable + squash auto-merge. If auto-merge cannot be enabled, it leaves a comment + with the GitHub API error. Otherwise it posts a comment explaining why + the PR needs manual review and requests a human reviewer, resolved via + [](`~ccbr_actions.post_release_pr.determine_post_release_reviewer`). + + If the PR already has an APPROVED review, no new review is submitted and + the function returns ``True`` immediately unless *force_review* is true. + + Args: + repo (str): Repository full name (e.g. ``"CCBR/actions"``). + pr_number (int | str): Pull request number. + reviewer (str, optional): GitHub username to request when human + review is required. If omitted, a reviewer is resolved + automatically. + token (str, optional): GitHub API token. + session: Requests-compatible session object for dependency injection. + force_review (bool, optional): Re-submit the review and reviewer + request even when the PR already has an approval. Defaults to + ``False``. + version_filepath (str): Path to the version file. Defaults to + ``"VERSION"``. + citation_filepath (str): Path to the citation file. Defaults to + ``"CITATION.cff"``. + changelog_filepath (str): Path to the changelog file. Defaults to + ``"CHANGELOG.md"``. + description_filepath (str): Path to the R DESCRIPTION file, used when + an R package's version file is the DESCRIPTION file. Defaults to + ``"DESCRIPTION"``. + + Returns: + bool: ``True`` if the PR was already approved or was automatically + approved, ``False`` if human review was requested. + """ + print(f"Reviewing post-release cleanup PR {repo}#{pr_number}") + already_approved = not force_review and is_pr_approved( + repo, pr_number, token=token, session=session + ) + if already_approved: + print("Result: PR already has an APPROVED review; no action was taken") + result = True + else: + result = _review_post_release_pr( + repo, + pr_number, + reviewer, + token, + session, + version_filepath, + citation_filepath, + changelog_filepath, + description_filepath, + ) + return result diff --git a/src/ccbr_actions/pr_review.py b/src/ccbr_actions/pr_review.py index 1def16f7..4914778e 100644 --- a/src/ccbr_actions/pr_review.py +++ b/src/ccbr_actions/pr_review.py @@ -183,6 +183,68 @@ def enable_auto_merge(repo, pr_number, token=None, session=None): ) +def approve_pending_workflow_runs(repo, branch, token=None, session=None): + """ + Approve workflow runs awaiting approval for a branch. + + Some repositories require manual approval before workflow runs triggered + by a pull request are allowed to execute. This lists runs for *branch* + with status ``action_required`` and approves each of them. + + Args: + repo (str): Repository full name (e.g. ``"CCBR/actions"``). + branch (str): Branch name (typically a pull request's head ref). + token (str, optional): GitHub API token. + session: Requests-compatible session object for dependency injection. + + Returns: + list[int]: IDs of the workflow runs that were approved. + """ + url = f"{GITHUB_API_URL}/repos/{repo}/actions/runs" + data = github_api_get( + url=url, + token=token, + session=session, + params={"branch": branch, "status": "action_required"}, + ) + approved_run_ids = [] + for run in data.get("workflow_runs", []): + run_id = run["id"] + approve_url = f"{GITHUB_API_URL}/repos/{repo}/actions/runs/{run_id}/approve" + response = github_api_post(url=approve_url, token=token, session=session) + response.raise_for_status() + approved_run_ids.append(run_id) + return approved_run_ids + + +def get_last_workflow_run_actor(repo, workflow_file, token=None, session=None): + """ + Return the triggering actor's login for the most recent run of a workflow. + + Args: + repo (str): Repository full name (e.g. ``"CCBR/actions"``). + workflow_file (str): Workflow filename (e.g. ``"draft-release.yml"``). + token (str, optional): GitHub API token. + session: Requests-compatible session object for dependency injection. + + Returns: + str | None: Login of the actor who triggered the most recent run, or + ``None`` if no runs were found or the request failed. + """ + url = f"{GITHUB_API_URL}/repos/{repo}/actions/workflows/{workflow_file}/runs" + try: + data = github_api_get( + url=url, token=token, session=session, params={"per_page": 1} + ) + except requests.exceptions.RequestException: + data = {} + runs = data.get("workflow_runs", []) + actor = ( + (runs[0].get("triggering_actor") or runs[0].get("actor") or {}) if runs else {} + ) + return actor.get("login") + + def request_reviewer(repo, pr_number, reviewer, token=None, session=None): """ Request a reviewer (user or team) on a pull request. diff --git a/tests/test_action_test_coverage.py b/tests/test_action_test_coverage.py index 855cf321..0d12ceae 100644 --- a/tests/test_action_test_coverage.py +++ b/tests/test_action_test_coverage.py @@ -20,6 +20,7 @@ UNTESTED_ACTIONS = { # Pure API callers with no workspace dependency (see plan Decisions). "review-pre-commit-pr": "pure API caller; no workspace dependency", + "review-post-release-pr": "pure API caller; no workspace dependency", "copy-ruleset": "pure API caller; no workspace dependency", "user-projects": "pure API caller; no workspace dependency", "add-issue-label-list": "pure API caller; no workspace dependency", diff --git a/tests/test_post_release_pr.py b/tests/test_post_release_pr.py new file mode 100644 index 00000000..899fb720 --- /dev/null +++ b/tests/test_post_release_pr.py @@ -0,0 +1,712 @@ +""" +Tests for ccbr_actions.post_release_pr module. +""" + +import pytest +import requests as requests_lib + +from ccbr_actions.post_release_pr import ( + POST_RELEASE_PR_TITLE_PATTERN, + _allowed_basenames, + _is_valid_bump_line, + _is_valid_insertion, + _release_bump_values, + _split_tokens, + check_only_allowed_files_changed, + check_patch_is_version_bump, + check_version_date_bumps, + determine_post_release_reviewer, + extract_release_tag, + get_release_by_tag, + is_allowed_filename, + is_post_release_pr, + review_post_release_pr, +) + +# --------------------------------------------------------------------------- +# Mock helpers +# --------------------------------------------------------------------------- + + +class MockResponse: + """Minimal requests.Response stand-in.""" + + def __init__(self, payload, status_code=200): + self.payload = payload + self.status_code = status_code + + def json(self): + return self.payload + + def raise_for_status(self): + if self.status_code >= 400: + raise requests_lib.exceptions.HTTPError(f"HTTP {self.status_code}") + + +class MockSession: + """Records calls and returns pre-configured payloads keyed by URL.""" + + def __init__(self, payloads=None, post_status=200): + self.payloads = payloads or {} + self.post_status = post_status + self.calls = [] + + def request(self, method, url, headers=None, **kwargs): + self.calls.append((method, url, kwargs)) + status = self.post_status if method == "POST" else 200 + default = {"data": {}} if method == "POST" else {} + return MockResponse(self.payloads.get(url, default), status) + + +class _RaisingSession(MockSession): + """Always raises a RequestException on GET.""" + + def request(self, method, url, headers=None, **kwargs): + self.calls.append((method, url, kwargs)) + raise requests_lib.exceptions.ConnectionError("boom") + + +# --------------------------------------------------------------------------- +# Fixture patches, based on CCBR/Tools#229 +# --------------------------------------------------------------------------- + +RELEASE_TAG = "v0.7.1" +PR_TITLE = f"chore: post-release cleanup for {RELEASE_TAG}" +RELEASE = { + "tag_name": RELEASE_TAG, + "published_at": "2026-09-17T00:00:00Z", + "created_at": "2026-09-17T00:00:00Z", +} + +CHANGELOG_PATCH = """\ +@@ -1,5 +1,7 @@ + ## Tools development version + ++## Tools 0.7.1 ++ + - minor documentation improvements. (#197, #198, @kelly-sovacool) +""" + +README_PATCH = """\ +@@ -181,7 +181,7 @@ guidelines](https://CCBR.github.io/Tools/CONTRIBUTING). + Please cite this software if you use it in a publication: + + > Sovacool K., Koparde V., Kuhn S., Tandon M., and Huse S. (2026). CCBR +-> Tools: Utilities for CCBR Bioinformatics Software (version v0.7.0). ++> Tools: Utilities for CCBR Bioinformatics Software (version v0.7.1). + > DOI: 10.5281/zenodo.13377166 URL: https://ccbr.github.io/Tools/ + + ### Bibtex entry +@@ -190,7 +190,7 @@ Please cite this software if you use it in a publication: + @misc{YourReferenceHere, + author = {Sovacool, Kelly and Koparde, Vishal and Kuhn, Skyler and Tandon, Mayank and Huse, Susan}, + doi = {10.5281/zenodo.13377166}, +- month = {6}, ++ month = {9}, + title = {CCBR Tools: Utilities for CCBR Bioinformatics Software}, + url = {https://ccbr.github.io/Tools/}, + year = {2026} +""" + +CITATION_PATCH = """\ +@@ -29,5 +29,5 @@ identifiers: + type: doi + value: 10.5281/zenodo.13377166 + doi: 10.5281/zenodo.13377166 +-version: v0.7.0 +-date-released: "2026-06-10" ++version: v0.7.1 ++date-released: "2026-09-17" +""" + +VERSION_PATCH = """\ +@@ -1 +1 @@ +-0.7.0-dev ++0.7.1-dev +""" + +# A non-version change slipped into the changelog (invalid) +INVALID_CHANGELOG_PATCH = """\ +@@ -1,5 +1,5 @@ + ## Tools development version + +-- minor documentation improvements. (#197, #198, @kelly-sovacool) ++- MAJOR documentation improvements. (#197, #198, @kelly-sovacool) +""" + + +def _pr_files(): + return [ + {"filename": "CHANGELOG.md", "patch": CHANGELOG_PATCH}, + {"filename": "README.md", "patch": README_PATCH}, + {"filename": "src/ccbr_tools/CITATION.cff", "patch": CITATION_PATCH}, + {"filename": "src/ccbr_tools/VERSION", "patch": VERSION_PATCH}, + ] + + +# --------------------------------------------------------------------------- +# is_post_release_pr / extract_release_tag +# --------------------------------------------------------------------------- + + +def test_post_release_pr_title_pattern_matches_expected_title(): + assert POST_RELEASE_PR_TITLE_PATTERN.match(PR_TITLE) + + +def test_is_post_release_pr_returns_true_for_matching_pr(): + assert is_post_release_pr(PR_TITLE, "Bot") is True + + +def test_is_post_release_pr_returns_false_for_wrong_title(): + assert is_post_release_pr("chore: bump deps", "Bot") is False + + +def test_is_post_release_pr_returns_false_for_non_bot_sender(): + assert is_post_release_pr(PR_TITLE, "User") is False + + +def test_extract_release_tag_returns_tag(): + assert extract_release_tag(PR_TITLE) == RELEASE_TAG + + +def test_extract_release_tag_returns_none_for_wrong_title(): + assert extract_release_tag("chore: bump deps") is None + + +# --------------------------------------------------------------------------- +# get_release_by_tag +# --------------------------------------------------------------------------- + + +def test_get_release_by_tag_returns_release_when_found(): + session = MockSession( + {"https://api.github.com/repos/CCBR/Tools/releases/tags/v0.7.1": RELEASE} + ) + assert get_release_by_tag("CCBR/Tools", "v0.7.1", session=session) == RELEASE + + +def test_get_release_by_tag_returns_none_when_missing(): + class NotFoundSession(MockSession): + def request(self, method, url, headers=None, **kwargs): + self.calls.append((method, url, kwargs)) + raise requests_lib.exceptions.HTTPError("404") + + assert get_release_by_tag("CCBR/Tools", "v9.9.9", session=NotFoundSession()) is None + + +# --------------------------------------------------------------------------- +# _allowed_basenames / is_allowed_filename / check_only_allowed_files_changed +# --------------------------------------------------------------------------- + + +def test_allowed_basenames_includes_defaults(): + basenames = _allowed_basenames( + "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" + ) + assert basenames == { + "VERSION", + "CITATION.cff", + "CHANGELOG.md", + "DESCRIPTION", + "codemeta.json", + "NEWS.md", + "NEWS", + } + + +def test_is_allowed_filename_matches_basename_regardless_of_path(): + basenames = _allowed_basenames( + "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" + ) + assert is_allowed_filename("src/ccbr_tools/VERSION", basenames) is True + + +def test_is_allowed_filename_matches_readme_case_insensitively(): + basenames = _allowed_basenames( + "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" + ) + assert is_allowed_filename("Readme.qmd", basenames) is True + + +def test_is_allowed_filename_returns_false_for_disallowed_file(): + basenames = _allowed_basenames( + "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" + ) + assert is_allowed_filename("src/main.py", basenames) is False + + +def test_check_only_allowed_files_changed_returns_true_for_pr_files(): + basenames = _allowed_basenames( + "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" + ) + assert check_only_allowed_files_changed(_pr_files(), basenames) is True + + +def test_check_only_allowed_files_changed_returns_false_when_extra_file(): + basenames = _allowed_basenames( + "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" + ) + pr_files = _pr_files() + [{"filename": "src/main.py"}] + assert check_only_allowed_files_changed(pr_files, basenames) is False + + +def test_check_only_allowed_files_changed_returns_false_when_empty(): + basenames = _allowed_basenames( + "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" + ) + assert check_only_allowed_files_changed([], basenames) is False + + +# --------------------------------------------------------------------------- +# _release_bump_values +# --------------------------------------------------------------------------- + + +def test_release_bump_values_includes_version_and_date_tokens(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert "v0.7.1" in values + assert "0.7.1" in values + assert "0.7.1-dev" in values + assert "2026-09-17" in values + assert "2026" in values + assert "9" in values + assert "17" in values + + +# --------------------------------------------------------------------------- +# _split_tokens +# --------------------------------------------------------------------------- + + +def test_split_tokens_separates_text_and_numeric_tokens(): + text, tokens = _split_tokens("version: v0.7.0") + assert tokens == ["0.7.0"] + assert text == ["version: v", ""] + + +# --------------------------------------------------------------------------- +# _is_valid_bump_line +# --------------------------------------------------------------------------- + + +def test_is_valid_bump_line_accepts_version_bump(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert _is_valid_bump_line("version: v0.7.0", "version: v0.7.1", values) is True + + +def test_is_valid_bump_line_rejects_non_matching_version(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert _is_valid_bump_line("version: v0.7.0", "version: v9.9.9", values) is False + + +def test_is_valid_bump_line_rejects_changed_surrounding_text(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert _is_valid_bump_line("version: v0.7.0", "ver: v0.7.1", values) is False + + +def test_is_valid_bump_line_rejects_mismatched_token_count(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert _is_valid_bump_line("v0.7.0", "v0.7.1 v0.7.1", values) is False + + +# --------------------------------------------------------------------------- +# _is_valid_insertion +# --------------------------------------------------------------------------- + + +def test_is_valid_insertion_accepts_blank_line(): + assert _is_valid_insertion("", "0.7.1") is True + assert _is_valid_insertion(" ", "0.7.1") is True + + +def test_is_valid_insertion_accepts_heading_with_release_version(): + assert _is_valid_insertion("## Tools 0.7.1", "0.7.1") is True + + +def test_is_valid_insertion_rejects_unrelated_line(): + assert _is_valid_insertion("- a new unrelated bullet point", "0.7.1") is False + + +# --------------------------------------------------------------------------- +# check_patch_is_version_bump +# --------------------------------------------------------------------------- + + +def test_check_patch_is_version_bump_accepts_changelog_heading_insertion(): + assert check_patch_is_version_bump(CHANGELOG_PATCH, {"0.7.1"}, "0.7.1") is True + + +def test_check_patch_is_version_bump_accepts_readme_citation_rerender(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert check_patch_is_version_bump(README_PATCH, values, "0.7.1") is True + + +def test_check_patch_is_version_bump_accepts_citation_cff_bump(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert check_patch_is_version_bump(CITATION_PATCH, values, "0.7.1") is True + + +def test_check_patch_is_version_bump_accepts_version_file_bump(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert check_patch_is_version_bump(VERSION_PATCH, values, "0.7.1") is True + + +def test_check_patch_is_version_bump_rejects_non_version_change(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert ( + check_patch_is_version_bump(INVALID_CHANGELOG_PATCH, values, "0.7.1") is False + ) + + +def test_check_patch_is_version_bump_rejects_version_not_matching_release(): + # Looks like a version bump, but doesn't match the fetched release's version. + patch = "@@ -1 +1 @@\n-0.7.0-dev\n+0.9.9-dev\n" + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert check_patch_is_version_bump(patch, values, "0.7.1") is False + + +def test_check_patch_is_version_bump_returns_true_for_empty_patch(): + assert check_patch_is_version_bump("", {"0.7.1"}, "0.7.1") is True + + +# --------------------------------------------------------------------------- +# check_version_date_bumps +# --------------------------------------------------------------------------- + + +def test_check_version_date_bumps_returns_true_for_valid_pr_files(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert check_version_date_bumps(_pr_files(), values, "0.7.1") is True + + +def test_check_version_date_bumps_returns_false_when_one_file_invalid(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + pr_files = _pr_files() + pr_files[0] = {"filename": "CHANGELOG.md", "patch": INVALID_CHANGELOG_PATCH} + assert check_version_date_bumps(pr_files, values, "0.7.1") is False + + +def test_check_version_date_bumps_returns_false_when_patch_missing(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + pr_files = [{"filename": "CHANGELOG.md"}] + assert check_version_date_bumps(pr_files, values, "0.7.1") is False + + +def test_check_version_date_bumps_returns_false_for_empty_files(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert check_version_date_bumps([], values, "0.7.1") is False + + +# --------------------------------------------------------------------------- +# determine_post_release_reviewer +# --------------------------------------------------------------------------- + + +def test_determine_post_release_reviewer_prefers_explicit_reviewer(): + session = MockSession({}) + result = determine_post_release_reviewer( + "CCBR/Tools", reviewer="explicit-user", token="tok", session=session + ) + assert result == "explicit-user" + assert session.calls == [] + + +def test_determine_post_release_reviewer_uses_last_draft_release_actor(): + runs_url = ( + "https://api.github.com/repos/CCBR/Tools/actions/workflows/" + "draft-release.yml/runs" + ) + session = MockSession( + {runs_url: {"workflow_runs": [{"triggering_actor": {"login": "actor-user"}}]}} + ) + result = determine_post_release_reviewer("CCBR/Tools", token="tok", session=session) + assert result == "actor-user" + + +def test_determine_post_release_reviewer_falls_back_to_default_codeowner(): + import base64 + + runs_url = ( + "https://api.github.com/repos/CCBR/Tools/actions/workflows/" + "draft-release.yml/runs" + ) + content = "* @default-owner\n" + encoded = base64.b64encode(content.encode("utf-8")).decode("ascii") + session = MockSession( + { + runs_url: {"workflow_runs": []}, + "https://api.github.com/repos/CCBR/Tools/contents/CODEOWNERS": { + "content": encoded + }, + } + ) + result = determine_post_release_reviewer("CCBR/Tools", token="tok", session=session) + assert result == "default-owner" + + +def test_determine_post_release_reviewer_returns_none_when_nothing_resolves(): + runs_url = ( + "https://api.github.com/repos/CCBR/Tools/actions/workflows/" + "draft-release.yml/runs" + ) + session = MockSession({runs_url: {"workflow_runs": []}}) + result = determine_post_release_reviewer("CCBR/Tools", token="tok", session=session) + assert result is None + + +# --------------------------------------------------------------------------- +# review_post_release_pr +# --------------------------------------------------------------------------- + + +def _make_review_session( + *, + pr_files=None, + pr_node_payload=None, + graphql_payload=None, + release_payload=None, + existing_reviews=None, + action_required_runs=None, +): + """Build a MockSession suitable for review_post_release_pr tests.""" + if pr_files is None: + pr_files = _pr_files() + if pr_node_payload is None: + pr_node_payload = { + "node_id": "PR_NODE_9", + "title": PR_TITLE, + "user": {"type": "Bot"}, + "head": {"ref": "release/v0.7.1"}, + } + if graphql_payload is None: + graphql_payload = { + "data": { + "enablePullRequestAutoMerge": { + "pullRequest": {"autoMergeRequest": {"enabledAt": "2026-09-17"}} + } + } + } + if release_payload is None: + release_payload = RELEASE + + pr_files_url = "https://api.github.com/repos/CCBR/Tools/pulls/9/files" + pr_node_url = "https://api.github.com/repos/CCBR/Tools/pulls/9" + graphql_url = "https://api.github.com/graphql" + reviews_url = "https://api.github.com/repos/CCBR/Tools/pulls/9/reviews" + reviewers_url = ( + "https://api.github.com/repos/CCBR/Tools/pulls/9/requested_reviewers" + ) + release_url = f"https://api.github.com/repos/CCBR/Tools/releases/tags/{RELEASE_TAG}" + runs_url = "https://api.github.com/repos/CCBR/Tools/actions/runs" + draft_runs_url = ( + "https://api.github.com/repos/CCBR/Tools/actions/workflows/" + "draft-release.yml/runs" + ) + + payloads = { + pr_files_url: pr_files, + pr_node_url: pr_node_payload, + graphql_url: graphql_payload, + reviews_url: existing_reviews if existing_reviews is not None else [], + reviewers_url: {}, + release_url: release_payload, + runs_url: {"workflow_runs": action_required_runs or []}, + draft_runs_url: {"workflow_runs": []}, + } + return MockSession(payloads) + + +def test_review_post_release_pr_approves_when_conditions_met(): + session = _make_review_session() + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is True + posted_urls = [c[1] for c in session.calls if c[0] == "POST"] + assert any("reviews" in u for u in posted_urls) + assert any("graphql" in u for u in posted_urls) + review_bodies = [ + c[2]["json"] for c in session.calls if c[0] == "POST" and "reviews" in c[1] + ] + assert any(body.get("event") == "APPROVE" for body in review_bodies) + + +def test_review_post_release_pr_approves_pending_workflow_runs(): + session = _make_review_session(action_required_runs=[{"id": 555}]) + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is True + approve_run_urls = [ + c[1] + for c in session.calls + if c[0] == "POST" and "actions/runs/555/approve" in c[1] + ] + assert approve_run_urls + + +def test_review_post_release_pr_skips_when_current_review_is_approved(): + session = _make_review_session( + existing_reviews=[{"user": {"login": "ccbr-bot"}, "state": "APPROVED"}] + ) + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is True + posted_urls = [c[1] for c in session.calls if c[0] == "POST"] + assert not posted_urls + + +def test_review_post_release_pr_reviews_again_when_force_review_is_enabled(): + session = _make_review_session( + existing_reviews=[{"user": {"login": "ccbr-bot"}, "state": "APPROVED"}] + ) + result = review_post_release_pr( + "CCBR/Tools", 9, force_review=True, token="tok", session=session + ) + assert result is True + review_calls = [c for c in session.calls if c[0] == "POST" and "reviews" in c[1]] + assert any(c[2]["json"].get("event") == "APPROVE" for c in review_calls) + + +def test_review_post_release_pr_requests_human_review_when_extra_file_changed(): + pr_files = _pr_files() + [{"filename": "src/main.py", "patch": "@@ -1 +1 @@\n"}] + session = _make_review_session(pr_files=pr_files) + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is False + posted_urls = [c[1] for c in session.calls if c[0] == "POST"] + review_bodies = [ + c[2]["json"] for c in session.calls if c[0] == "POST" and "reviews" in c[1] + ] + assert not any(body.get("event") == "APPROVE" for body in review_bodies) + assert any("issues/9/comments" in u for u in posted_urls) + + +def test_review_post_release_pr_requests_human_review_when_non_bump_change(): + pr_files = _pr_files() + pr_files[0] = {"filename": "CHANGELOG.md", "patch": INVALID_CHANGELOG_PATCH} + session = _make_review_session(pr_files=pr_files) + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is False + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/9/comments" in c[1] + ] + assert comment_calls + assert "requires human review" in comment_calls[0][2]["json"]["body"] + + +def test_review_post_release_pr_requests_human_review_when_release_not_found(): + class NoReleaseSession(MockSession): + def request(self, method, url, headers=None, **kwargs): + self.calls.append((method, url, kwargs)) + if "releases/tags/" in url: + raise requests_lib.exceptions.HTTPError("404") + status = self.post_status if method == "POST" else 200 + default = {"data": {}} if method == "POST" else {} + return MockResponse(self.payloads.get(url, default), status) + + session = _make_review_session() + session.__class__ = NoReleaseSession + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is False + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/9/comments" in c[1] + ] + assert comment_calls + assert "no matching GitHub release tag" in comment_calls[0][2]["json"]["body"] + + +def test_review_post_release_pr_requests_human_review_when_title_or_sender_mismatch(): + session = _make_review_session( + pr_node_payload={ + "node_id": "PR_NODE_9", + "title": "chore: bump deps", + "user": {"type": "User"}, + "head": {"ref": "some-branch"}, + } + ) + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is False + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/9/comments" in c[1] + ] + assert comment_calls + assert "post-release cleanup pattern" in comment_calls[0][2]["json"]["body"] + + +def test_review_post_release_pr_uses_resolved_reviewer_in_comment(): + runs_url = ( + "https://api.github.com/repos/CCBR/Tools/actions/workflows/" + "draft-release.yml/runs" + ) + session = _make_review_session( + pr_node_payload={ + "node_id": "PR_NODE_9", + "title": "chore: bump deps", + "user": {"type": "User"}, + "head": {"ref": "some-branch"}, + } + ) + session.payloads[runs_url] = { + "workflow_runs": [{"triggering_actor": {"login": "kelly-sovacool"}}] + } + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is False + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/9/comments" in c[1] + ] + assert "@kelly-sovacool" in comment_calls[0][2]["json"]["body"] + reviewer_calls = [ + c for c in session.calls if c[0] == "POST" and "requested_reviewers" in c[1] + ] + assert reviewer_calls + assert reviewer_calls[0][2]["json"]["reviewers"] == ["kelly-sovacool"] + + +def test_review_post_release_pr_keeps_approval_when_auto_merge_api_fails(): + session = _make_review_session( + graphql_payload={ + "errors": [{"message": "Resource not accessible by integration"}] + } + ) + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is True + review_calls = [c for c in session.calls if c[0] == "POST" and "reviews" in c[1]] + review_bodies = [c[2]["json"] for c in review_calls] + assert any(body.get("event") == "APPROVE" for body in review_bodies) + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/9/comments" in c[1] + ] + assert comment_calls + assert "GraphQL errors" in comment_calls[0][2]["json"]["body"] + + +def test_review_post_release_pr_comments_when_approval_fails(monkeypatch): + session = _make_review_session() + + def _fail_approval(*args, **kwargs): + raise RuntimeError("approval is not allowed") + + monkeypatch.setattr("ccbr_actions.post_release_pr.approve_pr", _fail_approval) + + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + + assert result is False + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/9/comments" in c[1] + ] + assert comment_calls + comment_body = comment_calls[0][2]["json"]["body"] + assert "automatic approval could not be completed" in comment_body + + +def test_review_post_release_pr_warns_when_reviewer_request_fails(monkeypatch): + pr_files = _pr_files() + [{"filename": "src/main.py", "patch": "@@ -1 +1 @@\n"}] + session = _make_review_session(pr_files=pr_files) + + def _fail_request(*args, **kwargs): + raise RuntimeError("reviewer not allowed on PR") + + monkeypatch.setattr("ccbr_actions.post_release_pr.request_reviewer", _fail_request) + monkeypatch.setattr( + "ccbr_actions.post_release_pr.determine_post_release_reviewer", + lambda *args, **kwargs: "dave", + ) + + with pytest.warns(UserWarning, match="Could not request reviewer"): + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is False diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 1e7cd8bb..ee9dc27b 100644 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -9,11 +9,13 @@ from ccbr_actions.pr_review import ( _codeowners_pattern_matches, + approve_pending_workflow_runs, approve_pr, determine_reviewer, enable_auto_merge, get_codeowners_content, get_last_human_committer, + get_last_workflow_run_actor, get_pr_files, is_pr_approved, match_codeowners, @@ -469,4 +471,99 @@ def test_determine_reviewer_returns_none_without_reviewer_or_path(): session = MockSession({}) result = determine_reviewer("CCBR/actions", token="tok", session=session) assert result is None - assert session.calls == [] + + +# --------------------------------------------------------------------------- +# approve_pending_workflow_runs +# --------------------------------------------------------------------------- + + +def test_approve_pending_workflow_runs_approves_each_pending_run(): + runs_url = "https://api.github.com/repos/CCBR/actions/actions/runs" + session = MockSession({runs_url: {"workflow_runs": [{"id": 111}, {"id": 222}]}}) + result = approve_pending_workflow_runs( + "CCBR/actions", "release/v1.0.0", token="tok", session=session + ) + assert result == [111, 222] + approve_urls = [c[1] for c in session.calls if c[0] == "POST"] + assert ( + "https://api.github.com/repos/CCBR/actions/actions/runs/111/approve" + in approve_urls + ) + assert ( + "https://api.github.com/repos/CCBR/actions/actions/runs/222/approve" + in approve_urls + ) + get_call = next(c for c in session.calls if c[0] == "GET") + assert get_call[2]["params"] == { + "branch": "release/v1.0.0", + "status": "action_required", + } + + +def test_approve_pending_workflow_runs_returns_empty_list_when_none_pending(): + runs_url = "https://api.github.com/repos/CCBR/actions/actions/runs" + session = MockSession({runs_url: {"workflow_runs": []}}) + result = approve_pending_workflow_runs( + "CCBR/actions", "release/v1.0.0", token="tok", session=session + ) + assert result == [] + + +# --------------------------------------------------------------------------- +# get_last_workflow_run_actor +# --------------------------------------------------------------------------- + + +def test_get_last_workflow_run_actor_returns_triggering_actor_login(): + runs_url = ( + "https://api.github.com/repos/CCBR/actions/actions/workflows/" + "draft-release.yml/runs" + ) + session = MockSession( + { + runs_url: { + "workflow_runs": [ + {"triggering_actor": {"login": "kelly-sovacool"}}, + ] + } + } + ) + result = get_last_workflow_run_actor( + "CCBR/actions", "draft-release.yml", token="tok", session=session + ) + assert result == "kelly-sovacool" + + +def test_get_last_workflow_run_actor_falls_back_to_actor_field(): + runs_url = ( + "https://api.github.com/repos/CCBR/actions/actions/workflows/" + "draft-release.yml/runs" + ) + session = MockSession( + {runs_url: {"workflow_runs": [{"actor": {"login": "a-human"}}]}} + ) + result = get_last_workflow_run_actor( + "CCBR/actions", "draft-release.yml", token="tok", session=session + ) + assert result == "a-human" + + +def test_get_last_workflow_run_actor_returns_none_when_no_runs(): + runs_url = ( + "https://api.github.com/repos/CCBR/actions/actions/workflows/" + "draft-release.yml/runs" + ) + session = MockSession({runs_url: {"workflow_runs": []}}) + result = get_last_workflow_run_actor( + "CCBR/actions", "draft-release.yml", token="tok", session=session + ) + assert result is None + + +def test_get_last_workflow_run_actor_returns_none_on_request_error(): + session = _RaisingSession({}) + result = get_last_workflow_run_actor( + "CCBR/actions", "draft-release.yml", token="tok", session=session + ) + assert result is None From fec2396db3372d3d3bb00c52daef0c8600a9c3aa Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Fri, 25 Sep 2026 12:48:31 -0400 Subject: [PATCH 06/23] docs: update CHANGELOG.md for #227 _commit message is ai-generated_ --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0215909..b0a1590d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## actions development version +- New action: `review-post-release-pr` to automatically review post-release cleanup PRs and approve them (approving pending workflow runs and enabling auto-merge) when only version/date bumps matching an actual release tag are present, or request a human reviewer otherwise. (#227, @kelly-sovacool, @copilot) - Fix `review-pre-commit-pr` to post a comment instead of requesting changes when a pre-commit.ci autoupdate PR needs human review. (#226, @kelly-sovacool, @copilot) ## actions 0.8.0 From 57e43dbab31411cf906afca9e4040590ed077d37 Mon Sep 17 00:00:00 2001 From: CCBR-bot <258092125+ccbr-bot@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:02:17 +0000 Subject: [PATCH 07/23] =?UTF-8?q?ci:=20=F0=9F=A4=96=20render=20readme=20&?= =?UTF-8?q?=20format=20everything=20with=20pre-commit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- README.md | 4 ++++ review-post-release-pr/README.md | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 31d39413..a6253a77 100644 --- a/README.md +++ b/README.md @@ -72,6 +72,10 @@ Custom actions used in our github workflows. using mkdocs + mike - [post-release](post-release) - Post-release cleanup chores, intended to be triggered by publishing a release +- [review-post-release-pr](review-post-release-pr) - Review a + post-release cleanup PR: approve pending workflow runs, approve, and + enable auto-merge when only version/date bumps matching a real release + tag are present, otherwise request a human reviewer - [review-pre-commit-pr](review-pre-commit-pr) - Review a pre-commit.ci autoupdate PR: approve and attempt auto-merge when only rev: versions changed (or a rev alias tag points at the same commit), otherwise diff --git a/review-post-release-pr/README.md b/review-post-release-pr/README.md index acdc9282..c0895e8d 100644 --- a/review-post-release-pr/README.md +++ b/review-post-release-pr/README.md @@ -6,7 +6,7 @@ version/date bumps matching a real release tag are present, otherwise request a human reviewer This action automates the review of post-release cleanup pull requests -opened by the [post-release](./post-release) action, with titles of the +opened by the [post-release](/post-release) action, with titles of the form `chore: post-release cleanup for `. The action verifies three conditions: @@ -92,7 +92,7 @@ jobs: pr-number: ${{ github.event.pull_request.number }} ``` -See also [pre-review-pr.yml](./examples/pre-review-pr.yml). +See also [pre-review-pr.yml](/examples/pre-review-pr.yml). ## Inputs From 450042080be3335dde8b5dd9ee2a8ab3969e8b9e Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Fri, 25 Sep 2026 13:07:15 -0400 Subject: [PATCH 08/23] test: cover remaining branches in post_release_pr module _commit message is ai-generated_ --- tests/test_post_release_pr.py | 50 +++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/tests/test_post_release_pr.py b/tests/test_post_release_pr.py index 899fb720..0e061e9c 100644 --- a/tests/test_post_release_pr.py +++ b/tests/test_post_release_pr.py @@ -369,6 +369,11 @@ def test_check_patch_is_version_bump_returns_true_for_empty_patch(): assert check_patch_is_version_bump("", {"0.7.1"}, "0.7.1") is True +def test_check_patch_is_version_bump_ignores_file_header_lines(): + patch = "--- a/VERSION\n+++ b/VERSION\n@@ -1 +1 @@\n-0.7.0-dev\n+0.7.1-dev\n" + assert check_patch_is_version_bump(patch, {"0.7.1", "0.7.1-dev"}, "0.7.1") is True + + # --------------------------------------------------------------------------- # check_version_date_bumps # --------------------------------------------------------------------------- @@ -710,3 +715,48 @@ def _fail_request(*args, **kwargs): with pytest.warns(UserWarning, match="Could not request reviewer"): result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) assert result is False + + +def test_review_post_release_pr_warns_when_human_review_comment_fails(monkeypatch): + pr_files = _pr_files() + [{"filename": "src/main.py", "patch": "@@ -1 +1 @@\n"}] + session = _make_review_session(pr_files=pr_files) + + def _fail_comment(*args, **kwargs): + raise RuntimeError("comment is not allowed") + + monkeypatch.setattr("ccbr_actions.post_release_pr.post_pr_comment", _fail_comment) + + with pytest.warns(UserWarning, match="Could not post human-review comment"): + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is False + + +def test_review_post_release_pr_warns_when_auto_merge_comment_fails(monkeypatch): + session = _make_review_session( + graphql_payload={"errors": [{"message": "auto-merge unavailable"}]} + ) + + def _fail_comment(*args, **kwargs): + raise RuntimeError("comment is not allowed") + + monkeypatch.setattr("ccbr_actions.post_release_pr.post_pr_comment", _fail_comment) + + with pytest.warns(UserWarning, match="Could not post auto-merge failure comment"): + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is True + + +def test_review_post_release_pr_warns_when_workflow_run_approval_fails(monkeypatch): + session = _make_review_session() + + def _fail_approve_runs(*args, **kwargs): + raise RuntimeError("actions: write permission required") + + monkeypatch.setattr( + "ccbr_actions.post_release_pr.approve_pending_workflow_runs", + _fail_approve_runs, + ) + + with pytest.warns(UserWarning, match="Could not approve pending workflow runs"): + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is True From 61d9706997073475fdb99831b57aeab6025f5f43 Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Fri, 25 Sep 2026 13:09:55 -0400 Subject: [PATCH 09/23] docs(review-pre-commit-pr): simplify example to a minimal standalone workflow _commit message is ai-generated_ --- review-pre-commit-pr/README.md | 84 ++++----------------------------- review-pre-commit-pr/README.qmd | 75 +++-------------------------- 2 files changed, 17 insertions(+), 142 deletions(-) diff --git a/review-pre-commit-pr/README.md b/review-pre-commit-pr/README.md index 511b700c..7f8a1c7f 100644 --- a/review-pre-commit-pr/README.md +++ b/review-pre-commit-pr/README.md @@ -61,107 +61,43 @@ check as one of the required checks. In your repo’s rulesets Consider also including other desired checks such as your build/test workflow, auto-format, etc. -
- - -
+![Branch protection +rule](https://raw.githubusercontent.com/CCBR/actions/main/review-pre-commit-pr/img/branch-protection-rule.png) -### Example - -[pre-review-pr.yml](/examples/pre-review-pr.yml) +### Basic example ```yaml -name: pre-review-pr - on: pull_request: types: - opened - review_requested - # Manually re-scan open PRs for any that meet the pre-commit.ci autoupdate criteria - workflow_dispatch: - inputs: - force-review: - description: Re-submit reviews for PRs that already have an approval - required: false - default: false - type: boolean permissions: contents: write issues: write pull-requests: write -concurrency: - group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true - jobs: - find-prec-prs: - # Only needed when manually dispatched; the pull_request event already has a single PR to act on - if: github.event_name == 'workflow_dispatch' - runs-on: ubuntu-latest - outputs: - pr-numbers: ${{ steps.search.outputs.pr-numbers }} - steps: - - name: Search for open pre-commit.ci autoupdate PRs - id: search - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - pr_numbers=$(gh pr list \ - --repo "${{ github.repository }}" \ - --state open \ - --json number,title,author \ - --jq '[.[] | select(.title == "[pre-commit.ci] pre-commit autoupdate" and .author.is_bot)] | map(.number)') - echo "pr-numbers=${pr_numbers}" >> "$GITHUB_OUTPUT" - review-pre-commit-pr: - needs: [find-prec-prs] runs-on: ubuntu-latest container: nciccbr/ccbr_actions:latest - # Only run for pre-commit.ci autoupdate PRs or when ccbr-bot[bot] is requested as a reviewer - if: > - always() && - ( - ( - github.event_name == 'pull_request' && - github.event.pull_request.title == '[pre-commit.ci] pre-commit autoupdate' && - ( - (github.event.action == 'opened' && github.event.sender.type == 'Bot') || - (github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'ccbr-bot[bot]') - ) - ) - || (github.event_name == 'workflow_dispatch' && needs.find-prec-prs.outputs.pr-numbers != '[]') - ) - strategy: - fail-fast: false - matrix: - pr-number: ${{ github.event_name == 'workflow_dispatch' && fromJson(needs.find-prec-prs.outputs.pr-numbers) || fromJson(format('[{0}]', github.event.pull_request.number)) }} - + # Only run for pre-commit.ci autoupdate PRs + if: github.event.pull_request.title == '[pre-commit.ci] pre-commit autoupdate' steps: - - uses: actions/checkout@v7 - - name: Generate CCBR-bot token + - uses: actions/create-github-app-token@v3 id: generate-token - uses: actions/create-github-app-token@v3 with: client-id: ${{ vars.CCBR_BOT_APP_ID }} private-key: ${{ secrets.CCBR_BOT_PRIVATE_KEY }} - owner: ${{ github.repository_owner }} - permission-contents: write - permission-issues: write - permission-pull-requests: write - - uses: CCBR/actions/review-pre-commit-pr@latest + - uses: CCBR/actions/review-pre-commit-pr@main with: github-token: ${{ steps.generate-token.outputs.token }} - pr-number: ${{ matrix.pr-number }} - repo: ${{ github.repository }} - reviewer: CCBR/adminteam - force-review: ${{ github.event.action == 'review_requested' || inputs.force-review || false }} + pr-number: ${{ github.event.pull_request.number }} ``` +See also: [pre-review-pr.yml](./examples/pre-review-pr.yml). + ## Inputs - `github-token`: GitHub API token (e.g. a CCBR-bot token generated with diff --git a/review-pre-commit-pr/README.qmd b/review-pre-commit-pr/README.qmd index aa1a83bd..0ec0ff0b 100644 --- a/review-pre-commit-pr/README.qmd +++ b/review-pre-commit-pr/README.qmd @@ -70,99 +70,38 @@ Consider also including other desired checks such as your build/test workflow, a ### Example -[pre-review-pr.yml](/examples/pre-review-pr.yml) - - ```yaml -name: pre-review-pr - +name: review-pre-commit-pr on: pull_request: types: - opened - review_requested - # Manually re-scan open PRs for any that meet the pre-commit.ci autoupdate criteria - workflow_dispatch: - inputs: - force-review: - description: Re-submit reviews for PRs that already have an approval - required: false - default: false - type: boolean permissions: contents: write issues: write pull-requests: write -concurrency: - group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true - jobs: - find-prec-prs: - # Only needed when manually dispatched; the pull_request event already has a single PR to act on - if: github.event_name == 'workflow_dispatch' - runs-on: ubuntu-latest - outputs: - pr-numbers: ${{ steps.search.outputs.pr-numbers }} - steps: - - name: Search for open pre-commit.ci autoupdate PRs - id: search - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - pr_numbers=$(gh pr list \ - --repo "${{ github.repository }}" \ - --state open \ - --json number,title,author \ - --jq '[.[] | select(.title == "[pre-commit.ci] pre-commit autoupdate" and .author.is_bot)] | map(.number)') - echo "pr-numbers=${pr_numbers}" >> "$GITHUB_OUTPUT" - review-pre-commit-pr: - needs: [find-prec-prs] runs-on: ubuntu-latest container: nciccbr/ccbr_actions:latest - # Only run for pre-commit.ci autoupdate PRs or when ccbr-bot[bot] is requested as a reviewer - if: > - always() && - ( - ( - github.event_name == 'pull_request' && - github.event.pull_request.title == '[pre-commit.ci] pre-commit autoupdate' && - ( - (github.event.action == 'opened' && github.event.sender.type == 'Bot') || - (github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'ccbr-bot[bot]') - ) - ) - || (github.event_name == 'workflow_dispatch' && needs.find-prec-prs.outputs.pr-numbers != '[]') - ) - strategy: - fail-fast: false - matrix: - pr-number: ${{ github.event_name == 'workflow_dispatch' && fromJson(needs.find-prec-prs.outputs.pr-numbers) || fromJson(format('[{0}]', github.event.pull_request.number)) }} - + # Only run for pre-commit.ci autoupdate PRs + if: github.event.pull_request.title == '[pre-commit.ci] pre-commit autoupdate' steps: - - uses: actions/checkout@v7 - - name: Generate CCBR-bot token + - uses: actions/create-github-app-token@v3 id: generate-token - uses: actions/create-github-app-token@v3 with: client-id: ${{ vars.CCBR_BOT_APP_ID }} private-key: ${{ secrets.CCBR_BOT_PRIVATE_KEY }} - owner: ${{ github.repository_owner }} - permission-contents: write - permission-issues: write - permission-pull-requests: write - - uses: CCBR/actions/review-pre-commit-pr@latest + - uses: CCBR/actions/review-pre-commit-pr@main with: github-token: ${{ steps.generate-token.outputs.token }} - pr-number: ${{ matrix.pr-number }} - repo: ${{ github.repository }} - reviewer: CCBR/adminteam - force-review: ${{ github.event.action == 'review_requested' || inputs.force-review || false }} + pr-number: ${{ github.event.pull_request.number }} ``` +See also: [pre-review-pr.yml](/examples/pre-review-pr.yml). ```{python} print(ccbr_actions.docs.action_markdown_io(action)) From 6639359541b398fa74a7323fc8ff4e9467ee40bc Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Fri, 25 Sep 2026 13:11:09 -0400 Subject: [PATCH 10/23] docs: minor improvement to example --- review-post-release-pr/README.qmd | 2 ++ 1 file changed, 2 insertions(+) diff --git a/review-post-release-pr/README.qmd b/review-post-release-pr/README.qmd index 6b5dc427..60db3f3e 100644 --- a/review-post-release-pr/README.qmd +++ b/review-post-release-pr/README.qmd @@ -67,6 +67,8 @@ must have these repository permissions as well. ### Basic example ```yaml +name: review-post-release-pr + on: pull_request: types: From 086393e8991e27ed086ae865e56dd488616e9a41 Mon Sep 17 00:00:00 2001 From: CCBR-bot <258092125+ccbr-bot@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:15:35 +0000 Subject: [PATCH 11/23] =?UTF-8?q?ci:=20=F0=9F=A4=96=20render=20readme=20&?= =?UTF-8?q?=20format=20everything=20with=20pre-commit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- review-post-release-pr/README.md | 2 ++ review-pre-commit-pr/README.md | 13 +++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/review-post-release-pr/README.md b/review-post-release-pr/README.md index c0895e8d..1e5d9523 100644 --- a/review-post-release-pr/README.md +++ b/review-post-release-pr/README.md @@ -62,6 +62,8 @@ installation must have these repository permissions as well. ### Basic example ```yaml +name: review-post-release-pr + on: pull_request: types: diff --git a/review-pre-commit-pr/README.md b/review-pre-commit-pr/README.md index 7f8a1c7f..99d0a432 100644 --- a/review-pre-commit-pr/README.md +++ b/review-pre-commit-pr/README.md @@ -61,12 +61,17 @@ check as one of the required checks. In your repo’s rulesets Consider also including other desired checks such as your build/test workflow, auto-format, etc. -![Branch protection -rule](https://raw.githubusercontent.com/CCBR/actions/main/review-pre-commit-pr/img/branch-protection-rule.png) +
+ + +
-### Basic example +### Example ```yaml +name: review-pre-commit-pr on: pull_request: types: @@ -96,7 +101,7 @@ jobs: pr-number: ${{ github.event.pull_request.number }} ``` -See also: [pre-review-pr.yml](./examples/pre-review-pr.yml). +See also: [pre-review-pr.yml](/examples/pre-review-pr.yml). ## Inputs From fdc6c9b08e3bb64f32d6e5b96c5dc168363a96bc Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Fri, 25 Sep 2026 13:51:49 -0400 Subject: [PATCH 12/23] fix(review-post-release-pr): address GitHub Copilot review feedback on #227 - Audit that the version file was actually bumped, not just that changed files are allowed. - Replace generic per-token diff validation with field-specific validators (version, R DESCRIPTION, CITATION.cff, codemeta.json, changelog heading insertion, readme) using full file content instead of GitHub's patch field. - Paginate get_pr_files and fail closed on a pagination/changed_files count mismatch. - Pin approvals to the validated head commit, recheck it immediately before approving, and abort if it changed concurrently. - Track approvals per-commit (is_pr_approved_for_commit) so a stale approval left before an auto-format push no longer short-circuits re-validation. - Add synchronize to the pull_request trigger so auto-format pushes are re-reviewed. - Fix the example workflow's action ref: review-post-release-pr isn't in any release yet, so use @main until the next release. _commit message is ai-generated_ --- CHANGELOG.md | 2 +- examples/pre-review-pr.yml | 4 +- review-post-release-pr/README.md | 48 +- review-post-release-pr/README.qmd | 40 +- review-post-release-pr/action.yml | 4 + src/ccbr_actions/post_release_pr.py | 581 +++++++++++++++----- src/ccbr_actions/pr_review.py | 107 +++- tests/test_examples.py | 18 +- tests/test_post_release_pr.py | 801 +++++++++++++++++++++------- tests/test_pr_review.py | 121 +++++ 10 files changed, 1341 insertions(+), 385 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0a1590d..1a5b8a84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ ## actions development version -- New action: `review-post-release-pr` to automatically review post-release cleanup PRs and approve them (approving pending workflow runs and enabling auto-merge) when only version/date bumps matching an actual release tag are present, or request a human reviewer otherwise. (#227, @kelly-sovacool, @copilot) +- New action: `review-post-release-pr` to automatically review post-release cleanup PRs and approve them (approving pending workflow runs and enabling auto-merge) when the version file was actually bumped and every changed file's new content is a valid, field-specific bump matching an actual release tag, or request a human reviewer otherwise. Approvals are pinned to the validated commit and re-validated if the PR's head changes afterward (e.g. after an `auto-format` push). (#227, @kelly-sovacool, @copilot) - Fix `review-pre-commit-pr` to post a comment instead of requesting changes when a pre-commit.ci autoupdate PR needs human review. (#226, @kelly-sovacool, @copilot) ## actions 0.8.0 diff --git a/examples/pre-review-pr.yml b/examples/pre-review-pr.yml index c95474c0..b59f045d 100644 --- a/examples/pre-review-pr.yml +++ b/examples/pre-review-pr.yml @@ -5,6 +5,7 @@ on: types: - opened - review_requested + - synchronize # Manually re-scan open PRs for any that meet the pre-commit.ci autoupdate criteria workflow_dispatch: inputs: @@ -118,6 +119,7 @@ jobs: github.event_name == 'pull_request' && startsWith(github.event.pull_request.title, 'chore: post-release cleanup for') && ( + github.event.action == 'synchronize' || (github.event.action == 'opened' && github.event.sender.type == 'Bot') || (github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'ccbr-bot[bot]') ) @@ -142,7 +144,7 @@ jobs: permission-contents: write permission-issues: write permission-pull-requests: write - - uses: CCBR/actions/review-post-release-pr@v0.8.0 + - uses: CCBR/actions/review-post-release-pr@main with: github-token: ${{ steps.generate-token.outputs.token }} pr-number: ${{ matrix.pr-number }} diff --git a/review-post-release-pr/README.md b/review-post-release-pr/README.md index 1e5d9523..9898bc48 100644 --- a/review-post-release-pr/README.md +++ b/review-post-release-pr/README.md @@ -6,10 +6,10 @@ version/date bumps matching a real release tag are present, otherwise request a human reviewer This action automates the review of post-release cleanup pull requests -opened by the [post-release](/post-release) action, with titles of the +opened by the [post-release](./post-release) action, with titles of the form `chore: post-release cleanup for `. -The action verifies three conditions: +The action verifies four conditions: 1. **The PR matches the post-release cleanup pattern** – the title matches `chore: post-release cleanup for `, the sender is a @@ -17,18 +17,29 @@ The action verifies three conditions: release in the repo. 2. **Only allowed files were changed** – the version file, citation file, `codemeta.json`, changelog file, news file, and/or readme - files. No other files should be modified. -3. **Only version/date bumps were made** – every change in those files - must be a version or date bump consistent with the release found in - condition - 1. A readme file’s suggested citation snippet may also be - re-rendered by the `auto-format` action in response to the - version/date bump; that type of change is accepted the same way. - -When all three conditions are satisfied the action: + files. No other files should be modified, and the full list of + changed files must be retrievable (the action fails closed if a + pagination mismatch is detected). +3. **The version file was actually bumped** – auditing that the + `post-release` action did its job, not just that the changed files + are individually allowed. +4. **Every changed file’s new content is a valid bump for its role** – + the version file must match the expected dev version exactly; + `CITATION.cff` and `codemeta.json` must match the release + tag/version with no other fields changed; the changelog/news file + must have gained only the expected release heading; and readme files + may only have existing version/date tokens replaced (no inserted + lines), matching a re-rendered citation snippet from the + `auto-format` action. + +When all conditions are satisfied the action: - Approves any workflow runs pending approval on the PR’s head branch. -- Approves the PR as the token’s actor (typically CCBR-bot). +- Re-checks the PR’s head commit immediately before approving, aborting + if it changed since validation (e.g. a concurrent push), to avoid + approving unreviewed changes. +- Approves the PR, pinned to the validated commit, as the token’s actor + (typically CCBR-bot). - Attempts to enable squash auto-merge so the PR merges automatically once all required checks pass. If auto-merge is unavailable, the approval remains and the action posts a comment containing the GitHub @@ -43,8 +54,13 @@ When any condition is **not** satisfied the action: `draft-release.yml` workflow run; 3. otherwise, the repo’s default (catch-all `*`) `CODEOWNERS` entry. +If the PR already has an APPROVED review tied to its _current_ head +commit, no new review is submitted. A stale approval left on an earlier +commit (e.g. before the `auto-format` action pushes a citation rerender) +does not count, so the PR is re-validated — trigger your workflow on the +`synchronize` PR event (as shown below) so new commits are re-checked. Set `force-review` to `true` to re-submit the review and reviewer -request even when the PR already has an approval. It defaults to +request even when the PR already has a current approval. It defaults to `false`. ## Usage @@ -69,6 +85,7 @@ on: types: - opened - review_requested + - synchronize permissions: actions: write @@ -94,7 +111,7 @@ jobs: pr-number: ${{ github.event.pull_request.number }} ``` -See also [pre-review-pr.yml](/examples/pre-review-pr.yml). +See also [pre-review-pr.yml](./examples/pre-review-pr.yml). ## Inputs @@ -117,6 +134,9 @@ See also [pre-review-pr.yml](/examples/pre-review-pr.yml). `CHANGELOG.md`. - `description-filepath`: Path to the R DESCRIPTION file, used when an R package is detected. Default: `DESCRIPTION`. +- `dev-header`: Header string to match to find the development version + entry in the changelog, typically of the form ‘\## + development version’. Default: `development version`. - `ccbr-actions-version`: The version of CCBR/actions to install when running outside the ccbr_actions container (branch, tag, or ‘latest’). **Required.** Default: `latest`. diff --git a/review-post-release-pr/README.qmd b/review-post-release-pr/README.qmd index 60db3f3e..442b3b3c 100644 --- a/review-post-release-pr/README.qmd +++ b/review-post-release-pr/README.qmd @@ -18,24 +18,34 @@ This action automates the review of post-release cleanup pull requests opened by the [post-release](/post-release) action, with titles of the form `chore: post-release cleanup for `. -The action verifies three conditions: +The action verifies four conditions: 1. **The PR matches the post-release cleanup pattern** – the title matches `chore: post-release cleanup for `, the sender is a GitHub App (Bot), and `` corresponds to an actual GitHub release in the repo. 2. **Only allowed files were changed** – the version file, citation file, `codemeta.json`, changelog file, news file, and/or readme files. No other - files should be modified. -3. **Only version/date bumps were made** – every change in those files must - be a version or date bump consistent with the release found in condition - 1. A readme file's suggested citation snippet may also be re-rendered by - the `auto-format` action in response to the version/date bump; that type - of change is accepted the same way. - -When all three conditions are satisfied the action: + files should be modified, and the full list of changed files must be + retrievable (the action fails closed if a pagination mismatch is detected). +3. **The version file was actually bumped** – auditing that the + `post-release` action did its job, not just that the changed files are + individually allowed. +4. **Every changed file's new content is a valid bump for its role** – the + version file must match the expected dev version exactly; `CITATION.cff` + and `codemeta.json` must match the release tag/version with no other + fields changed; the changelog/news file must have gained only the expected + release heading; and readme files may only have existing version/date + tokens replaced (no inserted lines), matching a re-rendered citation + snippet from the `auto-format` action. + +When all conditions are satisfied the action: - Approves any workflow runs pending approval on the PR's head branch. -- Approves the PR as the token's actor (typically CCBR-bot). +- Re-checks the PR's head commit immediately before approving, aborting if it + changed since validation (e.g. a concurrent push), to avoid approving + unreviewed changes. +- Approves the PR, pinned to the validated commit, as the token's actor + (typically CCBR-bot). - Attempts to enable squash auto-merge so the PR merges automatically once all required checks pass. If auto-merge is unavailable, the approval remains and the action posts a comment containing the GitHub API error. @@ -49,8 +59,13 @@ When any condition is **not** satisfied the action: workflow run; 3. otherwise, the repo's default (catch-all `*`) `CODEOWNERS` entry. -Set `force-review` to `true` to re-submit the review and reviewer request even -when the PR already has an approval. It defaults to `false`. +If the PR already has an APPROVED review tied to its *current* head commit, +no new review is submitted. A stale approval left on an earlier commit (e.g. +before the `auto-format` action pushes a citation rerender) does not count, so +the PR is re-validated — trigger your workflow on the `synchronize` PR event +(as shown below) so new commits are re-checked. Set `force-review` to `true` +to re-submit the review and reviewer request even when the PR already has a +current approval. It defaults to `false`. ## Usage @@ -74,6 +89,7 @@ on: types: - opened - review_requested + - synchronize permissions: actions: write diff --git a/review-post-release-pr/action.yml b/review-post-release-pr/action.yml index f7978298..c2666d09 100644 --- a/review-post-release-pr/action.yml +++ b/review-post-release-pr/action.yml @@ -32,6 +32,9 @@ inputs: description-filepath: description: "Path to the R DESCRIPTION file, used when an R package is detected." default: "DESCRIPTION" + dev-header: + description: "Header string to match to find the development version entry in the changelog, typically of the form '## development version'." + default: "development version" ccbr-actions-version: description: "The version of CCBR/actions to install when running outside the ccbr_actions container (branch, tag, or 'latest')." required: true @@ -73,6 +76,7 @@ runs: citation_filepath="${{ inputs.citation-filepath }}", changelog_filepath="${{ inputs.changelog-filepath }}", description_filepath="${{ inputs.description-filepath }}", + dev_header="${{ inputs.dev-header }}", token=os.environ["GITHUB_TOKEN"], ) print(f"Action completed with automatic_approval={result}") diff --git a/src/ccbr_actions/post_release_pr.py b/src/ccbr_actions/post_release_pr.py index bd80e572..1c16b3b7 100644 --- a/src/ccbr_actions/post_release_pr.py +++ b/src/ccbr_actions/post_release_pr.py @@ -3,11 +3,14 @@ ``post-release`` action. """ +import base64 +import json import os import re import warnings import requests +import yaml from .github import GITHUB_API_URL, github_api_get from .pr_review import ( @@ -17,15 +20,16 @@ get_codeowners_content, get_last_workflow_run_actor, get_pr_files, - is_pr_approved, + is_pr_approved_for_commit, match_codeowners, post_pr_comment, request_reviewer, ) +from .release import get_r_dev_version POST_RELEASE_PR_TITLE_PATTERN = re.compile(r"^chore: post-release cleanup for (\S+)$") DRAFT_RELEASE_WORKFLOW_FILE = "draft-release.yml" -_TOKEN_PATTERN = re.compile(r"\d+(?:[.\-]\d+)*") +_VERSION_TOKEN_PATTERN = re.compile(r"\d+(?:[.\-]\d+)*") def is_post_release_pr(pr_title, pr_sender_type): @@ -82,11 +86,11 @@ def get_release_by_tag(repo, tag, token=None, session=None): return release -def _allowed_basenames( +def _file_roles( version_filepath, citation_filepath, changelog_filepath, description_filepath ): """ - Build the set of file basenames a post-release cleanup PR may change. + Map allowed file basenames to the validator role used to check their bump. Args: version_filepath (str): Path to the version file. @@ -95,52 +99,54 @@ def _allowed_basenames( description_filepath (str): Path to the R DESCRIPTION file. Returns: - set[str]: Allowed file basenames (README files are matched separately - by prefix). + dict[str, str]: Basename to role (``"version"``, ``"citation"``, + ``"codemeta"``, ``"changelog"``, or ``"description"``). README files + are matched separately by prefix and default to the ``"readme"`` + role. """ return { - os.path.basename(version_filepath), - os.path.basename(citation_filepath), - os.path.basename(changelog_filepath), - os.path.basename(description_filepath), - "codemeta.json", - "NEWS.md", - "NEWS", + os.path.basename(version_filepath): "version", + os.path.basename(citation_filepath): "citation", + "codemeta.json": "codemeta", + os.path.basename(changelog_filepath): "changelog", + "NEWS.md": "changelog", + "NEWS": "changelog", + os.path.basename(description_filepath): "description", } -def is_allowed_filename(filename, allowed_basenames): +def is_allowed_filename(filename, roles): """ Check whether a changed file is allowed in a post-release cleanup PR. Args: filename (str): Changed file's path, relative to the repo root. - allowed_basenames (set[str]): Allowed file basenames, from - [](`~ccbr_actions.post_release_pr._allowed_basenames`). + roles (dict[str, str]): Allowed file basenames mapped to validator + roles, from [](`~ccbr_actions.post_release_pr._file_roles`). Returns: bool: ``True`` if *filename*'s basename is allowed, including any README file (matched case-insensitively by prefix). """ basename = os.path.basename(filename) - return basename in allowed_basenames or basename.lower().startswith("readme") + return basename in roles or basename.lower().startswith("readme") -def check_only_allowed_files_changed(pr_files, allowed_basenames): +def check_only_allowed_files_changed(pr_files, roles): """ Check that every changed file in the PR is an allowed post-release file. Args: pr_files (list[dict]): File objects as returned by [](`~ccbr_actions.pr_review.get_pr_files`). - allowed_basenames (set[str]): Allowed file basenames, from - [](`~ccbr_actions.post_release_pr._allowed_basenames`). + roles (dict[str, str]): Allowed file basenames mapped to validator + roles, from [](`~ccbr_actions.post_release_pr._file_roles`). Returns: bool: ``True`` if *pr_files* is non-empty and every file is allowed. """ return bool(pr_files) and all( - is_allowed_filename(f.get("filename", ""), allowed_basenames) for f in pr_files + is_allowed_filename(f.get("filename", ""), roles) for f in pr_files ) @@ -148,6 +154,8 @@ def _release_bump_values(release_tag, release): """ Build the set of token values allowed as a version/date bump target. + Used only for the lenient, prose-oriented readme validator. + Args: release_tag (str): The release tag name (e.g. ``"v0.7.1"``). release (dict): Release object from @@ -172,13 +180,13 @@ def _split_tokens(line): Split a line into its non-numeric text segments and numeric tokens. Args: - line (str): A single diff line's content (without the leading +/-). + line (str): A single line of file content. Returns: tuple[list[str], list[str]]: The text segments surrounding each numeric/version-like token, and the tokens themselves. """ - return _TOKEN_PATTERN.split(line), _TOKEN_PATTERN.findall(line) + return _VERSION_TOKEN_PATTERN.split(line), _VERSION_TOKEN_PATTERN.findall(line) def _is_valid_bump_line(old_line, new_line, valid_values): @@ -186,8 +194,8 @@ def _is_valid_bump_line(old_line, new_line, valid_values): Check whether a modified line is only a version/date bump. Args: - old_line (str): The removed line's content. - new_line (str): The added line's content. + old_line (str): The line's previous content. + new_line (str): The line's new content. valid_values (set[str]): Acceptable new token values, from [](`~ccbr_actions.post_release_pr._release_bump_values`). @@ -207,114 +215,345 @@ def _is_valid_bump_line(old_line, new_line, valid_values): return result -def _is_valid_insertion(new_line, release_version): +def _get_file_content(repo, path, ref, token=None, session=None): """ - Check whether a pure line insertion is an allowed post-release addition. + Fetch a file's decoded text content at a specific git ref. Args: - new_line (str): The added line's content. + repo (str): Repository full name (e.g. ``"CCBR/actions"``). + path (str): File path, relative to the repo root. + ref (str): Git ref (commit SHA, branch, or tag) to read the file at. + token (str, optional): GitHub API token. + session: Requests-compatible session object for dependency injection. + + Returns: + str | None: Decoded file content, or ``None`` if the file doesn't + exist at *ref* or its content couldn't be decoded. + """ + url = f"{GITHUB_API_URL}/repos/{repo}/contents/{path}" + try: + data = github_api_get( + url=url, token=token, session=session, params={"ref": ref} + ) + except requests.exceptions.RequestException: + data = None + content = None + if isinstance(data, dict) and isinstance(data.get("content"), str): + try: + content = base64.b64decode(data["content"]).decode("utf-8") + except (ValueError, UnicodeDecodeError): + content = None + return content + + +def _validate_version_file(new_content, release_version): + """ + Check that a plain version file was bumped to the expected dev version. + + Args: + new_content (str | None): The file's content at the PR's head. release_version (str): The release version (without leading ``v``). Returns: - bool: ``True`` if the line is blank or references the release - version (e.g. a new changelog heading). + bool: ``True`` if *new_content* is exactly ``"{release_version}-dev"``. """ - stripped = new_line.strip() - return not stripped or release_version in new_line + return ( + isinstance(new_content, str) and new_content.strip() == f"{release_version}-dev" + ) + + +def _validate_description_file(old_content, new_content, release_version): + """ + Check that an R ``DESCRIPTION`` file's ``Version:`` field was bumped. + Args: + old_content (str | None): The file's content at the PR's base. + new_content (str | None): The file's content at the PR's head. + release_version (str): The release version (without leading ``v``). -def _flush_diff_buffer(removed_buffer, added_buffer, valid_values, release_version): + Returns: + bool: ``True`` if the only change is the ``Version:`` line, bumped to + the R development version (``X.Y.Z.9000``). """ - Validate a buffered block of consecutive removed/added diff lines. + if not isinstance(old_content, str) or not isinstance(new_content, str): + result = False + else: + try: + expected_version = get_r_dev_version(release_version) + except ValueError: + expected_version = None + old_lines = old_content.splitlines() + new_lines = new_content.splitlines() + if expected_version is None or len(old_lines) != len(new_lines): + result = False + else: + diffs = [ + (old_line, new_line) + for old_line, new_line in zip(old_lines, new_lines) + if old_line != new_line + ] + result = ( + len(diffs) == 1 + and diffs[0][0].startswith("Version:") + and diffs[0][1] == f"Version: {expected_version}" + ) + return result + - Removed and added lines are paired positionally. Extra removed lines must - be blank, and extra added lines must be blank or reference the release - version. +def _validate_citation_file(old_content, new_content, release_tag): + """ + Check that a ``CITATION.cff`` file's version/date fields were bumped. Args: - removed_buffer (list[str]): Buffered removed line contents. - added_buffer (list[str]): Buffered added line contents. - valid_values (set[str]): Acceptable new token values, from - [](`~ccbr_actions.post_release_pr._release_bump_values`). + old_content (str | None): The file's content at the PR's base. + new_content (str | None): The file's content at the PR's head. + release_tag (str): The release tag (e.g. ``"v0.7.1"``). + + Returns: + bool: ``True`` if only ``version``/``date-released`` changed, the new + version equals *release_tag*, and the new date is a valid + ``YYYY-MM-DD`` string. + """ + try: + old_data = yaml.safe_load(old_content) if old_content else {} + new_data = yaml.safe_load(new_content) if new_content else {} + except yaml.YAMLError: + old_data, new_data = None, None + if not isinstance(old_data, dict) or not isinstance(new_data, dict): + result = False + else: + exempt_keys = {"version", "date-released"} + unchanged_keys = (set(old_data) | set(new_data)) - exempt_keys + fields_unchanged = all( + old_data.get(k) == new_data.get(k) for k in unchanged_keys + ) + new_date = new_data.get("date-released") + version_ok = new_data.get("version") == release_tag + date_ok = bool(re.fullmatch(r"\d{4}-\d{2}-\d{2}", str(new_date))) + result = fields_unchanged and version_ok and date_ok + return result + + +def _validate_codemeta_file(new_content, release_version, release_tag): + """ + Check that ``codemeta.json``'s version field (if present) matches the release. + + Args: + new_content (str | None): The file's content at the PR's head. release_version (str): The release version (without leading ``v``). + release_tag (str): The release tag (e.g. ``"v0.7.1"``). Returns: - bool: ``True`` if the buffered block only contains valid bumps and/or - allowed insertions. + bool: ``True`` if *new_content* is valid JSON and its ``version`` + field (if any) matches the release. """ - pair_count = min(len(removed_buffer), len(added_buffer)) - pairs_valid = all( - _is_valid_bump_line(removed_buffer[i], added_buffer[i], valid_values) - for i in range(pair_count) - ) - extra_removed_valid = all(not line.strip() for line in removed_buffer[pair_count:]) - extra_added_valid = all( - _is_valid_insertion(line, release_version) for line in added_buffer[pair_count:] - ) - return pairs_valid and extra_removed_valid and extra_added_valid + try: + new_data = json.loads(new_content) if new_content else None + except (json.JSONDecodeError, TypeError): + new_data = None + if not isinstance(new_data, dict): + result = False + else: + version_field = new_data.get("version") + result = version_field is None or version_field in ( + release_version, + release_tag, + f"v{release_version}", + ) + return result -def check_patch_is_version_bump(patch, valid_values, release_version): +def _validate_changelog_file(old_content, new_content, release_version, dev_header): """ - Verify a unified diff patch only contains version/date bump changes. + Check that a changelog/news file only gained a new release heading. + + The only allowed change is a two-line insertion (a heading referencing + *release_version*, followed by a blank line) placed immediately after the + development-version header; everything else must be unchanged. Args: - patch (str): Unified diff patch string for a single file. - valid_values (set[str]): Acceptable new token values, from - [](`~ccbr_actions.post_release_pr._release_bump_values`). - release_version (str): The release version (without leading ``v``), - allowed in pure line insertions such as a new changelog heading. + old_content (str | None): The file's content at the PR's base. + new_content (str | None): The file's content at the PR's head. + release_version (str): The release version (without leading ``v``). + dev_header (str): Development-version header text to locate (matched + case-insensitively). Returns: - bool: ``True`` if every change in the patch is a valid version/date - bump or an allowed insertion. - """ - removed_buffer = [] - added_buffer = [] - is_valid = True - for line in patch.splitlines(): - if line.startswith(("---", "+++")): - pass - elif line.startswith("-"): - removed_buffer.append(line[1:]) - elif line.startswith("+"): - added_buffer.append(line[1:]) + bool: ``True`` if the only change is the expected heading insertion. + """ + if not isinstance(old_content, str) or not isinstance(new_content, str): + result = False + else: + old_lines = old_content.splitlines(keepends=True) + new_lines = new_content.splitlines(keepends=True) + dev_header_index = next( + ( + i + for i, line in enumerate(old_lines) + if line.lstrip().startswith("#") and dev_header.lower() in line.lower() + ), + None, + ) + if dev_header_index is None: + result = False else: - is_valid = ( - _flush_diff_buffer( - removed_buffer, added_buffer, valid_values, release_version - ) - and is_valid + insert_at = dev_header_index + 1 + if insert_at < len(old_lines) and not old_lines[insert_at].strip(): + insert_at += 1 + inserted = new_lines[insert_at : insert_at + 2] + heading_pattern = re.compile( + r"^#+\s+\S.*\b" + re.escape(release_version) + r"\b.*$" ) - removed_buffer = [] - added_buffer = [] - is_valid = ( - _flush_diff_buffer(removed_buffer, added_buffer, valid_values, release_version) - and is_valid - ) - return is_valid + rebuilt = old_lines[:insert_at] + inserted + old_lines[insert_at:] + result = ( + len(new_lines) == len(old_lines) + 2 + and len(inserted) == 2 + and bool(heading_pattern.match(inserted[0].rstrip("\n"))) + and not inserted[1].strip() + and rebuilt == new_lines + ) + return result -def check_version_date_bumps(pr_files, valid_values, release_version): +def _validate_readme_file(old_content, new_content, valid_values): """ - Verify every changed file's patch is only a version/date bump. + Check that a readme file's changes are only version/date token bumps. + + No lines may be inserted or removed; each changed line must differ from + its previous content only in numeric/version-like tokens. Args: - pr_files (list[dict]): File objects as returned by - [](`~ccbr_actions.pr_review.get_pr_files`). + old_content (str | None): The file's content at the PR's base. + new_content (str | None): The file's content at the PR's head. valid_values (set[str]): Acceptable new token values, from [](`~ccbr_actions.post_release_pr._release_bump_values`). + + Returns: + bool: ``True`` if every line is unchanged or a valid token bump. + """ + if not isinstance(old_content, str) or not isinstance(new_content, str): + result = False + else: + old_lines = old_content.splitlines() + new_lines = new_content.splitlines() + result = len(old_lines) == len(new_lines) and all( + old_line == new_line + or _is_valid_bump_line(old_line, new_line, valid_values) + for old_line, new_line in zip(old_lines, new_lines) + ) + return result + + +def _validate_file_bump( + role, + old_content, + new_content, + release_tag, + release_version, + dev_header, + valid_values, +): + """ + Dispatch to the validator for an allowed file's role. + + Args: + role (str): One of ``"version"``, ``"description"``, ``"citation"``, + ``"codemeta"``, ``"changelog"``, or ``"readme"``. + old_content (str | None): The file's content at the PR's base. + new_content (str | None): The file's content at the PR's head. + release_tag (str): The release tag (e.g. ``"v0.7.1"``). release_version (str): The release version (without leading ``v``). + dev_header (str): Development-version header text for changelog files. + valid_values (set[str]): Acceptable new token values for readme files. Returns: - bool: ``True`` if *pr_files* is non-empty and every file's patch only - contains valid version/date bumps. + bool: ``True`` if the file's change is a valid bump for its role. + """ + if role == "version": + result = _validate_version_file(new_content, release_version) + elif role == "description": + result = _validate_description_file(old_content, new_content, release_version) + elif role == "citation": + result = _validate_citation_file(old_content, new_content, release_tag) + elif role == "codemeta": + result = _validate_codemeta_file(new_content, release_version, release_tag) + elif role == "changelog": + result = _validate_changelog_file( + old_content, new_content, release_version, dev_header + ) + else: + result = _validate_readme_file(old_content, new_content, valid_values) + return result + + +def check_files_are_valid_bumps( + repo, + filenames, + roles, + release_tag, + release, + dev_header, + base_sha, + head_sha, + token=None, + session=None, +): """ - return bool(pr_files) and all( - isinstance(f.get("patch"), str) - and check_patch_is_version_bump(f["patch"], valid_values, release_version) - for f in pr_files + Verify every changed file is a valid bump, and that the version file + itself was actually changed. + + This directly audits that the ``post-release`` action did its job: a PR + that only touches, say, a readme file without also bumping the version + file is rejected. + + Args: + repo (str): Repository full name (e.g. ``"CCBR/actions"``). + filenames (list[str]): Changed file paths from the PR. + roles (dict[str, str]): Basename to validator role, from + [](`~ccbr_actions.post_release_pr._file_roles`). + release_tag (str): The release tag (e.g. ``"v0.7.1"``). + release (dict): Release object from + [](`~ccbr_actions.post_release_pr.get_release_by_tag`). + dev_header (str): Development-version header text for changelog files. + base_sha (str): Commit SHA to read "old" file contents at. + head_sha (str): Commit SHA to read "new" file contents at. + token (str, optional): GitHub API token. + session: Requests-compatible session object for dependency injection. + + Returns: + bool: ``True`` if the version file was among the changed files and + every changed file's new content is a valid bump for its role. + """ + release_version = release_tag.lstrip("v") + valid_values = _release_bump_values(release_tag, release) + version_basename = next( + (basename for basename, role in roles.items() if role == "version"), None + ) + version_bumped = any( + os.path.basename(filename) == version_basename for filename in filenames ) + files_valid = True + for filename in filenames: + role = roles.get(os.path.basename(filename), "readme") + old_content = _get_file_content( + repo, filename, base_sha, token=token, session=session + ) + new_content = _get_file_content( + repo, filename, head_sha, token=token, session=session + ) + files_valid = ( + _validate_file_bump( + role, + old_content, + new_content, + release_tag, + release_version, + dev_header, + valid_values, + ) + and files_valid + ) + return version_bumped and files_valid def determine_post_release_reviewer( @@ -363,7 +602,12 @@ def determine_post_release_reviewer( def _collect_failure_reasons( - is_post_release, release, only_allowed_files, only_valid_bumps, approval_error + is_post_release, + release, + files_complete, + only_allowed_files, + only_valid_bumps, + approval_error, ): """ Build a list of human-readable reasons a post-release PR needs review. @@ -372,9 +616,11 @@ def _collect_failure_reasons( is_post_release (bool): Whether the PR matches the post-release cleanup pattern. release (dict | None): The matching GitHub release, or ``None``. + files_complete (bool): Whether the full list of changed files could + be retrieved (fails closed on a pagination mismatch). only_allowed_files (bool): Whether only allowed files were changed. - only_valid_bumps (bool): Whether all changes were valid version/date - bumps. + only_valid_bumps (bool): Whether the version file was bumped and all + changes were valid bumps. approval_error (Exception | None): The error raised while attempting automatic approval, if any. @@ -389,15 +635,21 @@ def _collect_failure_reasons( ) if release is None: reasons.append("no matching GitHub release tag could be found for this PR") - if release is not None and not only_allowed_files: + if release is not None and not files_complete: + reasons.append( + "the full list of changed files could not be verified (a " + "pagination mismatch was detected), so the PR was not approved" + ) + if release is not None and files_complete and not only_allowed_files: reasons.append( "only the version, citation, codemeta, changelog, news, and " "readme files should be changed, but other files were modified" ) if release is not None and only_allowed_files and not only_valid_bumps: reasons.append( - "the changes must be limited to version/date bumps matching the " - "release tag, but other modifications were found" + "the version file must be bumped and every change must be a " + "valid version/date bump matching the release tag, but " + "validation failed" ) if approval_error is not None: reasons.append( @@ -496,6 +748,8 @@ def _review_post_release_pr( citation_filepath, changelog_filepath, description_filepath, + dev_header, + pr_data, ): """ Evaluate and review a post-release cleanup PR that isn't already approved. @@ -507,14 +761,12 @@ def _review_post_release_pr( bool: ``True`` if the PR was automatically approved, ``False`` if human review was requested. """ - pr_data = github_api_get( - url=f"{GITHUB_API_URL}/repos/{repo}/pulls/{pr_number}", - token=token, - session=session, - ) pr_title = pr_data.get("title", "") pr_sender_type = pr_data.get("user", {}).get("type", "") head_ref = pr_data.get("head", {}).get("ref", "") + head_sha = pr_data.get("head", {}).get("sha", "") + base_sha = pr_data.get("base", {}).get("sha", "") + expected_file_count = pr_data.get("changed_files") is_post_release = is_post_release_pr(pr_title, pr_sender_type) release_tag = extract_release_tag(pr_title) release = ( @@ -525,25 +777,37 @@ def _review_post_release_pr( pr_files = get_pr_files(repo, pr_number, token=token, session=session) filenames = [f.get("filename", "") for f in pr_files] + files_complete = expected_file_count is None or len(pr_files) == expected_file_count print(f"PR title: {pr_title!r}") print(f"PR sender type: {pr_sender_type!r}") print(f"Changed files ({len(filenames)}): {', '.join(filenames) or ''}") print(f"Matches post-release cleanup PR: {is_post_release}") print(f"Release tag from title: {release_tag!r}") print(f"Release found on GitHub: {release is not None}") + print(f"Full file list retrieved: {files_complete}") - allowed_basenames = _allowed_basenames( + roles = _file_roles( version_filepath, citation_filepath, changelog_filepath, description_filepath ) - only_allowed_files = check_only_allowed_files_changed(pr_files, allowed_basenames) + only_allowed_files = files_complete and check_only_allowed_files_changed( + pr_files, roles + ) only_valid_bumps = False - if only_allowed_files and release is not None: - valid_values = _release_bump_values(release_tag, release) - only_valid_bumps = check_version_date_bumps( - pr_files, valid_values, release_tag.lstrip("v") + if only_allowed_files and release is not None and head_sha and base_sha: + only_valid_bumps = check_files_are_valid_bumps( + repo, + filenames, + roles, + release_tag, + release, + dev_header, + base_sha, + head_sha, + token=token, + session=session, ) print(f"Only allowed files changed: {only_allowed_files}") - print(f"Only version/date bumps found: {only_valid_bumps}") + print(f"Version bumped and all changes valid: {only_valid_bumps}") eligible = ( is_post_release @@ -561,13 +825,34 @@ def _review_post_release_pr( except (KeyError, requests.exceptions.RequestException, RuntimeError) as exc: print(f"Could not approve pending workflow runs: {exc}") warnings.warn(f"Could not approve pending workflow runs: {exc}") - try: - response = approve_pr(repo, pr_number, token=token, session=session) - was_auto_approved = True - print(f"Approval submitted successfully (HTTP {response.status_code})") - except (KeyError, requests.exceptions.RequestException, RuntimeError) as exc: - approval_error = exc - print(f"Approval failed: {exc}") + + current_pr_data = github_api_get( + url=f"{GITHUB_API_URL}/repos/{repo}/pulls/{pr_number}", + token=token, + session=session, + ) + current_head_sha = current_pr_data.get("head", {}).get("sha", "") + if current_head_sha and current_head_sha != head_sha: + approval_error = RuntimeError( + "the pull request's head commit changed during review; " + "aborting automatic approval to avoid approving unreviewed " + "changes" + ) + print(f"Aborting approval: {approval_error}") + else: + try: + response = approve_pr( + repo, pr_number, token=token, session=session, commit_id=head_sha + ) + was_auto_approved = True + print(f"Approval submitted successfully (HTTP {response.status_code})") + except ( + KeyError, + requests.exceptions.RequestException, + RuntimeError, + ) as exc: + approval_error = exc + print(f"Approval failed: {exc}") if was_auto_approved: _enable_merge_or_comment( repo, pr_number, reviewer, token=token, session=session @@ -578,6 +863,7 @@ def _review_post_release_pr( failed_reasons = _collect_failure_reasons( is_post_release, release, + files_complete, only_allowed_files, only_valid_bumps, approval_error, @@ -603,30 +889,44 @@ def review_post_release_pr( citation_filepath="CITATION.cff", changelog_filepath="CHANGELOG.md", description_filepath="DESCRIPTION", + dev_header="development version", ): """ Evaluate and review a post-release cleanup pull request. - Checks whether the PR satisfies three conditions: + Checks whether the PR satisfies four conditions: - **Condition 1** – the PR title matches the post-release cleanup pattern (``chore: post-release cleanup for ``) and ** corresponds to an actual GitHub release. - - **Condition 2** – only the version file, citation file, ``codemeta.json``, - changelog file, news file, and/or readme files were changed. - - **Condition 3** – every change in those files is a version/date bump - matching the release tag (or a re-rendered citation snippet in a - readme file, which is validated the same way). + - **Condition 2** – the full list of changed files could be retrieved + (fails closed if a pagination mismatch is detected), and only the + version file, citation file, ``codemeta.json``, changelog file, news + file, and/or readme files were changed. + - **Condition 3** – the version file was actually changed, auditing that + the ``post-release`` action did its job. + - **Condition 4** – every changed file's new content is a valid bump for + its role: the version file matches the expected dev version exactly; + ``CITATION.cff`` and ``codemeta.json`` match the release tag/version + with no other fields changed; the changelog/news file only gained the + expected release heading; and readme files only had existing + version/date tokens replaced (no inserted lines), matching a + re-rendered citation snippet from the ``auto-format`` action. When all conditions are met the function approves any pending workflow - runs on the PR's head branch, approves the PR, and attempts to enable - squash auto-merge. If auto-merge cannot be enabled, it leaves a comment - with the GitHub API error. Otherwise it posts a comment explaining why - the PR needs manual review and requests a human reviewer, resolved via + runs on the PR's head branch, re-checks the PR's head commit immediately + before approving (aborting if it changed since validation), approves the + PR pinned to that commit, and attempts to enable squash auto-merge. If + auto-merge cannot be enabled, it leaves a comment with the GitHub API + error. Otherwise it posts a comment explaining why the PR needs manual + review and requests a human reviewer, resolved via [](`~ccbr_actions.post_release_pr.determine_post_release_reviewer`). - If the PR already has an APPROVED review, no new review is submitted and - the function returns ``True`` immediately unless *force_review* is true. + If the PR already has an APPROVED review tied to its current head commit, + no new review is submitted and the function returns ``True`` immediately + unless *force_review* is true. A stale approval left on an earlier commit + (e.g. after the ``auto-format`` action pushes a citation rerender) does + not count, so the PR is re-validated. Args: repo (str): Repository full name (e.g. ``"CCBR/actions"``). @@ -648,17 +948,26 @@ def review_post_release_pr( description_filepath (str): Path to the R DESCRIPTION file, used when an R package's version file is the DESCRIPTION file. Defaults to ``"DESCRIPTION"``. + dev_header (str): Development-version header text to locate in the + changelog file (matched case-insensitively). Defaults to + ``"development version"``. Returns: bool: ``True`` if the PR was already approved or was automatically approved, ``False`` if human review was requested. """ print(f"Reviewing post-release cleanup PR {repo}#{pr_number}") - already_approved = not force_review and is_pr_approved( - repo, pr_number, token=token, session=session + pr_data = github_api_get( + url=f"{GITHUB_API_URL}/repos/{repo}/pulls/{pr_number}", + token=token, + session=session, + ) + head_sha = pr_data.get("head", {}).get("sha", "") + already_approved = not force_review and is_pr_approved_for_commit( + repo, pr_number, head_sha, token=token, session=session ) if already_approved: - print("Result: PR already has an APPROVED review; no action was taken") + print("Result: PR already has an APPROVED review for the current head commit") result = True else: result = _review_post_release_pr( @@ -671,5 +980,7 @@ def review_post_release_pr( citation_filepath, changelog_filepath, description_filepath, + dev_header, + pr_data, ) return result diff --git a/src/ccbr_actions/pr_review.py b/src/ccbr_actions/pr_review.py index 4914778e..d3fc72a7 100644 --- a/src/ccbr_actions/pr_review.py +++ b/src/ccbr_actions/pr_review.py @@ -17,24 +17,39 @@ _CODEOWNERS_PATHS = ("CODEOWNERS", ".github/CODEOWNERS", "docs/CODEOWNERS") -def get_pr_files(repo, pr_number, token=None, session=None): +def get_pr_files(repo, pr_number, token=None, session=None, per_page=100): """ - Return the list of file objects changed in a pull request. + Return the full, paginated list of file objects changed in a pull request. Args: repo (str): Repository full name (e.g. ``"CCBR/actions"``). pr_number (int | str): Pull request number. token (str, optional): GitHub API token. session: Requests-compatible session object for dependency injection. + per_page (int): Page size to request (max 100 per the GitHub API). Returns: - list[dict]: File objects from the GitHub pull request files API. + list[dict]: File objects from the GitHub pull request files API, + collected across all pages. """ url = f"{GITHUB_API_URL}/repos/{repo}/pulls/{pr_number}/files" - return github_api_get(url=url, token=token, session=session) + files = [] + page = 1 + has_more_pages = True + while has_more_pages: + page_files = github_api_get( + url=url, + token=token, + session=session, + params={"per_page": per_page, "page": page}, + ) + files.extend(page_files) + has_more_pages = len(page_files) == per_page + page += 1 + return files -def approve_pr(repo, pr_number, token=None, session=None): +def approve_pr(repo, pr_number, token=None, session=None, commit_id=None): """ Submit an *APPROVE* review on a pull request. @@ -43,16 +58,22 @@ def approve_pr(repo, pr_number, token=None, session=None): pr_number (int | str): Pull request number. token (str, optional): GitHub API token. session: Requests-compatible session object for dependency injection. + commit_id (str, optional): SHA of the commit the review applies to. + Pinning the review to a specific commit means a concurrent push + leaves the new head without a matching approval. Returns: requests.Response: Response from the GitHub reviews API. """ url = f"{GITHUB_API_URL}/repos/{repo}/pulls/{pr_number}/reviews" + payload = {"event": "APPROVE"} + if commit_id: + payload["commit_id"] = commit_id response = github_api_post( url=url, token=token, session=session, - json={"event": "APPROVE"}, + json=payload, ) response.raise_for_status() return response @@ -75,6 +96,32 @@ def get_pr_reviews(repo, pr_number, token=None, session=None): return github_api_get(url=url, token=token, session=session) +def _latest_reviews_by_reviewer(reviews): + """ + Reduce a PR's review history to each reviewer's most recent review. + + Args: + reviews (list[dict]): Review objects from + [](`~ccbr_actions.pr_review.get_pr_reviews`). + + Returns: + dict: Mapping of reviewer identity to their latest review object. + """ + latest_reviews = {} + for review_index, review in enumerate(reviews): + reviewer = review.get("user", {}).get("login") + if reviewer is None: + reviewer = review.get("user", {}).get("id", review.get("id")) + review_key = ( + review.get("submitted_at") or review.get("created_at") or "", + review_index, + ) + previous = latest_reviews.get(reviewer) + if previous is None or review_key > previous[0]: + latest_reviews[reviewer] = (review_key, review) + return {reviewer: entry[1] for reviewer, entry in latest_reviews.items()} + + def is_pr_approved(repo, pr_number, token=None, session=None): """ Check whether a pull request currently has an APPROVED review. @@ -96,26 +143,46 @@ def is_pr_approved(repo, pr_number, token=None, session=None): and no current reviewer has requested changes. """ reviews = get_pr_reviews(repo, pr_number, token=token, session=session) - latest_reviews = {} - for review_index, review in enumerate(reviews): - reviewer = review.get("user", {}).get("login") - if reviewer is None: - reviewer = review.get("user", {}).get("id", review.get("id")) - review_key = ( - review.get("submitted_at") or review.get("created_at") or "", - review_index, - ) - previous = latest_reviews.get(reviewer) - if previous is None or review_key > previous[0]: - latest_reviews[reviewer] = (review_key, review) - - current_states = [review[1].get("state") for review in latest_reviews.values()] + latest_reviews = _latest_reviews_by_reviewer(reviews) + current_states = [review.get("state") for review in latest_reviews.values()] has_changes_requested = "CHANGES_REQUESTED" in current_states has_approval = "APPROVED" in current_states result = has_approval and not has_changes_requested return result +def is_pr_approved_for_commit(repo, pr_number, commit_sha, token=None, session=None): + """ + Check whether a pull request has a current APPROVED review tied to *commit_sha*. + + Unlike [](`~ccbr_actions.pr_review.is_pr_approved`), this requires the + approving review's ``commit_id`` to match *commit_sha* (typically the + PR's current head), so a stale approval left on an earlier commit (e.g. + before an ``auto-format`` push) does not count. + + Args: + repo (str): Repository full name (e.g. ``"CCBR/actions"``). + pr_number (int | str): Pull request number. + commit_sha (str): Commit SHA the approval must be tied to. + token (str, optional): GitHub API token. + session: Requests-compatible session object for dependency injection. + + Returns: + bool: ``True`` if the current review state includes an APPROVED + review for *commit_sha* and no current reviewer has requested + changes. + """ + reviews = get_pr_reviews(repo, pr_number, token=token, session=session) + latest_reviews = _latest_reviews_by_reviewer(reviews) + current_states = [review.get("state") for review in latest_reviews.values()] + has_changes_requested = "CHANGES_REQUESTED" in current_states + has_matching_approval = bool(commit_sha) and any( + review.get("state") == "APPROVED" and review.get("commit_id") == commit_sha + for review in latest_reviews.values() + ) + return has_matching_approval and not has_changes_requested + + def request_changes(repo, pr_number, comment, token=None, session=None): """ Submit a *REQUEST_CHANGES* review on a pull request. diff --git a/tests/test_examples.py b/tests/test_examples.py index c173313b..87641ba6 100644 --- a/tests/test_examples.py +++ b/tests/test_examples.py @@ -13,6 +13,9 @@ CURRENT_VERSION = (REPOSITORY_ROOT / "VERSION").read_text().strip() CURRENT_RELEASE = f"v{CURRENT_VERSION.removesuffix('-dev')}" ACTION_REFERENCE = re.compile(r"CCBR/actions/([^@\s]+)@([^\s]+)") +# Actions with no release containing them yet may reference @main until the +# next release is cut; the ref should be repinned to that release afterward. +UNRELEASED_ACTIONS = {"review-post-release-pr"} def _iter_steps(value): @@ -28,14 +31,23 @@ def _iter_steps(value): def test_examples_use_current_release_refs(): - """Examples should use the current release rather than a moving ref.""" + """Examples should use the current release, except unreleased actions on @main.""" references = [ - match.group(2) + (match.group(1), match.group(2)) for example in EXAMPLES for match in ACTION_REFERENCE.finditer(example.read_text()) ] assert references - assert set(references) == {CURRENT_RELEASE} + unexpected = [ + (action_name, ref) + for action_name, ref in references + if not ( + ref == CURRENT_RELEASE + or (action_name in UNRELEASED_ACTIONS and ref == "main") + ) + ] + assert not unexpected + assert any(ref == CURRENT_RELEASE for _, ref in references) def test_examples_reference_declared_inputs_and_required_values(): diff --git a/tests/test_post_release_pr.py b/tests/test_post_release_pr.py index 0e061e9c..37019bc2 100644 --- a/tests/test_post_release_pr.py +++ b/tests/test_post_release_pr.py @@ -2,19 +2,27 @@ Tests for ccbr_actions.post_release_pr module. """ +import base64 + import pytest import requests as requests_lib from ccbr_actions.post_release_pr import ( POST_RELEASE_PR_TITLE_PATTERN, - _allowed_basenames, + _file_roles, + _get_file_content, _is_valid_bump_line, - _is_valid_insertion, _release_bump_values, _split_tokens, + _validate_changelog_file, + _validate_citation_file, + _validate_codemeta_file, + _validate_description_file, + _validate_file_bump, + _validate_readme_file, + _validate_version_file, + check_files_are_valid_bumps, check_only_allowed_files_changed, - check_patch_is_version_bump, - check_version_date_bumps, determine_post_release_reviewer, extract_release_tag, get_release_by_tag, @@ -58,6 +66,36 @@ def request(self, method, url, headers=None, **kwargs): return MockResponse(self.payloads.get(url, default), status) +class ContentSession(MockSession): + """MockSession variant that also serves file contents keyed by (path, ref).""" + + def __init__(self, contents=None, **kwargs): + super().__init__(**kwargs) + self.contents = contents or {} + self.pr_payloads_sequence = None + + def request(self, method, url, headers=None, **kwargs): + self.calls.append((method, url, kwargs)) + if "contents/" in url and method == "GET": + path = url.split("contents/", 1)[1] + ref = (kwargs.get("params") or {}).get("ref") + text = self.contents.get((path, ref)) + if text is None: + response = MockResponse({}, 404) + else: + encoded = base64.b64encode(text.encode("utf-8")).decode("ascii") + response = MockResponse({"content": encoded}) + elif self.pr_payloads_sequence is not None and url in self.pr_payloads_sequence: + queue = self.pr_payloads_sequence[url] + payload = queue.pop(0) if len(queue) > 1 else queue[0] + response = MockResponse(payload) + else: + status = self.post_status if method == "POST" else 200 + default = {"data": {}} if method == "POST" else {} + response = MockResponse(self.payloads.get(url, default), status) + return response + + class _RaisingSession(MockSession): """Always raises a RequestException on GET.""" @@ -67,10 +105,11 @@ def request(self, method, url, headers=None, **kwargs): # --------------------------------------------------------------------------- -# Fixture patches, based on CCBR/Tools#229 +# Fixture content, based on CCBR/Tools#229 # --------------------------------------------------------------------------- RELEASE_TAG = "v0.7.1" +RELEASE_VERSION = "0.7.1" PR_TITLE = f"chore: post-release cleanup for {RELEASE_TAG}" RELEASE = { "tag_name": RELEASE_TAG, @@ -78,70 +117,72 @@ def request(self, method, url, headers=None, **kwargs): "created_at": "2026-09-17T00:00:00Z", } -CHANGELOG_PATCH = """\ -@@ -1,5 +1,7 @@ - ## Tools development version - -+## Tools 0.7.1 -+ - - minor documentation improvements. (#197, #198, @kelly-sovacool) +VERSION_OLD = "0.7.0-dev\n" +VERSION_NEW = "0.7.1-dev\n" + +CITATION_OLD = """\ +cff-version: 1.2.0 +message: If you use this software, please cite it as below. +title: Tools +authors: + - family-names: Sovacool + given-names: Kelly +identifiers: + - type: doi + value: 10.5281/zenodo.13377166 +doi: 10.5281/zenodo.13377166 +version: v0.7.0 +date-released: "2026-06-10" """ +CITATION_NEW = CITATION_OLD.replace("version: v0.7.0", "version: v0.7.1").replace( + 'date-released: "2026-06-10"', 'date-released: "2026-09-17"' +) -README_PATCH = """\ -@@ -181,7 +181,7 @@ guidelines](https://CCBR.github.io/Tools/CONTRIBUTING). - Please cite this software if you use it in a publication: - - > Sovacool K., Koparde V., Kuhn S., Tandon M., and Huse S. (2026). CCBR --> Tools: Utilities for CCBR Bioinformatics Software (version v0.7.0). -+> Tools: Utilities for CCBR Bioinformatics Software (version v0.7.1). - > DOI: 10.5281/zenodo.13377166 URL: https://ccbr.github.io/Tools/ - - ### Bibtex entry -@@ -190,7 +190,7 @@ Please cite this software if you use it in a publication: - @misc{YourReferenceHere, - author = {Sovacool, Kelly and Koparde, Vishal and Kuhn, Skyler and Tandon, Mayank and Huse, Susan}, - doi = {10.5281/zenodo.13377166}, -- month = {6}, -+ month = {9}, - title = {CCBR Tools: Utilities for CCBR Bioinformatics Software}, - url = {https://ccbr.github.io/Tools/}, - year = {2026} -""" +CODEMETA_OLD = '{"version": "0.7.0"}' +CODEMETA_NEW = '{"version": "0.7.1"}' -CITATION_PATCH = """\ -@@ -29,5 +29,5 @@ identifiers: - type: doi - value: 10.5281/zenodo.13377166 - doi: 10.5281/zenodo.13377166 --version: v0.7.0 --date-released: "2026-06-10" -+version: v0.7.1 -+date-released: "2026-09-17" -""" +CHANGELOG_OLD = ( + "## Tools development version\n\n" + "- minor documentation improvements. (#197, #198, @kelly-sovacool)\n" +) +CHANGELOG_NEW = ( + "## Tools development version\n\n" + "## Tools 0.7.1\n\n" + "- minor documentation improvements. (#197, #198, @kelly-sovacool)\n" +) -VERSION_PATCH = """\ -@@ -1 +1 @@ --0.7.0-dev -+0.7.1-dev -""" +README_OLD = ( + "> Sovacool K. (2026). CCBR Tools: Utilities (version v0.7.0).\n month = {6},\n" +) +README_NEW = ( + "> Sovacool K. (2026). CCBR Tools: Utilities (version v0.7.1).\n month = {9},\n" +) -# A non-version change slipped into the changelog (invalid) -INVALID_CHANGELOG_PATCH = """\ -@@ -1,5 +1,5 @@ - ## Tools development version +DESCRIPTION_OLD = "Package: pkg\nVersion: 0.7.0.9000\nTitle: Example\n" +DESCRIPTION_NEW = "Package: pkg\nVersion: 0.7.1.9000\nTitle: Example\n" --- minor documentation improvements. (#197, #198, @kelly-sovacool) -+- MAJOR documentation improvements. (#197, #198, @kelly-sovacool) -""" +def _default_roles(): + return _file_roles("VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION") -def _pr_files(): - return [ - {"filename": "CHANGELOG.md", "patch": CHANGELOG_PATCH}, - {"filename": "README.md", "patch": README_PATCH}, - {"filename": "src/ccbr_tools/CITATION.cff", "patch": CITATION_PATCH}, - {"filename": "src/ccbr_tools/VERSION", "patch": VERSION_PATCH}, - ] + +def _default_contents(): + return { + ("VERSION", "base-sha"): VERSION_OLD, + ("VERSION", "head-sha"): VERSION_NEW, + ("CITATION.cff", "base-sha"): CITATION_OLD, + ("CITATION.cff", "head-sha"): CITATION_NEW, + ("codemeta.json", "base-sha"): CODEMETA_OLD, + ("codemeta.json", "head-sha"): CODEMETA_NEW, + ("CHANGELOG.md", "base-sha"): CHANGELOG_OLD, + ("CHANGELOG.md", "head-sha"): CHANGELOG_NEW, + ("README.md", "base-sha"): README_OLD, + ("README.md", "head-sha"): README_NEW, + } + + +def _default_filenames(): + return ["VERSION", "CITATION.cff", "codemeta.json", "CHANGELOG.md", "README.md"] # --------------------------------------------------------------------------- @@ -186,79 +227,63 @@ def test_get_release_by_tag_returns_release_when_found(): def test_get_release_by_tag_returns_none_when_missing(): - class NotFoundSession(MockSession): - def request(self, method, url, headers=None, **kwargs): - self.calls.append((method, url, kwargs)) - raise requests_lib.exceptions.HTTPError("404") - - assert get_release_by_tag("CCBR/Tools", "v9.9.9", session=NotFoundSession()) is None + assert get_release_by_tag("CCBR/Tools", "v9.9.9", session=_RaisingSession()) is None # --------------------------------------------------------------------------- -# _allowed_basenames / is_allowed_filename / check_only_allowed_files_changed +# _file_roles / is_allowed_filename / check_only_allowed_files_changed # --------------------------------------------------------------------------- -def test_allowed_basenames_includes_defaults(): - basenames = _allowed_basenames( - "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" - ) - assert basenames == { - "VERSION", - "CITATION.cff", - "CHANGELOG.md", - "DESCRIPTION", - "codemeta.json", - "NEWS.md", - "NEWS", +def test_file_roles_assigns_expected_roles(): + roles = _default_roles() + assert roles == { + "VERSION": "version", + "CITATION.cff": "citation", + "codemeta.json": "codemeta", + "CHANGELOG.md": "changelog", + "NEWS.md": "changelog", + "NEWS": "changelog", + "DESCRIPTION": "description", } def test_is_allowed_filename_matches_basename_regardless_of_path(): - basenames = _allowed_basenames( - "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" - ) - assert is_allowed_filename("src/ccbr_tools/VERSION", basenames) is True + roles = _default_roles() + assert is_allowed_filename("src/ccbr_tools/VERSION", roles) is True def test_is_allowed_filename_matches_readme_case_insensitively(): - basenames = _allowed_basenames( - "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" - ) - assert is_allowed_filename("Readme.qmd", basenames) is True + roles = _default_roles() + assert is_allowed_filename("Readme.qmd", roles) is True def test_is_allowed_filename_returns_false_for_disallowed_file(): - basenames = _allowed_basenames( - "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" - ) - assert is_allowed_filename("src/main.py", basenames) is False + roles = _default_roles() + assert is_allowed_filename("src/main.py", roles) is False def test_check_only_allowed_files_changed_returns_true_for_pr_files(): - basenames = _allowed_basenames( - "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" - ) - assert check_only_allowed_files_changed(_pr_files(), basenames) is True + roles = _default_roles() + pr_files = [{"filename": name} for name in _default_filenames()] + assert check_only_allowed_files_changed(pr_files, roles) is True def test_check_only_allowed_files_changed_returns_false_when_extra_file(): - basenames = _allowed_basenames( - "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" - ) - pr_files = _pr_files() + [{"filename": "src/main.py"}] - assert check_only_allowed_files_changed(pr_files, basenames) is False + roles = _default_roles() + pr_files = [{"filename": name} for name in _default_filenames()] + [ + {"filename": "src/main.py"} + ] + assert check_only_allowed_files_changed(pr_files, roles) is False def test_check_only_allowed_files_changed_returns_false_when_empty(): - basenames = _allowed_basenames( - "VERSION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION" - ) - assert check_only_allowed_files_changed([], basenames) is False + roles = _default_roles() + assert check_only_allowed_files_changed([], roles) is False # --------------------------------------------------------------------------- -# _release_bump_values +# _release_bump_values / _split_tokens / _is_valid_bump_line # --------------------------------------------------------------------------- @@ -268,14 +293,7 @@ def test_release_bump_values_includes_version_and_date_tokens(): assert "0.7.1" in values assert "0.7.1-dev" in values assert "2026-09-17" in values - assert "2026" in values assert "9" in values - assert "17" in values - - -# --------------------------------------------------------------------------- -# _split_tokens -# --------------------------------------------------------------------------- def test_split_tokens_separates_text_and_numeric_tokens(): @@ -284,19 +302,14 @@ def test_split_tokens_separates_text_and_numeric_tokens(): assert text == ["version: v", ""] -# --------------------------------------------------------------------------- -# _is_valid_bump_line -# --------------------------------------------------------------------------- - - def test_is_valid_bump_line_accepts_version_bump(): values = _release_bump_values(RELEASE_TAG, RELEASE) - assert _is_valid_bump_line("version: v0.7.0", "version: v0.7.1", values) is True + assert _is_valid_bump_line("(version v0.7.0).", "(version v0.7.1).", values) is True def test_is_valid_bump_line_rejects_non_matching_version(): values = _release_bump_values(RELEASE_TAG, RELEASE) - assert _is_valid_bump_line("version: v0.7.0", "version: v9.9.9", values) is False + assert _is_valid_bump_line("v0.7.0", "v9.9.9", values) is False def test_is_valid_bump_line_rejects_changed_surrounding_text(): @@ -304,102 +317,403 @@ def test_is_valid_bump_line_rejects_changed_surrounding_text(): assert _is_valid_bump_line("version: v0.7.0", "ver: v0.7.1", values) is False -def test_is_valid_bump_line_rejects_mismatched_token_count(): - values = _release_bump_values(RELEASE_TAG, RELEASE) - assert _is_valid_bump_line("v0.7.0", "v0.7.1 v0.7.1", values) is False +# --------------------------------------------------------------------------- +# _get_file_content +# --------------------------------------------------------------------------- + + +def test_get_file_content_returns_decoded_text(): + encoded = base64.b64encode(b"hello world").decode("ascii") + session = MockSession( + { + "https://api.github.com/repos/CCBR/Tools/contents/VERSION": { + "content": encoded + } + } + ) + result = _get_file_content("CCBR/Tools", "VERSION", "abc", session=session) + assert result == "hello world" + + +def test_get_file_content_returns_none_when_missing(): + result = _get_file_content( + "CCBR/Tools", "VERSION", "abc", session=_RaisingSession() + ) + assert result is None + + +def test_get_file_content_returns_none_for_invalid_base64(): + session = MockSession( + { + "https://api.github.com/repos/CCBR/Tools/contents/VERSION": { + "content": "not-valid-base64!!" + } + } + ) + result = _get_file_content("CCBR/Tools", "VERSION", "abc", session=session) + assert result is None # --------------------------------------------------------------------------- -# _is_valid_insertion +# _validate_version_file # --------------------------------------------------------------------------- -def test_is_valid_insertion_accepts_blank_line(): - assert _is_valid_insertion("", "0.7.1") is True - assert _is_valid_insertion(" ", "0.7.1") is True +def test_validate_version_file_accepts_exact_dev_bump(): + assert _validate_version_file(VERSION_NEW, RELEASE_VERSION) is True -def test_is_valid_insertion_accepts_heading_with_release_version(): - assert _is_valid_insertion("## Tools 0.7.1", "0.7.1") is True +def test_validate_version_file_rejects_wrong_version(): + assert _validate_version_file("9.9.9-dev\n", RELEASE_VERSION) is False -def test_is_valid_insertion_rejects_unrelated_line(): - assert _is_valid_insertion("- a new unrelated bullet point", "0.7.1") is False +def test_validate_version_file_rejects_missing_content(): + assert _validate_version_file(None, RELEASE_VERSION) is False # --------------------------------------------------------------------------- -# check_patch_is_version_bump +# _validate_description_file # --------------------------------------------------------------------------- -def test_check_patch_is_version_bump_accepts_changelog_heading_insertion(): - assert check_patch_is_version_bump(CHANGELOG_PATCH, {"0.7.1"}, "0.7.1") is True +def test_validate_description_file_accepts_valid_r_dev_bump(): + assert ( + _validate_description_file(DESCRIPTION_OLD, DESCRIPTION_NEW, RELEASE_VERSION) + is True + ) -def test_check_patch_is_version_bump_accepts_readme_citation_rerender(): - values = _release_bump_values(RELEASE_TAG, RELEASE) - assert check_patch_is_version_bump(README_PATCH, values, "0.7.1") is True +def test_validate_description_file_rejects_other_field_changes(): + tampered = DESCRIPTION_NEW.replace("Title: Example", "Title: Tampered") + assert ( + _validate_description_file(DESCRIPTION_OLD, tampered, RELEASE_VERSION) is False + ) -def test_check_patch_is_version_bump_accepts_citation_cff_bump(): - values = _release_bump_values(RELEASE_TAG, RELEASE) - assert check_patch_is_version_bump(CITATION_PATCH, values, "0.7.1") is True +def test_validate_description_file_rejects_wrong_version(): + tampered = DESCRIPTION_OLD.replace("Version: 0.7.0.9000", "Version: 9.9.9.9000") + assert ( + _validate_description_file(DESCRIPTION_OLD, tampered, RELEASE_VERSION) is False + ) -def test_check_patch_is_version_bump_accepts_version_file_bump(): - values = _release_bump_values(RELEASE_TAG, RELEASE) - assert check_patch_is_version_bump(VERSION_PATCH, values, "0.7.1") is True +def test_validate_description_file_rejects_missing_content(): + assert _validate_description_file(None, DESCRIPTION_NEW, RELEASE_VERSION) is False -def test_check_patch_is_version_bump_rejects_non_version_change(): - values = _release_bump_values(RELEASE_TAG, RELEASE) +def test_validate_description_file_rejects_non_semver_release_version(): + assert ( + _validate_description_file(DESCRIPTION_OLD, DESCRIPTION_NEW, "not-semver") + is False + ) + + +def test_validate_description_file_rejects_line_count_mismatch(): + new_with_extra_line = DESCRIPTION_NEW + "Extra: field\n" + assert ( + _validate_description_file( + DESCRIPTION_OLD, new_with_extra_line, RELEASE_VERSION + ) + is False + ) + + +# --------------------------------------------------------------------------- +# _validate_citation_file +# --------------------------------------------------------------------------- + + +def test_validate_citation_file_accepts_valid_bump(): + assert _validate_citation_file(CITATION_OLD, CITATION_NEW, RELEASE_TAG) is True + + +def test_validate_citation_file_rejects_other_field_changes(): + tampered = CITATION_NEW.replace("title: Tools", "title: Tampered") + assert _validate_citation_file(CITATION_OLD, tampered, RELEASE_TAG) is False + + +def test_validate_citation_file_rejects_version_not_matching_release(): + tampered = CITATION_NEW.replace("version: v0.7.1", "version: v9.9.9") + assert _validate_citation_file(CITATION_OLD, tampered, RELEASE_TAG) is False + + +def test_validate_citation_file_rejects_malformed_date(): + tampered = CITATION_NEW.replace( + 'date-released: "2026-09-17"', 'date-released: "not-a-date"' + ) + assert _validate_citation_file(CITATION_OLD, tampered, RELEASE_TAG) is False + + +def test_validate_citation_file_rejects_invalid_yaml(): + assert ( + _validate_citation_file(CITATION_OLD, "not: valid: yaml: [", RELEASE_TAG) + is False + ) + + +# --------------------------------------------------------------------------- +# _validate_codemeta_file +# --------------------------------------------------------------------------- + + +def test_validate_codemeta_file_accepts_matching_version(): + assert _validate_codemeta_file(CODEMETA_NEW, RELEASE_VERSION, RELEASE_TAG) is True + + +def test_validate_codemeta_file_accepts_missing_version_field(): + assert _validate_codemeta_file("{}", RELEASE_VERSION, RELEASE_TAG) is True + + +def test_validate_codemeta_file_rejects_mismatched_version(): + assert ( + _validate_codemeta_file('{"version": "9.9.9"}', RELEASE_VERSION, RELEASE_TAG) + is False + ) + + +def test_validate_codemeta_file_rejects_invalid_json(): + assert _validate_codemeta_file("not json", RELEASE_VERSION, RELEASE_TAG) is False + + +# --------------------------------------------------------------------------- +# _validate_changelog_file +# --------------------------------------------------------------------------- + + +def test_validate_changelog_file_accepts_expected_heading_insertion(): + assert ( + _validate_changelog_file( + CHANGELOG_OLD, CHANGELOG_NEW, RELEASE_VERSION, "development version" + ) + is True + ) + + +def test_validate_changelog_file_rejects_unrelated_bullet_insertion(): + tampered = CHANGELOG_OLD + "- an unrelated bullet\n" assert ( - check_patch_is_version_bump(INVALID_CHANGELOG_PATCH, values, "0.7.1") is False + _validate_changelog_file( + CHANGELOG_OLD, tampered, RELEASE_VERSION, "development version" + ) + is False ) -def test_check_patch_is_version_bump_rejects_version_not_matching_release(): - # Looks like a version bump, but doesn't match the fetched release's version. - patch = "@@ -1 +1 @@\n-0.7.0-dev\n+0.9.9-dev\n" +def test_validate_changelog_file_rejects_missing_dev_header(): + old_without_header = "- a bullet\n" + new_with_insertion = "## Tools 0.7.1\n\n- a bullet\n" + assert ( + _validate_changelog_file( + old_without_header, + new_with_insertion, + RELEASE_VERSION, + "development version", + ) + is False + ) + + +def test_validate_changelog_file_rejects_heading_without_release_version(): + tampered = CHANGELOG_NEW.replace("## Tools 0.7.1", "## Tools 9.9.9") + assert ( + _validate_changelog_file( + CHANGELOG_OLD, tampered, RELEASE_VERSION, "development version" + ) + is False + ) + + +def test_validate_changelog_file_rejects_missing_content(): + assert ( + _validate_changelog_file( + None, CHANGELOG_NEW, RELEASE_VERSION, "development version" + ) + is False + ) + + +# --------------------------------------------------------------------------- +# _validate_readme_file +# --------------------------------------------------------------------------- + + +def test_validate_readme_file_accepts_token_bumps(): values = _release_bump_values(RELEASE_TAG, RELEASE) - assert check_patch_is_version_bump(patch, values, "0.7.1") is False + assert _validate_readme_file(README_OLD, README_NEW, values) is True -def test_check_patch_is_version_bump_returns_true_for_empty_patch(): - assert check_patch_is_version_bump("", {"0.7.1"}, "0.7.1") is True +def test_validate_readme_file_rejects_inserted_lines(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + tampered = README_NEW + "Imports: unsafe-package (>= 0.7.1)\n" + assert _validate_readme_file(README_OLD, tampered, values) is False -def test_check_patch_is_version_bump_ignores_file_header_lines(): - patch = "--- a/VERSION\n+++ b/VERSION\n@@ -1 +1 @@\n-0.7.0-dev\n+0.7.1-dev\n" - assert check_patch_is_version_bump(patch, {"0.7.1", "0.7.1-dev"}, "0.7.1") is True +def test_validate_readme_file_rejects_unrelated_line_change(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + tampered = README_NEW.replace("Sovacool K.", "Someone Else.") + assert _validate_readme_file(README_OLD, tampered, values) is False + + +def test_validate_readme_file_rejects_missing_content(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert _validate_readme_file(None, README_NEW, values) is False # --------------------------------------------------------------------------- -# check_version_date_bumps +# _validate_file_bump dispatch # --------------------------------------------------------------------------- -def test_check_version_date_bumps_returns_true_for_valid_pr_files(): +def test_validate_file_bump_dispatches_to_version_validator(): values = _release_bump_values(RELEASE_TAG, RELEASE) - assert check_version_date_bumps(_pr_files(), values, "0.7.1") is True + assert ( + _validate_file_bump( + "version", + VERSION_OLD, + VERSION_NEW, + RELEASE_TAG, + RELEASE_VERSION, + "development version", + values, + ) + is True + ) + + +def test_validate_file_bump_dispatches_to_readme_validator_by_default(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert ( + _validate_file_bump( + "readme", + README_OLD, + README_NEW, + RELEASE_TAG, + RELEASE_VERSION, + "development version", + values, + ) + is True + ) -def test_check_version_date_bumps_returns_false_when_one_file_invalid(): +def test_validate_file_bump_dispatches_to_description_validator(): values = _release_bump_values(RELEASE_TAG, RELEASE) - pr_files = _pr_files() - pr_files[0] = {"filename": "CHANGELOG.md", "patch": INVALID_CHANGELOG_PATCH} - assert check_version_date_bumps(pr_files, values, "0.7.1") is False + assert ( + _validate_file_bump( + "description", + DESCRIPTION_OLD, + DESCRIPTION_NEW, + RELEASE_TAG, + RELEASE_VERSION, + "development version", + values, + ) + is True + ) + + +def test_validate_file_bump_dispatches_to_citation_validator(): + values = _release_bump_values(RELEASE_TAG, RELEASE) + assert ( + _validate_file_bump( + "citation", + CITATION_OLD, + CITATION_NEW, + RELEASE_TAG, + RELEASE_VERSION, + "development version", + values, + ) + is True + ) -def test_check_version_date_bumps_returns_false_when_patch_missing(): +def test_validate_file_bump_dispatches_to_codemeta_validator(): values = _release_bump_values(RELEASE_TAG, RELEASE) - pr_files = [{"filename": "CHANGELOG.md"}] - assert check_version_date_bumps(pr_files, values, "0.7.1") is False + assert ( + _validate_file_bump( + "codemeta", + CODEMETA_OLD, + CODEMETA_NEW, + RELEASE_TAG, + RELEASE_VERSION, + "development version", + values, + ) + is True + ) -def test_check_version_date_bumps_returns_false_for_empty_files(): +def test_validate_file_bump_dispatches_to_changelog_validator(): values = _release_bump_values(RELEASE_TAG, RELEASE) - assert check_version_date_bumps([], values, "0.7.1") is False + assert ( + _validate_file_bump( + "changelog", + CHANGELOG_OLD, + CHANGELOG_NEW, + RELEASE_TAG, + RELEASE_VERSION, + "development version", + values, + ) + is True + ) + + +# --------------------------------------------------------------------------- +# check_files_are_valid_bumps +# --------------------------------------------------------------------------- + + +def test_check_files_are_valid_bumps_returns_true_for_valid_pr(): + session = ContentSession(contents=_default_contents()) + result = check_files_are_valid_bumps( + "CCBR/Tools", + _default_filenames(), + _default_roles(), + RELEASE_TAG, + RELEASE, + "development version", + "base-sha", + "head-sha", + session=session, + ) + assert result is True + + +def test_check_files_are_valid_bumps_returns_false_when_version_file_not_changed(): + filenames = ["README.md"] + session = ContentSession(contents=_default_contents()) + result = check_files_are_valid_bumps( + "CCBR/Tools", + filenames, + _default_roles(), + RELEASE_TAG, + RELEASE, + "development version", + "base-sha", + "head-sha", + session=session, + ) + assert result is False + + +def test_check_files_are_valid_bumps_returns_false_when_one_file_invalid(): + contents = _default_contents() + contents[("CHANGELOG.md", "head-sha")] = CHANGELOG_OLD + "- unrelated bullet\n" + session = ContentSession(contents=contents) + result = check_files_are_valid_bumps( + "CCBR/Tools", + _default_filenames(), + _default_roles(), + RELEASE_TAG, + RELEASE, + "development version", + "base-sha", + "head-sha", + session=session, + ) + assert result is False # --------------------------------------------------------------------------- @@ -429,8 +743,6 @@ def test_determine_post_release_reviewer_uses_last_draft_release_actor(): def test_determine_post_release_reviewer_falls_back_to_default_codeowner(): - import base64 - runs_url = ( "https://api.github.com/repos/CCBR/Tools/actions/workflows/" "draft-release.yml/runs" @@ -472,16 +784,20 @@ def _make_review_session( release_payload=None, existing_reviews=None, action_required_runs=None, + contents=None, + second_pr_node_payload=None, ): - """Build a MockSession suitable for review_post_release_pr tests.""" + """Build a ContentSession suitable for review_post_release_pr tests.""" if pr_files is None: - pr_files = _pr_files() + pr_files = [{"filename": name} for name in _default_filenames()] if pr_node_payload is None: pr_node_payload = { "node_id": "PR_NODE_9", "title": PR_TITLE, "user": {"type": "Bot"}, - "head": {"ref": "release/v0.7.1"}, + "head": {"ref": "release/v0.7.1", "sha": "head-sha"}, + "base": {"sha": "base-sha"}, + "changed_files": len(pr_files), } if graphql_payload is None: graphql_payload = { @@ -493,6 +809,8 @@ def _make_review_session( } if release_payload is None: release_payload = RELEASE + if contents is None: + contents = _default_contents() pr_files_url = "https://api.github.com/repos/CCBR/Tools/pulls/9/files" pr_node_url = "https://api.github.com/repos/CCBR/Tools/pulls/9" @@ -518,7 +836,12 @@ def _make_review_session( runs_url: {"workflow_runs": action_required_runs or []}, draft_runs_url: {"workflow_runs": []}, } - return MockSession(payloads) + session = ContentSession(contents=contents, payloads=payloads) + if second_pr_node_payload is not None: + session.pr_payloads_sequence = { + pr_node_url: [pr_node_payload, second_pr_node_payload] + } + return session def test_review_post_release_pr_approves_when_conditions_met(): @@ -532,6 +855,7 @@ def test_review_post_release_pr_approves_when_conditions_met(): c[2]["json"] for c in session.calls if c[0] == "POST" and "reviews" in c[1] ] assert any(body.get("event") == "APPROVE" for body in review_bodies) + assert any(body.get("commit_id") == "head-sha" for body in review_bodies) def test_review_post_release_pr_approves_pending_workflow_runs(): @@ -548,7 +872,13 @@ def test_review_post_release_pr_approves_pending_workflow_runs(): def test_review_post_release_pr_skips_when_current_review_is_approved(): session = _make_review_session( - existing_reviews=[{"user": {"login": "ccbr-bot"}, "state": "APPROVED"}] + existing_reviews=[ + { + "user": {"login": "ccbr-bot"}, + "state": "APPROVED", + "commit_id": "head-sha", + } + ] ) result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) assert result is True @@ -556,9 +886,31 @@ def test_review_post_release_pr_skips_when_current_review_is_approved(): assert not posted_urls +def test_review_post_release_pr_revalidates_when_approval_is_stale(): + # Approval exists but was left on an earlier commit (e.g. before an + # auto-format push), so it must not count and the PR must be re-validated. + session = _make_review_session( + existing_reviews=[ + {"user": {"login": "ccbr-bot"}, "state": "APPROVED", "commit_id": "old-sha"} + ] + ) + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is True + review_bodies = [ + c[2]["json"] for c in session.calls if c[0] == "POST" and "reviews" in c[1] + ] + assert any(body.get("event") == "APPROVE" for body in review_bodies) + + def test_review_post_release_pr_reviews_again_when_force_review_is_enabled(): session = _make_review_session( - existing_reviews=[{"user": {"login": "ccbr-bot"}, "state": "APPROVED"}] + existing_reviews=[ + { + "user": {"login": "ccbr-bot"}, + "state": "APPROVED", + "commit_id": "head-sha", + } + ] ) result = review_post_release_pr( "CCBR/Tools", 9, force_review=True, token="tok", session=session @@ -569,7 +921,9 @@ def test_review_post_release_pr_reviews_again_when_force_review_is_enabled(): def test_review_post_release_pr_requests_human_review_when_extra_file_changed(): - pr_files = _pr_files() + [{"filename": "src/main.py", "patch": "@@ -1 +1 @@\n"}] + pr_files = [{"filename": name} for name in _default_filenames()] + [ + {"filename": "src/main.py"} + ] session = _make_review_session(pr_files=pr_files) result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) assert result is False @@ -581,9 +935,27 @@ def test_review_post_release_pr_requests_human_review_when_extra_file_changed(): assert any("issues/9/comments" in u for u in posted_urls) -def test_review_post_release_pr_requests_human_review_when_non_bump_change(): - pr_files = _pr_files() - pr_files[0] = {"filename": "CHANGELOG.md", "patch": INVALID_CHANGELOG_PATCH} +def test_review_post_release_pr_requests_human_review_when_file_count_mismatched(): + pr_node_payload = { + "node_id": "PR_NODE_9", + "title": PR_TITLE, + "user": {"type": "Bot"}, + "head": {"ref": "release/v0.7.1", "sha": "head-sha"}, + "base": {"sha": "base-sha"}, + "changed_files": 99, + } + session = _make_review_session(pr_node_payload=pr_node_payload) + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is False + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/9/comments" in c[1] + ] + assert comment_calls + assert "pagination mismatch" in comment_calls[0][2]["json"]["body"] + + +def test_review_post_release_pr_requests_human_review_when_version_not_bumped(): + pr_files = [{"filename": "README.md"}] session = _make_review_session(pr_files=pr_files) result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) assert result is False @@ -591,21 +963,19 @@ def test_review_post_release_pr_requests_human_review_when_non_bump_change(): c for c in session.calls if c[0] == "POST" and "issues/9/comments" in c[1] ] assert comment_calls - assert "requires human review" in comment_calls[0][2]["json"]["body"] + assert "version file must be bumped" in comment_calls[0][2]["json"]["body"] def test_review_post_release_pr_requests_human_review_when_release_not_found(): - class NoReleaseSession(MockSession): - def request(self, method, url, headers=None, **kwargs): - self.calls.append((method, url, kwargs)) - if "releases/tags/" in url: - raise requests_lib.exceptions.HTTPError("404") - status = self.post_status if method == "POST" else 200 - default = {"data": {}} if method == "POST" else {} - return MockResponse(self.payloads.get(url, default), status) - session = _make_review_session() - session.__class__ = NoReleaseSession + original_request = session.request + + def _patched_request(method, url, headers=None, **kwargs): + if "releases/tags/" in url: + raise requests_lib.exceptions.HTTPError("404") + return original_request(method, url, headers=headers, **kwargs) + + session.request = _patched_request result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) assert result is False comment_calls = [ @@ -621,7 +991,9 @@ def test_review_post_release_pr_requests_human_review_when_title_or_sender_misma "node_id": "PR_NODE_9", "title": "chore: bump deps", "user": {"type": "User"}, - "head": {"ref": "some-branch"}, + "head": {"ref": "some-branch", "sha": "head-sha"}, + "base": {"sha": "base-sha"}, + "changed_files": 5, } ) result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) @@ -633,7 +1005,7 @@ def test_review_post_release_pr_requests_human_review_when_title_or_sender_misma assert "post-release cleanup pattern" in comment_calls[0][2]["json"]["body"] -def test_review_post_release_pr_uses_resolved_reviewer_in_comment(): +def test_review_post_release_pr_requests_resolved_reviewer_successfully(): runs_url = ( "https://api.github.com/repos/CCBR/Tools/actions/workflows/" "draft-release.yml/runs" @@ -643,7 +1015,9 @@ def test_review_post_release_pr_uses_resolved_reviewer_in_comment(): "node_id": "PR_NODE_9", "title": "chore: bump deps", "user": {"type": "User"}, - "head": {"ref": "some-branch"}, + "head": {"ref": "some-branch", "sha": "head-sha"}, + "base": {"sha": "base-sha"}, + "changed_files": 5, } ) session.payloads[runs_url] = { @@ -651,10 +1025,6 @@ def test_review_post_release_pr_uses_resolved_reviewer_in_comment(): } result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) assert result is False - comment_calls = [ - c for c in session.calls if c[0] == "POST" and "issues/9/comments" in c[1] - ] - assert "@kelly-sovacool" in comment_calls[0][2]["json"]["body"] reviewer_calls = [ c for c in session.calls if c[0] == "POST" and "requested_reviewers" in c[1] ] @@ -662,6 +1032,34 @@ def test_review_post_release_pr_uses_resolved_reviewer_in_comment(): assert reviewer_calls[0][2]["json"]["reviewers"] == ["kelly-sovacool"] +def test_review_post_release_pr_aborts_approval_when_head_changes_concurrently(): + pr_node_payload = { + "node_id": "PR_NODE_9", + "title": PR_TITLE, + "user": {"type": "Bot"}, + "head": {"ref": "release/v0.7.1", "sha": "head-sha"}, + "base": {"sha": "base-sha"}, + "changed_files": len(_default_filenames()), + } + second_pr_node_payload = dict(pr_node_payload) + second_pr_node_payload["head"] = {"ref": "release/v0.7.1", "sha": "new-head-sha"} + session = _make_review_session( + pr_node_payload=pr_node_payload, + second_pr_node_payload=second_pr_node_payload, + ) + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is False + review_bodies = [ + c[2]["json"] for c in session.calls if c[0] == "POST" and "reviews" in c[1] + ] + assert not any(body.get("event") == "APPROVE" for body in review_bodies) + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/9/comments" in c[1] + ] + assert comment_calls + assert "head commit changed" in comment_calls[0][2]["json"]["body"] + + def test_review_post_release_pr_keeps_approval_when_auto_merge_api_fails(): session = _make_review_session( graphql_payload={ @@ -700,7 +1098,9 @@ def _fail_approval(*args, **kwargs): def test_review_post_release_pr_warns_when_reviewer_request_fails(monkeypatch): - pr_files = _pr_files() + [{"filename": "src/main.py", "patch": "@@ -1 +1 @@\n"}] + pr_files = [{"filename": name} for name in _default_filenames()] + [ + {"filename": "src/main.py"} + ] session = _make_review_session(pr_files=pr_files) def _fail_request(*args, **kwargs): @@ -718,7 +1118,9 @@ def _fail_request(*args, **kwargs): def test_review_post_release_pr_warns_when_human_review_comment_fails(monkeypatch): - pr_files = _pr_files() + [{"filename": "src/main.py", "patch": "@@ -1 +1 @@\n"}] + pr_files = [{"filename": name} for name in _default_filenames()] + [ + {"filename": "src/main.py"} + ] session = _make_review_session(pr_files=pr_files) def _fail_comment(*args, **kwargs): @@ -743,6 +1145,7 @@ def _fail_comment(*args, **kwargs): with pytest.warns(UserWarning, match="Could not post auto-merge failure comment"): result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is True diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index ee9dc27b..a675220e 100644 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -18,6 +18,7 @@ get_last_workflow_run_actor, get_pr_files, is_pr_approved, + is_pr_approved_for_commit, match_codeowners, post_pr_comment, request_changes, @@ -83,6 +84,25 @@ def test_get_pr_files_calls_correct_url(): ) +def test_get_pr_files_paginates_across_full_pages(): + class PaginatedSession(MockSession): + def request(self, method, url, headers=None, **kwargs): + self.calls.append((method, url, kwargs)) + page = kwargs["params"]["page"] + per_page = kwargs["params"]["per_page"] + if page == 1: + payload = [{"filename": f"file{i}.txt"} for i in range(per_page)] + else: + payload = [{"filename": "last.txt"}] + return MockResponse(payload) + + session = PaginatedSession() + result = get_pr_files("CCBR/actions", 42, token="tok", session=session, per_page=2) + assert len(result) == 3 + assert [f["filename"] for f in result] == ["file0.txt", "file1.txt", "last.txt"] + assert len(session.calls) == 2 + + # --------------------------------------------------------------------------- # approve_pr # --------------------------------------------------------------------------- @@ -95,6 +115,15 @@ def test_approve_pr_posts_to_reviews_endpoint(): assert method == "POST" assert url == "https://api.github.com/repos/CCBR/actions/pulls/42/reviews" assert kwargs["json"]["event"] == "APPROVE" + assert "commit_id" not in kwargs["json"] + + +def test_approve_pr_includes_commit_id_when_provided(): + session = MockSession(post_status=200) + approve_pr("CCBR/actions", 42, token="tok", session=session, commit_id="abc123") + method, _url, kwargs = session.calls[0] + assert method == "POST" + assert kwargs["json"]["commit_id"] == "abc123" # --------------------------------------------------------------------------- @@ -186,6 +215,98 @@ def test_is_pr_approved_falls_back_to_user_id_when_login_is_missing(): assert is_pr_approved("CCBR/actions", 42, token="tok", session=session) is True +# --------------------------------------------------------------------------- +# is_pr_approved_for_commit +# --------------------------------------------------------------------------- + + +def test_is_pr_approved_for_commit_returns_true_for_matching_commit(): + reviews_url = "https://api.github.com/repos/CCBR/actions/pulls/42/reviews" + session = MockSession( + { + reviews_url: [ + { + "user": {"login": "ccbr-bot"}, + "state": "APPROVED", + "commit_id": "sha-abc", + } + ] + } + ) + assert ( + is_pr_approved_for_commit( + "CCBR/actions", 42, "sha-abc", token="tok", session=session + ) + is True + ) + + +def test_is_pr_approved_for_commit_returns_false_for_stale_commit(): + reviews_url = "https://api.github.com/repos/CCBR/actions/pulls/42/reviews" + session = MockSession( + { + reviews_url: [ + { + "user": {"login": "ccbr-bot"}, + "state": "APPROVED", + "commit_id": "sha-old", + } + ] + } + ) + assert ( + is_pr_approved_for_commit( + "CCBR/actions", 42, "sha-new", token="tok", session=session + ) + is False + ) + + +def test_is_pr_approved_for_commit_returns_false_when_changes_requested(): + reviews_url = "https://api.github.com/repos/CCBR/actions/pulls/42/reviews" + session = MockSession( + { + reviews_url: [ + { + "user": {"login": "ccbr-bot"}, + "state": "APPROVED", + "commit_id": "sha-abc", + }, + { + "user": {"login": "human"}, + "state": "CHANGES_REQUESTED", + "commit_id": "sha-abc", + }, + ] + } + ) + assert ( + is_pr_approved_for_commit( + "CCBR/actions", 42, "sha-abc", token="tok", session=session + ) + is False + ) + + +def test_is_pr_approved_for_commit_returns_false_for_blank_commit_sha(): + reviews_url = "https://api.github.com/repos/CCBR/actions/pulls/42/reviews" + session = MockSession( + { + reviews_url: [ + { + "user": {"login": "ccbr-bot"}, + "state": "APPROVED", + "commit_id": "", + } + ] + } + ) + assert ( + is_pr_approved_for_commit("CCBR/actions", 42, "", token="tok", session=session) + is False + ) + + # --------------------------------------------------------------------------- # enable_auto_merge # --------------------------------------------------------------------------- From f03714a49deb83ac5d1a652d5d96bcb15555292c Mon Sep 17 00:00:00 2001 From: CCBR-bot <258092125+ccbr-bot@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:56:23 +0000 Subject: [PATCH 13/23] =?UTF-8?q?ci:=20=F0=9F=A4=96=20render=20readme=20&?= =?UTF-8?q?=20format=20everything=20with=20pre-commit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- review-post-release-pr/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/review-post-release-pr/README.md b/review-post-release-pr/README.md index 9898bc48..8fa38db2 100644 --- a/review-post-release-pr/README.md +++ b/review-post-release-pr/README.md @@ -6,7 +6,7 @@ version/date bumps matching a real release tag are present, otherwise request a human reviewer This action automates the review of post-release cleanup pull requests -opened by the [post-release](./post-release) action, with titles of the +opened by the [post-release](/post-release) action, with titles of the form `chore: post-release cleanup for `. The action verifies four conditions: @@ -111,7 +111,7 @@ jobs: pr-number: ${{ github.event.pull_request.number }} ``` -See also [pre-review-pr.yml](./examples/pre-review-pr.yml). +See also [pre-review-pr.yml](/examples/pre-review-pr.yml). ## Inputs From ff1e42682e6be41d46bdb0bf3cec76fc40f6ec32 Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Fri, 25 Sep 2026 14:08:09 -0400 Subject: [PATCH 14/23] fix(review-post-release-pr): avoid duplicate human-review comments/requests Track a per-commit marker in the human-review comment so a synchronize-triggered re-run doesn't spam duplicate comments and reviewer requests for a PR that still fails the same commit's validation. force-review still overrides the skip. _commit message is ai-generated_ --- src/ccbr_actions/post_release_pr.py | 98 ++++++++++++++++++++---- src/ccbr_actions/pr_review.py | 17 +++++ tests/test_post_release_pr.py | 113 ++++++++++++++++++++++++++++ 3 files changed, 213 insertions(+), 15 deletions(-) diff --git a/src/ccbr_actions/post_release_pr.py b/src/ccbr_actions/post_release_pr.py index 1c16b3b7..b86916de 100644 --- a/src/ccbr_actions/post_release_pr.py +++ b/src/ccbr_actions/post_release_pr.py @@ -19,6 +19,7 @@ enable_auto_merge, get_codeowners_content, get_last_workflow_run_actor, + get_pr_comments, get_pr_files, is_pr_approved_for_commit, match_codeowners, @@ -688,8 +689,46 @@ def _enable_merge_or_comment(repo, pr_number, reviewer, token=None, session=None warnings.warn(f"Could not post auto-merge failure comment: {comment_exc}") +def _human_review_marker(head_sha): + """ + Build the hidden marker used to detect a prior human-review comment. + + Args: + head_sha (str): Commit SHA the comment applies to. + + Returns: + str: An HTML comment embedding *head_sha*, invisible when rendered. + """ + return f"" + + +def _has_pending_human_review_comment( + repo, pr_number, head_sha, token=None, session=None +): + """ + Check whether a human-review comment was already posted for *head_sha*. + + Args: + repo (str): Repository full name (e.g. ``"CCBR/actions"``). + pr_number (int | str): Pull request number. + head_sha (str): Commit SHA to check for a prior comment. + token (str, optional): GitHub API token. + session: Requests-compatible session object for dependency injection. + + Returns: + bool: ``True`` if an existing comment already carries the marker for + *head_sha*. + """ + try: + comments = get_pr_comments(repo, pr_number, token=token, session=session) + except requests.exceptions.RequestException: + comments = [] + marker = _human_review_marker(head_sha) + return any(marker in (comment.get("body") or "") for comment in comments) + + def _request_human_review( - repo, pr_number, reviewer, failed_reasons, token=None, session=None + repo, pr_number, reviewer, failed_reasons, head_sha, token=None, session=None ): """ Post a comment explaining failed conditions and request a human reviewer. @@ -700,6 +739,9 @@ def _request_human_review( reviewer (str, optional): Explicit reviewer override. failed_reasons (list[str]): Reasons the PR needs human review, from [](`~ccbr_actions.post_release_pr._collect_failure_reasons`). + head_sha (str): The PR's current head commit SHA, embedded as a + hidden marker so a later run can detect this comment and avoid + posting a duplicate for the same commit. token (str, optional): GitHub API token. session: Requests-compatible session object for dependency injection. """ @@ -713,6 +755,7 @@ def _request_human_review( f"{reviewer_mention}This post-release cleanup PR requires human " "review. The changes were too complex for CCBR-bot to automatically " f"approve because the following conditions were not met:\n{reasons_text}" + f"\n\n{_human_review_marker(head_sha)}" ) try: response = post_pr_comment( @@ -749,6 +792,7 @@ def _review_post_release_pr( changelog_filepath, description_filepath, dev_header, + force_review, pr_data, ): """ @@ -860,18 +904,37 @@ def _review_post_release_pr( if not was_auto_approved: print("Policy result: automatic approval not performed") - failed_reasons = _collect_failure_reasons( - is_post_release, - release, - files_complete, - only_allowed_files, - only_valid_bumps, - approval_error, - ) - _request_human_review( - repo, pr_number, reviewer, failed_reasons, token=token, session=session + already_notified = ( + bool(head_sha) + and not force_review + and _has_pending_human_review_comment( + repo, pr_number, head_sha, token=token, session=session + ) ) - print("Result: human review requested") + if already_notified: + print( + "Result: human review was already requested for this commit; " + "skipping duplicate comment" + ) + else: + failed_reasons = _collect_failure_reasons( + is_post_release, + release, + files_complete, + only_allowed_files, + only_valid_bumps, + approval_error, + ) + _request_human_review( + repo, + pr_number, + reviewer, + failed_reasons, + head_sha, + token=token, + session=session, + ) + print("Result: human review requested") else: print("Result: automatically approved") @@ -926,7 +989,11 @@ def review_post_release_pr( no new review is submitted and the function returns ``True`` immediately unless *force_review* is true. A stale approval left on an earlier commit (e.g. after the ``auto-format`` action pushes a citation rerender) does - not count, so the PR is re-validated. + not count, so the PR is re-validated. Likewise, if a human-review comment + was already posted for the current head commit, no duplicate comment or + reviewer request is submitted (unless *force_review* is true) — this keeps + a `synchronize`-triggered workflow from spamming the PR while the same + unresolved commit is repeatedly re-evaluated. Args: repo (str): Repository full name (e.g. ``"CCBR/actions"``). @@ -937,8 +1004,8 @@ def review_post_release_pr( token (str, optional): GitHub API token. session: Requests-compatible session object for dependency injection. force_review (bool, optional): Re-submit the review and reviewer - request even when the PR already has an approval. Defaults to - ``False``. + request even when the PR already has an approval or was already + notified for the current commit. Defaults to ``False``. version_filepath (str): Path to the version file. Defaults to ``"VERSION"``. citation_filepath (str): Path to the citation file. Defaults to @@ -981,6 +1048,7 @@ def review_post_release_pr( changelog_filepath, description_filepath, dev_header, + force_review, pr_data, ) return result diff --git a/src/ccbr_actions/pr_review.py b/src/ccbr_actions/pr_review.py index d3fc72a7..7f7e7e0b 100644 --- a/src/ccbr_actions/pr_review.py +++ b/src/ccbr_actions/pr_review.py @@ -341,6 +341,23 @@ def request_reviewer(repo, pr_number, reviewer, token=None, session=None): return response +def get_pr_comments(repo, pr_number, token=None, session=None): + """ + Return the list of issue comments on a pull request. + + Args: + repo (str): Repository full name (e.g. ``"CCBR/actions"``). + pr_number (int | str): Pull request number. + token (str, optional): GitHub API token. + session: Requests-compatible session object for dependency injection. + + Returns: + list[dict]: Comment objects from the GitHub issue comments API. + """ + url = f"{GITHUB_API_URL}/repos/{repo}/issues/{pr_number}/comments" + return github_api_get(url=url, token=token, session=session) + + def post_pr_comment(repo, pr_number, comment, token=None, session=None): """ Post a comment on a pull request. diff --git a/tests/test_post_release_pr.py b/tests/test_post_release_pr.py index 37019bc2..58e58195 100644 --- a/tests/test_post_release_pr.py +++ b/tests/test_post_release_pr.py @@ -11,6 +11,8 @@ POST_RELEASE_PR_TITLE_PATTERN, _file_roles, _get_file_content, + _has_pending_human_review_comment, + _human_review_marker, _is_valid_bump_line, _release_bump_values, _split_tokens, @@ -771,6 +773,54 @@ def test_determine_post_release_reviewer_returns_none_when_nothing_resolves(): assert result is None +# --------------------------------------------------------------------------- +# _human_review_marker / _has_pending_human_review_comment +# --------------------------------------------------------------------------- + + +def test_human_review_marker_embeds_commit_sha(): + marker = _human_review_marker("abc123") + assert ( + marker + == "" + ) + + +def test_has_pending_human_review_comment_returns_true_for_matching_marker(): + comments_url = "https://api.github.com/repos/CCBR/Tools/issues/9/comments" + session = MockSession( + {comments_url: [{"body": f"hello\n\n{_human_review_marker('abc123')}"}]} + ) + assert ( + _has_pending_human_review_comment( + "CCBR/Tools", 9, "abc123", token="tok", session=session + ) + is True + ) + + +def test_has_pending_human_review_comment_returns_false_for_different_commit(): + comments_url = "https://api.github.com/repos/CCBR/Tools/issues/9/comments" + session = MockSession( + {comments_url: [{"body": f"hello\n\n{_human_review_marker('old-sha')}"}]} + ) + assert ( + _has_pending_human_review_comment( + "CCBR/Tools", 9, "abc123", token="tok", session=session + ) + is False + ) + + +def test_has_pending_human_review_comment_returns_false_on_request_error(): + assert ( + _has_pending_human_review_comment( + "CCBR/Tools", 9, "abc123", token="tok", session=_RaisingSession() + ) + is False + ) + + # --------------------------------------------------------------------------- # review_post_release_pr # --------------------------------------------------------------------------- @@ -783,6 +833,7 @@ def _make_review_session( graphql_payload=None, release_payload=None, existing_reviews=None, + existing_comments=None, action_required_runs=None, contents=None, second_pr_node_payload=None, @@ -819,6 +870,7 @@ def _make_review_session( reviewers_url = ( "https://api.github.com/repos/CCBR/Tools/pulls/9/requested_reviewers" ) + comments_url = "https://api.github.com/repos/CCBR/Tools/issues/9/comments" release_url = f"https://api.github.com/repos/CCBR/Tools/releases/tags/{RELEASE_TAG}" runs_url = "https://api.github.com/repos/CCBR/Tools/actions/runs" draft_runs_url = ( @@ -832,6 +884,7 @@ def _make_review_session( graphql_url: graphql_payload, reviews_url: existing_reviews if existing_reviews is not None else [], reviewers_url: {}, + comments_url: existing_comments if existing_comments is not None else [], release_url: release_payload, runs_url: {"workflow_runs": action_required_runs or []}, draft_runs_url: {"workflow_runs": []}, @@ -935,6 +988,66 @@ def test_review_post_release_pr_requests_human_review_when_extra_file_changed(): assert any("issues/9/comments" in u for u in posted_urls) +def test_review_post_release_pr_skips_duplicate_human_review_comment_for_same_commit(): + pr_files = [{"filename": name} for name in _default_filenames()] + [ + {"filename": "src/main.py"} + ] + existing_comments = [ + { + "body": ( + "This post-release cleanup PR requires human review.\n\n" + "" + ) + } + ] + session = _make_review_session( + pr_files=pr_files, existing_comments=existing_comments + ) + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is False + posted_urls = [c[1] for c in session.calls if c[0] == "POST"] + assert not any("issues/9/comments" in u for u in posted_urls) + assert not any("requested_reviewers" in u for u in posted_urls) + + +def test_review_post_release_pr_reposts_comment_for_different_commit_marker(): + pr_files = [{"filename": name} for name in _default_filenames()] + [ + {"filename": "src/main.py"} + ] + existing_comments = [ + { + "body": "" + } + ] + session = _make_review_session( + pr_files=pr_files, existing_comments=existing_comments + ) + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is False + posted_urls = [c[1] for c in session.calls if c[0] == "POST"] + assert any("issues/9/comments" in u for u in posted_urls) + + +def test_review_post_release_pr_reposts_duplicate_comment_when_force_review_enabled(): + pr_files = [{"filename": name} for name in _default_filenames()] + [ + {"filename": "src/main.py"} + ] + existing_comments = [ + { + "body": "" + } + ] + session = _make_review_session( + pr_files=pr_files, existing_comments=existing_comments + ) + result = review_post_release_pr( + "CCBR/Tools", 9, force_review=True, token="tok", session=session + ) + assert result is False + posted_urls = [c[1] for c in session.calls if c[0] == "POST"] + assert any("issues/9/comments" in u for u in posted_urls) + + def test_review_post_release_pr_requests_human_review_when_file_count_mismatched(): pr_node_payload = { "node_id": "PR_NODE_9", From a3910c16810e849a36840de2d9c1ee236c697024 Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Fri, 25 Sep 2026 15:44:39 -0400 Subject: [PATCH 15/23] fix(review-post-release-pr): address second round of Copilot review feedback - Tighten readme validation to only accept version/date token bumps within a recognized citation context (version mention or bibtex month/year), not any line whose token coincidentally matches. - Rewrite the codemeta.json validator to compare old vs new content and only permit version/date fields to change, matching the release. - Require CITATION.cff/codemeta.json date fields to exactly match the release's published/created date instead of only checking the date shape. - Filter approved workflow runs to the validated commit's exact head SHA, not just branch name, and approve them only after the head-SHA concurrency recheck (previously runs could be approved before an aborted approval was detected). - Route PRs with an active human CHANGES_REQUESTED review to human review instead of treating 'not yet approved' as permission to auto-approve. - Fix the version-bump audit for R packages where the version and description files are the same DESCRIPTION file (a validated description role no longer gets silently ignored). - Make the examples test's @main exception for unreleased actions self-expiring, tied to the release it was written against. _commit message is ai-generated_ --- review-post-release-pr/README.md | 36 +++-- review-post-release-pr/README.qmd | 27 ++-- src/ccbr_actions/post_release_pr.py | 217 +++++++++++++++++++++------- src/ccbr_actions/pr_review.py | 35 ++++- tests/test_examples.py | 11 +- tests/test_post_release_pr.py | 169 ++++++++++++++++++++-- tests/test_pr_review.py | 77 ++++++++++ 7 files changed, 485 insertions(+), 87 deletions(-) diff --git a/review-post-release-pr/README.md b/review-post-release-pr/README.md index 8fa38db2..ab6a4427 100644 --- a/review-post-release-pr/README.md +++ b/review-post-release-pr/README.md @@ -6,10 +6,10 @@ version/date bumps matching a real release tag are present, otherwise request a human reviewer This action automates the review of post-release cleanup pull requests -opened by the [post-release](/post-release) action, with titles of the +opened by the [post-release](./post-release) action, with titles of the form `chore: post-release cleanup for `. -The action verifies four conditions: +The action verifies five conditions: 1. **The PR matches the post-release cleanup pattern** – the title matches `chore: post-release cleanup for `, the sender is a @@ -22,22 +22,32 @@ The action verifies four conditions: pagination mismatch is detected). 3. **The version file was actually bumped** – auditing that the `post-release` action did its job, not just that the changed files - are individually allowed. + are individually allowed. For an R package where the version and + description files are the same `DESCRIPTION` file, a validated + description bump satisfies this condition. 4. **Every changed file’s new content is a valid bump for its role** – the version file must match the expected dev version exactly; - `CITATION.cff` and `codemeta.json` must match the release - tag/version with no other fields changed; the changelog/news file - must have gained only the expected release heading; and readme files - may only have existing version/date tokens replaced (no inserted - lines), matching a re-rendered citation snippet from the - `auto-format` action. + `CITATION.cff` and `codemeta.json` may only change their + version/date fields, matching the release tag and the release’s + actual published/created date exactly; the changelog/news file must + have gained only the expected release heading; and readme files may + only have existing version/date tokens replaced in a recognized + citation context (a “version” mention or a bibtex `month`/`year` + field, with no inserted lines), matching a re-rendered citation + snippet from the `auto-format` action. +5. **No reviewer currently has changes requested** – an active + `CHANGES_REQUESTED` review blocks automatic approval even if the + file checks above pass, so CCBR-bot never silently overrides a + human’s request. When all conditions are satisfied the action: -- Approves any workflow runs pending approval on the PR’s head branch. -- Re-checks the PR’s head commit immediately before approving, aborting - if it changed since validation (e.g. a concurrent push), to avoid +- Re-checks the PR’s head commit immediately before acting, aborting if + it changed since validation (e.g. a concurrent push), to avoid approving unreviewed changes. +- Approves workflow runs pending approval on the PR’s head branch, + filtered to the validated commit so an unrelated run sharing the + branch name isn’t approved. - Approves the PR, pinned to the validated commit, as the token’s actor (typically CCBR-bot). - Attempts to enable squash auto-merge so the PR merges automatically @@ -111,7 +121,7 @@ jobs: pr-number: ${{ github.event.pull_request.number }} ``` -See also [pre-review-pr.yml](/examples/pre-review-pr.yml). +See also [pre-review-pr.yml](./examples/pre-review-pr.yml). ## Inputs diff --git a/review-post-release-pr/README.qmd b/review-post-release-pr/README.qmd index 442b3b3c..e0c084cc 100644 --- a/review-post-release-pr/README.qmd +++ b/review-post-release-pr/README.qmd @@ -18,7 +18,7 @@ This action automates the review of post-release cleanup pull requests opened by the [post-release](/post-release) action, with titles of the form `chore: post-release cleanup for `. -The action verifies four conditions: +The action verifies five conditions: 1. **The PR matches the post-release cleanup pattern** – the title matches `chore: post-release cleanup for `, the sender is a GitHub App (Bot), @@ -29,21 +29,30 @@ The action verifies four conditions: retrievable (the action fails closed if a pagination mismatch is detected). 3. **The version file was actually bumped** – auditing that the `post-release` action did its job, not just that the changed files are - individually allowed. + individually allowed. For an R package where the version and description + files are the same `DESCRIPTION` file, a validated description bump + satisfies this condition. 4. **Every changed file's new content is a valid bump for its role** – the version file must match the expected dev version exactly; `CITATION.cff` - and `codemeta.json` must match the release tag/version with no other - fields changed; the changelog/news file must have gained only the expected - release heading; and readme files may only have existing version/date - tokens replaced (no inserted lines), matching a re-rendered citation - snippet from the `auto-format` action. + and `codemeta.json` may only change their version/date fields, matching + the release tag and the release's actual published/created date exactly; + the changelog/news file must have gained only the expected release + heading; and readme files may only have existing version/date tokens + replaced in a recognized citation context (a "version" mention or a + bibtex `month`/`year` field, with no inserted lines), matching a + re-rendered citation snippet from the `auto-format` action. +5. **No reviewer currently has changes requested** – an active + `CHANGES_REQUESTED` review blocks automatic approval even if the file + checks above pass, so CCBR-bot never silently overrides a human's request. When all conditions are satisfied the action: -- Approves any workflow runs pending approval on the PR's head branch. -- Re-checks the PR's head commit immediately before approving, aborting if it +- Re-checks the PR's head commit immediately before acting, aborting if it changed since validation (e.g. a concurrent push), to avoid approving unreviewed changes. +- Approves workflow runs pending approval on the PR's head branch, filtered to + the validated commit so an unrelated run sharing the branch name isn't + approved. - Approves the PR, pinned to the validated commit, as the token's actor (typically CCBR-bot). - Attempts to enable squash auto-merge so the PR merges automatically once all diff --git a/src/ccbr_actions/post_release_pr.py b/src/ccbr_actions/post_release_pr.py index b86916de..0575c5b1 100644 --- a/src/ccbr_actions/post_release_pr.py +++ b/src/ccbr_actions/post_release_pr.py @@ -21,6 +21,7 @@ get_last_workflow_run_actor, get_pr_comments, get_pr_files, + has_changes_requested, is_pr_approved_for_commit, match_codeowners, post_pr_comment, @@ -151,6 +152,26 @@ def check_only_allowed_files_changed(pr_files, roles): ) +def _release_dates(release): + """ + Build the set of valid ISO release dates (``YYYY-MM-DD``) for a release. + + Args: + release (dict): Release object from + [](`~ccbr_actions.post_release_pr.get_release_by_tag`). + + Returns: + set[str]: The release's ``published_at``/``created_at`` dates, since + a citation date set at draft time may match either. + """ + dates = set() + for date_key in ("published_at", "created_at"): + date_part = (release.get(date_key) or "")[:10] + if len(date_part) == 10 and date_part[4] == "-" and date_part[7] == "-": + dates.add(date_part) + return dates + + def _release_bump_values(release_tag, release): """ Build the set of token values allowed as a version/date bump target. @@ -168,11 +189,9 @@ def _release_bump_values(release_tag, release): """ version = release_tag.lstrip("v") values = {release_tag, version, f"{version}-dev"} - for date_key in ("published_at", "created_at"): - date_part = (release.get(date_key) or "")[:10] - if len(date_part) == 10 and date_part[4] == "-" and date_part[7] == "-": - year, month, day = date_part.split("-") - values.update({date_part, str(int(year)), str(int(month)), str(int(day))}) + for date_part in _release_dates(release): + year, month, day = date_part.split("-") + values.update({date_part, str(int(year)), str(int(month)), str(int(day))}) return values @@ -216,6 +235,36 @@ def _is_valid_bump_line(old_line, new_line, valid_values): return result +# A README's citation snippet is the only prose expected to carry release +# tokens; requiring one of these keywords near a changed line keeps an +# unrelated number (e.g. an issue reference or dependency pin) that happens +# to match a release token from being accepted as a "bump". +_README_BUMP_CONTEXT_PATTERN = re.compile( + r"version|\bmonth\s*=\s*\{|\byear\s*=\s*\{|date-released", re.IGNORECASE +) + + +def _is_valid_readme_bump_line(old_line, new_line, valid_values): + """ + Check whether a readme line is a valid, citation-context version/date bump. + + Args: + old_line (str): The line's previous content. + new_line (str): The line's new content. + valid_values (set[str]): Acceptable new token values, from + [](`~ccbr_actions.post_release_pr._release_bump_values`). + + Returns: + bool: ``True`` if the line is an unchanged line, or a valid bump + occurring in a recognized citation context (a "version" mention or a + bibtex ``month``/``year`` field). + """ + return old_line == new_line or ( + bool(_README_BUMP_CONTEXT_PATTERN.search(new_line)) + and _is_valid_bump_line(old_line, new_line, valid_values) + ) + + def _get_file_content(repo, path, ref, token=None, session=None): """ Fetch a file's decoded text content at a specific git ref. @@ -301,7 +350,7 @@ def _validate_description_file(old_content, new_content, release_version): return result -def _validate_citation_file(old_content, new_content, release_tag): +def _validate_citation_file(old_content, new_content, release_tag, release_dates): """ Check that a ``CITATION.cff`` file's version/date fields were bumped. @@ -309,11 +358,12 @@ def _validate_citation_file(old_content, new_content, release_tag): old_content (str | None): The file's content at the PR's base. new_content (str | None): The file's content at the PR's head. release_tag (str): The release tag (e.g. ``"v0.7.1"``). + release_dates (set[str]): Acceptable ``date-released`` values, from + [](`~ccbr_actions.post_release_pr._release_dates`). Returns: bool: ``True`` if only ``version``/``date-released`` changed, the new - version equals *release_tag*, and the new date is a valid - ``YYYY-MM-DD`` string. + version equals *release_tag*, and the new date matches the release. """ try: old_data = yaml.safe_load(old_content) if old_content else {} @@ -328,39 +378,58 @@ def _validate_citation_file(old_content, new_content, release_tag): fields_unchanged = all( old_data.get(k) == new_data.get(k) for k in unchanged_keys ) - new_date = new_data.get("date-released") version_ok = new_data.get("version") == release_tag - date_ok = bool(re.fullmatch(r"\d{4}-\d{2}-\d{2}", str(new_date))) + date_ok = str(new_data.get("date-released")) in release_dates result = fields_unchanged and version_ok and date_ok return result -def _validate_codemeta_file(new_content, release_version, release_tag): +def _validate_codemeta_file( + old_content, new_content, release_version, release_tag, release_dates +): """ - Check that ``codemeta.json``'s version field (if present) matches the release. + Check that ``codemeta.json``'s version/date fields were bumped to the release. Args: + old_content (str | None): The file's content at the PR's base. new_content (str | None): The file's content at the PR's head. release_version (str): The release version (without leading ``v``). release_tag (str): The release tag (e.g. ``"v0.7.1"``). + release_dates (set[str]): Acceptable date field values, from + [](`~ccbr_actions.post_release_pr._release_dates`). Returns: - bool: ``True`` if *new_content* is valid JSON and its ``version`` - field (if any) matches the release. + bool: ``True`` if only version/date-like fields changed between + *old_content* and *new_content*, and the new values match the + release. """ try: + old_data = json.loads(old_content) if old_content else {} new_data = json.loads(new_content) if new_content else None except (json.JSONDecodeError, TypeError): - new_data = None - if not isinstance(new_data, dict): + old_data, new_data = None, None + if not isinstance(old_data, dict) or not isinstance(new_data, dict): result = False else: - version_field = new_data.get("version") - result = version_field is None or version_field in ( + date_keys = {"dateModified", "datePublished", "dateCreated"} + exempt_keys = {"version"} | date_keys + unchanged_keys = (set(old_data) | set(new_data)) - exempt_keys + fields_unchanged = all( + old_data.get(k) == new_data.get(k) for k in unchanged_keys + ) + new_version = new_data.get("version") + version_ok = new_version is None or new_version in ( release_version, release_tag, f"v{release_version}", ) + date_fields_ok = all( + new_data.get(k) is None + or new_data.get(k) == old_data.get(k) + or str(new_data.get(k))[:10] in release_dates + for k in date_keys + ) + result = fields_unchanged and version_ok and date_fields_ok return result @@ -421,7 +490,10 @@ def _validate_readme_file(old_content, new_content, valid_values): Check that a readme file's changes are only version/date token bumps. No lines may be inserted or removed; each changed line must differ from - its previous content only in numeric/version-like tokens. + its previous content only in numeric/version-like tokens, and only within + a recognized citation context (a "version" mention or a bibtex + ``month``/``year`` field), so an unrelated number that happens to match a + release token (e.g. an issue reference) is not accepted. Args: old_content (str | None): The file's content at the PR's base. @@ -430,7 +502,7 @@ def _validate_readme_file(old_content, new_content, valid_values): [](`~ccbr_actions.post_release_pr._release_bump_values`). Returns: - bool: ``True`` if every line is unchanged or a valid token bump. + bool: ``True`` if every line is unchanged or a valid contextual bump. """ if not isinstance(old_content, str) or not isinstance(new_content, str): result = False @@ -438,8 +510,7 @@ def _validate_readme_file(old_content, new_content, valid_values): old_lines = old_content.splitlines() new_lines = new_content.splitlines() result = len(old_lines) == len(new_lines) and all( - old_line == new_line - or _is_valid_bump_line(old_line, new_line, valid_values) + _is_valid_readme_bump_line(old_line, new_line, valid_values) for old_line, new_line in zip(old_lines, new_lines) ) return result @@ -451,6 +522,7 @@ def _validate_file_bump( new_content, release_tag, release_version, + release_dates, dev_header, valid_values, ): @@ -464,6 +536,8 @@ def _validate_file_bump( new_content (str | None): The file's content at the PR's head. release_tag (str): The release tag (e.g. ``"v0.7.1"``). release_version (str): The release version (without leading ``v``). + release_dates (set[str]): Acceptable release date values, from + [](`~ccbr_actions.post_release_pr._release_dates`). dev_header (str): Development-version header text for changelog files. valid_values (set[str]): Acceptable new token values for readme files. @@ -475,9 +549,13 @@ def _validate_file_bump( elif role == "description": result = _validate_description_file(old_content, new_content, release_version) elif role == "citation": - result = _validate_citation_file(old_content, new_content, release_tag) + result = _validate_citation_file( + old_content, new_content, release_tag, release_dates + ) elif role == "codemeta": - result = _validate_codemeta_file(new_content, release_version, release_tag) + result = _validate_codemeta_file( + old_content, new_content, release_version, release_tag, release_dates + ) elif role == "changelog": result = _validate_changelog_file( old_content, new_content, release_version, dev_header @@ -496,6 +574,8 @@ def check_files_are_valid_bumps( dev_header, base_sha, head_sha, + version_filepath, + description_filepath, token=None, session=None, ): @@ -518,6 +598,10 @@ def check_files_are_valid_bumps( dev_header (str): Development-version header text for changelog files. base_sha (str): Commit SHA to read "old" file contents at. head_sha (str): Commit SHA to read "new" file contents at. + version_filepath (str): Path to the version file. + description_filepath (str): Path to the R DESCRIPTION file. For R + packages this is also the version file, so a validated + ``description`` bump counts as the required version bump. token (str, optional): GitHub API token. session: Requests-compatible session object for dependency injection. @@ -526,12 +610,14 @@ def check_files_are_valid_bumps( every changed file's new content is a valid bump for its role. """ release_version = release_tag.lstrip("v") + release_dates = _release_dates(release) valid_values = _release_bump_values(release_tag, release) - version_basename = next( - (basename for basename, role in roles.items() if role == "version"), None - ) + version_basenames = { + os.path.basename(version_filepath), + os.path.basename(description_filepath), + } version_bumped = any( - os.path.basename(filename) == version_basename for filename in filenames + os.path.basename(filename) in version_basenames for filename in filenames ) files_valid = True for filename in filenames: @@ -549,6 +635,7 @@ def check_files_are_valid_bumps( new_content, release_tag, release_version, + release_dates, dev_header, valid_values, ) @@ -608,6 +695,7 @@ def _collect_failure_reasons( files_complete, only_allowed_files, only_valid_bumps, + changes_requested, approval_error, ): """ @@ -622,6 +710,8 @@ def _collect_failure_reasons( only_allowed_files (bool): Whether only allowed files were changed. only_valid_bumps (bool): Whether the version file was bumped and all changes were valid bumps. + changes_requested (bool): Whether a reviewer currently has an active + CHANGES_REQUESTED review. approval_error (Exception | None): The error raised while attempting automatic approval, if any. @@ -652,6 +742,11 @@ def _collect_failure_reasons( "valid version/date bump matching the release tag, but " "validation failed" ) + if changes_requested: + reasons.append( + "a reviewer currently has changes requested on this PR; automatic " + "approval is skipped until that review is resolved" + ) if approval_error is not None: reasons.append( "automatic approval could not be completed because of a GitHub " @@ -847,29 +942,29 @@ def _review_post_release_pr( dev_header, base_sha, head_sha, + version_filepath, + description_filepath, token=token, session=session, ) + changes_requested = has_changes_requested( + repo, pr_number, token=token, session=session + ) print(f"Only allowed files changed: {only_allowed_files}") print(f"Version bumped and all changes valid: {only_valid_bumps}") + print(f"A reviewer currently has changes requested: {changes_requested}") eligible = ( is_post_release and release is not None and only_allowed_files and only_valid_bumps + and not changes_requested ) approval_error = None was_auto_approved = False if eligible: print("Policy result: eligible for automatic approval") - try: - approve_pending_workflow_runs(repo, head_ref, token=token, session=session) - print("Approved pending workflow runs (if any)") - except (KeyError, requests.exceptions.RequestException, RuntimeError) as exc: - print(f"Could not approve pending workflow runs: {exc}") - warnings.warn(f"Could not approve pending workflow runs: {exc}") - current_pr_data = github_api_get( url=f"{GITHUB_API_URL}/repos/{repo}/pulls/{pr_number}", token=token, @@ -884,6 +979,18 @@ def _review_post_release_pr( ) print(f"Aborting approval: {approval_error}") else: + try: + approve_pending_workflow_runs( + repo, head_ref, head_sha=head_sha, token=token, session=session + ) + print("Approved pending workflow runs (if any)") + except ( + KeyError, + requests.exceptions.RequestException, + RuntimeError, + ) as exc: + print(f"Could not approve pending workflow runs: {exc}") + warnings.warn(f"Could not approve pending workflow runs: {exc}") try: response = approve_pr( repo, pr_number, token=token, session=session, commit_id=head_sha @@ -923,6 +1030,7 @@ def _review_post_release_pr( files_complete, only_allowed_files, only_valid_bumps, + changes_requested, approval_error, ) _request_human_review( @@ -957,7 +1065,7 @@ def review_post_release_pr( """ Evaluate and review a post-release cleanup pull request. - Checks whether the PR satisfies four conditions: + Checks whether the PR satisfies five conditions: - **Condition 1** – the PR title matches the post-release cleanup pattern (``chore: post-release cleanup for ``) and ** @@ -967,22 +1075,31 @@ def review_post_release_pr( version file, citation file, ``codemeta.json``, changelog file, news file, and/or readme files were changed. - **Condition 3** – the version file was actually changed, auditing that - the ``post-release`` action did its job. + the ``post-release`` action did its job. For an R package where the + version and description files are the same ``DESCRIPTION`` file, a + validated description bump satisfies this condition. - **Condition 4** – every changed file's new content is a valid bump for its role: the version file matches the expected dev version exactly; - ``CITATION.cff`` and ``codemeta.json`` match the release tag/version - with no other fields changed; the changelog/news file only gained the - expected release heading; and readme files only had existing - version/date tokens replaced (no inserted lines), matching a - re-rendered citation snippet from the ``auto-format`` action. - - When all conditions are met the function approves any pending workflow - runs on the PR's head branch, re-checks the PR's head commit immediately - before approving (aborting if it changed since validation), approves the - PR pinned to that commit, and attempts to enable squash auto-merge. If - auto-merge cannot be enabled, it leaves a comment with the GitHub API - error. Otherwise it posts a comment explaining why the PR needs manual - review and requests a human reviewer, resolved via + ``CITATION.cff`` and ``codemeta.json`` change only their version/date + fields, matching the release tag and the release's actual + published/created date exactly; the changelog/news file only gained + the expected release heading; and readme files only had existing + version/date tokens replaced in a recognized citation context (a + "version" mention or a bibtex ``month``/``year`` field, with no + inserted lines), matching a re-rendered citation snippet from the + ``auto-format`` action. + - **Condition 5** – no reviewer currently has an active CHANGES_REQUESTED + review; automatic approval is skipped until that review is resolved, + even if the file-level checks above would otherwise pass. + + When all conditions are met the function re-checks the PR's head commit + immediately before acting (aborting if it changed since validation), + approves any pending workflow runs tied to that exact commit on the PR's + head branch, approves the PR pinned to that commit, and attempts to + enable squash auto-merge. If auto-merge cannot be enabled, it leaves a + comment with the GitHub API error. Otherwise it posts a comment + explaining why the PR needs manual review and requests a human reviewer, + resolved via [](`~ccbr_actions.post_release_pr.determine_post_release_reviewer`). If the PR already has an APPROVED review tied to its current head commit, diff --git a/src/ccbr_actions/pr_review.py b/src/ccbr_actions/pr_review.py index 7f7e7e0b..1f5a73be 100644 --- a/src/ccbr_actions/pr_review.py +++ b/src/ccbr_actions/pr_review.py @@ -183,6 +183,27 @@ def is_pr_approved_for_commit(repo, pr_number, commit_sha, token=None, session=N return has_matching_approval and not has_changes_requested +def has_changes_requested(repo, pr_number, token=None, session=None): + """ + Check whether any reviewer currently has an active CHANGES_REQUESTED review. + + Args: + repo (str): Repository full name (e.g. ``"CCBR/actions"``). + pr_number (int | str): Pull request number. + token (str, optional): GitHub API token. + session: Requests-compatible session object for dependency injection. + + Returns: + bool: ``True`` if the latest review from any reviewer is currently + ``CHANGES_REQUESTED``. + """ + reviews = get_pr_reviews(repo, pr_number, token=token, session=session) + latest_reviews = _latest_reviews_by_reviewer(reviews) + return any( + review.get("state") == "CHANGES_REQUESTED" for review in latest_reviews.values() + ) + + def request_changes(repo, pr_number, comment, token=None, session=None): """ Submit a *REQUEST_CHANGES* review on a pull request. @@ -250,7 +271,9 @@ def enable_auto_merge(repo, pr_number, token=None, session=None): ) -def approve_pending_workflow_runs(repo, branch, token=None, session=None): +def approve_pending_workflow_runs( + repo, branch, head_sha=None, token=None, session=None +): """ Approve workflow runs awaiting approval for a branch. @@ -261,6 +284,9 @@ def approve_pending_workflow_runs(repo, branch, token=None, session=None): Args: repo (str): Repository full name (e.g. ``"CCBR/actions"``). branch (str): Branch name (typically a pull request's head ref). + head_sha (str, optional): If given, only approve runs whose + ``head_sha`` matches exactly, so runs from an unrelated PR or an + earlier push that happen to share the branch name are ignored. token (str, optional): GitHub API token. session: Requests-compatible session object for dependency injection. @@ -274,8 +300,13 @@ def approve_pending_workflow_runs(repo, branch, token=None, session=None): session=session, params={"branch": branch, "status": "action_required"}, ) + matching_runs = [ + run + for run in data.get("workflow_runs", []) + if not head_sha or run.get("head_sha") == head_sha + ] approved_run_ids = [] - for run in data.get("workflow_runs", []): + for run in matching_runs: run_id = run["id"] approve_url = f"{GITHUB_API_URL}/repos/{repo}/actions/runs/{run_id}/approve" response = github_api_post(url=approve_url, token=token, session=session) diff --git a/tests/test_examples.py b/tests/test_examples.py index 87641ba6..98a081ae 100644 --- a/tests/test_examples.py +++ b/tests/test_examples.py @@ -14,8 +14,11 @@ CURRENT_RELEASE = f"v{CURRENT_VERSION.removesuffix('-dev')}" ACTION_REFERENCE = re.compile(r"CCBR/actions/([^@\s]+)@([^\s]+)") # Actions with no release containing them yet may reference @main until the -# next release is cut; the ref should be repinned to that release afterward. -UNRELEASED_ACTIONS = {"review-post-release-pr"} +# next release is cut. Each entry records the release this exception was +# written against, so it self-expires: once CURRENT_RELEASE advances past +# that value, the exception no longer applies and @main starts failing this +# test again, forcing the ref to be repinned to the new release. +UNRELEASED_ACTIONS = {"review-post-release-pr": "v0.8.0"} def _iter_steps(value): @@ -43,7 +46,9 @@ def test_examples_use_current_release_refs(): for action_name, ref in references if not ( ref == CURRENT_RELEASE - or (action_name in UNRELEASED_ACTIONS and ref == "main") + or ( + ref == "main" and UNRELEASED_ACTIONS.get(action_name) == CURRENT_RELEASE + ) ) ] assert not unexpected diff --git a/tests/test_post_release_pr.py b/tests/test_post_release_pr.py index 58e58195..5d2abc05 100644 --- a/tests/test_post_release_pr.py +++ b/tests/test_post_release_pr.py @@ -15,6 +15,7 @@ _human_review_marker, _is_valid_bump_line, _release_bump_values, + _release_dates, _split_tokens, _validate_changelog_file, _validate_citation_file, @@ -118,6 +119,7 @@ def request(self, method, url, headers=None, **kwargs): "published_at": "2026-09-17T00:00:00Z", "created_at": "2026-09-17T00:00:00Z", } +RELEASE_DATES = {"2026-09-17"} VERSION_OLD = "0.7.0-dev\n" VERSION_NEW = "0.7.1-dev\n" @@ -298,6 +300,14 @@ def test_release_bump_values_includes_version_and_date_tokens(): assert "9" in values +def test_release_dates_extracts_published_and_created_dates(): + assert _release_dates(RELEASE) == RELEASE_DATES + + +def test_release_dates_ignores_missing_or_malformed_dates(): + assert _release_dates({"published_at": "", "created_at": "not-a-date"}) == set() + + def test_split_tokens_separates_text_and_numeric_tokens(): text, tokens = _split_tokens("version: v0.7.0") assert tokens == ["0.7.0"] @@ -426,29 +436,54 @@ def test_validate_description_file_rejects_line_count_mismatch(): def test_validate_citation_file_accepts_valid_bump(): - assert _validate_citation_file(CITATION_OLD, CITATION_NEW, RELEASE_TAG) is True + assert ( + _validate_citation_file(CITATION_OLD, CITATION_NEW, RELEASE_TAG, RELEASE_DATES) + is True + ) def test_validate_citation_file_rejects_other_field_changes(): tampered = CITATION_NEW.replace("title: Tools", "title: Tampered") - assert _validate_citation_file(CITATION_OLD, tampered, RELEASE_TAG) is False + assert ( + _validate_citation_file(CITATION_OLD, tampered, RELEASE_TAG, RELEASE_DATES) + is False + ) def test_validate_citation_file_rejects_version_not_matching_release(): tampered = CITATION_NEW.replace("version: v0.7.1", "version: v9.9.9") - assert _validate_citation_file(CITATION_OLD, tampered, RELEASE_TAG) is False + assert ( + _validate_citation_file(CITATION_OLD, tampered, RELEASE_TAG, RELEASE_DATES) + is False + ) def test_validate_citation_file_rejects_malformed_date(): tampered = CITATION_NEW.replace( 'date-released: "2026-09-17"', 'date-released: "not-a-date"' ) - assert _validate_citation_file(CITATION_OLD, tampered, RELEASE_TAG) is False + assert ( + _validate_citation_file(CITATION_OLD, tampered, RELEASE_TAG, RELEASE_DATES) + is False + ) + + +def test_validate_citation_file_rejects_date_not_matching_release(): + # Well-formed date, but not the actual release date. + tampered = CITATION_NEW.replace( + 'date-released: "2026-09-17"', 'date-released: "2099-01-01"' + ) + assert ( + _validate_citation_file(CITATION_OLD, tampered, RELEASE_TAG, RELEASE_DATES) + is False + ) def test_validate_citation_file_rejects_invalid_yaml(): assert ( - _validate_citation_file(CITATION_OLD, "not: valid: yaml: [", RELEASE_TAG) + _validate_citation_file( + CITATION_OLD, "not: valid: yaml: [", RELEASE_TAG, RELEASE_DATES + ) is False ) @@ -459,22 +494,53 @@ def test_validate_citation_file_rejects_invalid_yaml(): def test_validate_codemeta_file_accepts_matching_version(): - assert _validate_codemeta_file(CODEMETA_NEW, RELEASE_VERSION, RELEASE_TAG) is True + assert ( + _validate_codemeta_file( + CODEMETA_OLD, CODEMETA_NEW, RELEASE_VERSION, RELEASE_TAG, RELEASE_DATES + ) + is True + ) def test_validate_codemeta_file_accepts_missing_version_field(): - assert _validate_codemeta_file("{}", RELEASE_VERSION, RELEASE_TAG) is True + assert ( + _validate_codemeta_file("{}", "{}", RELEASE_VERSION, RELEASE_TAG, RELEASE_DATES) + is True + ) def test_validate_codemeta_file_rejects_mismatched_version(): assert ( - _validate_codemeta_file('{"version": "9.9.9"}', RELEASE_VERSION, RELEASE_TAG) + _validate_codemeta_file( + CODEMETA_OLD, + '{"version": "9.9.9"}', + RELEASE_VERSION, + RELEASE_TAG, + RELEASE_DATES, + ) is False ) def test_validate_codemeta_file_rejects_invalid_json(): - assert _validate_codemeta_file("not json", RELEASE_VERSION, RELEASE_TAG) is False + assert ( + _validate_codemeta_file( + CODEMETA_OLD, "not json", RELEASE_VERSION, RELEASE_TAG, RELEASE_DATES + ) + is False + ) + + +def test_validate_codemeta_file_rejects_unrelated_field_replacement(): + # A completely different JSON object must not pass just because the old + # content is ignored and the version happens to be absent. + tampered = '{"name": "Totally different metadata", "license": "MIT"}' + assert ( + _validate_codemeta_file( + CODEMETA_OLD, tampered, RELEASE_VERSION, RELEASE_TAG, RELEASE_DATES + ) + is False + ) # --------------------------------------------------------------------------- @@ -561,6 +627,16 @@ def test_validate_readme_file_rejects_missing_content(): assert _validate_readme_file(None, README_NEW, values) is False +def test_validate_readme_file_rejects_bump_outside_citation_context(): + # "9" happens to equal the release month, but this line has nothing to do + # with the citation snippet (e.g. an issue reference), so it must not + # pass just because the token is individually valid. + values = _release_bump_values(RELEASE_TAG, RELEASE) + old = "Fixes #6 for details.\n" + README_OLD + new = "Fixes #9 for details.\n" + README_OLD + assert _validate_readme_file(old, new, values) is False + + # --------------------------------------------------------------------------- # _validate_file_bump dispatch # --------------------------------------------------------------------------- @@ -575,6 +651,7 @@ def test_validate_file_bump_dispatches_to_version_validator(): VERSION_NEW, RELEASE_TAG, RELEASE_VERSION, + RELEASE_DATES, "development version", values, ) @@ -591,6 +668,7 @@ def test_validate_file_bump_dispatches_to_readme_validator_by_default(): README_NEW, RELEASE_TAG, RELEASE_VERSION, + RELEASE_DATES, "development version", values, ) @@ -607,6 +685,7 @@ def test_validate_file_bump_dispatches_to_description_validator(): DESCRIPTION_NEW, RELEASE_TAG, RELEASE_VERSION, + RELEASE_DATES, "development version", values, ) @@ -623,6 +702,7 @@ def test_validate_file_bump_dispatches_to_citation_validator(): CITATION_NEW, RELEASE_TAG, RELEASE_VERSION, + RELEASE_DATES, "development version", values, ) @@ -639,6 +719,7 @@ def test_validate_file_bump_dispatches_to_codemeta_validator(): CODEMETA_NEW, RELEASE_TAG, RELEASE_VERSION, + RELEASE_DATES, "development version", values, ) @@ -655,6 +736,7 @@ def test_validate_file_bump_dispatches_to_changelog_validator(): CHANGELOG_NEW, RELEASE_TAG, RELEASE_VERSION, + RELEASE_DATES, "development version", values, ) @@ -678,6 +760,8 @@ def test_check_files_are_valid_bumps_returns_true_for_valid_pr(): "development version", "base-sha", "head-sha", + "VERSION", + "DESCRIPTION", session=session, ) assert result is True @@ -695,6 +779,8 @@ def test_check_files_are_valid_bumps_returns_false_when_version_file_not_changed "development version", "base-sha", "head-sha", + "VERSION", + "DESCRIPTION", session=session, ) assert result is False @@ -713,11 +799,39 @@ def test_check_files_are_valid_bumps_returns_false_when_one_file_invalid(): "development version", "base-sha", "head-sha", + "VERSION", + "DESCRIPTION", session=session, ) assert result is False +def test_check_files_are_valid_bumps_counts_r_description_bump_as_version_bump(): + # For an R package, version-filepath and description-filepath are both + # DESCRIPTION; a validated description bump must satisfy the version-bump + # audit even though no basename maps to the "version" role. + roles = _file_roles("DESCRIPTION", "CITATION.cff", "CHANGELOG.md", "DESCRIPTION") + contents = { + ("DESCRIPTION", "base-sha"): DESCRIPTION_OLD, + ("DESCRIPTION", "head-sha"): DESCRIPTION_NEW, + } + session = ContentSession(contents=contents) + result = check_files_are_valid_bumps( + "CCBR/Tools", + ["DESCRIPTION"], + roles, + RELEASE_TAG, + RELEASE, + "development version", + "base-sha", + "head-sha", + "DESCRIPTION", + "DESCRIPTION", + session=session, + ) + assert result is True + + # --------------------------------------------------------------------------- # determine_post_release_reviewer # --------------------------------------------------------------------------- @@ -911,8 +1025,29 @@ def test_review_post_release_pr_approves_when_conditions_met(): assert any(body.get("commit_id") == "head-sha" for body in review_bodies) +def test_review_post_release_pr_defers_to_active_changes_requested_review(): + # Even though the file checks would otherwise pass, an active human + # CHANGES_REQUESTED review must block automatic approval. + session = _make_review_session( + existing_reviews=[{"user": {"login": "human"}, "state": "CHANGES_REQUESTED"}] + ) + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is False + review_bodies = [ + c[2]["json"] for c in session.calls if c[0] == "POST" and "reviews" in c[1] + ] + assert not any(body.get("event") == "APPROVE" for body in review_bodies) + comment_calls = [ + c for c in session.calls if c[0] == "POST" and "issues/9/comments" in c[1] + ] + assert comment_calls + assert "changes requested" in comment_calls[0][2]["json"]["body"] + + def test_review_post_release_pr_approves_pending_workflow_runs(): - session = _make_review_session(action_required_runs=[{"id": 555}]) + session = _make_review_session( + action_required_runs=[{"id": 555, "head_sha": "head-sha"}] + ) result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) assert result is True approve_run_urls = [ @@ -923,6 +1058,20 @@ def test_review_post_release_pr_approves_pending_workflow_runs(): assert approve_run_urls +def test_review_post_release_pr_does_not_approve_workflow_runs_for_other_commits(): + session = _make_review_session( + action_required_runs=[{"id": 555, "head_sha": "unrelated-sha"}] + ) + result = review_post_release_pr("CCBR/Tools", 9, token="tok", session=session) + assert result is True + approve_run_urls = [ + c[1] + for c in session.calls + if c[0] == "POST" and "actions/runs/555/approve" in c[1] + ] + assert not approve_run_urls + + def test_review_post_release_pr_skips_when_current_review_is_approved(): session = _make_review_session( existing_reviews=[ diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index a675220e..2014fb2a 100644 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -17,6 +17,7 @@ get_last_human_committer, get_last_workflow_run_actor, get_pr_files, + has_changes_requested, is_pr_approved, is_pr_approved_for_commit, match_codeowners, @@ -307,6 +308,54 @@ def test_is_pr_approved_for_commit_returns_false_for_blank_commit_sha(): ) +# --------------------------------------------------------------------------- +# has_changes_requested +# --------------------------------------------------------------------------- + + +def test_has_changes_requested_returns_true_when_active(): + reviews_url = "https://api.github.com/repos/CCBR/actions/pulls/42/reviews" + session = MockSession( + {reviews_url: [{"user": {"login": "human"}, "state": "CHANGES_REQUESTED"}]} + ) + assert ( + has_changes_requested("CCBR/actions", 42, token="tok", session=session) is True + ) + + +def test_has_changes_requested_returns_false_when_none_active(): + reviews_url = "https://api.github.com/repos/CCBR/actions/pulls/42/reviews" + session = MockSession( + {reviews_url: [{"user": {"login": "human"}, "state": "APPROVED"}]} + ) + assert ( + has_changes_requested("CCBR/actions", 42, token="tok", session=session) is False + ) + + +def test_has_changes_requested_ignores_superseded_review(): + reviews_url = "https://api.github.com/repos/CCBR/actions/pulls/42/reviews" + session = MockSession( + { + reviews_url: [ + { + "user": {"login": "human"}, + "state": "CHANGES_REQUESTED", + "submitted_at": "2026-09-17T11:00:00Z", + }, + { + "user": {"login": "human"}, + "state": "APPROVED", + "submitted_at": "2026-09-17T12:00:00Z", + }, + ] + } + ) + assert ( + has_changes_requested("CCBR/actions", 42, token="tok", session=session) is False + ) + + # --------------------------------------------------------------------------- # enable_auto_merge # --------------------------------------------------------------------------- @@ -631,6 +680,34 @@ def test_approve_pending_workflow_runs_returns_empty_list_when_none_pending(): assert result == [] +def test_approve_pending_workflow_runs_filters_to_matching_head_sha(): + runs_url = "https://api.github.com/repos/CCBR/actions/actions/runs" + session = MockSession( + { + runs_url: { + "workflow_runs": [ + {"id": 111, "head_sha": "matching-sha"}, + {"id": 222, "head_sha": "unrelated-sha"}, + ] + } + } + ) + result = approve_pending_workflow_runs( + "CCBR/actions", + "release/v1.0.0", + head_sha="matching-sha", + token="tok", + session=session, + ) + assert result == [111] + approve_urls = [c[1] for c in session.calls if c[0] == "POST"] + assert ( + "https://api.github.com/repos/CCBR/actions/actions/runs/111/approve" + in approve_urls + ) + assert not any("runs/222/approve" in u for u in approve_urls) + + # --------------------------------------------------------------------------- # get_last_workflow_run_actor # --------------------------------------------------------------------------- From c5f461ace6301ad5c5273cfb7deff3872cb53ffe Mon Sep 17 00:00:00 2001 From: CCBR-bot <258092125+ccbr-bot@users.noreply.github.com> Date: Fri, 25 Sep 2026 19:49:10 +0000 Subject: [PATCH 16/23] =?UTF-8?q?ci:=20=F0=9F=A4=96=20render=20readme=20&?= =?UTF-8?q?=20format=20everything=20with=20pre-commit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- review-post-release-pr/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/review-post-release-pr/README.md b/review-post-release-pr/README.md index ab6a4427..d280cb46 100644 --- a/review-post-release-pr/README.md +++ b/review-post-release-pr/README.md @@ -6,7 +6,7 @@ version/date bumps matching a real release tag are present, otherwise request a human reviewer This action automates the review of post-release cleanup pull requests -opened by the [post-release](./post-release) action, with titles of the +opened by the [post-release](/post-release) action, with titles of the form `chore: post-release cleanup for `. The action verifies five conditions: @@ -121,7 +121,7 @@ jobs: pr-number: ${{ github.event.pull_request.number }} ``` -See also [pre-review-pr.yml](./examples/pre-review-pr.yml). +See also [pre-review-pr.yml](/examples/pre-review-pr.yml). ## Inputs From 05f89375b5298d9b26e80c785854ce67c154bc1b Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Fri, 25 Sep 2026 16:57:23 -0400 Subject: [PATCH 17/23] ci: use new pre-review action --- .github/workflows/pre-review-pr.yml | 152 +++++++++++++++++++++ .github/workflows/review-pre-commit-pr.yml | 71 ---------- 2 files changed, 152 insertions(+), 71 deletions(-) create mode 100644 .github/workflows/pre-review-pr.yml delete mode 100644 .github/workflows/review-pre-commit-pr.yml diff --git a/.github/workflows/pre-review-pr.yml b/.github/workflows/pre-review-pr.yml new file mode 100644 index 00000000..f0e454e5 --- /dev/null +++ b/.github/workflows/pre-review-pr.yml @@ -0,0 +1,152 @@ +name: pre-review-pr + +on: + pull_request: + types: + - opened + - review_requested + - synchronize + # Manually re-scan open PRs for any that meet the pre-commit.ci autoupdate criteria + workflow_dispatch: + inputs: + force-review: + description: Re-submit reviews for PRs that already have an approval + required: false + default: false + type: boolean + +permissions: + actions: write + contents: write + issues: write + pull-requests: write + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + find-prec-prs: + # Only needed when manually dispatched; the pull_request event already has a single PR to act on + if: github.event_name == 'workflow_dispatch' + runs-on: ubuntu-latest + outputs: + pr-numbers: ${{ steps.search.outputs.pr-numbers }} + steps: + - name: Search for open pre-commit.ci autoupdate PRs + id: search + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + pr_numbers=$(gh pr list \ + --repo "${{ github.repository }}" \ + --state open \ + --json number,title,author \ + --jq '[.[] | select(.title == "[pre-commit.ci] pre-commit autoupdate" and .author.is_bot)] | map(.number)') + echo "pr-numbers=${pr_numbers}" >> "$GITHUB_OUTPUT" + + review-pre-commit-pr: + needs: [find-prec-prs] + runs-on: ubuntu-latest + container: nciccbr/ccbr_actions:latest + # Only run for pre-commit.ci autoupdate PRs or when ccbr-bot[bot] is requested as a reviewer + if: > + always() && + ( + ( + github.event_name == 'pull_request' && + github.event.pull_request.title == '[pre-commit.ci] pre-commit autoupdate' && + ( + (github.event.action == 'opened' && github.event.sender.type == 'Bot') || + (github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'ccbr-bot[bot]') + ) + ) + || (github.event_name == 'workflow_dispatch' && needs.find-prec-prs.outputs.pr-numbers != '[]') + ) + strategy: + fail-fast: false + matrix: + pr-number: ${{ github.event_name == 'workflow_dispatch' && fromJson(needs.find-prec-prs.outputs.pr-numbers) || fromJson(format('[{0}]', github.event.pull_request.number)) }} + + steps: + - uses: actions/checkout@v7 + - name: Generate CCBR-bot token + id: generate-token + uses: actions/create-github-app-token@v3 + with: + client-id: ${{ vars.CCBR_BOT_APP_ID }} + private-key: ${{ secrets.CCBR_BOT_PRIVATE_KEY }} + owner: ${{ github.repository_owner }} + permission-contents: write + permission-issues: write + permission-pull-requests: write + - uses: CCBR/actions/review-pre-commit-pr@main + with: + github-token: ${{ steps.generate-token.outputs.token }} + pr-number: ${{ matrix.pr-number }} + repo: ${{ github.repository }} + reviewer: CCBR/adminteam + force-review: ${{ github.event.action == 'review_requested' || inputs.force-review || false }} + + find-post-release-prs: + # Only needed when manually dispatched; the pull_request event already has a single PR to act on + if: github.event_name == 'workflow_dispatch' + runs-on: ubuntu-latest + outputs: + pr-numbers: ${{ steps.search.outputs.pr-numbers }} + steps: + - name: Search for open post-release cleanup PRs + id: search + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + pr_numbers=$(gh pr list \ + --repo "${{ github.repository }}" \ + --state open \ + --json number,title,author \ + --jq '[.[] | select(.title | startswith("chore: post-release cleanup for")) | select(.author.is_bot)] | map(.number)') + echo "pr-numbers=${pr_numbers}" >> "$GITHUB_OUTPUT" + + review-post-release-pr: + needs: [find-post-release-prs] + runs-on: ubuntu-latest + container: nciccbr/ccbr_actions:latest + # Only run for post-release cleanup PRs or when ccbr-bot[bot] is requested as a reviewer + if: > + always() && + ( + ( + github.event_name == 'pull_request' && + startsWith(github.event.pull_request.title, 'chore: post-release cleanup for') && + ( + github.event.action == 'synchronize' || + (github.event.action == 'opened' && github.event.sender.type == 'Bot') || + (github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'ccbr-bot[bot]') + ) + ) + || (github.event_name == 'workflow_dispatch' && needs.find-post-release-prs.outputs.pr-numbers != '[]') + ) + strategy: + fail-fast: false + matrix: + pr-number: ${{ github.event_name == 'workflow_dispatch' && fromJson(needs.find-post-release-prs.outputs.pr-numbers) || fromJson(format('[{0}]', github.event.pull_request.number)) }} + + steps: + - uses: actions/checkout@v7 + - name: Generate CCBR-bot token + id: generate-token + uses: actions/create-github-app-token@v3 + with: + client-id: ${{ vars.CCBR_BOT_APP_ID }} + private-key: ${{ secrets.CCBR_BOT_PRIVATE_KEY }} + owner: ${{ github.repository_owner }} + permission-actions: write + permission-contents: write + permission-issues: write + permission-pull-requests: write + - uses: CCBR/actions/review-post-release-pr@main + with: + github-token: ${{ steps.generate-token.outputs.token }} + pr-number: ${{ matrix.pr-number }} + repo: ${{ github.repository }} + force-review: ${{ github.event.action == 'review_requested' || inputs.force-review || false }} diff --git a/.github/workflows/review-pre-commit-pr.yml b/.github/workflows/review-pre-commit-pr.yml deleted file mode 100644 index 3e630105..00000000 --- a/.github/workflows/review-pre-commit-pr.yml +++ /dev/null @@ -1,71 +0,0 @@ -name: review-pre-commit-pr - -on: - pull_request: - types: - - opened - # Manually re-scan open PRs for any that meet the pre-commit.ci autoupdate criteria - workflow_dispatch: - -permissions: - contents: write - pull-requests: write - -concurrency: - group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true - -jobs: - find-prs: - # Only needed when manually dispatched; the pull_request event already has a single PR to act on - if: github.event_name == 'workflow_dispatch' - runs-on: ubuntu-latest - outputs: - pr-numbers: ${{ steps.search.outputs.pr-numbers }} - steps: - - name: Search for open pre-commit.ci autoupdate PRs - id: search - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - pr_numbers=$(gh pr list \ - --repo "${{ github.repository }}" \ - --state open \ - --json number,title,author \ - --jq '[.[] | select(.title == "[pre-commit.ci] pre-commit autoupdate" and .author.is_bot)] | map(.number)') - echo "pr-numbers=${pr_numbers}" >> "$GITHUB_OUTPUT" - - review-pre-commit-pr: - needs: [find-prs] - runs-on: ubuntu-latest - container: nciccbr/ccbr_actions:latest - # Only run for pre-commit.ci autoupdate PRs, whether from the pull_request event or a manual re-scan - if: > - always() && - ( - (github.event_name == 'pull_request' && github.event.pull_request.title == '[pre-commit.ci] pre-commit autoupdate' && github.event.sender.type == 'Bot') - || (github.event_name == 'workflow_dispatch' && needs.find-prs.outputs.pr-numbers != '[]') - ) - strategy: - fail-fast: false - matrix: - pr-number: ${{ github.event_name == 'workflow_dispatch' && fromJson(needs.find-prs.outputs.pr-numbers) || fromJson(format('[{0}]', github.event.pull_request.number)) }} - - steps: - - uses: actions/checkout@v7 - - name: Generate CCBR-bot token - id: generate-token - uses: actions/create-github-app-token@v3 - with: - client-id: ${{ vars.CCBR_BOT_APP_ID }} - private-key: ${{ secrets.CCBR_BOT_PRIVATE_KEY }} - owner: ${{ github.repository_owner }} - permission-contents: write - permission-issues: write - permission-pull-requests: write - - - uses: CCBR/actions/review-pre-commit-pr@latest - with: - github-token: ${{ steps.generate-token.outputs.token }} - pr-number: ${{ matrix.pr-number }} - repo: ${{ github.repository }} From d343bf17100e407974164363a3fbfaa91cc1c9de Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Mon, 28 Sep 2026 12:00:31 -0400 Subject: [PATCH 18/23] fix(review-pr): require bot sender for pull_request synchronize trigger The review-post-release-pr job's synchronize branch had no sender check, so anyone able to push to a PR titled "chore: post-release cleanup for ..." could trigger the elevated CCBR-bot review token. Require sender.type == 'Bot' for opened/synchronize, matching the existing review_requested check. Also restores the bot-sender check in the README "Example" snippets for review-pre-commit-pr and review-post-release-pr, which had been simplified away. _AI-assisted: Claude Sonnet 5 via Copilot_ --- .github/workflows/pre-review-pr.yml | 3 +-- examples/pre-review-pr.yml | 3 +-- review-post-release-pr/README.md | 15 ++++++++++----- review-post-release-pr/README.qmd | 11 ++++++++--- review-pre-commit-pr/README.md | 19 ++++++++++--------- review-pre-commit-pr/README.qmd | 9 +++++++-- 6 files changed, 37 insertions(+), 23 deletions(-) diff --git a/.github/workflows/pre-review-pr.yml b/.github/workflows/pre-review-pr.yml index f0e454e5..5c202901 100644 --- a/.github/workflows/pre-review-pr.yml +++ b/.github/workflows/pre-review-pr.yml @@ -119,8 +119,7 @@ jobs: github.event_name == 'pull_request' && startsWith(github.event.pull_request.title, 'chore: post-release cleanup for') && ( - github.event.action == 'synchronize' || - (github.event.action == 'opened' && github.event.sender.type == 'Bot') || + ((github.event.action == 'opened' || github.event.action == 'synchronize') && github.event.sender.type == 'Bot') || (github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'ccbr-bot[bot]') ) ) diff --git a/examples/pre-review-pr.yml b/examples/pre-review-pr.yml index b59f045d..072ac32a 100644 --- a/examples/pre-review-pr.yml +++ b/examples/pre-review-pr.yml @@ -119,8 +119,7 @@ jobs: github.event_name == 'pull_request' && startsWith(github.event.pull_request.title, 'chore: post-release cleanup for') && ( - github.event.action == 'synchronize' || - (github.event.action == 'opened' && github.event.sender.type == 'Bot') || + ((github.event.action == 'opened' || github.event.action == 'synchronize') && github.event.sender.type == 'Bot') || (github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'ccbr-bot[bot]') ) ) diff --git a/review-post-release-pr/README.md b/review-post-release-pr/README.md index d280cb46..5d1d3c83 100644 --- a/review-post-release-pr/README.md +++ b/review-post-release-pr/README.md @@ -6,7 +6,7 @@ version/date bumps matching a real release tag are present, otherwise request a human reviewer This action automates the review of post-release cleanup pull requests -opened by the [post-release](/post-release) action, with titles of the +opened by the [post-release](./post-release) action, with titles of the form `chore: post-release cleanup for `. The action verifies five conditions: @@ -85,7 +85,7 @@ must allow `actions: write` (to approve pending workflow runs), for comments when auto-merge cannot be enabled. The GitHub App installation must have these repository permissions as well. -### Basic example +### Example ```yaml name: review-post-release-pr @@ -107,8 +107,13 @@ jobs: review-post-release-pr: runs-on: ubuntu-latest container: nciccbr/ccbr_actions:latest - # Only run for post-release cleanup PRs - if: startsWith(github.event.pull_request.title, 'chore: post-release cleanup for') + # Only run for post-release cleanup PRs opened/pushed by the bot, or when ccbr-bot is requested as a reviewer + if: > + startsWith(github.event.pull_request.title, 'chore: post-release cleanup for') && + ( + github.event.sender.type == 'Bot' || + (github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'ccbr-bot[bot]') + ) steps: - uses: actions/create-github-app-token@v3 id: generate-token @@ -121,7 +126,7 @@ jobs: pr-number: ${{ github.event.pull_request.number }} ``` -See also [pre-review-pr.yml](/examples/pre-review-pr.yml). +See also [pre-review-pr.yml](./examples/pre-review-pr.yml). ## Inputs diff --git a/review-post-release-pr/README.qmd b/review-post-release-pr/README.qmd index e0c084cc..8b26b212 100644 --- a/review-post-release-pr/README.qmd +++ b/review-post-release-pr/README.qmd @@ -88,7 +88,7 @@ The `contents` permission is required to enable auto-merge; `issues` is used for comments when auto-merge cannot be enabled. The GitHub App installation must have these repository permissions as well. -### Basic example +### Example ```yaml name: review-post-release-pr @@ -110,8 +110,13 @@ jobs: review-post-release-pr: runs-on: ubuntu-latest container: nciccbr/ccbr_actions:latest - # Only run for post-release cleanup PRs - if: startsWith(github.event.pull_request.title, 'chore: post-release cleanup for') + # Only run for post-release cleanup PRs opened/pushed by the bot, or when ccbr-bot is requested as a reviewer + if: > + startsWith(github.event.pull_request.title, 'chore: post-release cleanup for') && + ( + github.event.sender.type == 'Bot' || + (github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'ccbr-bot[bot]') + ) steps: - uses: actions/create-github-app-token@v3 id: generate-token diff --git a/review-pre-commit-pr/README.md b/review-pre-commit-pr/README.md index 99d0a432..83facba1 100644 --- a/review-pre-commit-pr/README.md +++ b/review-pre-commit-pr/README.md @@ -61,12 +61,8 @@ check as one of the required checks. In your repo’s rulesets Consider also including other desired checks such as your build/test workflow, auto-format, etc. -
- - -
+![Branch protection +rule](https://raw.githubusercontent.com/CCBR/actions/main/review-pre-commit-pr/img/branch-protection-rule.png) ### Example @@ -87,8 +83,13 @@ jobs: review-pre-commit-pr: runs-on: ubuntu-latest container: nciccbr/ccbr_actions:latest - # Only run for pre-commit.ci autoupdate PRs - if: github.event.pull_request.title == '[pre-commit.ci] pre-commit autoupdate' + # Only run for pre-commit.ci autoupdate PRs opened by the bot, or when ccbr-bot is requested as a reviewer + if: > + github.event.pull_request.title == '[pre-commit.ci] pre-commit autoupdate' && + ( + github.event.sender.type == 'Bot' || + (github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'ccbr-bot[bot]') + ) steps: - uses: actions/create-github-app-token@v3 id: generate-token @@ -101,7 +102,7 @@ jobs: pr-number: ${{ github.event.pull_request.number }} ``` -See also: [pre-review-pr.yml](/examples/pre-review-pr.yml). +See also: [pre-review-pr.yml](./examples/pre-review-pr.yml). ## Inputs diff --git a/review-pre-commit-pr/README.qmd b/review-pre-commit-pr/README.qmd index 0ec0ff0b..fe0b3897 100644 --- a/review-pre-commit-pr/README.qmd +++ b/review-pre-commit-pr/README.qmd @@ -87,8 +87,13 @@ jobs: review-pre-commit-pr: runs-on: ubuntu-latest container: nciccbr/ccbr_actions:latest - # Only run for pre-commit.ci autoupdate PRs - if: github.event.pull_request.title == '[pre-commit.ci] pre-commit autoupdate' + # Only run for pre-commit.ci autoupdate PRs opened by the bot, or when ccbr-bot is requested as a reviewer + if: > + github.event.pull_request.title == '[pre-commit.ci] pre-commit autoupdate' && + ( + github.event.sender.type == 'Bot' || + (github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'ccbr-bot[bot]') + ) steps: - uses: actions/create-github-app-token@v3 id: generate-token From 5d9b9d43453246b41d482569662c75d86a94d49d Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Mon, 28 Sep 2026 12:00:46 -0400 Subject: [PATCH 19/23] fix(review-pr): pass action inputs via env instead of embedding in python review-post-release-pr and review-pre-commit-pr spliced `${{ inputs.* }}` directly into the literal text of an embedded `shell: python {0}` script. Any value expanded there is exploitable as a template-injection vector (GitHub expands the template before Python ever sees the source). Move each value into a step-level `env:` and read it via `os.environ[...]` instead. _AI-assisted: Claude Sonnet 5 via Copilot_ --- review-post-release-pr/action.yml | 27 ++++++++++++++++++--------- review-pre-commit-pr/action.yml | 12 ++++++++---- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/review-post-release-pr/action.yml b/review-post-release-pr/action.yml index c2666d09..972c4635 100644 --- a/review-post-release-pr/action.yml +++ b/review-post-release-pr/action.yml @@ -68,17 +68,26 @@ runs: from ccbr_actions.post_release_pr import review_post_release_pr result = review_post_release_pr( - repo="${{ inputs.repo }}", - pr_number="${{ inputs.pr-number }}", - reviewer="${{ inputs.reviewer }}", - force_review="${{ inputs.force-review }}".lower() == "true", - version_filepath="${{ inputs.version-filepath }}", - citation_filepath="${{ inputs.citation-filepath }}", - changelog_filepath="${{ inputs.changelog-filepath }}", - description_filepath="${{ inputs.description-filepath }}", - dev_header="${{ inputs.dev-header }}", + repo=os.environ["REPO"], + pr_number=os.environ["PR_NUMBER"], + reviewer=os.environ["REVIEWER"], + force_review=os.environ["FORCE_REVIEW"].lower() == "true", + version_filepath=os.environ["VERSION_FILEPATH"], + citation_filepath=os.environ["CITATION_FILEPATH"], + changelog_filepath=os.environ["CHANGELOG_FILEPATH"], + description_filepath=os.environ["DESCRIPTION_FILEPATH"], + dev_header=os.environ["DEV_HEADER"], token=os.environ["GITHUB_TOKEN"], ) print(f"Action completed with automatic_approval={result}") env: GITHUB_TOKEN: ${{ inputs.github-token }} + REPO: ${{ inputs.repo }} + PR_NUMBER: ${{ inputs.pr-number }} + REVIEWER: ${{ inputs.reviewer }} + FORCE_REVIEW: ${{ inputs.force-review }} + VERSION_FILEPATH: ${{ inputs.version-filepath }} + CITATION_FILEPATH: ${{ inputs.citation-filepath }} + CHANGELOG_FILEPATH: ${{ inputs.changelog-filepath }} + DESCRIPTION_FILEPATH: ${{ inputs.description-filepath }} + DEV_HEADER: ${{ inputs.dev-header }} diff --git a/review-pre-commit-pr/action.yml b/review-pre-commit-pr/action.yml index 88747ae3..3eb3cdb4 100644 --- a/review-pre-commit-pr/action.yml +++ b/review-pre-commit-pr/action.yml @@ -53,12 +53,16 @@ runs: from ccbr_actions.pre_commit import review_pre_commit_pr result = review_pre_commit_pr( - repo="${{ inputs.repo }}", - pr_number="${{ inputs.pr-number }}", - reviewer="${{ inputs.reviewer }}", - force_review="${{ inputs.force-review }}".lower() == "true", + repo=os.environ["REPO"], + pr_number=os.environ["PR_NUMBER"], + reviewer=os.environ["REVIEWER"], + force_review=os.environ["FORCE_REVIEW"].lower() == "true", token=os.environ["GITHUB_TOKEN"], ) print(f"Action completed with automatic_approval={result}") env: GITHUB_TOKEN: ${{ inputs.github-token }} + REPO: ${{ inputs.repo }} + PR_NUMBER: ${{ inputs.pr-number }} + REVIEWER: ${{ inputs.reviewer }} + FORCE_REVIEW: ${{ inputs.force-review }} From 9d5edc7bba4bd4c9d049f819419e9222da94bec9 Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Mon, 28 Sep 2026 12:01:00 -0400 Subject: [PATCH 20/23] fix(post-release-pr): reject codemeta bump that drops a date field _validate_codemeta_file treated a previously-present dateModified/ datePublished/dateCreated field going missing as an unchanged field, silently accepting the removal as a valid bump. Require the field to either stay unchanged or be present and match a release date. _AI-assisted: Claude Sonnet 5 via Copilot_ --- src/ccbr_actions/post_release_pr.py | 8 +++++--- tests/test_post_release_pr.py | 12 ++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/ccbr_actions/post_release_pr.py b/src/ccbr_actions/post_release_pr.py index 0575c5b1..7d4043fb 100644 --- a/src/ccbr_actions/post_release_pr.py +++ b/src/ccbr_actions/post_release_pr.py @@ -424,9 +424,11 @@ def _validate_codemeta_file( f"v{release_version}", ) date_fields_ok = all( - new_data.get(k) is None - or new_data.get(k) == old_data.get(k) - or str(new_data.get(k))[:10] in release_dates + new_data.get(k) == old_data.get(k) + or ( + new_data.get(k) is not None + and str(new_data.get(k))[:10] in release_dates + ) for k in date_keys ) result = fields_unchanged and version_ok and date_fields_ok diff --git a/tests/test_post_release_pr.py b/tests/test_post_release_pr.py index 5d2abc05..43a64b09 100644 --- a/tests/test_post_release_pr.py +++ b/tests/test_post_release_pr.py @@ -543,6 +543,18 @@ def test_validate_codemeta_file_rejects_unrelated_field_replacement(): ) +def test_validate_codemeta_file_rejects_dropped_date_field(): + # Removing a previously-present date field must not be treated as a no-op. + old_content = '{"version": "0.7.0", "dateModified": "2024-01-01"}' + new_content = '{"version": "0.7.1"}' + assert ( + _validate_codemeta_file( + old_content, new_content, RELEASE_VERSION, RELEASE_TAG, RELEASE_DATES + ) + is False + ) + + # --------------------------------------------------------------------------- # _validate_changelog_file # --------------------------------------------------------------------------- From 64b481df2b59b4cfc975fc24dceb47653a297478 Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Mon, 28 Sep 2026 12:01:10 -0400 Subject: [PATCH 21/23] docs(pr-review): clarify get_last_workflow_run_actor is best-effort Note that it returns the actor for the most recent run of the given workflow file repo-wide, not necessarily the run tied to the specific release/PR under review. _AI-assisted: Claude Sonnet 5 via Copilot_ --- src/ccbr_actions/pr_review.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/ccbr_actions/pr_review.py b/src/ccbr_actions/pr_review.py index 1f5a73be..fedd97c3 100644 --- a/src/ccbr_actions/pr_review.py +++ b/src/ccbr_actions/pr_review.py @@ -319,6 +319,11 @@ def get_last_workflow_run_actor(repo, workflow_file, token=None, session=None): """ Return the triggering actor's login for the most recent run of a workflow. + This is a best-effort heuristic: it returns the actor for the single most + recent run of *workflow_file* across the whole repo, not necessarily the + run that produced the specific release/PR being reviewed (e.g. if another + run of *workflow_file* started afterward but before review happened). + Args: repo (str): Repository full name (e.g. ``"CCBR/actions"``). workflow_file (str): Workflow filename (e.g. ``"draft-release.yml"``). From 275718c5aee6695ffe0649918c787a789599d768 Mon Sep 17 00:00:00 2001 From: Kelly Sovacool Date: Mon, 28 Sep 2026 12:01:27 -0400 Subject: [PATCH 22/23] docs(changelog): record review-pr security hardening (#227) _AI-assisted: Claude Sonnet 5 via Copilot_ --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a5b8a84..c7aa7fa0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ ## actions development version - New action: `review-post-release-pr` to automatically review post-release cleanup PRs and approve them (approving pending workflow runs and enabling auto-merge) when the version file was actually bumped and every changed file's new content is a valid, field-specific bump matching an actual release tag, or request a human reviewer otherwise. Approvals are pinned to the validated commit and re-validated if the PR's head changes afterward (e.g. after an `auto-format` push). (#227, @kelly-sovacool, @copilot) + - Require the PR's `synchronize` event to come from the bot (not just any pusher) before re-running the automatic review, and pass action inputs to the embedded Python scripts via `env:` instead of splicing `${{ inputs.* }}` directly into the script text, in `review-pre-commit-pr` and `review-post-release-pr`. (#227, @kelly-sovacool, @copilot) - Fix `review-pre-commit-pr` to post a comment instead of requesting changes when a pre-commit.ci autoupdate PR needs human review. (#226, @kelly-sovacool, @copilot) ## actions 0.8.0 From 8605feff4d9e5a4017e56eea912a3f78f0d3862a Mon Sep 17 00:00:00 2001 From: CCBR-bot <258092125+ccbr-bot@users.noreply.github.com> Date: Mon, 28 Sep 2026 16:08:42 +0000 Subject: [PATCH 23/23] =?UTF-8?q?ci:=20=F0=9F=A4=96=20render=20readme=20&?= =?UTF-8?q?=20format=20everything=20with=20pre-commit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- review-post-release-pr/README.md | 4 ++-- review-pre-commit-pr/README.md | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/review-post-release-pr/README.md b/review-post-release-pr/README.md index 5d1d3c83..53019975 100644 --- a/review-post-release-pr/README.md +++ b/review-post-release-pr/README.md @@ -6,7 +6,7 @@ version/date bumps matching a real release tag are present, otherwise request a human reviewer This action automates the review of post-release cleanup pull requests -opened by the [post-release](./post-release) action, with titles of the +opened by the [post-release](/post-release) action, with titles of the form `chore: post-release cleanup for `. The action verifies five conditions: @@ -126,7 +126,7 @@ jobs: pr-number: ${{ github.event.pull_request.number }} ``` -See also [pre-review-pr.yml](./examples/pre-review-pr.yml). +See also [pre-review-pr.yml](/examples/pre-review-pr.yml). ## Inputs diff --git a/review-pre-commit-pr/README.md b/review-pre-commit-pr/README.md index 83facba1..2771c416 100644 --- a/review-pre-commit-pr/README.md +++ b/review-pre-commit-pr/README.md @@ -61,8 +61,12 @@ check as one of the required checks. In your repo’s rulesets Consider also including other desired checks such as your build/test workflow, auto-format, etc. -![Branch protection -rule](https://raw.githubusercontent.com/CCBR/actions/main/review-pre-commit-pr/img/branch-protection-rule.png) +
+ + +
### Example @@ -102,7 +106,7 @@ jobs: pr-number: ${{ github.event.pull_request.number }} ``` -See also: [pre-review-pr.yml](./examples/pre-review-pr.yml). +See also: [pre-review-pr.yml](/examples/pre-review-pr.yml). ## Inputs