fix(extractor): oracle-driven FP hardening — 3 precision fixes + FP baseline#156
Merged
Conversation
… for main) The oracle workflow_dispatch is blocked for the automation token (403). Point the existing dev-only push fallback at this branch so a corpus/oracle-target.txt bump runs the cross-tool oracle. Dev-loop only — not to be merged to main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Remove the claude/mos-ownership-summary branch from the oracle.yml push filter and restore corpus/oracle-target.txt to the systemevents-console fixture. These were temporary edits to dispatch the cross-tool oracle on five NuGet targets (Newtonsoft.Json, CsvHelper, serilog, NLog, protobuf-net); the runs are done, so the dev scaffolding comes back out. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Triaged all 20 own-only findings from the 2026-06-27 cross-tool oracle run (Newtonsoft.Json, CsvHelper, serilog, NLog, protobuf-net) against each target's real source. 12 are false positives, 4 are true-but-some-benign catches the oracle leak query can't express (kept visible), 2 are test/sample noise. None came from the new owned-API recognition — it added no noise on third-party code. To keep re-runs actionable: - oracle_compare.py gains --baseline: an allowlist of verified FPs, matched by (repo, basename, OWN code, message-substring) NOT line number (targets drift at HEAD), moved into a separate 'Known FP (baselined)' section so the triage queue surfaces only new findings. Selftest covers the loader, name-not-line match key, repo scoping, '*' wildcard, and render (30/30). - _is_test_path gains a safe 'unittest' substring rule so camelCase test projects (protobuf's BuildToolsUnitTests) are excluded as non-product. - corpus/oracle-fp-baseline.txt seeds 13 rules (covering 14 findings) with a per-entry root cause; the workflow passes it automatically. - docs/notes/oracle-known-fps.md records every verdict and the four analyzer limitations behind the FPs, each naming the fix that would retire its entry. True positives (serilog CTS, NLog benign field leaks) are deliberately not suppressed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
A field released through a 'drain and dispose' sink — a first-party extension method that disposes its receiver, like NLog's WaitForDispose(this Timer, TimeSpan) — was reported as an OWN001 owned-field leak, because the disposal scan only keys on a literal .Dispose()/.Close()/.DisposeAsync() on the field. Credit field.M(...) as a release when M is a first-party extension method whose receiver it disposes. CallReleasesReceiver reuses the existing ConsumesParam on the reduced extension method's receiver parameter, so the release is proved by inspecting M's real body (following first-party forwarding, cycle-guarded, IDisposable-only) — never guessed from the name. Scope is extension methods only; an instance method disposing its own 'this' is left out. Clears the NLog timer FPs where the sink is called on the field directly or through a 'var t = _field;' alias (AsyncTaskTarget, AsyncTargetWrapper, BufferingTargetWrapper). TimeoutContinuation, which disposes the result of Interlocked.Exchange(ref _timer, null), stays baselined until ref-exchange alias tracking lands. New corpus fixture field-dispose-via-helper proves it: before.cs leaks the Timer field (OWN001 caught), after.cs releases it via the sink and must be silent — the benchmark's after-clean gate is the end-to-end check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
…er FPs Temporary, dev-only: point the push-triggered oracle at NLog/NLog and disable the four direct/alias timer baseline entries that the CallReleasesReceiver fix (6f4818c) should now clear. If they stay out of own-only on the live run, the fix cleared them for real and the entries are deleted; if they resurface, they are restored. TimeoutContinuation stays baselined — its Interlocked.Exchange ref-alias is out of the fix's scope. Reverted after the run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
…line Live NLog oracle re-run with the four WaitForDispose timer entries disabled confirms the CallReleasesReceiver fix cleared them at the source: leak-class own-only total 8 -> 4, and the four timers (AsyncTaskTarget _taskTimeoutTimer/ _lazyWriterTimer, AsyncTargetWrapper _lazyWriterTimer, BufferingTargetWrapper _flushTimer) appear in neither own-only nor the baselined section. The remaining NLog own-only is just the three benign-but-real undisposed fields (XmlParser _xmlSource, FileTarget _reusable*Stream), with only TimeoutContinuation _timeoutTimer baselined (its Interlocked.Exchange ref-alias is out of scope). - Delete the four now-redundant baseline entries (baseline: 13 -> 9 rules). - Document the fix landed in oracle-known-fps.md (root-cause #1 mostly-fixed). - Restore the dev scaffolding: oracle-target.txt back to the systemevents-console fixture, remove the branch from the oracle.yml push filter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Clears the last NLog WaitForDispose timer FP — TimeoutContinuation, which
releases its timer via the atomic detach-and-dispose idiom:
var current = Interlocked.Exchange(ref _timeoutTimer, null);
current?.WaitForDispose(TimeSpan.Zero);
Exchange atomically nulls the field and RETURNS the object it used to own, so
the local aliases the field's just-detached disposable. RefExchangeNulledField
binds such a local to the field in aliasToField (alongside the plain
'var x = _field;' alias), so the existing null-conditional + CallReleasesReceiver
scan credits the field as released. Restricted to a null/default replacement —
the unambiguous teardown; an exchange installing a new non-null value re-arms the
field with an object the syntactic scan can't follow, so it is declined
(precision-first: never hide a real leak).
New corpus fixture field-dispose-via-exchange (faithful to TimeoutContinuation)
proves it: before.cs leaks (OWN001), after.cs releases via the idiom and is
silent. All five NLog timer FPs are now fixed at the source; the NLog FP baseline
is empty. Re-pointing the oracle at NLog to confirm own-only 4 -> 3 on real code.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
…re scaffolding Live NLog oracle re-run confirms the Interlocked.Exchange teardown fix clears the last timer FP: Own.NET leak-class total 4 -> 3, own-only is now just the three genuine benign-but-real undisposed fields (XmlParser _xmlSource, FileTarget _reusable*Stream), TimeoutContinuation _timeoutTimer absent from both own-only and the baselined section, and the report has no Known-FP section at all (NLog baseline empty). Restore the dev scaffolding: oracle-target.txt back to the systemevents-console fixture, remove the branch from the oracle.yml push filter. Full arc: NLog FP baseline 5 -> 0, own-only 8 -> 3, every WaitForDispose timer FP closed at the source (4 by CallReleasesReceiver, 1 by RefExchangeNulledField). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
A subscription whose event SOURCE is reached through a get-only property the class
owns — this.Child.Event += this-capturing-handler, where Child is a property over a
constructed field — is the same collectable self-cycle as subscribing on the owned
field directly, not a leak. The self-owned exemption already covered this/fields/
locals the class constructs but not the PROPERTY receiver, so protobuf-net's
CommandLineOptions (XsltOptions, a get-only property over a constructed
XsltArgumentList, with a this-capturing handler) was falsely warned as an injected
subscription leak.
PropertyReturnsOwnedMember recognises a get-only property whose value the class
owns: an auto-property '{ get; } = new T()', or a getter returning a constructed
member (=> _owned / get => _owned / get { return _owned; }). Get-only is required —
a settable property could be reassigned to an injected, longer-lived object, which
we cannot prove bounded, so it falls through to the honest 'injected' warning
(precision-first: never silently drop a real leak).
New corpus fixture subscription-self-owned-property: before.cs subscribes to an
injected bus (real OWN001 leak, caught), after.cs to a self-owned property (silent).
Newtonsoft's serializer.Error (serializer = Create() that escapes, handler is a
param's delegate) stays baselined: proving it non-leak needs real lifetime
modelling, so the 'may outlive' warning is honest, not a clear FP.
Re-pointing the oracle at protobuf to confirm the XsltOptions FP clears.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
…restore scaffolding Live protobuf oracle re-run confirms PropertyReturnsOwnedMember clears the CommandLineOptions XsltMessageEncountered self-cycle FP at the source: own-only 0, the finding absent from both own-only and the baselined section. Delete its baseline entry (protobuf baseline 5 -> 4 findings) and document the fix landed (root-cause #3 PARTLY FIXED). Restore the dev scaffolding: oracle-target.txt back to the systemevents-console fixture, remove the branch from the oracle.yml push filter. Newtonsoft serializer.Error stays baselined as an honest 'may outlive' warning (its source escapes and the handler is a parameter's delegate — non-leak is unprovable without lifetime modelling). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds extractor rules for owned get-only properties, extension-method disposal, and ChangesExtractor: New Disposal & Subscription Recognition Rules
Corpus Fixtures for New Patterns
Oracle FP Baseline Workflow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: edbf203184
ℹ️ 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".
Comment on lines
+495
to
+496
| return recv is IPropertySymbol { IsStatic: false } p | ||
| && PropertyReturnsOwnedMember(p, owned, model); |
There was a problem hiding this comment.
When the subscription source is reached through another instance, e.g. other.Channel.Changed += ..., recv still resolves to the get-only Channel property and PropertyReturnsOwnedMember can return true because that property owns other's field. That suppresses a real leak: other.Channel may outlive this subscriber and retain the handler. Please only apply this property exemption for bare/this property accesses, or otherwise prove the property receiver is self-owned.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@corpus/real-world/field-dispose-via-helper/notes.md`:
- Around line 34-40: Update the caveat in the notes to reflect the new
ref-exchange alias tracking support: the current text in the notes section still
says the `Interlocked.Exchange(ref _timer, null)` teardown is not tracked and
remains in the oracle FP baseline until tracking lands. Revise that wording to
match the new behavior introduced by the `field-dispose-via-exchange` fixture
and the `TimeoutContinuation`/`Interlocked.Exchange` path, removing the obsolete
limitation and keeping the scope note accurate.
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 529-532: The self-ownership check in Program.cs is using rsym.Name
in the returned-member test, which can incorrectly match an unrelated field or
property with the same name. Update the logic in the symbol analysis around
returned/owned members to compare the actual IFieldSymbol or IPropertySymbol
against the owned members of the analyzed containing type, rather than using
names. Use the existing symbols rsym, pm, and the owning type context in the
extractor flow to ensure only members declared on the analyzed type can satisfy
the ownership check.
- Around line 2582-2586: The argument check in the Interlocked.Exchange pattern
matcher only handles the `default` literal, so `default(T)` is missed. Update
the condition in `Program`’s matching logic to also accept
`DefaultExpressionSyntax` alongside `SyntaxKind.DefaultLiteralExpression`, while
keeping the existing `ref` and null/default handling intact.
In `@scripts/oracle_compare.py`:
- Around line 252-269: The _load_baseline parser currently accepts empty message
substrings, which makes BaselineRule matching behave like a wildcard because an
empty string matches every message. Update the parsing logic in _load_baseline
so malformed or empty substrings are rejected before creating a BaselineRule,
and only append rules when the message-substring field is non-empty after
trimming.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bcde37f5-4ca9-470c-9802-f0b6a1b4ccc8
📒 Files selected for processing (21)
.github/workflows/oracle.ymlcorpus/oracle-fp-baseline.txtcorpus/real-world/field-dispose-via-exchange/after.cscorpus/real-world/field-dispose-via-exchange/before.cscorpus/real-world/field-dispose-via-exchange/case.owncorpus/real-world/field-dispose-via-exchange/expected-diagnostics.txtcorpus/real-world/field-dispose-via-exchange/notes.mdcorpus/real-world/field-dispose-via-helper/after.cscorpus/real-world/field-dispose-via-helper/before.cscorpus/real-world/field-dispose-via-helper/case.owncorpus/real-world/field-dispose-via-helper/expected-diagnostics.txtcorpus/real-world/field-dispose-via-helper/notes.mdcorpus/real-world/subscription-self-owned-property/after.cscorpus/real-world/subscription-self-owned-property/before.cscorpus/real-world/subscription-self-owned-property/case.owncorpus/real-world/subscription-self-owned-property/expected-diagnostics.txtcorpus/real-world/subscription-self-owned-property/notes.mddocs/notes/oracle-known-fps.mddocs/notes/oracle.mdfrontend/roslyn/OwnSharp.Extractor/Program.csscripts/oracle_compare.py
) Addresses the Codex/CodeRabbit review on #156: - IsSelfOwnedSource: gate the get-only-property exemption to a THIS/bare access (`Channel` / `this.Channel`). `other.Channel.Event += h` reaches another instance's property, which may outlive this subscriber, so it stays flagged (Codex P2). - PropertyReturnsOwnedMember: require the returned member's CONTAINING TYPE to be the analyzed class, not just a name match — a same-named field on another type no longer satisfies ownership (CodeRabbit, major). - RefExchangeNulledField: also accept `default(T)` (DefaultExpressionSyntax), not only the `default` literal — `Interlocked.Exchange(ref _f, default(Timer))` was missed (CodeRabbit, minor). - oracle_compare _load_baseline: reject an empty message-substring (it would make `substr in message` a wildcard a typo could turn into a blanket suppressor), and preserve `|` in the reason via join (CodeRabbit, major). Selftest extended (31 checks). - field-dispose-via-helper/notes.md: drop the obsolete caveat — the Interlocked.Exchange teardown is now covered by the field-dispose-via-exchange fixture, not baselined (CodeRabbit, minor). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
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.
Что и зачем
Прогнал кросс-тул оракул (Own.NET vs CodeQL vs Infer#) на 5 топ-NuGet библиотеках, оттриажил все own-only находки против реального исходника и устранил ложные срабатывания: 3 точечных фикса экстрактора закрывают целые классы FP прямо в коде (каждый подтверждён повторным прогоном оракула на реальном репо), а проверенные-но-непочинибельные FP уезжают в именованный baseline, чтобы повторные прогоны показывали только новое. Recall-сторону тоже проверил — настоящих пропущенных продакшн-течей не нашлось (oracle-only = CodeQL over-reporting на escape/no-op + sample-код).
Тип изменения
Как проверено
python tests/run_tests.py(зелёный; corpus 27/27 reductions)ruff check .иmypy(в CI: lint job — pass)python scripts/oracle_compare.py --selftest30/30,python scripts/benchmark.py --selftest)C# проверяется в CI (локально dotnet нет): golden C#, leak extractor, corpus benchmark 32/33 caught · 34/34 fixes clean · 0 FP. Сверх того, каждый из 3 фиксов подтверждён живым прогоном оракула на реальном коде: NLog own-only 8 → 3, protobuf own-only → 0.
Связанные issue
Нет. Продолжение oracle-методологии (
docs/notes/oracle.md).Чеклист
docs/notes/oracle-known-fps.md(новый),oracle.mdfix:/oracle:scoped)Подробнее
3 фикса экстрактора (precision, все аддитивны — recall защищён floor'ом)
CallReleasesReceiver— поле, освобождаемое через first-party extension-метод, дизпозящий свой ресивер (NLogWaitForDispose(this Timer)), переиспользуяConsumesParam. Чистит 4 NLog timer FP (прямой/alias-ресивер).RefExchangeNulledField— teardown-идиомаvar t = Interlocked.Exchange(ref _f, null); t?.Dispose()(NLogTimeoutContinuation). Закрывает 5-й таймер → NLog FP baseline 5 → 0, own-only 8 → 3 (остаток — настоящие benign-поля, оставлены видимыми).PropertyReturnsOwnedMember— self-cycle подписки через get-only property над owned-полем (this.OwnedChild.Event += handler; protobufXsltOptions). get-only обязательно (settable можно подменить на injected). protobuf own-only → 0.Оракул-инфраструктура (
scripts/oracle_compare.py)--baseline: allowlist проверенных FP, матчинг по(repo, basename, OWN-code, msg-substring)— не по строке (переживает дрейф HEAD), отдельная секция «Known FP (baselined)»._is_test_path: ловит camelCase*UnitTests*проекты.corpus/oracle-fp-baseline.txt: 7 правил, у каждого корневая причина.Документация
docs/notes/oracle-known-fps.md— полный триаж 20 own-only находок, 4 корневые причины, что починено в коде vs что осталось честным warning/baseline (напр. Newtonsoftserializer.Error: source уезжает, доказать non-leak нельзя без lifetime-моделирования).Что НЕ делалось (осознанно)
serializer.Error,_textWriter, CsvHelper process-lived host — остаются в baseline как честные warning / непочинибельные без отдельной фичи.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation