feat(di): DI004 — transient IDisposable service-located from the root provider#65
Merged
Conversation
… provider The explicit / service-locator form of the transient-disposable leak — the call site the registration graph (DI001/2/3) cannot see. A singleton that injects an IServiceProvider (the root container) and resolves a transient IDisposable from it by hand (GetService<T>() / GetRequiredService<T>()) accumulates instances the root tracks and disposes only at application shutdown: an unbounded, repeated-at-runtime leak. Extractor: records per class the injected-IServiceProvider names (ctor params of that type plus fields assigned from them) and reads each resolution off them into a new `root_resolves` fact; a `scope.ServiceProvider.Get...` receiver is deliberately excluded. Core: find_explicit_root_resolutions flags a singleton whose root_resolves reaches a transient ∧ disposable service, surfaced as a warning (DI004) — a distinct code, not "DI003 explicit" (different detection, different fix: resolve from an IServiceScope). Precision (0 FP) is held by three guards, each pinned by a silent control in DiCaptiveSample.cs: singleton-only (RequestResolver, scoped, silent), the injected provider never a scope's (ScopedResolver, silent), transient ∧ disposable (PlainResolver resolving non-disposable UnitOfWork, silent); ConnectionResolver is the single flagged case. ownir load() validates root_resolves as an array of strings. Pinned end-to-end by the wpf-extractor CI job (exactly 1 DI004 + the three silent controls; DI001=4 / DI002=3 / DI003=1 unchanged) and at the graph level by tests/test_ownir.py (now 103/103). Docs: P-006 + di-captive-extractor note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // FLAGGED (DI004, warning) — singleton ConnectionResolver resolves the transient | ||
| // IDisposable PooledConnection by hand off its injected ROOT IServiceProvider | ||
| // (service locator), tracked to app shutdown. A call site, not a ctor edge. | ||
| services.AddSingleton<ConnectionResolver>(); |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR implements DI004 end-to-end: a new diagnostic that detects singletons resolving transient ChangesDI004 Service-Locator Detection
Sequence Diagram(s)sequenceDiagram
participant Roslyn as OwnSharp.Extractor
participant OwnIR as ownir.py
participant DI as di.py
Roslyn->>OwnIR: emit registration fact with root_resolves[]
OwnIR->>OwnIR: validate root_resolves is array (load)
OwnIR->>DI: Service(root_resolves=(...)) via _di_findings()
DI->>DI: find_explicit_root_resolutions(services)
Note over DI: filter: singleton + transient + disposable
DI-->>OwnIR: ExplicitRootResolution(singleton, resolved, file, line)
OwnIR-->>OwnIR: emit Finding(code="DI004", severity="warning")
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 60315a48d6
ℹ️ 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".
| if (p.Type is not null && DiTypeName(p.Type) == "IServiceProvider") | ||
| providerNames.Add(p.Identifier.Text); | ||
| foreach (var mem in cls.Members) | ||
| if (mem is ConstructorDeclarationSyntax ctorDecl && ctorDecl.Body is { } body) |
There was a problem hiding this comment.
When a singleton stores the injected provider with an expression-bodied constructor, e.g. public Resolver(IServiceProvider sp) => _sp = sp;, ctorDecl.Body is null here, so _sp never enters providerNames; the later _sp.GetRequiredService<PooledConnection>() is skipped and DI004 is not emitted. This is a common service-constructor shape and causes root-provider transient IDisposable leaks to be missed, so this alias collection should include expression-bodied constructors (and similar initializer forms) as well as block bodies.
Useful? React with 👍 / 👎.
| tnode = by_name.get(t) | ||
| if tnode is None: | ||
| continue | ||
| if tnode.lifetime == TRANSIENT and tnode.disposable: |
There was a problem hiding this comment.
When a singleton resolves a non-disposable transient wrapper from the root, e.g. _sp.GetRequiredService<Mid>() where Mid depends on transient IDisposable Pool, the root provider still constructs and tracks Pool until root disposal. This direct target check only reports when the resolved service itself is disposable, unlike DI003's transient DFS, so DI004 misses the same leak whenever the service-located type is a non-disposable transient with disposable transient dependencies.
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: 3
🤖 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 `@docs/notes/di-captive-extractor.md`:
- Around line 151-153: The sentence describing the "singleton-only" behavior is
too broad and misleading as it implies all scoped/transient services necessarily
get a request-scope provider. Reword the explanation in the "singleton-only"
section to be specific to the RequestResolver example rather than making a
general statement about scoped/transient services. Clarify that RequestResolver
(which is scoped) specifically demonstrates silent behavior because it has its
own request-scope provider, rather than suggesting this is always the case for
all scoped/transient services, since a transient service resolved from root
could still see the root provider.
In `@docs/proposals/P-006-di-lifetimes.md`:
- Around line 115-118: In the section discussing DI004 implementation (around
the IServiceScopeFactory paragraph), clarify that IServiceScopeFactory itself is
not currently modeled in the tracking logic and is only suggested as a future
fix pattern. Revise the text to explicitly state that the current implementation
only tracks generic GetService<T>() and GetRequiredService<T>() calls on
injected IServiceProvider names while excluding scope.ServiceProvider receivers,
and that modeling IServiceScopeFactory is a future extension rather than a
current feature. The phrase "natural extension" should be reworded to make clear
this refers to future functionality.
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 1010-1014: The code in the foreach loop that processes
AssignmentExpressionSyntax is adding all results from
AssignedFieldName(asg.Left) to providerNames without verifying that the
assignment target is an actual class field rather than a local variable or other
identifier. This causes non-field assignments (like constructor local aliases)
to be incorrectly included in providerNames, leading to false positives in
ReceiverIsProvider matching. Add a validation check after the AssignedFieldName
call to ensure the assignment target is an actual class field before adding it
to providerNames. You may need to verify the assignment is to a field by
checking the semantic model or syntax context to distinguish between field
assignments and local variable assignments.
🪄 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: 5192ea74-d1a3-4dd8-a8d5-01abb024fda9
📒 Files selected for processing (8)
.github/workflows/ci.ymldocs/notes/di-captive-extractor.mddocs/proposals/P-006-di-lifetimes.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/DiCaptiveSample.csownlang/di.pyownlang/ownir.pytests/test_ownir.py
…itive DFS, field-only aliasing Address both review bots on #65: - Codex P2: an EXPRESSION-bodied constructor (`=> _sp = sp;`) has a null Body, so the provider alias was missed and DI004 dropped a real leak. Scan the whole ctor (block- or expression-bodied) and field initializers (the primary-ctor shape) for the `_field = providerParam` capture. - Codex P2: resolving a NON-disposable transient wrapper off the root still makes the root build and track its disposable transient deps. find_explicit_root_resolutions now walks the resolved type's transient subtree exactly as DI003 does (entered at the call site), reporting disposables reached directly or transitively; scoped edges are not followed. ExplicitRootResolution carries the path. - CodeRabbit (Major): restrict the alias capture to real class fields, so a constructor LOCAL alias can never enter providerNames and same-name-match an unrelated receiver (no DI004 false positive). - CodeRabbit (docs): narrow the "singleton-only" wording to the RequestResolver sample; clarify a directly-injected IServiceScopeFactory is a future extension, not modelled. Sample gains ExprBodiedResolver (expression-bodied ctor) and the transitive WrapperResolver -> MidConnection -> PooledConnection (primary-ctor field initializer); CI now asserts exactly 3 DI004 + the rendered transitive path; the three controls stay silent. ruff + mypy --strict clean, ownir 104/104, full-sample bridge reproduces DI001=4 / DI002=3 / DI003=1 / DI004=3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
| services.AddScoped<RequestResolver>(); | ||
|
|
||
| // FLAGGED (DI004) — an expression-bodied ctor stores the injected root provider; same leak. | ||
| services.AddSingleton<ExprBodiedResolver>(); |
| // FLAGGED (DI004, transitive) — service-locates the non-disposable transient MidConnection | ||
| // off the root, which drags in the transient IDisposable PooledConnection (tracked to app exit). | ||
| services.AddTransient<MidConnection>(); | ||
| services.AddSingleton<WrapperResolver>(); |
This was referenced Jun 21, 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
3 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.
The explicit / service-locator form of the transient-disposable leak
DI001/2/3 read the registration graph. DI004 reads what the graph cannot see: a resolution call site. A singleton that injects an
IServiceProviderand resolves a transientIDisposablefrom it by hand — the service-locator anti-pattern — leaks.For a singleton the injected provider is the root container; the root tracks every
IDisposableit resolves and frees them only at application shutdown — so each call accumulates a transient that itstransientregistration says should be short-lived (the classic "transient disposables captured by the root container" leak), made worse by being a repeated runtime resolution. A warning, like DI003.Filed as a distinct code (not "DI003, the explicit form"): different detection (a call site, not a constructor edge), different fix (resolve from an
IServiceScope), one-code-per-rule keeps the SARIF catalogue honest.How
IServiceProvider— ctor params of that type plus fields assigned from them — then reads eachname.GetService<T>()/GetRequiredService<T>()whose receiver is one of those names into a newroot_resolvesfact. Ascope.ServiceProvider.Get...receiver is deliberately excluded.ownlang/di.py):find_explicit_root_resolutionsflags a singleton whoseroot_resolvesreaches a transient ∧ disposable service → DI004 (warning).ownirload()validatesroot_resolvesas an array of strings (likedeps/weak_deps).Precision (0 FP) — three guards, each pinned by a silent control
ConnectionResolverIDisposableoff the injected rootScopedResolverscope.ServiceProvider) — the correct fixPlainResolverUnitOfWork) — root doesn't track itRequestResolverAliases through locals, unknown receivers, and the non-generic
GetService(Type)form are not guessed — silent (recall left on the table to keep precision absolute).Pinned in CI
DiCaptiveSample.csadds the four classes; thewpf-extractorjob asserts exactly 1 DI004 (ConnectionResolver → PooledConnection) and that the three controls stay silent. The existing DI001=4 / DI002=3 / DI003=1 counts are unchanged (the new classes inject onlyIServiceProvider, so they add no graph edges).Validated locally: ruff + mypy
--strictclean, ownir 103/103 (the full-sample bridge reproduces DI001=4/DI002=3/DI003=1/DI004=1 with the controls silent).Docs: P-006 + the
di-captive-extractornote (DI004 shipped; naming rationale; the deferred call-site anchoring / plural-resolution slices).🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
New Features
IServiceProviderdirectly resolves a transientIDisposable, the extractor now reports a warning with the relevant resolution path.Documentation
Tests
Chores