audit(xaml): emit the Phase-2 facts layer (xaml-facts.json)#115
Merged
Conversation
Builds the Phase-1 -> Phase-2 seam from the design note: the markup pass now
emits a structured fact document alongside its SARIF, so the existing Roslyn
extractor (frontend/roslyn/OwnSharp.Extractor -> OwnIR) and the binding-path
join have something to read instead of re-parsing XAML.
- xaml_facts.py: extracts the two fact families from the parsed tree —
XamlResourceGraph (resources, merged_dictionaries) and XamlBindingFacts
(bindings with a parsed binding-markup parser: path / mode /
UpdateSourceTrigger / converter / Delay / RelativeSource, plus event_handlers,
converters_used, and the file's x:Class). Envelope mirrors OwnIR's
*.facts.json ({xaml_facts_version, module, documents, totals}). Pure stdlib.
- xaml_check: split analyze_text into analyze_root + analyze_text so run_xaml_check
parses each file once and feeds the same tree to both the rules and the facts
extractor; it now writes xaml-facts.json next to xaml-check.sarif.
- selftest (16 checks): the binding-markup parser (positional path, nested
Converter, RelativeSource, TemplateBinding, non-binding rejection, brace-aware
comma split), document facts on a representative view, EventSetter handlers,
and the OwnIR-parallel envelope. Wired into CI.
- design note + README: document the seam and that Phase 2 consumes (not
re-derives) xaml-facts.json.
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new XAML facts extractor, updates ChangesXAML Phase-2 seam
Sequence Diagram(s)sequenceDiagram
participant run_xaml_check
participant parse_xaml
participant analyze_root
participant document_facts
participant module_facts
participant Sarif as xaml-check.sarif
participant Facts as xaml-facts.json
run_xaml_check->>parse_xaml: parse each XAML file once
run_xaml_check->>analyze_root: run Phase-1 rules on the tree
run_xaml_check->>document_facts: collect per-file facts
run_xaml_check->>module_facts: aggregate document facts
run_xaml_check->>Sarif: write findings
run_xaml_check->>Facts: write facts artifact
Estimated code review effort🎯 4 (Complex) | ⏱️ ~30-90 minutes 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: 892cdd1246
ℹ️ 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) Two correctness fixes from the Codex review on PR #115: - _resource_scope derives a keyed resource's scope from its nearest owning <X.Resources> ancestor, not the dictionary element. The explicit-wrapper form <UserControl.Resources><ResourceDictionary>... previously recorded scope "root" (the inner ResourceDictionary is not a property element), collapsing the view-local-vs-global distinction the Phase-2 resource graph relies on. Now it keeps "UserControl". - document_facts also captures property-element bindings — <TextBlock.Text> <Binding Path="Name" Converter="{StaticResource money}"/></...> — via a new binding_from_element (attributes read like parse_binding reads the markup-ext string, incl. a nested <Binding.RelativeSource>). The owning control/property come from the parent <Type.Prop> element. These forms are common once a binding needs a converter/relative-source, and were silently missing from bindings / converters_used, so the Roslyn join would skip those paths. Selftest grows to 19 checks (explicit-wrapper scope + property-element binding). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zoG4YNCUf7RA5r8GAXMmb
![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 735-737: The artifact writes in run_xaml_check are still
unconditional, so a permission or disk failure from sarif_path.write_text or
facts_path.write_text can abort the run despite the best-effort contract. Wrap
the SARIF and facts file writes in separate try/except blocks inside
run_xaml_check, log or otherwise record the failure, and continue so the
function preserves the “never a crash” behavior; use the existing helpers around
_to_sarif, module_facts, sarif_path, and facts_path to locate the write
sequence.
In `@audit/static/tools/xaml_facts.py`:
- Around line 169-177: The binding collector in xaml_facts.py only handles
attribute-value bindings in the n.attrib loop, so property-element bindings like
TextBlock.Text/Binding are missed. Update the binding extraction logic around
the current parse_binding path to also inspect child elements representing
verbose/property-element bindings, convert them into the same bindings record
shape used in bindings.append, and keep updating converters from any discovered
b["converter"]. Reference the existing parse_binding, bindings.append, and
converters.add flow so the new handling stays consistent with the current
attribute-based parsing.
🪄 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: b03b93b0-ed08-4f24-8525-577e4bf9854b
📒 Files selected for processing (5)
.github/workflows/ci.ymlaudit/README.mdaudit/static/tools/xaml_check.pyaudit/static/tools/xaml_facts.pydocs/notes/xaml-analyzer-design.md
run_xaml_check documents a never-crash, best-effort contract, but the xaml-check.sarif / xaml-facts.json write_text calls were unconditional, so a permission/disk error would raise and abort the run. Wrap both writes in a single try/except OSError that records the failure as the tier reason and returns available=False — consistent with how a missing target or absent SDK is handled. (The other CodeRabbit comment, property-element bindings, was already addressed in cdd089d.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zoG4YNCUf7RA5r8GAXMmb
This was referenced Jun 26, 2026
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.
Builds the Phase-1 → Phase-2 seam from the XAML analyzer design note: the markup pass now emits a structured fact document alongside its SARIF, so the existing Roslyn extractor (
frontend/roslyn/OwnSharp.Extractor→ OwnIR) and the binding-path join have something to read instead of re-parsing XAML.This is the follow-up to #114 (the Phase-1 markup runner). It adds the facts layer only — no rule changes, no findings changes.
What's in it
audit/static/tools/xaml_facts.py— extracts the two fact families from the parsed tree (the design note's split):resources(keyed type + scope + line) andmerged_dictionaries.bindings(a parsed binding-markup parser:path/mode/UpdateSourceTrigger/converter/Delay/RelativeSource),event_handlers(Click=/EventSetter),converters_used, and the file'sx:Class. These are the pointers into C# the Roslyn join resolves.*.facts.json({xaml_facts_version, module, documents, totals}) so the two fact sources read alike. Pure stdlib — no toolchain, runs on Linux CI.xaml_check.py— splitanalyze_textintoanalyze_root+analyze_textsorun_xaml_checkparses each file once and feeds the same tree to both the rules and the facts extractor; it now writesxaml-facts.jsonnext toxaml-check.sarif.Converter,RelativeSource,TemplateBinding, non-binding rejection, brace-aware comma split), document facts on a representative view,EventSetterhandlers, and the OwnIR-parallel envelope. Wired into CI.xaml-facts.json.Why
Phase 2 (the Roslyn-linked binding-path join →
XAML2xx) needs structured XAML facts to join against OwnIR. The "Layer 2" machinery it joins to —OwnSharp.Extractor(→ OwnIR) and the ClrMD collectors underaudit/runtime/— already exists in this repo; what was missing was the XAML-side fact input. This PR supplies it. The join itself and Phase 3 (runtime correlation) remain deferred.Validation
Full audit selftest suite green (normalize/score/report/xaml_check/xaml_facts/run_static/ingest) and
ruff check .clean. The new module gates on Linux CI like the other build-free runners.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
xaml-facts.json) alongside existing analysis output.Bug Fixes
Documentation