Skip to content

Extend the release tests' PyPI scan and Test-job guard - #649

Merged
MaxGhenis merged 2 commits into
masterfrom
claude/core-648-review-followups-a5en9f
Nov 3, 2026
Merged

MaxGhenis merged 2 commits into
masterfrom
claude/core-648-review-followups-a5en9f

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Follow-ups from the independent review of #648 (landed as 28ac7aa852): #648 (comment), items P3 1 to P3 5. The PR changes only tests/core/test_release_tagging.py and adds changelog.d/core648-release-test-followups.changed.md. Nothing under policyengine_core/ or .github/ changes.

What's changed

  • More PyPI publishers (P3 1). PYPI_UPLOAD_COMMAND now holds the review's regex, \b(twine\s+upload|(uv|poetry|flit|hatch|pdm|maturin)\s+publish)\b. test_only_push_publish_job_uploads_to_pypi now also fails when a recipe line in the root Makefile matches it (pypi_uploading_recipe_lines). The copy test gains poetry publish and maturin publish cases. On master the scan still returns [("push.yaml", "Publish")].
  • Remote reusable workflows and composite actions (P3 2). The check fails on any job-level uses: that doesn't start with ./, because that job runs another repository's workflow, whose steps the scan can't read (remote_reusable_workflow_jobs). Local reusable workflows sit in .github/workflows/, and the scan already reads them as files. The steps of every action.yml and action.yaml under .github/actions/ (runs.steps) go through uploads_to_pypi (pypi_publishing_actions). A missing folder passes, as on master.
  • copytree (P3 3). The copy test uses shutil.copytree(WORKFLOWS, tmp_path, dirs_exist_ok=True), so a subdirectory under .github/workflows/ no longer crashes it.
  • Exact-match message and locator (P3 4). The make test exact match now uses the review's message, which says what to do. The locator finds the test step by step.get("run", "").split()[:2] == ["make", "test"], so it no longer takes a separate make test-country-template step for it. A new in-suite test covers that case.
  • More masks (P3 5). assert_test_job_fails_when_tests_fail now takes the whole workflow. It fails if the test step, the Test job's defaults.run or the workflow's defaults.run sets shell, or if the env of the workflow, the Test job or the test step sets PYTEST_ADDOPTS. A new assert_make_test_fails_when_tests_fail fails if a recipe line of make test, or of a prerequisite that its rule line names, has a prefix that contains - (any mix of @, + and -). Each new check has in-suite mutant tests.

What the checks still can't see

  • An upload inside a script or program that a workflow step or recipe calls, for example bash release.sh or python upload.py.
  • An upload command in another form, such as uv --verbose publish (an option between the tool and publish), $TOOL publish, a curl to the upload API, or a publisher not in the regex.
  • An upload in a remote action that a step uses (uses: owner/action@ref), other than pypa/gh-action-pypi-publish. The scan also skips a local action outside .github/actions/ (uses: ./some/dir). Under .github/actions/, it reads only the runs.steps of composite actions, so it skips Docker and JavaScript actions (runs.using: docker or node*).
  • Makefiles other than the root one, included makefiles, line continuations, the .IGNORE special target, -i in MAKEFLAGS (whether the workflow, job or step env sets it or the Makefile adds it with MAKEFLAGS += -i), and a make test recipe line that masks its own failure (… || true, …; true). The recipe parser targets this repository's Makefile. The checks catch a make -i test call in the test step itself, because the step must run exactly make test.
  • PYTEST_ADDOPTS or another option that comes from elsewhere: an earlier step that writes it to $GITHUB_ENV, pyproject.toml, pytest.ini or conftest.py, or a different variable.
  • Workflow files in subdirectories of .github/workflows/, which GitHub doesn't run (the review quotes its docs).
  • PyPI's trusted-publisher configuration, which these tests can't read.

Tests

One file per process, uv run pytest -q -p no:cacheprovider <file>:

test_release_tagging.py test_docs_deployment.py
master c86af926a6 25 passed 5 passed
this PR 42 passed 5 passed

ruff format --check and ruff check pass on the changed test file. The hand-off comment gives the mutant results for M6–M15, X1–X16 and the new mutants. At head 38c72db4a0, all 19 CI checks in run 41714952193 passed: lint, the workflow lint, the changelog check, and the tests on Ubuntu and Windows with Python 3.11 to 3.14.


🤖 Generated with Claude Code

The independent review of #648 found uploads the scan missed (other
publishers, Makefile recipes, remote reusable workflows, composite
actions), a crash on a subdirectory under .github/workflows, a test-step
locator that also matched make test-country-template, and masks the
Test-job guard missed (a custom shell, PYTEST_ADDOPTS, a - prefix on a
make test recipe line).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LwQq6JfvPczG14bBktQcFP
This PR changes only the release tests and its changelog fragment.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LwQq6JfvPczG14bBktQcFP
@MaxGhenis

Copy link
Copy Markdown
Contributor Author

Ready for hub review

Head: 38c72db4a00345c0862f6cb8ac5bb0bf472e3c42 (base master c86af926a6). It's a draft, and only tests/core/test_release_tagging.py and changelog.d/core648-release-test-followups.changed.md change. The second commit puts back uv.lock, which a local uv sync had changed in the first.

Items

  1. More PyPI publishers (review P3 1): done.
    • The regex is now the review's (tests/core/test_release_tagging.py:347-349).
    • The review's comment line, edited to cover what the checks now scan, sits above the assertions at :440-444.
    • poetry-publish and maturin-publish cases are added to the copy test (:467-468, :473-474).
    • Makefile recipes are scanned by make_rules (:408-425) and pypi_uploading_recipe_lines (:428-434), asserted at :450-452, with an in-suite test at :521.
    • X8–X13 now fail (table below).
    • On master the scan returns [('push.yaml', 'Publish')]. The remote-workflow, composite-action and Makefile scans each return [].
  2. Remote reusable workflows and composite actions (P3 2): done.
    • remote_reusable_workflow_jobs (:376-386) flags any job-level uses: that doesn't start with ./.
    • pypi_publishing_actions (:389-405) runs uploads_to_pypi on runs.steps of every action.yml/action.yaml found recursively under .github/actions/. A missing folder returns [].
    • They are asserted at :446-448 and :449.
    • In-suite tests are at :491 (one remote job and one local job, and only the remote one is flagged) and :502 (both file names, plus the missing folder).
    • Mutants N1–N3 are below.
  3. copytree (P3 3): done (:456, in the shared helper copy_workflows_with_pr_test_job_change). N5 (a copy of pr.yaml at .github/workflows/sub/x.yaml) gives 42 passed, against 3 failed on master's test file. X14 shows the same.
  4. Message and locator (P3 4): done.
    • The review's message is at :553-557.
    • The locator is now split()[:2] == ["make", "test"] (:546).
    • Mutant N6 inserts a make test-country-template step before the test step in push.yaml. It gives 42 passed, so the guard still finds exactly one test step. Master's test file gives 3 failed.
    • The same case is now an in-suite test (:671).
  5. More masks (P3 5): done, all three.
    • assert_test_job_fails_when_tests_fail now takes the whole workflow (:533).
    • It fails on shell in the step, the job's defaults.run or the workflow's defaults.run (:562-569).
    • It fails on PYTEST_ADDOPTS in the env of the workflow, the job or the step (:572-575).
    • assert_make_test_fails_when_tests_fail (:578-587) fails if a recipe line of test, or of a prerequisite on its rule line, has a prefix that contains -. It's called from test_test_job_cannot_pass_with_failing_tests (:590-594).
    • There are 6 new in-suite mutant params (:654-659) and 4 Makefile prefix params (:681-698, test at :686).

Tests (one file per process, uv run pytest -q -p no:cacheprovider <file>)

test_release_tagging.py test_docs_deployment.py
master c86af926a6 25 passed 5 passed
head 38c72db4a0 42 passed 5 passed

ruff format --check and ruff check on the test file both pass.

Mutants

Each mutant was applied to the working tree, then both files ran, each in its own pytest process. The tree was restored after each with git checkout -- .github Makefile && git clean -fdq .github, and a script asserted that git status --porcelain .github Makefile was empty each time. The master column runs master's test file against the same mutant. M8's master count was re-run by hand. X2 isn't described in the review, so I didn't run it. Unless noted, test_docs_deployment.py gave 5 passed.

Mutant head killed at head by master test file
unmodified 42 passed — 25 passed
M6 job continue-on-error: true 10 failed, 32 passed test_test_job_cannot_pass_with_failing_tests (+ in-suite tests on the mutated file) 3 failed
M7 step continue-on-error: true 10 failed, 32 passed same 3 failed
M8 make test || true 11 failed, 31 passed same 4 failed
M9 step if: false 9 failed, 33 passed same 2 failed
M10 pypi action in pr.yaml Test 1 failed, 41 passed test_only_push_publish_job_uploads_to_pypi 1 failed
M11 step continue-on-error: ${{ true }} 10 failed, 32 passed test_test_job_cannot_pass_with_failing_tests 3 failed
M12 make test; true 11 failed, 31 passed same 4 failed
M13 step if: ${{ false }} 9 failed, 33 passed same 2 failed
M14 uv publish in new release.yml 6 failed, 36 passed (docs: 6 passed) test_only_push_publish_job_uploads_to_pypi + 5 copy cases 4 failed
M15 job uses: ./.github/workflows/publish.yaml, file missing 42 passed none (expected: local, nothing to read) 25 passed
X1 M15 + publish.yaml running uv publish 6 failed, 36 passed (docs: 6 passed) test_only_push_publish_job_uploads_to_pypi 4 failed
X3 multi-line make test then true 11 failed, 31 passed test_test_job_cannot_pass_with_failing_tests 4 failed
X4 job defaults.run.shell: bash {0} 5 failed, 37 passed same 25 passed
X5 step shell: bash -c true {0} 5 failed, 37 passed same 25 passed
X6 job env: PYTEST_ADDOPTS: --collect-only 2 failed, 40 passed same 25 passed
X7 - prefix on Makefile test: recipe 2 failed, 40 passed same 25 passed
X8 twine upload dist/* in Makefile build: 2 failed, 40 passed test_only_push_publish_job_uploads_to_pypi 25 passed
X9 poetry publish in pr.yaml Test 1 failed, 41 passed same 25 passed
X10 flit publish 1 failed, 41 passed same 25 passed
X11 hatch publish 1 failed, 41 passed same 25 passed
X12 pdm publish 1 failed, 41 passed same 25 passed
X13 maturin publish 1 failed, 41 passed same 25 passed
X14 uv publish in .github/workflows/sub/release.yaml 42 passed none (expected: GitHub doesn't run nested files) 3 failed (IsADirectoryError)
X15 SHA-pinned pypi action in pr.yaml Test 1 failed, 41 passed test_only_push_publish_job_uploads_to_pypi 1 failed
X16 python -m twine upload in new .yml 6 failed, 36 passed (docs: 6 passed) same 4 failed
N1 job uses: some-org/some-repo/.github/workflows/publish.yml@v1 2 failed, 40 passed test_only_push_publish_job_uploads_to_pypi (+ the in-suite remote test, which then sees two remote jobs) 25 passed
N2 .github/actions/publish/action.yml runs uv publish 1 failed, 41 passed test_only_push_publish_job_uploads_to_pypi 25 passed
N3 the same as action.yaml 1 failed, 41 passed same 25 passed
N4 composite action with no upload 42 passed none (expected) 25 passed
N5 subdirectory .github/workflows/sub/x.yaml 42 passed none (expected) 3 failed (IsADirectoryError)
N6 make test-country-template step before the test step 42 passed none (expected) 3 failed (expected one test step)
N7 workflow defaults.run.shell: bash -c true {0} 5 failed, 37 passed test_test_job_cannot_pass_with_failing_tests 25 passed
N8 step env: PYTEST_ADDOPTS 2 failed, 40 passed same 25 passed
N9 workflow env: PYTEST_ADDOPTS 2 failed, 40 passed same 25 passed
N10 @- prefix on test: recipe 2 failed, 40 passed same 25 passed
N11 - prefix on test-country-template: recipe 1 failed, 41 passed same 25 passed

The extra failures at head are the in-suite tests, which load the already-mutated push.yaml or Makefile, as in #648.

Deleting each new check from the test file (restored after each):

Check deleted Result Failing tests
shell 3 failed the three shell mutant params
PYTEST_ADDOPTS 3 failed the three env params
make - prefix 4 failed all four Makefile params
remote reusable workflow filter 1 failed test_reusable_workflow_check_finds_only_remote_workflows
composite action scan 2 failed both file names
Makefile upload scan 1 failed test_pypi_upload_check_finds_upload_in_makefile_recipe
new publishers removed from the regex 2 failed the poetry and maturin copy cases

Hand checks

  • GNU Make 4.3, recipe false then echo after:
    • prefixes -, @-, -@, +-, and a space before - all exit 0;
    • @ alone exits 2.
    • That is why the prefix check looks at the whole run of @+- and whitespace, not just the first character.
  • PYTEST_ADDOPTS=--collect-only pytest tests/core/test_docs_deployment.py prints 5 tests collected and exits 0.
  • Regex spot checks:
    • python -m uv publish, uv publish --token x and python -m twine upload match.
    • $TOOL publish and uv --verbose publish don't. Both are listed under the PR body's limits.
  • GitHub docs (source fetched from github/docs main on raw.githubusercontent.com):
    • "inserts the file name for the temporary script at {0}";
    • "you can specify a default shell for all run steps in a job, or for all run steps in the entire workflow".
    • The shell comment at :558-561 rests on these.

CI

The PR is a draft. At 38c72db4a0 the Pull request run https://github.com/PolicyEngine/policyengine-core/actions/runs/41714952193 was pending when I posted this, so it hasn't run.

Judgment calls

  • Branch. The PR is on claude/core-648-review-followups-a5en9f, not claude/core648-release-test-followups, because this session may push only to its assigned branch. Before starting, I checked that the brief's branch returns 404 and that no open PR covers this change. The fragment follows Guard the release tests against masked test failures and other PyPI uploads #648's convention and is named core648-release-test-followups.changed.md, not after the branch path, which contains a /.
  • X4 is now flagged. The review's table says bash {0} on the job doesn't mask a failure. The item asked to fail on any shell in those three places, and I did. It's conservative, and the new comment says why.
  • Makefile - prefix. The check covers the test: recipe and the recipes of the prerequisites on its rule line (test-country-template). It does this because a masked failure in a prerequisite is equally hidden. Master isn't flagged.
  • Composite actions are found recursively under .github/actions/, since actions usually sit at .github/actions/<name>/action.yml.
  • Remote reusable workflows are flagged by "job-level uses: not starting with ./", not by matching owner/repo/.github/workflows/... exactly. Any job-level uses: that isn't local is a remote workflow.
  • The four PyPI assertions stay in one test (test_only_push_publish_job_uploads_to_pypi), with messages that name which scan failed. Splitting them would show all failures at once, at the cost of more test functions.

Proposed follow-ups

  • Scan local actions referenced by path, and $GITHUB_ENV writes, in the release guards. A step uses: ./some/dir outside .github/actions/ isn't scanned. An earlier step can export PYTEST_ADDOPTS through $GITHUB_ENV, and the guard doesn't see it.
  • Guard pytest options set in config. addopts in pyproject.toml/pytest.ini, or a conftest.py hook, could stop tests running without touching the workflow. A check that make test collects more than zero tests, or that addopts holds no --collect-only, would cover it.

Generated by Claude Code

@MaxGhenis
MaxGhenis marked this pull request as ready for review November 3, 2026 21:05
@MaxGhenis

Copy link
Copy Markdown
Contributor Author

Hub review in progress

Reviewing head 38c72db4a00345c0862f6cb8ac5bb0bf472e3c42. Findings so far, to be replaced by the full review:

  • Tests: test_release_tagging.py 42 passed at head, 25 at master c86af926a6; test_docs_deployment.py 5 passed at both. Only the test file and the fragment differ from master; uv.lock is identical.
  • Hand-off mutants X8–X13, N1–N3 reproduce; X14, N4–N6 pass as expected.
  • Guard gaps confirmed by mutant (each gives 42 passed): || true on the Makefile test: recipe line, .IGNORE: test, .IGNORE:, MAKEFLAGS: -i in the Test job's env, MAKEFLAGS += -i in the Makefile, a PyO3/maturin-action upload step, a remote action step. Most are missing from the body's limits list.
  • CI run 41714952193 at the head: all 19 jobs succeeded.

Generated by Claude Code

@MaxGhenis

Copy link
Copy Markdown
Contributor Author

Ready for hub review
VERDICT: APPROVE
38c72db
METHODOLOGY_CHOICE: no

I reviewed head 38c72db4a00345c0862f6cb8ac5bb0bf472e3c42, which is still the PR head, so nothing changed after the hand-off. The base is master c86af926a6.

The PR does all five items in the build brief. Every hand-off mutant I reran gives the result the hand-off reports, except one count that my mutant changed (N1, explained below). CI is green. I found nothing at P1. One P2 is about the PR body: its "can't see" list leaves out several masks that I confirmed by mutant, and it lists one case that the guard does catch. The hub can fix that by editing the body. Everything else is P3. None of the gaps exists in the repository today, and the PR doesn't make any of them worse.

Assignment. Nothing the brief asked for is missing. The PR differs from the brief in two places, and the hand-off discloses both:

  • The branch is claude/core-648-review-followups-a5en9f, not claude/core648-release-test-followups.
  • The fragment is named core648-release-test-followups.changed.md, not <branch>.changed.md. The release tooling ignores the name (see "Checked and sound").

pulls/649/files lists only changelog.d/core648-release-test-followups.changed.md and tests/core/test_release_tagging.py. git diff --stat c86af926a6 38c72db4a0 shows the same 2 files, with nothing under policyengine_core/ or .github/. git diff --quiet c86af926a6 38c72db4a0 -- uv.lock exits 0, so uv.lock matches master byte for byte. The first commit a4d6dd29 changed it (4 lines), and the second commit reverts that.

P2

1. The body's "What the checks still can't see" list leaves out masks I confirmed, and lists one case that the guard catches (PR body, the section "What the checks still can't see").

Failure scenario: a maintainer reads the list as complete, then adds || true to the Makefile's test: recipe or sets MAKEFLAGS: -i on the Test job. The release tests stay green, and a failing test run can publish.

Command: each mutant was applied to the working tree, then I ran python -m pytest -q -p no:cacheprovider tests/core/test_release_tagging.py and restored the tree. Every one of these gives 42 passed at head:

  • R12: || true appended to the coverage run … pytest … line of Makefile's test: recipe;
  • R9: env: MAKEFLAGS: -i on push.yaml's Test job;
  • R14: MAKEFLAGS += -i at the top of the Makefile;
  • R8 and R13: .IGNORE: test, or a bare .IGNORE:, in the Makefile;
  • R7: a step uses: PyO3/maturin-action@v1 with with: command: upload;
  • R11: a step uses: some-org/publish-action@v1.

The body names R7, R11 ("remote action used at step level"), R8 and R13 (".IGNORE"). It doesn't name R9, R12 or R14. It also doesn't name a Docker or JavaScript action under .github/actions/, which the composite scan skips (see P3 2).

Two entries in the list need correcting:

  • It lists "a make -i / -k call" as a gap. A step run: make -i test or make -k test fails the guard: the exact match rejects it, or the locator finds no test step. In the GNU Make 4.3 probe below, -k alone doesn't mask a failure (exit 2).
  • The real gap is -i arriving through MAKEFLAGS.

GNU Make 4.3 probe. The recipe was false then echo after, run with make -s test, and the exit status was recorded:

plain:            exit=2
.IGNORE: test:    exit=0
.IGNORE: (none):  exit=0
MAKEFLAGS env -i: exit=0
MAKEFLAGS env i:  exit=0
MAKEFLAGS env -k: exit=2
MAKEFLAGS += -i:  exit=0
|| true in line:  exit=0
-@ prefix:        exit=0
+- prefix:        exit=0

Replacement text for the third bullet of that section:

  • Makefiles other than the root one, included makefiles, the .IGNORE special target, -i passed through MAKEFLAGS (set in the workflow's, job's or step's env, or with MAKEFLAGS += -i in the Makefile), and a make test recipe line that masks its own failure (… || true, …; true). The recipe parser is written for this repository's Makefile. A make -i test call in the test step itself is caught, because the step must run exactly make test.

Add to the second bullet:

A Docker or JavaScript action under .github/actions/ (runs.using: docker or node*) isn't scanned either: only the runs.steps of composite actions are.

P3

1. The guard could close the Makefile masks above cheaply (tests/core/test_release_tagging.py:578-587, :570-575). This is a recommendation for this PR or a follow-up. The hub decides. The workflow side already pins the step to the exact make test. The Makefile side checks only the - prefix. So R12 is the Makefile twin of the make test || true that the workflow guard exists to catch. Suggested additions, each of which would turn the mutant above into a failure:

    # In assert_make_test_fails_when_tests_fail, after the prefix loop:
    assert ".IGNORE" not in rules, "the Makefile ignores recipe failures (.IGNORE)"
    for target in ["test", *prerequisites]:
        for line in rules.get(target, ([], []))[1]:
            assert not re.search(r"\|\||;\s*(true|exit\s+0|:)\s*$", line), (
                f"make {target} masks a failing command: {line}"
            )
    # In assert_test_job_fails_when_tests_fail, beside the PYTEST_ADDOPTS check:
    assert not any(
        "MAKEFLAGS" in (item.get("env") or {}) for item in [workflow, test_job, test_step]
    ), "Test sets MAKEFLAGS"

For the Makefile itself, add assert not re.search(r"^\s*(export\s+)?MAKEFLAGS\b", MAKEFILE.read_text(), re.M). Master has none of these, and the test recipe's coverage xml -i matches none of the patterns. I didn't run these snippets.

2. Remote, Docker and JavaScript actions: a stronger check than more regexes (:376-405). Step-level remote actions are used throughout master: actions/checkout, actions/setup-python, astral-sh/setup-uv, actions/create-github-app-token, EndBug/add-and-commit, codecov/codecov-action, JamesIves/github-pages-deploy-action and docker://rhysd/actionlint. Today:

  • .github/actions/ doesn't exist.
  • No job has a job-level uses:.
  • No step uses a local action.

So none of these gaps applies today. The scan can't read inside a remote action, and an allowlist of actions would churn. According to the comment at push.yaml:120-121, trusted publishing limits uploads to push.yaml in the pypi environment. A test that only the Publish job declares environment: pypi and id-token: write would cover R7, R11 and Docker or JavaScript actions in a single check. No test in the file checks either today (grep -n "id-token\|environment" tests/core/test_release_tagging.py finds nothing). I couldn't check PyPI's side of the trusted-publisher setup. Recommend as a follow-up.

3. The scan has false positives on text that only mentions an upload, and misses a few spellings (:347-357, :428-434). Probe output from uploads_to_pypi and pypi_uploading_recipe_lines:

  • Flagged, although they upload nothing:
    • run: "# never run twine upload here";
    • run: "echo 'run uv publish after review'";
    • Makefile recipe lines \t# twine upload later and \techo uv publish.
  • A step's name: isn't read, so name: twine upload with run: make build passes, correctly.
  • Missed:
    • twine \⏎ upload (a line continuation between the words);
    • $TWINE upload;
    • uv --verbose publish;
    • PyPA/gh-action-pypi-publish@release/v1 (mixed-case owner; mutant R15 gives 42 passed). I didn't confirm that GitHub resolves a mixed-case owner.
  • Flagged, as it should be: twine upload --repository testpypi. It doesn't reach PyPI, but a test that fails loudly on it is the safe direction.

The false positives fail loudly, and none occurs on master, so I'd leave the regex alone. The test comment at :440-444 and the fragment claim no more than the code checks. If you want the comment exact, use this replacement:

    # Recognises the pypa publish action (lower-case owner), twine upload and
    # uv/poetry/flit/hatch/pdm/maturin publish written as adjacent words, in
    # workflow steps, composite action steps under .github/actions and root
    # Makefile recipes, and rejects reusable workflows from other repositories;
    # not uploads inside scripts, remote, Docker or JavaScript actions.

4. Locator: two cases fail without saying what to do (:543-549, only_step at :149-152). Results from assert_test_job_fails_when_tests_fail on push.yaml with the test step's run replaced:

run: One test step found? Message
make test -j4 yes the new exact-match message, which says to update the check
make test yes the same message. "runs more than make test" is slightly off for extra whitespace, but the instruction is right
make -C . test no ([]) expected one test step, found []
uv run make test no ([]) expected one test step, found []
make test-country-template inserted beside the test step (N6) yes, exactly one passes

All five are rejected or accepted correctly. For the two found [] cases, the message doesn't say what to do. Recommend only: pass a description such as "test (a step whose run starts with make test; if the command moved, update this locator and the exact match below)".

5. Failing on every shell is stricter than needed for an explicit bash, but I'd keep it (:558-569). shell: bash and shell: bash -e {0} both fail with test step sets a shell. I couldn't reach docs.github.com from this session (see "What I couldn't check"). The #648 review quotes the docs as saying:

  • the default Linux invocation is bash -e {0};
  • shell: bash runs bash --noprofile --norc -eo pipefail {0}.

If those quotes are right, an explicit bash is stricter than the default and masks nothing. Allowing exactly bash would save a maintainer one edit. The cost is a weaker rule and an exception to keep correct. The comment at :558-561 explains the rule, so a maintainer who hits it knows why. No change needed.

Mutants

Each mutant was a text edit on the working tree, made by a script. Each file ran in its own pytest process. After each mutant I ran git checkout -- .github Makefile && git clean -fdq .github, then checked that git status --porcelain -- .github Makefile was empty. It was empty every time, and git status was clean at the end. "Master test file" means master's test_release_tagging.py copied in place and run against the same mutant, then restored. test_docs_deployment.py gave 5 passed in every row, except R7 and R11, which gave 6 because they add a workflow file.

Mutant Head Killed at head by Master test file
unmodified 42 passed — 25 passed
X8 twine upload dist/* in Makefile build: 2 failed, 40 passed test_only_push_publish_job_uploads_to_pypi (+ the in-suite Makefile test, which reads the mutated Makefile) 25 passed
X9–X13 poetry/flit/hatch/pdm/maturin publish step in pr.yaml Test 1 failed, 41 passed each test_only_push_publish_job_uploads_to_pypi 25 passed each
X14 uv publish in .github/workflows/sub/release.yaml 42 passed none (expected: GitHub doesn't run nested files) 3 failed (copy test)
N1 job uses: some-org/some-repo/.github/workflows/publish.yml@v1 in pr.yaml 1 failed, 41 passed test_only_push_publish_job_uploads_to_pypi 25 passed
N2 .github/actions/publish/action.yml runs uv publish 1 failed, 41 passed same 25 passed
N3 the same as action.yaml 1 failed, 41 passed same 25 passed
N4 composite action with no upload 42 passed none (expected) 25 passed
N5 copy of pr.yaml at .github/workflows/sub/x.yaml 42 passed none (expected) 3 failed (copy test)
N6 make test-country-template step before push.yaml's test step 42 passed none (expected) 3 failed
R7 new workflow, step uses: PyO3/maturin-action@v1, with: command: upload 42 passed none (P2, P3 2) 25 passed
R8 .IGNORE: test in Makefile 42 passed none (P2, P3 1) 25 passed
R9 MAKEFLAGS: -i in the Test job's env 42 passed none (P2, P3 1) 25 passed
R10 run: make test || true on push.yaml's test step 11 failed, 31 passed test_test_job_cannot_pass_with_failing_tests (+ 10 in-suite tests that load the mutated file) 4 failed
R11 new workflow, step uses: some-org/publish-action@v1 42 passed none (P2, P3 2) 25 passed
R12 || true on the Makefile test: pytest line 42 passed none (P2, P3 1) 25 passed
R13 bare .IGNORE: in Makefile 42 passed none (P2, P3 1) 25 passed
R14 MAKEFLAGS += -i in Makefile 42 passed none (P2, P3 1) 25 passed
R15 step uses: PyPA/gh-action-pypi-publish@release/v1 in pr.yaml Test 42 passed none (P3 3) 25 passed
R16 step run: uv --verbose publish in pr.yaml Test 42 passed none (listed in the body) 25 passed

The brief's N7–N11 are R7–R11 here, renamed because the hand-off already uses N7–N11 for other mutants. On N1 the hand-off reports 2 failed and I get 1. My mutant named its job Remote, and the in-suite test_reusable_workflow_check_finds_only_remote_workflows writes a job with the same key into its copy, which overwrote mine. A differently named job would also fail that test. This changes the count, not the result.

The brief also asked about the copytree change. A scratch subdirectory under .github/workflows/ no longer crashes the tests (X14, N5: 42 passed, against 3 failed with master's file). Copying the subdirectory doesn't hide an upload that GitHub would run. load_workflows (:360-364) globs only the top level, both before and after the copy, and X14's nested upload isn't flagged. That matches the #648 review's quote that GitHub doesn't support subdirectories of workflows, which I couldn't re-read here.

Checked and sound

  • Tests, one file per process (python -m pytest -q -p no:cacheprovider <file>):

    test_release_tagging.py test_docs_deployment.py
    master c86af926a6 25 passed 5 passed
    head 38c72db4a0 42 passed 5 passed

    These match the hand-off. ruff format --check and ruff check (ruff 0.17.3) pass on the test file.

  • Regex (item 1). It is the brief's regex verbatim (:347-349). Probe of uploads_to_pypi:

    • Flagged: pypa/gh-action-pypi-publish at a branch ref and at a SHA ref, twine upload, python -m twine upload, uvx twine upload, uv publish (also with a continued line after it), poetry, flit, hatch, pdm and maturin publish.
    • Not flagged: twine check, uv build, gh release upload.
    • Where each runs:
      • workflow run: (pypi_publishing_jobs);
      • composite action runs.steps (pypi_publishing_actions), including a step that uses: pypa/gh-action-pypi-publish@…;
      • Makefile recipe lines (pypi_uploading_recipe_lines), which use the regex only, as they should, since an action can't appear there.
    • On master, the four scans return [("push.yaml", "Publish")], [], [] and [] (the unmodified run passes test_only_push_publish_job_uploads_to_pypi).
  • Reusable workflows (item 2).

    • In a scratch folder, a job uses: o/r/.github/workflows/p.yml@v1 beside a job uses: ./.github/workflows/b.yaml, where b.yaml runs uv publish:
      • remote_reusable_workflow_jobs returns [('a.yaml', 'R')];
      • pypi_publishing_jobs returns [('b.yaml', 'P')].
    • So the local file's jobs are read, as the Guard the release tests against masked test failures and other PyPI uploads #648 review's X1 says.
    • A composite action whose step uses: a remote action (PyO3/maturin-action@v1, command: upload) isn't followed. A docker or node20 action has no runs.steps and isn't scanned. Neither exists today (P3 2).
  • Masks (item 5).

    • On master's push.yaml, the shell and PYTEST_ADDOPTS checks pass. push.yaml sets no defaults, no shell and no env on the workflow, Test or the test step. Only pr.yaml's smoke job sets env, and the guard doesn't read pr.yaml.
    • The - prefix check catches -, @- and +- on test: and on its rule-line prerequisite test-country-template (in-suite params, all pass). The GNU Make probe confirms each of those prefixes masks a failure.
    • It doesn't catch .IGNORE or MAKEFLAGS (R8, R9, R13, R14).
  • Changelog fragment.

    • changed is a configured towncrier type (pyproject.toml:93-96). docs/engineering/skills/github-prs.md:50-52 files tooling under changed, and the format there is changelog.d/<short-slug>.<type>.md, not the branch name.
    • The name has no effect on the release:
      • .github/bump_version.py:20-38 reads only the suffix and the stem's last dotted part, so this is a patch bump;
      • pr.yaml's "Check changelog fragment" job only counts files in changelog.d/.
    • The text names exactly what the checks catch:
      • the five added publishers alongside the action, twine upload and uv publish;
      • root Makefile recipes;
      • composite action steps under .github/actions/;
      • remote reusable workflows;
      • shell in the three places;
      • PYTEST_ADDOPTS in the three envs;
      • the - prefix on make test and its rule-line prerequisites.
    • It claims nothing the code doesn't check.
  • Scope. The search repo:PolicyEngine/policyengine-core is:pr is:open test_release_tagging returns 0 results. Of the open non-draft PRs, only Test policyengine-us main against the Core under test on each PR #635 touches .github/ (pr.yaml, country_main_tests.py, a requirements file), and none touches this PR's files. Running this PR's scans against Test policyengine-us main against the Core under test on each PR #635's head pr.yaml (00167396f4) gives [('push.yaml', 'Publish')] and no remote reusable jobs, so landing both in either order won't trip the new checks.

  • CI. Pull request run 41714952193 at 38c72db4a00345c0862f6cb8ac5bb0bf472e3c42 is completed / success, and it is the only run at that SHA. All 19 jobs are success: Lint, Lint workflows, Check changelog fragment, 8 Test jobs (ubuntu and windows, Python 3.11–3.14) and 8 "Test Core and country packages" jobs.

What I couldn't check

  • The documentation. docs.github.com and www.gnu.org don't resolve from this session's network, and the image has no GNU make manual (/usr/share/info holds only automake). So I quote neither.
  • Whether GitHub resolves a mixed-case action owner such as PyPA/ (P3 3).
  • PyPI's trusted-publisher configuration, which P3 2's suggested environment: pypi check relies on.
  • The suggested code in P3 1, which I didn't run.

Generated by Claude Code

@MaxGhenis
MaxGhenis merged commit d9da48a into master Nov 3, 2026
19 checks passed
@MaxGhenis

Copy link
Copy Markdown
Contributor Author

US + core hub merge audit, core#649 at 38c72db4a00345c0862f6cb8ac5bb0bf472e3c42 (squash): Extend the release tests' PyPI scan and Test-job guard.

  • Review: an independent review in a separate Claude Code session approved this head with one P2, five P3 findings and no P1: Extend the release tests' PyPI scan and Test-job guard #649 (comment)
    • Its reruns of the hand-off's mutants gave the hand-off's results, apart from one count that its own mutant changed (N1).
    • Its mutants R7 to R9 and R11 to R14 passed the new tests, and the description now lists each of those gaps. R15, a step that uses a mixed-case PyPA/ owner, also passed; the review couldn't confirm that GitHub resolves that owner.
  • Change after the review: none to the code. The hub edited the description (P2). The list of what the checks can't see now adds -i in MAKEFLAGS, a make test recipe line that masks its own failure, and Docker and JavaScript actions under .github/actions/. It drops "a make -i / -k call", since the guard rejects a make -i test or make -k test step. The Tests section now reports CI at the head.
  • Follow-ups for a later core PR (the review's P3s):
    • P3 1: fail on .IGNORE, on MAKEFLAGS in the env of the workflow, the Test job or the test step or in the Makefile, and on a make test recipe line that masks its own failure (… || true, …; true);
    • P3 2: check that only the Publish job declares environment: pypi and id-token: write, one test that covers remote, Docker and JavaScript actions;
    • P3 3: make the scan's comment list what it recognizes and what it misses, and leave the regex as it is;
    • P3 4: give the test-step locator a message that says what to do when it finds no step.
    • P3 5 needs no change.
  • Classification: CORE_SEMANTICS no. The PR changes release tests and the changelog, not package code.
  • PE-US core gate: the hub skipped it, since no package code changes.
  • Gates the lander checked at merge:
    • every check passes at the head;
    • MERGEABLE;
    • not a draft;
    • no CHANGES_REQUESTED review;
    • the merge pins the head (--match-head-commit).
  • Axiom: n/a; the PR changes no policy.

🤖 Generated with Claude Code

@MaxGhenis
MaxGhenis deleted the claude/core-648-review-followups-a5en9f branch November 3, 2026 22:43
MaxGhenis added a commit that referenced this pull request Nov 5, 2026
…650)

* Close Makefile masks and limit PyPI credentials in the release tests

The independent review of #649 confirmed that .IGNORE, MAKEFLAGS (in the
workflow env or the Makefile) and a `|| true`-style recipe line each let
make test pass after a failure without any release test noticing, and that
remote actions in other jobs could upload unseen. The make checks now reject
those forms, and a new check keeps the pypi environment and id-token: write
on push.yaml's Publish job alone, which covers uploads the text scan cannot
read when they rely on trusted publishing. The scan comment and the test
step locator's failure message now say what they cover and what to do.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Walk make test's whole prerequisite tree and reject Makefile settings that mask failures

The review of #650 found that an indented .IGNORE:, override MAKEFLAGS,
$(eval MAKEFLAGS ...), .ONESHELL, SHELL/.SHELLFLAGS, a prerequisite's
prerequisite, a later rule line for the same target and a double-colon rule
each let make test pass after pytest fails while the release tests stayed
green. Take the review's guard and tighten it where GNU Make 4.3 probes showed
further masks: any mention of .IGNORE, .ONESHELL or MAKEFLAGS (which also
covers GNUMAKEFLAGS and `foo .IGNORE:`), a target-specific SHELL, a later rule
line indented with spaces or naming other targets too, a recipe after `;` on
the rule line, and order-only prerequisites. Apply the one-rule-line check to
every Makefile target in the PyPI upload scan too, so that `build::` or a later
`build:` line cannot hide a twine upload from it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Narrow the changelog fragment to the Makefile forms the checks now catch

The review found the fragment named forms the checks missed (an indented
.IGNORE: and override MAKEFLAGS). With the wider guard, name exactly what it
rejects: any mention of .IGNORE, .ONESHELL or MAKEFLAGS, a SHELL or
.SHELLFLAGS assignment, a target without exactly one plain single-colon rule
line, and the masked recipe forms at any prerequisite depth.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Let make_rules read past recipe comments and refuse syntax it can't follow

A comment-only line, a conditional or .RECIPEPREFIX inside a recipe ended
make_rules's recipe, so later recipe lines (an `|| true` or a twine upload)
went unread while GNU make still ran them. make_rules now skips comment-only
lines, as make does, and fails on conditionals, define blocks, .RECIPEPREFIX
and trailing backslashes. Adds in-suite cases for each form.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Name the parser's refused syntax and comment handling in the fragment

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Let make_rules refuse a directive after any whitespace, as make reads it

GNU make skips any whitespace before a conditional or define, not only
spaces, so a directive after a space and a tab, after a form feed, or after
a tab outside a rule passed every check while make obeyed it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Fail the make test check on any mention of PYTEST_ADDOPTS in the Makefile

make exports a variable named in an export line to the recipes it runs, and
pytest reads options from PYTEST_ADDOPTS, so export PYTEST_ADDOPTS =
--collect-only let make test pass without running a test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Name PYTEST_ADDOPTS among the Makefile mentions the fragment lists

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Fail make_rules on line breaks that Python sees and make doesn't

Python's splitlines breaks a line at a form feed, vertical tab, lone
carriage return, \x1c-\x1e, \x85, \u2028 and \u2029, but GNU make ends a
line only at \n. A recipe line such as `cmd $(if ,<form feed>)|| true`
therefore reached make_rules as two lines, hiding its `|| true` or
upload. make_rules now refuses those characters, reading raw bytes so a
lone \r stays visible and allowing \r\n for CRLF checkouts on Windows.

The docstring and assertion message now also name the directive-word
recipe line that make_rules already refuses.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Name non-newline line breaks in the fragment's parser clause

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Write U+2028 and U+2029 as escapes in the line-break pattern

The previous commit put the raw U+2028 and U+2029 characters into the
pattern and dropped U+2029 from its comment. Escapes keep the source
ASCII and readable; the pattern matches the same nine characters.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants