From 76f4ccaa39a5a71825c41426a05def7b812ae9ba Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 8 Oct 2026 20:24:02 -0400 Subject: [PATCH 1/3] Add an opt-in integration branch CI pilot Run the existing three cheap checks on integration PRs while preserving all 26 runners and check names for main PRs. Promote immutable snapshots every four hours or manually with an App token, synchronize main back after a promotion, and document the merge-commit and bisect recovery procedures. Cover base and draft budgets, no-op gates, snapshot isolation, sync conflicts and races with code-health tests that use only tiny local Git repositories. Co-Authored-By: Claude Opus 5.5 --- .github/actions/candidate-wheel/action.yaml | 2 +- .github/promote_integration.py | 151 +++++++ .github/workflows/pr.yaml | 8 +- .github/workflows/promote-integration.yaml | 56 +++ CONTRIBUTING.md | 46 ++ .../ci-capacity-integration-branch.changed.md | 1 + .../code_health/test_pr_runner_budget.py | 174 +++++++- .../test_pr_workflow_skips_drafts.py | 3 +- .../code_health/test_promote_integration.py | 400 ++++++++++++++++++ 9 files changed, 817 insertions(+), 24 deletions(-) create mode 100644 .github/promote_integration.py create mode 100644 .github/workflows/promote-integration.yaml create mode 100644 changelog.d/ci-capacity-integration-branch.changed.md create mode 100644 policyengine_us/tests/code_health/test_promote_integration.py diff --git a/.github/actions/candidate-wheel/action.yaml b/.github/actions/candidate-wheel/action.yaml index c82af1e3567..00dd4964ad9 100644 --- a/.github/actions/candidate-wheel/action.yaml +++ b/.github/actions/candidate-wheel/action.yaml @@ -22,7 +22,7 @@ runs: echo "present=true" >> "$GITHUB_OUTPUT" else echo "present=false" >> "$GITHUB_OUTPUT" - echo "::notice title=Candidate wheel skipped::The PR head predates the release tooling (.github/release_lock.py and .github/release_build.py). Merge main into the branch to build its candidate wheel." + echo "::notice title=Candidate wheel skipped::The PR head predates the release tooling (.github/release_lock.py and .github/release_build.py). Merge the PR base branch into the branch to build its candidate wheel." fi - name: Check committed registry lock if: steps.tooling.outputs.present == 'true' diff --git a/.github/promote_integration.py b/.github/promote_integration.py new file mode 100644 index 00000000000..6630d7bd7f2 --- /dev/null +++ b/.github/promote_integration.py @@ -0,0 +1,151 @@ +"""Synchronize integration and open one immutable promotion at a time. + +Uses only git, gh and the Python standard library; never imports the model. +""" + +from __future__ import annotations + +from datetime import datetime, timezone +import json +import os +import subprocess + + +def git(*args: str) -> str: + return subprocess.run( + ["git", *args], check=True, capture_output=True, text=True + ).stdout.strip() + + +def api(repository: str, resource: str, *, fields=None, paginate=False): + command = ["gh", "api", f"repos/{repository}/{resource}"] + if fields is not None: + command.extend(["--method", "POST", "--input", "-"]) + if paginate: + command.extend(["--paginate", "--slurp"]) + result = subprocess.run( + command, + input=json.dumps(fields) if fields is not None else None, + check=True, + capture_output=True, + text=True, + ) + data = json.loads(result.stdout) + return [item for page in data for item in page] if paginate else data + + +def ancestor(older: str, newer: str) -> bool: + result = subprocess.run( + ["git", "merge-base", "--is-ancestor", older, newer], + capture_output=True, + text=True, + ) + if result.returncode not in (0, 1): + result.check_returncode() + return result.returncode == 0 + + +def run(repository: str, now: datetime | None = None) -> None: + if not git("ls-remote", "--heads", "origin", "refs/heads/integration"): + print("::notice::Integration branch is absent; the pilot is inactive.") + return + git( + "fetch", + "--no-tags", + "origin", + "+refs/heads/main:refs/remotes/origin/main", + "+refs/heads/integration:refs/remotes/origin/integration", + ) + opened = api(repository, "pulls?state=open&base=main&per_page=100", paginate=True) + for pull in opened: + head = pull["head"] + if ( + head["ref"].startswith("promote/") + and (head.get("repo") or {}).get("full_name", "").lower() + == repository.lower() + ): + print(f"::notice::Promotion #{pull['number']} is open; leaving it frozen.") + return + + git("checkout", "-B", "integration", "origin/integration") + if not ancestor("origin/main", "HEAD"): + try: + git("merge", "--no-edit", "origin/main") + except subprocess.CalledProcessError as exc: + git("merge", "--abort") + print(exc.stdout or "") + print(exc.stderr or "") + print( + "::warning::Main could not merge into integration. Resolve the " + "conflict on integration before the next promotion." + ) + return + # A concurrent member merge rejects this ordinary push. Fail the tick + # rather than overwrite it or create a snapshot from a stale head. + git("push", "origin", "HEAD:refs/heads/integration") + + commits = git("rev-list", "--reverse", "origin/main..HEAD").splitlines() + if not commits: + print("::notice::Integration has no unpromoted commits; no promotion needed.") + return + closed = api( + repository, "pulls?state=closed&base=integration&per_page=100", paginate=True + ) + members = [ + pull + for pull in closed + if pull.get("merged_at") and pull.get("merge_commit_sha") in commits + ] + entries = git("log", "--reverse", "--format=%H%x09%s", "origin/main..HEAD") + snapshot = git("rev-parse", "HEAD") + good = git("rev-parse", "origin/main") + stamp = (now or datetime.now(timezone.utc)).astimezone(timezone.utc) + branch = f"promote/{stamp:%Y%m%dT%H%M%S%fZ}" + body = [ + f"Frozen integration snapshot: `{snapshot}`. Pre-batch main: `{good}`.", + "", + "Merge this PR with a **merge commit**, not squash or rebase, so each " + "fix retains its commits and changelog fragments.", + "", + "Member PRs:", + ] + body.extend(f"- #{pull['number']}: {pull['title']}" for pull in members) + if not members: + body.append("- No associated merged integration PRs; see the commits below.") + body.extend(["", "Member commits:"]) + for entry in entries.splitlines(): + sha, subject = entry.split("\t", 1) + body.append( + f"- [{sha[:12]}](https://github.com/{repository}/commit/{sha}) {subject}" + ) + body.extend( + [ + "", + "If CI fails, follow CONTRIBUTING.md's red-promotion procedure. " + "Close this PR after correcting integration, then dispatch Promote " + "integration to cut a new snapshot.", + ] + ) + # Creating a ref rejects a name collision instead of moving a snapshot. + api(repository, "git/refs", fields={"ref": f"refs/heads/{branch}", "sha": snapshot}) + pull = api( + repository, + "pulls", + fields={ + "head": branch, + "base": "main", + "title": f"Promote integration ({stamp:%Y-%m-%d %H:%M:%S UTC})", + "body": "\n".join(body), + "draft": False, + }, + ) + print(f"::notice::Created promotion {pull['html_url']}") + + +if __name__ == "__main__": + try: + run(os.environ["GITHUB_REPOSITORY"]) + except subprocess.CalledProcessError as exc: + print(exc.stdout or "") + print(exc.stderr or "") + raise diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index a65ddef67d9..61a1928fb02 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -1,8 +1,8 @@ name: Pull request on: pull_request: - branches: [main] - # Draft PRs skip CI; marking a PR ready runs the full suite. Converting a + branches: [main, integration] + # Draft PRs skip CI; marking a PR ready runs the tier for its base. Converting a # PR back to draft starts a run whose jobs all skip, which cancels any # in-progress run through the concurrency group below. types: [opened, synchronize, reopened, ready_for_review, converted_to_draft] @@ -245,9 +245,11 @@ jobs: Baseline: name: Baseline tests needs: ReleaseLock + if: github.base_ref == 'main' uses: ./.github/workflows/baseline-tests.yaml HouseholdAPIPartners: needs: ReleaseLock + if: github.base_ref == 'main' name: Household API Partners runs-on: ubuntu-latest timeout-minutes: 60 @@ -277,8 +279,10 @@ jobs: Contrib: name: Contrib tests needs: ReleaseLock + if: github.base_ref == 'main' uses: ./.github/workflows/contrib-tests.yaml PythonTests: name: Python tests needs: ReleaseLock + if: github.base_ref == 'main' uses: ./.github/workflows/python-tests.yaml diff --git a/.github/workflows/promote-integration.yaml b/.github/workflows/promote-integration.yaml new file mode 100644 index 00000000000..ecf748bd9dc --- /dev/null +++ b/.github/workflows/promote-integration.yaml @@ -0,0 +1,56 @@ +name: Promote integration + +on: + schedule: + - cron: "17 */4 * * *" + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: promote-integration + cancel-in-progress: false + +jobs: + promote: + if: github.repository == 'PolicyEngine/policyengine-us' + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v6 + - name: Check whether the pilot is enabled + id: pilot + shell: bash + run: | + set -euo pipefail + branch=$(git ls-remote --heads origin refs/heads/integration) + if [ -z "$branch" ]; then + echo "::notice::Integration branch is absent; the pilot is inactive." + echo "enabled=false" >> "$GITHUB_OUTPUT" + else + echo "enabled=true" >> "$GITHUB_OUTPUT" + fi + - name: Generate GitHub App token + if: steps.pilot.outputs.enabled == 'true' + id: app-token + uses: actions/create-github-app-token@v3 + with: + app-id: ${{ secrets.APP_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + permission-contents: write + permission-pull-requests: write + permission-workflows: write + - uses: actions/checkout@v6 + if: steps.pilot.outputs.enabled == 'true' + with: + fetch-depth: 0 + token: ${{ steps.app-token.outputs.token }} + - name: Synchronize and promote integration + if: steps.pilot.outputs.enabled == 'true' + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + run: | + git config user.name "github-actions[bot]" + git config user.email "github-actions[bot]@users.noreply.github.com" + python .github/promote_integration.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b929048a687..06aa276a9df 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -40,6 +40,52 @@ everything on a laptop. Python 3.9–3.14 (`requires-python = ">=3.9,<3.15"`; CI smoke-imports on every minor). Default branch: `main`. +## Integration branch pilot + +Small independent fixes, especially agent-authored fixes, target `integration` +during this opt-in pilot. Urgent fixes, changes that must publish today, and +changes to CI itself target `main`. A maintainer enables the pilot by creating +`integration` from `main` after the promotion workflow reaches `main`. +The GitHub App installation needs contents, pull requests, and workflows write +permissions for promotion and synchronization ([GitHub App permissions](https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app/choosing-permissions-for-a-github-app)). + +Ready PRs into `integration` run three runner jobs: PR validation (`ReleaseLock`), +package and compatibility (`PackageCompatibility`), and selective tests with +coverage (`Quick-Feedback`). Ready PRs into `main`, including promotion PRs, run +the existing full suite of 26 runner jobs. Draft PRs run no jobs for either base. +The release and publishing workflow still runs on pushes to `main`. + +The promotion workflow is scheduled every four hours in UTC and also runs +manually. It opens no promotion when `integration` is absent, has no commits +that `main` lacks, or already has an open promotion PR. Otherwise it freezes +the current head on `promote/` and opens a non-draft PR into +`main` listing its member PRs and commits. New fixes on `integration` wait for +the next promotion; the workflow never updates an existing snapshot. The PR +uses the GitHub App token so its CI runs automatically, as described in +[GitHub's workflow trigger documentation](https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/trigger-a-workflow#triggering-a-workflow-from-a-workflow). + +Merge a successful promotion with a **merge commit**, preserving each fix's +commits and changelog fragments. Do not squash the promotion. The next workflow +tick brings `main`, including the release bot's version bump, back into +`integration` with a clean merge. A merge conflict stops the promotion and emits +a warning; a maintainer resolves the conflict on `integration` before running +the workflow again. A push to `integration` starts no full-suite workflow. + +For a red promotion, preserve the failed PR and logs while investigating: + +1. In a separate local checkout of the frozen snapshot, reproduce the failure + with the failing shard's exact command and dependency setup from its workflow. + Verify that command passes at the promotion's pre-batch `main` commit. +2. Run `git bisect start --first-parent ` + and `git bisect run `, then `git bisect reset`. + First-parent history follows the member fixes in their landing order. If the + result is a merge, inspect the member commits or the synchronization with + `main` to identify the failing change. +3. Revert the culprit on `integration` (`git revert `, or + `git revert -m 1 ` for a member-PR merge), with a changelog fragment. + Close the failed promotion PR, keeping it as evidence, and run the promotion + workflow manually to cut a fresh snapshot. Leave the failed snapshot intact. + ## Writing variables and programs Four types of files usually change together: diff --git a/changelog.d/ci-capacity-integration-branch.changed.md b/changelog.d/ci-capacity-integration-branch.changed.md new file mode 100644 index 00000000000..76e8a294b85 --- /dev/null +++ b/changelog.d/ci-capacity-integration-branch.changed.md @@ -0,0 +1 @@ +Add an opt-in integration branch CI flow with selective PR checks and frozen full-suite promotion PRs into main. diff --git a/policyengine_us/tests/code_health/test_pr_runner_budget.py b/policyengine_us/tests/code_health/test_pr_runner_budget.py index ef6f6197167..5dbc67f5255 100644 --- a/policyengine_us/tests/code_health/test_pr_runner_budget.py +++ b/policyengine_us/tests/code_health/test_pr_runner_budget.py @@ -3,6 +3,7 @@ import math from pathlib import Path +import pytest import yaml ROOT = Path(__file__).resolve().parents[3] @@ -14,6 +15,55 @@ } PYTHON_JOBS = GROUP_WORKFLOWS["PythonTests"]["jobs"] KEEP_RUNNING = "${{ !cancelled() }}" +READY_GUARD = "github.event.pull_request.draft == false" +MAIN_ONLY = "github.base_ref == 'main'" +CHEAP_JOBS = {"ReleaseLock", "PackageCompatibility", "Quick-Feedback"} +# The main PR runner layout before the integration pilot, including each shard. +MAIN_RUNNER_COUNTS = { + "ReleaseLock": 1, + "PackageCompatibility": 1, + "Quick-Feedback": 1, + "HouseholdAPIPartners": 1, + "Baseline": 11, + "Contrib": 9, + "Rest": 1, + "Microsimulation": 1, +} + + +def eligible_jobs(base, draft): + """Evaluate the supported PR guards and successful dependency chain.""" + conditions = {None: True, READY_GUARD: not draft, MAIN_ONLY: base == "main"} + + def eligible(name): + job = JOBS[name] + condition = job.get("if") + assert condition in conditions, (name, condition) + needs = job.get("needs", []) + if isinstance(needs, str): + needs = [needs] + return conditions[condition] and all(eligible(parent) for parent in needs) + + return {name for name in JOBS if eligible(name)} + + +def runner_counts(job_names): + runner_jobs = {} + for name in job_names: + if name in GROUP_WORKFLOWS: + additions = GROUP_WORKFLOWS[name]["jobs"] + else: + additions = {name: JOBS[name]} + assert not runner_jobs.keys() & additions.keys() + runner_jobs.update(additions) + counts = {} + for name, job in runner_jobs.items(): + matrix = job.get("strategy", {}).get("matrix", {}) + if "include" in matrix: + counts[name] = len(matrix["include"]) + else: + counts[name] = math.prod(len(values) for values in matrix.values()) + return counts def action(name): @@ -31,27 +81,77 @@ def calls(job, name): def test_pr_runner_budget_keeps_the_heavy_suites_separate(): - counts = {} - runner_jobs = {name: job for name, job in JOBS.items() if "uses" not in job} - for workflow in GROUP_WORKFLOWS.values(): - for name, job in workflow["jobs"].items(): - assert name not in runner_jobs - runner_jobs[name] = job - for name, job in runner_jobs.items(): - matrix = job.get("strategy", {}).get("matrix", {}) - if "include" in matrix: - counts[name] = len(matrix["include"]) - else: - counts[name] = math.prod(len(values) for values in matrix.values()) + counts = runner_counts(eligible_jobs("main", draft=False)) + assert counts == MAIN_RUNNER_COUNTS assert sum(counts.values()) == 26 - assert { - name: counts[name] - for name in ("Baseline", "Contrib", "Rest", "Microsimulation") - } == { - "Baseline": 11, - "Contrib": 9, - "Rest": 1, - "Microsimulation": 1, + + +@pytest.mark.parametrize( + "base,draft,expected", + [ + ("main", False, MAIN_RUNNER_COUNTS), + ("integration", False, {name: 1 for name in CHEAP_JOBS}), + ("main", True, {}), + ("integration", True, {}), + ], +) +def test_pr_jobs_and_runner_budget_by_base_and_draft(base, draft, expected): + jobs = eligible_jobs(base, draft) + assert runner_counts(jobs) == expected + if base == "integration" and not draft: + assert jobs == CHEAP_JOBS + + +def test_main_runner_checks_and_matrix_members_are_unchanged(): + assert set(eligible_jobs("main", draft=False)) == { + "ReleaseLock", + "PackageCompatibility", + "Quick-Feedback", + "Baseline", + "HouseholdAPIPartners", + "Contrib", + "PythonTests", + } + assert {name: JOBS[name]["name"] for name in CHEAP_JOBS} == { + "ReleaseLock": "PR validation (lock, guard tests, lint, changelog)", + "PackageCompatibility": "Package and compatibility", + "Quick-Feedback": "Quick Feedback (Selective Tests + Coverage)", + } + assert JOBS["HouseholdAPIPartners"]["name"] == "Household API Partners" + for name, expected_groups in { + "Baseline": [ + "states-shard-1", + "states-shard-2", + "states-shard-3", + "states-shard-4", + "irs", + "household", + "ssa-usda", + "rest-a", + "rest-b", + "contrib-hhs", + "reform", + ], + "Contrib": [ + "states-shard-1", + "states-shard-2", + "states-shard-3", + "states-shard-4", + "other-shard-1", + "other-shard-2a", + "other-shard-2b", + "other-shard-3", + "congress", + ], + }.items(): + job = GROUP_WORKFLOWS[name]["jobs"][name] + assert job["name"] == f"Full Suite - {name} (${{{{ matrix.group }}}})" + assert [item["group"] for item in job["strategy"]["matrix"]["include"]] == ( + expected_groups + ) + assert {name: job["name"] for name, job in PYTHON_JOBS.items()} == { + "Rest": "Full Suite - Rest (Python + variables)", + "Microsimulation": "Full Suite - Microsimulation", } @@ -66,6 +166,7 @@ def test_suite_groups_have_no_extra_runners_or_triggers(): assert JOBS[name] == { "name": label, "needs": "ReleaseLock", + "if": MAIN_ONLY, "uses": f"./.github/workflows/{file}", } # PyYAML parses the bare `on` key as True. No push/PR trigger here: @@ -198,3 +299,36 @@ def test_validation_collects_independent_results_after_failure(): assert any("ruff format --check" in step["run"] for step in runs) assert any("No valid changelog fragment found" in step["run"] for step in runs) assert any("git ls-files -- changelog.d" in step["run"] for step in runs) + + +def test_cheap_checks_use_the_actual_pr_base_or_checked_out_commit(): + validation = JOBS["ReleaseLock"]["steps"] + checkout = validation[0] + assert checkout["uses"].startswith("actions/checkout@") + assert checkout["with"] == {"fetch-depth": 2} + # The changelog check uses the checked-out merge and its first parent. + changelog = next( + step + for step in validation + if step.get("name") == "Check changed changelog fragments" + ) + assert "HEAD^1" in changelog["run"] + assert "HEAD \\" in changelog["run"] + assert "origin/main" not in changelog["run"] + assert any( + step.get("run") == "python .github/release_lock.py --rehearse" + for step in validation + ) + feedback = JOBS["Quick-Feedback"]["steps"] + fetch = next(step for step in feedback if step.get("name") == "Fetch base branch") + assert fetch["run"] == "git fetch origin ${{ github.base_ref }} --depth=1000" + selective = next( + step + for step in feedback + if step.get("name") == "Run selective tests based on changed files" + ) + assert selective["env"]["GITHUB_BASE_REF"] == "${{ github.base_ref }}" + assert "--base-branch ${{ github.base_ref }}" in selective["run"] + for job in CHEAP_JOBS: + assert "origin/main" not in str(JOBS[job]) + assert "Merge main" not in str(action("candidate-wheel")) diff --git a/policyengine_us/tests/code_health/test_pr_workflow_skips_drafts.py b/policyengine_us/tests/code_health/test_pr_workflow_skips_drafts.py index 77d61c87857..bbe6ce67c68 100644 --- a/policyengine_us/tests/code_health/test_pr_workflow_skips_drafts.py +++ b/policyengine_us/tests/code_health/test_pr_workflow_skips_drafts.py @@ -1,4 +1,4 @@ -"""Draft pull requests must not run the PR workflow; ready ones run all of it.""" +"""Draft PRs skip every job; ready PRs run the tier selected by their base.""" from pathlib import Path @@ -21,6 +21,7 @@ def _needs(job): def test_ready_for_review_and_converted_to_draft_trigger_the_workflow(): trigger, _ = _workflow() + assert set(trigger["branches"]) == {"main", "integration"} assert {"opened", "synchronize", "reopened", "ready_for_review"} <= set( trigger["types"] ) diff --git a/policyengine_us/tests/code_health/test_promote_integration.py b/policyengine_us/tests/code_health/test_promote_integration.py new file mode 100644 index 00000000000..cc66d5c3468 --- /dev/null +++ b/policyengine_us/tests/code_health/test_promote_integration.py @@ -0,0 +1,400 @@ +"""Exercise promotion orchestration with tiny Git repositories and no API writes.""" + +from datetime import datetime, timezone +import importlib.util +import json +from pathlib import Path +import subprocess + +import pytest +import yaml + +ROOT = Path(__file__).resolve().parents[3] +REPOSITORY = "PolicyEngine/policyengine-us" +NOW = datetime(2026, 10, 8, 20, 17, tzinfo=timezone.utc) + + +def command(path, *args): + return subprocess.run( + ["git", *args], cwd=path, capture_output=True, text=True, check=True + ).stdout.strip() + + +class GitRepository: + def __init__(self, root): + self.remote = root / "remote.git" + self.work = root / "work" + command(root, "init", "--bare", "--initial-branch=main", str(self.remote)) + command(root, "clone", str(self.remote), str(self.work)) + self.configure(self.work) + self.initial = self.commit("initial.txt", "initial", "Initial main") + self.git("push", "origin", "main") + + @staticmethod + def configure(path): + command(path, "config", "user.name", "Promotion test") + command(path, "config", "user.email", "promotion-test@example.com") + + def git(self, *args): + return command(self.work, *args) + + def commit(self, name, contents, subject): + (self.work / name).write_text(contents) + self.git("add", name) + self.git("commit", "-m", subject) + return self.git("rev-parse", "HEAD") + + def integration(self): + self.git("checkout", "-b", "integration") + self.git("push", "origin", "integration") + + def refs(self): + output = command( + self.remote, "for-each-ref", "--format=%(refname) %(objectname)" + ) + return dict(line.split() for line in output.splitlines()) + + +@pytest.fixture +def repository(tmp_path, monkeypatch): + repository = GitRepository(tmp_path) + monkeypatch.chdir(repository.work) + return repository + + +@pytest.fixture +def promotion(): + spec = importlib.util.spec_from_file_location( + "promote_integration", ROOT / ".github/promote_integration.py" + ) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.fixture +def api(monkeypatch, promotion, repository): + class FakeAPI: + def __init__(self): + self.opened = [] + self.closed = [] + self.created = [] + self.queries = [] + + def __call__(self, repository, resource, *, fields=None, paginate=False): + assert repository == REPOSITORY + if fields is not None: + assert not paginate + if resource == "git/refs": + command( + local.remote, + "update-ref", + fields["ref"], + fields["sha"], + "0" * 40, + ) + return {"ref": fields["ref"]} + assert resource == "pulls" + self.created.append(fields) + return {"html_url": f"https://github.com/{REPOSITORY}/pull/123"} + self.queries.append((resource, paginate)) + assert paginate, "An existing promotion must be found beyond the first page" + if "state=open&base=main" in resource: + return self.opened + assert "state=closed&base=integration" in resource + return self.closed + + local = repository + fake = FakeAPI() + monkeypatch.setattr(promotion, "api", fake) + return fake + + +def test_absent_integration_is_inert(repository, promotion, api, capsys): + before = repository.refs() + promotion.run(REPOSITORY, NOW) + assert repository.refs() == before + assert not api.queries + assert not api.created + assert "branch is absent" in capsys.readouterr().out + + +def test_equal_integration_opens_no_promotion(repository, promotion, api, capsys): + repository.integration() + before = repository.refs() + promotion.run(REPOSITORY, NOW) + assert repository.refs() == before + assert not api.created + assert len(api.queries) == 1 + assert "no unpromoted commits" in capsys.readouterr().out + + +def test_open_promotion_freezes_every_remote_ref(repository, promotion, api): + repository.integration() + frozen = repository.commit("fix.txt", "first", "First member fix") + repository.git("push", "origin", "integration") + repository.git("push", "origin", "HEAD:refs/heads/promote/existing") + repository.commit("another.txt", "second", "Next batch member") + repository.git("push", "origin", "integration") + repository.git("checkout", "main") + repository.commit("version.txt", "1.0.1", "Update PolicyEngine US") + repository.git("push", "origin", "main") + api.opened = [ + { + "number": 42, + "head": {"ref": "promote/existing", "repo": {"full_name": REPOSITORY}}, + } + ] + before = repository.refs() + promotion.run(REPOSITORY, NOW) + assert repository.refs() == before + assert repository.refs()["refs/heads/promote/existing"] == frozen + assert not api.created + assert len(api.queries) == 1 + + +def test_ready_batch_creates_non_draft_frozen_snapshot(repository, promotion, api): + repository.integration() + first = repository.commit("first.txt", "first", "First member fix") + second = repository.commit("second.txt", "second", "Second member fix") + repository.git("push", "origin", "integration") + api.closed = [ + { + "number": 101, + "title": "First member fix", + "merged_at": "2026-10-08T12:00:00Z", + "merge_commit_sha": first, + }, + { + "number": 102, + "title": "Unmerged PR is excluded", + "merged_at": None, + "merge_commit_sha": second, + }, + { + "number": 103, + "title": "Previous batch is excluded", + "merged_at": "2026-10-07T12:00:00Z", + "merge_commit_sha": repository.initial, + }, + ] + # A branch with the same prefix in a fork does not block this repository. + api.opened = [ + { + "number": 99, + "head": {"ref": "promote/fork", "repo": {"full_name": "fork/repo"}}, + } + ] + promotion.run(REPOSITORY, NOW) + (pull,) = api.created + assert pull["head"] == "promote/20261008T201700000000Z" + assert pull["base"] == "main" + assert pull["draft"] is False + assert pull["title"].startswith("Promote integration") + assert repository.refs()[f"refs/heads/{pull['head']}"] == second + body = pull["body"] + assert first in body and second in body and repository.initial in body + assert body.index("First member fix", body.index("Member commits:")) < body.index( + "Second member fix", body.index("Member commits:") + ) + assert "#101: First member fix" in body + assert "#102" not in body and "#103" not in body + assert "**merge commit**" in body + repository.commit("later.txt", "later", "Next integration batch") + repository.git("push", "origin", "integration") + assert repository.refs()[f"refs/heads/{pull['head']}"] == second + + +def test_existing_snapshot_ref_is_never_advanced(repository, promotion, api): + repository.integration() + frozen = repository.commit("first.txt", "first", "First member") + branch = "promote/20261008T201700000000Z" + repository.git("push", "origin", f"HEAD:refs/heads/{branch}") + repository.commit("second.txt", "second", "Second member") + repository.git("push", "origin", "integration") + before = repository.refs() + with pytest.raises(subprocess.CalledProcessError): + promotion.run(REPOSITORY, NOW) + assert repository.refs() == before + assert repository.refs()[f"refs/heads/{branch}"] == frozen + assert not api.created + + +def test_promoted_integration_receives_version_bump_without_new_pr( + repository, promotion, api +): + repository.integration() + repository.commit("fix.txt", "fix", "Promoted fix") + repository.git("push", "origin", "integration") + repository.git("checkout", "main") + repository.git("merge", "--no-ff", "integration", "-m", "Merge promotion") + bumped = repository.commit("version.txt", "1.0.1", "Update PolicyEngine US") + repository.git("push", "origin", "main") + promotion.run(REPOSITORY, NOW) + assert repository.refs()["refs/heads/integration"] == bumped + assert not api.created + assert len(api.queries) == 1 + + +def test_new_member_after_promotion_preserves_main_and_member_commits( + repository, promotion, api +): + repository.integration() + repository.commit("old.txt", "old", "Previous batch fix") + repository.git("push", "origin", "integration") + repository.git("checkout", "main") + repository.git("merge", "--no-ff", "integration", "-m", "Merge promotion") + bumped = repository.commit("version.txt", "1.0.1", "Update PolicyEngine US") + repository.git("push", "origin", "main") + repository.git("checkout", "integration") + member = repository.commit("new.txt", "new", "Next batch fix") + repository.git("push", "origin", "integration") + promotion.run(REPOSITORY, NOW) + (pull,) = api.created + snapshot = repository.refs()[f"refs/heads/{pull['head']}"] + assert repository.refs()["refs/heads/integration"] == snapshot + assert promotion.ancestor(bumped, snapshot) + assert promotion.ancestor(member, snapshot) + assert (repository.work / "version.txt").read_text() == "1.0.1" + assert member in pull["body"] + + +def test_merge_conflict_leaves_remote_untouched_and_reports_resolution( + repository, promotion, api, capsys +): + repository.integration() + repository.commit("initial.txt", "integration", "Member change") + repository.git("push", "origin", "integration") + repository.git("checkout", "main") + repository.commit("initial.txt", "main", "Main change") + repository.git("push", "origin", "main") + before = repository.refs() + promotion.run(REPOSITORY, NOW) + assert repository.refs() == before + assert not api.created + assert not repository.git("status", "--porcelain") + assert "Resolve the conflict on integration" in capsys.readouterr().out + + +def test_concurrent_member_push_rejects_stale_sync_without_promotion( + repository, promotion, api, monkeypatch +): + repository.integration() + repository.commit("member.txt", "first", "First member") + repository.git("push", "origin", "integration") + repository.git("checkout", "main") + repository.commit("main.txt", "main", "Urgent main fix") + repository.git("push", "origin", "main") + other = repository.work.parent / "other" + command(other.parent, "clone", str(repository.remote), str(other)) + repository.configure(other) + command(other, "checkout", "integration") + (other / "race.txt").write_text("concurrent member") + command(other, "add", "race.txt") + command(other, "commit", "-m", "Concurrent member") + concurrent = command(other, "rev-parse", "HEAD") + original_git = promotion.git + + def racing_git(*args): + if args == ("push", "origin", "HEAD:refs/heads/integration"): + command(other, "push", "origin", "integration") + return original_git(*args) + + monkeypatch.setattr(promotion, "git", racing_git) + with pytest.raises(subprocess.CalledProcessError): + promotion.run(REPOSITORY, NOW) + assert repository.refs()["refs/heads/integration"] == concurrent + assert not any(ref.startswith("refs/heads/promote/") for ref in repository.refs()) + assert not api.created + + +def test_api_pagination_keeps_promotions_from_later_pages(promotion, monkeypatch): + pages = [[{"number": 1}], [{"number": 2}]] + calls = [] + + def run(args, **kwargs): + calls.append((args, kwargs)) + return subprocess.CompletedProcess(args, 0, stdout=json.dumps(pages)) + + monkeypatch.setattr(promotion.subprocess, "run", run) + assert promotion.api(REPOSITORY, "pulls?state=open", paginate=True) == [ + {"number": 1}, + {"number": 2}, + ] + ((args, kwargs),) = calls + assert args == [ + "gh", + "api", + f"repos/{REPOSITORY}/pulls?state=open", + "--paginate", + "--slurp", + ] + assert kwargs["check"] + + +def test_api_posts_draft_as_a_json_boolean(promotion, monkeypatch): + calls = [] + + def run(args, **kwargs): + calls.append((args, kwargs)) + return subprocess.CompletedProcess(args, 0, stdout='{"number": 123}') + + monkeypatch.setattr(promotion.subprocess, "run", run) + fields = {"head": "promote/snapshot", "base": "main", "draft": False} + assert promotion.api(REPOSITORY, "pulls", fields=fields) == {"number": 123} + ((args, kwargs),) = calls + assert args == [ + "gh", + "api", + f"repos/{REPOSITORY}/pulls", + "--method", + "POST", + "--input", + "-", + ] + assert json.loads(kwargs["input"]) == fields + + +def test_promotion_workflow_keeps_token_gate_and_bounded_schedule(): + workflow = yaml.safe_load( + (ROOT / ".github/workflows/promote-integration.yaml").read_text() + ) + push = yaml.safe_load((ROOT / ".github/workflows/push.yaml").read_text()) + assert push[True] == {"push": {"branches": ["main"]}} + assert workflow[True] == { + "schedule": [{"cron": "17 */4 * * *"}], + "workflow_dispatch": None, + } + assert workflow["permissions"] == {"contents": "read"} + assert workflow["concurrency"] == { + "group": "promote-integration", + "cancel-in-progress": False, + } + assert set(workflow["jobs"]) == {"promote"} + job = workflow["jobs"]["promote"] + assert job["timeout-minutes"] == 10 + assert job["if"] == "github.repository == 'PolicyEngine/policyengine-us'" + steps = job["steps"] + pilot = next(step for step in steps if step.get("id") == "pilot") + token = next(step for step in steps if step.get("id") == "app-token") + push_token = next( + step + for step in push["jobs"]["versioning"]["steps"] + if step.get("id") == "app-token" + ) + assert token["uses"] == push_token["uses"] + assert token["with"] == { + **push_token["with"], + "permission-contents": "write", + "permission-pull-requests": "write", + "permission-workflows": "write", + } + assert "git ls-remote --heads origin refs/heads/integration" in pilot["run"] + assert steps.index(pilot) < steps.index(token) + gate = "steps.pilot.outputs.enabled == 'true'" + assert all(step["if"] == gate for step in steps[steps.index(token) :]) + run = steps[-1] + assert run["env"]["GH_TOKEN"] == "${{ steps.app-token.outputs.token }}" + checkout = steps[-2] + assert checkout["with"]["token"] == "${{ steps.app-token.outputs.token }}" + assert checkout["with"]["fetch-depth"] == 0 From 2b65f9d4435e8461ac70c415685f3a451478160e Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 9 Oct 2026 07:47:55 -0400 Subject: [PATCH 2/3] Harden integration promotion ancestry and workflow contracts Reject rewritten promotion history before synchronization, pin PR job dependencies and guards, and cover retry and no-op promotion ticks. Document safe bisect confirmation and workflow serialization. Co-Authored-By: Claude Opus 5.5 --- .github/promote_integration.py | 37 +++++- CONTRIBUTING.md | 14 ++- .../ci-capacity-integration-branch.changed.md | 2 +- .../code_health/test_pr_runner_budget.py | 33 ++++- .../code_health/test_promote_integration.py | 113 +++++++++++++++++- 5 files changed, 182 insertions(+), 17 deletions(-) diff --git a/.github/promote_integration.py b/.github/promote_integration.py index 6630d7bd7f2..3e4f4925454 100644 --- a/.github/promote_integration.py +++ b/.github/promote_integration.py @@ -45,6 +45,14 @@ def ancestor(older: str, newer: str) -> bool: return result.returncode == 0 +def is_promotion(repository: str, pull: dict) -> bool: + head = pull["head"] + return ( + head["ref"].startswith("promote/") + and (head.get("repo") or {}).get("full_name", "").lower() == repository.lower() + ) + + def run(repository: str, now: datetime | None = None) -> None: if not git("ls-remote", "--heads", "origin", "refs/heads/integration"): print("::notice::Integration branch is absent; the pilot is inactive.") @@ -56,17 +64,34 @@ def run(repository: str, now: datetime | None = None) -> None: "+refs/heads/main:refs/remotes/origin/main", "+refs/heads/integration:refs/remotes/origin/integration", ) + # Checking for an open promotion and creating one are separate API calls. + # The workflow's constant concurrency group serializes scheduled and + # dispatched runs so they cannot create competing snapshots. opened = api(repository, "pulls?state=open&base=main&per_page=100", paginate=True) for pull in opened: - head = pull["head"] - if ( - head["ref"].startswith("promote/") - and (head.get("repo") or {}).get("full_name", "").lower() - == repository.lower() - ): + if is_promotion(repository, pull): print(f"::notice::Promotion #{pull['number']} is open; leaving it frozen.") return + closed_promotions = api( + repository, "pulls?state=closed&base=main&per_page=100", paginate=True + ) + latest = max( + ( + pull + for pull in closed_promotions + if pull.get("merged_at") and is_promotion(repository, pull) + ), + key=lambda pull: pull["merged_at"], + default=None, + ) + if latest and not ancestor(latest["head"]["sha"], "origin/main"): + print( + f"::error::Promotion #{latest['number']} was not merged with a merge " + "commit and needs manual recovery before integration can be synchronized." + ) + raise SystemExit(1) + git("checkout", "-B", "integration", "origin/integration") if not ancestor("origin/main", "HEAD"): try: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 06aa276a9df..fc49d734fdc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -63,10 +63,16 @@ the current head on `promote/` and opens a non-draft PR into the next promotion; the workflow never updates an existing snapshot. The PR uses the GitHub App token so its CI runs automatically, as described in [GitHub's workflow trigger documentation](https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/trigger-a-workflow#triggering-a-workflow-from-a-workflow). +When `integration` only lacks commits from `main`, the tick synchronizes and +pushes it without opening a promotion. Merge a successful promotion with a **merge commit**, preserving each fix's -commits and changelog fragments. Do not squash the promotion. The next workflow -tick brings `main`, including the release bot's version bump, back into +commits and changelog fragments. Do not squash or rebase the promotion. Before +synchronizing, the workflow checks that the frozen head of the most recently +merged `promote/*` PR from this repository into `main` is an ancestor of `main`. +If it is not, the job fails with an error naming the PR and requiring manual +recovery before another batch can proceed. The next successful workflow tick +brings `main`, including the release bot's version bump, back into `integration` with a clean merge. A merge conflict stops the promotion and emits a warning; a maintainer resolves the conflict on `integration` before running the workflow again. A push to `integration` starts no full-suite workflow. @@ -81,6 +87,10 @@ For a red promotion, preserve the failed PR and logs while investigating: First-parent history follows the member fixes in their landing order. If the result is a merge, inspect the member commits or the synchronization with `main` to identify the failing change. + Member commits before the last synchronization lack newer `main` fixes. + Before reverting a suspected culprit, confirm it by reverting it on the + frozen snapshot (or merging the recorded pre-batch `main` into the candidate) + and re-running the failing shard's exact command. 3. Revert the culprit on `integration` (`git revert `, or `git revert -m 1 ` for a member-PR merge), with a changelog fragment. Close the failed promotion PR, keeping it as evidence, and run the promotion diff --git a/changelog.d/ci-capacity-integration-branch.changed.md b/changelog.d/ci-capacity-integration-branch.changed.md index 76e8a294b85..784e9af9945 100644 --- a/changelog.d/ci-capacity-integration-branch.changed.md +++ b/changelog.d/ci-capacity-integration-branch.changed.md @@ -1 +1 @@ -Add an opt-in integration branch CI flow with selective PR checks and frozen full-suite promotion PRs into main. +Add an opt-in integration branch CI flow with selective PR checks and frozen full-suite promotion PRs into main, stopping for manual recovery when a promotion's frozen commits were not preserved by a merge commit. diff --git a/policyengine_us/tests/code_health/test_pr_runner_budget.py b/policyengine_us/tests/code_health/test_pr_runner_budget.py index 5dbc67f5255..0155324eae2 100644 --- a/policyengine_us/tests/code_health/test_pr_runner_budget.py +++ b/policyengine_us/tests/code_health/test_pr_runner_budget.py @@ -18,6 +18,15 @@ READY_GUARD = "github.event.pull_request.draft == false" MAIN_ONLY = "github.base_ref == 'main'" CHEAP_JOBS = {"ReleaseLock", "PackageCompatibility", "Quick-Feedback"} +JOB_DEPENDENCIES_AND_GUARDS = { + "ReleaseLock": {"if": READY_GUARD}, + "PackageCompatibility": {"needs": "ReleaseLock"}, + "Quick-Feedback": {"needs": "ReleaseLock"}, + "Baseline": {"needs": "ReleaseLock", "if": MAIN_ONLY}, + "HouseholdAPIPartners": {"needs": "ReleaseLock", "if": MAIN_ONLY}, + "Contrib": {"needs": "ReleaseLock", "if": MAIN_ONLY}, + "PythonTests": {"needs": "ReleaseLock", "if": MAIN_ONLY}, +} # The main PR runner layout before the integration pilot, including each shard. MAIN_RUNNER_COUNTS = { "ReleaseLock": 1, @@ -31,8 +40,8 @@ } -def eligible_jobs(base, draft): - """Evaluate the supported PR guards and successful dependency chain.""" +def eligible_jobs(base, draft, failed_jobs=()): + """Model supported guards and the implicit successful dependency requirement.""" conditions = {None: True, READY_GUARD: not draft, MAIN_ONLY: base == "main"} def eligible(name): @@ -42,7 +51,9 @@ def eligible(name): needs = job.get("needs", []) if isinstance(needs, str): needs = [needs] - return conditions[condition] and all(eligible(parent) for parent in needs) + return conditions[condition] and all( + parent not in failed_jobs and eligible(parent) for parent in needs + ) return {name for name in JOBS if eligible(name)} @@ -96,12 +107,28 @@ def test_pr_runner_budget_keeps_the_heavy_suites_separate(): ], ) def test_pr_jobs_and_runner_budget_by_base_and_draft(base, draft, expected): + assert { + name: {key: job[key] for key in ("needs", "if") if key in job} + for name, job in JOBS.items() + } == JOB_DEPENDENCIES_AND_GUARDS jobs = eligible_jobs(base, draft) assert runner_counts(jobs) == expected if base == "integration" and not draft: assert jobs == CHEAP_JOBS +@pytest.mark.parametrize("base", ["main", "integration"]) +def test_failed_release_lock_prevents_all_dependent_runners(base): + # The exact needs/if contract above rules out bypassing dependency success + # with always() or a status guard. This bounded model checks the intended + # failure route, including main-only suites; static YAML cannot verify + # GitHub's live expression evaluation or actual scheduling after a failure. + jobs = eligible_jobs(base, draft=False, failed_jobs={"ReleaseLock"}) + assert jobs == {"ReleaseLock"} + assert runner_counts(jobs) == {"ReleaseLock": 1} + assert all(not job.get("continue-on-error") for job in JOBS.values()) + + def test_main_runner_checks_and_matrix_members_are_unchanged(): assert set(eligible_jobs("main", draft=False)) == { "ReleaseLock", diff --git a/policyengine_us/tests/code_health/test_promote_integration.py b/policyengine_us/tests/code_health/test_promote_integration.py index cc66d5c3468..87a1ecce128 100644 --- a/policyengine_us/tests/code_health/test_promote_integration.py +++ b/policyengine_us/tests/code_health/test_promote_integration.py @@ -1,6 +1,6 @@ """Exercise promotion orchestration with tiny Git repositories and no API writes.""" -from datetime import datetime, timezone +from datetime import datetime, timedelta, timezone import importlib.util import json from pathlib import Path @@ -78,12 +78,15 @@ class FakeAPI: def __init__(self): self.opened = [] self.closed = [] + self.closed_main = [] self.created = [] self.queries = [] + self.writes = [] def __call__(self, repository, resource, *, fields=None, paginate=False): assert repository == REPOSITORY if fields is not None: + self.writes.append((resource, fields)) assert not paginate if resource == "git/refs": command( @@ -101,6 +104,8 @@ def __call__(self, repository, resource, *, fields=None, paginate=False): assert paginate, "An existing promotion must be found beyond the first page" if "state=open&base=main" in resource: return self.opened + if "state=closed&base=main" in resource: + return self.closed_main assert "state=closed&base=integration" in resource return self.closed @@ -110,6 +115,18 @@ def __call__(self, repository, resource, *, fields=None, paginate=False): return fake +def closed_promotion(snapshot, *, number=42, merged_at="2026-10-08T12:00:00Z"): + return { + "number": number, + "merged_at": merged_at, + "head": { + "ref": f"promote/{number}", + "sha": snapshot, + "repo": {"full_name": REPOSITORY}, + }, + } + + def test_absent_integration_is_inert(repository, promotion, api, capsys): before = repository.refs() promotion.run(REPOSITORY, NOW) @@ -119,13 +136,17 @@ def test_absent_integration_is_inert(repository, promotion, api, capsys): assert "branch is absent" in capsys.readouterr().out -def test_equal_integration_opens_no_promotion(repository, promotion, api, capsys): +def test_two_ticks_without_new_commits_open_no_promotion( + repository, promotion, api, capsys +): repository.integration() before = repository.refs() promotion.run(REPOSITORY, NOW) + promotion.run(REPOSITORY, NOW + timedelta(hours=4)) assert repository.refs() == before assert not api.created - assert len(api.queries) == 1 + assert not api.writes + assert len(api.queries) == 4 assert "no unpromoted commits" in capsys.readouterr().out @@ -186,6 +207,8 @@ def test_ready_batch_creates_non_draft_frozen_snapshot(repository, promotion, ap } ] promotion.run(REPOSITORY, NOW) + assert not api.closed_main + assert ("pulls?state=closed&base=main&per_page=100", True) in api.queries (pull,) = api.created assert pull["head"] == "promote/20261008T201700000000Z" assert pull["base"] == "main" @@ -205,6 +228,78 @@ def test_ready_batch_creates_non_draft_frozen_snapshot(repository, promotion, ap assert repository.refs()[f"refs/heads/{pull['head']}"] == second +def test_closed_unmerged_promotion_allows_a_fresh_snapshot(repository, promotion, api): + repository.integration() + snapshot = repository.commit("fix.txt", "fix", "Member fix") + repository.git("push", "origin", "integration") + promotion.run(REPOSITORY, NOW) + (first,) = api.created + closed = closed_promotion(snapshot, merged_at=None) + closed["head"]["ref"] = first["head"] + api.closed_main = [closed] + promotion.run(REPOSITORY, NOW + timedelta(hours=4)) + assert len(api.created) == 2 + second = api.created[1] + assert first["head"] != second["head"] + refs = repository.refs() + assert refs[f"refs/heads/{first['head']}"] == snapshot + assert refs[f"refs/heads/{second['head']}"] == snapshot + assert refs["refs/heads/integration"] == snapshot + + +@pytest.mark.parametrize("method", ["squash", "rebase"]) +def test_rewritten_promotion_fails_before_sync_push_or_api_writes( + repository, promotion, api, monkeypatch, capsys, method +): + repository.integration() + snapshot = repository.commit("fix.txt", "fix", "Promoted fix") + repository.git("push", "origin", "integration") + repository.git("checkout", "main") + if method == "squash": + repository.git("merge", "--squash", "integration") + repository.git("commit", "-m", "Squash promotion") + else: + # Replaying on newer main gives the member a different parent and SHA. + repository.commit("main.txt", "main", "New main fix") + repository.git("cherry-pick", snapshot) + repository.commit("version.txt", "1.0.1", "Update PolicyEngine US") + repository.git("push", "origin", "main") + latest = closed_promotion(snapshot) + fork = closed_promotion(snapshot, number=99, merged_at="2026-10-09T12:00:00Z") + fork["head"]["repo"]["full_name"] = "fork/repo" + ordinary = closed_promotion(snapshot, number=100, merged_at="2026-10-09T12:00:00Z") + ordinary["head"]["ref"] = "ordinary-fix" + api.closed_main = [ + latest, + closed_promotion( + repository.initial, number=41, merged_at="2026-10-07T12:00:00Z" + ), + fork, + ordinary, + closed_promotion(snapshot, number=101, merged_at=None), + ] + before = repository.refs() + calls = [] + original_git = promotion.git + + def tracked_git(*args): + calls.append(args) + return original_git(*args) + + monkeypatch.setattr(promotion, "git", tracked_git) + with pytest.raises(SystemExit) as exc: + promotion.run(REPOSITORY, NOW) + assert exc.value.code == 1 + assert repository.refs() == before + assert not any(args[0] in {"checkout", "merge", "push"} for args in calls) + assert not api.writes + assert not api.created + output = capsys.readouterr().out + assert "::error::Promotion #42" in output + assert "was not merged with a merge commit" in output + assert "needs manual recovery" in output + + def test_existing_snapshot_ref_is_never_advanced(repository, promotion, api): repository.integration() frozen = repository.commit("first.txt", "first", "First member") @@ -224,16 +319,24 @@ def test_promoted_integration_receives_version_bump_without_new_pr( repository, promotion, api ): repository.integration() - repository.commit("fix.txt", "fix", "Promoted fix") + snapshot = repository.commit("fix.txt", "fix", "Promoted fix") repository.git("push", "origin", "integration") repository.git("checkout", "main") repository.git("merge", "--no-ff", "integration", "-m", "Merge promotion") bumped = repository.commit("version.txt", "1.0.1", "Update PolicyEngine US") repository.git("push", "origin", "main") + # API order is unrelated to merge order; the latest promotion's head is + # preserved in main even though an older API record is not an ancestor. + unrelated = repository.commit("unused.txt", "unused", "Unmerged local commit") + api.closed_main = [ + closed_promotion(unrelated, number=41, merged_at="2026-10-07T12:00:00Z"), + closed_promotion(snapshot), + ] promotion.run(REPOSITORY, NOW) assert repository.refs()["refs/heads/integration"] == bumped assert not api.created - assert len(api.queries) == 1 + assert not api.writes + assert len(api.queries) == 2 def test_new_member_after_promotion_preserves_main_and_member_commits( From d7d964648caaa8e433062ae8dd5a9cf8a43f13d0 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 9 Oct 2026 09:38:19 -0400 Subject: [PATCH 3/3] Make promotion guard cheap, diagnosable and recoverable Co-Authored-By: Claude Opus 5.5 --- .github/promote_integration.py | 84 ++++- .github/promotion-recovered.txt | 1 + CONTRIBUTING.md | 19 +- .../ci-capacity-integration-branch.changed.md | 2 +- .../code_health/test_promote_integration.py | 310 +++++++++++++++++- 5 files changed, 386 insertions(+), 30 deletions(-) create mode 100644 .github/promotion-recovered.txt diff --git a/.github/promote_integration.py b/.github/promote_integration.py index 3e4f4925454..f87fcf3d183 100644 --- a/.github/promote_integration.py +++ b/.github/promote_integration.py @@ -53,6 +53,46 @@ def is_promotion(repository: str, pull: dict) -> bool: ) +def recovered_promotions() -> set[int]: + path = ".github/promotion-recovered.txt" + try: + contents = git("show", f"origin/main:{path}") + except subprocess.CalledProcessError: + # Only an absent file is empty; other Git failures must stop the tick. + if git("ls-tree", "origin/main", "--", path): + raise + return set() + numbers = (line.partition("#")[0].strip() for line in contents.splitlines()) + return {int(number) for number in numbers if number} + + +def latest_merged_promotion(repository: str, recovered: set[int]) -> dict | None: + latest = None + page = 1 + while True: + pulls = api( + repository, + "pulls?state=closed&base=main&sort=updated&direction=desc" + f"&per_page=100&page={page}", + ) + for pull in pulls: + if ( + pull.get("merged_at") + and pull["number"] not in recovered + and is_promotion(repository, pull) + and (latest is None or pull["merged_at"] > latest["merged_at"]) + ): + latest = pull + if len(pulls) < 100: + return latest + # merged_at <= updated_at for every PR. In updated-descending order, + # once this page's oldest update precedes the best merge, no later + # page can contain a newer merge (including unacknowledged promotions). + if latest and pulls[-1]["updated_at"] < latest["merged_at"]: + return latest + page += 1 + + def run(repository: str, now: datetime | None = None) -> None: if not git("ls-remote", "--heads", "origin", "refs/heads/integration"): print("::notice::Integration branch is absent; the pilot is inactive.") @@ -73,24 +113,32 @@ def run(repository: str, now: datetime | None = None) -> None: print(f"::notice::Promotion #{pull['number']} is open; leaving it frozen.") return - closed_promotions = api( - repository, "pulls?state=closed&base=main&per_page=100", paginate=True - ) - latest = max( - ( - pull - for pull in closed_promotions - if pull.get("merged_at") and is_promotion(repository, pull) - ), - key=lambda pull: pull["merged_at"], - default=None, - ) - if latest and not ancestor(latest["head"]["sha"], "origin/main"): - print( - f"::error::Promotion #{latest['number']} was not merged with a merge " - "commit and needs manual recovery before integration can be synchronized." - ) - raise SystemExit(1) + latest = latest_merged_promotion(repository, recovered_promotions()) + if latest: + try: + # GitHub retains the PR head ref after deleting the source branch. + git( + "fetch", + "--no-tags", + "origin", + f"+refs/pull/{latest['number']}/head:refs/remotes/origin/promotion-head", + ) + preserved = ancestor("origin/promotion-head", "origin/main") + except (subprocess.CalledProcessError, OSError) as exc: + detail = ( + (exc.stderr or exc.stdout or str(exc)) + if isinstance(exc, subprocess.CalledProcessError) + else str(exc) + ) + reason = " ".join(detail.splitlines()) + print(f"::error::Cannot verify promotion #{latest['number']}: {reason}") + raise SystemExit(1) from None + if not preserved: + print( + f"::error::Promotion #{latest['number']} was not merged with a merge " + "commit and needs manual recovery before integration can be synchronized." + ) + raise SystemExit(1) git("checkout", "-B", "integration", "origin/integration") if not ancestor("origin/main", "HEAD"): diff --git a/.github/promotion-recovered.txt b/.github/promotion-recovered.txt new file mode 100644 index 00000000000..2c7b06fb8fc --- /dev/null +++ b/.github/promotion-recovered.txt @@ -0,0 +1 @@ +# Reconciled rewritten promotion PRs: one PR number per line, with # comments allowed. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fc49d734fdc..d44cbe59f4c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -69,7 +69,8 @@ pushes it without opening a promotion. Merge a successful promotion with a **merge commit**, preserving each fix's commits and changelog fragments. Do not squash or rebase the promotion. Before synchronizing, the workflow checks that the frozen head of the most recently -merged `promote/*` PR from this repository into `main` is an ancestor of `main`. +merged, unreconciled `promote/*` PR from this repository into `main` is an ancestor +of `main`. If it is not, the job fails with an error naming the PR and requiring manual recovery before another batch can proceed. The next successful workflow tick brings `main`, including the release bot's version bump, back into @@ -77,6 +78,16 @@ brings `main`, including the release bot's version bump, back into a warning; a maintainer resolves the conflict on `integration` before running the workflow again. A push to `integration` starts no full-suite workflow. +If a promotion is accidentally squashed or rebased, first reset `integration` +to `main` or repair its history so the commits already replayed into `main` are +dropped. Then add the reconciled promotion's PR number to +`.github/promotion-recovered.txt` through a PR into `main`. The file accepts one +PR number per line and `#` comments. This acknowledges that the rewritten merge +has been reconciled; resetting `integration` alone does not clear the guard. +The workflow reads the file from `main`, skips acknowledged promotions, and +checks the next-newest promotion. An older unreconciled promotion still stops +the workflow if its frozen head is outside `main`'s ancestry. + For a red promotion, preserve the failed PR and logs while investigating: 1. In a separate local checkout of the frozen snapshot, reproduce the failure @@ -89,8 +100,10 @@ For a red promotion, preserve the failed PR and logs while investigating: `main` to identify the failing change. Member commits before the last synchronization lack newer `main` fixes. Before reverting a suspected culprit, confirm it by reverting it on the - frozen snapshot (or merging the recorded pre-batch `main` into the candidate) - and re-running the failing shard's exact command. + frozen snapshot and requiring the failing shard's exact command to pass. + Alternatively, merge the recorded pre-batch `main` into both the candidate + and its first-parent predecessor, then require that command to fail on the + candidate and pass on its predecessor with the same `main` merged in. 3. Revert the culprit on `integration` (`git revert `, or `git revert -m 1 ` for a member-PR merge), with a changelog fragment. Close the failed promotion PR, keeping it as evidence, and run the promotion diff --git a/changelog.d/ci-capacity-integration-branch.changed.md b/changelog.d/ci-capacity-integration-branch.changed.md index 784e9af9945..ff23ec2372e 100644 --- a/changelog.d/ci-capacity-integration-branch.changed.md +++ b/changelog.d/ci-capacity-integration-branch.changed.md @@ -1 +1 @@ -Add an opt-in integration branch CI flow with selective PR checks and frozen full-suite promotion PRs into main, stopping for manual recovery when a promotion's frozen commits were not preserved by a merge commit. +Add an opt-in integration branch CI flow with selective PR checks and frozen full-suite promotion PRs into main, stopping for manual recovery when the latest merged promotion's frozen commits were not preserved by a merge commit and allowing reconciled rewritten merges to be acknowledged explicitly. diff --git a/policyengine_us/tests/code_health/test_promote_integration.py b/policyengine_us/tests/code_health/test_promote_integration.py index 87a1ecce128..0c88fa71b6e 100644 --- a/policyengine_us/tests/code_health/test_promote_integration.py +++ b/policyengine_us/tests/code_health/test_promote_integration.py @@ -5,6 +5,7 @@ import json from pathlib import Path import subprocess +from urllib.parse import parse_qs, urlsplit import pytest import yaml @@ -39,6 +40,7 @@ def git(self, *args): return command(self.work, *args) def commit(self, name, contents, subject): + (self.work / name).parent.mkdir(parents=True, exist_ok=True) (self.work / name).write_text(contents) self.git("add", name) self.git("commit", "-m", subject) @@ -48,6 +50,9 @@ def integration(self): self.git("checkout", "-b", "integration") self.git("push", "origin", "integration") + def publish_pull_head(self, number, snapshot): + self.git("push", "origin", f"{snapshot}:refs/pull/{number}/head") + def refs(self): output = command( self.remote, "for-each-ref", "--format=%(refname) %(objectname)" @@ -101,11 +106,25 @@ def __call__(self, repository, resource, *, fields=None, paginate=False): self.created.append(fields) return {"html_url": f"https://github.com/{REPOSITORY}/pull/123"} self.queries.append((resource, paginate)) - assert paginate, "An existing promotion must be found beyond the first page" + if "state=closed&base=main" in resource: + assert not paginate + query = parse_qs(urlsplit(resource).query) + assert query == { + "state": ["closed"], + "base": ["main"], + "sort": ["updated"], + "direction": ["desc"], + "per_page": ["100"], + "page": query["page"], + } + page = int(query["page"][0]) + ordered = sorted( + self.closed_main, key=lambda pull: pull["updated_at"], reverse=True + ) + return ordered[(page - 1) * 100 : page * 100] + assert paginate, "Open and integration PR queries must read every page" if "state=open&base=main" in resource: return self.opened - if "state=closed&base=main" in resource: - return self.closed_main assert "state=closed&base=integration" in resource return self.closed @@ -115,10 +134,17 @@ def __call__(self, repository, resource, *, fields=None, paginate=False): return fake -def closed_promotion(snapshot, *, number=42, merged_at="2026-10-08T12:00:00Z"): +def closed_promotion( + snapshot, + *, + number=42, + merged_at="2026-10-08T12:00:00Z", + updated_at=None, +): return { "number": number, "merged_at": merged_at, + "updated_at": updated_at or merged_at or "2026-10-08T12:00:00Z", "head": { "ref": f"promote/{number}", "sha": snapshot, @@ -127,6 +153,110 @@ def closed_promotion(snapshot, *, number=42, merged_at="2026-10-08T12:00:00Z"): } +def ordinary_pulls(count, updated_at, *, first_number=1000): + pulls = [ + closed_promotion( + "unused", number=first_number + index, updated_at=updated_at, merged_at=None + ) + for index in range(count) + ] + for pull in pulls: + pull["head"]["ref"] = "ordinary-fix" + return pulls + + +def page_api(promotion, monkeypatch, pages): + calls = [] + + def api(repository, resource, *, paginate=False): + assert repository == REPOSITORY + assert not paginate + page = len(calls) + 1 + assert resource == ( + "pulls?state=closed&base=main&sort=updated&direction=desc" + f"&per_page=100&page={page}" + ) + calls.append(resource) + return pages[page - 1] + + monkeypatch.setattr(promotion, "api", api) + return calls + + +def test_latest_promotion_stops_after_first_full_page(promotion, monkeypatch): + candidate = closed_promotion("latest", updated_at="2026-10-08T13:00:00Z") + pages = [ + [candidate, *ordinary_pulls(99, "2026-10-08T11:00:00Z")], + [closed_promotion("older", number=41, merged_at="2026-10-07T12:00:00Z")], + ] + calls = page_api(promotion, monkeypatch, pages) + assert promotion.latest_merged_promotion(REPOSITORY, set()) is candidate + assert len(calls) == 1 + + +def test_latest_promotion_can_be_on_third_page(promotion, monkeypatch): + candidate = closed_promotion("latest") + calls = page_api( + promotion, + monkeypatch, + [ + ordinary_pulls(100, "2026-10-10T12:00:00Z"), + ordinary_pulls(100, "2026-10-09T12:00:00Z", first_number=1100), + [candidate], + ], + ) + assert promotion.latest_merged_promotion(REPOSITORY, set()) is candidate + assert len(calls) == 3 + + +def test_newer_update_does_not_hide_newest_merge_on_later_page(promotion, monkeypatch): + older = closed_promotion( + "older", + number=41, + merged_at="2026-10-07T12:00:00Z", + updated_at="2026-10-10T12:00:00Z", + ) + latest = closed_promotion("latest", updated_at="2026-10-08T13:00:00Z") + calls = page_api( + promotion, + monkeypatch, + [ + [older, *ordinary_pulls(99, "2026-10-09T12:00:00Z")], + [latest], + ], + ) + assert promotion.latest_merged_promotion(REPOSITORY, set()) is latest + assert len(calls) == 2 + + +def test_no_promotion_reads_every_page_and_returns_none(promotion, monkeypatch): + fork = closed_promotion("fork") + fork["head"]["repo"]["full_name"] = "fork/repo" + closed = closed_promotion("unmerged", number=43, merged_at=None) + calls = page_api( + promotion, + monkeypatch, + [ + ordinary_pulls(100, "2026-10-10T12:00:00Z"), + ordinary_pulls(100, "2026-10-09T12:00:00Z", first_number=1100), + [fork, closed], + ], + ) + assert promotion.latest_merged_promotion(REPOSITORY, set()) is None + assert len(calls) == 3 + + +def test_equal_update_boundary_continues_to_next_page(promotion, monkeypatch): + candidate = closed_promotion("latest") + calls = page_api( + promotion, + monkeypatch, + [[candidate, *ordinary_pulls(99, candidate["merged_at"])], []], + ) + assert promotion.latest_merged_promotion(REPOSITORY, set()) is candidate + assert len(calls) == 2 + + def test_absent_integration_is_inert(repository, promotion, api, capsys): before = repository.refs() promotion.run(REPOSITORY, NOW) @@ -208,7 +338,10 @@ def test_ready_batch_creates_non_draft_frozen_snapshot(repository, promotion, ap ] promotion.run(REPOSITORY, NOW) assert not api.closed_main - assert ("pulls?state=closed&base=main&per_page=100", True) in api.queries + assert ( + "pulls?state=closed&base=main&sort=updated&direction=desc&per_page=100&page=1", + False, + ) in api.queries (pull,) = api.created assert pull["head"] == "promote/20261008T201700000000Z" assert pull["base"] == "main" @@ -254,6 +387,7 @@ def test_rewritten_promotion_fails_before_sync_push_or_api_writes( repository.integration() snapshot = repository.commit("fix.txt", "fix", "Promoted fix") repository.git("push", "origin", "integration") + repository.publish_pull_head(42, snapshot) repository.git("checkout", "main") if method == "squash": repository.git("merge", "--squash", "integration") @@ -300,6 +434,160 @@ def tracked_git(*args): assert "needs manual recovery" in output +def test_unfetchable_promotion_head_reports_error_before_any_writes( + repository, promotion, api, monkeypatch, capsys +): + repository.integration() + repository.commit("fix.txt", "fix", "Unpromoted fix") + repository.git("push", "origin", "integration") + repository.git("checkout", "main") + repository.commit("main.txt", "main", "New main fix") + repository.git("push", "origin", "main") + api.closed_main = [closed_promotion("f" * 40)] + before = repository.refs() + calls = [] + original_git = promotion.git + + def tracked_git(*args): + calls.append(args) + return original_git(*args) + + monkeypatch.setattr(promotion, "git", tracked_git) + with pytest.raises(SystemExit) as exc: + promotion.run(REPOSITORY, NOW) + assert exc.value.code == 1 + assert ( + "fetch", + "--no-tags", + "origin", + "+refs/pull/42/head:refs/remotes/origin/promotion-head", + ) in calls + assert not any(args[0] in {"checkout", "merge", "push"} for args in calls) + assert repository.refs() == before + assert not api.created + assert not api.writes + output = capsys.readouterr().out + assert "::error::Cannot verify promotion #42" in output + assert "refs/pull/42/head" in output + + +def test_ancestry_error_reports_promotion_instead_of_unhandled_exception( + repository, promotion, api, monkeypatch, capsys +): + repository.integration() + snapshot = repository.commit("fix.txt", "fix", "Promoted fix") + repository.git("push", "origin", "integration") + repository.publish_pull_head(42, snapshot) + api.closed_main = [closed_promotion(snapshot)] + before = repository.refs() + + def broken_ancestor(older, newer): + assert (older, newer) == ("origin/promotion-head", "origin/main") + raise subprocess.CalledProcessError( + 128, + ["git", "merge-base", "--is-ancestor", older, newer], + stderr="fatal: unable to read tree object\n", + ) + + monkeypatch.setattr(promotion, "ancestor", broken_ancestor) + with pytest.raises(SystemExit) as exc: + promotion.run(REPOSITORY, NOW) + assert exc.value.code == 1 + assert repository.refs() == before + assert not api.created + assert not api.writes + output = capsys.readouterr().out + assert "::error::Cannot verify promotion #42" in output + assert "fatal: unable to read tree object" in output + + +def test_recovered_promotion_file_is_read_from_main_with_comments( + repository, promotion +): + repository.commit( + ".github/promotion-recovered.txt", + "# Reconciled promotions\n42\n\n 41 # Older repaired batch\n", + "Acknowledge repaired promotions", + ) + repository.git("push", "origin", "main") + # An uncommitted local file cannot acknowledge recovery for the guard. + (repository.work / ".github/promotion-recovered.txt").write_text("99\n") + assert promotion.recovered_promotions() == {41, 42} + + +def test_missing_recovery_file_is_empty(repository, promotion): + assert promotion.recovered_promotions() == set() + + +def test_acknowledged_squash_allows_repaired_integration_to_run( + repository, promotion, api, capsys +): + repository.integration() + snapshot = repository.commit("fix.txt", "fix", "Promoted fix") + repository.git("push", "origin", "integration") + repository.publish_pull_head(42, snapshot) + repository.git("checkout", "main") + repository.git("merge", "--squash", "integration") + repository.git("commit", "-m", "Squash promotion") + repository.commit( + ".github/promotion-recovered.txt", + "# Reconciled promotions\n42\n", + "Recover #42", + ) + repository.git("push", "origin", "main") + repository.git("checkout", "integration") + repository.git("reset", "--hard", "main") + repository.git("push", "--force", "origin", "integration") + api.closed_main = [closed_promotion(snapshot)] + before = repository.refs() + promotion.run(REPOSITORY, NOW) + assert repository.refs() == before + assert not api.created + assert not api.writes + assert "no unpromoted commits" in capsys.readouterr().out + + +def test_acknowledged_squash_does_not_hide_older_unreconciled_squash( + repository, promotion, api, capsys +): + repository.integration() + older = repository.commit("older.txt", "older", "Older promoted fix") + repository.git("push", "origin", "integration") + repository.publish_pull_head(41, older) + repository.git("checkout", "main") + repository.git("merge", "--squash", "integration") + repository.git("commit", "-m", "Squash older promotion") + repository.git("checkout", "integration") + repository.git("reset", "--hard", "main") + latest = repository.commit("latest.txt", "latest", "Latest promoted fix") + repository.git("push", "--force", "origin", "integration") + repository.publish_pull_head(42, latest) + repository.git("checkout", "main") + repository.git("merge", "--squash", "integration") + repository.git("commit", "-m", "Squash latest promotion") + repository.commit( + ".github/promotion-recovered.txt", "42\n", "Acknowledge latest repaired batch" + ) + repository.git("push", "origin", "main") + repository.git("checkout", "integration") + repository.git("reset", "--hard", "main") + repository.git("push", "--force", "origin", "integration") + api.closed_main = [ + closed_promotion(latest), + closed_promotion(older, number=41, merged_at="2026-10-07T12:00:00Z"), + ] + before = repository.refs() + with pytest.raises(SystemExit) as exc: + promotion.run(REPOSITORY, NOW) + assert exc.value.code == 1 + assert repository.refs() == before + assert not api.created + assert not api.writes + output = capsys.readouterr().out + assert "::error::Promotion #41" in output + assert "needs manual recovery" in output + + def test_existing_snapshot_ref_is_never_advanced(repository, promotion, api): repository.integration() frozen = repository.commit("first.txt", "first", "First member") @@ -321,15 +609,21 @@ def test_promoted_integration_receives_version_bump_without_new_pr( repository.integration() snapshot = repository.commit("fix.txt", "fix", "Promoted fix") repository.git("push", "origin", "integration") + repository.publish_pull_head(42, snapshot) repository.git("checkout", "main") repository.git("merge", "--no-ff", "integration", "-m", "Merge promotion") bumped = repository.commit("version.txt", "1.0.1", "Update PolicyEngine US") repository.git("push", "origin", "main") - # API order is unrelated to merge order; the latest promotion's head is - # preserved in main even though an older API record is not an ancestor. + # Update order is unrelated to merge order; the latest promotion's head is + # preserved in main even though the more recently updated older one is not. unrelated = repository.commit("unused.txt", "unused", "Unmerged local commit") api.closed_main = [ - closed_promotion(unrelated, number=41, merged_at="2026-10-07T12:00:00Z"), + closed_promotion( + unrelated, + number=41, + merged_at="2026-10-07T12:00:00Z", + updated_at="2026-10-09T12:00:00Z", + ), closed_promotion(snapshot), ] promotion.run(REPOSITORY, NOW)