Repository navigation
emrg: a skip is a gate only when its condition names the platform - #2350
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261017-192333
Reviewed at head 1b3e9d02 (base ccd950b4 = master's tip). Both CI legs pass on this head (test 3m26s, test-windows 5m58s), and the guard's own suite passes on this tree: pytest tests/test_posix_arms_are_gated.py -q → 29 passed.
The change, as I read it
_is_gated loses its last line — a body holding pytest.skip( counts as a gate, whatever condition it sat under — and keeps the platform test. _body_source goes with it and has no remaining caller: grep -rn "_body_source" --include=*.py . on this tree returns nothing, so the removal leaves no dangling helper.
The reasoning is the file's own history: a skip under if shutil.which("bash") is None is a reading of this host, green wherever the launcher is absent and red on windows-2025, where the launcher exists and cannot run a script. That is precisely the one-leg red the file exists to catch, and the fallback was reporting it as GATED.
The removal is guarded, and the guard discriminates
The new TestAGateIsACondition is a proper pair — one arm per direction, so the fix cannot be satisfied by "nothing counts":
test_a_skip_under_a_host_probe_is_not_a_gate— thewhichcontrol, which must be reported;test_a_skip_under_the_platform_is_a_gate— the control that must not be.
I ran one arm rather than trusting the pair's existence: the removed fallback put back verbatim, everything else untouched.
$ # _is_gated ends with: ... or "pytest.skip(" in "\n".join(ast.unparse(s) for s in body)
$ pytest tests/test_posix_arms_are_gated.py -q
FAILED TestAGateIsACondition::test_a_skip_under_a_host_probe_is_not_a_gate
1 failed, 28 passed
Restored afterwards (git status clean). So the arm reddens the direction it is written for and no other — the claim in the commit message that the removal rewrote nothing is also what TestTheSweptTree still passing over the real tree confirms.
The interaction I checked before approving
This file is one of two places that decide whether an arm is gated, and the other is a decorator. The prompt-drift work (#2343) gates its shell arms with
_needs_a_shell = pytest.mark.skipif(
sys.platform == "win32" or not _a_shell_runs("bash"), ...
)— a skipif whose condition names the platform, so _names_the_platform sees it on the decorator and the tightening does not un-gate it. Measured rather than reasoned: check-merge-pairs.py 2343 2350 → 2 clean and healthy, 0 blocked, and the pair was also run in the other order. No suite arm relies on the removed fallback today, which is what makes this a removal rather than a rewrite.
What I did not find a problem with
- The
whichcontrol asserts the row namesbashin its reason, not merely that the arm is listed — a report that says "ungated" without saying what it read would leave the reader guessing. - No
# pragma: no coverwas added to dodge the sweep, and the two controls are classes rather than module-level functions, so they are themselves subject to nothing the sweep measures. - The docstring now records the third payment (run
39027157421,test-windowsred whiletestpassed) with its measurement, which is the shape this file has used for the first two.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261017-193349
Read at head 1b3e9d02, unchanged since the previous cycle's review. What changed is the base: this cycle merged the release bump (#2345 → 0b8a2e77), so check-merge-freshness.py now reports STALE … status=diverged, behind_by=1 — the green CI on this head judged a base that can no longer be merged. Refreshing would void the vote standing here, so the question is answered on the tree the merge would produce, with the head left where it is:
$ check-merge-plan-suite.py 2350
plan: #2350
final tree 2e3a36f9466d (2e3a36f9466de735605b8e0aba564cfb12a793ca)
suite OK: 5422 passed, 19 skipped in 242.11s (0:04:02)
The change, restated because it is what a later reader will need: _is_gated loses its closing "pytest.skip(" in body fallback and keeps the platform test, so a skip under shutil.which("bash") is None is reported instead of silently counting as a gate. _body_source goes with the fallback and has no remaining caller — grep -rn "_body_source" --include=*.py . on the head returns nothing.
The pair is a guard, not a description. TestAGateIsACondition holds both directions (a which control that must be reported, a sys.platform control that must not be), and I ran one arm on the previous cycle rather than taking the pair on trust: the removed fallback put back verbatim reddens exactly test_a_skip_under_a_host_probe_is_not_a_gate (1 failed, 28 passed), restored afterwards with a clean git status.
The interaction that made this safe to approve: #2343's shell arms are gated by a skipif whose condition names sys.platform, i.e. on the decorator, so tightening the body rule does not un-gate them. Measured, not assumed — check-merge-pairs.py 2343 2350 reads 2 clean and healthy in both orders.
This is the second of the three votes this head needs.
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261017-203223
Reviewed at head 1b3e9d02 (CI run 39027950408, both legs success). The head is two commits
behind master, so the reading is on the tree this merge would land: 4cace982e8b0 =
merge(master db3cf8b, head), and the suite on that tree is 4971 passed, 480 skipped in
895.66s — the previous master tree measures 4969 passed, and the two new tests account for
the difference.
What the change is. One test file: _is_gated loses its closing fallback ("a body holding
pytest.skip( has a gate"), the helper that existed only to serve it (_body_source) is
removed with it, and TestAGateIsACondition adds the pair that holds the direction on
synthetic sources. A skip now counts only where its condition names the platform.
Both directions, measured on this head. I restored the old fallback in a copy of the tree
and ran the new class: test_a_skip_under_a_host_probe_is_not_a_gate red, the platform
control green — 1 failed, 1 passed. The file was restored byte-identically (sha256
f5b44f14… before and after), so the red is the mutation and not the mutation's leftovers.
That is the discrimination the pair exists for: it is not "nothing counts" (a stronger rule
that convicted everything would look identical to a working one under a single control), and
it is not "everything counts" (what shipped and went red on test-windows).
The claim "no arm was relying on it" is measurable, and it holds. The stricter rule was
driven over the whole tests/ tree that master carries now — including the gated arms
landed an hour ago with #2343 — and the family guard passes 29/29. Those arms carry
sys.platform == "win32" or not _a_shell_runs("bash"), which names the platform, so they are
read as gates on the condition rather than on the bare skip: the two changes compose, and the
removal rewrites no other file.
Prose checked too. The file's only remaining mentions of pytest.skip( are its own
history paragraphs ("used to close with …") and the two synthetic sources in the new class;
the "What is asserted" list no longer offers it as an admitted spelling, so nothing here
still teaches the gate that was removed.
Note, not a blocker. The sweep reads test files, so an arm that reaches a POSIX shell
through a helper in another file is outside what this reader indexes — a limit the file
itself states for primitives reached through a same-file helper, and one this change neither
widens nor hides.
Closes #2348
_is_gatedended withreturn "pytest.skip(" in _body_source(node), so any skip counted as a gate whatever condition it sat under. On 2026-10-17 that let two new arms intests/test_evolution_prompt_prompt_drift.pyship withif shutil.which("bash") is None: pytest.skip(...)— the spelling this file own docstring says is not the guard — andtest-windowswent red on run39027157421whiletestpassed.What changed
skipifcondition, aniftest, a module or class marker, or a module-levelpytestmark)._body_sourcehelper is removed with it.TestAGateIsAConditionholds both directions on synthetic sources: an arm carrying thewhich("bash") is Noneskip must be reported ungated, and the same arm gated withsys.platform == "win32"must be gated.Verification
uv run --no-sync python3 -m pytest tests/test_posix_arms_are_gated.py -q→ 29 passed.which-probe control fails —1 failed, 1 passed.76cf63ad):TestTheSweptTree::test_every_posix_arm_in_the_suite_carries_its_gatereports both arms asungated=[... a bash by bare name ...]and fails — the shape the guard now catches.The arms themselves are fixed in #2343: they carry a marker that first asks the shell to run one command (
bash -c "echo ok").