diff --git a/.github/scripts/dependency-update-commit-and-push.sh b/.github/scripts/dependency-update-commit-and-push.sh index 86c935280..ed12aeef0 100755 --- a/.github/scripts/dependency-update-commit-and-push.sh +++ b/.github/scripts/dependency-update-commit-and-push.sh @@ -1,39 +1,57 @@ #!/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. +# +# - 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 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 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" + changes_detected=true 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 + 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 -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 -echo "branch_pushed=${branch_pushed}" >> "${GITHUB_OUTPUT}" +{ + echo "changes_detected=${changes_detected}" + echo "branch_pushed=${branch_pushed}" +} >> "${GITHUB_OUTPUT}" diff --git a/.github/scripts/dependency-update-create-pr.sh b/.github/scripts/dependency-update-create-pr.sh index b35d4d1df..4c0f16137 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 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. - 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/.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..f6f9c9c93 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,14 +58,15 @@ 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 - # of its description names the tracking issue (rule - # sim.process.pr-tracking-issue). + # 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 - 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/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 a3cae0fd7..5a4f012e4 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,177 @@ 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, 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")[3:] == [ + ["git", "push", "origin", "HEAD:refs/heads/update-dependencies"] + ] + + +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, - "true", + REMOTE_HEAD=REMOTE_HEAD, FAKE_GIT_STATUS=" M uv.lock\n", - FAKE_COMMITS_TO_PUSH="1", + FAKE_GIT_DIFF_EXIT="0", ) 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 "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_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") +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 "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")) + assert github_output(output) == { + "changes_detected": "true", + "branch_pushed": "true", + } + assert ["git", "diff", "--quiet", REMOTE_HEAD, "HEAD", "--"] 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 +@pytest.mark.parametrize("remote_head", [REMOTE_HEAD, ""]) +def test_commit_and_push_does_nothing_when_the_update_changes_nothing( + fakes, tmp_path, remote_head ): - result, output = _commit_and_push(fakes, tmp_path, has_open_pr) + """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 github_output(output) == {"branch_pushed": "false"} - assert not any(call[1] in ("commit", "push") for call in fakes.calls("git")) + 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_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 +298,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", ) @@ -271,6 +324,17 @@ 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 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." +) +# 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." ) @@ -291,9 +355,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", @@ -403,8 +467,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, @@ -440,41 +506,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( @@ -515,17 +582,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 }}"}