fix: close 4 precision gaps from the oracle sweep (#220, #222, #223, #224)#231
Conversation
…224) Four small, independently-verified extractor fixes surfaced by the issue positive sample (silenced) and at least one adversarial negative control (still flagged) so precision on the general case can't regress silently: - #223: a curated allowlist (exactly one entry) for System.Windows.Input.CommandManager.RequerySuggested, a WPF static event implemented over weak references — a subscriber is never pinned by it, so never detaching is the normal, safe idiom. Every other static-source subscription still raises OWN014 unchanged. - #224: a subscribed handler that unsubscribes ITSELF inside its own body (a self-detaching one-shot handler) is now recognised as bounded — no external `-=` required. Matched by the handler's own method symbol and the subscribed event's name, so a same-named handler on an unrelated subscription, or a self-detach against the wrong event, cannot falsely suppress a real leak. - #220: `using (field = new T())` — the field IS the `using` acquisition target — is now recognised as releasing the field at scope exit, closing the sibling gap next to the already-handled `using (existingLocal)` local form. A field assigned the same way but outside any `using` still leaks. - #222: the self-owned-template-part exemption (GetTemplateChild/ Template.FindName) now covers a template part captured as a plain local variable or an `is T x` pattern variable, not only a field assignment. IsSelfOwnedSource already accepted a local receiver by name — it only needed template-part local names folded into the self-owned set. Every fix is scoped to the extractor (frontend/roslyn/OwnSharp.Extractor); the Python core and OwnIR schema are untouched, no new fact fields, no OWNIR_VERSION bump. Verified locally end-to-end (facts extracted with a temporary .NET 8 SDK install, run through the exact updated CI script) — the 4 positive samples are silent, all 6 negative controls (including the pre-existing 32-sample regression suite) still raise their findings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D6Naf8CcezjCbikueKdeYv
|
Warning Review limit reached
Next review available in: 14 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 (2)
📝 WalkthroughWalkthroughThe Roslyn extractor now recognizes weak static events, self-detaching handlers, using-scoped field disposal, and template-part locals. New samples and CI assertions validate positive and negative classifications for these patterns. ChangesLeak detection precision
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CIWorkflow
participant OwnSharpExtractor
participant OwnIR
participant AssertionScript
CIWorkflow->>OwnSharpExtractor: process Roslyn sample inputs
OwnSharpExtractor->>OwnIR: emit ownership and release facts
AssertionScript->>OwnIR: query expected OWN001 and OWN014 findings
OwnIR-->>AssertionScript: return matching facts
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| public OrdinaryStaticSubscriber() | ||
| { | ||
| handler = OnSomethingChanged; | ||
| OtherStaticSource.SomethingChanged += handler; |
| public NonDetachingSubscriber(PopupLike content) | ||
| { | ||
| this.content = content; | ||
| content.Closed += OnClosed; |
| public WrongEventDetachSubscriber(PopupLike content) | ||
| { | ||
| this.content = content; | ||
| content.Closed += OnClosed; |
| // "any field assignment from `new` is a release." | ||
| public sealed class LeakyAssignerLike | ||
| { | ||
| private CancellationTokenSource? cts; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 412ea0e311
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| foreach (var declr in cls.DescendantNodes().OfType<VariableDeclaratorSyntax>()) | ||
| if (declr.Initializer?.Value is { } declInit | ||
| && IsTemplatePartFetch(declInit) | ||
| && model.GetDeclaredSymbol(declr) is ILocalSymbol declLocal) | ||
| selfOwned.Add(declLocal.Name); |
There was a problem hiding this comment.
When a class contains a template-fetch local named, for example, button, this adds only that name to a class-wide set. Any later ILocalSymbol named button in another method/scope then satisfies IsSelfOwnedSource via owned.Contains(recv.Name), so an injected publisher aliased with the same local name is treated as self-owned and skipped instead of reporting OWN001. Scope the exemption to the actual local/pattern symbol rather than the reusable identifier.
Useful? React with 👍 / 👎.
| || innerLeft.Name.Identifier.Text != ev.Name) | ||
| continue; | ||
| var innerHandlerSym = bodyModel.GetSymbolInfo(NormalizeHandler(inner.Right)).Symbol; | ||
| if (SymbolEqualityComparer.Default.Equals(innerHandlerSym, handlerMethod)) | ||
| return true; |
There was a problem hiding this comment.
For a handler subscribed as content.Closed += OnClosed, a body that executes other.Closed -= OnClosed returns true here because only the event name and handler method are compared. The original publisher still holds the delegate in that scenario, but the emitted fact is marked released, hiding the OWN001/OWN014 finding for any different source exposing the same event name.
Useful? React with 👍 / 👎.
… checks Two real precision bugs caught by Codex's review of PR #231, both fixed and covered by a dedicated regression sample: - Template-part locals (issue #222) were folded into `selfOwned` by NAME, the same set used for fields. Field names are unique per class, so name-based lookup was sound there; a LOCAL's name is only unique within its own method, so an unrelated same-named local aliasing an injected source in a different method could wrongly borrow the exemption. Fixed by tracking template-part locals in a separate symbol-keyed set (`selfOwnedLocals`, SymbolEqualityComparer) that IsSelfOwnedSource checks by SYMBOL, never by name — fields are unaffected. - HandlerSelfDetaches (issue #224) only compared the event NAME and the handler METHOD symbol, never the `-=`'s RECEIVER — so a handler that unsubscribed an unrelated object exposing an event of the same name would be wrongly credited as self-detaching, hiding a real leak on the actual subscribed source. Fixed by requiring the `-=`'s receiver (after stripping cast/`as`/parens/`!`) to resolve to the handler's own `sender` parameter — the only expression inside the handler provably referring to the actual runtime event source. Also fixes an infinite loop in the new `UnwrapToBase` helper introduced for the second fix: the default arm of the unwrap switch reassigned `e = e` and never returned, so any handler body containing a cast/`as`/ parens/`!` expression that bottomed out at a plain identifier (i.e. every realistic case) hung the extractor forever. Caught locally before push by actually running the extractor against the sample fixtures — a `timeout`- wrapped, single-file bisection isolated it to this exact function. Extends the same 2 sample files with regression controls reproducing each bug directly (a same-named unrelated local in a different method; a self-detach off an unrelated receiver instead of `sender`), wired into the CI assertions alongside the existing ones. Re-verified end to end with the local .NET 8 SDK: full sample suite (35 files) + updated CI script, exit 0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D6Naf8CcezjCbikueKdeYv
| { | ||
| this.content = content; | ||
| this.other = other; | ||
| content.Closed += OnClosed; |
| public void Wire() | ||
| { | ||
| ButtonStub local = externalButton; // aliases an INJECTED field, not a template fetch | ||
| local.Click += OnClick; |
| public void WireInjected() | ||
| { | ||
| ButtonStub sameName = injectedButton; | ||
| sameName.Click += OnInjectedClick; |
Same-tail collision as 2ad7dc9: main's #218 rotation block (Program.cs released-disjunct + ci.yml sample/assertions) and this branch's four quick-win blocks appended to the same places. Kept both: both released disjuncts (self-detach #224 + rotation #218), both sample lists, main's merged OK line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LsWw4Ay8KLTHFom1HvRu3U
Lines starting with "#201)" / "#230/#231" after a soft wrap read as malformed ATX headings to markdownlint. No content change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D6Naf8CcezjCbikueKdeYv
3 participants
Что и зачем
Закрывает 4 мелких precision-gap issue из oracle-sweep (#201) одним PR: #223 (allowlist для
CommandManager.RequerySuggested), #220 (using (field = new T())как release поля), #222 (self-owned-template-part exemption для локалов/pattern-var), #224 (self-detaching handler). Все четыре — точечные правки в экстракторе (frontend/roslyn/OwnSharp.Extractor/Program.cs), ядро (ownlang) не тронуто.Тип изменения
Как проверено
python tests/run_tests.pyruff check .иmypypython scripts/<...>.py --selftest)В окружении временно установлен .NET 8 SDK (официальный
dotnet-install.sh, сам SDK не коммитится) — экстрактор собран и прогнан локально по всем новым сэмплам плюс полному существующему набору (32 файла + новыйOwnIgnoreSample.csпосле ребейза на актуальныйmain). Точный текст обновлённого CI-шага «Check facts through the core» прогнан против реального вывода — все ассерты (старые и новые) проходят,exit 0. Вывод ниже.Связанные issue
Closes #220. Closes #222. Closes #223. Closes #224.
Чеклист
frontend/roslyn/samples/, у каждого позитивный кейс (перестаёт флагаться) и хотя бы один негативный контроль (обязан остаться флагнутым), плюс ассерты в обе стороны в CI-джобе «C# leak extractor»docs/notes/oracle-sweep-2026-07-10.mdиfield-notes-patterns.md(записи 14/16/17/18); отдельных правок документации в этом PR не требуетсяfeat:,fix:,docs:…)Детали по каждому фиксу
#223 —
CommandManager.RequerySuggestedallowlist. Ровно одна запись (IsWeakReferencedStaticEvent, namespace+type+name), без общего ослабления static-source тира. Сэмпл:RequerySuggestedAllowlistSample.cs— позитив (ImeSupportLike, молчит) + негатив (OrdinaryStaticSubscriberна НЕ-allowlisted статик-событии — по-прежнемуOWN014).#224 — self-detaching handler. Поиск
-=в теле самого подписанного хендлера (тот же символ метода, то же имя события — не просто текстовое совпадение). Сэмпл:SelfDetachingHandlerSample.cs— позитив (DropDownButtonLike, молчит) + два негатива (не детачится вовсе; детачится, но от ДРУГОГО события — оба по-прежнемуOWN001).#220 —
using (field = new T()). Расширение существующегоusing-lowering (using (existingLocal)) на acquisition-присваивание полю. Сэмпл:UsingFieldAcquisitionSample.cs— позитив (HashCheckerLike, молчит) + негатив (то же поле, то же присваивание, но БЕЗusing— по-прежнемуOWN001).#222 — template-part exemption для локалов/pattern-var.
IsSelfOwnedSourceуже принималILocalSymbolпо имени — не хватало добавления имён template-part-локалов вselfOwned. Сэмпл:TemplatePartLocalCaptureSample.cs— два позитива (is T xчерезGetTemplateChild; обычный локал черезTemplate.FindName, молчат) + негатив (локал-алиас ИНЖЕКТИРОВАННОГО поля, не template-fetch — по-прежнемуOWN001).Локальный прогон (dotnet 8.0.422, временный) — вывод обновлённого шага "Check facts through the core"
Ни
ImeSupportLike,DropDownButtonLike,HashCheckerLike,MetroWindowLike,OverloadViewerLikeв выводе не появляются (молчат, как и требуется) — а весь пред-существующий 32-файловый (33 после мержа сOwnIgnoreSample.cs) регресс-набор проходит без изменений.Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
usingblocks.Tests