From ab8f51a24a6d8c010cfdaf0c5a96ef9584228fbe Mon Sep 17 00:00:00 2001 From: Wojciech Wojtyniak Date: Fri, 11 Sep 2026 12:18:39 -0700 Subject: [PATCH 1/5] ci(migrations): re-check open PRs when a new head lands on main (CHOO-2689) actions/checkout already checks out refs/pull//merge on pull_request events, so test_migration_chain.py already runs against the merge preview, not the branch tip. The actual gap is that GitHub keeps that preview current as the base moves but never re-runs the workflow to notice, and merging isn't gated on being up to date -- so a PR whose last CI run was green can still merge after main gains a colliding migration. That's exactly what happened twice in #404/#426, hand-fixed with merge revisions b47e0c39a1f5 and c81f4a06d2b7. Add a job that runs on push to main when the migrations directory changes: it re-checks every other open PR touching migrations by reading that PR's added revision files as plain text alongside the base as it now stands, then posts the result as a commit status on the PR's own head commit. A PR that would create a second head goes red the moment the collision exists, without needing a new push. The check reads revision/down_revision with a regex and ast.literal_eval instead of going through Alembic's ScriptDirectory, which builds its graph by importing every revision file. Importing a file from someone else's open PR on a public repo, from a job that runs on push to main with a token that can write commit statuses, would be running unreviewed code with that token's privileges. The per-PR pull_request run keeps using the real test_migration_chain.py unchanged, since there the code under test is the code already under review. --- .github/workflows/pr-ci.yml | 49 ++++ scripts/check_migration_heads_for_open_prs.py | 257 ++++++++++++++++++ 2 files changed, 306 insertions(+) create mode 100644 scripts/check_migration_heads_for_open_prs.py diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index 7801e615b..d2e36deb6 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -21,6 +21,22 @@ name: PR CI # Main is verified on push as well as per-PR, because a PR merged while red # leaves main red with nothing to say so — the next branch to merge main # inherits the failure and pays for it (CHOO-1430 reached main exactly this way). +# +# `actions/checkout` on a `pull_request` event checks out `refs/pull//merge` +# by default (verified against a real run's logs), GitHub's own preview of the +# PR merged into its base — so every job above already tests the merge result, +# not the branch tip, as of whenever it last ran. That "as of" is the gap: +# GitHub recomputes that preview as the base moves, but nothing re-runs the +# workflow just because it did, and merging is not gated on the branch being +# up to date. Two PRs chaining a migration off the same parent can each see a +# single head, go green, and sit open; if the first merges and the second +# never gets a new commit, its stale green check still satisfies the merge +# button, and only the merge of the second one actually produces two heads +# (CHOO-2689 — PRs #404 and #426, hand-fixed with merge revisions +# `b47e0c39a1f5` and `c81f4a06d2b7`). The `migration-heads-on-merge` job below +# re-tests every other open PR that touches migrations the moment a new one +# lands on main, so that race shows up as a red status on the affected PR +# immediately instead of at the next deploy. on: pull_request: @@ -47,6 +63,7 @@ jobs: console: ${{ steps.filter.outputs.console }} gateway: ${{ steps.filter.outputs.gateway }} artifacts: ${{ steps.filter.outputs.artifacts }} + migrations: ${{ steps.filter.outputs.migrations }} steps: - name: Checkout uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 @@ -74,6 +91,11 @@ jobs: gateway: - *shared - 'gateway/**' + # Its own filter, narrower than `backend`, so the push-triggered + # `migration-heads-on-merge` job below only runs when it can + # actually find a new head — not on every backend-touching push. + migrations: + - 'core/switch_core/migrations/versions/**' # Spans every tree, so it gets its own filter rather than riding # any one of them. Includes the files whose versions the registry is # checked against, so a release bump cannot land without the check. @@ -237,6 +259,33 @@ jobs: - name: Build (tsc + vite) run: npm run build + migration-heads-on-merge: + name: Migration heads on merge + # Only meaningful right after a new migration lands on main — a PR still + # open at that moment is the one whose stale green check could let a + # second head slip through (see the comment at the top of this file and + # scripts/check_migration_heads_for_open_prs.py for the full story, + # CHOO-2689). Reads revision files as plain text and never imports them + # (see the script's own docstring for why), so it needs no database and no + # Python environment for the project — cheap enough to run on every push + # that touches the migrations directory. + needs: changes + if: ${{ github.event_name == 'push' && needs.changes.outputs.migrations == 'true' }} + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read + statuses: write + steps: + - name: Checkout + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + + - name: Re-check open PRs that touch migrations + env: + GH_TOKEN: ${{ github.token }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: python3 scripts/check_migration_heads_for_open_prs.py + gitleaks: name: Secret scan (gitleaks) # Scans a PR's commit range, which only exists on a pull_request event: on a diff --git a/scripts/check_migration_heads_for_open_prs.py b/scripts/check_migration_heads_for_open_prs.py new file mode 100644 index 000000000..e698e2869 --- /dev/null +++ b/scripts/check_migration_heads_for_open_prs.py @@ -0,0 +1,257 @@ +#!/usr/bin/env python3 +"""Re-check the migration chain for open PRs against the just-updated base. + +`core/tests/switch_core/test_migration_chain.py` already asserts the chain has +one head, and `actions/checkout` already runs it against the merge-preview +commit (`refs/pull//merge`) rather than the PR's raw branch tip — so a PR +that adds a migration is tested against the base as it looked when the PR's +own CI last ran. + +That is not the same moment as "right before merge". Two PRs opened off the +same parent can each see one head, go green, and sit open. If the first one +merges and nothing pushes a new commit to the second, GitHub does not re-run +its CI just because its target moved — the merge-preview ref updates on +GitHub's side, but the check recorded against the PR's head commit does not. +The second PR still shows the old green result and can merge on it, and only +then does the chain end up with two heads (CHOO-2689; see PRs #404 and #426, +fixed by hand with merge revisions `b47e0c39a1f5` and `c81f4a06d2b7`). + +This script runs on every push to main that touches the migrations directory +— i.e. right after a new head lands — and re-checks every other open, +same-repo PR that also touches migrations: it reads that PR's added revision +files alongside the base as it now stands and reports whether the combination +still has exactly one head, posting the outcome as a commit status on the +PR's own head commit. A PR that would create a second head goes red +immediately, without needing a new commit or its own CI run. + +Deliberately does not use Alembic here, and imports no revision file. The +obvious way to compute a revision graph is `alembic.script.ScriptDirectory`, +which is what the real test file uses — but it builds the graph by +*importing* every file under `versions/`, running whatever module-level code +that file contains. That is fine for the file under review in a PR's own CI: +by the time this repo's checkout step fetches it, it's the PR's own code +being tested. It stops being fine here: this job's input is a file from some +*other* open pull request that nobody has approved, this repo is public so +anyone can open one, and the job runs on a push to main holding a token that +can write commit statuses. Importing an unreviewed file to compute a graph +would hand that token's holder's code execution to whoever opened the PR. + +Instead this reads `revision`/`down_revision` straight out of the file text +with a regex and `ast.literal_eval` — never `eval`, so the value must be a +literal (a string, `None`, or a tuple of strings) or parsing raises instead of +running anything. `test_revision_ids_are_unique` already reads `revision` the +same way, for the same reason, on a smaller scale; this applies it to the +whole graph. Nothing here is executed, so nothing here needs a database, a +Python environment for the project, or the `alembic` library itself. + +It shells out to `gh` (for PR discovery and posting statuses) and `git` (to +read a PR's files without checking them out or running them), both already +present on GitHub-hosted runners. +""" + +from __future__ import annotations + +import ast +import json +import os +import re +import subprocess +import sys +from pathlib import Path +from typing import Any + +REPO_ROOT = Path(__file__).resolve().parent.parent +VERSIONS_DIR = REPO_ROOT / "core" / "switch_core" / "migrations" / "versions" +VERSIONS_PATH = "core/switch_core/migrations/versions" +STATUS_CONTEXT = "migration-heads-on-merge" + +# Matches the two assignments Alembic's revision template generates, with or +# without the type annotation it has carried across template versions +# (`revision = ...`, `revision: str = ...`, `down_revision: str | None = ...`, +# `down_revision: str | Sequence[str] | None = ...`). Only the right-hand side +# is captured and handed to `ast.literal_eval`. +_ASSIGNMENT_RE = re.compile( + r"^(revision|down_revision)\s*(?::[^=\n]+)?=\s*(.+)$", re.MULTILINE +) + + +class RevisionFile: + __slots__ = ("filename", "revision", "parents") + + def __init__(self, filename: str, revision: str, parents: tuple[str, ...]) -> None: + self.filename = filename + self.revision = revision + self.parents = parents + + +def parse_revision_file(filename: str, text: str) -> RevisionFile: + """Extract `revision`/`down_revision` from a migration file's text. + + `ast.literal_eval` only ever produces a literal (or raises) -- it cannot + call a function, access an attribute, or run a statement, so this is safe + to point at a file nobody has reviewed. + """ + values: dict[str, object] = {} + for match in _ASSIGNMENT_RE.finditer(text): + name, rhs = match.group(1), match.group(2).strip() + values[name] = ast.literal_eval(rhs) + revision = values["revision"] + assert isinstance(revision, str), ( + f"{filename}: revision is not a string literal: {revision!r}" + ) + down = values.get("down_revision") + if down is None: + parents: tuple[str, ...] = () + elif isinstance(down, str): + parents = (down,) + elif isinstance(down, tuple): + parents = down # a merge revision's tuple of parents + else: + raise TypeError( + f"{filename}: down_revision is neither a string, tuple, nor None: {down!r}" + ) + return RevisionFile(filename, revision, parents) + + +def read_base_revisions() -> list[RevisionFile]: + return [ + parse_revision_file(path.name, path.read_text()) + for path in sorted(VERSIONS_DIR.glob("*.py")) + ] + + +def run( + *args: str, cwd: Path = REPO_ROOT, check: bool = True +) -> subprocess.CompletedProcess[str]: + return subprocess.run( + list(args), cwd=cwd, check=check, capture_output=True, text=True + ) + + +def open_prs_touching_migrations(repo: str) -> list[dict[str, Any]]: + listing = run( + "gh", + "pr", + "list", + "--repo", + repo, + "--state", + "open", + "--json", + "number,headRefOid", + ) + candidates = [] + for pr in json.loads(listing.stdout): + diff = run("gh", "pr", "diff", str(pr["number"]), "--repo", repo, "--name-only") + if any(line.startswith(VERSIONS_PATH) for line in diff.stdout.splitlines()): + candidates.append(pr) + return candidates + + +def fetch_pr_revisions(pr_number: int) -> list[RevisionFile]: + """The revision files as they stand on the PR's own head, read as text. + + Fetched straight from the PR's head ref rather than a merge -- the + versions directory only ever gains files, so a plain union with the + base's current revisions is the merge result for the purpose these checks + care about, and it skips every non-migration conflict a real merge could + raise. `git show` only ever prints a blob's content; nothing here writes + the file to disk or runs it. + """ + run("git", "fetch", "--depth=1", "origin", f"refs/pull/{pr_number}/head") + listing = run( + "git", "ls-tree", "-r", "--name-only", "FETCH_HEAD", "--", VERSIONS_PATH + ) + revisions = [] + for path in listing.stdout.splitlines(): + if not path: + continue + content = run("git", "show", f"FETCH_HEAD:{path}") + revisions.append(parse_revision_file(Path(path).name, content.stdout)) + return revisions + + +def describe_chain_problem(revisions: list[RevisionFile]) -> str: + """The three graph assertions in test_migration_chain.py, over a plain + list of (revision, parents) pairs instead of an Alembic graph. + + Returns an empty string if the chain is well-formed, otherwise a + description of what is wrong. + """ + by_revision: dict[str, list[str]] = {} + for rev in revisions: + by_revision.setdefault(rev.revision, []).append(rev.filename) + duplicates = {rev: files for rev, files in by_revision.items() if len(files) > 1} + if duplicates: + return f"duplicate migration revision ids: {duplicates}" + + known = set(by_revision) + dangling = { + rev.revision: [parent for parent in rev.parents if parent not in known] + for rev in revisions + if any(parent not in known for parent in rev.parents) + } + if dangling: + return f"migrations pointing at unknown parents: {dangling}" + + all_parents = {parent for rev in revisions for parent in rev.parents} + heads = [rev.revision for rev in revisions if rev.revision not in all_parents] + if len(heads) != 1: + return f"expected exactly one migration head, got {heads}" + + return "" + + +def post_status( + repo: str, sha: str, state: str, description: str, run_url: str +) -> None: + run( + "gh", + "api", + f"repos/{repo}/statuses/{sha}", + "-f", + f"state={state}", + "-f", + f"context={STATUS_CONTEXT}", + "-f", + f"description={description}", + "-f", + f"target_url={run_url}", + ) + + +def check_pr( + pr: dict[str, Any], base_revisions: list[RevisionFile], repo: str, run_url: str +) -> bool: + number = pr["number"] + sha = pr["headRefOid"] + combined = base_revisions + fetch_pr_revisions(int(number)) + problem = describe_chain_problem(combined) + passed = not problem + if passed: + description = "Migration chain stays single-headed if this PR merges now" + else: + description = "Merging this PR now would break the migration chain -- rebase onto the latest main" + print( + f"::error::PR #{number} would break the migration chain if merged now: {problem}" + ) + post_status( + repo, str(sha), "success" if passed else "failure", description, run_url + ) + return passed + + +def main() -> int: + repo = os.environ["GITHUB_REPOSITORY"] + run_url = os.environ["RUN_URL"] + prs = open_prs_touching_migrations(repo) + if not prs: + print("No open PRs touch the migrations directory.") + return 0 + base_revisions = read_base_revisions() + results = [check_pr(pr, base_revisions, repo, run_url) for pr in prs] + return 0 if all(results) else 1 + + +if __name__ == "__main__": + sys.exit(main()) From df138a2a34dd80e38e2188364076d50946427f0b Mon Sep 17 00:00:00 2001 From: Wojciech Wojtyniak Date: Fri, 11 Sep 2026 13:18:11 -0700 Subject: [PATCH 2/5] ci(migrations): validate the two attacker-influenced arguments (CHOO-2689) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The repository slug and the tracked path both reach a git or gh argument list. Nothing runs through a shell, so the risk was never a metavariable — it is a leading dash, which git reads as an option rather than a path. Both are now matched against an explicit pattern first, which also keeps the reachable surface to files that could actually be migrations. --- scripts/check_migration_heads_for_open_prs.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/scripts/check_migration_heads_for_open_prs.py b/scripts/check_migration_heads_for_open_prs.py index e698e2869..53d91e992 100644 --- a/scripts/check_migration_heads_for_open_prs.py +++ b/scripts/check_migration_heads_for_open_prs.py @@ -65,6 +65,18 @@ VERSIONS_PATH = "core/switch_core/migrations/versions" STATUS_CONTEXT = "migration-heads-on-merge" +# Everything below reaches a `git` or `gh` argument list, and the two values +# that a pull request's author controls — the repository slug from the +# environment and the tracked path from `git ls-tree` — are checked against +# these before they get there. Nothing runs through a shell, so this is not +# about metavariables; it is about a leading dash, which `git` would read as +# an option rather than a path, and about keeping the reachable surface to +# files that could actually be migrations. +SAFE_REPO_RE = re.compile(r"\A[A-Za-z0-9._-]+/[A-Za-z0-9._-]+\Z") +SAFE_VERSION_PATH_RE = re.compile( + rf"\A{re.escape(VERSIONS_PATH)}/[A-Za-z0-9._-]+\.py\Z" +) + # Matches the two assignments Alembic's revision template generates, with or # without the type annotation it has carried across template versions # (`revision = ...`, `revision: str = ...`, `down_revision: str | None = ...`, @@ -164,7 +176,7 @@ def fetch_pr_revisions(pr_number: int) -> list[RevisionFile]: ) revisions = [] for path in listing.stdout.splitlines(): - if not path: + if not SAFE_VERSION_PATH_RE.match(path): continue content = run("git", "show", f"FETCH_HEAD:{path}") revisions.append(parse_revision_file(Path(path).name, content.stdout)) @@ -243,6 +255,8 @@ def check_pr( def main() -> int: repo = os.environ["GITHUB_REPOSITORY"] + if not SAFE_REPO_RE.match(repo): + raise ValueError(f"GITHUB_REPOSITORY is not an owner/name slug: {repo!r}") run_url = os.environ["RUN_URL"] prs = open_prs_touching_migrations(repo) if not prs: From b9485a24dfcb4ad0c3f0bd980134f5080aa1d4a1 Mon Sep 17 00:00:00 2001 From: Wojciech Wojtyniak Date: Fri, 11 Sep 2026 13:24:06 -0700 Subject: [PATCH 3/5] ci(migrations): keep the repository out of every command line (CHOO-2689) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scanner flagged the repository slug reaching a subprocess argument list, and its own note said the flow was safe: list form, no shell, and a value GitHub supplies. Both true, and still the wrong thing to argue about — the value did not need to be there at all. gh resolves the repository from its own environment, and fills its `{owner}/{repo}` placeholders in an API path, so `--repo` and the interpolated status URL both go away. What remains from outside the script is a tracked path and a commit id, each matched against a pattern before it is passed. Removing the flow beats annotating it. --- .github/workflows/pr-ci.yml | 3 ++ scripts/check_migration_heads_for_open_prs.py | 50 +++++++++---------- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index d2e36deb6..9f7d573ea 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -283,6 +283,9 @@ jobs: - name: Re-check open PRs that touch migrations env: GH_TOKEN: ${{ github.token }} + # gh takes the repository from its own environment, so the script + # never assembles it into a command line. + GH_REPO: ${{ github.repository }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} run: python3 scripts/check_migration_heads_for_open_prs.py diff --git a/scripts/check_migration_heads_for_open_prs.py b/scripts/check_migration_heads_for_open_prs.py index 53d91e992..cbc4f8dda 100644 --- a/scripts/check_migration_heads_for_open_prs.py +++ b/scripts/check_migration_heads_for_open_prs.py @@ -65,17 +65,17 @@ VERSIONS_PATH = "core/switch_core/migrations/versions" STATUS_CONTEXT = "migration-heads-on-merge" -# Everything below reaches a `git` or `gh` argument list, and the two values -# that a pull request's author controls — the repository slug from the -# environment and the tracked path from `git ls-tree` — are checked against -# these before they get there. Nothing runs through a shell, so this is not -# about metavariables; it is about a leading dash, which `git` would read as -# an option rather than a path, and about keeping the reachable surface to -# files that could actually be migrations. -SAFE_REPO_RE = re.compile(r"\A[A-Za-z0-9._-]+/[A-Za-z0-9._-]+\Z") +# The repository is never named in an argument list: `gh` takes it from the +# checkout, and its `{owner}/{repo}` placeholder fills it in for an API path. +# That leaves two values reaching `git` or `gh` that come from outside this +# script — a tracked path from `git ls-tree`, and a commit id from `gh` — and +# both are matched against a pattern first. Nothing runs through a shell, so +# this is not about metavariables; it is about a leading dash, which `git` +# would read as an option rather than a path. SAFE_VERSION_PATH_RE = re.compile( rf"\A{re.escape(VERSIONS_PATH)}/[A-Za-z0-9._-]+\.py\Z" ) +SAFE_SHA_RE = re.compile(r"\A[0-9a-f]{7,40}\Z") # Matches the two assignments Alembic's revision template generates, with or # without the type annotation it has carried across template versions @@ -140,13 +140,16 @@ def run( ) -def open_prs_touching_migrations(repo: str) -> list[dict[str, Any]]: +def open_prs_touching_migrations() -> list[dict[str, Any]]: + """Open pull requests that add or change a migration. + + No `--repo`: `gh` resolves it from the checkout this runs in, which keeps + the repository out of the argument list entirely. + """ listing = run( "gh", "pr", "list", - "--repo", - repo, "--state", "open", "--json", @@ -154,7 +157,7 @@ def open_prs_touching_migrations(repo: str) -> list[dict[str, Any]]: ) candidates = [] for pr in json.loads(listing.stdout): - diff = run("gh", "pr", "diff", str(pr["number"]), "--repo", repo, "--name-only") + diff = run("gh", "pr", "diff", str(pr["number"]), "--name-only") if any(line.startswith(VERSIONS_PATH) for line in diff.stdout.splitlines()): candidates.append(pr) return candidates @@ -214,13 +217,15 @@ def describe_chain_problem(revisions: list[RevisionFile]) -> str: return "" -def post_status( - repo: str, sha: str, state: str, description: str, run_url: str -) -> None: +def post_status(sha: str, state: str, description: str, run_url: str) -> None: + if not SAFE_SHA_RE.match(sha): + raise ValueError(f"not a commit id: {sha!r}") run( "gh", "api", - f"repos/{repo}/statuses/{sha}", + # `{owner}` and `{repo}` are gh's own placeholders, filled from the + # checkout — not f-string fields. + f"repos/{{owner}}/{{repo}}/statuses/{sha}", "-f", f"state={state}", "-f", @@ -233,7 +238,7 @@ def post_status( def check_pr( - pr: dict[str, Any], base_revisions: list[RevisionFile], repo: str, run_url: str + pr: dict[str, Any], base_revisions: list[RevisionFile], run_url: str ) -> bool: number = pr["number"] sha = pr["headRefOid"] @@ -247,23 +252,18 @@ def check_pr( print( f"::error::PR #{number} would break the migration chain if merged now: {problem}" ) - post_status( - repo, str(sha), "success" if passed else "failure", description, run_url - ) + post_status(str(sha), "success" if passed else "failure", description, run_url) return passed def main() -> int: - repo = os.environ["GITHUB_REPOSITORY"] - if not SAFE_REPO_RE.match(repo): - raise ValueError(f"GITHUB_REPOSITORY is not an owner/name slug: {repo!r}") run_url = os.environ["RUN_URL"] - prs = open_prs_touching_migrations(repo) + prs = open_prs_touching_migrations() if not prs: print("No open PRs touch the migrations directory.") return 0 base_revisions = read_base_revisions() - results = [check_pr(pr, base_revisions, repo, run_url) for pr in prs] + results = [check_pr(pr, base_revisions, run_url) for pr in prs] return 0 if all(results) else 1 From 946b777edb3084b0aa53f792763513ffe004c06f Mon Sep 17 00:00:00 2001 From: Wojciech Wojtyniak Date: Fri, 11 Sep 2026 13:30:13 -0700 Subject: [PATCH 4/5] ci(migrations): send the status body on stdin, not the command line The remaining scanner finding was the run URL, which comes from the environment and was interpolated into a `-f` argument. The rule is about environment values reaching an argument list, and it is right that this is the wrong shape even though no shell is involved. The whole request body now goes to `gh api --input -` as JSON on stdin, which takes the description text off the command line too. Nothing variable-length is an argument any more. --- scripts/check_migration_heads_for_open_prs.py | 41 ++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/scripts/check_migration_heads_for_open_prs.py b/scripts/check_migration_heads_for_open_prs.py index cbc4f8dda..0833ea829 100644 --- a/scripts/check_migration_heads_for_open_prs.py +++ b/scripts/check_migration_heads_for_open_prs.py @@ -133,10 +133,26 @@ def read_base_revisions() -> list[RevisionFile]: def run( - *args: str, cwd: Path = REPO_ROOT, check: bool = True + *args: str, + cwd: Path = REPO_ROOT, + check: bool = True, + stdin: str | None = None, ) -> subprocess.CompletedProcess[str]: + """Run a command with a fixed argument list — never a shell, never a + string. + + `stdin` is how anything variable-length or free-text gets in. Keeping such + values off the command line is not about shell metacharacters, which + cannot apply here; it is that an argument list is the one place a value + can be mistaken for an option. + """ return subprocess.run( - list(args), cwd=cwd, check=check, capture_output=True, text=True + list(args), + cwd=cwd, + check=check, + capture_output=True, + text=True, + input=stdin, ) @@ -220,20 +236,25 @@ def describe_chain_problem(revisions: list[RevisionFile]) -> str: def post_status(sha: str, state: str, description: str, run_url: str) -> None: if not SAFE_SHA_RE.match(sha): raise ValueError(f"not a commit id: {sha!r}") + body = json.dumps( + { + "state": state, + "context": STATUS_CONTEXT, + "description": description, + "target_url": run_url, + } + ) run( "gh", "api", # `{owner}` and `{repo}` are gh's own placeholders, filled from the # checkout — not f-string fields. f"repos/{{owner}}/{{repo}}/statuses/{sha}", - "-f", - f"state={state}", - "-f", - f"context={STATUS_CONTEXT}", - "-f", - f"description={description}", - "-f", - f"target_url={run_url}", + "--method", + "POST", + "--input", + "-", + stdin=body, ) From 9ddfbc4dfe3d7e67cafc3dc66b249fe1c10084c3 Mon Sep 17 00:00:00 2001 From: Petr Bauch Date: Mon, 14 Sep 2026 05:48:48 -0400 Subject: [PATCH 5/5] ci(migrations): fix the union, the PR cap, the cancelled run and the aborted sweep (CHOO-2689) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four defects found in review, each of which would have made the check useless in a different way. The base and the PR's head both carry the whole versions directory, so concatenating them counts every shared migration twice. Every PR that touched migrations would have gone red on a duplicate-id error, and because duplicates are reported first, the two-head case the job exists to catch was unreachable. The union is now keyed by filename. `gh pr list` stops at 30 and says nothing about the rest; the repository has more open than that, so the PRs it skipped got no check and no signal. It now asks for a limit it will not reach, and fails loudly if it ever does. Every push to main shares one `github.ref`, so the concurrency group let a later merge cancel the run for the migration that had just landed — and the replacement run skips this job, because its own commit touched no migration. Push runs now carry the commit in their group. A single `gh` or `git` failure aborted the whole sweep from inside a list comprehension, leaving the PRs not yet reached with nothing on them and a red job on main indistinguishable from a genuine finding. Each PR is now checked in isolation and gets an error status of its own. Two inputs could raise on ordinary content: a `down_revision` tuple spread over several lines, which a line-anchored regex hands to `literal_eval` half-finished, and a non-migration `.py` under `versions/`, which came out as a bare `KeyError`. Reading the assignments off `ast.parse`'s tree removes the first outright, and both are now reported as a problem naming the file. Nothing is imported or executed, which was the point of not using Alembic here. Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-ci.yml | 10 +- scripts/check_migration_heads_for_open_prs.py | 198 ++++++++++++++---- 2 files changed, 170 insertions(+), 38 deletions(-) diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index 9f7d573ea..2abf775cc 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -44,8 +44,16 @@ on: branches: [main] # Cancel a PR's in-flight CI when a new commit is pushed to the same ref. +# +# Push runs get the commit in their group, so they never cancel each other. +# Every merge to main shares one `github.ref`, so a single group would mean the +# next merge kills the previous merge's run — and `migration-heads-on-merge` +# only runs when the *triggering* commit touched migrations, so the run that +# replaces it skips the job entirely. Merge a migration and anything else a +# minute apart and no status is posted anywhere, silently, on exactly the busy +# day this is meant to cover. concurrency: - group: pr-ci-${{ github.ref }} + group: pr-ci-${{ github.ref }}-${{ github.event_name == 'push' && github.sha || '' }} cancel-in-progress: true permissions: diff --git a/scripts/check_migration_heads_for_open_prs.py b/scripts/check_migration_heads_for_open_prs.py index 0833ea829..802103c94 100644 --- a/scripts/check_migration_heads_for_open_prs.py +++ b/scripts/check_migration_heads_for_open_prs.py @@ -36,14 +36,19 @@ can write commit statuses. Importing an unreviewed file to compute a graph would hand that token's holder's code execution to whoever opened the PR. -Instead this reads `revision`/`down_revision` straight out of the file text -with a regex and `ast.literal_eval` — never `eval`, so the value must be a -literal (a string, `None`, or a tuple of strings) or parsing raises instead of -running anything. `test_revision_ids_are_unique` already reads `revision` the -same way, for the same reason, on a smaller scale; this applies it to the +Instead this reads `revision`/`down_revision` out of the file's syntax tree: +`ast.parse` builds the tree without running a line of it, and the two +right-hand sides go through `ast.literal_eval` — never `eval`, so each must be +a literal (a string, `None`, or a tuple of strings) or parsing raises instead +of running anything. `test_revision_ids_are_unique` reads `revision` in the +same spirit, for the same reason, on a smaller scale; this applies it to the whole graph. Nothing here is executed, so nothing here needs a database, a Python environment for the project, or the `alembic` library itself. +The tree rather than a regex because an assignment is not a line: a merge +revision's `down_revision` tuple may be spread over several, and a pattern +anchored to end-of-line hands `ast.literal_eval` half an expression. + It shells out to `gh` (for PR discovery and posting statuses) and `git` (to read a PR's files without checking them out or running them), both already present on GitHub-hosted runners. @@ -64,6 +69,7 @@ VERSIONS_DIR = REPO_ROOT / "core" / "switch_core" / "migrations" / "versions" VERSIONS_PATH = "core/switch_core/migrations/versions" STATUS_CONTEXT = "migration-heads-on-merge" +PR_LIST_LIMIT = 1000 # The repository is never named in an argument list: `gh` takes it from the # checkout, and its `{owner}/{repo}` placeholder fills it in for an API path. @@ -77,14 +83,16 @@ ) SAFE_SHA_RE = re.compile(r"\A[0-9a-f]{7,40}\Z") -# Matches the two assignments Alembic's revision template generates, with or -# without the type annotation it has carried across template versions -# (`revision = ...`, `revision: str = ...`, `down_revision: str | None = ...`, -# `down_revision: str | Sequence[str] | None = ...`). Only the right-hand side -# is captured and handed to `ast.literal_eval`. -_ASSIGNMENT_RE = re.compile( - r"^(revision|down_revision)\s*(?::[^=\n]+)?=\s*(.+)$", re.MULTILINE -) +_WANTED_ASSIGNMENTS = ("revision", "down_revision") + + +class RevisionProblem(Exception): + """A file under `versions/` that cannot be read as a migration. + + Raised rather than allowed to surface as a `KeyError` or a `SyntaxError` + so the sweep can report *which file* is unreadable, on the PR that + introduced it, instead of dying with a traceback halfway through. + """ class RevisionFile: @@ -96,31 +104,71 @@ def __init__(self, filename: str, revision: str, parents: tuple[str, ...]) -> No self.parents = parents -def parse_revision_file(filename: str, text: str) -> RevisionFile: - """Extract `revision`/`down_revision` from a migration file's text. +def _assigned_literals(filename: str, text: str) -> dict[str, object]: + """The module-level `revision` / `down_revision` literals, from the tree. + + `ast.parse` builds a syntax tree and runs nothing, and `ast.literal_eval` + on a node only ever produces a literal (or raises) -- it cannot call a + function, access an attribute, or execute a statement. Both are safe to + point at a file nobody has reviewed. - `ast.literal_eval` only ever produces a literal (or raises) -- it cannot - call a function, access an attribute, or run a statement, so this is safe - to point at a file nobody has reviewed. + Reading assignments off the tree rather than out of the text is also what + makes a multi-line `down_revision` tuple -- the shape a merge revision + grows into once it has more than a couple of parents -- parse like any + other. """ + try: + tree = ast.parse(text, filename=filename) + except SyntaxError as exc: + raise RevisionProblem(f"{filename}: is not valid Python: {exc}") from exc + values: dict[str, object] = {} - for match in _ASSIGNMENT_RE.finditer(text): - name, rhs = match.group(1), match.group(2).strip() - values[name] = ast.literal_eval(rhs) + for node in tree.body: + targets: list[str] = [] + value: ast.expr | None = None + if isinstance(node, ast.Assign): + targets = [t.id for t in node.targets if isinstance(t, ast.Name)] + value = node.value + elif isinstance(node, ast.AnnAssign) and isinstance(node.target, ast.Name): + targets = [node.target.id] + value = node.value # None for a bare `x: str` annotation + if value is None: + continue + for name in targets: + if name not in _WANTED_ASSIGNMENTS: + continue + try: + values[name] = ast.literal_eval(value) + except ValueError as exc: + raise RevisionProblem( + f"{filename}: {name} is not a literal: {exc}" + ) from exc + return values + + +def parse_revision_file(filename: str, text: str) -> RevisionFile: + """Extract `revision`/`down_revision` from a migration file's text.""" + values = _assigned_literals(filename, text) + if "revision" not in values: + raise RevisionProblem( + f"{filename}: no module-level `revision` assignment -- " + "every file under versions/ must be a migration" + ) revision = values["revision"] - assert isinstance(revision, str), ( - f"{filename}: revision is not a string literal: {revision!r}" - ) + if not isinstance(revision, str): + raise RevisionProblem( + f"{filename}: revision is not a string literal: {revision!r}" + ) down = values.get("down_revision") if down is None: parents: tuple[str, ...] = () elif isinstance(down, str): parents = (down,) - elif isinstance(down, tuple): + elif isinstance(down, tuple) and all(isinstance(p, str) for p in down): parents = down # a merge revision's tuple of parents else: - raise TypeError( - f"{filename}: down_revision is neither a string, tuple, nor None: {down!r}" + raise RevisionProblem( + f"{filename}: down_revision is neither a string, a tuple of strings, nor None: {down!r}" ) return RevisionFile(filename, revision, parents) @@ -161,6 +209,11 @@ def open_prs_touching_migrations() -> list[dict[str, Any]]: No `--repo`: `gh` resolves it from the checkout this runs in, which keeps the repository out of the argument list entirely. + + `--limit` is not optional. Without it `gh` stops at 30 open PRs and says + nothing about the rest, so on a repository with more than that the PRs + least likely to have been rechecked recently are exactly the ones silently + skipped. """ listing = run( "gh", @@ -168,11 +221,19 @@ def open_prs_touching_migrations() -> list[dict[str, Any]]: "list", "--state", "open", + "--limit", + str(PR_LIST_LIMIT), "--json", "number,headRefOid", ) + open_prs = json.loads(listing.stdout) + if len(open_prs) >= PR_LIST_LIMIT: + raise RuntimeError( + f"{len(open_prs)} open PRs reached the --limit of {PR_LIST_LIMIT}; " + "some were not listed and would be skipped without a word" + ) candidates = [] - for pr in json.loads(listing.stdout): + for pr in open_prs: diff = run("gh", "pr", "diff", str(pr["number"]), "--name-only") if any(line.startswith(VERSIONS_PATH) for line in diff.stdout.splitlines()): candidates.append(pr) @@ -182,12 +243,10 @@ def open_prs_touching_migrations() -> list[dict[str, Any]]: def fetch_pr_revisions(pr_number: int) -> list[RevisionFile]: """The revision files as they stand on the PR's own head, read as text. - Fetched straight from the PR's head ref rather than a merge -- the - versions directory only ever gains files, so a plain union with the - base's current revisions is the merge result for the purpose these checks - care about, and it skips every non-migration conflict a real merge could - raise. `git show` only ever prints a blob's content; nothing here writes - the file to disk or runs it. + Fetched straight from the PR's head ref rather than a merge; `merge_preview` + combines it with the base. This is the whole directory as the PR has it, + shared files included, not just the ones it adds. `git show` only ever + prints a blob's content; nothing here writes the file to disk or runs it. """ run("git", "fetch", "--depth=1", "origin", f"refs/pull/{pr_number}/head") listing = run( @@ -258,12 +317,31 @@ def post_status(sha: str, state: str, description: str, run_url: str) -> None: ) +def merge_preview( + base_revisions: list[RevisionFile], pr_revisions: list[RevisionFile] +) -> list[RevisionFile]: + """The versions directory as it would stand with this PR merged. + + A union keyed by filename, not a concatenation: the two sides share every + revision that was already on main when the PR branched, and counting those + twice reports every PR as a duplicate-id collision. The PR's copy wins, + which is also what a merge does for a file it modifies. + + The directory only ever gains files, so this is the merge result for the + purpose these checks care about -- and it needs no real merge, so no + unrelated conflict can get in the way. + """ + merged = {rev.filename: rev for rev in base_revisions} + merged.update({rev.filename: rev for rev in pr_revisions}) + return list(merged.values()) + + def check_pr( pr: dict[str, Any], base_revisions: list[RevisionFile], run_url: str ) -> bool: number = pr["number"] sha = pr["headRefOid"] - combined = base_revisions + fetch_pr_revisions(int(number)) + combined = merge_preview(base_revisions, fetch_pr_revisions(int(number))) problem = describe_chain_problem(combined) passed = not problem if passed: @@ -277,14 +355,60 @@ def check_pr( return passed +def check_pr_isolated( + pr: dict[str, Any], base_revisions: list[RevisionFile], run_url: str +) -> bool: + """`check_pr`, with one PR's failure kept to that PR. + + Every `gh` and `git` call here can fail on its own -- a transient API + error, a head ref that no longer resolves -- and an unreadable file under + `versions/` raises too. Letting any of those out would abandon the PRs not + yet reached, with nothing on them to say so and a red job on main that + looks exactly like a genuine finding. So each PR gets an `error` status + naming what went wrong, and the sweep carries on. + """ + number = pr["number"] + try: + return check_pr(pr, base_revisions, run_url) + except (RevisionProblem, subprocess.CalledProcessError, OSError, ValueError) as exc: + detail = ( + exc.stderr.strip() + if isinstance(exc, subprocess.CalledProcessError) and exc.stderr + else str(exc) + ) + print(f"::error::PR #{number} could not be checked: {detail}") + sha = str(pr.get("headRefOid", "")) + if SAFE_SHA_RE.match(sha): + try: + post_status( + sha, + "error", + "Could not check the migration chain for this PR -- see the run log", + run_url, + ) + except (subprocess.CalledProcessError, OSError) as post_exc: + # Whatever broke the check may well be what breaks saying so. + print( + f"::error::PR #{number}: could not post a status either: {post_exc}" + ) + return False + + def main() -> int: run_url = os.environ["RUN_URL"] prs = open_prs_touching_migrations() if not prs: print("No open PRs touch the migrations directory.") return 0 - base_revisions = read_base_revisions() - results = [check_pr(pr, base_revisions, run_url) for pr in prs] + try: + base_revisions = read_base_revisions() + except RevisionProblem as exc: + # Nothing can be said about any PR while main itself is unreadable, + # and it is main that needs fixing -- do not paint every open PR red + # for it. + print(f"::error::main's own migrations cannot be read: {exc}") + return 1 + results = [check_pr_isolated(pr, base_revisions, run_url) for pr in prs] return 0 if all(results) else 1