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