xaml: XAML105 cross-file Source= merged-dictionary key shadowing#135
Merged
Conversation
Closes the documented deferred tail of XAML105. The in-file rule only sees inline merged dictionaries; this adds a project-level pass that resolves <MergedDictionaries> Source= includes to real files and flags an x:Key that collides across scopes when at least one is an external dictionary — the classic case of a theme dictionary silently shadowing (or being shadowed by) an app dictionary's key, order-dependent and invisible to a single-file pass. - _resolve_source handles relative, app-root (/Colors.xaml) and pack (;component/) Source forms; anything that escapes the target, carries an http(s)/siteoforigin scheme, or doesn't land on a scanned file is skipped — an unresolved include can never manufacture a false collision. - analyze_cross_file runs outside the per-file RULES loop (it needs every parsed tree + paths at once) and is disjoint from the in-file rule by construction: it emits only when an external file participates AND the host's own primary/inline scopes don't already collide, so a host the in-file rule flags is never double-reported. Findings anchor at the host include site. - Selftests: primary+external, two externals, pack-form resolution, and the negatives (distinct keys, missing file, http Source, in-file-disjoint). Docs (design note + audit README) updated to mark cross-file done. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zoG4YNCUf7RA5r8GAXMmb
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe XAML analyzer now runs a cross-file XAML105 pass that resolves ChangesXAML105 cross-file shadowing
Sequence Diagram(s)sequenceDiagram
participant run_xaml_check
participant analyze_cross_file
participant _resolve_source
participant _external_keys
participant parsed trees
run_xaml_check->>analyze_cross_file: parsed trees and root path
analyze_cross_file->>parsed trees: index host and external XAML files
analyze_cross_file->>_resolve_source: resolve Source="..." include
analyze_cross_file->>_external_keys: read keyed resources from external dictionary
analyze_cross_file->>run_xaml_check: emit XAML105 findings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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: f01419c363
ℹ️ 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".
Codex review: stripping the 'Lib;component/' prefix and probing target_root/Themes/Colors.xaml could match an unrelated same-named file in a multi-project target — the app's own Themes/Colors.xaml instead of Lib's — inventing a false XAML105 collision and breaking the "skip rather than guess" contract. A pack '/Asm;component/<path>' Source now resolves <path> against the named assembly's project directory, located by a unique <Asm>.csproj (indexed once per run). When the assembly maps to no project, or to more than one (ambiguous), the include is skipped rather than guessed. Relative and app-root forms are unchanged. Added selftests: pack-via-csproj resolution, a multi-project case that must resolve to the named assembly's dictionary, and the FP guard where the named assembly lacks the file (must not fall back to another project's copy). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zoG4YNCUf7RA5r8GAXMmb
Owner
Author
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@audit/static/tools/xaml_check.py`:
- Around line 590-607: The cross-file XAML105 finding is still being anchored to
the earliest resource origin instead of the include site when a host key
collides with an external dictionary. In the XAML105 aggregation logic in
xaml_check.py, update the anchor selection in the block that builds the
XamlFinding so it prefers the include-site line from the external entry set (the
child.line captured in occ) rather than min(by_origin.values()). Also adjust the
selftest to assert the exact include line for the primary+external collision
case, so the SARIF location matches the contract.
- Around line 499-503: The Source URI resolution in xaml_check.py is too
permissive for plain relative paths, because the fallback to target-root can
incorrectly treat a missing include as a match and trigger false XAML105
collisions. Update the URI handling logic in the relative-path branch so that
plain relative Source values are resolved only against the containing XAML/base
context (the host_dir-based lookup) and do not fall back to troot; keep the
app-root behavior in the same resolution flow but remove the repo-root fallback
for non-absolute relative URIs. Verify the change in the Source
parsing/resolution path where rel and bases are computed.
🪄 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: bbbe5ee2-c24d-43c4-97e0-b9c59ff310b5
📒 Files selected for processing (3)
audit/README.mdaudit/static/tools/xaml_check.pydocs/notes/xaml-analyzer-design.md
…ive resolve
Two CodeRabbit review findings on the cross-file pass:
- Anchor: min(by_origin.values()) picked the primary resource line whenever the
host key sat above the Source= include, contradicting the rule's own "anchor
at the host include site" contract and pointing SARIF at the wrong line for
primary+external collisions. Now anchors at the earliest external include line
(the any-external guard makes that set non-empty). Selftest tightened to assert
the exact include line (4) rather than just >= 1.
- Relative Source resolution: WPF resolves a plain relative Source against the
containing file's base URI only, with no application-root fallback. The extra
target-root probe could map a missing include onto an unrelated same-named file
and invent a collision. Relative now resolves against host_dir only; app-root
("/X") and pack ";component/" forms are unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zoG4YNCUf7RA5r8GAXMmb
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.
What
Closes the documented deferred tail of XAML105 (merged from #130). The in-file rule only sees inline merged dictionaries; this adds a project-level pass that resolves
<MergedDictionaries>Source=includes to real files and flags anx:Keythat collides across scopes when at least one is an external dictionary.The classic case it catches: a theme dictionary silently shadowing — or being shadowed by — an app dictionary's key. It's order-dependent (last merged wins, primary beats merged) and completely invisible to a single-file pass.
How
_resolve_sourcehandles the real WPFSource=forms: relative (Themes/Colors.xaml), app/site-root (/Colors.xaml), and pack/component (pack://application:,,,/Asm;component/Themes/Colors.xamlor the bare/Asm;component/...). Anything that escapes the scanned target, carries anhttp(s)/siteoforiginscheme, or doesn't land on an existing scanned file returnsNone— an unresolved include can never manufacture a false collision (the conservative half).analyze_cross_fileruns outside the per-fileRULESloop because it needs every parsed tree + their paths at once. It is disjoint from the in-file rule by construction: it emits only when an external file participates AND the host's own primary/inline scopes don't already collide (< 2distinct in-file scopes for that key) — so a host the in-file rule already flags is never double-reported. Findings anchor at the host include site, at the same path the host's other findings use.Tests
xaml_check --selftest(now 61 checks) covers: primary+external collision (with message + anchor assertions), two-external collision, pack-form resolution, and the negatives — distinct keys, aSourceresolving to no file, anhttp(s)Source, and the in-file-disjoint guard. End-to-end verified throughrun_xaml_check(SARIF emitted at the host include site naming the resolved external dictionary).Taxonomy needs no change (XAML105 already mapped → cat 9
merged-dictionary-shadowing). Design note + audit README updated to mark cross-file done.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Source="..."inclusions, including commonSourcereference forms when they resolve to scanned markup files.Tests
Documentation