From 65685806937d30d062300c4c8fd300084698f7a6 Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Thu, 27 Aug 2026 21:08:48 +0000 Subject: [PATCH] MAINT: Run CI on release branches The workflows never ran on release branches. `build_and_test`, `diff_cover`, `docker_build` and `frontend_tests` filtered pull requests on `release/**`, but release branches are named `releases/vx.y.z`, so the filter matched nothing, and their push triggers listed only `main`. Only `docs.yml` used the correct pattern, which is why it was the sole workflow to run on past release branches. They now match `docs.yml`. `diff_cover` keeps its pull request trigger limited to `main`, because diff coverage compares against `origin/main` and a release branch differs from it by the whole release. It gains the manual trigger the other three already have. The pre-commit step now declares `shell: bash`, because it grew a shell conditional and the Windows runner defaults to PowerShell. A pull request into a release branch is compared against that branch rather than against `main`, because everything the release branch already carries is not part of the change under review. Only the push and merge queue paths, which carry no base branch to compare against, skip the history comparison. Running these workflows on release branches exposed two problems that were previously invisible. The alembic revision immutability check compares the branch against `origin/main`, and a release branch cut from an earlier tag carries cherry-picked commits that legitimately edit already-released revisions, so the check reported them as violations; it now skips that comparison on release branches while still checking staged changes. The concurrency groups keyed on `github.head_ref`, which is empty for a push, so every push formed its own group and nothing was ever superseded, defeating the stated intent of the block; they now fall back to `github.ref`. The memory models guide stated the immutability rule without qualification, so it now records the release branch exception alongside it. --- .github/workflows/build_and_test.yml | 9 +- .github/workflows/diff_cover.yml | 9 +- .github/workflows/docker_build.yml | 5 +- .github/workflows/frontend_tests.yml | 5 +- .../enforce_alembic_revision_immutability.py | 46 ++++- doc/contributing/11_memory_models.md | 2 + ...t_enforce_alembic_revision_immutability.py | 163 ++++++++++++++++++ 7 files changed, 227 insertions(+), 12 deletions(-) create mode 100644 tests/unit/build_scripts/test_enforce_alembic_revision_immutability.py diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index afdf2c9c76..7ee2b7d6e5 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -9,17 +9,18 @@ on: push: branches: - "main" + - "releases/v*" pull_request: branches: - "main" - - "release/**" + - "releases/**" merge_group: workflow_dispatch: concurrency: # This ensures after each commit the old jobs are cancelled and the new ones # run instead. - group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} + group: ${{ github.workflow }}-${{ github.head_ref || github.ref }} cancel-in-progress: true jobs: @@ -75,8 +76,12 @@ jobs: # latent violations in untouched files, including the tests/integration, # tests/partner_integration, and tests/end_to_end tiers, are caught before # merge instead of only on the post-merge run against main. + shell: bash run: | git fetch origin main + if [ -n "${GITHUB_BASE_REF:-}" ]; then + git fetch origin "$GITHUB_BASE_REF" + fi uv run pre-commit run --all-files # Main job runs only if pre-commit succeeded diff --git a/.github/workflows/diff_cover.yml b/.github/workflows/diff_cover.yml index 0c30e526e6..f65c3d4dbc 100644 --- a/.github/workflows/diff_cover.yml +++ b/.github/workflows/diff_cover.yml @@ -1,5 +1,9 @@ # Single place for all coverage checks: overall threshold + diff coverage on PRs. # Runs once on Ubuntu/Python 3.12 instead of across the full OS x Python matrix. +# +# Release branches run on push only, for the overall coverage threshold. The diff-coverage +# step is deliberately limited to pull requests targeting main, because it compares against +# origin/main and a release branch differs from main by the entire release delta. name: coverage @@ -7,14 +11,15 @@ on: push: branches: - "main" + - "releases/v*" pull_request: branches: - "main" - - "release/**" merge_group: + workflow_dispatch: concurrency: - group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} + group: ${{ github.workflow }}-${{ github.head_ref || github.ref }} cancel-in-progress: true jobs: diff --git a/.github/workflows/docker_build.yml b/.github/workflows/docker_build.yml index 739504ff42..f78befecd9 100644 --- a/.github/workflows/docker_build.yml +++ b/.github/workflows/docker_build.yml @@ -6,10 +6,11 @@ on: push: branches: - "main" + - "releases/v*" pull_request: branches: - "main" - - "release/**" + - "releases/**" merge_group: workflow_dispatch: @@ -17,7 +18,7 @@ permissions: contents: read concurrency: - group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} + group: ${{ github.workflow }}-${{ github.head_ref || github.ref }} cancel-in-progress: true jobs: diff --git a/.github/workflows/frontend_tests.yml b/.github/workflows/frontend_tests.yml index 18696381b2..f7a715996c 100644 --- a/.github/workflows/frontend_tests.yml +++ b/.github/workflows/frontend_tests.yml @@ -7,15 +7,16 @@ on: push: branches: - "main" + - "releases/v*" pull_request: branches: - "main" - - "release/**" + - "releases/**" merge_group: workflow_dispatch: concurrency: - group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} + group: ${{ github.workflow }}-${{ github.head_ref || github.ref }} cancel-in-progress: true env: diff --git a/build_scripts/enforce_alembic_revision_immutability.py b/build_scripts/enforce_alembic_revision_immutability.py index 9fa5e1122e..e7891e352c 100644 --- a/build_scripts/enforce_alembic_revision_immutability.py +++ b/build_scripts/enforce_alembic_revision_immutability.py @@ -6,6 +6,8 @@ Checks staged changes (local pre-commit), the full branch diff against origin/main (CI PRs), and the previous commit (CI merge-queue / push-to-main). + +The two history checks are skipped on release branches, which legitimately diverge from main. """ import os @@ -13,6 +15,7 @@ import sys _VERSIONS_PATH = "pyrit/memory/alembic/versions/" +_MERGE_QUEUE_REF_PREFIX = "refs/heads/gh-readonly-queue/" def _git(*args: str) -> subprocess.CompletedProcess[str]: @@ -42,6 +45,29 @@ def _fail_ci(reason: str) -> bool: return False +def _on_release_branch() -> bool: + """ + Report whether the checks below are running against a release branch. + + A release branch is cut from an earlier tag and carries cherry-picked commits, so it + legitimately differs from ``main`` in ways the history checks below would read as + edits to already-released revisions. + """ + # push events; pull_request events set GITHUB_REF to refs/pull//merge instead, + # which carries no branch name, so the target branch has to come from GITHUB_BASE_REF. + github_ref = os.environ.get("GITHUB_REF", "") + base_ref = os.environ.get("GITHUB_BASE_REF", "") + if github_ref or base_ref: + # merge_group events run on a temporary queue branch named + # refs/heads/gh-readonly-queue//pr-- and leave GITHUB_BASE_REF + # unset, so the target branch has to be read back out of the ref itself. + if github_ref.startswith(_MERGE_QUEUE_REF_PREFIX): + github_ref = f"refs/heads/{github_ref[len(_MERGE_QUEUE_REF_PREFIX) :]}" + return github_ref.startswith("refs/heads/releases/") or base_ref.startswith("releases/") + # Neither variable is set outside CI, so fall back to the checked-out branch. + return _git_stdout("rev-parse", "--abbrev-ref", "HEAD").startswith("releases/") + + def has_revision_violations() -> bool: # Local pre-commit: check staged changes violations = _get_violations(["--cached"]) @@ -49,17 +75,29 @@ def has_revision_violations() -> bool: _report(violations) return True - # CI (PR): diff branch against its merge-base with origin/main. + # A release branch carries cherry-picked fixes that amend already-released revisions on + # purpose, so comparing it against `main` reports intentional work as violations. A pull + # request is still comparable against its own base, so only the push and merge queue paths + # are skipped. `git cherry-pick` does not run pre-commit, so the staged check above rarely + # fires on those paths either: review is the remaining control there. + base_ref = os.environ.get("GITHUB_BASE_REF", "") + if _on_release_branch() and not base_ref: + return False + + # CI (PR): diff branch against its merge-base with the branch it targets. A pull request + # into a release branch has to compare against that branch, because everything the release + # branch already carries is not part of the change under review. # The three-dot syntax (A...B) resolves to ``git diff $(merge-base A B) B`` # automatically, so we don't need a separate merge-base call. When - # origin/main is missing (shallow clone) git exits non-zero. - pr_diff = _git("diff", "--name-status", "origin/main...HEAD", "--", _VERSIONS_PATH) + # the base is missing (shallow clone) git exits non-zero. + base = f"origin/{base_ref}" if base_ref else "origin/main" + pr_diff = _git("diff", "--name-status", f"{base}...HEAD", "--", _VERSIONS_PATH) if pr_diff.returncode == 0: violations = [line for line in pr_diff.stdout.strip().splitlines() if line and not line.startswith("A")] if violations: _report(violations) return True - elif _fail_ci("origin/main is not available (shallow clone?)"): + elif _fail_ci(f"{base} is not available (shallow clone?)"): return True # CI (merge-queue / push-to-main): on main the branch *is* origin/main, so diff --git a/doc/contributing/11_memory_models.md b/doc/contributing/11_memory_models.md index 56db805e91..1e1c84b746 100644 --- a/doc/contributing/11_memory_models.md +++ b/doc/contributing/11_memory_models.md @@ -93,6 +93,8 @@ await initialize_pyrit_async("SQLite", skip_schema_migration=True) Once a migration revision is committed, it **must not be modified or deleted**. This is enforced by a pre-commit hook (`enforce_alembic_revision_immutability`). If you need to fix a migration, create a new revision instead. +A release branch is the one exception. A patch release cherry-picks a fix onto a branch cut from an earlier tag, and that fix may legitimately amend a revision that has already shipped, so the hook does not compare history on a release branch. Review is the control there. + ### Pre-commit hooks Two hooks run automatically when you touch memory-related files: diff --git a/tests/unit/build_scripts/test_enforce_alembic_revision_immutability.py b/tests/unit/build_scripts/test_enforce_alembic_revision_immutability.py new file mode 100644 index 0000000000..752f31354a --- /dev/null +++ b/tests/unit/build_scripts/test_enforce_alembic_revision_immutability.py @@ -0,0 +1,163 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT license. + +import os +import subprocess +from types import SimpleNamespace +from unittest.mock import patch + +import pytest + +from build_scripts.enforce_alembic_revision_immutability import ( + _on_release_branch, + has_revision_violations, +) + +MODIFIED_REVISION = "M\tpyrit/memory/alembic/versions/b2f4c6a8d1e3_add_conversations_table.py" + + +def _completed(stdout: str = "", returncode: int = 0) -> subprocess.CompletedProcess: + return subprocess.CompletedProcess(args=["git"], returncode=returncode, stdout=stdout, stderr="") + + +@pytest.mark.parametrize( + "environment, expected", + [ + ({"GITHUB_REF": "refs/heads/releases/v1.1.0"}, True), + ({"GITHUB_REF": "refs/heads/releases/v1.0.1"}, True), + ({"GITHUB_REF": "refs/heads/main"}, False), + ({"GITHUB_REF": "refs/heads/releases-notes"}, False), + ({"GITHUB_REF": "refs/tags/v1.1.0"}, False), + ({"GITHUB_REF": "refs/pull/42/merge", "GITHUB_BASE_REF": "releases/v1.1.0"}, True), + ({"GITHUB_REF": "refs/pull/42/merge", "GITHUB_BASE_REF": "main"}, False), + ({"GITHUB_REF": "refs/heads/gh-readonly-queue/releases/v1.1.0/pr-42-abc123"}, True), + ({"GITHUB_REF": "refs/heads/gh-readonly-queue/main/pr-42-abc123"}, False), + ({}, False), + ], +) +def test_on_release_branch_recognizes_release_refs(environment: dict[str, str], expected: bool) -> None: + """pull_request events carry the target branch in GITHUB_BASE_REF; merge_group embeds it in GITHUB_REF.""" + with patch.dict("os.environ", environment, clear=True): + with patch("build_scripts.enforce_alembic_revision_immutability._git_stdout", return_value="main"): + assert _on_release_branch() is expected + + +@pytest.mark.parametrize( + "checked_out_branch, expected", + [("releases/v1.1.0", True), ("main", False), ("HEAD", False)], +) +def test_on_release_branch_falls_back_to_checked_out_branch(checked_out_branch: str, expected: bool) -> None: + """Runs outside GitHub Actions have no ref variables, leaving the branch name as the only signal.""" + with patch.dict("os.environ", {}, clear=True): + with patch( + "build_scripts.enforce_alembic_revision_immutability._git_stdout", + return_value=checked_out_branch, + ) as mock_git_stdout: + assert _on_release_branch() is expected + + assert mock_git_stdout.call_args.args == ("rev-parse", "--abbrev-ref", "HEAD") + + +def test_on_release_branch_ignores_branch_name_when_ci_refs_are_present() -> None: + """A PR from a release-named source branch into main must still be enforced.""" + environment = {"GITHUB_REF": "refs/pull/42/merge", "GITHUB_BASE_REF": "main"} + with patch.dict("os.environ", environment, clear=True): + with patch( + "build_scripts.enforce_alembic_revision_immutability._git_stdout", + return_value="releases/v1.1.0", + ) as mock_git_stdout: + assert _on_release_branch() is False + + mock_git_stdout.assert_not_called() + + +def test_release_branch_push_skips_history_checks() -> None: + """A release branch push shares neither origin/main nor a comparable previous commit.""" + + def _fail_if_called(*args, **kwargs): + raise AssertionError(f"history check ran on a release branch push: {args}") + + with patch.dict(os.environ, {"GITHUB_BASE_REF": ""}, clear=False): + with patch("build_scripts.enforce_alembic_revision_immutability._on_release_branch", return_value=True): + with patch("build_scripts.enforce_alembic_revision_immutability._get_violations", return_value=[]): + with patch("build_scripts.enforce_alembic_revision_immutability._git", side_effect=_fail_if_called): + assert has_revision_violations() is False + + +def test_release_pull_request_compares_against_its_base() -> None: + """A pull request into a release branch is comparable against that branch.""" + calls: list[tuple] = [] + + def _record(*args, **kwargs): + calls.append(args) + return SimpleNamespace(returncode=0, stdout="", stderr="") + + with patch.dict(os.environ, {"GITHUB_BASE_REF": "releases/v1.1.0"}, clear=False): + with patch("build_scripts.enforce_alembic_revision_immutability._on_release_branch", return_value=True): + with patch("build_scripts.enforce_alembic_revision_immutability._get_violations", return_value=[]): + with patch("build_scripts.enforce_alembic_revision_immutability._git", side_effect=_record): + assert has_revision_violations() is False + + assert any("origin/releases/v1.1.0...HEAD" in call for call in calls) + assert not any("origin/main...HEAD" in call for call in calls) + + +def test_release_pull_request_reports_modified_revision() -> None: + """The base comparison still catches a revision the pull request itself edits.""" + + def _modified(*args, **kwargs): + if "diff" in args and any(arg == "origin/releases/v1.1.0...HEAD" for arg in args): + return SimpleNamespace(returncode=0, stdout=f"M\t{MODIFIED_REVISION}\n", stderr="") + return SimpleNamespace(returncode=0, stdout="", stderr="") + + with patch.dict(os.environ, {"GITHUB_BASE_REF": "releases/v1.1.0"}, clear=False): + with patch("build_scripts.enforce_alembic_revision_immutability._on_release_branch", return_value=True): + with patch("build_scripts.enforce_alembic_revision_immutability._get_violations", return_value=[]): + with patch("build_scripts.enforce_alembic_revision_immutability._git", side_effect=_modified): + assert has_revision_violations() is True + + +def test_release_branch_still_reports_staged_violations() -> None: + """Skipping the history checks must not stop the staged-change check.""" + with patch("build_scripts.enforce_alembic_revision_immutability._on_release_branch", return_value=True): + with patch( + "build_scripts.enforce_alembic_revision_immutability._get_violations", + return_value=[MODIFIED_REVISION], + ): + assert has_revision_violations() is True + + +def test_branch_comparison_still_runs_off_release_branches() -> None: + """Positive control: the origin/main comparison must keep catching violations everywhere else.""" + with patch.dict(os.environ, {"GITHUB_BASE_REF": ""}, clear=False): + with patch("build_scripts.enforce_alembic_revision_immutability._on_release_branch", return_value=False): + with patch("build_scripts.enforce_alembic_revision_immutability._get_violations", return_value=[]): + with patch( + "build_scripts.enforce_alembic_revision_immutability._git", + return_value=_completed(stdout=MODIFIED_REVISION), + ) as mock_git: + assert has_revision_violations() is True + + assert mock_git.call_args.args[:3] == ("diff", "--name-status", "origin/main...HEAD") + + +def test_previous_commit_check_still_runs_off_release_branches() -> None: + """Positive control: the HEAD~1..HEAD check must keep catching violations everywhere else.""" + + def _violations_for(diff_spec: list[str]) -> list[str]: + return [MODIFIED_REVISION] if diff_spec == ["HEAD~1..HEAD"] else [] + + with patch("build_scripts.enforce_alembic_revision_immutability._on_release_branch", return_value=False): + with patch( + "build_scripts.enforce_alembic_revision_immutability._get_violations", + side_effect=_violations_for, + ): + with patch("build_scripts.enforce_alembic_revision_immutability._git", return_value=_completed()): + assert has_revision_violations() is True + + +def test_clean_history_off_release_branches_passes() -> None: + with patch("build_scripts.enforce_alembic_revision_immutability._on_release_branch", return_value=False): + with patch("build_scripts.enforce_alembic_revision_immutability._get_violations", return_value=[]): + with patch("build_scripts.enforce_alembic_revision_immutability._git", return_value=_completed()): + assert has_revision_violations() is False