fix(extractor): #278 — a -= releases only in a proven, unguarded teardown context - #293
Merged
Merged
Conversation
Two corpus/wpf cases pin the OWN001 false negative from issue #278, both heap-motivated by the SectorTS GTD leak (66% retained heap, ClrMD-proven): * subscription-param-guarded-unregister — ctor `+=`, the only `-=` inside `UnregisterEventHandlers(bool UnregOnlyGoodys)` behind `if (!UnregOnlyGoodys)`; the leaking callers pass `true`. before.cs must be OWN001; after.cs releases unconditionally in Dispose and must stay silent. * subscription-nonteardown-release — ctor `+=`, an unconditional `-=` in an arbitrary non-teardown method nobody is proven to call. before.cs must be OWN001; after.cs detaches in a handler wired to the class's own Unloaded lifecycle event and must stay silent (the recognised-teardown control). Under the shipped "any matching `-=` in the class = released" model both before.cs are silent — the red half of this pair. The case.own reductions already fail honestly (guard modelled as an early return past the release; non-teardown `-=` not modelled as a release). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MdYFKUaygHz1T9H1qJ7BqK
…rdown context Honour P-001/P-004 as written: a matching `target -= handler` credits the subscription's release ONLY when it is proven to run at the subscriber's end-of-life. The `unsub` collector now requires both: * a recognised TEARDOWN CONTEXT — Dispose/DisposeAsync/OnClosed/OnClosing/ OnUnloaded/OnFormClosed/OnFormClosing by name, a finalizer, a handler wired (`+=`, bare/`this.` receiver) to the class's OWN Closed/Closing/Unloaded/ FormClosed/FormClosing/Disposed lifecycle event (inline lambda handlers included), the XAML-wiring `*_Closed`/`*_Closing`/`*_Unloaded`/... naming convention, or any method such a context calls directly on `this` (intra-class fixpoint — deliberately NO whole-program call graph); * no parameter guard — a `-=` under a branch whose condition depends on a parameter of its enclosing method cannot be proven to run from the subscription site (SectorTS: `if (!UnregOnlyGoodys)`, callers pass true). The one canonical exception is a POSITIVE `if (disposing)` in `Dispose(bool)`; `if (!disposing)` still demotes. A `-=` in an arbitrary method, a ctor, or behind a caller-controlled flag now keeps the honest OWN001/OWN014 instead of silently swallowing the leak class — the #238 doctrine. Self-detaching handlers, old->new rotation and the timer `.Stop()` release are untouched. OwnIR schema, the Python core, and the S0/S2 fix pipeline are unchanged (the `--fix-candidates` teardown metadata keeps its own candidate scan by design). Evidence (docs/notes/own278-corpus-diff.md): corpus benchmark 40/44 -> 42/46 caught, 46/46 fixes clean, 0 FPs, every pre-existing row byte-identical; the samples diff flips exactly InpcAmbiguousTeardown + HandlerReassignedField (golden regenerated per tests/goldens/README.md, byte-parity gate passes); ScreenToGif sweep +4 findings, all one triaged shape (release only in a custom-named Destroy()); CsvHelper and the oracle push-target fixture byte-identical. The SectorTS reduction now flags GTD and PGC while KDT stays flagged and a Dispose-releasing sibling stays silent. Closes #278 acceptance rules 1-3; the call-graph reachability rule stays out of this slice by design. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MdYFKUaygHz1T9H1qJ7BqK
…n unproven release Four corpus/wpf cases pin the silent-exemption holes the #278 review found in the first slice's teardown model. Each before.cs is SILENT under that slice (verified against its extractor) and must be OWN001: * subscription-finalizer-release — the only `-=` in the finalizer. Circularly unreachable: the publisher's delegate keeps the subscriber reachable, so the finalizer never runs while the subscription is live. * subscription-xaml-name-only-release — the only `-=` in a method NAMED `Window_Closing` that nothing in code wires. A name is not wiring; a bare handler-shaped name may be stale dead code. after.cs pins both wired good forms (method group + inline lambda on `this.Closing`). * subscription-overload-conflated-cleanup — Dispose calls `Cleanup()`; the `-=` lives only in the uncalled `Cleanup(bool)`. A name-keyed closure conflates the overloads. * subscription-uncalled-local-function — the `-=` in a local function (and a lambda) DECLARED inside Dispose but never invoked. Declaration is not execution. after.cs pins the called-local-function good form. The previously name-carried control screentogif-loaded-subscription/after.cs now wires `Closing += Window_Closing` in code — the honest, provable form of the same fix (the real ScreenToGif attaches it in XAML, which the extractor never sees); the name-only shape moves to the new bad case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MdYFKUaygHz1T9H1qJ7BqK
…se teardown paths 1. A FINALIZER is no longer a teardown context (InTeardownContext: DestructorDeclarationSyntax => false). For a subscription leak the publisher's delegate keeps the subscriber reachable, so the finalizer never runs while the subscription is live — its `-=` can never break the hold. 2. The `*_Closed`/`*_Closing`/`*_Unloaded`/... NAME-SUFFIX exemption is removed. A XAML attach never reaches the extractor, so a name alone proves nothing (it may be stale dead code). A `Window_Closing`-style handler counts only when the class provably wires it in code; XAML-backed release stays a kept warning until a XAML-aware slice can credit the attach with evidence. 3. The intra-class teardown closure is SYMBOL-based (IMethodSymbol + SymbolEqualityComparer): an invocation extends the set only with the specific own method/local function it RESOLVES to, so `Dispose() => Cleanup();` credits exactly `Cleanup()` — never an uncalled `Cleanup(bool)` overload. Unresolved calls extend nothing. One narrow name fallback stays, for method-GROUP handlers wired to an UNRESOLVED lifecycle event (`Closing +=` under an unreferenced WPF Window base): a method group carries no argument list, so its name denotes the whole overload set — not the invocation-overload conflation above. 4. Nested callables no longer inherit their lexical teardown context. A local function counts only when the symbol closure proves a teardown CALLS it; a lambda only as the handler wired to a lifecycle event. The closure walks each callable's own body (never descending into nested function bodies), so an invocation inside an uncalled nested function extends nothing either. Timer .Stop(), rotation, self-detach, OwnIR schema, the core, and the fix pipeline are unchanged. Evidence (docs/notes/own278-corpus-diff.md, follow-up section): corpus benchmark 46/50 caught, 50/50 fixes clean, 0 FPs, all pre-existing rows unchanged; samples output byte-identical to slice 1; golden untouched; full suite/ruff/mypy green. 5-repo sweep: CsvHelper, Dapper, Newtonsoft.Json, RestSharp identical; ScreenToGif +9, all one triaged shape — real `-=` in `*_Closing` handlers wired ONLY in XAML, the deliberate rule-2 kept-warning trade-off and the first candidates for the XAML-aware slice. SectorTS reduction unchanged: GTD/PGC/KDT flagged, Dispose sibling silent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MdYFKUaygHz1T9H1qJ7BqK
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (43)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…rpus cases tests/run_tests.py went red in CI: the P-022 Rust-parity fixtures (tests/fixtures/cfg_parity.json, diag_parity.json) are generated over the whole corpus, so the six new corpus/wpf case.own files made the committed copies stale. Regenerated per the tests' own instruction (test_cfg_fixtures.py --write / test_diag_fixtures.py --write) — the diff is purely additive (the new cases' lowerings and (line, code) pairs) — and the Rust side replays them clean: cargo test passes, including full_parity_on_the_frozen_corpus, with no Rust changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MdYFKUaygHz1T9H1qJ7BqK
… ground a teardown corpus/wpf/subscription-ambiguous-overload-wiring pins the last silent- exemption residue: `Closing += Window_Closing` on an UNRESOLVED lifecycle event with TWO `Window_Closing` overloads, where the delegate-compatible overload detaches nothing and the `-=` sits in the never-attached sibling. The runtime delegate attaches exactly one overload — chosen by the event's delegate signature, the very information the extractor lacks — so the name fallback that credited every same-named method silently cleared OWN001 (verified silent at the previous head). before.cs must be OWN001; after.cs keeps a single `Window_Closing` holding the `-=` (unambiguous name) and must stay silent. CFG/diag parity fixtures regenerated for the new case.own (additive); the Rust side replays them clean with no Rust changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MdYFKUaygHz1T9H1qJ7BqK
…en unambiguous The unresolved-lifecycle-event fallback (`Closing += Window_Closing` where the event binds no definite symbol) now grounds a teardown ONLY when exactly one method with that name exists in the immediate class. Zero or 2+ same-named methods credit nothing — the delegate attaches exactly one overload, selected by the event's delegate signature the extractor is missing, so an ambiguous name may not let a `-=` in the never-attached overload clear OWN001 (the unresolved twin of the invocation-overload conflation closed previously). The former CandidateSymbols crediting is removed along with it: candidates of a failed method-group binding are the same ambiguous overload set by another name. The symbol-RESOLVED path is unchanged — a resolved event credits the delegate's exact target, even among overloads. Evidence (docs/notes/own278-corpus-diff.md, follow-up 2): benchmark 47/51 caught, 51/51 fixes clean, 0 FPs, prior rows unchanged; samples byte-identical; golden unchanged; suite/ruff/mypy, fix-candidates, S2 gates, and Rust parity (full_parity_on_the_frozen_corpus) green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MdYFKUaygHz1T9H1qJ7BqK
PhysShell
marked this pull request as ready for review
July 18, 2026 15:01
PhysShell
pushed a commit
that referenced
this pull request
Jul 25, 2026
…, enrollment split Four blockers plus the smaller fixes from the architecture review: 1. Migration plan no longer describes merged work as future. #258 is closed (spec/Bridge.md is the merged normative contract); #278 is closed by #293 and extended to WPF002 Stop() by #302 — those landed extractor predicates are named the current bounded implementation and the regression floor. Phase 2 is retargeted to the new post-cutover tracker #304 (summary-backed lifecycle release reachability); #278 stays the historical motivating incident, not a reusable implementation issue. 2. MVP guarded-effects policy defined: summaries preserve guards over simple boolean/null parameter predicates, callsite application substitutes statically-known constants, anything outside that vocabulary degrades to May/Unknown — never Must, never silence. Without this the summary layer loses to the landed predicates on the flagship Teardown(bool) case. input_contract semantics defined in the summary envelope. 3. Lifecycle reasoning split into two theorems: LifecycleEffect (release happens IF the root runs) vs LifecycleEnrollment (this instance provably reaches that root). Effect without enrollment is degraded/conditional, never clean — a perfect Dispose() nobody calls proves nothing. 4. Bridge-boundary authority table added: until parity+cutover #258/#259 own the boundary (MOS in own-bridge, byte-parity); after cutover a dedicated extraction slice per this proposal; wire schema, verdicts, and parity artifacts invariant across both. Also: OwnCFG claim corrected to intended-MIR-equivalent (today: plain succ edges, calls as instructions, AST re-export); Call-instruction vs Invoke-terminator model made explicit; evidence split into a single proof DAG vs a per-finding displayed witness. Refs #303 review; tracker: #304. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
PhysShell
pushed a commit
that referenced
this pull request
Jul 25, 2026
…cates Adversarial code reading of the landed teardown-context/guard predicates (enumerate the implicit axioms, attack each, verify against Program.cs). Full attack matrix — six confirmed hole families, eleven survived attacks, doctrine assessment, bounded fix directions — in docs/notes/teardown-predicate-adversarial-audit.md. Two P1 holes (silently swallowed leaks, the #238 doctrine violation) are pinned as red corpus fixtures; the CI corpus benchmark is the empirical arbiter (before.cs MISSED expected — recall floor is an absolute count; after.cs reuses proven-silent shapes): - subscription-teardown-early-return-guard: the SectorTS flag guard rewritten from `if (!flag) { -= }` to `if (flag) return; -=` — semantically identical, invisible to IsParamGuardedRelease (ancestors-only walk), while the symbol closure credits the helper regardless of argument values. The C# twin of the bridge's D7/INF-S3 defect. - subscription-disposing-else-branch-release: a `-=` in the ELSE of the canonical `if (disposing)` is credited (the exception classifies the parameter's use in the condition, never the branch holding the site) — yet it runs only on the finalizer path the extractor's own doctrine declares unreachable while the subscription is live. Both .own reductions are caught by the branch-sensitive core (wpf corpus 26/26) — extractor gaps, not core gaps. cfg/diag parity fixtures regenerated (additive); Rust parity green on the grown corpus. WPF002 Stop() shares the predicate, so the holes apply verbatim — timer twins land with the fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
This was referenced Jul 25, 2026
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
2 participants
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.
Что и зачем
Устраняет soundness false negative OWN001 из #278: любой matching
-=где угодно в классе засчитывался как гарантированный release — heap-proven leak SectorTSGTD(ctor+=,-=подif (!UnregOnlyGoodys)в non-teardown методе) молча глотался. Теперь-=кредитует release только в доказанном teardown-контексте (Dispose/DisposeAsync/OnClosed/Unloaded-style, wired-in-code lifecycle handler, либо метод/local function, который teardown-путь доказуемо вызывает — symbol-based fixpoint) и без parameter-guard (каноничный позитивныйif (disposing)— исключение). Follow-up 1 закрыл четыре silent-exemption пути: finalizer (недостижим, пока delegate держит subscriber), name-onlyWindow_Closingбез кодового wiring, name-keyed overload conflation (Cleanup()vsCleanup(bool)), lexical inheritance для невызванных local functions/lambda. Follow-up 2 закрыл unresolved lifecycle-handler overload ambiguity: при неразрешённом событии method-group имя кредитует teardown только при ровно одном собственномIMethodSymbolс этим именем (0 или 2+ — release не доказан, warning сохраняется);CandidateSymbols-ambiguity больше не используется как teardown evidence; symbol-resolved path точен без изменений. Изменение только в C# Roslyn extractor; OwnIR schema, Python/Rust core, autofix pipeline, timer.Stop(), OwnAudit не тронуты.CODE SCOPE — ACCEPTED / CLOSED / FROZEN (head
18000d0): новых code changes не будет, кроме исправления фактической ошибки, найденной CI или review.Тип изменения
Как проверено
python tests/run_tests.pyruff check .иmypypython scripts/<...>.py --selftest)scripts/benchmark.py: 40/44 (до) → 47/51 caught · 51/51 fixes clean · 0 FP; все прежние строки байт-в-байтfrontend/roslyn/samples(только два intended-флипа вFixCandidatesSampleиз slice 1, golden перегенерирован поtests/goldens/README.md; follow-up 1 и 2 — byte-identical),check_fix_candidates_facts.py, weak-subscribe checks, S0 Part B,tests/gate_regressions.shcargo test, включаяfull_parity_on_the_frozen_corpus), Rust-код не менялся-=в*_Closing, wired только в XAML — осознанный kept-warning trade-off, кандидаты XAML-aware slice). Детали:docs/notes/own278-corpus-diff.mdСвязанные issue
Closes #278. Refs #238, #240, #270, PhysShell/OwnAudit#13.
Чеклист
feat:,fix:,docs:…)Draft — не мержить. Merge заблокирован тремя внешними gates:
runtime-only;18000d0поSTS_new/SectorTS.После gates: приложить evidence к PR → дождаться полного зелёного CI → ready-for-review → review → merge → повторить OwnAudit GTD scenario и доказать переход
runtime-only → confirmed.🤖 Generated with Claude Code
https://claude.ai/code/session_01MdYFKUaygHz1T9H1qJ7BqK