extractor: pooled-buffer-transfer FP fixes (Redis oracle sweep) + FP re-triage#162
Conversation
…er FP cleared Point the push-triggered oracle at protobuf-net to verify the using-statement local-release fix clears the assorted/ Silverlight Page.xaml.cs `timer` FP. With the baseline entry still present, the finding must be absent from BOTH own-only and the baselined section. Reverted in the next commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
…dev config Ran the cross-tool oracle on protobuf-net (via the push-trigger dev fallback; the token can't workflow_dispatch) to confirm whether the using-statement local-release fix clears the assorted/ Silverlight Page.xaml.cs `timer` FP. Result: NOT cleared — the finding is still emitted and still baselined. Re-triage of the real source shows the prior baseline reason was a misdiagnosis read from COMMENTED-OUT code: the only `using (timer)` block is commented out, and `timer` is a Stopwatch (RunTest) / a DispatcherTimer local (btnRunTest_Click) — neither is IDisposable. So the using-fix does not apply; the entry stays baselined as a non-product sample FP, and the real open question is why a non-IDisposable timer-shaped local is flagged at all (a precision follow-up needing a local repro). - Corrected the baseline entry reason and oracle-known-fps.md (root-cause #1 + the per-finding row). - Reverted the dev-only oracle config (oracle.yml push branch + oracle-target.txt back to the systemevents-console fixture). Selftests: oracle 31/31, corpus 30/30. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Cross-reference the tracked precision follow-up (non-IDisposable timer local flagged OWN001) from the baseline entry and oracle-known-fps.md root-cause #1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
…pwatch), not an FP Cracked the mechanism behind issue #161 from the core side: the finding renders via the flow-locals path (ownir.py rkind=="flow-local"), whose extractor candidate gate REQUIRES ImplementsIDisposable. The raw protobuf source shows the file has no `using System.Diagnostics;` — it has `using Nuxleus.Performance;` and uses `Stopwatch.UnitPrecision` / `UnitPrecision.NANOSECONDS` / `timer.Scope = () => …`. So `Stopwatch` is `Nuxleus.Performance.Stopwatch`, a custom IDisposable scope-timer (you can only `using` an IDisposable), NOT System.Diagnostics.Stopwatch. `Stopwatch timer = new Stopwatch()` is genuinely never disposed — the disposing `using (timer) { … }` block is commented out — so the OWN001 is a REAL leak the extractor correctly flagged. It stays baselined as NON-PRODUCT sample (assorted/ Silverlight demo), like the NetTranscoder `sync` entry — reclassified from FP to true-but-benign-sample. The earlier "missed using" and "non-IDisposable Stopwatch FP" readings were both wrong; issue #161 is closed as invalid. Corrected the baseline entry reason, oracle-known-fps.md root-cause #1, the per-finding verdict row, and the disposition summary (Baselined FP 6→5, true-but-benign sample 2→3). oracle selftest 31/31. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
… recursive Dispose-no-op idea Investigated the deferred "recursive Dispose-is-a-no-op" feature against its only motivating case (Newtonsoft TraceJsonReader._textWriter). Reading the real JsonTextWriter source settles it: disposal is NOT a no-op type. - Close() -> base.Close() auto-completes open JSON tokens (writes closing brackets to the underlying writer), then CloseBufferAndWriter(). - CloseBufferAndWriter() RETURNS the rented _writeBuffer to _arrayPool when an ArrayPool is configured — a real pooled-buffer release (the POOL-leak class Own.NET tracks) — and closes the underlying writer. So a recursive Dispose-no-op recognizer would be unsound (it can leak a pooled buffer) or would correctly decline — either way it does not clear this finding. The _textWriter instance is benign only by instance facts (no ArrayPool set + the sink is a StringWriter), not a type-level no-op — the same reason writers are excluded from IsNoOpDisposeWrapper. Shelved as not worth building, not deferred. Sharpened the baseline entry reason and updated no-op-dispose-wrapper.md + oracle-known-fps.md root-cause #2 with the evidence. No code change. Selftests: oracle 31/31, corpus 30/30. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
…(di_source_life) The "Remaining: migrating the injected-source tier" note was stale. Investigation (ownir.py + test_ownir.py, 208/208 green) confirms the migration shipped via the DI graph (P-006 + P-004): an injected `+=` whose source_type resolves in the registration graph reroutes through the region engine — source proven to outlive the subscriber escalates OWN001-warning to OWN014 (error), a co-lifetimed/shorter source is refuted and dropped, and an unresolved (non-DI) source stays the honest OWN001 warning. The residual is by design; the curated "app-lived source type" escalation (MVVM messengers) is deliberately deferred as FP-prone — modern WeakReferenceMessenger holds recipients weakly, so a type-name allowlist would mis-flag the safe case. Doc-only; reflects shipped behavior. No code change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Point the push-triggered oracle at DapperLib/Dapper to look for new FP classes / recall gaps in our target niche (ADO.NET owned-API recognition). Dev-only; reverted after the sweep. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Dapper: 0 own-only findings — ADO recognition added no noise. Oracle-only was Infer# WrappedBasicReader ownership-transfer FPs + CodeQL dispose-on-throw honest-skips (huge SqlMapper methods). No new FP class. Next: Redis (subscription tokens / ConnectionMultiplexer / timers). Dev-only; reverted after the sweep. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
…al is not a leak Oracle sweep (StackExchange.Redis) surfaced a false-positive class: a rented ArrayPool buffer handed to `new Wrapper(buf)` that is bound to a LOCAL which then leaves the method — `var lease = new Lease(arr); return lease;` (Lease<T>.Create) and `var r = new ScanResult(keys); SetResult(m, r);` (RedisServer scan) — was flagged OWN001 "rented but never returned". The buffer is transferred to the wrapper (which owns the Return), not leaked. PassedToEscapingCtor already exempts the DIRECT `return new Wrapper(buf)` / `_field = new Wrapper(buf)`. The Codex one-level rule deliberately kept a wrapper-in-a-local as a borrow (a method-scoped wrapper that never returns the buffer is a real leak). WrapperLocalEscapes adds the one-extra-hop case while preserving that: the `new Wrapper(buf)` bound to a local `w` is a transfer only when `w` provably leaves the method (`return w` / `<field> = w` / `w` as a call argument); a wrapper local that stays method-scoped still leaks. Corpus fixture pool-transfer-via-wrapper-local: before.cs (wrapper dropped method-scoped -> OWN001 caught) / after.cs (wrapper returned -> clean). Also re-runs the Redis oracle (oracle-target bump) to confirm the 4 own-only FPs clear. corpus 31/31, ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
…c POOL001 pass) Second Redis pooled-buffer FP: RedisServer scan does `RedisKey[] keys; … keys = ArrayPool<RedisKey>.Shared.Rent(count); … new ScanResult(cursor, keys, count, true)`. The buffer is transferred to ScanResult, which owns the Return (its Recycle() -> ArrayPool.Return, called by CursorEnumerable on ProcessReply/ Dispose) — confirmed in source. So OWN001 "rented but never returned" was a false positive. Root cause: the syntactic POOL001 pass classifies an assignment-target rent via FieldName(asg.Left), which is purely syntactic (returns the identifier text for ANY target). A bare local (`keys = Rent()`, keys pre-declared) was thus treated as a field-backed rent with isField=true and NO escape/transfer analysis. The pass is meant for real fields (rented in ctor, Returned class-wide in Dispose); gate the assignment arm on `model.GetSymbolInfo(asg.Left).Symbol is IFieldSymbol` so a bare-local assignment rent is not flagged here. Such a local belongs to the flow pass, which does escape analysis but does not track the assignment form — so it is honestly untracked rather than flagged unsoundly (precision-first; the rare bare-assign-then-dropped local leak is a recall gap, not a false positive). No sample regression (all sample assignment rents are `this.field = Rent()` fields or `var`/`T[] x = Rent()` declarations). Re-runs the Redis oracle to confirm RedisServer.cs:917 clears. corpus 31/31. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Sweep found and fixed two pooled-buffer-transfer FP classes on StackExchange.Redis (WrapperLocalEscapes for var-decl transfers, IFieldSymbol gate for bare-local assignment rents); Dapper was clean. Live Redis re-runs took own-only 10 -> 9 -> 6, all pooled-buffer FPs cleared; the remaining 6 are real/warning findings. - Reverted the dev-only oracle config (oracle.yml push branch + oracle-target.txt back to the systemevents-console fixture). - Documented the sweep in oracle-known-fps.md (Dapper clean; Redis two fixes + the remaining Timer/CTS/injected-sub dispositions). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
|
Warning Review limit reached
Next review available in: 50 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)
📝 WalkthroughWalkthroughThis PR extends the C# extractor's pooled-buffer escape analysis to recognize ownership transfer via a wrapper local that escapes the enclosing method, tightens field-rent classification to require real field symbols, adds a corresponding corpus test case, and updates oracle baseline documentation re-triaging existing findings. ChangesExtractor Wrapper-Local Escape Detection
Oracle Baseline Re-triage and Documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
![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: d694959c62
ℹ️ 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".
| SyntaxNode? body = method?.Body ?? (SyntaxNode?)method?.ExpressionBody; | ||
| if (body is null) | ||
| return false; | ||
| foreach (var id in body.DescendantNodes().OfType<IdentifierNameSyntax>()) |
There was a problem hiding this comment.
This scans every reference to the local in the whole method and treats any matching return/field assignment/call argument as an escape of this particular new. If the local is reused, e.g. Sink(holder); ... holder = new Holder(buf); or holder = new Holder(buf); holder = other; return holder;, the object that owns buf never leaves the method, but PassedToEscapingCtor still untracks buf, so the ArrayPool leak is missed. The escape check needs to be constrained to uses after this creation and before any rebind/pure write of the local.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 8d747fe. You're right that scanning every reference to a reused local is unsound (a missed pool leak, not an FP): var w = new Wrapper(buf); w = other; return w; returns a different object, and Sink(w); … w = new Wrapper(buf); uses an earlier value.
Rather than thread precise "after this creation / before any rebind" flow ordering into this syntactic pass, I constrained it conservatively: bind only the declaration form (var w = new Wrapper(buf), a fresh local) and bail if w is reassigned anywhere (w = … on the LHS). Then every reference to w provably denotes the wrapper that owns buf, so a return/field =/call-arg is a genuine escape; any reassignment → we keep the buffer flagged (sound — no missed leak). The assignment-form binding is dropped (the risky reused case). Lease<T>.Create still clears (var lease = new Lease(arr); return lease; — declared once, never reassigned).
Generated by Claude Code
…(Codex P2) WrapperLocalEscapes scanned every reference to the local across the whole method and treated any return/field-assign/call-arg as an escape of THIS `new Wrapper(buf)`. For a reused local that is unsound (a missed pool leak, not an FP): `var w = new Wrapper(buf); w = other; return w;` returns a different object, and `Sink(w); … w = new Wrapper(buf);` uses an earlier value — yet buf would be untracked. Fix: bind only the DECLARATION form (`var w = new Wrapper(buf)`, a fresh local), and bail if `w` is reassigned anywhere (`w = …` on the LHS) — then w no longer provably refers to the wrapper that owns buf, so the buffer stays flagged (sound; a false negative here would hide a real leak). Lease<T>.Create still clears (var lease = new Lease(arr); return lease; — declared once, never reassigned). The assignment-form binding is dropped (rare; the risky reused case). corpus 31/31; ruff clean. Fixture notes updated with the reused-local caveat. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
corpus/real-world/pool-transfer-via-wrapper-local/notes.md (1)
24-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd corpus coverage for the non-return escape branches.
The notes document three positive paths now, but this fixture set only exercises
return holderinafter.cs. A small field-assignment variant and a call-argument variant would keepWrapperLocalEscapes’s other two exits pinned against regressions.🤖 Prompt for 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. In `@corpus/real-world/pool-transfer-via-wrapper-local/notes.md` around lines 24 - 39, Add corpus coverage for the missing non-return escape paths in WrapperLocalEscapes: the current fixture only pins the return-holder case, so add one test that assigns the wrapper local to a real field and another that passes the wrapper local as a call argument. Update the relevant corpus fixture(s) so these two exits are exercised alongside the existing after.cs return case, keeping the rule behavior locked against regressions.
🤖 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 `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 2138-2151: The escape scan in WrapperLocalEscapes is currently
walking into nested lambdas and local functions via body.DescendantNodes(),
which can incorrectly treat method-scoped wrappers as escaping. Update the scan
to ignore nested callable bodies while still checking the top-level method body,
so captures inside LambdaExpressionSyntax, AnonymousMethodExpressionSyntax, and
LocalFunctionStatementSyntax do not trigger a true result unless the callable
itself escapes.
---
Nitpick comments:
In `@corpus/real-world/pool-transfer-via-wrapper-local/notes.md`:
- Around line 24-39: Add corpus coverage for the missing non-return escape paths
in WrapperLocalEscapes: the current fixture only pins the return-holder case, so
add one test that assigns the wrapper local to a real field and another that
passes the wrapper local as a call argument. Update the relevant corpus
fixture(s) so these two exits are exercised alongside the existing after.cs
return case, keeping the rule behavior locked against regressions.
🪄 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: 46677514-7d4c-4d19-abc7-1a73a136d680
📒 Files selected for processing (10)
corpus/oracle-fp-baseline.txtcorpus/real-world/pool-transfer-via-wrapper-local/after.cscorpus/real-world/pool-transfer-via-wrapper-local/before.cscorpus/real-world/pool-transfer-via-wrapper-local/case.owncorpus/real-world/pool-transfer-via-wrapper-local/expected-diagnostics.txtcorpus/real-world/pool-transfer-via-wrapper-local/notes.mddocs/ROADMAP.mddocs/notes/no-op-dispose-wrapper.mddocs/notes/oracle-known-fps.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
… (CodeRabbit) body.DescendantNodes() walked into nested lambdas / local functions, so a use of the wrapper local inside a callable that never leaves the method (`Action a = () => Sink(w);`) counted as an escape and untracked buf — hiding a real pool leak. Prune descent into AnonymousFunctionExpressionSyntax / LocalFunctionStatementSyntax so only the wrapper's top-level uses drive the escape decision. corpus 31/31; ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
2 participants
Что и зачем
Precision-работа из свежего cross-tool oracle-свипа (Dapper, StackExchange.Redis) плюс честный ре-триаж двух ранее-задокументированных находок. Два реальных FP-класса найдены и починены, оба verified end-to-end (live Redis oracle re-runs + CI).
Два pooled-buffer-transfer FP-фикса (Redis): арендованный
ArrayPool-буфер, переданный обёртке, которая владеет возвратом, ошибочно флагалсяOWN001«rented but never returned».WrapperLocalEscapes—var w = new Wrapper(buf); return w;/Sink(w): обёртка в локале, который покидает метод, — это transfer (одно-hop расширение существующегоreturn new Wrapper(buf); Codex-правило про method-scoped обёртку сохранено — она всё ещё течёт). ЧинитLease<T>.Create.IFieldSymbol-гейт синтаксического POOL001-прохода — bare-localkeys = Rent()(пред-объявлен) ошибочно принимался за field-rent (FieldNameчисто синтаксический) и флагался без escape-анализа. ЧинитRedisServerscan (буфер уходит вScanResult, который возвращает его черезRecycle()— подтверждено по сорсуCursorEnumerable).Ре-триаж (docs/baseline, без изменения поведения):
Page.xaml.cstimer— оказался TRUE POSITIVE (кастомный IDisposableNuxleus.Performance.Stopwatch, не BCL), не FP; issue FP: undisposed non-IDisposable timer local (Stopwatch / DispatcherTimer) flagged OWN001 #161 заведена и закрыта как invalid._textWriter— recursive «Dispose-is-no-op» анализ снят как not-worth-building (JsonTextWriter возвращает pooled-буфер + пишет token-completion → не no-op тип).di_source_life).Тип изменения
Как проверено
python tests/test_corpus.py→ 31/31 (новый фикстурpool-transfer-via-wrapper-local)python tests/test_ownir.py→ 208/208oracle_compare.py --selftest→ 31/31;ruffcleanСвязанные issue
Refs #161 (closed as invalid — TRUE POSITIVE). Продолжение precision-трека #159/#160.
Чеклист
pool-transfer-via-wrapper-local: before.cs method-scoped обёртка → OWN001 ловится, after.cs escaping → чисто) + live-oracle подтверждениемoracle-known-fps.md2026-07-01 sweep + ре-триаж;no-op-dispose-wrapper.md; ROADMAP)Честные границы (recall)
IFieldSymbol-гейт делает bare-local assignment-rent (T[] x; x = Rent()) невидимым для синтаксического прохода. Такой локал принадлежит flow-проходу, который escape-анализ делает, но форму присваивания не трекает — так что редкийx = Rent(); /* dropped */теперь recall-gap, а не unsound FP. Precision-first, консистентно с существующей transfer-политикой.🤖 Generated with Claude Code
https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation