fix(guard): pre-yield permit check excused conditionally-exiting arms and credited unreachable releases - #873
Merged
Merged
Conversation
Collaborator
Author
FleetReview
Reviewed with 2 of 3 model families — openai unavailable. Confidence: 1/5 Findings
FleetReview provenance · models: C=claude-code-opus-5, D=grok-4.6, G=grok-4.6 · cost: $11.90 · duration: 32m 14s · rounds: 2 · files examined: 2 |
Collaborator
Author
FleetReview
Reviewed with 2 of 3 model families — openai unavailable. Confidence: 1/5 Findings
FleetReview provenance · models: C=claude-code-opus-5, D=grok-4.6, G=grok-4.6 · cost: $12.33 · duration: 38m 40s · rounds: 1 · files examined: 2 |
… 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.
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.
…ere" 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.
…ures and hashing
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.
Kyzcreig
force-pushed
the
fix/preyield-guard-reachability
branch
from
September 22, 2026 14:08
44901a1 to
8a86dbb
Compare
Collaborator
Author
FleetReview
Reviewed with 2 of 3 model families — openai unavailable. Confidence: 1/5 Findings
FleetReview provenance · models: C=claude-code-opus-5, D=grok-4.6, G=grok-4.6 · cost: $12.47 · duration: 37m 24s · rounds: 2 · files examined: 2 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
STACKED on #863 (base is
fix/preyield-guard-hardening, notmain). Rebase tomainafter #863 merges; the diff is one file plus its tests.Filed by Argus as t_9d51cb96 during review of #863. Not a regression from #863 — measured present on BOTH the merged #860 guard and the #863 candidate, so a pre-existing coverage gap, split out per the house rule rather than widening a verified branch.
Findings
1 —
_handler_exits()misclassified CONDITIONALLY-exiting arms as swallowing. It scannedreversed(handler.body)and returned True only on a BARE trailingRaise/Return, breaking on any other statement type. An arm exiting through a compound statement was dropped from the exiting set and excused from releasing. It escapes only in the SIBLING shape: a correctexcept BaseException:arm makes the chain look satisfied while the misclassified narrow arm is the one that actually runs.Replaced by
_handler_may_exit()— "can ANY path through this arm leave via raise/return?" Nested callables excluded (own lifecycle). Deliberately conservative: araisea nested handler swallows still counts as a possible exit, so such an arm is asked to release. False positive at worst; the other direction is a missed leak.2 —
_released_in()credited UNREACHABLE releases. It usedast.walk, so a textually-present release that can never run still satisfied the check. Now only statically-reachable statements count: nesteddef/lambdabodies,if False:branches, loops over an empty literal, and everything after an unconditional raise/return are skipped. Anything the compiler cannot settle still counts — a release under a runtime condition, in a non-empty loop, or in awithbody is unaffected (gated by its own test).3 — false POSITIVE on
except BaseException: passas the sole arm. 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. A swallow-only chain that does NOT cover BaseException is still flagged (gated).Verification
Independent RUNTIME oracle — real
asyncio.Semaphore, synchronous fault injected at the pre-yield log, measured permit delta. HEALTHY arm clean on all 13 shapes, so no degenerate fixtures. Reproduces Argus's matrix:The new guard's static verdict matches that measurement 1:1 across 18 probed shapes, 0 mismatches.
Mutation battery, 7/7 RED, controls 49 green at both ends (restores from git objects, refuses a dirty start):
M1 is the acceptance criterion: narrowing the reachability discriminator back to the current "bare trailing raise" rule turns the guard RED.
Disclosed: M4 and M5 SURVIVED the first battery — the dead-code truncation and the nested-callable exclusion were implemented but ungated by my fixtures. Fixed with three more shapes (F3/F5/F6), each measured on the oracle, in a second commit rather than shipping behaviour no test names.
Floors held. Repo sweep exit 0 over the same 2 enumerated sites — no live site has any of these shapes, so no in-repo fix was needed. Per-file invocation exit 0; empty-dir sweep still exit 2 (vacuity floor). 31 → 49 tests, all green.
ruff checkclean on both files.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.