Add PropertyChanged-storm profiler (Plan.md §2 cat. 6)#104
Conversation
The last runtime detector in the set. Frequency — not correctness — is a runtime property: the static INPC0xx tier (cat. 5) catches a missing nameof or a broken arg, but cannot see that a property fires PropertyChanged thousands of times for one user operation, half of them with no value change, thrashing every binding. This profiler measures that storm. - taxonomy: map RUNTIME-PROPCHANGED-STORM -> category 6 (P2). A storm in the same file as a static INPC0xx hit clusters with it -> high confidence (§3.5). - ingest.py: add propertychanged_storm_to_sarif + --propertychanged-storm (in the mutually-exclusive source group). When the instrumentation resolved a source file the finding keeps it (so it clusters with INPC0xx); otherwise it gets a unique inpc://<type>/<property> synthetic uri so distinct storming properties stay distinct clusters. Generalized the duplicate-detector's _dup_uri into a shared _synthetic_uri(scheme, ...) helper (heap:// vs inpc://). Selftest proves: below-threshold dropped, level warning, located storm keeps its line + clusters with a static INPC003 (1 high-confidence cluster), distinct properties stay distinct, category 6. - run_static.py: fold propertychanged-storm.sarif into the runtime pickup loop; selftest asserts it. - PropertyChangedStorm/ (C#, Windows/build-required, NOT CI-gated): reads an ETW .etl (Microsoft.Diagnostics.Tracing.TraceEvent) emitted by a diagnostic build's INPC EventSource, aggregates per (type, property), thresholds raises-per- operation, emits JSON. Hardened CLI/exit-code style matching the other harnesses. Selftests: ingest 24/24, run_static 17/17, normalize/score/report unchanged; ruff clean; C# brace/paren-balanced with all TraceEvent tokens present. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QDPpNT9Uh8RoTrKPvgcoRE
|
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 due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a new ChangesPropertyChanged storm runtime pipeline
Sequence Diagram(s)sequenceDiagram
participant PropertyChangedStormProfiler
participant audit_runtime_ingest_py as audit/runtime/ingest.py
participant audit_static_run_static_py as audit/static/run_static.py
PropertyChangedStormProfiler->>audit_runtime_ingest_py: emits propertychanged-storm JSON
audit_runtime_ingest_py->>audit_static_run_static_py: writes propertychanged-storm.sarif
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2dd38ae7b
ℹ️ 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".
…review) Address Codex: propertychanged_storm_to_sarif wrote the bare source file at line 0 whenever the profiler resolved a SourceFile but not a SourceLine (the C# falls back to 0). The scorer clusters by basename + line window, so every file-only storm in one file collapsed into a single cluster, and a static finding on lines 1-3 could become a false high-confidence match. A source location is now usable only when line >= 1 (the same discipline as render_sarif's "region only when line>=1"); otherwise the finding keeps its unique per-property inpc:// synthetic uri. Selftest adds a file-only (line 0) storm and asserts it falls back to a synthetic uri with no region. ingest selftest 25/25; run_static 17/17; ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QDPpNT9Uh8RoTrKPvgcoRE
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/runtime/ingest.py`:
- Around line 356-385: The selftest in ingest should cover unresolved storm
findings more strongly: add a second unresolved property alongside the existing
synthetic storm result so the test exercises multiple unresolved `inpc://...`
URIs, then tighten the assertions in
`propertychanged_storm_to_sarif`/`storm["runs"][0]["results"]` to verify those
synthetic URIs are distinct (or that they form separate clusters) rather than
only checking the total count and the single resolved
`src/Vm/DeclarationViewModel.cs` location.
In `@audit/runtime/README.md`:
- Around line 104-107: Update the README section that describes unresolved storm
clustering to match the synthetic URI shape produced by ingest.py: the current
wording says `inpc://<type>/<property>`, but the actual format is
`inpc://<type>/<nnnn>-<slug>`. Adjust the documentation in the relevant README
paragraph so it precisely reflects the URI emitted by the storm handling logic
in ingest.py and stays consistent with any report/assertion expectations.
🪄 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: 51167418-6d61-4143-a780-6dbcca7ac0c3
📒 Files selected for processing (6)
audit/runtime/PropertyChangedStorm/Program.csaudit/runtime/PropertyChangedStorm/PropertyChangedStorm.csprojaudit/runtime/README.mdaudit/runtime/ingest.pyaudit/static/run_static.pyaudit/static/taxonomy/categories.yml
…iew) Address two CodeRabbit nitpicks: - ingest.py selftest: assert the two UNRESOLVED storms (Subtotal: no location, Discount: file-only line 0) get DISTINCT synthetic inpc:// uris, not just that the total uri count is 3 — so a regression that reused one synthetic uri for every unresolved property is caught directly. - README: the synthetic uri is inpc://<type>/<NNNN>-<property> (indexed), not inpc://<type>/<property>; correct the docs to match what ingest.py emits. ingest selftest 26/26; run_static 17/17; ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QDPpNT9Uh8RoTrKPvgcoRE
What
The last runtime detector in the set: a PropertyChanged-storm profiler for Plan.md §2 category 6. Frequency — not correctness — is a runtime property. The static
INPC0xxtier (cat. 5) catches a missingnameofor a broken arg; it cannot see that a property firesPropertyChangedthousands of times for one user operation, half of them with no value change, thrashing every binding. This profiler measures that storm.How it fits the existing pipeline
Same shape as the leak-harness and duplicate-detector — a C# tool emits JSON,
ingest.pybridges it to SARIF, andrun_staticfolds it into the samenormalize → score → reportpipeline (so a storm in the same file as a staticINPC0xxhit clusters with it → high confidence, §3.5).categories.yml)RUNTIME-PROPCHANGED-STORM→ category 6 (P2)ingest.pypropertychanged_storm_to_sarif+--propertychanged-stormflag (in the mutually-exclusive source group). Keeps a resolved source line when present (→ clusters withINPC0xx); else a uniqueinpc://<type>/<property>synthetic URI so distinct storming properties stay distinct clusters. Generalized the duplicate-detector's_dup_uriinto a shared_synthetic_uri(scheme, …)helper (heap://vsinpc://).run_static.pypropertychanged-storm.sarifinto the runtime pickup loopPropertyChangedStorm/(C#).etl(Microsoft.Diagnostics.Tracing.TraceEvent) emitted by a diagnostic build's INPCEventSource, aggregates per(type, property), thresholds raises-per-operation, emits JSON. Windows/build-required, not CI-gated.Verification
ingest.py --selftest→ 24/24 (below-threshold dropped, levelwarning, located storm keeps its line + forms 1 high-confidence cluster with a staticINPC003, distinct properties stay distinct, category 6)run_static.py --selftest→ 17/17 (asserts the storm SARIF is picked up)normalize19/19,score11/11,report34/34 — unchangedruff check audit/clean; C# brace/paren-balanced with all TraceEvent tokens present[P2 · propertychanged-storm]clustersAs with the other harnesses, the C# is Windows/build-required and verified by reading (not compiled in Linux CI) — the Python bridge is what CI exercises.
🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
propertychanged-stormruntime diagnostic and integrated its SARIF output into the unified analysis pipeline.propertychanged-stormresults, and taxonomy includes category 6 with updated severity (P2).report:falsefindings are omitted and related storm hits form correct high-confidence clusters.propertychanged-storm.