From 2b71a9c144dd020b7013b82c21e1d130eee2e68f Mon Sep 17 00:00:00 2001 From: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com> Date: Tue, 22 Sep 2026 03:25:08 -0700 Subject: [PATCH 1/4] fix(guard): pre-yield permit check excused conditionally-exiting arms and credited unreachable releases Pre-existing coverage gaps in scripts/check_preyield_permit_release.py, found by Argus's independent RUNTIME oracle during review of #863 and measured present on BOTH the merged #860 guard and the #863 candidate -- so this is not a regression from either, and is filed separately rather than widening a verified branch. FINDING 1 -- `_handler_exits()` asked "is the LAST statement a bare raise/return?", scanning reversed(handler.body) and breaking on any other statement type. An except arm that exits through a compound statement was therefore classified as swallowing and excused from releasing. It escapes only in the SIBLING shape: a correct `except BaseException:` arm makes the chain look satisfied while the misclassified narrow arm is the one that runs. Replaced by `_handler_may_exit()`, which asks whether ANY path through the arm can leave via raise/return (nested callables excluded -- own lifecycle). Conservative by design: a raise a nested handler swallows still counts as a possible exit, so such an arm is asked to release. False positive at worst. FINDING 2 -- `_released_in()` used ast.walk, so a textually-present release that can never run still satisfied the check. Now only statically-reachable statements are credited: nested def/lambda bodies, `if False:` branches, loops over an empty literal, and everything after an unconditional raise/return are skipped. Anything the compiler cannot settle still counts, so a release under a runtime condition, in a non-empty loop, or in a `with` body is unaffected. FINDING 3 -- `except BaseException: pass` as the only arm was FLAGGED, though it does not leak: the arm swallows and control reaches the yield still legitimately holding the permit, which is the guard's own stated rationale for not demanding a release from a swallowing arm. Cause was asking the BaseException-coverage question of the EXITING arms only, so an all-swallowing chain produced an empty set and read as unprotected. Coverage is now asked of the whole chain; the release demand only of arms that can exit. VERIFIED Runtime oracle (real asyncio.Semaphore, synchronous fault injected at the pre-yield log, measured permit delta; HEALTHY arm clean on all 11 shapes so no degenerate fixtures) reproduces Argus's matrix exactly: I1/I2/I3/I4/I7 and F1/F2/F3/F4 each LEAK 1 permit; controls I6 (releases then conditionally raises) and H1 (BaseException: pass) are clean. The new guard's static verdict matches that measurement 1:1 across all 18 probed shapes, 0 mismatches. 15 new tests (46 total, was 31 on #863's head). Sweep still exit 0 over the same 2 enumerated sites -- no live site has any of these shapes. Per-file invocation exit 0, empty-dir sweep still exit 2 (vacuity floor preserved). ruff 0.15.20 clean on both files. --- scripts/check_preyield_permit_release.py | 213 ++++++++++++-- tests/scripts/test_preyield_permit_release.py | 259 ++++++++++++++++++ 2 files changed, 444 insertions(+), 28 deletions(-) diff --git a/scripts/check_preyield_permit_release.py b/scripts/check_preyield_permit_release.py index cdccd78da13d..2b1c01955e03 100644 --- a/scripts/check_preyield_permit_release.py +++ b/scripts/check_preyield_permit_release.py @@ -51,10 +51,32 @@ multi-permit site (``self.total`` + ``self.internal``, as in ``gateway/turn_admission.py``) an ``except`` arm that releases only one of them used to satisfy the check. - * **Every handler arm must release.** A sibling ``except ValueError:`` that - re-raises without releasing leaks, even when a later - ``except BaseException:`` arm does release — the narrow arm is the one that - runs. + * **Every handler arm that CAN exit must release.** A sibling + ``except ValueError:`` that re-raises without releasing leaks, even when a + later ``except BaseException:`` arm does release — the narrow arm is the + one that runs. + +Two further sharpenings (Argus runtime oracle on PR #863, filed as a +pre-existing gap present on BOTH builds): + + * **Exit is a REACHABILITY question, not a last-statement question.** The + arm classifier used to accept only a BARE trailing ``raise``/``return``, + so an arm exiting through a compound statement (``if cond: raise``, a + ``raise`` inside an inner ``try``/``with``/``for``, ``if cond: return``) + was classified as swallowing and excused from releasing — five measured + 1-permit leaks. It now asks whether ANY path through the arm can leave. + * **An unreachable release does not count.** Matching by ``ast.walk`` + credited a release buried in a nested ``def``, under ``if False:``, in a + loop over an empty literal, or after an unconditional ``raise`` — four + more measured leaks. Only statically-reachable statements are credited; + anything the compiler cannot settle still counts, so a release under a + runtime condition is not rejected. + * **A chain whose only arm SWALLOWS is safe.** ``except BaseException: + pass`` falls through to the yield still legitimately holding the permit + (measured: no leak), but the ``BaseException`` coverage test used to be + asked of the *exiting* arms only, so an all-swallowing chain produced an + empty set and was reported as unprotected. Coverage is now asked of the + whole chain and the release demand only of the arms that can exit. DOES NOT COVER (stated boundary, not a hidden gap) -------------------------------------------------- @@ -171,42 +193,170 @@ def _receiver_key(call: ast.Call) -> str | None: return None -def _objects_called(node: ast.AST, names: set[str]) -> set[str]: - """Receivers of every ``.()`` call anywhere under ``node``.""" +def _static_truth(node: ast.AST) -> bool | None: + """``True``/``False`` for a test the compiler can settle; ``None`` otherwise.""" + if isinstance(node, ast.Constant): + try: + return bool(node.value) + except Exception: # pragma: no cover - bool() is total on constants + return None + return None + + +def _statically_empty_iterable(node: ast.AST) -> bool: + """True for a literal that provably yields nothing (``[]``, ``()``, ``{}``).""" + if isinstance(node, (ast.List, ast.Tuple, ast.Set)): + return not node.elts and not any(isinstance(e, ast.Starred) for e in node.elts) + if isinstance(node, ast.Dict): + return not node.keys + if isinstance(node, ast.Constant) and isinstance(node.value, (str, bytes)): + return len(node.value) == 0 + return False + + +def _direct_releases(node: ast.AST) -> set[str]: + """Release receivers in ``node``, NOT descending into a nested callable.""" found: set[str] = set() - for sub in ast.walk(node): + stack: list[ast.AST] = [node] + while stack: + current = stack.pop() + if current is not node and isinstance( + current, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda) + ): + # Its body runs on its own lifecycle, if it is ever called at all. + continue if ( - isinstance(sub, ast.Call) - and isinstance(sub.func, ast.Attribute) - and sub.func.attr in names + isinstance(current, ast.Call) + and isinstance(current.func, ast.Attribute) + and current.func.attr in _RELEASE_METHODS ): - key = _receiver_key(sub) + key = _receiver_key(current) if key is not None: found.add(key) + stack.extend(ast.iter_child_nodes(current)) return found +def _released_in_stmt(stmt: ast.stmt) -> set[str]: + """Release receivers reachable when ``stmt`` runs. + + A textually-present release that can never execute must not satisfy the + check: ``ast.walk`` credited a release buried in a nested ``def``, under + ``if False:``, or in the body of a loop over an empty literal, none of + which ever hand the permit back. + """ + if isinstance(stmt, (ast.FunctionDef, ast.AsyncFunctionDef)): + return set() + if isinstance(stmt, ast.If): + truth = _static_truth(stmt.test) + found = _direct_releases(stmt.test) + if truth is not False: + found |= _released_in(stmt.body) + if truth is not True: + found |= _released_in(stmt.orelse) + return found + if isinstance(stmt, (ast.For, ast.AsyncFor)): + found = _direct_releases(stmt.iter) + if not _statically_empty_iterable(stmt.iter): + found |= _released_in(stmt.body) + return found | _released_in(stmt.orelse) + if isinstance(stmt, ast.While): + found = _direct_releases(stmt.test) + if _static_truth(stmt.test) is not False: + found |= _released_in(stmt.body) + return found | _released_in(stmt.orelse) + if isinstance(stmt, ast.Try): + found = ( + _released_in(stmt.body) + | _released_in(stmt.orelse) + | _released_in(stmt.finalbody) + ) + for handler in stmt.handlers: + found |= _released_in(handler.body) + return found + if isinstance(stmt, (ast.With, ast.AsyncWith)): + found: set[str] = set() + for item in stmt.items: + found |= _direct_releases(item.context_expr) + return found | _released_in(stmt.body) + return _direct_releases(stmt) + + def _released_in(body: list[ast.stmt]) -> set[str]: + """Receivers released on some STATICALLY REACHABLE path through ``body``.""" released: set[str] = set() for stmt in body: - released |= _objects_called(stmt, _RELEASE_METHODS) + released |= _released_in_stmt(stmt) + if not _falls_through(stmt): + # Everything after an unconditional raise/return is dead code. + break return released -def _handler_exits(handler: ast.ExceptHandler) -> bool: - """True if this arm leaves the generator instead of falling through. +def _body_falls_through(body: list[ast.stmt]) -> bool: + """True if control can reach the end of ``body``.""" + for stmt in body: + if not _falls_through(stmt): + return False + return True + - An arm that SWALLOWS (no ``raise``/``return``) continues on to the yield - with the permit still legitimately held — demanding a release there would - be a false positive, and acting on it would be a double release. Only an - arm that exits owes the permit back. +def _falls_through(stmt: ast.stmt) -> bool: + """True if control can continue past ``stmt`` to the next statement.""" + if isinstance(stmt, (ast.Raise, ast.Return, ast.Break, ast.Continue)): + return False + if isinstance(stmt, ast.If): + truth = _static_truth(stmt.test) + if truth is True: + return _body_falls_through(stmt.body) + if truth is False: + return _body_falls_through(stmt.orelse) + return _body_falls_through(stmt.body) or _body_falls_through(stmt.orelse) + if isinstance(stmt, (ast.With, ast.AsyncWith)): + return _body_falls_through(stmt.body) + if isinstance(stmt, ast.Try): + if stmt.finalbody and not _body_falls_through(stmt.finalbody): + return False + through_body = _body_falls_through(stmt.body) + if through_body and stmt.orelse: + through_body = _body_falls_through(stmt.orelse) + return through_body or any(_body_falls_through(h.body) for h in stmt.handlers) + # Loops may run zero times or `break`; a match/other compound statement is + # assumed to fall through. Erring this way keeps an arm out of the + # "provably exits" class only when we cannot prove it, which is safe: + # `_handler_may_exit` below is what demands the release. + return True + + +def _handler_may_exit(handler: ast.ExceptHandler) -> bool: + """True if ANY path through this arm can leave via ``raise``/``return``. + + An arm that provably SWALLOWS (no reachable ``raise``/``return`` at all) + continues on to the yield with the permit still legitimately held — + demanding a release there would be a false positive, and acting on it + would be a double release. + + The previous rule asked a much narrower question: "is the LAST statement a + bare ``raise``/``return``?", scanning ``reversed(handler.body)`` and + breaking on any other statement type. That excused every arm that exits + through a compound statement — ``if cond: raise``, ``raise`` inside an + inner ``try``/``with``/``for``, ``if cond: return`` — each a measured + 1-permit leak. Asking "can it exit?" instead of "does its last statement + exit?" covers the nested forms too. + + Deliberately conservative: a ``raise`` that a nested handler swallows still + counts as a possible exit, so such an arm is asked to release. That is a + false positive at worst; the other direction is a missed leak. Nested + function/lambda bodies do not count — they run on their own lifecycle. """ - for stmt in reversed(handler.body): - if isinstance(stmt, (ast.Raise, ast.Return)): + stack: list[ast.AST] = list(handler.body) + while stack: + node = stack.pop() + if isinstance(node, (ast.Raise, ast.Return)): return True - if isinstance(stmt, ast.Pass): + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda)): continue - break + stack.extend(ast.iter_child_nodes(node)) return False @@ -256,20 +406,27 @@ def _try_releases(node: ast.Try, required: set[str]) -> bool: Two sound shapes: * a ``finally`` that releases them all (runs on every exceptional exit), or - * a handler chain that covers ``BaseException`` and, in EVERY arm that - exits, releases them all. A sibling ``except ValueError:`` arm that + * a handler chain that covers ``BaseException`` and, in every arm that CAN + exit, releases them all. A sibling ``except ValueError:`` arm that re-raises without releasing is the leak an any-arm check missed. + + An arm that provably swallows owes nothing: control falls through to the + yield still holding the permit, which is correct. That is why the + ``BaseException`` coverage question is asked of the WHOLE chain and not + only of the exiting arms — ``except BaseException: pass`` as the sole arm + is safe, and treating its empty exiting-set as "unprotected" flagged it. """ if _released_in(node.finalbody) >= required: return True - exiting = [h for h in node.handlers if _handler_exits(h)] - if not exiting: + if not node.handlers: return False - if not any(_is_baseexception_handler(h) for h in exiting): + if not any(_is_baseexception_handler(h) for h in node.handlers): # Nothing here catches a cancel; the window is still unprotected. return False - return all(_released_in(h.body) >= required for h in exiting) + return all( + _released_in(h.body) >= required for h in node.handlers if _handler_may_exit(h) + ) def _releases_on_the_way_out( diff --git a/tests/scripts/test_preyield_permit_release.py b/tests/scripts/test_preyield_permit_release.py index 9c3568ffb1e1..035a7983dfe0 100644 --- a/tests/scripts/test_preyield_permit_release.py +++ b/tests/scripts/test_preyield_permit_release.py @@ -664,6 +664,265 @@ async def gate(sem, key): assert violations == [] +# -------------------------------------------------------------------------- +# 3b. Reachability: "can control reach the end of this arm?", not "is the last +# statement a bare raise/return?". Every shape below is a MEASURED +# 1-permit leak (real asyncio.Semaphore, synchronous fault at the pre-yield +# log) that was CLEAN on both the #860 guard and the #863 candidate. +# -------------------------------------------------------------------------- + + +def _sibling_arm_source(narrow_arm: str) -> str: + """A two-arm gate: a narrow arm under test + a correct BaseException arm. + + The sibling matters: with only the narrow arm the try has no + ``BaseException`` coverage and is flagged for that reason instead, so the + fixture would pass for the wrong reason. The correct sibling is what makes + the narrow arm's own classification the thing under test. + """ + return textwrap.dedent( + """ + from contextlib import asynccontextmanager, suppress + + @asynccontextmanager + async def gate(sem, surface): + await sem.acquire() + try: + _LOG.info("admitted %s", surface) + {arm} + except BaseException: + sem.release() + raise + try: + yield + finally: + sem.release() + """ + ).replace("{arm}", textwrap.indent(textwrap.dedent(narrow_arm), " ").rstrip()) + + +@pytest.mark.parametrize( + "shape,arm", + [ + ("if cond: raise", "except Boom:\n if surface:\n raise\n"), + ( + "raise inside an inner try/finally", + "except Boom:\n try:\n raise\n finally:\n pass\n", + ), + ( + "raise inside a with", + "except Boom:\n with suppress(ValueError):\n raise\n", + ), + ("raise inside a for", "except Boom:\n for _ in (1,):\n raise\n"), + ("if cond: return", "except Boom:\n if surface:\n return\n"), + ], +) +def test_guard_flags_a_conditionally_exiting_arm_that_does_not_release(shape, arm): + """FINDING 1: an arm that exits through a COMPOUND statement still owes. + + The old rule read ``reversed(handler.body)`` and returned True only on a + BARE trailing ``Raise``/``Return``, breaking on any other statement type. + So each shape here was classified as swallowing and excused from + releasing, while the correct ``BaseException`` sibling made the chain look + satisfied. All five are measured 1-permit leaks. + """ + guard = _load_guard() + _sites, violations = guard.scan_source(_sibling_arm_source(arm), "new.py") + assert [v["function"] for v in violations] == ["gate"], ( + f"an arm that exits via {shape} must be asked to release" + ) + + +def test_conditionally_exiting_arm_is_accepted_once_it_releases(): + """The discriminator's other side: releasing first clears the same shape. + + Argus's CONTROL I6 — measured NOT to leak — so the new rule must not flag + it, or the fixture above would pass by flagging everything. + """ + guard = _load_guard() + releasing = "except Boom:\n sem.release()\n if surface:\n raise\n" + _sites, violations = guard.scan_source(_sibling_arm_source(releasing), "new.py") + assert violations == [] + + +@pytest.mark.parametrize( + "shape,arm", + [ + ( + "nested def", + "except BaseException:\n def _later():\n sem.release()\n raise\n", + ), + ( + "if False", + "except BaseException:\n if False:\n sem.release()\n raise\n", + ), + ( + "loop over an empty literal", + "except BaseException:\n for _ in []:\n sem.release()\n raise\n", + ), + ], +) +def test_guard_does_not_credit_an_unreachable_release(shape, arm): + """FINDING 2: ``ast.walk`` counted a release that can never run. + + A release buried in a nested ``def`` (its own lifecycle), under + ``if False:``, or in the body of a loop over an empty literal is textually + present and never executes. All three are measured 1-permit leaks. + """ + guard = _load_guard() + source = textwrap.dedent( + """ + from contextlib import asynccontextmanager + + @asynccontextmanager + async def gate(sem, surface): + await sem.acquire() + try: + _LOG.info("admitted %s", surface) + {arm} + try: + yield + finally: + sem.release() + """ + ).replace("{arm}", textwrap.indent(textwrap.dedent(arm), " ").rstrip()) + _sites, violations = guard.scan_source(source, "new.py") + assert [v["function"] for v in violations] == ["gate"], ( + f"a release reachable only via {shape} must not satisfy the check" + ) + + +@pytest.mark.parametrize( + "shape,arm", + [ + ("under a runtime condition", "if surface:\n sem.release()\n"), + ("in a loop over a non-empty literal", "for _ in (1,):\n sem.release()\n"), + ("in a with body", "with suppress(ValueError):\n sem.release()\n"), + ], +) +def test_guard_still_credits_a_conditionally_reachable_release(shape, arm): + """The reachability narrowing must not reject releases that CAN run. + + Only statically-dead paths lose credit; anything the compiler cannot + settle stays creditable, or the finding-2 fixture would pass by rejecting + every non-trivial release. + """ + guard = _load_guard() + source = textwrap.dedent( + """ + from contextlib import asynccontextmanager, suppress + + @asynccontextmanager + async def gate(sem, surface): + await sem.acquire() + try: + _LOG.info("admitted %s", surface) + except BaseException: + {arm} + raise + try: + yield + finally: + sem.release() + """ + ).replace("{arm}", textwrap.indent(textwrap.dedent(arm), " ").rstrip()) + # Two releases: the one under test in the handler, and the finally around + # the yield. If the fixture ever loses one, it is no longer the shape. + assert source.count("sem.release()") == 2 + _sites, violations = guard.scan_source(source, "new.py") + assert violations == [], f"a release {shape} is reachable and must count" + + +def test_guard_does_not_flag_a_sole_swallowing_baseexception_arm(): + """FINDING 3: ``except BaseException: pass`` as the ONLY arm is SAFE. + + The arm swallows and control proceeds to the yield still legitimately + holding the permit — measured NOT to leak. The old code asked for a + ``BaseException`` among the EXITING arms only, so a chain whose sole arm + swallows produced an empty exiting-set and was reported as unprotected, + contradicting the guard's own swallow rationale. + """ + guard = _load_guard() + source = textwrap.dedent( + """ + from contextlib import asynccontextmanager + + @asynccontextmanager + async def gate(sem, surface): + await sem.acquire() + try: + _LOG.info("admitted %s", surface) + except BaseException: + pass + try: + yield + finally: + sem.release() + """ + ) + _sites, violations = guard.scan_source(source, "new.py") + assert violations == [] + + +def test_guard_does_not_flag_a_narrow_releasing_arm_beside_a_swallowing_base(): + """FINDING 3 (H2): narrow arm releases+raises, BaseException swallows. + + Neither path leaks — the narrow arm hands the permit back before exiting, + and the BaseException arm falls through to the yield still holding it. + """ + guard = _load_guard() + source = textwrap.dedent( + """ + from contextlib import asynccontextmanager + + @asynccontextmanager + async def gate(sem, surface): + await sem.acquire() + try: + _LOG.info("admitted %s", surface) + except ValueError: + sem.release() + raise + except BaseException: + pass + try: + yield + finally: + sem.release() + """ + ) + _sites, violations = guard.scan_source(source, "new.py") + assert violations == [] + + +def test_swallowing_chain_still_needs_baseexception_coverage(): + """A swallow-only chain that does NOT cover BaseException is still a leak. + + ``except ValueError: pass`` catches nothing a cancel raises, so the window + remains unprotected — finding 3's relaxation must not extend to it. + """ + guard = _load_guard() + source = textwrap.dedent( + """ + from contextlib import asynccontextmanager + + @asynccontextmanager + async def gate(sem, surface): + await sem.acquire() + try: + _LOG.info("admitted %s", surface) + except ValueError: + pass + try: + yield + finally: + sem.release() + """ + ) + _sites, violations = guard.scan_source(source, "new.py") + assert [v["function"] for v in violations] == ["gate"] + + def test_guard_enumerates_both_permits_at_the_live_turn_admission_site(): """Positive coverage on the REAL multi-permit site, named not counted.""" guard = _load_guard() From ab9c8eb029ca673ea0d7be720b2043f7357de457 Mon Sep 17 00:00:00 2001 From: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com> Date: Tue, 22 Sep 2026 03:29:08 -0700 Subject: [PATCH 2/4] test(guard): gate the dead-code truncation and nested-callable exclusion M4 and M5 survived the mutation battery -- dropping the dead-code truncation in _released_in (release after an unconditional raise) and the nested-callable exclusion in _direct_releases (release only reachable inside a nested def or lambda) both left 46/46 green, so those two behaviours were implemented but ungated. Adds the three shapes that name them; each is a measured 1-permit leak on the runtime oracle (F3, F5, F6) with the HEALTHY arm clean. --- tests/scripts/test_preyield_permit_release.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/scripts/test_preyield_permit_release.py b/tests/scripts/test_preyield_permit_release.py index 035a7983dfe0..c2d157970dcd 100644 --- a/tests/scripts/test_preyield_permit_release.py +++ b/tests/scripts/test_preyield_permit_release.py @@ -752,6 +752,14 @@ def test_conditionally_exiting_arm_is_accepted_once_it_releases(): "nested def", "except BaseException:\n def _later():\n sem.release()\n raise\n", ), + ( + "nested def under a condition", + "except BaseException:\n if surface:\n def _later():\n sem.release()\n raise\n", + ), + ( + "lambda under a condition", + "except BaseException:\n if surface:\n _later = lambda: sem.release()\n raise\n", + ), ( "if False", "except BaseException:\n if False:\n sem.release()\n raise\n", @@ -760,6 +768,10 @@ def test_conditionally_exiting_arm_is_accepted_once_it_releases(): "loop over an empty literal", "except BaseException:\n for _ in []:\n sem.release()\n raise\n", ), + ( + "after an unconditional raise", + "except BaseException:\n raise\n sem.release()\n", + ), ], ) def test_guard_does_not_credit_an_unreachable_release(shape, arm): From 70c40e5fe7564e1bc9cba6f0facbe532e63050b7 Mon Sep 17 00:00:00 2001 From: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com> Date: Tue, 22 Sep 2026 04:57:49 -0700 Subject: [PATCH 3/4] fix(guard): the swallow exemption is a whitelist, not "no ast.Raise here" Review found 4 MEASURED permit-leak regressions vs the merged #860 guard. `_handler_may_exit` excused an arm when it found no `ast.Raise`/`ast.Return` node in it -- but an arm also leaves via a call that raises, an `assert`, arithmetic, or a plain logging call given a bad format argument. Each is a real 1-permit loss that #860 caught and this branch let through. "This arm swallows" is not AST-decidable: the SAME arm text leaks 0 with a benign logger and leaks 1 with a bad format arg. So the question is inverted and answered on a whitelist -- an arm is excused only when every statement in it is provably inert (pass, a bare constant, an assignment between names/constants, a nested def). Anything else owes the permit back. The arm check now asks the same question the window check always has. The branch's own fixture at tests/.../:636 was the counterexample (its arm held a logging call), so it is narrowed to statements that cannot leave. MEASURED on the runtime oracle (real asyncio.Semaphore, synchronous fault at the pre-yield log, permit delta; HEALTHY arm clean on all 26 shapes): regressions vs #860 4 -> 0 false negatives 7 -> 2 (X4/X9 only, both pre-existing on #860/#863 and now named in the DOES NOT COVER block) false positives 0 -> 0 every prior win kept I1-I7, F1-F5, X5, X6 still FIRE; H1/H2 still clean X10 closes for free Mutation battery 7/7 RED, control 61 green at both ends: M8 exemption -> "contains ast.Raise" 7 failed / 54 passed M9 _inert_expr always True 6 failed / 55 passed M10 _inert_stmt always True 23 failed / 38 passed M11 _inert_stmt always False 8 failed / 53 passed M1 reachability -> #860 bare-trailing rule 13 failed / 48 passed M2 _released_in -> ast.walk 6 failed / 55 passed M3 BaseException coverage -> exiting only 2 failed / 59 passed Floors held: repo sweep exit 0 over the same 2 sites, per-file exit 0, empty-dir sweep exit 2, ruff clean. --- scripts/check_preyield_permit_release.py | 106 +++++++++++++----- tests/scripts/test_preyield_permit_release.py | 63 ++++++++++- 2 files changed, 139 insertions(+), 30 deletions(-) diff --git a/scripts/check_preyield_permit_release.py b/scripts/check_preyield_permit_release.py index 2b1c01955e03..edf47f80587e 100644 --- a/scripts/check_preyield_permit_release.py +++ b/scripts/check_preyield_permit_release.py @@ -96,6 +96,13 @@ assignment itself is a raise-capable node in the window), so this errs toward a false positive, not a miss; a new spelling is WONTFIX unless it appears in production code. + * **A release whose REACHABILITY depends on runtime values.** Two measured + 1-permit leaks sit here and are not caught by any build of this guard: a + release that runs only in a nested ``except ValueError`` that never fires, + and a release under a runtime-false condition. The check credits any path + the compiler cannot settle (see ``_released_in_stmt``); narrowing that + would reject the legitimate ``if acquired: release()`` shape the live + multi-permit site uses. Static reachability is the declared line. NO LONGER A GAP: releasing the WRONG object used to satisfy the check, because matching was by method name. Release is now matched by RECEIVER @@ -328,38 +335,81 @@ def _falls_through(stmt: ast.stmt) -> bool: return True -def _handler_may_exit(handler: ast.ExceptHandler) -> bool: - """True if ANY path through this arm can leave via ``raise``/``return``. - - An arm that provably SWALLOWS (no reachable ``raise``/``return`` at all) - continues on to the yield with the permit still legitimately held — - demanding a release there would be a false positive, and acting on it - would be a double release. - - The previous rule asked a much narrower question: "is the LAST statement a - bare ``raise``/``return``?", scanning ``reversed(handler.body)`` and - breaking on any other statement type. That excused every arm that exits - through a compound statement — ``if cond: raise``, ``raise`` inside an - inner ``try``/``with``/``for``, ``if cond: return`` — each a measured - 1-permit leak. Asking "can it exit?" instead of "does its last statement - exit?" covers the nested forms too. - - Deliberately conservative: a ``raise`` that a nested handler swallows still - counts as a possible exit, so such an arm is asked to release. That is a - false positive at worst; the other direction is a missed leak. Nested - function/lambda bodies do not count — they run on their own lifecycle. +def _inert_expr(node: ast.AST) -> bool: + """True only for an expression that provably cannot raise. + + Deliberately a WHITELIST. Anything not listed here — a call, an attribute + load, a subscript, an ``await``, arithmetic on a name — can leave the arm + by raising, and must therefore keep the arm in the "may exit" class. """ - stack: list[ast.AST] = list(handler.body) - while stack: - node = stack.pop() - if isinstance(node, (ast.Raise, ast.Return)): - return True - if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda)): - continue - stack.extend(ast.iter_child_nodes(node)) + if isinstance(node, ast.Constant): + return True + if isinstance(node, ast.Name): + # A local load can raise NameError only in code that is already broken; + # treating it as inert keeps `x = y` out of the exit class. + return isinstance(node.ctx, (ast.Load, ast.Store)) + if isinstance(node, (ast.Tuple, ast.List, ast.Set)): + return all(_inert_expr(e) for e in node.elts) + if isinstance(node, ast.Dict): + return all( + k is not None and _inert_expr(k) for k in node.keys + ) and all(_inert_expr(v) for v in node.values) return False +def _inert_stmt(stmt: ast.stmt) -> bool: + """True only for a statement that provably cannot leave the arm.""" + if isinstance(stmt, (ast.Pass, ast.Break, ast.Continue, ast.Global, ast.Nonlocal)): + return True + if isinstance(stmt, ast.Expr): + return _inert_expr(stmt.value) + if isinstance(stmt, ast.Assign): + return all(_inert_expr(t) for t in stmt.targets) and _inert_expr(stmt.value) + if isinstance(stmt, ast.AnnAssign): + return _inert_expr(stmt.target) and ( + stmt.value is None or _inert_expr(stmt.value) + ) + if isinstance(stmt, (ast.FunctionDef, ast.AsyncFunctionDef)): + # Defining a nested callable does not run it; its body is its own + # lifecycle. The `def` statement itself cannot leave the arm. + return not stmt.decorator_list + return False + + +def _handler_may_exit(handler: ast.ExceptHandler) -> bool: + """True unless every statement in this arm PROVABLY cannot leave it. + + An arm that provably SWALLOWS continues on to the yield with the permit + still legitimately held — demanding a release there would be a false + positive, and acting on it would be a double release. + + Two rules have been wrong here, in opposite directions: + + * "is the LAST statement a bare ``raise``/``return``?" (scanning + ``reversed(handler.body)``, breaking on any other statement type) excused + every arm that exits through a compound statement — ``if cond: raise``, + ``raise`` inside an inner ``try``/``with``/``for``, ``if cond: return`` — + each a measured 1-permit leak. + * "does it contain an ``ast.Raise``/``ast.Return`` node?" excused every arm + that leaves for a reason that is not a ``raise`` STATEMENT: a call that + raises, an ``assert``, arithmetic that divides by zero, or a plain + logging call given a bad format argument. All four are measured + 1-permit leaks that the pre-#873 guard caught. + + "This arm swallows" is not decidable from the AST — the SAME arm text is + safe with a benign logger and leaks with a bad format argument. So the + question is inverted and answered on a whitelist: an arm is excused only + when every statement in it is provably inert (``pass``, a bare constant, an + assignment between names/constants, a nested ``def``). Anything else keeps + the arm in the "may exit" class and owes the permit back. That is a false + positive at worst; the other direction is a missed leak. + + This makes the arm check ask the same question as the window check, which + has always treated a call/attribute/subscript/await as raise-capable. + """ + return not all(_inert_stmt(stmt) for stmt in handler.body) + + def _build_parent_map(fn: ast.AST) -> dict[int, tuple[ast.AST, str]]: """Map id(child) -> (parent, field_name) for everything in ``fn``'s own body.""" parents: dict[int, tuple[ast.AST, str]] = {} diff --git a/tests/scripts/test_preyield_permit_release.py b/tests/scripts/test_preyield_permit_release.py index c2d157970dcd..5d25c739d3de 100644 --- a/tests/scripts/test_preyield_permit_release.py +++ b/tests/scripts/test_preyield_permit_release.py @@ -637,7 +637,13 @@ def test_guard_does_not_demand_a_release_from_a_swallowing_arm(): """An arm that SWALLOWS falls through to the yield still holding the permit. Demanding a release there would be a false positive, and acting on it would - double-release — so only arms that exit (raise/return) owe the permit back. + double-release — so only arms that can exit owe the permit back. + + The arm body must be PROVABLY inert. This fixture used to hold + ``_LOG.warning("tolerated; still admitted")``, which is not: driven with a + benign logger that arm leaks 0, and with a bad format argument — identical + AST — it leaks 1. A fixture whose safety depends on runtime values cannot + state the property, so it is narrowed to statements that cannot leave. """ guard = _load_guard() source = textwrap.dedent( @@ -650,7 +656,8 @@ async def gate(sem, key): try: _LOG.info("admitted %s", key) except ValueError: - _LOG.warning("tolerated; still admitted") + tolerated = True + pass except BaseException: sem.release() raise @@ -664,6 +671,58 @@ async def gate(sem, key): assert violations == [] +@pytest.mark.parametrize( + "shape,arm", + [ + ("a call that raises", "except BaseException:\n _reraise()\n"), + ("an assert", "except BaseException:\n assert surface\n"), + ("arithmetic", "except BaseException:\n x = 1 / surface\n"), + ("a logging call", 'except BaseException:\n _LOG.info("swallowed")\n'), + ("an attribute load", "except BaseException:\n x = surface.depth\n"), + ("a subscript", "except BaseException:\n x = surface[0]\n"), + ("an await", "except BaseException:\n await _drain()\n"), + ], +) +def test_guard_flags_an_arm_that_can_leave_without_a_raise_statement(shape, arm): + """An arm leaves for many reasons that are not an ``ast.Raise`` node. + + Asking "does this arm contain a ``raise``/``return`` STATEMENT?" excused a + call that raises, an ``assert``, arithmetic that divides by zero, and a + plain logging call given a bad format argument. Each is a measured + 1-permit leak. The window check has always treated a call / attribute / + subscript / await as raise-capable; the arm check now agrees. + """ + guard = _load_guard() + _sites, violations = guard.scan_source(_sibling_arm_source(arm), "new.py") + assert [v["function"] for v in violations] == ["gate"], ( + f"an arm that can leave via {shape} must be asked to release" + ) + + +@pytest.mark.parametrize( + "shape,arm", + [ + ("pass", "except BaseException:\n pass\n"), + ("a bare constant", 'except BaseException:\n "swallowed"\n'), + ("an assignment between names", "except BaseException:\n x = surface\n"), + ("a constant assignment", "except BaseException:\n x = 0\n"), + ( + "a nested def", + "except BaseException:\n def _later():\n sem.release()\n", + ), + ], +) +def test_guard_still_excuses_a_provably_inert_arm(shape, arm): + """The other side: an arm that provably cannot leave still owes nothing. + + Without this the swallow exemption would collapse to "never excuse + anything" and the finding-3 false-positive fix would be undone. + """ + guard = _load_guard() + _sites, violations = guard.scan_source(_sibling_arm_source(arm), "new.py") + assert violations == [], f"an arm holding only {shape} cannot leave" + + # -------------------------------------------------------------------------- # 3b. Reachability: "can control reach the end of this arm?", not "is the last # statement a bare raise/return?". Every shape below is a MEASURED From 8a86dbbd50c3f48480ba5547a6e19af09af8eb6b Mon Sep 17 00:00:00 2001 From: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com> Date: Tue, 22 Sep 2026 07:08:06 -0700 Subject: [PATCH 4/4] fix(guard): the swallow whitelist must be sound about targets, signatures and hashing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review measured 11 permit leaks that merged main catches and the first whitelist admitted. Three root causes, all "the construct is admitted but it can still leave the arm": * `_inert_expr` was applied to assignment TARGETS. A target is not a value: a Tuple/List target UNPACKS (TypeError on a non-iterable, ValueError on an arity mismatch) and an Attribute/Subscript target runs __setattr__ / __setitem__. `_inert_target()` now admits only a bare Name in Store ctx. * A nested `def` was treated as wholly inert, but only its BODY is deferred. Decorators, defaults, kw-only defaults, parameter annotations, the return annotation and PEP 695 type params all run when the `def` executes. `_inert_signature()` grades them. * Set and dict displays HASH at construction, so inert elements are not enough: `{[1]}` and `{[1]: 2}` raise while every sub-expression is inert. `_hashable_literal()` grades Set elements and Dict keys. Also: `AnnAssign.annotation` was unread (evaluated at runtime without PEP 563), and `Name(Load)` was whitelisted as inert. Y12 shows the latter is wrong — an unbound local inside an except arm is an ordinary possibility, not "already broken code" — so Name loads are dropped from the whitelist. `x = y` is no longer excused; that is a false positive, which is the safe side. Verified: reviewer's attack_whitelist_v2.py, unmodified apart from the build root, against guards rebuilt from git objects (860=6319e22775, main=e1b92e2acb == fork/main content md5 bdf592a6, candidate=this tree): 19 shapes, 0 void fixtures, 0 false negatives (Y12 included), 0 regressions vs main. Reviewer's 26-shape oracle_argus_r2.py: 0 regressions, 0 false positives, X4/X9 remain the only declared misses. Mutation battery 11/11 RED, control 77 green at both ends. Repo sweep exit 0 over 2 sites, per-file exit 0, empty-dir exit 2, ruff clean. --- scripts/check_preyield_permit_release.py | 87 +++++++++++++++---- tests/scripts/test_preyield_permit_release.py | 57 +++++++++++- 2 files changed, 124 insertions(+), 20 deletions(-) diff --git a/scripts/check_preyield_permit_release.py b/scripts/check_preyield_permit_release.py index edf47f80587e..1dc6f55973b9 100644 --- a/scripts/check_preyield_permit_release.py +++ b/scripts/check_preyield_permit_release.py @@ -335,28 +335,77 @@ def _falls_through(stmt: ast.stmt) -> bool: return True +def _hashable_literal(node: ast.AST) -> bool: + """True only for a literal that provably hashes without raising. + + A set display and a dict display HASH their elements/keys at construction + time, so inert elements are not enough: ``{[1]}`` and ``{[1]: 2}`` each + raise ``TypeError`` while every sub-expression in them is inert. + """ + if isinstance(node, ast.Constant): + return True + if isinstance(node, ast.Tuple): + return all(_hashable_literal(e) for e in node.elts) + return False + + def _inert_expr(node: ast.AST) -> bool: """True only for an expression that provably cannot raise. Deliberately a WHITELIST. Anything not listed here — a call, an attribute - load, a subscript, an ``await``, arithmetic on a name — can leave the arm - by raising, and must therefore keep the arm in the "may exit" class. + load, a subscript, an ``await``, arithmetic on a name, a bare NAME load — + can leave the arm by raising, and must therefore keep the arm in the + "may exit" class. """ if isinstance(node, ast.Constant): return True - if isinstance(node, ast.Name): - # A local load can raise NameError only in code that is already broken; - # treating it as inert keeps `x = y` out of the exit class. - return isinstance(node.ctx, (ast.Load, ast.Store)) - if isinstance(node, (ast.Tuple, ast.List, ast.Set)): + if isinstance(node, (ast.Tuple, ast.List)): return all(_inert_expr(e) for e in node.elts) + if isinstance(node, ast.Set): + return all(_hashable_literal(e) for e in node.elts) if isinstance(node, ast.Dict): - return all( - k is not None and _inert_expr(k) for k in node.keys - ) and all(_inert_expr(v) for v in node.values) + return all(k is not None and _hashable_literal(k) for k in node.keys) and all( + _inert_expr(v) for v in node.values + ) return False +def _inert_target(node: ast.AST) -> bool: + """True only for an assignment target that provably cannot raise. + + A target is not a value. A ``Tuple``/``List`` target is an UNPACK, which + raises ``TypeError`` on a non-iterable and ``ValueError`` on an arity + mismatch even when every name in it is inert; an ``Attribute``/``Subscript`` + target runs ``__setattr__``/``__setitem__``. Only a bare local name is + safe, and only in ``Store`` context. + """ + return isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store) + + +def _inert_signature(fn: ast.FunctionDef | ast.AsyncFunctionDef) -> bool: + """True only when executing this ``def`` statement evaluates nothing. + + Only the BODY of a nested function is deferred. Decorators, defaults, + keyword-only defaults, parameter annotations and the return annotation are + all evaluated when the ``def`` executes, so any of them can leave the arm. + """ + if fn.decorator_list: + return False + if getattr(fn, "type_params", ()): + return False + args = fn.args + if not all(_inert_expr(d) for d in args.defaults): + return False + if not all(d is None or _inert_expr(d) for d in args.kw_defaults): + return False + if fn.returns is not None and not _inert_expr(fn.returns): + return False + slots = [*args.posonlyargs, *args.args, *args.kwonlyargs, args.vararg, args.kwarg] + return all( + a is None or a.annotation is None or _inert_expr(a.annotation) for a in slots + ) + + def _inert_stmt(stmt: ast.stmt) -> bool: """True only for a statement that provably cannot leave the arm.""" if isinstance(stmt, (ast.Pass, ast.Break, ast.Continue, ast.Global, ast.Nonlocal)): @@ -364,15 +413,21 @@ def _inert_stmt(stmt: ast.stmt) -> bool: if isinstance(stmt, ast.Expr): return _inert_expr(stmt.value) if isinstance(stmt, ast.Assign): - return all(_inert_expr(t) for t in stmt.targets) and _inert_expr(stmt.value) + return all(_inert_target(t) for t in stmt.targets) and _inert_expr(stmt.value) if isinstance(stmt, ast.AnnAssign): - return _inert_expr(stmt.target) and ( - stmt.value is None or _inert_expr(stmt.value) + # The annotation is evaluated at runtime unless the module carries + # `from __future__ import annotations`; we do not read the module's + # future-imports here, so an annotation that is not itself inert keeps + # the arm in the may-exit class. + return ( + _inert_target(stmt.target) + and _inert_expr(stmt.annotation) + and (stmt.value is None or _inert_expr(stmt.value)) ) if isinstance(stmt, (ast.FunctionDef, ast.AsyncFunctionDef)): - # Defining a nested callable does not run it; its body is its own - # lifecycle. The `def` statement itself cannot leave the arm. - return not stmt.decorator_list + # Defining a nested callable does not run its BODY; its body is its own + # lifecycle. The signature, however, runs now. + return _inert_signature(stmt) return False diff --git a/tests/scripts/test_preyield_permit_release.py b/tests/scripts/test_preyield_permit_release.py index 5d25c739d3de..4ec65511aef0 100644 --- a/tests/scripts/test_preyield_permit_release.py +++ b/tests/scripts/test_preyield_permit_release.py @@ -681,6 +681,41 @@ async def gate(sem, key): ("an attribute load", "except BaseException:\n x = surface.depth\n"), ("a subscript", "except BaseException:\n x = surface[0]\n"), ("an await", "except BaseException:\n await _drain()\n"), + # A bare NAME load raises UnboundLocalError when the binding has not + # run yet — an ordinary possibility inside an except arm, and a + # measured 1-permit leak. + ("a name load", "except BaseException:\n x = surface\n"), + # An assignment TARGET is not a value: a tuple/list target UNPACKS, + # which raises TypeError on a non-iterable and ValueError on an arity + # mismatch, even when every name in it is inert. + ("a tuple-target unpack", "except BaseException:\n a, b = 0\n"), + ("a starred tuple-target unpack", "except BaseException:\n a, *b = 0\n"), + ("a list-target unpack", "except BaseException:\n [a, b] = 0\n"), + ("an attribute target", "except BaseException:\n surface.x = 0\n"), + ("a subscript target", "except BaseException:\n surface[0] = 0\n"), + # Only a nested def's BODY is deferred; its signature runs now. + ( + "a nested def with a raising default", + "except BaseException:\n def _later(x=_boom()):\n pass\n", + ), + ( + "a nested def with a raising kw-only default", + "except BaseException:\n def _later(*, x=_boom()):\n pass\n", + ), + ( + "a nested def with a raising arg annotation", + "except BaseException:\n def _later(x: _boom()):\n pass\n", + ), + ( + "a nested def with a raising return annotation", + "except BaseException:\n def _later() -> _boom():\n pass\n", + ), + # Set/dict displays HASH at construction, so inert elements are not + # enough to make the display inert. + ("an unhashable set literal", "except BaseException:\n s = {[1]}\n"), + ("an unhashable dict key", "except BaseException:\n d = {[1]: 2}\n"), + # AnnAssign evaluates its annotation at runtime without PEP 563. + ("a raising annotation", "except BaseException:\n x: _boom() = 1\n"), ], ) def test_guard_flags_an_arm_that_can_leave_without_a_raise_statement(shape, arm): @@ -688,9 +723,14 @@ def test_guard_flags_an_arm_that_can_leave_without_a_raise_statement(shape, arm) Asking "does this arm contain a ``raise``/``return`` STATEMENT?" excused a call that raises, an ``assert``, arithmetic that divides by zero, and a - plain logging call given a bad format argument. Each is a measured - 1-permit leak. The window check has always treated a call / attribute / - subscript / await as raise-capable; the arm check now agrees. + plain logging call given a bad format argument. The first whitelist then + excused a second family: an unpacking assignment target, a nested ``def`` + whose SIGNATURE (defaults / annotations) is evaluated at definition time, a + set/dict display that hashes an unhashable element, and a bare name load + that can be an unbound local. Every shape here is a measured 1-permit + leak. The window check has always treated a call / attribute / subscript / + await as raise-capable; the arm check now agrees, and additionally refuses + to excuse anything whose targets or signature can raise. """ guard = _load_guard() _sites, violations = guard.scan_source(_sibling_arm_source(arm), "new.py") @@ -704,12 +744,21 @@ def test_guard_flags_an_arm_that_can_leave_without_a_raise_statement(shape, arm) [ ("pass", "except BaseException:\n pass\n"), ("a bare constant", 'except BaseException:\n "swallowed"\n'), - ("an assignment between names", "except BaseException:\n x = surface\n"), ("a constant assignment", "except BaseException:\n x = 0\n"), + ("a constant tuple value", "except BaseException:\n x = (1, 2)\n"), + ("a hashable set literal", "except BaseException:\n s = {1, 2}\n"), + ( + "a constant annotated assignment", + 'except BaseException:\n x: "int" = 0\n', + ), ( "a nested def", "except BaseException:\n def _later():\n sem.release()\n", ), + ( + "a nested def with a constant default", + "except BaseException:\n def _later(x=0):\n sem.release()\n", + ), ], ) def test_guard_still_excuses_a_provably_inert_arm(shape, arm):