fix(guard): pre-yield permit check missed line-adjacent leaks and partial multi-permit releases - #863
Merged
Merged
Conversation
…tial multi-permit releases FleetReview coverage gaps on PR #860's new class guard scripts/check_preyield_permit_release.py. The landed fix is correct; these are holes in the enforcement mechanism, split out rather than widening a verified branch. Reproduced against the landed guard before changing it (34-case matrix; 7 cases RED pre-fix, 34/34 green after): 1. The window was LINE-based (`acquire.lineno < n.lineno < yield.lineno`), so `await sem.acquire(); _LOG.info(...)` and `_LOG.info(...); yield` both fell outside it and passed clean. Now positional (lineno, col_offset), with a node that CONTAINS the yield excluded -- the yield runs first, so an expression-yield's wrapper is not an offender. NOTE: the finding as filed ("yield expression skipped") does not reproduce; `x = yield`, `await handle((yield))`, tuple/ifexp forms all already fired. Line granularity was the actual gap. 2. A release was matched by METHOD NAME, so at the two-semaphore site gateway/turn_admission.py (total + internal) an except arm releasing only one of them satisfied the check. Release is now matched by RECEIVER and every object acquired before the yield must come back. Independent oracle on real source: deleting only the `self.internal` release from TurnAdmission.slot makes the new guard FIRE (exit 1) while the landed one PASSES (exit 0) -- the escape, reproduced. This also closes the wrong-semaphore false negative disclosed as ISSUE-2 on #860; the docstring boundary section is updated to match measurement rather than restating the old claim. 3. Any single `except BaseException` arm that released satisfied the try, so a sibling `except ValueError:` that re-raised bare leaked whenever it matched. Now every EXITING arm must release; an arm that swallows falls through to the yield still holding the permit and is correctly not asked to. 4. Zero-sites exit 2 fired on per-FILE invocations, making the documented `[paths...]` form unusable from pre-commit (most files hold no sites). Vacuity is now exit 2 only for a directory SWEEP, where it does mean the discovery shape broke; the sweep test still pins that. Gate module (hermes_cli/session_db_heavy_gate.py), same review: 5. `_record_stats` ran FIRST in the pre-yield window, so an abort after it left acquired_count/queued_count/queue_wait_seconds_total counting a slot the caller never received -- overstating served load exactly when the gate was faulting. Moved LAST inside the guarded window. Measured with an injected fault: acquired_count 1 -> 0 on abort, still 1 on a granted admission, permit 2/2 throughout. 6. The per-admission cap read went through `load_config()` (full expansion + copy.deepcopy) on the event loop. Swapped to the repo's existing read-only idiom `read_raw_config_readonly()`, same (mtime_ns, size) freshness key: 445.4 us/call -> 7.5 us/call against the live 22 KB config.yaml (2000-call loop, py3.11.15), value unchanged at 2. Verified: ruff clean on all 3 files; 76 passed (this suite + test_turn_concurrency + test_web_server_sessiondb_eventloop); tree sweep exit 0, 2 sites enumerated. Every new test mutation-proven RED -- guard reverted to a9dd9ba = 9 failures; _record_stats moved back = 1 failure; load_config restored = 1 failure; control 31 passed with both files cmp-verified restored.
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: $17.21 · duration: 57m 15s · rounds: 2 · files examined: 3 |
Kyzcreig
added a commit
that referenced
this pull request
Sep 22, 2026
…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.
Collaborator
Author
|
🤖 merged-by: apollo · lane: t_892f48ad · gate: BYPASS: FR router degraded (t_8d3c4eeb); argus artifact review stands in · why: argus r1 APPROVED (artifact lens): guard hardening for the 6 FleetReview findings on #860 (expression-form yield, partial multi-permit release, sibling except arm, zero-sites exit-2, pre-yield counter inflation, sync config I/O). CI green, MERGEABLE/CLEAN. |
Kyzcreig
added a commit
that referenced
this pull request
Sep 22, 2026
… 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.
Kyzcreig
added a commit
that referenced
this pull request
Sep 22, 2026
…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.
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.
Follow-up to #860 (merged as
6319e227); rebased ontomainafter that PR landed mid-run. Closes the FleetReview coverage gaps on that PR's new class guardscripts/check_preyield_permit_release.py, split out per house rule rather than widening a verified branch.Reproduced before fixing
A 34-case matrix was run against the landed guard (
a9dd9bae) first. 7 cases RED pre-fix, 34/34 green after.exceptarm skipped (P2)[paths...](P2)1. The real gap is line granularity, not yield form
The filed P1 says expression-form yields are skipped. Measured:
x = yield,acc += yield,handle((yield)),await handle((yield)),pair = ((yield), 1)and the ifexp form all already fired on the landed guard.The actual hole was the window bound
acquire.lineno < n.lineno < yield.lineno- a semicolon-joined leak on either boundary line escaped:Now positional
(lineno, col_offset), with any node that contains the yield excluded - the yield runs first, so an expression-yield's wrapper is not an offender. Both directions are parametrized over 7 yield forms.2. Partial multi-permit release - verified on real source
Release was matched by method name, so at the two-semaphore site
gateway/turn_admission.py(self.total+self.internal) an except arm releasing only one satisfied the guard. Now matched by receiver, and every object acquired before the yield must come back.Independent oracle, real source not a fixture - delete only the
self.internalrelease fromTurnAdmission.slot:This also closes the wrong-semaphore false negative disclosed as ISSUE-2 on #860. The docstring's DOES NOT COVER section now states measured behaviour rather than repeating the old claim.
3. Sibling except arm
Any single releasing
except BaseExceptionsatisfied the try, so a narrower sibling that re-raised bare leaked whenever it matched. Every exiting arm must now release. An arm that swallows falls through to the yield still legitimately holding the permit and is correctly not asked to release (demanding it would be a double release).4. Per-file invocation
Zero sites is exit 2 only for a directory sweep, where it genuinely means discovery broke (that test still pins it). A named file with no sites is exit 0; a named file that violates is still exit 1.
5-6. Gate module
_record_statsran first in the pre-yield window, so an abort after it counted a slot the caller never received - overstating served load exactly when the gate was faulting. Moved last inside the guarded window. With an injected fault:acquired_count1 -> 0 on abort, still 1 on a granted admission, permit 2/2 throughout.The per-admission cap read went through
load_config()(full expansion +copy.deepcopy) on the event loop. Swapped to the repo's existing read-only idiomread_raw_config_readonly()- same(mtime_ns, size)freshness key, so a cost change not a behaviour change:Cap value unchanged at 2. Stated trade-off: the raw read skips
${VAR}expansion; this is an int cap, no live config spells it that way, and a non-int already falls back via_coerce_positive_int.Verification
ruff checkclean on all 3 filestests/gateway/test_turn_concurrency.py+tests/test_web_server_sessiondb_eventloop.pya9dd9bae-> 9 failures;_record_statsmoved back to first -> 1 failure;load_configrestored -> 1 failure; control 31 passed with both filescmp-verified restoredReviewer note: the
load_configabsence assertion is AST-based (imports + calls), not a text grep - the docstring legitimately namesload_configto explain the choice.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.