Repository navigation
Extend the release tests' PyPI scan and Test-job guard - #649
Conversation
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
|
Ready for hub review Head: Items
Tests (one file per process,
|
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
falsethenecho 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.
- prefixes
PYTEST_ADDOPTS=--collect-only pytest tests/core/test_docs_deployment.pyprints5 tests collectedand exits 0.- Regex spot checks:
python -m uv publish,uv publish --token xandpython -m twine uploadmatch.$TOOL publishanduv --verbose publishdon't. Both are listed under the PR body's limits.
- GitHub docs (source fetched from
github/docsmainon raw.githubusercontent.com):- "inserts the file name for the temporary script at
{0}"; - "you can specify a default shell for all
runsteps in a job, or for allrunsteps in the entire workflow". - The shell comment at
:558-561rests on these.
- "inserts the file name for the temporary script at
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, notclaude/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 namedcore648-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 anyshellin those three places, and I did. It's conservative, and the new comment says why. - Makefile
-prefix. The check covers thetest: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 matchingowner/repo/.github/workflows/...exactly. Any job-leveluses: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_ENVwrites, in the release guards. A stepuses: ./some/diroutside.github/actions/isn't scanned. An earlier step can exportPYTEST_ADDOPTSthrough$GITHUB_ENV, and the guard doesn't see it. - Guard pytest options set in config.
addoptsinpyproject.toml/pytest.ini, or aconftest.pyhook, could stop tests running without touching the workflow. A check thatmake testcollects more than zero tests, or thataddoptsholds no--collect-only, would cover it.
Generated by Claude Code
|
Hub review in progress Reviewing head
Generated by Claude Code |
|
Ready for hub review I reviewed head 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:
P21. 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 Command: each mutant was applied to the working tree, then I ran
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 Two entries in the list need correcting:
GNU Make 4.3 probe. The recipe was Replacement text for the third bullet of that section:
Add to the second bullet:
P31. The guard could close the Makefile masks above cheaply ( # 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 2. Remote, Docker and JavaScript actions: a stronger check than more regexes (
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 3. The scan has false positives on text that only mentions an upload, and misses a few spellings (
The false positives fail loudly, and none occurs on master, so I'd leave the regex alone. The test comment at # 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 (
All five are rejected or accepted correctly. For the two 5. Failing on every
If those quotes are right, an explicit MutantsEach 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
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 The brief also asked about the Checked and sound
What I couldn't check
Generated by Claude Code |
|
US + core hub merge audit, core#649 at
🤖 Generated with Claude Code |
…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>
Follow-ups from the independent review of #648 (landed as
28ac7aa852): #648 (comment), items P3 1 to P3 5. The PR changes onlytests/core/test_release_tagging.pyand addschangelog.d/core648-release-test-followups.changed.md. Nothing underpolicyengine_core/or.github/changes.What's changed
PYPI_UPLOAD_COMMANDnow 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_pypinow also fails when a recipe line in the rootMakefilematches it (pypi_uploading_recipe_lines). The copy test gainspoetry publishandmaturin publishcases. On master the scan still returns[("push.yaml", "Publish")].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 everyaction.ymlandaction.yamlunder.github/actions/(runs.steps) go throughuploads_to_pypi(pypi_publishing_actions). A missing folder passes, as on master.copytree(P3 3). The copy test usesshutil.copytree(WORKFLOWS, tmp_path, dirs_exist_ok=True), so a subdirectory under.github/workflows/no longer crashes it.make testexact match now uses the review's message, which says what to do. The locator finds the test step bystep.get("run", "").split()[:2] == ["make", "test"], so it no longer takes a separatemake test-country-templatestep for it. A new in-suite test covers that case.assert_test_job_fails_when_tests_failnow takes the whole workflow. It fails if the test step, the Test job'sdefaults.runor the workflow'sdefaults.runsetsshell, or if theenvof the workflow, the Test job or the test step setsPYTEST_ADDOPTS. A newassert_make_test_fails_when_tests_failfails if a recipe line ofmake 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
bash release.shorpython upload.py.uv --verbose publish(an option between the tool andpublish),$TOOL publish, acurlto the upload API, or a publisher not in the regex.uses: owner/action@ref), other thanpypa/gh-action-pypi-publish. The scan also skips a local action outside.github/actions/(uses: ./some/dir). Under.github/actions/, it reads only theruns.stepsof composite actions, so it skips Docker and JavaScript actions (runs.using: dockerornode*).included makefiles, line continuations, the.IGNOREspecial target,-iinMAKEFLAGS(whether the workflow, job or stepenvsets it or theMakefileadds it withMAKEFLAGS += -i), and amake testrecipe line that masks its own failure (… || true,…; true). The recipe parser targets this repository's Makefile. The checks catch amake -i testcall in the test step itself, because the step must run exactlymake test.PYTEST_ADDOPTSor another option that comes from elsewhere: an earlier step that writes it to$GITHUB_ENV,pyproject.toml,pytest.iniorconftest.py, or a different variable..github/workflows/, which GitHub doesn't run (the review quotes its docs).Tests
One file per process,
uv run pytest -q -p no:cacheprovider <file>:test_release_tagging.pytest_docs_deployment.pyc86af926a6ruff format --checkandruff checkpass on the changed test file. The hand-off comment gives the mutant results for M6–M15, X1–X16 and the new mutants. At head38c72db4a0, 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