Repository navigation
Close Makefile masks and limit PyPI credentials in the release tests - #650
Conversation
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> Claude-Session: https://claude.ai/code/session_01LwQq6JEB5vwPLRB2PWBmRa
|
Ready for hub review Follow-ups from the #649 review (#649 (comment)), P3 1 to P3 4. Base: master Items
Tests (one file per process,
|
test_release_tagging.py |
test_docs_deployment.py |
|
|---|---|---|
master eaef9c2, before |
42 passed | 5 passed |
head 3acba61, after |
65 passed | 5 passed |
ruff format --check and ruff check pass on the test file.
Mutants
A script applied each mutant to the working tree. Workflow mutants were YAML round-trips, Makefile mutants were text edits, and R7/R11/A-new used a new workflow release-extra.yaml. For each mutant I ran the head file, test_docs_deployment.py and master's test_release_tagging.py in separate pytest processes, then ran git checkout -- .github Makefile tests/core/test_release_tagging.py && git clean -fdq .github and asserted that git status --porcelain was empty for those paths. The tree was clean at the end.
| Mutant | Head | Killed by (head) | docs | Master test file |
|---|---|---|---|---|
| unmodified | 65 passed | — | 5 passed | 42 passed |
R7 PyO3/maturin-action@v1 command: upload, new workflow, no creds |
65 passed | none (expected: no trusted-publisher token) | 6 passed | 42 passed |
R7 + environment: pypi + id-token: write |
7 failed, 58 passed | test_only_publish_job_gets_pypi_credentials (+ 6 in-suite credential tests that copy the mutated dir) |
6 passed | 42 passed |
R8 .IGNORE: test |
14 failed, 51 passed | test_test_job_cannot_pass_with_failing_tests (+ in-suite Makefile tests that read the mutated file) |
5 passed | 42 passed |
R9 MAKEFLAGS: -i on Test job env |
2 failed, 63 passed | test_test_job_cannot_pass_with_failing_tests, …accepts_separate_template_test_step |
5 passed | 42 passed |
R10 make test || true |
16 failed, 49 passed | test_test_job_cannot_pass_with_failing_tests (+ in-suite) |
5 passed | 11 failed, 31 passed |
R11 some-org/publish-action@v1, no creds |
65 passed | none (expected) | 6 passed | 42 passed |
R11 + environment: pypi + id-token: write |
7 failed, 58 passed | test_only_publish_job_gets_pypi_credentials (+ in-suite) |
6 passed | 42 passed |
R12 || true on make test's pytest line |
3 failed, 62 passed | test_test_job_cannot_pass_with_failing_tests (+ in-suite) |
5 passed | 42 passed |
R13 bare .IGNORE: |
14 failed, 51 passed | same as R8 | 5 passed | 42 passed |
R14 MAKEFLAGS += -i |
11 failed, 54 passed | test_test_job_cannot_pass_with_failing_tests (+ in-suite) |
5 passed | 42 passed |
R15 PyPA/gh-action-pypi-publish step in pr.yaml Test |
65 passed | none (out of scope; listed in body) | 5 passed | 42 passed |
R16 uv --verbose publish step in pr.yaml Test |
65 passed | none (out of scope; listed in body) | 5 passed | 42 passed |
A1 MAKEFLAGS: -i on test step env |
2 failed, 63 passed | test_test_job_cannot_pass_with_failing_tests (+1) |
5 passed | 42 passed |
A2 MAKEFLAGS: -i on push.yaml workflow env |
2 failed, 63 passed | same | 5 passed | 42 passed |
A3 export MAKEFLAGS = -i |
11 failed, 54 passed | test_test_job_cannot_pass_with_failing_tests (+ in-suite) |
5 passed | 42 passed |
A4 ; true on pytest line |
3 failed, 62 passed | same | 5 passed | 42 passed |
A5 ; exit 0 on coverage xml -i |
3 failed, 62 passed | same | 5 passed | 42 passed |
A6 ; : on pytest line |
3 failed, 62 passed | same | 5 passed | 42 passed |
A7 || true on test-country-template recipe |
1 failed, 64 passed | test_test_job_cannot_pass_with_failing_tests |
5 passed | 42 passed |
A8 environment: pypi on pr.yaml Test |
4 failed, 61 passed | test_only_publish_job_gets_pypi_credentials (+ in-suite) |
5 passed | 42 passed |
A9 permissions: {id-token: write} on pr.yaml Test |
4 failed, 61 passed | same | 5 passed | 42 passed |
A10 workflow-level permissions: write-all on pr.yaml |
7 failed, 58 passed | same | 5 passed | 42 passed |
A11 Publish drops environment: pypi |
7 failed, 58 passed | same | 5 passed | 42 passed |
A12 test step run: make -C . test |
16 failed, 49 passed | test_test_job_cannot_pass_with_failing_tests (message above) |
5 passed | 11 failed, 31 passed |
A13 test step run: uv run make test |
16 failed, 49 passed | same | 5 passed | 11 failed, 31 passed |
So R8, R9, R12, R13 and R14 now fail. R7 and R11 fail once their jobs add the environment and the permission.
GNU Make 4.3 probe (make -s test with a recipe line false, exit codes): plain 2; .IGNORE: test 0; .IGNORE: 0; env MAKEFLAGS=-i 0; MAKEFLAGS += -i 0; export MAKEFLAGS = -i 0; || true, || exit 0, ; true, ; exit 0, ; : 0 each; || true on a prerequisite's recipe 0; env GNUMAKEFLAGS=-i 0; false \ + || true 0 (the parser catches it); false; \ + true 0 (the parser misses it). Hand computation: coverage xml -i contains no || and doesn't end in ; true|exit 0|:, and no Makefile line starts with MAKEFLAGS or .IGNORE.
CI
The PR is a draft. Pull request run 41736909058 at 3acba61 was in_progress when I posted this: https://github.com/PolicyEngine/policyengine-core/actions/runs/41736909058. I haven't checked its result.
Judgment calls
- Branch. This session can push only to
claude/core-pr-649-followups-c3va8g, so I used that branch instead ofclaude/core649-release-test-followups(Extend the release tests' PyPI scan and Test-job guard #649 had the same situation). Before I pushed, neither branch existed (404), and no open PR mentionedtest_release_tagging. Base ismaster, notmain. - Where the
MakefileMAKEFLAGScheck lives. The brief put it inassert_test_job_fails_when_tests_fail. I put it inassert_make_test_fails_when_tests_fail, which now takes theMakefilepath instead of parsed rules, so each function reads one file. Both run intest_test_job_cannot_pass_with_failing_tests, so coverage is the same. - The credentials check is broader than "declares". It also counts
permissions: write-alland workflow-levelpermissionsthat a job without its own inherits (A10), plus anenvironmentmapping'snamecompared ignoring case. These are stricter. The check does not claim GitHub's or PyPI's semantics for them, because docs.github.com, docs.pypi.org, gnu.org and web.archive.org were unreachable from this session (403 or DNS failure). - The
MAKEFLAGSregex matches leading whitespace, so a tab-indented recipe line that startsMAKEFLAGS=…would also fail. That is a loud false positive and none exists on master. - Locator text. I wrote "starts with the words
make test" instead of the brief's "starts withmake test", because the locator compares split words, andmake test-country-templatestarts with that text but isn't matched. - No
axiom:line. The item doesn't name one.
Follow-ups I'd propose
- Reject
GNUMAKEFLAGSalongsideMAKEFLAGSin the release tests. GNU Make 4.3 ignores the failure with envGNUMAKEFLAGS=-i(exit 0 above). It's the same check with a second name. - Join
\-continued recipe lines before scanning.make_rulestreats each physical line separately, socmd; \+trueslips past the masking check. Joining continuations in the parser would close that gap.
Generated by Claude Code
|
Hub review in progress Head
The full review follows. Generated by Claude Code |
|
Ready for hub review I reviewed head The PR does the four items. Every hand-off mutant I reran (R7–R16, A1–A13) gives the hand-off's count, and CI is green. I found no P1. There are two P2s, both about claims:
A small code change, which I ran and give below, closes most of them. The other option is to narrow the fragment and the list. None of these gaps exists in the repository today. Assignment. The PR does everything the build brief asked for, apart from the gaps above. The branch is P21. The fragment says the tests fail on "a Failure scenario: someone adds
The brief asked the fragment to "name exactly the forms the new checks catch, and no more". Commands and output:
Fix: either apply P3 1's code, which closes both, or narrow the fragment. Replacement for the fragment's first clause:
The rest of the fragment is accurate. I checked each remaining clause against the code and against mutants A1–A13, N3 and N4. 2. The body's "What the checks still can't see" list leaves out Makefile masks I confirmed (PR body, fifth bullet). Failure scenario: a maintainer reads the list as complete and relies on the guard to catch any of the forms below. Each row was confirmed two ways. GNU Make 4.3 exits 0 on a failing recipe, from a small probe Makefile. And the head test file passes against the repository's
The list's "a recipe that masks a failure in another form" covers the last row, and arguably the Replacement for that bullet (keep line continuations):
If P3 1's code lands, drop the forms it closes from this bullet. P31. A small change closes most of P2's Makefile forms (
# A setting that lets make test pass after a recipe line fails: .IGNORE or
# .ONESHELL (only the last line's status counts), any mention of MAKEFLAGS
# (=, +=, override, export, $(eval …)), or a SHELL or .SHELLFLAGS of its own.
MAKE_SETTING_THAT_MASKS = re.compile(
r"^ *(\.IGNORE|\.ONESHELL)\s*:(?!=)|\bMAKEFLAGS\b"
r"|^ *(override\s+|export\s+)*(SHELL|\.SHELLFLAGS)\s*[:+?!]?=",
re.MULTILINE,
)
def assert_make_test_fails_when_tests_fail(makefile: Path) -> None:
text = makefile.read_text()
rules = make_rules(makefile)
assert not MAKE_SETTING_THAT_MASKS.search(text), (
"the Makefile can ignore recipe failures"
)
# Walk test's prerequisites at every depth.
checked, queue = [], ["test"]
while queue:
target = queue.pop()
if target in checked:
continue
checked.append(target)
prerequisites, recipe = rules.get(target, ([], []))
queue += prerequisites
# make_rules keeps only a target's last rule line and skips
# double-colon rules, so require exactly one single-colon rule line.
colons = re.findall(rf"^{re.escape(target)}\s*(::?)(?!=)", text, re.M)
assert colons == [":"], f"make {target} needs exactly one single-colon rule line"
for line in recipe:
prefix = line[: len(line) - len(line.lstrip("@+- \t"))]
assert "-" not in prefix, f"make {target} ignores a failing recipe line"
assert not MASKED_RECIPE_LINE.search(line), (
f"make {target} masks a failing command: {line}"
)Notes on this code:
2. The same parser gap hides an upload from the Makefile upload scan. This is not new in this PR.
Mutants:
For each, 3. The masked-line regex fails on text that masks nothing. That is loud, and I'd keep it.
Each of these fails loudly, and none occurs on master. MutantsEach mutant was a text edit on the working tree, made by a script. For each one, the script ran three pytest processes: head's
Locator messages (item 4), taken from the pytest output:
Checked and sound
What I couldn't check
Generated by Claude Code |
|
Picking this up for the US hub. I'll post a hand-off comment here when it's ready. Generated by Claude Code |
… 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> Claude-Session: https://claude.ai/code/session_01LwQq6Jdt6qgZLwHQRNYNF6
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> Claude-Session: https://claude.ai/code/session_01LwQq6Jdt6qgZLwHQRNYNF6
|
Ready for hub review Answers the review at #650 (comment). Two commits on Items
MutantsEach mutant is a scripted text edit on the working tree. For each one, the script ran head's
GNU Make 4.3 probesEach probe is a scratch Makefile whose
TestsOne file per process,
CIThe Pull request run 41833712248 at Judgment calls
Proposed follow-ups
Generated by Claude Code |
|
Hub review in progress Round 2 review of head
The final review follows. Generated by Claude Code |
|
Ready for hub review I reviewed head The fix session did items 1 to 6. Every hand-off mutant I reran gives the hand-off's count (46 rows: head, docs and master files). Master's There are two P2s, both of the same kind as round 1's:
None of these forms exists in the repository today. Assignment. The PR does items 1 to 6 of the fix brief.
P21. A comment-only line or a conditional inside a recipe hides the recipe lines after it, from both the test guard and the upload scan (
Failure scenario: someone writes test-country-template:
@echo running template tests
# run them
policyengine-core test policyengine_core/country_template/tests -c policyengine_core.country_template || true
Commands and output:
Fix. Let the parser skip comment-only lines as make does, and fail closed on the syntax it can't follow. Master's @@ -405,22 +405,39 @@
]
+# Make syntax that make_rules does not follow, so recipe lines after it could
+# run unread: a conditional or define block, a changed recipe prefix, or a
+# backslash at the end of a line.
+MAKE_SYNTAX_MAKE_RULES_SKIPS = re.compile(
+ r"^ *((override|export)\s+)*(ifn?eq|ifn?def|else|endif|define|endef)\b"
+ r"|\.RECIPEPREFIX|\\$",
+ re.MULTILINE,
+)
+
+
def make_rules(makefile: Path) -> dict[str, tuple[list[str], list[str]]]:
"""Each target's prerequisites and recipe lines (without the tab).
Enough for this Makefile: it has no line continuations, includes or
pattern rules.
"""
+ text = makefile.read_text()
+ assert not MAKE_SYNTAX_MAKE_RULES_SKIPS.search(text), (
+ "make_rules cannot read a conditional, define block, .RECIPEPREFIX or "
+ "line continuation in the Makefile"
+ )
rules = {}
recipe = []
- for line in makefile.read_text().splitlines():
+ for line in text.splitlines():
if line.startswith("\t"):
recipe.append(line[1:])
elif rule := MAKE_RULE.match(line):
recipe = []
for target in rule.group(1).split():
rules[target] = (rule.group(2).split(), recipe)
- elif line.strip():
+ # make ignores blank lines and lines of just a comment among recipe
+ # lines, so neither ends a recipe.
+ elif line.strip() and not line.lstrip().startswith("#"):
recipe = []
return rulesWhat this diff does, run as a scratch copy of the head test file:
If the hub prefers not to change the code, narrow the fragment's recipe clause instead:
Then add the forms to the body's list (P2 2). 2. The body's "What the checks still can't see" leaves out Makefile and environment forms that head misses (PR body, the Makefile bullets). Failure scenario: a maintainer reads the list as complete and relies on the guard for one of the forms below.
The body's "Makefiles other than the root one" doesn't clearly cover Replacement for the body's Makefile bullets, if P2 1's diff lands:
Keep the
If P2 1's diff doesn't land, also add P2 1's forms and P31. The fragment's 2. One loud false positive isn't in the body's list. A recipe line such as MutantsEach mutant was a text edit on the working tree, made by a script. For each one, the script ran three pytest processes:
After each mutant it ran How to read the table:
Results:
Checked and sound
What I couldn't check
Generated by Claude Code |
…ollow 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> Claude-Session: https://claude.ai/code/session_01LwQq6Kc7ibRJ5e1WVQfQD7
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwQq6Kc7ibRJ5e1WVQfQD7
|
Picking this up for the US hub. I'll post a hand-off comment here when it's ready. Generated by Claude Code |
|
Ready for hub review This answers the round 2 review at #650 (comment). It adds two commits on Items
MutantsEach row is a scripted text edit, run in one of four detached
Each file was copied into place before its run. After every process the script ran How to read the table:
GNU Make 4.3 probes (
TestsOne file per process,
On CIThe Pull request run 41892304633 at
https://github.com/PolicyEngine/policyengine-core/actions/runs/41892304633 Judgment calls
Proposed follow-ups
Generated by Claude Code |
|
Hub review in progress Done. The round 3 review at Generated by Claude Code |
|
Ready for hub review I reviewed head The two new commits apply round 2's diff exactly, and they make every text change the fix brief asked for. Every hand-off row I reran gives the hand-off's count. Master's There is one P2, of the same kind as round 2's P2 1.
The fragment's "They fail if it holds a conditional ( None of these forms exists in the repository today. Assignment
P21. A directive after a tab or form feed, rather than spaces, passes What GNU make 4.3 does, from
The manual's "a tab is not allowed" (Conditional Syntax) covers only a tab as the first character inside a rule context. A space then a tab is not a recipe line, and neither is a tab line outside a rule. Failure scenario: someone writes this in test: test-country-template
ifeq (1,1)
coverage run -a --branch -m pytest tests … || true
endif
coverage xml -i
Mutants on the working tree (head's file,
For each row, docs gives 5 passed and master's file 42 passed. Small probes confirm the mechanism: T3 and T6 need a tab line after a variable assignment, which is unusual. T1, T2 and T4 need only an indent that mixes a space and a tab, or a form feed. Fix: one character class. MAKE_SYNTAX_MAKE_RULES_SKIPS = re.compile(
- r"^ *((override|export)\s+)*(ifn?eq|ifn?def|else|endif|define|endef)\b"
+ r"^[ \t\f\v]*((override|export)\s+)*(ifn?eq|ifn?def|else|endif|define|endef)\b"
r"|\.RECIPEPREFIX|\\$",
re.MULTILINE,
)I ran this on a scratch copy of head's file:
Add these in-suite parameters to pytest.param(
"\tcoverage xml -i\n",
" \tifeq (1,1)\n\tcoverage xml -i || true\n \tendif\n",
id="conditional-after-space-and-tab",
),
pytest.param(
"all: install",
"\fdefine SHELL\n/bin/true\n\fendef\n\nall: install",
id="define-after-form-feed",
),
pytest.param(
"all: install",
"X = 1\n\tifeq (1,0)\n\tendif\n\nall: install",
id="tab-conditional-outside-rule",
),With the fix, a recipe line whose first word after the tab is a directive word now fails loudly too.
With the fix, the fragment and the body's first bullet hold as written. If the hub prefers not to change the code, add this to "What the checks still can't see" and narrow the fragment's sentence to "… a conditional (
P31. The
The alternative is to add MutantsEach row was a scripted text edit on the working tree. Three pytest processes ran for each row: head's I rebuilt the rows from the hand-off's and round 2's one-line descriptions, so my edits may differ in detail from theirs. Where they differ, the count can differ. Every row below matches the hand-off's "This head" column.
"Both" means The first time I ran it, my script put C4 on the Checked and sound
What I couldn't check
Generated by Claude Code |
|
Picking this up for the US hub. I'll post a hand-off comment here when it's ready. Generated by Claude Code |
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> Claude-Session: https://claude.ai/code/session_01LwQq6GrDhQfRQyGWsSRqqw
…file 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> Claude-Session: https://claude.ai/code/session_01LwQq6GrDhQfRQyGWsSRqqw
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwQq6GrDhQfRQyGWsSRqqw
|
Ready for hub review This answers the round 3 review (#650 (comment)): P2 1 is fixed in code, and P3 1 is closed in code rather than in the body. There are three new commits on Items
TestsOne file per process,
MutantsEach row was a scripted text edit, rebuilt from the review's one-line descriptions, applied separately for each of four pytest processes:
After each process the script ran "Both" means
What kills each row:
GNU Make 4.3 on scratch copies of each mutated
Rows outside T1 to T6 each fail the same test functions at head as at G3 gives 22 failed here against the review's 23 because my rebuild differs in detail. It gives the same 22 at both heads. All other non-T rows match the review's counts plus 5. CIThe Pull request run at
None was cancelled or skipped. Judgment calls
What I couldn't check
Proposed follow-up
Generated by Claude Code |
|
Hub review in progress Round 4 review of head
Generated by Claude Code |
|
Ready for hub review I reviewed head The three new commits do what the fix brief asked. The directive pattern now kills T1 to T6 through both guards, and I found one P2 in the same family as round 3's P2: Python and make split the Assignment
P21. A line break that make doesn't see hides a recipe line's Python's Failure scenario: someone writes coverage run -a --branch -m pytest tests --disable-pytest-warnings --reruns 2 --reruns-delay 5 $(if ,<form feed>)|| true
Commands: scripted edits to the root
For the Fix: refuse those characters before @@ -414,6 +414,13 @@ MAKE_SYNTAX_MAKE_RULES_SKIPS = re.compile(
re.MULTILINE,
)
+# Python ends a line at \r, \v, \f, \x1c-\x1e, \x85,
and
too
+# (read_text turns a lone \r into \n, and splitlines splits at the rest), but
+# make ends one only at \n. So a recipe line such as
+# `cmd $(if ,<form feed>)|| true` would reach make_rules as two lines, and its
+# `|| true` or upload would go unread.
+MAKE_LINE_BREAK_MAKE_IGNORES = re.compile(r"\r(?!\n)|[\v\f\x1c-\x1e\x85
]")
+
def make_rules(makefile: Path) -> dict[str, tuple[list[str], list[str]]]:
@@ (in make_rules, before `text = makefile.read_text()`)
+ assert not MAKE_LINE_BREAK_MAKE_IGNORES.search(makefile.read_bytes().decode()), (
+ "make_rules cannot read a line break other than \\n or \\r\\n in the Makefile"
+ )
text = makefile.read_text()Add these parameters to pytest.param(
"\tcoverage xml -i\n",
"\tcoverage xml -i $(if ,\f)|| true\n",
id="form-feed-inside-recipe-line",
),
pytest.param(
"\tcoverage xml -i\n",
"\tcoverage xml -i $(if ,\r)|| true\n",
id="carriage-return-inside-recipe-line",
),I ran the fix, with the docstring and message from P3 2, on a scratch copy of head's file:
Then:
With this check in place, P31. The body's I checked each route against pytest 9.2.0's installed source,
Replacement:
2. The hand-off's account is right. The docstring names "a conditional, a define block, .RECIPEPREFIX or a line that ends in a backslash", and the message names the same four. At head, other line ends the recipe. It fails on a conditional, a define block,
.RECIPEPREFIX, a line that ends in a backslash or a line break other than
a newline, since recipe lines after them could run unread. It also fails
on a recipe line that opens with a directive word, such as `else`, which
it can't tell from a directive. It misreads a double-colon, indented or
repeated rule line and a `;` recipe on the rule line, which "make_rules cannot read a conditional, define block, .RECIPEPREFIX, "
"line continuation or recipe line that opens with a directive word in "
"the Makefile"3. No test pins MutantsEach row was a scripted edit to the root
Checked and sound
What I couldn't check
Generated by Claude Code |
|
Picking this up for the US hub. I'll post a hand-off comment here when it's ready. Generated by Claude Code |
Python's splitlines breaks a line at a form feed, vertical tab, lone carriage return, \x1c-\x1e, \x85, and , 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> Claude-Session: https://claude.ai/code/session_01LwQq6JgiWGYWii3moSdXzX
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwQq6JgiWGYWii3moSdXzX
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> Claude-Session: https://claude.ai/code/session_01LwQq6JgiWGYWii3moSdXzX
|
Ready for hub review Head: New commits: Items
TestsOne file per process,
Pattern check: over all 0x110000 code points, the 9 characters other than MutantsA script made each row as a text edit to the root
Code mutants of the new check, each run in its own process with the file restored after (
CIPull request run 41957742064 at Judgment calls
Proposed follow-up
Generated by Claude Code |
|
Hub review in progress Round 5, head Generated by Claude Code |
|
Ready for hub review I reviewed head The three new commits do what the fix brief asked. The new check refuses each of the nine characters that Python splits a line at and GNU make doesn't, and allows Assignment
P31. The self-tests don't pin Failure scenario: someone drops
Command: code mutants on a scratch copy of the file, one process each, file restored after: Replacement: add this test after @pytest.mark.parametrize(
"line_break", ["\v", "\x1c", "\x1d", "\x1e", "\x85", "
", "
"]
)
def test_make_rules_rejects_each_line_break_make_ignores(tmp_path, line_break):
makefile = tmp_path / "Makefile"
makefile.write_bytes(
MAKEFILE.read_bytes().replace(
b"\tcoverage xml -i",
f"\tcoverage xml -i $(if ,{line_break})|| true".encode(),
)
)
with pytest.raises(AssertionError, match="make_rules cannot read a line break"):
make_rules(makefile)On a scratch copy at head this gives 109 passed, and 2. The CRLF allowance is pinned only on a CRLF checkout ( 3. Judgment calls
MutantsA script made each row as a byte-level edit to the root
Code mutants of the new lines, one process each, file restored after, run with the LF
Checked and sound
What I couldn't check
Generated by Claude Code |
|
US + core hub merge audit, core#650 at
🤖 Generated with Claude Code |
* Pin each line break make_rules refuses, and its CRLF allowance The independent review of #650 found that dropping \v (mutant c6) or \x1c-\x1e, \x85, U+2028 and U+2029 (c7) from MAKE_LINE_BREAK_MAKE_IGNORES left all 102 tests passing, and that turning \r(?!\n) into \r (c4) failed only on a CRLF checkout. A parametrized test now writes each of the seven unpinned characters into a recipe line as bytes, so the result doesn't depend on the locale encoding, and expects make_rules to refuse it. A second test writes LF and CRLF copies of the root Makefile (normalized first, in case the checkout is already CRLF) and expects the same rules from both, so c4 fails on an LF checkout too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Follow-ups from the independent review of #649 (landed as
d9da48a1c7): #649 (comment), items P3 1 to P3 4. The PR changes onlytests/core/test_release_tagging.pyand addschangelog.d/core649-release-test-followups.changed.md. Nothing underpolicyengine_core/or.github/changes.impact: tests only; no change to the package or to any workflow.
What's changed
assert_make_test_fails_when_tests_failtakes theMakefilepath. It fails whenMAKE_SETTING_THAT_MASKSmatches anywhere in the file: any mention of.IGNORE,.ONESHELL,MAKEFLAGS(soGNUMAKEFLAGS,override MAKEFLAGSand$(eval MAKEFLAGS …)too) orPYTEST_ADDOPTS, or an assignment toSHELLor.SHELLFLAGS, including target-specific ones. It then walkstest's prerequisites at every depth, including order-only ones. Each target it reaches must have a single rule line, single-colon, starting in the first column and with no;recipe, becausemake_rulesmisreads every other form.make_rulesskips comment-only lines inside a recipe, as make does. It fails, naming the syntax, on a conditional, adefineblock,.RECIPEPREFIX, a line that ends in\or a line break other than a newline, since recipe lines after them could run unread. The check fails on any recipe line it reaches that starts with-, contains||, or ends in; true,; exit 0or; :.assert_test_job_fails_when_tests_failalso fails when theenvof the workflow, the Test job or the test step setsMAKEFLAGS. Master passes: itsMakefilementions none of these settings, each target has one plain rule line, andcoverage xml -imatches none of the recipe forms.test_only_push_publish_job_uploads_to_pypiapplies the same one-rule-line check to every target on a rule line in the rootMakefile. So abuild::rule, a laterbuild: installline, orbuild: ; twine upload dist/*can no longer hide an upload frompypi_uploading_recipe_lines.test_only_publish_job_gets_pypi_credentialsreads every workflow file. It fails if any job other thanpush.yaml's Publish declaresenvironment: pypior getsid-token: write, or if Publish lacks either. It reads the environment as a name or as a mapping'sname, ignoring case, and a job's ownpermissions, or the workflow's when the job sets none, countingwrite-all. On master no other job declaresid-token: write(Docs hascontents: writeonly), so the test checks both declarations.test_only_push_publish_job_uploads_to_pypiwith the review's text, adds a pointer to the new credentials check, and leaves the regex as it was.expected one test (a step whose run starts with the words `make test`; if the command moved, update this locator and the exact match below) step, found [].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. The scan also misses the publish action with a mixed-case owner (PyPA/gh-action-pypi-publish).uses: owner/action@ref), other thanpypa/gh-action-pypi-publish, when its job declares neitherenvironment: pypinorid-token: write. 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*).${{ … }}).Makefile: a rootGNUmakefileormakefile, which make reads instead; a makefile named inMAKEFILESin a workflowenv, which make reads first; andincluded makefiles (include,-include,sinclude).$(MAKE).$(T):, or.$(I):withI = IGNORE), or an assignment whose name is one ($(S) = /bin/truewithS = SHELL).-ithat a workflowenvsets inGNUMAKEFLAGS, or that an earlier step writes to$GITHUB_ENVinMAKEFLAGSorGNUMAKEFLAGS. The checks catch amake -i testcall in the test step itself, because they require the step's command to equalmake test.if cmd; then :; fi,cmd | tee log, orcmd; true # note. The recipe parser targets this repository's Makefile.# a || b, orecho 'a || b';$(MAKEFLAGS), or a comment that mentions.IGNOREorPYTEST_ADDOPTS;.PHONY:line;\, a conditional (ifeq…endif), adefineblock or.RECIPEPREFIX, or a recipe line that opens with one of those directive words, such aselse;SHELLfor one command, such asSHELL=bash ./run;testthat names a file, a variable or a pattern-rule target with no rule line of its own.PYTEST_ADDOPTSto$GITHUB_ENV, or theMakefilecan build that name from parts (export $(P)OPTSwithP = PYTEST_ADD). Options can also come fromaddoptsinpytest.toml,.pytest.tomlorpytest.ini, fromconftest.py, or from a different variable. If someone deletespytest.ini,addoptsin.pytest.ini,pyproject.toml,tox.iniorsetup.cfgtakes effect too..github/workflows/, which GitHub doesn't run (the Guard the release tests against masked test failures and other PyPI uploads #648 review quotes its docs).Tests
One file per process,
uv run pytest -q -p no:cacheprovider <file>:test_release_tagging.pytest_docs_deployment.pyeaef9c2ruff format --checkandruff checkpass on the changed test file. Comments on this PR give the mutant results: R7–R16 from #649's review, A1–A13 for the first round of checks, and N1–N15 from this PR's review. X1–X13 cover the further Makefile forms, and C1–C6 the forms the list named before the round 2 review (C4,cmd; \withtrueon the next line, now fails). From the round 2 review, F1–F11, F9b, G1–G4 and H1–H4 cover forms it found, Q1–Q7 its probes, W1–W9 the list's workflow entries and L1–L6 the fail-closed entries. From the round 3 review, T1–T6 cover a conditional ordefineafter a space and a tab, after a form feed, or after a tab outside a rule, and PA1–PA2 anexport PYTEST_ADDOPTS = --collect-onlyline, global and target-specific. From the round 4 review, B1–B5 cover a|| trueor an upload after a form feed,\u2028,\x1cor a lone carriage return inside a recipe line.🤖 Generated with Claude Code