feat: PoC oficially proven? - #16
Merged
Merged
Conversation
…e), exempt dispose-optional types, default the wrappers to flow
…on leak Pin the real GTM CatalogService.GetProductsFromCatalogWODocuments finding: a local UnitOfWork (IDisposable) used only through member access to build a returned DEFERRED IQueryable. The bare `uow` never escapes, so the flow detector keeps it tracked and reports OWN001 (disposed on no path) — a leak a naive `using` cannot fix (the deferred query would run after dispose). - frontend/roslyn/samples/UnitOfWorkFlowSample.cs: self-contained distillation of the GTM method, plus the using-var/materialize fix that must stay silent. - tests/fixtures/ownir/unitofwork_flow.facts.json + tests/test_ownir.py: the dotnet-free bridge+core pin (OWN001 on 'uow' at the C# line, tag disposable). - .github/workflows/ci.yml: end-to-end extractor->core assertion on the sample. https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
…ship-transfer sample Two follow-ups on the GTM UnitOfWork pin: - ownir bridge: word the flow-local OWN001 from whether the local is released anywhere in the body (ever_released). Released on no path -> "is never disposed"; released on some branch but leaked on another -> "may not be disposed on every path". Both stay the core's OWN001 — computed in the bridge from the flow body the extractor already emits (no schema change, no version bump). - UnitOfWorkFlowSample: add the Option B (ownership-transfer) fixes as silent contrasts to the materialize fix — uow returned (uowOwned) and moved out as an argument (uowMoved) both escape, so the flow detector does not flag them. - tests/CI: new flow_leak_on_else fixture pins the partial-path wording dotnet- free; test_ownir asserts both phrasings; the FlowLocals + UnitOfWork CI steps assert never-vs-every-path wording on real C# and that all three fixes stay silent. https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughImplements P-016 B0b/B2 path-sensitive ChangesFlow-sensitive local IDisposable leak detection (P-016 B0b/B2)
Sequence Diagram(s)sequenceDiagram
participant User as User / CI
participant Script as own-check.sh / .ps1
participant Extractor as OwnSharp.Extractor (--flow-locals)
participant FactsJSON as facts.json (functions[])
participant OwnIR as ownlang/ownir.py (to_module + check_facts)
participant Output as OWN001/002/003 Findings
User->>Script: run (no --legacy)
Script->>Extractor: dotnet run -- paths --flow-locals -o facts.json
Extractor->>Extractor: lower method bodies into acquire/use/release/if/return
Extractor->>FactsJSON: emit { components, functions[] }
Script->>OwnIR: load(facts.json)
OwnIR->>OwnIR: _lower_flow() → loc_<n> stmts + ever_released
OwnIR->>OwnIR: check_facts() → flow-local branch
OwnIR->>Output: Finding(OWN001, "never disposed" or "not on every path")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 226-229: The UsingStatementSyntax case (line 226) does not emit a
release when the using statement wraps an existing local variable, and the
disposal method matching logic (lines 243-246) only recognizes Dispose and Close
invocations but misses DisposeAsync calls including awaited variants. Fix the
using statement handler to emit a release for the variable being disposed when
using(existingLocal) is encountered, and extend the disposal method matching at
lines 243-246 to also recognize DisposeAsync method invocations so that both
synchronous and asynchronous disposal paths are properly tracked by the flow
analysis.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f46555e8-d24c-4f23-ad63-9470fceaa3d6
📒 Files selected for processing (11)
.github/workflows/ci.ymldocs/proposals/P-016-deep-fact-extraction.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FlowLocalsSample.csfrontend/roslyn/samples/UnitOfWorkFlowSample.csownlang/ownir.pyscripts/own-check.ps1scripts/own-check.shtests/fixtures/ownir/flow_leak_on_else.facts.jsontests/fixtures/ownir/unitofwork_flow.facts.jsontests/test_ownir.py
Comment on lines
+226
to
+229
| case UsingStatementSyntax us: | ||
| // using(...) {body}: the using local is auto-disposed (untracked); still | ||
| // lower the body so a tracked plain local used inside is seen. | ||
| return us.Statement is null || LowerFlowStmt(us.Statement, tracked, nodes); |
There was a problem hiding this comment.
Flow lowering currently misses valid disposal paths and can misreport leaks.
Line 226 lowers using bodies but does not emit a release for using(existingLocal).
Lines 243-246 only match direct Dispose/Close invocations and miss DisposeAsync (including await x.DisposeAsync()). With --flow-locals enabled (and D1 suppressed), these become false OWN001/OWN009-style outcomes.
Proposed fix
@@
case UsingStatementSyntax us:
// using(...) {body}: the using local is auto-disposed (untracked); still
// lower the body so a tracked plain local used inside is seen.
- return us.Statement is null || LowerFlowStmt(us.Statement, tracked, nodes);+ if (us.Statement is not null && !LowerFlowStmt(us.Statement, tracked, nodes))+ return false;+ // using(existingTrackedLocal) disposes that local at scope end.+ if (us.Expression is IdentifierNameSyntax uid+ && tracked.Contains(uid.Identifier.Text))+ nodes.Add(new { op = "release", var = uid.Identifier.Text, line = LineOf(us) });+ return true;
@@
static void EmitFlowExpr(ExpressionSyntax expr, HashSet<string> tracked, List<object> nodes)
{
// x.Dispose()/x.Close() on a tracked local -> release.
- if (expr is InvocationExpressionSyntax inv+ var call = expr switch+ {+ InvocationExpressionSyntax i => i,+ AwaitExpressionSyntax { Expression: InvocationExpressionSyntax i } => i,+ _ => null,+ };+ if (call is InvocationExpressionSyntax inv
&& inv.Expression is MemberAccessExpressionSyntax ma
- && ma.Name.Identifier.Text is "Dispose" or "Close"+ && ma.Name.Identifier.Text is "Dispose" or "Close" or "DisposeAsync"
&& ma.Expression is IdentifierNameSyntax rid
&& tracked.Contains(rid.Identifier.Text))
{
nodes.Add(new { op = "release", var = rid.Identifier.Text, line = LineOf(inv) });
return;
}Also applies to: 243-246
🤖 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 `@frontend/roslyn/OwnSharp.Extractor/Program.cs` around lines 226 - 229, The
UsingStatementSyntax case (line 226) does not emit a release when the using
statement wraps an existing local variable, and the disposal method matching
logic (lines 243-246) only recognizes Dispose and Close invocations but misses
DisposeAsync calls including awaited variants. Fix the using statement handler
to emit a release for the variable being disposed when using(existingLocal) is
encountered, and extend the disposal method matching at lines 243-246 to also
recognize DisposeAsync method invocations so that both synchronous and
asynchronous disposal paths are properly tracked by the flow analysis.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jun 16, 2026
Merged
Merged
10 tasks
7 tasks
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 freeto 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.
Summary by CodeRabbit
New Features
Tests
Documentation