ci(create-release): scope the cascade-guard comment to the publishing predecessors (#960)

[JarryShaw]

Please follow the guide below

• Searched for similar pull requests
• Followed the coding style (make pylint, make mypy, make isort)
• make test passes, and a test case covers the change
• Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — N/A — a workflow comment and a test; no library behaviour change

What is the purpose of your pull request?

• fix — corrects a defect
• feat — adds a feature
• perf — changes performance, not behaviour
• refactor — changes neither behaviour nor performance
• test — tests only
• docs — documentation only
• ci — workflows or build tooling
• chore — anything else

---

Description of your pull request and other information

Closes #960. The third and last instance of one error: the comment above jobs: in create-release.yml said tag, pypi and conda spell the cascade guard as the success-or-skipped equality pair "for each of their own direct dependencies".

That is false for version_check, a direct dependency of all three — needs: [ github, version_check ] at :443 and :521, [ tag, github, version_check ] at :620 — which each require to have strictly succeeded, with no || 'skipped', at :445, :523, :622. The equality pair applies only to github, and for conda also tag.

Why a comment is worth a pull request. This comment is where the documentation got the claim from. #958 was the identical error in releasing.rst, corrected in #959; pep.rst carried a related one, corrected in #957. Leaving this would have had the docs describe this file more accurately than the file describes itself, and the next person reconciling the two would reasonably have trusted the code.

No executable YAML changed. git diff -- .github/workflows/create-release.yml | grep -E '^[+-]' | grep -vE '^(\+\+\+|---)' | grep -vE '^[+-]#' returns nothing — every changed line is a # comment. yaml.safe_load still parses the file.

A test that nothing covered. test_release_gates.py pinned the publishing predecessors' equality pair but had no version_check.result assertion at all, so neither the comment nor the prose had cover for that clause. test_version_check_must_strictly_succeed now asserts, per job, that needs.version_check.result == 'success' is present and needs.version_check.result == 'skipped' is not. The fully-qualified string avoids a substring false-pass.

Falsifiability, shown both ways. The "fix" here is the workflow's existing behaviour, so a passing test proves nothing on its own. Against a copy of the workflow with || needs.version_check.result == 'skipped' added to tag:

AssertionError: "needs.version_check.result == 'skipped'" unexpectedly found in …
FAILED (failures=1)

Against the real file: OK. I ran both myself rather than relying on the report.

Counts: tests/project/test_release_gates.py 35 passed, 26 subtests, re-derived as 35 tests, OK under plain unittest (pytest-subtests undercounts, hence the second run).

Not verified: the workflow was not dispatched, and must not be — it publishes releases. The comment change cannot affect its behaviour, which is what the grep above establishes.

[JarryShaw]

NEEDS CHANGES at d180f6bc5 — opus cross-review, round 1. The comment prose survived every clause check; the new test did not. Fixed at 93dbdc9a0.

It built eight mutants and three stayed green while breaking the intent. The serious one:

(needs.version_check.result != 'failure' || needs.version_check.result == 'success')

Logically just != 'failure', so it admits skipped and cancelled — reinstating the exact weak sibling-file spelling the new comment says version_check is not held to, while keeping the == 'success' literal as a vacuous disjunct. The test reported OK. Two more passed green on a bare || == 'cancelled', which the test's own docstring says must stop the job.

One correction to its report, which I verified before relying on either version. Its table says all eight mutants fail once the neighbouring test's third assertion (assertNotIn "!= 'failure'") is added. That is not true of the || == 'cancelled' pair: such a condition contains == 'success', contains neither == 'skipped' nor != 'failure', so all three substring assertions pass and it stays green. Measured on m4's exact condition string.

So I took its stronger alternative rather than the matching-idiom one — the set of comparisons against version_check.result must be exactly {('==', 'success')}:

head                          -> success=True  failures=0
m1 tag || skipped             -> success=False failures=1
m4 tag || cancelled           -> success=False failures=1
m8 tag != failure||== success  -> success=False failures=1
m2 tag clause deleted         -> success=False failures=1

It also showed my comment-only check was weak. grep -vE '^[+-]#' only matches # at column zero and says nothing about indented YAML. It parsed both revisions with yaml.safe_load and compared the loaded structures — STRUCTURES EQUAL: True. That is the claim worth making on a workflow that publishes to PyPI and Anaconda, and the commit message now cites it instead.

Two prose points it settled that I had asked about: a skipped version_check is genuinely reachable — unit-tests carries an if: on github.event.workflow_run.conclusion == 'success', so a Vendor Update completing unsuccessfully skips it and cascades — so the comment does not overstate. And the differ-only-at-cancelled clause, the one stated backwards in pep.rst round 2, is correct here: the whole-run case does not break it, because !cancelled() makes both spellings false and they still agree.

Round 2 running against 93dbdc9a0, briefed to attack the set assertion for over-strictness and for a hole in the regex. Staying at review: pending.

[JarryShaw]

NEEDS CHANGES at 93dbdc9a0 — opus cross-review, round 2. Two more green-but-broken mutants against the set form, both real, both fixed at 4ce1ee4aa.

The set assertion anchored needs.version_check.result on the left of the operator, and == is symmetric. So the mirror image slips through:

(needs.version_check.result == 'success' || 'skipped' == needs.version_check.result)

Semantically identical to the mutation the test was written to catch, and it reported OK. A second hole used no == at all — contains(fromJSON('["skipped","cancelled"]'), needs.version_check.result) — so the regex found nothing to add to the set.

The argument behind the fix is why I took it whole rather than patching those two spellings. The set form's failure mode is asymmetric: rewrite the real clause in an unrecognised spelling and the set goes empty, which fails loudly and safely; add an unrecognised weakening alongside the retained clause and it passes silently. So the test now strips whitespace and asserts both the comparison set and count('version_check.result') == 1. The count is an exhaustiveness check rather than another enumerated spelling — anything added raises it however written.

Reproduced myself at the new head, each mutant yaml.safe_load-checked first:

head                         success=True  failures=0
m1  || == 'skipped'          caught        m9  reversed operands    caught
m4  || == 'cancelled'        caught        m10 contains/fromJSON    caught
m8  (!=failure||==success)    caught        m11 spaced dots          caught
m2  clause deleted           caught

It also confirmed my correction and withdrew its own table row, which is worth recording: its round-1 column labelled "with the missing assertion" had in fact measured the third assertNotIn and the set form together, so the singular label was wrong and the third assertion alone does not close || == 'cancelled'. That is the kind of retraction that makes the rest of a review trustworthy.

Two judgements it offered that I agree with. The set form is not over-strict: a loosening must fail by design, and a redundant tightening like && != 'cancelled' deserves to fail too, since == 'success' already excludes cancelled and anyone adding it has misunderstood the very thing this sequence has been correcting. And m12 — ('failure' != needs.version_check.result), which drops the == 'success' literal — is correctly caught by the assertIn, so that assertion still earns its place.

I took its note on the failure message: the maintainer-facing explanation moved into the docstring, which now records why substrings cannot pin this clause and names the two forms the count exists for.

Unverified, and it says so: whether Actions accepts whitespace-padded property dots, and whether it rejects double-quoted literals. Both would need a real run, which is forbidden here. The whitespace-padded mutant's reachability therefore rests on unconfirmed syntax — but the other two holes do not, and either alone justifies the fix.

Round 3 running against 4ce1ee4aa. Staying at review: pending.

[JarryShaw]

NEEDS CHANGES at 4ce1ee4aa — opus cross-review, round 3. A third generation of mutants, and one of them is a one-character typo. Fixed at b6a1c6dfe.

Seven mutants kept exactly one version_check.result reference and exactly one == 'success' comparison, and passed. Its diagnosis of why: the count and set assertions pin the condition's vocabulary; nothing pinned its boolean structure. Two of the seven are credible rather than contrived, and it said plainly which:

• && → ||. !cancelled() || needs.version_check.result == 'success' makes the gate true on every run that was not cancelled. One character, catastrophic, green.
• || github.event_name == 'push', which someone might add as "allow a direct tag push" — the workflow's own primary trigger — silently bypassing the evidence gate on the main path. The same shape as the original ci: create-release.yml's comment over-generalises the cascade guard to version_check #960 error: a disjunction added without working out what it admits.

The fix is one more assertion: the clause must appear as a top-level && conjunct. Verified by me against all three generations — head passes, and || == 'skipped', reversed operands, contains(fromJSON(...)), || event_name, a negation, &&→|| and || outputs != '' all fail.

I took its scope judgement rather than its tightest option. It offered a prefix anchor that closes everything including a true || short-circuit, but that pins the exact opening of the expression, so any legitimate reordering of conjuncts fails. Three rounds in, that is tighter than the defect profile warrants, and a test people route around is worse than one residual hole. The true || case stays open and I am naming it rather than burying it.

It also found that whitespace-stripping can invent a false reading — an identifier or literal split across the folded scalar's line break compacts to the pristine form and passes. Low severity and I am keeping the compaction: those expressions are ones Actions rejects, so the real gate fails closed and nothing publishes, whereas the hole compaction closes fails open. The docstring now records that trade.

Two of my questions it settled with measurement rather than argument. count on version_check.result is the right granularity — at head each job references .result once and .outputs once, so counting version_check would have forbidden the evidence reference the gate legitimately needs.

The principled endpoint it identified, which I am not doing here: this module already contains TestReleaseStatusScriptExecutesCorrectly, which extracts the release_status script and runs it, precisely because string assertions over YAML were "structurally unable to catch" the #905 defects. A small evaluator for the Actions-expression subset, run over the four result values, would be immune to all twenty mutants across three rounds. That is real work and its own issue — filing it.

Round 4 running against b6a1c6dfe. Staying at review: pending.

[JarryShaw]

NEEDS CHANGES at b6a1c6dfe — opus cross-review, round 4. No new defect family this time; two bookkeeping findings, both real and both taken at a93e726a8.

The comparison-set assertion was provably redundant, and I reproduced the proof before removing it. Given the conjunct substring and count == 1, the set is necessarily {('==', 'success')} — the conjunct guarantees one occurrence followed by == 'success', the count guarantees it is the only occurrence, and the regex matches only at occurrences. Brute-forced over 173,088 synthetic conditions built from twelve fragments including every spelling from rounds 1–3: zero cases where it fires alone. Two assertions now instead of three. It is redundant only because round 4 replaced the plain assertIn with the conjunct form — the dependency is in the commit message.

And it caught me recording the residual in the wrong place, and describing it too narrowly. I grepped my own output: the true || acceptance appears once across the PR comments and zero times in the commit message or the docstring — which is the one place a future maintainer will look. Worse, the plausible member is the suffix form, not the prefix: appending || github.event_name == 'workflow_dispatch' to a long condition is a natural edit, where wrapping it in true || is not. The docstring now says a top-level || anywhere defeats the conjunct pin, gives that example, notes that conjunction is monotone so added && conjuncts need no guard, and points at #962.

That is the thesis of #960 applied to this PR: an undocumented hole became a documented one.

Its docstring cut was specific and I took it as given — the catalogue of defeated spellings went, since that is recoverable from this PR's history and #962; the #960 provenance and the whitespace fail-open/fail-closed trade stayed, since neither is recoverable elsewhere. ~20 lines down to ~12.

Branch rebased onto a1a13e65d now that #957 and #959 have merged and this was BEHIND. One commit; create-release.yml 14/8 comment lines, test_release_gates.py 58/0.

Round 5 running to confirm. Staying at review: pending.

[JarryShaw]

GOOD TO GO at a93e726a8 — opus cross-review, round 5. Five rounds, and I verified each of its three confirmations myself.

parent = a1a13e65d        merge-base(HEAD, origin/main) = a1a13e65d
git diff --numstat a1a13e65d...a93e726a8  ->  14/8 create-release.yml, 58/0 test_release_gates.py
git diff 414c52569 a93e726a8 -- tests/project/test_release_gates.py  ->  empty
yaml.safe_load(a1a13e65d) == yaml.safe_load(a93e726a8)  ->  True

The parent is the merge-base, so the branch is genuinely on top of current main; one commit; the test body byte-identical to the pre-rebase head. And the comment-only guarantee re-verified against the new base, which matters because the base moved under it — #957 and #959 merging did not touch this workflow, so the structural equality carries across the rebase.

A detail from its matrix worth recording: m14, m16 and m17 are caught by the conjunct assertion alone — the count assertion reads clean on all three. That assertion is the one round 4 added, and it is load-bearing for exactly the two defect shapes rated as plausible, including the one-character &&→||.

It found one fifth-generation candidate and then disqualified it honestly. A duplicate if: key on the job attacks the scanner rather than the expression: YAML says last-wins, declared_if returns the first match, so with the real gate first and always() second this test passes while Actions evaluates always(). Both its assertions read clean. But TestYAMLAgreesWithTheScanner.test_the_same_job_to_if_mapping (:1159, which I confirmed exists) catches it in both orderings — that is the job that test exists for. It flagged it anyway so the record shows the coverage comes from the sibling rather than from here, and so nobody later "simplifies" the agreement test on the grounds that the scanner is obviously right. That is the correct instinct.

Its closing observation is the one I would keep. Five rounds found four real defects — (!= 'failure' || == 'success'), the reversed-operand and contains(fromJSON(...)) pair, the one-character &&→||, and the undocumented residual — and none of them was in the comment this PR set out to fix. That comment was clean at round 1 and still is. The cost was entirely in the test I added beyond what #960 asked for, and the test is now worth having.

Still open by design: a top-level || outside the conjunction chain, documented in the docstring and tracked as #962, which is blocked behind this.

Labelled review: good-to-go. CI is running on the rebased head, so not ready to merge yet — and the merge is yours.