feat(extractor): P-014 Tier B — external reference resolution via --ref-dir#109
Merged
Conversation
…ce resolution Resolve events on third-party types (DevExpress, etc.) by widening the Roslyn compilation's reference set from a built `bin/` (or a restored package `lib/`), so they bind to real symbols instead of surfacing as OWN050. - Extractor: `--ref-dir <dir>` (repeatable) adds every DLL under <dir> RECURSIVELY as a MetadataReference. First simple-name wins (a TPA/framework or OWN_EXTRA_REF_DIRS reference is never double-added, so a multi-TFM bin/ references one copy); an unloadable DLL is skipped, not fatal (we read metadata only). The scriptable twin of OWN_EXTRA_REF_DIRS (which stays non-recursive for framework ref packs). - Roslyn reads metadata only, so a .NET Framework bin/ resolves exactly as modern .NET — only the DLLs differ; the mechanism is perfected in CI on net8 and ports to Framework/DevExpress unchanged. - CI `tier-b-refs` A/B job: same TierBSample.cs subscribing to a CommunityToolkit.Mvvm ObservableObject.PropertyChanged yields OWN050 WITHOUT the reference (honest skip) and a real OWN001 WITH --ref-dir — proving resolution flips the verdict. The fixture DLL is fetched from nuget (pinned 8.2.2) and pre-flighted with a pure-Python dnfile metadata read (no .NET runtime) so a fixture-rot failure is legible. - Docs: P-014 / ROADMAP / proposals index — Tier B light path shipped; full MSBuildWorkspace transitive-closure auto-discovery still deferred. 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}
| public TierBSubscriber(ObservableObject vm) | ||
| { | ||
| _vm = vm; | ||
| _vm.PropertyChanged += OnChanged; // subscribed, never -='d |
|
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a Tier B external-reference path for semantic resolution, including a new ChangesTier B external-reference resolution
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions job tier-b-refs
participant Check as Python dnfile check
participant Extractor as ownsharp-extract
participant Toolkit as CommunityToolkit.Mvvm.dll
CI->>Check: validate ObservableObject and PropertyChanged
CI->>Extractor: run TierBSample.cs without --ref-dir
Extractor->>CI: emit OWN050
CI->>Extractor: run TierBSample.cs with --ref-dir RUNNER_TEMP/refdir
Extractor->>Toolkit: load metadata references recursively
Extractor->>CI: emit OWN001
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
1114-1118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent grep anchoring between the two diagnostic codes.
The OWN001 checks anchor on
\[OWN001\](bracketed) while the OWN050 checks match bareOWN050. The bare match can false-trigger on any incidental mention of the substring (e.g., a summary/help line), making the A/B assertions slightly less precise. Anchoring both the same way (e.g.,\[OWN050\]) keeps the guards symmetric and robust.Also applies to: 1125-1128
🤖 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 1114 - 1118, The OWN050 assertion in the CI workflow uses an unanchored grep pattern, unlike the OWN001 check, so tighten the matching in the relevant shell test block by anchoring OWN050 the same way as the bracketed diagnostic code check. Update the diagnostic validation logic around the grep-based assertions so both OWN050 and OWN001 use consistent, bracketed matching and the A/B guard remains robust.frontend/roslyn/OwnSharp.Extractor/Program.cs (1)
2414-2417: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win"First simple-name wins" is non-deterministic, and a failed load permanently consumes the name.
Two related concerns in the dedup/load loop:
Directory.EnumerateFiles(dir, "*.dll", SearchOption.AllDirectories)does not guarantee a stable ordering across platforms/filesystems. When abin/carries multiple target-framework copies of the same assembly (the exact scenario called out in the comment on Line 2406-2407), which copy "wins" is effectively arbitrary, so resolution can vary run-to-run. Sorting the enumeration makes the winner deterministic and reproducible.
refNames.Add(Path.GetFileName(dll))runs before thetry. IfCreateFromFilethrows (native/corrupt DLL), the name is already marked as seen, so a later loadable assembly with the same file name elsewhere in the tree is silently skipped. Moving theAddinto the success path avoids burning the name on a failed load.♻️ Proposed adjustment
- foreach (var dll in Directory.EnumerateFiles(dir, "*.dll", SearchOption.AllDirectories)) - if (refNames.Add(Path.GetFileName(dll))) - try { references.Add(MetadataReference.CreateFromFile(dll)); added++; } - catch (Exception ex) { Console.Error.WriteLine($"extractor: skipped {Path.GetFileName(dll)}: {ex.GetType().Name}"); } + foreach (var dll in Directory.EnumerateFiles(dir, "*.dll", SearchOption.AllDirectories) + .OrderBy(p => p, StringComparer.Ordinal)) + { + var name = Path.GetFileName(dll); + if (!refNames.Contains(name)) + try { references.Add(MetadataReference.CreateFromFile(dll)); refNames.Add(name); added++; } + catch (Exception ex) { Console.Error.WriteLine($"extractor: skipped {name}: {ex.GetType().Name}"); } + }🤖 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 2414 - 2417, The dedup/load loop in Program.cs is using an unstable file enumeration and marks names as seen before a load succeeds, making the selected assembly copy non-deterministic and causing failed loads to permanently block later valid matches. Update the `Directory.EnumerateFiles(..., SearchOption.AllDirectories)` pass in the reference-building loop to use a deterministic sorted order, and change the `refNames.Add(Path.GetFileName(dll))` flow so the name is only recorded after `MetadataReference.CreateFromFile(dll)` succeeds. Keep the fix localized to the dll scan/load logic so the “first simple-name wins” behavior is reproducible and failures don’t consume the name.
🤖 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/proposals/P-014-semantic-resolution.md`:
- Around line 9-19: The Tier B proposal text uses the wrong fixture type name,
so update the paragraph in the semantic-resolution doc to match the fully
qualified type used by the sample and CI job. Replace the shortened
`CommunityToolkit.Mvvm.ObservableObject` reference with the full
`CommunityToolkit.Mvvm.ComponentModel.ObservableObject` name, keeping the rest
of the A/B test description aligned with the `TierBSample` and `tier-b-refs`
workflow context.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 1114-1118: The OWN050 assertion in the CI workflow uses an
unanchored grep pattern, unlike the OWN001 check, so tighten the matching in the
relevant shell test block by anchoring OWN050 the same way as the bracketed
diagnostic code check. Update the diagnostic validation logic around the
grep-based assertions so both OWN050 and OWN001 use consistent, bracketed
matching and the A/B guard remains robust.
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 2414-2417: The dedup/load loop in Program.cs is using an unstable
file enumeration and marks names as seen before a load succeeds, making the
selected assembly copy non-deterministic and causing failed loads to permanently
block later valid matches. Update the `Directory.EnumerateFiles(...,
SearchOption.AllDirectories)` pass in the reference-building loop to use a
deterministic sorted order, and change the `refNames.Add(Path.GetFileName(dll))`
flow so the name is only recorded after `MetadataReference.CreateFromFile(dll)`
succeeds. Keep the fix localized to the dll scan/load logic so the “first
simple-name wins” behavior is reproducible and failures don’t consume the name.
🪄 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: 48dcd2d8-aaa2-4c63-8ea2-325dc2ad15f9
📒 Files selected for processing (6)
.github/workflows/ci.ymldocs/ROADMAP.mddocs/proposals/P-014-semantic-resolution.mddocs/proposals/README.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/TierBSample.cs
…deRabbit) - Program.cs: ordinal-sort the recursive DLL enumeration so 'first simple-name wins' is reproducible across platforms; record the simple name only after a successful load so a failed DLL no longer burns the name and skips a loadable same-named assembly elsewhere in the tree. - ci.yml: anchor the Tier B A/B OWN050 assertions as [OWN050] to match the bracketed [OWN001] checks (both render bracketed) — no incidental substring match. - P-014 doc: use the fully qualified CommunityToolkit.Mvvm.ComponentModel.ObservableObject to match the sample and CI fixture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
8 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 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.
P-014 Tier B — external reference resolution (light path)
Tier A (shipped in #108) resolves
+=against the framework referenceset so
IEventSymbolbinding distinguishes real event subscriptions fromarithmetic. Tier B extends that to external package references: types
defined in NuGet/third-party assemblies (e.g. an
ObservableObjectbase fromCommunityToolkit.Mvvm) are resolved from their compiled metadata, so an
event subscription on a library-defined event is recognized as OWN001
instead of degrading to the OWN050 advisory.
What it does
--ref-dir <dir>flag on the extractor: recursively loads every*.dllunder
diras aMetadataReference, de-duped by filename against theframework refs already loaded. Roslyn binds against assembly metadata,
not source — no decompiler or runtime needed.
bin/(or any folder ofpackage DLLs). The full MSBuildWorkspace project-closure remains deferred
(tracked in P-014) — this slice covers the common "I have the built output"
case without the MSBuild dependency.
Why this is correct (proven in CI, not asserted)
The new
tier-b-refsjob is an A/B test on a real package:PropertyChangedevent actually exists in the package metadata — so thetest fails loudly if the package shape changes, rather than silently
passing.
TierBSample.cswith no--ref-dir→asserts OWN050 advisory present, no
[OWN001](unresolved base → analysisskipped, as designed).
--ref-dir'd at the restored package →asserts
[OWN001]raised, no OWN050 (base resolved → leak seen).The delta between A and B is the resolution working.
Files
frontend/roslyn/OwnSharp.Extractor/Program.cs—--ref-dirparse +recursive metadata load.
frontend/roslyn/samples/TierBSample.cs— subscribes to aCommunityToolkit
ObservableObject.PropertyChangedwithout-=..github/workflows/ci.yml—tier-b-refsA/B job with dnfile pre-flight.docs/— P-014 / ROADMAP / proposals README status updated to reflectTier B light path shipped, MSBuild closure deferred.
🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
--ref-dir <dir>argument to include external DLL references during analysis.