From 68433bad14cde57a8cf1ab0ccae0aab29b6be1fe Mon Sep 17 00:00:00 2001 From: Harry Lee Date: Sun, 13 Sep 2026 16:26:21 -0700 Subject: [PATCH 1/2] fix(ci): compare rebased copies against their PR base Signed-off-by: Harry Lee --- .github/codeowners/test_aisimulate_policy.py | 8 +- .github/workflows/ci.yml | 17 +- .github/workflows/fast-ci.yml | 10 +- tests/test_ci_workflow_contracts.py | 175 +++++++++++++++++++ 4 files changed, 205 insertions(+), 5 deletions(-) diff --git a/.github/codeowners/test_aisimulate_policy.py b/.github/codeowners/test_aisimulate_policy.py index 800a543bf..c4dafe981 100644 --- a/.github/codeowners/test_aisimulate_policy.py +++ b/.github/codeowners/test_aisimulate_policy.py @@ -456,19 +456,23 @@ def test_full_ci_trusted_copy_verification(tmp_path: Path) -> None: "set -euo pipefail\n" "exit_code=${FAKE_GH_EXIT:-0}\n" 'if [[ ${exit_code} != 0 ]]; then exit "${exit_code}"; fi\n' - "printf '%s\\n' \"${FAKE_PR_HEAD:-}\"\n" + "printf '%s\\t%s\\n' \"${FAKE_PR_HEAD:-}\" \"${FAKE_PR_BASE:-}\"\n" ) fake_gh.chmod(0o755) - target_sha = "0123456789abcdef" + target_sha = "a" * 40 + base_sha = "b" * 40 matching = { "GITHUB_REF": "refs/heads/pull-request/135", "REPOSITORY": "ai-dynamo/aisimulate", "RUN_SHA": target_sha, "FAKE_PR_HEAD": target_sha, + "FAKE_PR_BASE": base_sha, + "GITHUB_OUTPUT": str(tmp_path / "output.txt"), "PATH": f"{fake_bin}:{os.environ['PATH']}", } assert _run_readiness_script(copy_script, tmp_path, matching).returncode == 0 + assert (tmp_path / "output.txt").read_text() == f"base_sha={base_sha}\n" mismatched = {**matching, "FAKE_PR_HEAD": "fedcba9876543210"} assert _run_readiness_script(copy_script, tmp_path, mismatched).returncode != 0 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fc70ccd9a..ed1d2570a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,6 +31,7 @@ jobs: runs-on: ubuntu-latest outputs: comparison-base: ${{ steps.comparison-base.outputs.sha }} + whitespace-base: ${{ steps.pr-target.outputs.base_sha || steps.comparison-base.outputs.sha }} env: EXPECTED_SHA: ${{ inputs.expected_sha }} RUN_SHA: ${{ github.sha }} @@ -47,6 +48,7 @@ jobs: exit 1 fi - name: Verify trusted PR copy matches originating head + id: pr-target if: startsWith(github.ref, 'refs/heads/pull-request/') shell: bash env: @@ -59,13 +61,23 @@ jobs: echo "::error::invalid trusted PR copy ref: ${GITHUB_REF}" exit 1 fi - pr_head="$( - gh api -X GET "repos/${REPOSITORY}/pulls/${pr_number}" --jq '.head.sha' + pr_metadata="$( + gh api -X GET "repos/${REPOSITORY}/pulls/${pr_number}" \ + --jq '[.head.sha, .base.sha] | @tsv' )" + IFS=$'\t' read -r pr_head pr_base <<< "${pr_metadata}" if [[ "${pr_head}" != "${RUN_SHA}" ]]; then echo "::error::trusted copy ${RUN_SHA} does not match PR #${pr_number} head ${pr_head}" exit 1 fi + if [[ ! "${pr_base}" =~ ^[0-9a-f]{40}$ ]]; then + echo "::error::PR comparison base must resolve to a full commit SHA" + exit 1 + fi + # A copied branch's push.before is its previous head, which may no + # longer be reachable after a rebase. Check the actual PR range, + # including the parent branch when this PR is stacked. + printf 'base_sha=%s\n' "${pr_base}" >> "${GITHUB_OUTPUT}" - name: Resolve immutable comparison base id: comparison-base shell: bash @@ -194,6 +206,7 @@ jobs: uses: ./.github/workflows/fast-ci.yml with: expected_sha: ${{ github.sha }} + base_sha: ${{ needs.verify-target.outputs.whitespace-base }} platform-wheels: name: Platform Wheels diff --git a/.github/workflows/fast-ci.yml b/.github/workflows/fast-ci.yml index e79277cb5..060cd4b7a 100644 --- a/.github/workflows/fast-ci.yml +++ b/.github/workflows/fast-ci.yml @@ -10,6 +10,11 @@ on: required: false type: string default: "" + base_sha: + description: Immutable comparison base resolved by the caller + required: false + type: string + default: "" permissions: contents: read @@ -88,7 +93,7 @@ jobs: timeout-minutes: 10 env: TARGET_SHA: ${{ inputs.expected_sha || github.event.pull_request.head.sha || github.sha }} - BASE_SHA: ${{ github.event.pull_request.base.sha || github.event.before }} + BASE_SHA: ${{ inputs.base_sha || github.event.pull_request.base.sha || github.event.before }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: @@ -127,6 +132,9 @@ jobs: shell: bash run: | if [[ -n "${BASE_SHA}" && ! "${BASE_SHA}" =~ ^0+$ ]]; then + if ! git cat-file -e "${BASE_SHA}^{commit}" 2>/dev/null; then + git fetch --no-tags origin "${BASE_SHA}" + fi git diff --check "${BASE_SHA}...${TARGET_SHA}" else git diff --check "${TARGET_SHA}^" "${TARGET_SHA}" diff --git a/tests/test_ci_workflow_contracts.py b/tests/test_ci_workflow_contracts.py index 1b139f175..45a16ff6a 100644 --- a/tests/test_ci_workflow_contracts.py +++ b/tests/test_ci_workflow_contracts.py @@ -463,6 +463,181 @@ def _run_comparison_base( return result, output +def test_fast_ci_whitespace_receives_the_verified_pr_base() -> None: + jobs = _workflow("ci.yml")["jobs"] + assert jobs["verify-target"]["outputs"]["whitespace-base"] == ( + "${{ steps.pr-target.outputs.base_sha || steps.comparison-base.outputs.sha }}" + ) + assert jobs["fast-ci"]["with"]["base_sha"] == "${{ needs.verify-target.outputs.whitespace-base }}" + fast_ci = _workflow("fast-ci.yml") + base_input = fast_ci["on"]["workflow_call"]["inputs"]["base_sha"] + assert base_input["required"] == "false" + assert base_input["type"] == "string" + assert fast_ci["jobs"]["python-static"]["env"]["BASE_SHA"] == ( + "${{ inputs.base_sha || github.event.pull_request.base.sha || github.event.before }}" + ) + + +def _run_pr_target( + tmp_path: Path, head: str, base: str, api_status: int = 0 +) -> tuple[subprocess.CompletedProcess[str], dict[str, str]]: + step = next(step for step in _workflow("ci.yml")["jobs"]["verify-target"]["steps"] if step.get("id") == "pr-target") + gh = tmp_path / "gh" + gh.write_text( + '#!/bin/bash\nprintf "%s\\n" "$*" >> "${GH_CALLED}"\n' + 'if [[ "${TEST_API_STATUS}" != 0 ]]; then exit "${TEST_API_STATUS}"; fi\n' + 'printf "%s\\t%s\\n" "${TEST_PR_HEAD}" "${TEST_PR_BASE}"\n' + ) + gh.chmod(0o755) + output_path = tmp_path / "pr-output" + calls_path = tmp_path / "gh-called" + result = _run_workflow_script( + step["run"], + { + "PATH": f"{tmp_path}{os.pathsep}{os.environ['PATH']}", + "GITHUB_EVENT_NAME": "push", + "GITHUB_REF": "refs/heads/pull-request/121", + "RUN_SHA": "a" * 40, + "REPOSITORY": "ai-dynamo/aisimulate", + "GITHUB_OUTPUT": str(output_path), + "GH_CALLED": str(calls_path), + "TEST_PR_HEAD": head, + "TEST_PR_BASE": base, + "TEST_API_STATUS": str(api_status), + }, + ) + assert calls_path.read_text().splitlines() == [ + "api -X GET repos/ai-dynamo/aisimulate/pulls/121 --jq [.head.sha, .base.sha] | @tsv" + ] + output = dict(line.split("=", 1) for line in output_path.read_text().splitlines()) if output_path.exists() else {} + return result, output + + +def test_full_ci_trusted_copy_exports_the_stacked_pr_base(tmp_path: Path) -> None: + result, output = _run_pr_target(tmp_path, "a" * 40, "d" * 40) + assert result.returncode == 0, result.stderr + assert output == {"base_sha": "d" * 40} + + +@pytest.mark.parametrize( + ("head", "base", "api_status"), + [ + pytest.param("b" * 40, "d" * 40, 0, id="head-changed"), + pytest.param("a" * 40, "main", 0, id="mutable-base"), + pytest.param("a" * 40, "d" * 39, 0, id="short-base"), + pytest.param("a" * 40, "", 0, id="missing-base"), + pytest.param("a" * 40, "d" * 40, 1, id="api-failure"), + ], +) +def test_full_ci_trusted_copy_base_fails_closed(tmp_path: Path, head: str, base: str, api_status: int) -> None: + result, output = _run_pr_target(tmp_path, head, base, api_status) + assert result.returncode != 0 + assert output == {} + + +def _git(repository: Path, *arguments: str, check: bool = True) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [ + "git", + "-c", + "user.name=CI contract test", + "-c", + "user.email=ci-test@example.invalid", + "-c", + "commit.gpgsign=false", + "-c", + "core.hooksPath=/dev/null", + "-C", + str(repository), + *arguments, + ], + env={**os.environ, "GIT_CONFIG_NOSYSTEM": "1", "GIT_CONFIG_GLOBAL": os.devnull}, + capture_output=True, + text=True, + check=check, + ) + + +def _commit_file(repository: Path, name: str, contents: str) -> str: + (repository / name).write_text(contents) + _git(repository, "add", name) + _git(repository, "commit", "--quiet", "-m", name) + return _git(repository, "rev-parse", "HEAD").stdout.strip() + + +def _run_whitespace_step(repository: Path, base: str, target: str) -> subprocess.CompletedProcess[str]: + job = _workflow("fast-ci.yml")["jobs"]["python-static"] + script = next(step["run"] for step in job["steps"] if step.get("name") == "Check changed-line whitespace") + return subprocess.run( + ["bash", "-c", script], + cwd=repository, + env={ + **os.environ, + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_CONFIG_GLOBAL": os.devnull, + "GITHUB_EVENT_NAME": "push", + "GITHUB_REF": "refs/heads/pull-request/121", + "BEFORE_SHA": "f" * 40, + "BASE_SHA": base, + "TARGET_SHA": target, + }, + capture_output=True, + text=True, + check=False, + ) + + +@pytest.mark.parametrize("earlier_whitespace", [False, True], ids=["clean-rebased-copy", "earlier-pr-commit"]) +def test_fast_ci_whitespace_checks_the_whole_pr_above_its_stacked_base( + tmp_path: Path, earlier_whitespace: bool +) -> None: + _git(tmp_path, "init", "--quiet") + _commit_file(tmp_path, "root.txt", "root\n") + base = _commit_file(tmp_path, "parent-pr.txt", "outside this PR \n") + _commit_file(tmp_path, "first-pr.txt", "first change \n" if earlier_whitespace else "first change\n") + target = _commit_file(tmp_path, "last-pr.txt", "last change\n") + assert _git(tmp_path, "cat-file", "-e", f"{'f' * 40}^{{commit}}", check=False).returncode != 0 + assert _git(tmp_path, "diff", "--check", f"{target}^", target).returncode == 0 + + result = _run_whitespace_step(tmp_path, base, target) + + assert (result.returncode != 0) is earlier_whitespace, result.stdout + result.stderr + assert "parent-pr.txt" not in result.stdout + if earlier_whitespace: + assert "first-pr.txt" in result.stdout + assert "trailing whitespace" in result.stdout + + +@pytest.mark.parametrize("available", [True, False], ids=["fetch-base-history", "missing-base-fails-closed"]) +def test_fast_ci_whitespace_fetches_an_absent_base_without_truncating_history(tmp_path: Path, available: bool) -> None: + origin = tmp_path / "origin" + checkout = tmp_path / "checkout" + origin.mkdir() + checkout.mkdir() + _git(origin, "init", "--quiet") + common = _commit_file(origin, "root.txt", "root\n") + target = _commit_file(origin, "pr.txt", "PR change\n") + _git(origin, "checkout", "--quiet", "-b", "base", common) + _commit_file(origin, "base-first.txt", "first base change\n") + base = _commit_file(origin, "base-last.txt", "last base change\n") + _git(origin, "tag", "not-needed-for-whitespace", base) + _git(checkout, "init", "--quiet") + _git(checkout, "remote", "add", "origin", origin.as_uri()) + _git(checkout, "fetch", "--no-tags", "origin", target) + _git(checkout, "checkout", "--quiet", "--detach", target) + assert _git(checkout, "cat-file", "-e", f"{base}^{{commit}}", check=False).returncode != 0 + + result = _run_whitespace_step(checkout, base if available else "f" * 40, target) + + if available: + assert result.returncode == 0, result.stdout + result.stderr + assert _git(checkout, "merge-base", base, target).stdout.strip() == common + assert _git(checkout, "rev-parse", "--is-shallow-repository").stdout.strip() == "false" + assert not _git(checkout, "tag", "--list").stdout.strip() + else: + assert result.returncode != 0 + + def test_collector_comparison_fetch_preserves_full_history() -> None: commands = _run_commands(_workflow("collector-check.yml")["jobs"]["check"]) From c08ebd5b8a338f8d6ed85b4117b213c092d3a642 Mon Sep 17 00:00:00 2001 From: Harry Lee Date: Sun, 13 Sep 2026 18:36:43 -0700 Subject: [PATCH 2/2] fix(ci): authenticate missing-base fetches Signed-off-by: Harry Lee --- .github/workflows/ci.yml | 2 +- .github/workflows/fast-ci.yml | 8 +++++- tests/test_ci_workflow_contracts.py | 40 +++++++++++++++++++++++++---- 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ed1d2570a..7c71077ea 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,7 +31,7 @@ jobs: runs-on: ubuntu-latest outputs: comparison-base: ${{ steps.comparison-base.outputs.sha }} - whitespace-base: ${{ steps.pr-target.outputs.base_sha || steps.comparison-base.outputs.sha }} + whitespace-base: ${{ steps.pr-target.outputs.base_sha }} env: EXPECTED_SHA: ${{ inputs.expected_sha }} RUN_SHA: ${{ github.sha }} diff --git a/.github/workflows/fast-ci.yml b/.github/workflows/fast-ci.yml index 060cd4b7a..f58e67b5e 100644 --- a/.github/workflows/fast-ci.yml +++ b/.github/workflows/fast-ci.yml @@ -130,10 +130,16 @@ jobs: tests - name: Check changed-line whitespace shell: bash + env: + GH_TOKEN: ${{ github.token }} run: | if [[ -n "${BASE_SHA}" && ! "${BASE_SHA}" =~ ^0+$ ]]; then if ! git cat-file -e "${BASE_SHA}^{commit}" 2>/dev/null; then - git fetch --no-tags origin "${BASE_SHA}" + # Checkout removes its credentials; authenticate this fetch only, + # as in collector-check.yml, without persisting the token. + basic_token="$(printf 'x-access-token:%s' "${GH_TOKEN}" | base64 | tr -d '\n')" + git -c "http.https://github.com/.extraheader=AUTHORIZATION: basic ${basic_token}" fetch \ + --no-tags origin "${BASE_SHA}" fi git diff --check "${BASE_SHA}...${TARGET_SHA}" else diff --git a/tests/test_ci_workflow_contracts.py b/tests/test_ci_workflow_contracts.py index 45a16ff6a..4cd93342d 100644 --- a/tests/test_ci_workflow_contracts.py +++ b/tests/test_ci_workflow_contracts.py @@ -465,9 +465,7 @@ def _run_comparison_base( def test_fast_ci_whitespace_receives_the_verified_pr_base() -> None: jobs = _workflow("ci.yml")["jobs"] - assert jobs["verify-target"]["outputs"]["whitespace-base"] == ( - "${{ steps.pr-target.outputs.base_sha || steps.comparison-base.outputs.sha }}" - ) + assert jobs["verify-target"]["outputs"]["whitespace-base"] == "${{ steps.pr-target.outputs.base_sha }}" assert jobs["fast-ci"]["with"]["base_sha"] == "${{ needs.verify-target.outputs.whitespace-base }}" fast_ci = _workflow("fast-ci.yml") base_input = fast_ci["on"]["workflow_call"]["inputs"]["base_sha"] @@ -478,6 +476,22 @@ def test_fast_ci_whitespace_receives_the_verified_pr_base() -> None: ) +def test_fast_ci_missing_base_fetch_uses_temporary_checkout_authentication() -> None: + workflow = _workflow("fast-ci.yml") + job = workflow["jobs"]["python-static"] + checkout = next(step for step in job["steps"] if step.get("uses", "").startswith("actions/checkout@")) + whitespace = next(step for step in job["steps"] if step.get("name") == "Check changed-line whitespace") + + assert workflow["permissions"]["contents"] == "read" + assert checkout["with"]["persist-credentials"] == "false" + assert whitespace["env"]["GH_TOKEN"] == "${{ github.token }}" + assert "printf 'x-access-token:%s' \"${GH_TOKEN}\"" in whitespace["run"] + assert ( + 'git -c "http.https://github.com/.extraheader=AUTHORIZATION: basic ${basic_token}" fetch' in whitespace["run"] + ) + assert '--no-tags origin "${BASE_SHA}"' in whitespace["run"] + + def _run_pr_target( tmp_path: Path, head: str, base: str, api_status: int = 0 ) -> tuple[subprocess.CompletedProcess[str], dict[str, str]]: @@ -569,7 +583,7 @@ def _run_whitespace_step(repository: Path, base: str, target: str) -> subprocess job = _workflow("fast-ci.yml")["jobs"]["python-static"] script = next(step["run"] for step in job["steps"] if step.get("name") == "Check changed-line whitespace") return subprocess.run( - ["bash", "-c", script], + ["bash", "--noprofile", "--norc", "-eo", "pipefail", "-c", script], cwd=repository, env={ **os.environ, @@ -580,6 +594,7 @@ def _run_whitespace_step(repository: Path, base: str, target: str) -> subprocess "BEFORE_SHA": "f" * 40, "BASE_SHA": base, "TARGET_SHA": target, + "GH_TOKEN": "ci-contract-dummy-token", }, capture_output=True, text=True, @@ -608,6 +623,18 @@ def test_fast_ci_whitespace_checks_the_whole_pr_above_its_stacked_base( assert "trailing whitespace" in result.stdout +@pytest.mark.parametrize("base", ["", "0" * 40], ids=["manual-run", "new-release-branch"]) +def test_fast_ci_without_a_base_preserves_the_last_commit_range(tmp_path: Path, base: str) -> None: + _git(tmp_path, "init", "--quiet") + _commit_file(tmp_path, "earlier.txt", "outside the last commit \n") + target = _commit_file(tmp_path, "last.txt", "last change\n") + + result = _run_whitespace_step(tmp_path, base, target) + + assert result.returncode == 0, result.stdout + result.stderr + assert "earlier.txt" not in result.stdout + + @pytest.mark.parametrize("available", [True, False], ids=["fetch-base-history", "missing-base-fails-closed"]) def test_fast_ci_whitespace_fetches_an_absent_base_without_truncating_history(tmp_path: Path, available: bool) -> None: origin = tmp_path / "origin" @@ -634,8 +661,11 @@ def test_fast_ci_whitespace_fetches_an_absent_base_without_truncating_history(tm assert _git(checkout, "merge-base", base, target).stdout.strip() == common assert _git(checkout, "rev-parse", "--is-shallow-repository").stdout.strip() == "false" assert not _git(checkout, "tag", "--list").stdout.strip() + assert _git(checkout, "config", "--local", "--get-regexp", "extraheader", check=False).returncode == 1 else: assert result.returncode != 0 + assert "upload-pack: not our ref" in result.stderr + assert "Invalid symmetric difference expression" not in result.stderr def test_collector_comparison_fetch_preserves_full_history() -> None: @@ -752,7 +782,7 @@ def _run_workflow_script( env: dict[str, str], ) -> subprocess.CompletedProcess[str]: return subprocess.run( - ["bash", "-c", script], + ["bash", "--noprofile", "--norc", "-eo", "pipefail", "-c", script], cwd=REPOSITORY_ROOT, env={**os.environ, **env}, capture_output=True,