feat(di): anchor captive findings at the consuming constructor (P-006 Q#1)#66
Merged
Conversation
… Q#1) DI001/DI002/DI003 keep their primary anchor at the registration site but now also name the CONSUMING CONSTRUCTOR — where the captive dependency is injected — so the developer is pointed at the code, not just the wiring. P-006 open question #1 ("anchor at the registration line, the consuming constructor, or both?") answered: both, with the capture path shown, like OWN014. - Extractor records each implementation's ctor location (the widest public ctor's declaration, or the class declaration for a C# 12 primary / implicit ctor) into ctor_file / ctor_line on the service fact. - Core appends ` [consumed by the '<singleton>' constructor at <file>:<line>]` to the captive message (every surface) and carries it as a structured Finding.related triple, emitted as a SARIF relatedLocation — clickable + cross-file (registration in Startup.cs, ctor in EmailSender.cs). Degrades cleanly (no tail, no related) when the location is unknown, so hand-authored facts / an older extractor still produce the prior form. - ownir load() validates ctor_file/ctor_line like file/line. Pinned end-to-end by DiCaptiveSample.cs (explicit-ctor EmailSender:25, primary-ctor PrimaryCtorService:33, ConnectionWarmer:50, WeakCache:57) in the wpf-extractor CI job, the cross-file case by the di.facts.json fixture, and the SARIF related location by tests/test_ownir.py. DI004's consumer is a resolution call site (not a ctor) — its anchor is the sibling follow-up. ruff + mypy --strict clean, ownir 109/109; DI counts unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds consuming-constructor location anchors to DI001/DI002/DI003 captive-dependency findings. The Roslyn extractor records each class's constructor file/line, propagates those through DI dataclasses and message text in ChangesConsuming-Constructor Anchor Pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
…ertions The extractor emits the ctor location with the full relative path (frontend/roslyn/samples/DiCaptiveSample.cs:25), but the new DI consuming-constructor assertions grepped for the bare filename (at DiCaptiveSample.cs:25), which is not a substring — so the wpf-extractor job failed even though the output was correct. Switch the four assertions to a regex that allows the path prefix (`at .*DiCaptiveSample\.cs:NN]`), matching how the existing DI greps already tolerate it. Feature unchanged; CI-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![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: d977dc8cf6
ℹ️ 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".
| message degrades cleanly to just the registration-anchored form.""" | ||
| if line < 1: | ||
| return "" | ||
| return f" [consumed by the '{singleton}' constructor at {file}:{line}]" |
There was a problem hiding this comment.
For registrations where service and implementation types differ, e.g. AddSingleton<IEmailSender, EmailSender>(), the extractor stores the implementation constructor location but the new suffix formats singleton (the service name) as the constructor owner. That makes diagnostics/SARIF say the IEmailSender constructor is at EmailSender.cs, which points users to a non-existent constructor; carry the implementation/ctor type through the fact or avoid naming the constructor owner.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/proposals/P-006-di-lifetimes.md (1)
114-115: 💤 Low valueMinor: Consider splitting the resolved answer for readability.
The sentence combining the question, its resolution, and the explanation is dense. A brief split (e.g., "Resolved — both. For the captive family (DI001/DI002/DI003)…") would improve scannability in this technical proposal.
🤖 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 `@docs/proposals/P-006-di-lifetimes.md` around lines 114 - 115, Split the dense sentence at lines 114-115 that combines the question "constructor, or both?" with the resolution and explanation. Separate the brief resolution "Resolved — both" from the detailed explanation about the capture path display. Restructure it so the resolution stands on its own first, then introduce the captive family (DI001/DI002/DI003) reference and related explanation on a new line or sentence for improved scannability in this technical proposal document.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@docs/proposals/P-006-di-lifetimes.md`:
- Around line 114-115: Split the dense sentence at lines 114-115 that combines
the question "constructor, or both?" with the resolution and explanation.
Separate the brief resolution "Resolved — both" from the detailed explanation
about the capture path display. Restructure it so the resolution stands on its
own first, then introduce the captive family (DI001/DI002/DI003) reference and
related explanation on a new line or sentence for improved scannability in this
technical proposal document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b5b41f21-502f-4e96-952f-99b5cee3de06
📒 Files selected for processing (8)
.github/workflows/ci.ymldocs/notes/di-captive-extractor.mddocs/proposals/P-006-di-lifetimes.mdfrontend/roslyn/OwnSharp.Extractor/Program.csownlang/di.pyownlang/ownir.pytests/fixtures/ownir/di.facts.jsontests/test_ownir.py
…ner (Codex)
For an interface registration (AddSingleton<IFoo, Foo>), the singleton's service name is
the ctor-less interface IFoo, but the consuming constructor is Foo's — so naming the
service ('IFoo') as the ctor owner pointed users at a non-existent IFoo constructor. Carry
the implementation type through the fact (ctor_type = impl) and name IT in the message tail
and the SARIF related-location label. For concrete registrations ctor_type == name, so the
message is unchanged; degrades to a plain "the constructor" when the impl is unknown.
Pinned by an interface-registration case in tests/test_ownir.py (IBilling -> Billing names
Billing, never IBilling) and ctor_type load-validation. Also split the dense P-006 Q#1
sentence for readability (CodeRabbit nitpick). ruff + mypy --strict clean, ownir 112/112.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
…r name)
The Codex impl-naming fix added consumed_type to the DI001 and DI002 collectors but
missed find_captured_transient_disposables (DI003), so a DI003 finding rendered the
empty-type fallback "[consumed by the constructor at …]" instead of naming the impl
("[consumed by the 'ConnectionWarmer' constructor at …]"), failing the wpf-extractor CI
assertion. Pass consumed_type=s.ctor_type here too, and strengthen the DI003 unit test to
set ctor_type and assert the named anchor (the gap that let this through — the DI003
fixtures carried no ctor location). ruff + mypy --strict clean, ownir 113/113.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
PhysShell
added a commit
that referenced
this pull request
Jun 21, 2026
…, DI004 arm) (#67) #66 anchored DI001/2/3 at the consuming constructor; this finishes the anchoring story for DI004, whose consumer is different — a resolution CALL SITE (GetRequiredService<T>()), not a ctor. And because the leak *is* that call (the fix is there too, while the resolver's registration is barely relevant), the call site is DI004's PRIMARY anchor — unlike the registration-graph rules (DI001/2/3), which rightly anchor at the registration. The registration is demoted to the Finding.related secondary + a [singleton registered at …] message tail; falls back to the registration when the call is unknown. - Extractor: RootResolvedTypes yields the call line; the service fact gains a parallel root_resolve_sites ({type, file, line}) alongside the string-array root_resolves (additive, backward-compatible). - Core: ExplicitRootResolution gains resolved_file/resolved_line (the entry type's call — path[1], so a transitive WrapperResolver -> MidConnection -> PooledConnection anchors at where MidConnection was hand-resolved, not the container-built disposable); the bridge makes that the finding's primary location. ownir load() validates root_resolve_sites. Pinned by DiCaptiveSample.cs (primary line = call site: ConnectionResolver:79, ExprBodiedResolver:123, transitive WrapperResolver:137; the registration named as the secondary, exactly 3) and by tests/test_ownir.py. ruff + mypy --strict clean, ownir 116/116; DI001=4 / DI002=3 / DI003=1 / DI004=3 unchanged. All four DI rules now anchor at their consumer. Next: per-parameter precision for the captive anchor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
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.
Anchor the capture at the code, not just the wiring
A captive finding's primary anchor is the registration site — where you wire the lifetime. But the capture is introduced elsewhere: the consuming constructor that injects the captive dependency. P-006 open question #1 — "anchor at the registration line, the consuming constructor, or both?" — is now answered both, with the path shown (like OWN014).
DI001 / DI002 / DI003 now also name the consuming constructor:
relatedLocation— which GitHub code scanning renders as a second, clickable, labelled location, and which is cross-file (the registration may live inStartup.cs, the ctor inEmailSender.cs).The primary anchor stays at the registration site (one fix site; no churn to the line-based assertions), so this is purely additive.
How
ctor_file/ctor_lineon the service fact (classCtorLoc).ownlang/di.py): the three captive dataclasses gainconsumed_file/consumed_line;_consumed_suffixappends the tail when known.ownir.pyadds aFinding.relatedtriple list → SARIFrelatedLocations, set for DI001/2/3 from the singleton's ctor.load()validatesctor_file/ctor_linelikefile/line.Pinned
wpf-extractorCIEmailSender:25, primary-ctorPrimaryCtorService:33,ConnectionWarmer:50(DI003),WeakCache:57(DI002)di.facts.jsonfixtureStartup.cs, ctorEmailSender.cs:5)tests/test_ownir.pyrelatedtriple, the SARIF relatedLocation, and clean degradation when the ctor location is absentValidated locally: ruff + mypy
--strictclean, ownir 109/109; DI001=4 / DI002=3 / DI003=1 / DI004=1 counts unchanged.Scope
This covers the ctor-injected captive family (DI001/2/3). DI004's consumer is a resolution call site (
GetRequiredService<T>()), not a constructor — anchoring it there needs the call-site location threaded throughroot_resolves, and is the natural sibling follow-up (noted in the docs).🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
New Features
relatedLocations(cross-file clickable).Documentation
Bug Fixes / Improvements
ctor_file,ctor_line,ctor_type) and uses it to populate the related location anchors (with graceful degradation when unknown).Tests / CI