From b9679f4ff0f754ff7fa4e2f98d1adc77ef9ba7d6 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 01:20:41 +0400 Subject: [PATCH 1/4] Rebuild the dependency-update branch from main on every run The prepare step merged main into the open update PR's branch with `-X theirs`. Merging generated uv.lock files line by line mixed two resolutions, and uv refused the result ("Failed to parse uv.lock"), so every run since at least 2026-10-10 failed at "Generate API clients". The prepare step now checks out update-dependencies from origin/main on every run and records the commit the remote branch points at. The push step replaces the remote branch with --force-with-lease against that commit, or creates it with a plain push when it does not exist. Neither step asks gh about pull requests any more, so the prepare step no longer gets GH_TOKEN. Co-Authored-By: Claude Opus 5.5 --- .../dependency-update-commit-and-push.sh | 39 ++-- .../dependency-update-prepare-branch.sh | 45 ++-- .github/workflows/update-dependencies.yml | 6 +- .../tests/test_workflow_scripts.py | 202 +++++++++--------- 4 files changed, 144 insertions(+), 148 deletions(-) diff --git a/.github/scripts/dependency-update-commit-and-push.sh b/.github/scripts/dependency-update-commit-and-push.sh index 86c935280..fdb77f99f 100755 --- a/.github/scripts/dependency-update-commit-and-push.sh +++ b/.github/scripts/dependency-update-commit-and-push.sh @@ -1,38 +1,43 @@ #!/usr/bin/env bash -# Commit generated dependency changes and update the reusable PR branch. +# Commit the regenerated clients and upgraded locks on the update branch, which +# the prepare step rebuilt from main, and replace the remote update branch. +# +# The push replaces the remote branch only while it still points at the commit +# the prepare step recorded (--force-with-lease), so a push made during the run +# is not lost silently. Without a remote branch, a plain push creates it. +# +# Environment: +# GITHUB_OUTPUT receives branch_pushed=true or branch_pushed=false +# REMOTE_HEAD the prepare step's remote_head output: the remote update +# branch's commit, or empty when the branch does not exist set -euo pipefail : "${GITHUB_OUTPUT:?GITHUB_OUTPUT is required}" -: "${HAS_OPEN_PR:?HAS_OPEN_PR is required}" -if [[ "${HAS_OPEN_PR}" != "true" && "${HAS_OPEN_PR}" != "false" ]]; then - echo "HAS_OPEN_PR must be either true or false" >&2 +branch="update-dependencies" +remote_head="${REMOTE_HEAD:-}" +if [[ -n "${remote_head}" && ! "${remote_head}" =~ ^[0-9a-f]{40}$ ]]; then + echo "REMOTE_HEAD must be empty or a full commit hash: ${remote_head}" >&2 exit 1 fi -changes_detected=false +branch_pushed=false if [[ -z "$(git status --porcelain)" ]]; then echo "No changes detected" else echo "Changes detected" git add --all git commit -m "Update dependencies ($(date -u +%Y-%m-%d))" - changes_detected=true -fi -branch_pushed=false -if [[ "${HAS_OPEN_PR}" == "true" ]]; then - commits_to_push="$(git rev-list --count origin/update-dependencies..HEAD)" - if [[ "${commits_to_push}" -gt 0 ]]; then - git push origin HEAD:update-dependencies - branch_pushed=true + if [[ -n "${remote_head}" ]]; then + git push \ + --force-with-lease="refs/heads/${branch}:${remote_head}" \ + origin "HEAD:refs/heads/${branch}" + else + git push origin "HEAD:refs/heads/${branch}" fi -elif [[ "${changes_detected}" == "true" ]]; then - # No open PR owns this branch, so replacing a stale remote branch cannot - # rewrite active review history. - git push --force-with-lease origin HEAD:update-dependencies branch_pushed=true fi diff --git a/.github/scripts/dependency-update-prepare-branch.sh b/.github/scripts/dependency-update-prepare-branch.sh index ea63f00b0..b880b4ab4 100755 --- a/.github/scripts/dependency-update-prepare-branch.sh +++ b/.github/scripts/dependency-update-prepare-branch.sh @@ -1,36 +1,31 @@ #!/usr/bin/env bash -# Reuse the branch for an open dependency-update PR, or start a new branch -# from current main when no update PR exists. +# Rebuild the update branch from current main, and record the commit the +# remote update branch points at, so the push can replace exactly that commit. +# +# Every run starts from main, even when an update PR is open. Merging main into +# an older update branch merges generated uv.lock files line by line, which +# mixes two resolutions into a lock that uv refuses to parse. +# +# Environment: +# GITHUB_OUTPUT receives remote_head: the remote update branch's commit, or +# empty when the branch does not exist. git reads the remote +# with the credentials the checkout stored. set -euo pipefail -: "${GITHUB_REPOSITORY:?GITHUB_REPOSITORY is required}" : "${GITHUB_OUTPUT:?GITHUB_OUTPUT is required}" +branch="update-dependencies" + git config user.name "policyengine-auto" git config user.email "policyengine-auto@users.noreply.github.com" -open_pr="$( - gh pr list \ - --repo "${GITHUB_REPOSITORY}" \ - --base main \ - --head update-dependencies \ - --state open \ - --json number \ - --jq '.[0].number // empty' -)" - -if [[ -n "${open_pr}" ]]; then - echo "has_open_pr=true" >> "${GITHUB_OUTPUT}" - git fetch origin update-dependencies - git checkout -B update-dependencies origin/update-dependencies - - # Preserve the open PR's commits while incorporating current main. Prefer - # main's version of conflicts because later steps regenerate clients and - # recreate dependency lock changes. - git merge --no-edit -X theirs origin/main -else - echo "has_open_pr=false" >> "${GITHUB_OUTPUT}" - git checkout -B update-dependencies origin/main +remote_head="$(git ls-remote --heads origin "refs/heads/${branch}" | cut -f1)" +if [[ -n "${remote_head}" ]]; then + # The push step compares the rebuilt branch with this commit. + git fetch --quiet origin "${remote_head}" fi + +git checkout -B "${branch}" origin/main +echo "remote_head=${remote_head}" >> "${GITHUB_OUTPUT}" diff --git a/.github/workflows/update-dependencies.yml b/.github/workflows/update-dependencies.yml index 3c4d1d265..3e5cf30c2 100644 --- a/.github/workflows/update-dependencies.yml +++ b/.github/workflows/update-dependencies.yml @@ -43,10 +43,10 @@ jobs: with: python-version: "3.13" + # Rebuilds update-dependencies from main on every run, and records the + # commit the remote branch points at for the push step's lease. - name: Prepare update branch id: branch - env: - GH_TOKEN: ${{ steps.app-token.outputs.token }} run: .github/scripts/dependency-update-prepare-branch.sh - name: Generate API clients @@ -58,7 +58,7 @@ jobs: - name: Commit and push update id: push env: - HAS_OPEN_PR: ${{ steps.branch.outputs.has_open_pr }} + REMOTE_HEAD: ${{ steps.branch.outputs.remote_head }} run: .github/scripts/dependency-update-commit-and-push.sh # Opens the update PR, or, when one is open, makes sure the first line diff --git a/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py b/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py index a3cae0fd7..3da094541 100644 --- a/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py +++ b/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py @@ -45,21 +45,48 @@ def test_scripts_that_workflows_run_directly_are_executable(): COMMIT_AND_PUSH = "dependency-update-commit-and-push.sh" CREATE_PR = "dependency-update-create-pr.sh" +REMOTE_HEAD = "0123456789abcdef0123456789abcdef01234567" + +# The scripts no longer ask gh about pull requests; this fake answers as the +# pre-fix script expected, so a test can show that an open PR changes nothing. FAKE_GH_PR_LIST = """\ if [[ "$1 $2" == "pr list" ]]; then printf '%s' "${FAKE_OPEN_PR:-}" - exit "${FAKE_GH_EXIT:-0}" fi -exit "${FAKE_GH_EXIT:-0}" """ +# Each run rebuilds the update branch from main; the fake git reports the +# remote branch's head from FAKE_REMOTE_HEAD (unset: the branch does not exist). FAKE_GIT = """\ case "$1" in + ls-remote) + if [[ -n "${FAKE_REMOTE_HEAD:-}" ]]; then + printf '%s\\trefs/heads/update-dependencies\\n' "${FAKE_REMOTE_HEAD}" + fi + exit "${FAKE_GIT_LS_REMOTE_EXIT:-0}" + ;; status) printf '%s' "${FAKE_GIT_STATUS:-}" ;; - rev-list) printf '%s\\n' "${FAKE_COMMITS_TO_PUSH:-0}" ;; + diff) exit "${FAKE_GIT_DIFF_EXIT:-1}" ;; push) exit "${FAKE_GIT_PUSH_EXIT:-0}" ;; esac """ +GIT_IDENTITY = [ + ["git", "config", "user.name", "policyengine-auto"], + [ + "git", + "config", + "user.email", + "policyengine-auto@users.noreply.github.com", + ], +] +REMOTE_BRANCH_LOOKUP = [ + "git", + "ls-remote", + "--heads", + "origin", + "refs/heads/update-dependencies", +] +REBUILD_FROM_MAIN = ["git", "checkout", "-B", "update-dependencies", "origin/main"] def test_update_dependencies_workflow_runs_the_tested_scripts(): @@ -69,10 +96,18 @@ def test_update_dependencies_workflow_runs_the_tested_scripts(): assert f"run: .github/scripts/{script}" in workflow +def _update_dependencies_steps() -> dict[str, dict]: + workflow = yaml.safe_load( + (WORKFLOWS_DIR / "update-dependencies.yml").read_text(encoding="utf-8") + ) + steps = workflow["jobs"]["update-dependencies"]["steps"] + return {step["run"]: step for step in steps if "run" in step} + + def _prepare_branch(fakes: FakeCommands, tmp_path: Path, **env: str): output = tmp_path / "github-output" fakes.add("gh", FAKE_GH_PR_LIST) - fakes.add("git") + fakes.add("git", FAKE_GIT) result = run_script( PREPARE_BRANCH, env=fakes.env( @@ -84,159 +119,120 @@ def _prepare_branch(fakes: FakeCommands, tmp_path: Path, **env: str): return result, output -def test_prepare_branch_reuses_the_branch_of_an_open_update_pr(fakes, tmp_path): - result, output = _prepare_branch(fakes, tmp_path, FAKE_OPEN_PR="42") +def test_prepare_branch_rebuilds_the_branch_from_main_when_an_update_pr_is_open( + fakes, tmp_path +): + """Merging main into an old update branch mixed two uv.lock resolutions.""" + result, output = _prepare_branch( + fakes, tmp_path, FAKE_OPEN_PR="583", FAKE_REMOTE_HEAD=REMOTE_HEAD + ) assert result.returncode == 0, result.stderr - assert github_output(output) == {"has_open_pr": "true"} - assert fakes.calls("gh") == [ - [ - "gh", - "pr", - "list", - "--repo", - "PolicyEngine/policyengine-sim-api", - "--base", - "main", - "--head", - "update-dependencies", - "--state", - "open", - "--json", - "number", - "--jq", - ".[0].number // empty", - ] - ] assert fakes.calls("git") == [ - ["git", "config", "user.name", "policyengine-auto"], - [ - "git", - "config", - "user.email", - "policyengine-auto@users.noreply.github.com", - ], - ["git", "fetch", "origin", "update-dependencies"], - [ - "git", - "checkout", - "-B", - "update-dependencies", - "origin/update-dependencies", - ], - ["git", "merge", "--no-edit", "-X", "theirs", "origin/main"], + *GIT_IDENTITY, + REMOTE_BRANCH_LOOKUP, + ["git", "fetch", "--quiet", "origin", REMOTE_HEAD], + REBUILD_FROM_MAIN, ] + assert fakes.calls("gh") == [] + assert github_output(output) == {"remote_head": REMOTE_HEAD} -def test_prepare_branch_starts_from_main_without_an_open_update_pr(fakes, tmp_path): +def test_prepare_branch_starts_from_main_without_an_update_branch(fakes, tmp_path): result, output = _prepare_branch(fakes, tmp_path) assert result.returncode == 0, result.stderr - assert github_output(output) == {"has_open_pr": "false"} - assert fakes.calls("git")[2:] == [ - ["git", "checkout", "-B", "update-dependencies", "origin/main"] + assert github_output(output) == {"remote_head": ""} + assert fakes.calls("git") == [ + *GIT_IDENTITY, + REMOTE_BRANCH_LOOKUP, + REBUILD_FROM_MAIN, ] -def test_prepare_branch_fails_when_the_pr_lookup_fails(fakes, tmp_path): - result, output = _prepare_branch(fakes, tmp_path, FAKE_GH_EXIT="1") +def test_prepare_branch_fails_when_the_branch_lookup_fails(fakes, tmp_path): + result, output = _prepare_branch(fakes, tmp_path, FAKE_GIT_LS_REMOTE_EXIT="128") assert result.returncode != 0 assert github_output(output) == {} assert not any(call[1] == "checkout" for call in fakes.calls("git")) -def test_prepare_branch_requires_the_repository(fakes, tmp_path): - fakes.add("gh", FAKE_GH_PR_LIST) - fakes.add("git") - env = fakes.env(GITHUB_OUTPUT=str(tmp_path / "github-output")) - env.pop("GITHUB_REPOSITORY", None) +def test_prepare_branch_requires_the_output_file(fakes, tmp_path): + fakes.add("git", FAKE_GIT) + env = fakes.env() + env.pop("GITHUB_OUTPUT", None) result = run_script(PREPARE_BRANCH, env=env) assert result.returncode != 0 - assert "GITHUB_REPOSITORY is required" in result.stderr + assert "GITHUB_OUTPUT is required" in result.stderr assert fakes.calls() == [] -def _commit_and_push(fakes: FakeCommands, tmp_path: Path, has_open_pr: str, **env): +def test_update_dependencies_workflow_passes_the_recorded_head_to_the_push(): + steps = _update_dependencies_steps() + + assert "env" not in steps[f".github/scripts/{PREPARE_BRANCH}"] + assert steps[f".github/scripts/{COMMIT_AND_PUSH}"]["env"] == { + "REMOTE_HEAD": "${{ steps.branch.outputs.remote_head }}" + } + + +def _commit_and_push(fakes: FakeCommands, tmp_path: Path, **env: str): output = tmp_path / "github-output" fakes.add("git", FAKE_GIT) fakes.add("date", "echo 2026-10-13\n") result = run_script( COMMIT_AND_PUSH, - env=fakes.env(GITHUB_OUTPUT=str(output), HAS_OPEN_PR=has_open_pr, **env), + env=fakes.env(GITHUB_OUTPUT=str(output), **env), ) return result, output -def test_commit_and_push_replaces_the_branch_when_no_pr_owns_it(fakes, tmp_path): +def test_commit_and_push_replaces_the_branch_with_a_lease_on_the_recorded_head( + fakes, tmp_path +): result, output = _commit_and_push( - fakes, tmp_path, "false", FAKE_GIT_STATUS=" M uv.lock\n" + fakes, tmp_path, REMOTE_HEAD=REMOTE_HEAD, FAKE_GIT_STATUS=" M uv.lock\n" ) assert result.returncode == 0, result.stderr assert "Changes detected" in result.stdout - assert github_output(output) == {"branch_pushed": "true"} - assert fakes.calls("git") == [ + assert github_output(output)["branch_pushed"] == "true" + git_calls = fakes.calls("git") + assert git_calls[:3] == [ ["git", "status", "--porcelain"], ["git", "add", "--all"], ["git", "commit", "-m", "Update dependencies (2026-10-13)"], - [ - "git", - "push", - "--force-with-lease", - "origin", - "HEAD:update-dependencies", - ], + ] + assert git_calls[-1] == [ + "git", + "push", + f"--force-with-lease=refs/heads/update-dependencies:{REMOTE_HEAD}", + "origin", + "HEAD:refs/heads/update-dependencies", ] assert fakes.calls("date") == [["date", "-u", "+%Y-%m-%d"]] -def test_commit_and_push_appends_to_the_branch_of_an_open_pr(fakes, tmp_path): +def test_commit_and_push_creates_the_branch_with_a_plain_push(fakes, tmp_path): result, output = _commit_and_push( - fakes, - tmp_path, - "true", - FAKE_GIT_STATUS=" M uv.lock\n", - FAKE_COMMITS_TO_PUSH="1", + fakes, tmp_path, REMOTE_HEAD="", FAKE_GIT_STATUS=" M uv.lock\n" ) assert result.returncode == 0, result.stderr - assert github_output(output) == {"branch_pushed": "true"} - assert fakes.calls("git")[-2:] == [ - ["git", "rev-list", "--count", "origin/update-dependencies..HEAD"], - ["git", "push", "origin", "HEAD:update-dependencies"], + assert github_output(output)["branch_pushed"] == "true" + assert fakes.calls("git")[3:] == [ + ["git", "push", "origin", "HEAD:refs/heads/update-dependencies"] ] -def test_commit_and_push_pushes_earlier_commits_to_an_open_pr(fakes, tmp_path): - """A merge from main can leave commits to push even without new changes.""" - result, output = _commit_and_push(fakes, tmp_path, "true", FAKE_COMMITS_TO_PUSH="2") - - assert result.returncode == 0, result.stderr - assert "No changes detected" in result.stdout - assert github_output(output) == {"branch_pushed": "true"} - assert ["git", "push", "origin", "HEAD:update-dependencies"] in fakes.calls("git") - assert not any(call[1] == "commit" for call in fakes.calls("git")) - - -@pytest.mark.parametrize("has_open_pr", ["true", "false"]) -def test_commit_and_push_skips_the_push_when_nothing_is_new( - fakes, tmp_path, has_open_pr -): - result, output = _commit_and_push(fakes, tmp_path, has_open_pr) - - assert result.returncode == 0, result.stderr - assert github_output(output) == {"branch_pushed": "false"} - assert not any(call[1] in ("commit", "push") for call in fakes.calls("git")) - - -def test_commit_and_push_rejects_an_invalid_open_pr_flag(fakes, tmp_path): - result, output = _commit_and_push(fakes, tmp_path, "yes") +def test_commit_and_push_rejects_a_remote_head_that_is_not_a_commit(fakes, tmp_path): + result, output = _commit_and_push(fakes, tmp_path, REMOTE_HEAD="main") assert result.returncode == 1 - assert "HAS_OPEN_PR must be either true or false" in result.stderr + assert "REMOTE_HEAD must be empty or a full commit hash" in result.stderr assert github_output(output) == {} assert fakes.calls() == [] @@ -245,7 +241,7 @@ def test_commit_and_push_fails_when_the_push_fails(fakes, tmp_path): result, output = _commit_and_push( fakes, tmp_path, - "false", + REMOTE_HEAD=REMOTE_HEAD, FAKE_GIT_STATUS=" M uv.lock\n", FAKE_GIT_PUSH_EXIT="1", ) From f3ff5c779b0e0a5489a10fbeb520c49f7bd39461 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 01:21:38 +0400 Subject: [PATCH 2/4] Skip the dependency-update push when the branch already holds the update The push step now compares the rebuilt branch's files with the commit the remote branch points at and skips the push when they match; the commit message carries the date, so the commits themselves always differ. When the update changes no file on main, nothing is committed or pushed: pushing main's own commit would leave an open update PR with no changes. The step reports changes_detected, and the PR step runs whenever the update changed files on main, so an update PR is opened or kept current even when the push was skipped. Co-Authored-By: Claude Opus 5.5 --- .../dependency-update-commit-and-push.sh | 27 +++++-- .github/workflows/update-dependencies.yml | 5 +- .../tests/test_workflow_scripts.py | 71 ++++++++++++++++--- 3 files changed, 84 insertions(+), 19 deletions(-) diff --git a/.github/scripts/dependency-update-commit-and-push.sh b/.github/scripts/dependency-update-commit-and-push.sh index fdb77f99f..ed12aeef0 100755 --- a/.github/scripts/dependency-update-commit-and-push.sh +++ b/.github/scripts/dependency-update-commit-and-push.sh @@ -3,12 +3,17 @@ # Commit the regenerated clients and upgraded locks on the update branch, which # the prepare step rebuilt from main, and replace the remote update branch. # -# The push replaces the remote branch only while it still points at the commit -# the prepare step recorded (--force-with-lease), so a push made during the run -# is not lost silently. Without a remote branch, a plain push creates it. +# - When the update changes no file on main, nothing is committed or pushed. +# - When the remote branch already holds the same files, the push is skipped. +# The commit message carries the date, so commits are compared by their files. +# - Otherwise the push replaces the remote branch only while it still points at +# the commit the prepare step recorded (--force-with-lease), so a push made +# during the run is not lost silently. Without a remote branch, a plain push +# creates it. # # Environment: -# GITHUB_OUTPUT receives branch_pushed=true or branch_pushed=false +# GITHUB_OUTPUT receives changes_detected (true when the update changed files +# on main, so an update PR should exist) and branch_pushed # REMOTE_HEAD the prepare step's remote_head output: the remote update # branch's commit, or empty when the branch does not exist @@ -23,22 +28,30 @@ if [[ -n "${remote_head}" && ! "${remote_head}" =~ ^[0-9a-f]{40}$ ]]; then exit 1 fi +changes_detected=false branch_pushed=false if [[ -z "$(git status --porcelain)" ]]; then echo "No changes detected" else echo "Changes detected" + changes_detected=true git add --all git commit -m "Update dependencies ($(date -u +%Y-%m-%d))" - if [[ -n "${remote_head}" ]]; then + if [[ -n "${remote_head}" ]] && git diff --quiet "${remote_head}" HEAD --; then + echo "${branch} already holds this update; not pushing." + elif [[ -n "${remote_head}" ]]; then git push \ --force-with-lease="refs/heads/${branch}:${remote_head}" \ origin "HEAD:refs/heads/${branch}" + branch_pushed=true else git push origin "HEAD:refs/heads/${branch}" + branch_pushed=true fi - branch_pushed=true fi -echo "branch_pushed=${branch_pushed}" >> "${GITHUB_OUTPUT}" +{ + echo "changes_detected=${changes_detected}" + echo "branch_pushed=${branch_pushed}" +} >> "${GITHUB_OUTPUT}" diff --git a/.github/workflows/update-dependencies.yml b/.github/workflows/update-dependencies.yml index 3e5cf30c2..b8cd82b8e 100644 --- a/.github/workflows/update-dependencies.yml +++ b/.github/workflows/update-dependencies.yml @@ -63,9 +63,10 @@ jobs: # Opens the update PR, or, when one is open, makes sure the first line # of its description names the tracking issue (rule - # sim.process.pr-tracking-issue). + # sim.process.pr-tracking-issue). It runs whenever the update changed + # files on main, including when the branch already held them. - name: Open or update the pull request - if: steps.push.outputs.branch_pushed == 'true' + if: steps.push.outputs.changes_detected == 'true' env: GH_TOKEN: ${{ steps.app-token.outputs.token }} run: .github/scripts/dependency-update-create-pr.sh diff --git a/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py b/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py index 3da094541..4bbd5af23 100644 --- a/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py +++ b/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py @@ -228,6 +228,63 @@ def test_commit_and_push_creates_the_branch_with_a_plain_push(fakes, tmp_path): ] +def test_commit_and_push_skips_the_push_when_the_branch_holds_the_update( + fakes, tmp_path +): + """The commit's date differs every day, so the push compares files.""" + result, output = _commit_and_push( + fakes, + tmp_path, + REMOTE_HEAD=REMOTE_HEAD, + FAKE_GIT_STATUS=" M uv.lock\n", + FAKE_GIT_DIFF_EXIT="0", + ) + + assert result.returncode == 0, result.stderr + assert "update-dependencies already holds this update" in result.stdout + assert github_output(output) == { + "changes_detected": "true", + "branch_pushed": "false", + } + assert fakes.calls("git")[-1] == [ + "git", + "diff", + "--quiet", + REMOTE_HEAD, + "HEAD", + "--", + ] + + +def test_commit_and_push_reports_a_push_that_changed_the_branch(fakes, tmp_path): + result, output = _commit_and_push( + fakes, tmp_path, REMOTE_HEAD=REMOTE_HEAD, FAKE_GIT_STATUS=" M uv.lock\n" + ) + + assert result.returncode == 0, result.stderr + assert github_output(output) == { + "changes_detected": "true", + "branch_pushed": "true", + } + assert ["git", "diff", "--quiet", REMOTE_HEAD, "HEAD", "--"] in fakes.calls("git") + + +@pytest.mark.parametrize("remote_head", [REMOTE_HEAD, ""]) +def test_commit_and_push_does_nothing_when_the_update_changes_nothing( + fakes, tmp_path, remote_head +): + """Pushing main's own commit would leave an open update PR with no changes.""" + result, output = _commit_and_push(fakes, tmp_path, REMOTE_HEAD=remote_head) + + assert result.returncode == 0, result.stderr + assert "No changes detected" in result.stdout + assert github_output(output) == { + "changes_detected": "false", + "branch_pushed": "false", + } + assert fakes.calls("git") == [["git", "status", "--porcelain"]] + + def test_commit_and_push_rejects_a_remote_head_that_is_not_a_commit(fakes, tmp_path): result, output = _commit_and_push(fakes, tmp_path, REMOTE_HEAD="main") @@ -511,17 +568,11 @@ def test_create_pr_requires_the_repository(fakes): assert fakes.calls() == [] -def test_update_dependencies_workflow_updates_the_pr_whenever_it_pushes(): - """With or without an open update PR, a push runs the create-PR script.""" - workflow = yaml.safe_load( - (WORKFLOWS_DIR / "update-dependencies.yml").read_text(encoding="utf-8") - ) - steps = workflow["jobs"]["update-dependencies"]["steps"] - create_pr = next( - step for step in steps if step.get("run") == f".github/scripts/{CREATE_PR}" - ) +def test_update_dependencies_workflow_updates_the_pr_whenever_files_change(): + """The PR step runs even when the branch already held the update.""" + create_pr = _update_dependencies_steps()[f".github/scripts/{CREATE_PR}"] - assert create_pr["if"] == "steps.push.outputs.branch_pushed == 'true'" + assert create_pr["if"] == "steps.push.outputs.changes_detected == 'true'" assert create_pr["env"] == {"GH_TOKEN": "${{ steps.app-token.outputs.token }}"} From 1f0ed7f174212b3967223579f56787a904bdfae7 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 01:22:48 +0400 Subject: [PATCH 3/4] Reuse the open dependency-update PR and replace its description Each run now rebuilds the update branch from main, so the open update PR's description said the wrong thing ("Subsequent runs append commits"). The create-PR step still reuses an open update PR, but now replaces its whole description, "Fixes #N" first, whenever it differs from the current one; before, it changed only the first line. The description says that each run replaces the branch, so commits pushed by hand are lost on the next run. The open-PR lookup now ignores pull requests from forks whose branch is also named update-dependencies, as the Public API's relock does, so the step never rewrites a fork PR's description. Co-Authored-By: Claude Opus 5.5 --- .../scripts/dependency-update-create-pr.sh | 61 +++++++++---------- changelog_entry.yaml | 1 + docs/engineering/skills/github-prs.md | 5 +- .../tests/test_workflow_scripts.py | 38 ++++++++---- 4 files changed, 57 insertions(+), 48 deletions(-) diff --git a/.github/scripts/dependency-update-create-pr.sh b/.github/scripts/dependency-update-create-pr.sh index b35d4d1df..9b3f6e809 100755 --- a/.github/scripts/dependency-update-create-pr.sh +++ b/.github/scripts/dependency-update-create-pr.sh @@ -1,18 +1,20 @@ #!/usr/bin/env bash -# After an update branch has been pushed, make sure an open dependency-update -# PR exists and that the first line of its description names its tracking -# issue, as rule sim.process.pr-tracking-issue requires of every PR. +# After the dependency update changed files on main, make sure an open +# dependency-update PR exists and that the first line of its description names +# its tracking issue, as rule sim.process.pr-tracking-issue requires of every PR. # # The tracking issue is the oldest open issue titled "Update dependencies", or # a new one when none is open. "Fixes #N" closes it when the PR merges, so the # next update PR gets a new issue. # -# - Without an open update PR, open one whose description starts with -# "Fixes #N". -# - With one, make "Fixes #N" its first line: replace an earlier "Fixes #" -# line, or add the line above a description that has none (such as a PR -# opened before the rule). +# - Without an open update PR, open one. +# - With one, keep it, and replace its description when it differs from the +# current one. Each run rebuilds the branch from main, so the description is +# the workflow's, not a reviewer's; this also rewrites the description of a +# PR opened before the rule, or before the branch was rebuilt on every run. +# +# Either way the description is "Fixes #N", a blank line and the summary. # # Environment: # GITHUB_REPOSITORY owner/name of this repository (set by the runner) @@ -25,7 +27,6 @@ set -euo pipefail ISSUE_TITLE="Update dependencies" ISSUE_BODY="Tracks the automated daily dependency updates from update-dependencies.yml. The update pull request names this issue on the first line of its description and closes it when it merges." -PR_SUMMARY="Automated daily dependency updates. Subsequent runs append commits to this branch while the pull request remains open." # Set ISSUE_NUMBER to the oldest open "Update dependencies" issue, opening one # when none is open. The issues API lists pull requests too; they have a @@ -61,35 +62,30 @@ find_or_open_tracking_issue() { ISSUE_NUMBER="${issue_number}" } -# Print the description with "Fixes #" as its first line. -description_naming_issue() { - local description="$1" - local issue_number="$2" - local first_line="${description%%$'\n'*}" - local fixes_line="Fixes #${issue_number}" +# Print the update PR's description, naming the tracking issue first. +update_pr_description() { + printf 'Fixes #%s\n\n' "${ISSUE_NUMBER}" + cat <<'EOF' +Automated daily dependency update by `.github/workflows/update-dependencies.yml`. It regenerates the API clients, then runs `make update`, which runs `uv lock --upgrade` in every project. CI on this pull request tests the upgraded versions. - if [[ "${first_line}" =~ ^Fixes\ \#[0-9]+[[:space:]]*$ ]]; then - if [[ "${description}" == *$'\n'* ]]; then - printf '%s\n%s' "${fixes_line}" "${description#*$'\n'}" - else - printf '%s' "${fixes_line}" - fi - else - printf '%s\n\n%s' "${fixes_line}" "${description}" - fi +Each run rebuilds `update-dependencies` from `main` and replaces it, so commits pushed to this branch by hand are lost on the next run. +EOF } +# --head matches a branch of the same name in a fork too; only this +# repository's branch is the update branch. open_pr="$( gh pr list \ --repo "${GITHUB_REPOSITORY}" \ --base main \ --head update-dependencies \ --state open \ - --json number \ - --jq '.[0].number // empty' + --json number,isCrossRepository \ + --jq 'map(select(.isCrossRepository | not)) | .[0].number // empty' )" ISSUE_NUMBER="" find_or_open_tracking_issue +description="$(update_pr_description)" if [[ -z "${open_pr}" ]]; then gh pr create \ @@ -98,24 +94,23 @@ if [[ -z "${open_pr}" ]]; then --body-file - \ --base main \ --head update-dependencies \ - <<<"Fixes #${ISSUE_NUMBER}"$'\n\n'"${PR_SUMMARY}" + <<<"${description}" exit 0 fi -description="$( +current_description="$( gh pr view "${open_pr}" \ --repo "${GITHUB_REPOSITORY}" \ --json body \ --jq '.body' )" -linked_description="$(description_naming_issue "${description}" "${ISSUE_NUMBER}")" -if [[ "${linked_description}" == "${description}" ]]; then - echo "Pull request #${open_pr} already names tracking issue #${ISSUE_NUMBER}." +if [[ "${current_description}" == "${description}" ]]; then + echo "Pull request #${open_pr} already has the current description." exit 0 fi gh pr edit "${open_pr}" \ --repo "${GITHUB_REPOSITORY}" \ --body-file - \ - <<<"${linked_description}" -echo "Pull request #${open_pr} now names tracking issue #${ISSUE_NUMBER}." + <<<"${description}" +echo "Pull request #${open_pr} now has the current description, naming tracking issue #${ISSUE_NUMBER}." diff --git a/changelog_entry.yaml b/changelog_entry.yaml index 2da3c0513..821a21cf1 100644 --- a/changelog_entry.yaml +++ b/changelog_entry.yaml @@ -27,6 +27,7 @@ - Ran the full PR workflow and the Modal image smoke only on the lowest unmerged pull request in a stack, behind a new CI complete gate job that fails on the others fixed: - Stopped trusting the compromised eMudhra (emSign) intermediate CA certificates; every lockfile now pins certifi 2026.10.19, which drops emSign ECC Root CA - G3, every Modal image upgrades Debian's ca-certificates package right after its base layer, and the Modal images and the Cloud Run entrypoint image set GRPC_DEFAULT_SSL_ROOTS_FILE_PATH so grpcio verifies servers against the operating-system CA bundle instead of the CA list compiled into it + - Rebuilt the daily dependency update's branch from main on every run and replaced the remote branch with a lease, instead of merging main into the open update PR's branch, which mixed two uv.lock resolutions into locks uv could not parse; the push is skipped when the branch already holds the update, and the open update PR is reused with a rewritten description - Preserved bounded reform stop dates in UK and US simulations while retaining current-law baseline artifact reuse - Resolved PolicyEngine-managed regional datasets using bundled data versions instead of release-manifest commit hashes when loading from GCS - Added a Modal app-release bundle backfill for missing data artifact revisions diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 77288057a..31c6f6e15 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -98,8 +98,9 @@ To fix a failure: Automated pull requests follow the rule too. The policyengine package update (`update-policyengine-package.sh`) opens an issue for each version. The daily dependency update (`dependency-update-create-pr.sh`) reuses the open "Update -dependencies" issue or opens one, and also puts its `Fixes` line first on an -update pull request that is already open. +dependencies" issue or opens one. It also reuses an update pull request that is +already open, and replaces its description, `Fixes` line first, because each run +rebuilds the update branch from `main`. ## PR Title diff --git a/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py b/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py index 4bbd5af23..117440ec5 100644 --- a/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py +++ b/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py @@ -324,6 +324,15 @@ def test_commit_and_push_fails_when_the_push_fails(fakes, tmp_path): esac """ UPDATE_PR_SUMMARY = ( + "Automated daily dependency update by " + "`.github/workflows/update-dependencies.yml`. It regenerates the API " + "clients, then runs `make update`, which runs `uv lock --upgrade` in every " + "project. CI on this pull request tests the upgraded versions.\n\n" + "Each run rebuilds `update-dependencies` from `main` and replaces it, so " + "commits pushed to this branch by hand are lost on the next run." +) +# The summary of update PRs opened before each run rebuilt the branch. +APPENDING_UPDATE_PR_SUMMARY = ( "Automated daily dependency updates. Subsequent runs append commits to " "this branch while the pull request remains open." ) @@ -344,9 +353,9 @@ def test_commit_and_push_fails_when_the_push_fails(fakes, tmp_path): "--state", "open", "--json", - "number", + "number,isCrossRepository", "--jq", - ".[0].number // empty", + "map(select(.isCrossRepository | not)) | .[0].number // empty", ] OPEN_TRACKING_ISSUE_LOOKUP = [ "gh", @@ -456,8 +465,10 @@ def test_create_pr_fails_when_the_issue_lookup_fails(fakes, tmp_path): assert descriptions == "" -def test_create_pr_puts_the_tracking_issue_first_on_an_open_update_pr(fakes, tmp_path): - """An update PR opened before the rule, such as #583, gets a Fixes line.""" +def test_create_pr_reuses_the_open_update_pr_and_rewrites_its_description( + fakes, tmp_path +): + """An update PR opened before the rule, such as #583, is kept and rewritten.""" result, descriptions = _create_pr( fakes, tmp_path, @@ -493,41 +504,42 @@ def test_create_pr_puts_the_tracking_issue_first_on_an_open_update_pr(fakes, tmp "-", ], ] - assert descriptions == f"Fixes #870\n\n{LEGACY_UPDATE_PR_BODY}\n" + assert descriptions == f"Fixes #870\n\n{UPDATE_PR_SUMMARY}\n" @pytest.mark.parametrize("line_ending", ["\n", "\r\n"]) -def test_create_pr_replaces_an_earlier_fixes_line_on_the_open_update_pr( +def test_create_pr_names_the_open_issue_instead_of_a_closed_one( fakes, tmp_path, line_ending ): """An open update PR whose issue was closed names the open one instead.""" - rest = f"{line_ending}{UPDATE_PR_SUMMARY}" result, descriptions = _create_pr( fakes, tmp_path, FAKE_OPEN_PR="900", - FAKE_OPEN_PR_BODY=f"Fixes #860 {line_ending}{rest}", + FAKE_OPEN_PR_BODY=f"Fixes #860{line_ending}{line_ending}{UPDATE_PR_SUMMARY}", FAKE_OPEN_ISSUES="870\n", ) assert result.returncode == 0, result.stderr - assert descriptions == f"Fixes #870\n{rest}\n" + assert descriptions == f"Fixes #870\n\n{UPDATE_PR_SUMMARY}\n" -def test_create_pr_replaces_a_fixes_line_that_is_the_whole_description(fakes, tmp_path): +def test_create_pr_rewrites_a_description_that_says_runs_append_commits( + fakes, tmp_path +): result, descriptions = _create_pr( fakes, tmp_path, FAKE_OPEN_PR="900", - FAKE_OPEN_PR_BODY="Fixes #860", + FAKE_OPEN_PR_BODY=f"Fixes #870\n\n{APPENDING_UPDATE_PR_SUMMARY}", FAKE_OPEN_ISSUES="870\n", ) assert result.returncode == 0, result.stderr - assert descriptions == "Fixes #870\n" + assert descriptions == f"Fixes #870\n\n{UPDATE_PR_SUMMARY}\n" -def test_create_pr_leaves_an_open_update_pr_that_names_the_tracking_issue( +def test_create_pr_leaves_an_open_update_pr_with_the_current_description( fakes, tmp_path ): result, descriptions = _create_pr( From f52ba83585298578b4cc5bf850e028c6ea817d29 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 01:28:23 +0400 Subject: [PATCH 4/4] Say the dependency-update PR changes only uv.lock files The update PR's summary said the workflow "regenerates the API clients", which reads as if the PR contained client changes. The generated clients are not committed, so the PR changes only uv.lock files; the summary now says so. The workflow's comment on the PR step now says the step replaces an open PR's description, not only its first line. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/dependency-update-create-pr.sh | 2 +- .github/workflows/update-dependencies.yml | 4 ++-- .../tests/test_workflow_scripts.py | 8 +++++--- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/.github/scripts/dependency-update-create-pr.sh b/.github/scripts/dependency-update-create-pr.sh index 9b3f6e809..4c0f16137 100755 --- a/.github/scripts/dependency-update-create-pr.sh +++ b/.github/scripts/dependency-update-create-pr.sh @@ -66,7 +66,7 @@ find_or_open_tracking_issue() { update_pr_description() { printf 'Fixes #%s\n\n' "${ISSUE_NUMBER}" cat <<'EOF' -Automated daily dependency update by `.github/workflows/update-dependencies.yml`. It regenerates the API clients, then runs `make update`, which runs `uv lock --upgrade` in every project. CI on this pull request tests the upgraded versions. +Automated daily dependency update by `.github/workflows/update-dependencies.yml`. It runs `./scripts/generate-clients.sh` (the generated clients are not committed), then `make update`, which runs `uv lock --upgrade` in every project, so this pull request changes only `uv.lock` files. CI on this pull request tests the upgraded versions. Each run rebuilds `update-dependencies` from `main` and replaces it, so commits pushed to this branch by hand are lost on the next run. EOF diff --git a/.github/workflows/update-dependencies.yml b/.github/workflows/update-dependencies.yml index b8cd82b8e..f6f9c9c93 100644 --- a/.github/workflows/update-dependencies.yml +++ b/.github/workflows/update-dependencies.yml @@ -61,8 +61,8 @@ jobs: REMOTE_HEAD: ${{ steps.branch.outputs.remote_head }} run: .github/scripts/dependency-update-commit-and-push.sh - # Opens the update PR, or, when one is open, makes sure the first line - # of its description names the tracking issue (rule + # Opens the update PR, or, when one is open, replaces its description, + # whose first line names the tracking issue (rule # sim.process.pr-tracking-issue). It runs whenever the update changed # files on main, including when the branch already held them. - name: Open or update the pull request diff --git a/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py b/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py index 117440ec5..5a4f012e4 100644 --- a/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py +++ b/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py @@ -325,9 +325,11 @@ def test_commit_and_push_fails_when_the_push_fails(fakes, tmp_path): """ UPDATE_PR_SUMMARY = ( "Automated daily dependency update by " - "`.github/workflows/update-dependencies.yml`. It regenerates the API " - "clients, then runs `make update`, which runs `uv lock --upgrade` in every " - "project. CI on this pull request tests the upgraded versions.\n\n" + "`.github/workflows/update-dependencies.yml`. It runs " + "`./scripts/generate-clients.sh` (the generated clients are not " + "committed), then `make update`, which runs `uv lock --upgrade` in every " + "project, so this pull request changes only `uv.lock` files. CI on this " + "pull request tests the upgraded versions.\n\n" "Each run rebuilds `update-dependencies` from `main` and replaces it, so " "commits pushed to this branch by hand are lost on the next run." )