From 8d6bff78de6c140b1674f95187498749765f1f45 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 25 Oct 2026 06:08:53 -0400 Subject: [PATCH 1/2] Report Dependabot runs as skipped in Aggregate impact The measure step exited early for Dependabot, which has no data access, before it copied the tool into $RUNNER_TEMP/tool. The compare step then failed with "No module named pe_uk_aggregate_impact", so every Dependabot pull request failed the check. Set the tool up before any early exit, and test the measure and compare steps as a Dependabot run. Fixes #2664 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/aggregate-impact.yaml | 8 +-- .../aggregate-impact-dependabot-skip.fixed.md | 1 + .../test_aggregate_impact_workflow.py | 50 +++++++++++++++++++ 3 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 changelog.d/aggregate-impact-dependabot-skip.fixed.md diff --git a/.github/workflows/aggregate-impact.yaml b/.github/workflows/aggregate-impact.yaml index fe842b9637..f308ca24e1 100644 --- a/.github/workflows/aggregate-impact.yaml +++ b/.github/workflows/aggregate-impact.yaml @@ -146,6 +146,11 @@ jobs: HEAD_NAME: ${{ steps.commits.outputs.head_name }} run: | mkdir -p measured/base measured/head + # The comparison runs the tool even when nothing is measured, so set + # it up before any early exit. + tool="$RUNNER_TEMP/tool" + mkdir -p "$tool" + cp -R head/policyengine_uk/utils/aggregate_impact "$tool/pe_uk_aggregate_impact" if [ -z "$HUGGING_FACE_TOKEN" ]; then if [ "$ACTOR" = "dependabot[bot]" ]; then echo "::notice::Dependabot runs have no data access; the aggregate impact report is skipped." @@ -156,9 +161,6 @@ jobs: echo "::error::HUGGING_FACE_TOKEN is not available, so the certified dataset cannot be read." exit 1 fi - tool="$RUNNER_TEMP/tool" - mkdir -p "$tool" - cp -R head/policyengine_uk/utils/aggregate_impact "$tool/pe_uk_aggregate_impact" measure() { side="$1"; name="$2" # A crash is recorded in result.json for the comparison to judge. diff --git a/changelog.d/aggregate-impact-dependabot-skip.fixed.md b/changelog.d/aggregate-impact-dependabot-skip.fixed.md new file mode 100644 index 0000000000..cd5dd6d49d --- /dev/null +++ b/changelog.d/aggregate-impact-dependabot-skip.fixed.md @@ -0,0 +1 @@ +- The Aggregate impact report now reports a Dependabot pull request as skipped, as it was meant to, instead of failing it. Dependabot runs have no data access, and the measure step stopped before it set up the tool the comparison runs. diff --git a/policyengine_uk/tests/code_health/test_aggregate_impact_workflow.py b/policyengine_uk/tests/code_health/test_aggregate_impact_workflow.py index 503f4a414e..2806669476 100644 --- a/policyengine_uk/tests/code_health/test_aggregate_impact_workflow.py +++ b/policyengine_uk/tests/code_health/test_aggregate_impact_workflow.py @@ -17,6 +17,11 @@ base the job chose. 4. A manual dispatch compares the commits it names, as before. +No data access. A Dependabot run cannot read the certified dataset, so it +measures nothing. The comparison still runs, reports the run as skipped and +passes. (It used to fail every Dependabot pull request: the measure step +stopped before it set up the tool the comparison runs.) + Data protection. Only aggregates may leave the job. The certified dataset is licensed survey microdata. The measurement keeps household-level detail for the decile comparison under ``private/``; these tests fail if the @@ -27,7 +32,9 @@ import json import os import re +import shutil import subprocess +import sys import tempfile from pathlib import Path @@ -359,3 +366,46 @@ def test_no_commit_of_the_base_branch_counts_as_the_pull_requests(before, events assert process.returncode == 0, process.stdout + process.stderr assert outputs == {"sha": main} assert origin.attributed(outputs["sha"], merge) == own | {merge} + + +# Runs with no data access. + + +def test_a_dependabot_run_is_reported_as_skipped_and_passes(tmp_path): + """Dependabot gets no secrets, so nothing is measured, and the job passes. + + Runs the measure and compare steps as a Dependabot run does, with the + head's own tool and a Python that can import its dependencies. + """ + head = tmp_path / "head" + shutil.copytree( + REPO / "policyengine_uk" / "utils" / "aggregate_impact", + head / "policyengine_uk" / "utils" / "aggregate_impact", + ) + python = head / ".venv" / "bin" / "python" + python.parent.mkdir(parents=True) + python.write_text(f'#!/bin/sh\nexec "{sys.executable}" "$@"\n') + python.chmod(0o755) + (tmp_path / "runner_temp").mkdir() + runner = { + "RUNNER_TEMP": str(tmp_path / "runner_temp"), + "GITHUB_STEP_SUMMARY": str(tmp_path / "step_summary.md"), + "GITHUB_SERVER_URL": "https://github.com", + "GITHUB_REPOSITORY": "PolicyEngine/policyengine-uk", + "GITHUB_RUN_ID": "1", + } + + measured, _ = run_step( + "Measure base, then head", tmp_path, ACTOR="dependabot[bot]", **runner + ) + assert measured.returncode == 0, measured.stdout + measured.stderr + assert "Dependabot runs have no data access" in measured.stdout + + compared, outputs = run_step("Compare base with head", tmp_path, **runner) + assert compared.returncode == 0, compared.stdout + compared.stderr + assert outputs["status"] == "0", compared.stdout + compared.stderr + comment = (tmp_path / "report" / "comment.md").read_text() + assert "## Aggregate impact on certified data: skipped" in comment + assert step_named("Fail on a structural failure")["if"] == ( + "steps.compare.outputs.status != '0'" + ) From 050f52f484379f518e8c75d52a40873f17622bd9 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 25 Oct 2026 06:32:51 -0400 Subject: [PATCH 2/2] Test that only Dependabot may skip, and drop the fragment's list marker The review of #2665 found the second half of the invariant untested: any other same-repository run with no token still fails. The new test runs the measure step as a maintainer with no token and checks it fails with the token error and writes no result. It passes on main and with the fix. Towncrier adds the bullet itself, so the fragment no longer starts with one. Co-Authored-By: Claude Opus 5.5 --- .../aggregate-impact-dependabot-skip.fixed.md | 2 +- .../test_aggregate_impact_workflow.py | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/changelog.d/aggregate-impact-dependabot-skip.fixed.md b/changelog.d/aggregate-impact-dependabot-skip.fixed.md index cd5dd6d49d..1d7d76d47d 100644 --- a/changelog.d/aggregate-impact-dependabot-skip.fixed.md +++ b/changelog.d/aggregate-impact-dependabot-skip.fixed.md @@ -1 +1 @@ -- The Aggregate impact report now reports a Dependabot pull request as skipped, as it was meant to, instead of failing it. Dependabot runs have no data access, and the measure step stopped before it set up the tool the comparison runs. +The Aggregate impact report now reports a Dependabot pull request as skipped, as it was meant to, instead of failing it. Dependabot runs have no data access, and the measure step stopped before it set up the tool the comparison runs. diff --git a/policyengine_uk/tests/code_health/test_aggregate_impact_workflow.py b/policyengine_uk/tests/code_health/test_aggregate_impact_workflow.py index 2806669476..86d57efe6d 100644 --- a/policyengine_uk/tests/code_health/test_aggregate_impact_workflow.py +++ b/policyengine_uk/tests/code_health/test_aggregate_impact_workflow.py @@ -409,3 +409,21 @@ def test_a_dependabot_run_is_reported_as_skipped_and_passes(tmp_path): assert step_named("Fail on a structural failure")["if"] == ( "steps.compare.outputs.status != '0'" ) + + +def test_only_dependabot_may_skip_a_run_with_no_data_access(tmp_path): + """Any other run with no token fails the job instead of skipping.""" + shutil.copytree( + REPO / "policyengine_uk" / "utils" / "aggregate_impact", + tmp_path / "head" / "policyengine_uk" / "utils" / "aggregate_impact", + ) + (tmp_path / "runner_temp").mkdir() + measured, _ = run_step( + "Measure base, then head", + tmp_path, + ACTOR="a-maintainer", + RUNNER_TEMP=str(tmp_path / "runner_temp"), + ) + assert measured.returncode != 0 + assert "::error::HUGGING_FACE_TOKEN is not available" in measured.stdout + assert not (tmp_path / "measured" / "head" / "result.json").exists()