feat(di): DI005 (scope-factory misuse) + first real-world captive corpus case#107
Merged
Conversation
…ning) ROADMAP listed DI002 (weak-ref), the root-`GetService` form, and the consuming-constructor anchor as remaining, but all are built and tested: `find_weak_captive_dependencies` (DI002), `find_explicit_root_resolutions` (DI004), and the `consuming_ctor` anchor (P-006 OQ#1, "both"). Update the Milestone-3 status and the P-006 index row to DI001–DI004 end-to-end; the real remaining items are a real-world corpus case, IServiceScopeFactory recognition, and the explicit dynamic-registration non-goals. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
The DI captive family (DI001–DI004) was pinned only on the synthetic DiCaptiveSample.cs. Add a real-world-shaped case: a singleton NotificationService injecting a scoped EF AppDbContext -> DI001, with the standard IServiceScopeFactory fix in after.cs (silent). DI has no `.own` reduction (registrations live in the `services` fact graph, not the resource/flow DSL), so this can't be a corpus/real-world case.own — it lives in a new benchmark-only `corpus/di/` directory, added to scripts/benchmark.py's default corpus dirs and scored by the dotnet corpus-benchmark job (not the Python test_corpus `.own` runner). Bump the recall floor 24 -> 25 to pin the newly-caught case. P-006 / ROADMAP updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
… done wrong) A new captive-family member. A singleton that injects IServiceScopeFactory and opens a scope (CreateScope()) — the *correct* remedy for a DI001 captive — but then CACHES the scope-resolved scoped service into a field. The `using` scope is disposed when the operation ends, so the cached instance dangles (use-after-dispose) and is promoted to application lifetime: the captive returns, hidden behind the very API meant to fix it, invisible to a static surface that "sees the fix". - Core (ownlang/di.py): `scope_cached` / `scope_cache_sites` Service fields, a `ScopeCachedCaptive` finding, and `find_scope_cached_captives` — a singleton whose scope_cached names a SCOPED service (a cached singleton is shareable; a cached transient is the DI003/DI004 family). A store-site property, anchored at the field assignment like DI004 anchors at its call site. - Bridge (ownlang/ownir.py): DI005 Finding at the store site (warning) with the registration as the related secondary; strict load() validation of the new fields. Pinned by 5 new ownir checks (locally verified, 118 -> 123). - Extractor (Program.cs): collect scope-creator names (injected IServiceScopeFactory / provider, same this-field discipline as DI004), the scope locals their CreateScope() produces, and each `scope.ServiceProvider.Get(Required)Service<T>()` ASSIGNED TO A FIELD -> scope_cached + its store site. - Sample + CI: DiCaptiveSample.cs gains ScopeCachingService (flagged) and the ScopeUsingService / ClockCachingService controls (used-in-scope local; cached singleton) that must stay silent; CI asserts the DI005 finding, count, store-site anchor (line 154), and the controls. P-006 / ROADMAP updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
![github-advanced-security[bot]](https://cdn.github.com/in/57789?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| // control: caches a SINGLETON service (Clock) resolved from a scope into a field — a singleton |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds DI005 detection for singleton services that cache scoped values resolved from created scopes, wires the new facts through the Roslyn extractor and OwnIR bridge, and updates the matching corpus fixtures, tests, CI checks, docs, and benchmark defaults. ChangesDI005 scope-cached captives
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ 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://cdn.github.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: 04a8a92fef
ℹ️ 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".
| reported: set[str] = set() | ||
| for dep in s.scope_cached: | ||
| node = by_name.get(dep) | ||
| if node is None or node.lifetime != SCOPED or dep in reported: |
There was a problem hiding this comment.
When a singleton caches a transient resolved from its own scope, and that transient depends on a scoped service, this branch skips it because the cached type itself is not SCOPED. That leaves a real DI005 shape undetected: e.g. _uow = scope.ServiceProvider.GetRequiredService<UnitOfWork>() where UnitOfWork is transient and ctor-injects scoped AppDbContext; the scope disposes the DbContext when the method exits, but the singleton keeps the UnitOfWork. This path is not reported by DI001/DI003/DI004 either, since it is not a constructor edge or a root-provider resolution.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Good catch — fixed in 02d9192. find_scope_cached_captives now walks each cached entry's strong transient graph like DI001: a cached scoped service is the captive directly, and a cached transient that ctor-injects a scoped service (directly or transitively) drags it into the singleton's lifetime too — anchored at the cached entry's store site. Pinned by a new sample case UnitOfWorkCachingService (-> UnitOfWork -> AppDbContext, CI line 201 + path text) and two new core checks.
Generated by Claude Code
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_ownir.py (1)
694-748: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative
load()coverage for the new DI005 schema fields.This section covers the DI005 happy path, but the new
load()branches forscope_cachedandscope_cache_sitesare still unpinned. A couple of_load_raises(...)checks here would give the new external-input validation the same protection thatweak_depsandroot_resolve_sitesalready have.Suggested additions
+ checks += 1 + if not _load_raises({"ownir_version": OWNIR_VERSION, "components": [], + "services": [{"name": "X", "lifetime": "singleton", + "scope_cached": "abc"}]}): + fails.append("a non-array service scope_cached did not raise OwnIRError") + checks += 1 + if not _load_raises({"ownir_version": OWNIR_VERSION, "components": [], + "services": [{"name": "X", "lifetime": "singleton", + "scope_cache_sites": [{"type": "Db", "line": "NaN"}]}]}): + fails.append("a malformed service scope_cache_sites did not raise OwnIRError")🤖 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 `@tests/test_ownir.py` around lines 694 - 748, The DI005 happy-path test in find_scope_cached_captives/check_facts does not yet cover invalid input for the new load() schema fields, so add negative _load_raises(...) cases for scope_cached and scope_cache_sites. Place the checks alongside the existing DI005 assertions in tests/test_ownir.py and verify load() rejects malformed or wrong-typed values for those fields just like the existing weak_deps and root_resolve_sites validation.
🤖 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 @.github/workflows/ci.yml:
- Around line 600-620: The DI005 test in the CI workflow only checks the
field-store anchor and count, so add an assertion that also matches the
registration tail / relatedLocation metadata for the DI005 finding. Update the
existing grep-based verification around the DiCaptiveSample.cs checks so it
explicitly validates the secondary suffix emitted for ScopeCachingService and
AppDbContext, ensuring the test fails if that metadata is dropped while keeping
the current anchor and count checks.
In `@docs/ROADMAP.md`:
- Line 240: Update the P-006 roadmap index row so it matches the milestone
status by including DI005 in the coverage range. Locate the proposal entry for
P-006 in the roadmap table and revise the DI001–DI004 text to reflect
DI001–DI005 end-to-end, keeping the row consistent with the surrounding
milestone description.
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 2051-2055: The scope cache site record is using the constructor
file instead of the file where the cache is actually stored, so DI005 can point
to the wrong artifact. Update the scope cache site emission in Program.cs where
scopeCacheSites.Add is built to use the assignment/store file for the current
scope cache write rather than ctorFile, and apply the same fix in the other
matching scope cache block referenced by the review.
---
Nitpick comments:
In `@tests/test_ownir.py`:
- Around line 694-748: The DI005 happy-path test in
find_scope_cached_captives/check_facts does not yet cover invalid input for the
new load() schema fields, so add negative _load_raises(...) cases for
scope_cached and scope_cache_sites. Place the checks alongside the existing
DI005 assertions in tests/test_ownir.py and verify load() rejects malformed or
wrong-typed values for those fields just like the existing weak_deps and
root_resolve_sites validation.
🪄 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: 51687edc-bcad-4eb3-b71c-0fbb5a3d70c1
📒 Files selected for processing (13)
.github/workflows/ci.ymlcorpus/di/singleton-captures-scoped-dbcontext/after.cscorpus/di/singleton-captures-scoped-dbcontext/before.cscorpus/di/singleton-captures-scoped-dbcontext/expected-diagnostics.txtcorpus/di/singleton-captures-scoped-dbcontext/notes.mddocs/ROADMAP.mddocs/proposals/P-006-di-lifetimes.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/DiCaptiveSample.csownlang/di.pyownlang/ownir.pyscripts/benchmark.pytests/test_ownir.py
Codex: a singleton that caches a TRANSIENT (resolved from its scope) which ctor-injects a scoped service is the same captive — the scope disposes the scoped service, but the singleton keeps the transient holding it (use-after-dispose), a shape DI001/DI003/DI004 cannot see. find_scope_cached_captives now walks each cached entry's strong transient graph like DI001 (cached scoped = direct; cached transient -> scoped = transitive), anchored at the cached entry's store site. New sample case UnitOfWorkCachingService + CI assertion (line 201, path text); core pinned by 2 new ownir checks. CodeRabbit: - ROADMAP index row DI001–DI004 -> DI001–DI005. - CI: assert the DI005 registration-site suffix on each finding (nreg5 == 2), mirroring DI004's nreg. - tests: negative load() coverage for scope_cached / scope_cache_sites (a non-array and a malformed site must raise OwnIRError), like weak_deps / root_resolve_sites. Not changed: the scope-cache-site `file` uses the per-class tree file (`ctorFile`), which IS the file containing the class and its method bodies where the store lives — identical to DI004's proven rootResolveSites; not the ctor's file in a misleading sense. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
…abbit) `scope_cache_sites` serialized `file = ctorFile` (the per-class tree file). That is the store's file in the single-file case, but to be unambiguous — and correct even if the cache write lives in another partial-class file — read the file from the assignment's own location span (`asg.GetLocation().GetLineSpan().Path`) instead. ScopeCachedTypes now yields (type, file, line); the emission uses that file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 25d4e8d0-44f9-4d39-bf09-3516bb312721
📒 Files selected for processing (6)
.github/workflows/ci.ymldocs/ROADMAP.mddocs/proposals/P-006-di-lifetimes.mdfrontend/roslyn/samples/DiCaptiveSample.csownlang/di.pytests/test_ownir.py
✅ Files skipped from review due to trivial changes (2)
- docs/proposals/P-006-di-lifetimes.md
- docs/ROADMAP.md
🚧 Files skipped from review as they are similar to previous changes (2)
- ownlang/di.py
- .github/workflows/ci.yml
…bit) The header still said a cached transient is not the violation, but the transient DFS now flags a cached transient that drags in a scoped service. Reword to "the cached value must REACH a scoped service — a cached scoped type, or a transient that ctor-injects one; a cached singleton or a transient with no scoped path is not the violation." Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
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.
Two increments on the P-006 DI lifetime track.
1 — First real-world captive-dependency corpus case (DI001)
The captive family (DI001–DI004) was pinned only on the synthetic
DiCaptiveSample.cs. Adds a real-world-shaped case — a singletonNotificationServiceinjecting a scoped EFAppDbContext→ DI001, with the standardIServiceScopeFactoryfix inafter.cs(silent).DI has no
.ownreduction (registrations live in theservicesfact graph, not the resource/flow DSL), so it can't be acorpus/real-worldcase.own. It lives in a new benchmark-onlycorpus/di/directory (added toscripts/benchmark.py's default corpus dirs, scored by the dotnetcorpus-benchmarkjob — not the Pythontest_corpus.ownrunner). Recall floor bumped 24 → 25.2 — DI005: scoped service cached from a created scope (the fix done wrong)
A genuinely new captive-family member. A singleton that does inject
IServiceScopeFactoryand open a scope (CreateScope()) — the correct remedy — but then caches the scope-resolved scoped service into a field:The
usingscope is disposed when the operation ends, so_dbdangles (use-after-dispose) and is promoted to application lifetime — the captive returns, hidden behind the very API meant to fix it. A static surface "sees the fix" (CreateScope) and would otherwise stay silent, which is exactly why it needs a dedicated check.ownlang/di.py):scope_cached/scope_cache_sitesfields, aScopeCachedCaptivefinding,find_scope_cached_captives(singleton whosescope_cachednames a scoped service — a cached singleton is shareable, a cached transient is the DI003/DI004 family). A store-site property, anchored at the field assignment like DI004 anchors at its call site.ownlang/ownir.py): DI005Finding(warning) at the store site, registration as the related secondary; strictload()validation of the new fields.Program.cs): collect scope-creator names (injectedIServiceScopeFactory/provider, same this-field discipline as DI004), theirCreateScope()scope locals, and eachscope.ServiceProvider.Get(Required)Service<T>()assigned to a field →scope_cached+ store site.DiCaptiveSample.csgainsScopeCachingService(flagged) and theScopeUsingService/ClockCachingServicecontrols (used-in-scope local; cached singleton) that must stay silent.Validation
ownirchecks 118 → 123 (the new DI005 finder, anchoring, and "not also DI001/2/3/4" guards), full suite +ruff+mypy --strict+ benchmark--selftestall green.corpus-benchmarkjob (the newcorpus/diDI001 case, floor 25) and the DI sample job (the DI005 assertions onDiCaptiveSample.cs).🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit