feat(di): anchor DI004 at the GetRequiredService call site (P-006 Q#1, DI004 arm)#67
Merged
Merged
Conversation
…, DI004 arm) #66 anchored DI001/2/3 at the consuming constructor; DI004's consumer is different — a resolution CALL SITE (`GetRequiredService<T>()`), not a ctor. The extractor now emits, in parallel with `root_resolves`, a `root_resolve_sites` fact ({type, file, line} per resolved type — the call's location); the core appends ` [resolved at <file>:<line>]` to the DI004 message and carries the site as a structured Finding.related triple -> SARIF relatedLocation, same shape as #66. The site is the ENTRY type's call (path[1]): for a transitive leak (WrapperResolver -> MidConnection -> PooledConnection) it points at where MidConnection was hand-resolved, not at the container-built PooledConnection. Degrades cleanly (no suffix, no related) when absent. ownir load() validates root_resolve_sites as an array of {type:str, file:str, line:int}. Pinned end-to-end by DiCaptiveSample.cs (ConnectionResolver:79, ExprBodiedResolver:123, transitive WrapperResolver:137) in the wpf-extractor CI job, and at the graph level (direct + transitive call site, related location, load validation) by tests/test_ownir.py. ruff + mypy --strict clean, ownir 116/116; DI counts unchanged. Now all four DI rules anchor at their consumer. Remaining anchoring follow-up: per-parameter precision. 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDI004 findings are now anchored at ChangesDI004 Resolution Call-Site Anchoring
Sequence Diagram(s)sequenceDiagram
participant Extractor as OwnSharp.Extractor
participant OwnIR as ownlang/ownir.py
participant DI as ownlang/di.py
Extractor->>Extractor: RootResolvedTypes yields (type, line) per GetService call
Extractor->>OwnIR: emit service fact with root_resolve_sites [{type, file, line}]
OwnIR->>OwnIR: load() validates root_resolve_sites shape
OwnIR->>OwnIR: _resolve_sites() parses to (type, file, line) triples
OwnIR->>DI: construct Service with root_resolve_sites
DI->>DI: find_explicit_root_resolutions builds sites lookup
DI->>DI: ExplicitRootResolution populated with resolved_file, resolved_line
OwnIR->>OwnIR: _di004_primary/_di004_related anchor Finding at call site
OwnIR-->>OwnIR: Finding emitted with call-site location and registration-related tuple
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
380-386: ⚡ Quick winAdd the missing ExprBodiedResolver call-site assertion.
Line 381 states three DI004 call-site anchors are pinned, but the checks at Line 383–386 only assert two. A regression on
ExprBodiedResolver:123would currently pass CI.Suggested CI assertion
echo "$out" | grep -qE "'ConnectionResolver'.*resolved at .*DiCaptiveSample\.cs:79\]" \ || { echo "FAIL: expected the DI004 call-site anchor (ConnectionResolver, line 79)"; exit 1; } + echo "$out" | grep -qE "'ExprBodiedResolver'.*resolved at .*DiCaptiveSample\.cs:123\]" \ + || { echo "FAIL: expected the DI004 call-site anchor (ExprBodiedResolver, line 123)"; exit 1; } echo "$out" | grep -qE "'WrapperResolver'.*resolved at .*DiCaptiveSample\.cs:137\]" \ || { echo "FAIL: expected the transitive DI004 call-site anchor (WrapperResolver entry, line 137)"; exit 1; }🤖 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 @.github/workflows/ci.yml around lines 380 - 386, The CI workflow is missing an assertion for the ExprBodiedResolver call-site anchor mentioned in the comment at line 381. Add a grep check after the existing ConnectionResolver assertion that validates ExprBodiedResolver is resolved at line 123 in DiCaptiveSample.cs, following the same pattern as the ConnectionResolver and WrapperResolver checks above it. This ensures that any regression in the ExprBodiedResolver DI004 anchor would be caught by CI.
🤖 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 @.github/workflows/ci.yml:
- Around line 380-386: The CI workflow is missing an assertion for the
ExprBodiedResolver call-site anchor mentioned in the comment at line 381. Add a
grep check after the existing ConnectionResolver assertion that validates
ExprBodiedResolver is resolved at line 123 in DiCaptiveSample.cs, following the
same pattern as the ConnectionResolver and WrapperResolver checks above it. This
ensures that any regression in the ExprBodiedResolver DI004 anchor would be
caught by CI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 029bc2ae-e828-43c4-844c-297ee4e5c28a
📒 Files selected for processing (7)
.github/workflows/ci.ymldocs/notes/di-captive-extractor.mddocs/proposals/P-006-di-lifetimes.mdfrontend/roslyn/OwnSharp.Extractor/Program.csownlang/di.pyownlang/ownir.pytests/test_ownir.py
![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: ae6595e136
ℹ️ 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".
| component=c.singleton, event=c.resolved, handler="", | ||
| message=c.message, kind="DI lifetime", severity="warning") | ||
| message=c.message, kind="DI lifetime", severity="warning", | ||
| related=_resolution_related(c)) |
There was a problem hiding this comment.
Adding the GetRequiredService site only as related leaves the actual finding anchored at c.file/c.line above, which are still the service registration site. In the new DI004 path this means human/GitHub/MSBuild output continues to annotate Startup.cs rather than the leaking _sp.GetRequiredService<T>() invocation, and SARIF still has the registration as its primary location with the call as secondary. For facts like file: S.cs,line:5 plus root_resolve_sites: R.cs:42, check_facts() still returns S.cs:5, so the advertised call-site anchoring is not achieved.
Useful? React with 👍 / 👎.
…d (Codex) Codex P2: the prior commit added the GetRequiredService call site only as a `related` location, so the DI004 finding stayed anchored at the registration site (Startup.cs) — human/GitHub/MSBuild/SARIF still annotated the registration, not the leaking call. The PR's stated goal (anchor at the call site) was not actually delivered. Unlike DI001/2/3 (a registration-graph property, rightly anchored at the registration), DI004 is a CALL-SITE property — the leak *is* `_sp.GetRequiredService<T>()` and the fix is there too, while the resolver's registration is barely relevant. So flip it: the call site is DI004's PRIMARY anchor (the finding's file/line), with the registration demoted to the Finding.related secondary + a `[singleton registered at <reg>]` message tail. Falls back to the registration site when the call is unknown. Also add the missing ExprBodiedResolver:123 call-site CI assertion (CodeRabbit: the comment claimed three anchors, only two were checked). ruff + mypy --strict clean, ownir 116/116; DI counts unchanged. CI now pins the primary line = call site (79/123/137) + the registration named as the secondary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
391-392: 💤 Low valueConsider counting "singleton registered at" occurrences for precision.
The current assertion checks for the presence of
"singleton registered at"anywhere in the output, which validates that the message suffix is being generated. For slightly better precision, consider counting the occurrences to ensure it appears exactly 3 times (once for each DI004 finding), mirroring the existing pattern at lines 308 and 377:- echo "$out" | grep -q "singleton registered at " \ - || { echo "FAIL: expected the DI004 registration site named as the secondary anchor"; exit 1; } + nreg=$(echo "$out" | grep -c "singleton registered at ") + [ "$nreg" = "3" ] \ + || { echo "FAIL: expected exactly 3 DI004 findings with registration-site suffix, got $nreg"; exit 1; }This would catch regressions where the suffix is generated for some findings but not all.
🤖 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 @.github/workflows/ci.yml around lines 391 - 392, The grep assertion at lines 391-392 only checks for the presence of "singleton registered at" in the output, but it should verify that this string appears exactly 3 times (once for each DI004 finding) to catch regressions where the suffix is only generated for some findings but not all. Replace the grep -q presence check with a count-based assertion that verifies the occurrence count equals 3, following the same pattern already used at lines 308 and 377 in the workflow file.
🤖 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 @.github/workflows/ci.yml:
- Around line 391-392: The grep assertion at lines 391-392 only checks for the
presence of "singleton registered at" in the output, but it should verify that
this string appears exactly 3 times (once for each DI004 finding) to catch
regressions where the suffix is only generated for some findings but not all.
Replace the grep -q presence check with a count-based assertion that verifies
the occurrence count equals 3, following the same pattern already used at lines
308 and 377 in the workflow file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 05393b62-6cad-4585-a869-69c6f5d747cb
📒 Files selected for processing (6)
.github/workflows/ci.ymldocs/notes/di-captive-extractor.mddocs/proposals/P-006-di-lifetimes.mdownlang/di.pyownlang/ownir.pytests/test_ownir.py
✅ Files skipped from review due to trivial changes (1)
- docs/notes/di-captive-extractor.md
🚧 Files skipped from review as they are similar to previous changes (2)
- ownlang/di.py
- tests/test_ownir.py
…presence CodeRabbit review: the registration-site secondary-anchor assertion used grep -q (presence anywhere), which would pass if the `[singleton registered at …]` tail were generated for some DI004 findings but not all. Count it (== 3, one per finding), mirroring the existing exactly-N patterns. CI-only. 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 DI004 at its real consumer — the resolution call site
#66 anchored DI001/DI002/DI003 at the consuming constructor. DI004's consumer is different: a resolution call site (
GetRequiredService<T>()), not a ctor. This arm finishes the anchoring story — now all four DI rules point at their consumer.The call site is the PRIMARY anchor (the finding's
file/line), not just a related location — because unlike DI001/2/3 (a registration-graph property, rightly anchored at the registration, where you'd change the lifetime), DI004 is a call-site property: the leak is_sp.GetRequiredService<T>()and the fix is there too, while the resolver's registration is barely relevant. The registration is demoted to the secondary: aFinding.relatedlocation + a[singleton registered at …]message tail.(This corrects the first cut, which left the call site as
related-only and kept the registration as primary — Codex caught that human/GitHub/MSBuild still annotatedStartup.cs, not the leak.)The transitive subtlety
The site is the entry type's call (
path[1]), not the leaked disposable. ForWrapperResolver -> MidConnection -> PooledConnection, the anchor points at whereMidConnectionwas hand-resolved (line 137) — that's the service-location call the developer wrote;PooledConnectionwas dragged in by the container.How
RootResolvedTypesyields the call line too; the service fact gains a parallelroot_resolve_sites({type, file, line}), alongside the existing string-arrayroot_resolves(additive, backward-compatible).ownlang/di.py):ExplicitRootResolutiongainsresolved_file/resolved_line(the entry call site); the bridge makes that the finding's primary location, the registration itsrelated, with a clean fallback to the registration when the call is unknown.ownir.pyvalidatesroot_resolve_sitesinload().Pinned
wpf-extractorCIConnectionResolver:79,ExprBodiedResolver:123, transitiveWrapperResolver:137; registration named as the secondarytests/test_ownir.pyFinding, the registrationrelated,root_resolve_sitesload-validationValidated locally: ruff + mypy
--strictclean, ownir 116/116; DI001=4 / DI002=3 / DI003=1 / DI004=3 counts unchanged.Next anchoring follow-up (separate slice): per-parameter precision for the captive anchor.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Summary by CodeRabbit
GetRequiredService/GetService), including direct and transitive cases.