audit(xaml): add XAML105 MergedDictionaryKeyShadowing (in-file)#130
Merged
Conversation
Completes the merged-dictionary story (XAML104 = duplicate include) with the key shadowing case: the same x:Key defined in >=2 inline merged dictionaries, or in the primary dictionary AND an inline merged one. The effective value then depends on merge order (WPF: last merged wins, a primary key beats merged ones) — a silent order-dependence that breaks when includes are reordered. In-file only: dictionaries pulled in by Source="..." reference external files this single-file pass can't resolve, so their keys aren't compared (cross-file variant documented as a later slice). FP-safe — only literal same-key collisions flag; a lone key (the normal case) never does. - xaml_check.py: _rule_merged_dict_shadowing, registered in RULES; docstring updated. - taxonomy: XAML105 -> category 9 (merged-dictionary-shadowing). - selftest grows to 47: two inline merged dicts colliding flags; primary+merged collision flags; distinct keys don't; external Source= dicts are not compared. - README + design-note catalogue updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zoG4YNCUf7RA5r8GAXMmb
![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: b351ea4066
ℹ️ 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".
A ResourceDictionary that declares the same x:Key twice directly (or twice inside one inline merged dictionary) is a duplicate-key error within a single scope — runtime-invalid, but a different problem from XAML105's order-dependent merge shadowing. The old check fired on any key with 2+ occurrences, so two primary entries sharing a key were mislabeled as [primary, primary] shadowing. Collisions now require the key to span 2+ DISTINCT scopes (primary vs merged, or two different merged dicts). Added a selftest: two same-key primary entries plus a non-colliding merged dict must not emit XAML105. 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)
📝 WalkthroughWalkthroughAdds XAML105 for merged-dictionary key shadowing, updates taxonomy and documentation to describe the inline/in-file scope, and extends the analyzer with rule logic, registration, and selftests. ChangesXAML105 merged-dictionary shadowing
Sequence Diagram(s)sequenceDiagram
participant xaml_check.py
participant RULES
participant _rule_merged_dict_shadowing
participant XAML105
xaml_check.py->>RULES: iterate phase-1 rules
RULES->>_rule_merged_dict_shadowing: run analyzer
_rule_merged_dict_shadowing->>_rule_merged_dict_shadowing: scan primary and inline merged dictionaries
_rule_merged_dict_shadowing->>XAML105: emit shadowing finding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 1
🤖 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 432-443: The XAML105 finding in xaml_check.py still formats raw
occurrences, so same-scope duplicates can leak into the reported scope count and
location list. Update the XamlFinding construction in the XAML105 path to
deduplicate occurrences by scope before building the message, while keeping the
existing distinct-scope gate logic in place. Use the keymap/occ processing
around the XAML105 emission so the count and “where” string only reflect unique
scopes, not repeated entries from the same scope.
🪄 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: 07d3ea69-3a48-478a-86a2-269fbdc4d977
📒 Files selected for processing (4)
audit/README.mdaudit/static/taxonomy/categories.ymlaudit/static/tools/xaml_check.pydocs/notes/xaml-analyzer-design.md
The distinct-scope gate was correct, but the emitted finding still formatted raw occurrences: a key appearing twice in the primary dict AND once in a merged dict reported "3 merged/primary scopes" and listed "primary" twice. Collapse occurrences to the first line per scope before building the message, so the count and the "where" list reflect distinct scopes only. Added a selftest (dup primary + merged → "2 scopes", "primary" listed once). 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.
Completes the merged-dictionary story — XAML104 catches a duplicate include, XAML105 catches a duplicate key.
XAML105 MergedDictionaryKeyShadowing
The same
x:Keydefined in ≥2 inline merged dictionaries, or in the primary dictionary AND an inline merged one. The effective value then depends on merge order (WPF: last merged wins; a primary key beats merged ones) — a silent order-dependence that breaks when includes are reordered.Source="..."reference external files this single-file pass can't resolve, so their keys aren't compared — the cross-file variant is a documented later slice (it needs the runner to resolveSourcepaths against the scanned file set).Source=dicts are explicitly not compared.Changes
xaml_check.py—_rule_merged_dict_shadowing, registered inRULES; docstring + deferred-note updated.XAML105→ category 9 (merged-dictionary-shadowing).Source=dicts are not compared.Validation
Full audit selftest suite green (192 checks: normalize/score/report/xaml_check/xaml_facts/xaml_join/run_static/ingest),
ruff check .clean.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
x:Keyshadowing across the primary resources and inline merged dictionaries.Documentation
Sourcehandling deferred).Tests