feat(extractor): --stats coverage + lock the lambda-handler event leak#28
Merged
Conversation
…kipped) The oracle run on Dapper exposed a blind spot: "0 findings" can mean "clean" or "we didn't reach it" (async/loops/try we skip). --stats closes it. - Program.cs: count, over the --flow-locals pass, methods that have a non-escaping disposable local (the denominator worth checking) and split them into flow-analysed vs honestly-skipped (LowerFlowBody bailed on an unmodelled for/do/try/switch/async). `--stats` prints a one-line `coverage:` summary to stderr and stamps the same counts into the facts JSON as an additive `stats` object (the core's load() ignores unknown keys, verified). - own-check.sh: `--stats` passthrough. - mine.sh / mine_report.py: own-check runs with --stats during mining; the coverage line is captured from the extractor stderr and surfaced in the report header (and the "Clean" note), so a clean mine now reads "analysed N, skipped M" rather than an ambiguous zero. New `--coverage` arg; selftest 8/8. - ci.yml: a step asserts --stats emits the coverage line on stderr and the stats object in the facts JSON. Python/shell validated locally (mine_report selftest 8/8, ruff, run_tests exit 0, bash -n); the extractor C# is validated in CI (no local dotnet). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
…F track) The event-leak track already detects `evt += handler` without `-=`, but the nastiest real-world variant wasn't pinned: a LAMBDA handler. A lambda literal has no handle, so it can never be `-=`'d at all — yet the LHS still binds to the event symbol, so the extractor emits released=false and the core reports OWN001. - New sample `LambdaHandlerViewModel.cs`: `bus.CustomerChanged += (s,e) => _count++` (captures `this`, so not a static-handler exemption) — a leak with no possible un-subscribe. Reuses the existing `IEventBus` (SampleTypes.cs). - ci.yml wpf-extractor golden: the sample is added to the extracted set and a new assertion locks that `LambdaHandlerViewModel.cs` is reported (and the existing silent cases stay silent). ci.yml valid YAML; the C# golden runs in CI (no local dotnet). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Record the deliberation behind keeping `event += without -=` as OWN001: the core is already domain-neutral (OWN001 + [resource: subscription token]); P-004's WPF001..005 are profile rule mnemonics that map to OWN001/OWN014, not diagnostic codes. The capability is .NET-wide (WinForms/Avalonia/MAUI/Unity/ ASP.NET singletons/console event buses), so the WPFxxx framing in the docs over-claims. Captures the proposed neutral code families (SUB/TMR/...), the domain-neutral OwnIR facts + profile-heuristics split, and the severity-tiering direction (static->error, field/param->warning, local->drop), with the WPF/ region profile as the evidence that escalates a hedged warning to an OWN014 verdict. Naming/framing only — no core or code changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughImplements P-004 severity tiering for OWN001 subscription leaks, downgrading injected-source cases (dependencies with unproven lifetime) to warning level. Adds extractor classification for subscription sources (static, local, injected), bridge-level Finding.severity field, and detailed docs clarifying that subscription leaks are domain-neutral .NET core with WPF as a profile layer. Also introduces a ChangesOWN001 Subscription Severity Tiering (P-004)
--stats Flow-Locals Coverage Tracking and Reporting
Sequence DiagramsequenceDiagram
participant CLI as CLI args
participant Program as Program.cs
participant LowerFlowBody as LowerFlowBody
participant JSON as facts JSON
participant Stderr as stderr
CLI->>Program: --stats flag sets reportStats=true
Program->>Program: initialize coverage counters
Program->>LowerFlowBody: process each method with flow-locals
LowerFlowBody-->>Program: null/empty → increment skipped
LowerFlowBody-->>Program: non-empty → increment analysed
Program->>JSON: emit stats object with all counters
Program->>Stderr: print coverage: summary line (when reportStats=true)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![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 (1)
.github/workflows/ci.yml (1)
253-254: ⚡ Quick winStrengthen the
stats.jsonassertion with structured parsing.Line 253 only checks for a substring, so malformed JSON or wrong value types can still pass. Parse JSON and assert numeric fields/invariant instead.
Suggested CI check refinement
- grep -q '"methods_flow_analysed"' "$RUNNER_TEMP/stats.json" \ - || { echo "FAIL: expected a stats object in the facts JSON"; exit 1; } + python - <<'PY' +import json, sys +p = r"""$RUNNER_TEMP/stats.json""" +with open(p, encoding="utf-8") as f: + data = json.load(f) +stats = data.get("stats") +if not isinstance(stats, dict): + raise SystemExit("FAIL: missing stats object") +required = ("methods_with_local", "methods_flow_analysed", "methods_skipped_unmodelled") +if not all(isinstance(stats.get(k), int) for k in required): + raise SystemExit("FAIL: stats fields missing or non-integer") +if stats["methods_flow_analysed"] + stats["methods_skipped_unmodelled"] != stats["methods_with_local"]: + raise SystemExit("FAIL: stats invariant violated") +PY🤖 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 253 - 254, The current check using grep to search for the substring "methods_flow_analysed" in stats.json is too weak and doesn't validate actual JSON structure or field values. Replace the grep-based substring check with proper JSON parsing using jq or similar JSON query tool to extract and validate the methods_flow_analysed field, ensuring it exists with the expected type and value. This will catch malformed JSON and incorrect field types that would currently pass the weak substring test.
🤖 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 `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Line 53: The `--stats` flag can be enabled independently without
`--flow-locals`, allowing the coverage summary emission at line 684 to report
statistics even when flow analysis never ran, which is misleading. Add a guard
to ensure that when the `reportStats` flag is set to true (in the argument
parsing where `--stats` is handled), validate that flow analysis was actually
enabled. Either prevent `--stats` from being processed without `--flow-locals`
being enabled, or add a validation check before emitting the coverage summary
around line 684 to ensure flow analysis actually executed before reporting any
statistics.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 253-254: The current check using grep to search for the substring
"methods_flow_analysed" in stats.json is too weak and doesn't validate actual
JSON structure or field values. Replace the grep-based substring check with
proper JSON parsing using jq or similar JSON query tool to extract and validate
the methods_flow_analysed field, ensuring it exists with the expected type and
value. This will catch malformed JSON and incorrect field types that would
currently pass the weak substring test.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8174feef-071d-49aa-b88f-24e26b00c6ca
📒 Files selected for processing (7)
.github/workflows/ci.ymldocs/notes/subscription-leaks-and-profiles.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/LambdaHandlerViewModel.csscripts/mine.shscripts/mine_report.pyscripts/own-check.sh
…assertion Two CodeRabbit nits on #28: - Program.cs: `--stats` reports flow-locals coverage, but the counters only move inside the `--flow-locals` pass. Without it the summary reads "0/0 methods flow-analysed" — the exact ambiguous zero --stats exists to kill (reachable via `own-check.sh --legacy --stats`). Refuse the contradictory combo with a clear error (return 2). - ci.yml: replace the `grep '"methods_flow_analysed"'` substring check with a jq parse that asserts the stats object exists, all three counters are numbers, and the invariant holds (flow_analysed + skipped == with_local). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
A bare `event += handler` leak is no longer always a hard error. The severity now
follows what we can prove about the event SOURCE's lifetime — the honest answer
until ownership/lifetime modelling lands, and it kills the bulk of the false
positives mining real repos surfaced:
- extractor (Program.cs): classify the source at the emission point (the symbol
info is already there). `SubscriptionSourceKind` returns static / local /
injected; a LOCAL source is method-bounded and can't outlive `this`, so it is
dropped (not a heap leak). Static/injected ride along as a `source` hint, plus a
`lambda` flag for inline-lambda handlers. Self-owned / static-handler exemptions
are unchanged. Timers stay error (a running timer is dispatcher-rooted).
- core (ownir.py): Finding gains an optional per-finding `severity`. An injected
source (ctor param / field / property of unknown lifetime) -> OWN001 at WARNING
("possible leak"); a static event -> error (None = host default). A lambda
handler adds an honest "no `-=` handle, can never be detached" note.
- renderer (__main__.py): severity is the weaker of the host's --severity and the
finding's intrinsic level (advisory and --severity warning still win).
Behavior change: CustomerViewModel + LambdaHandlerViewModel (both subscribe to an
injected `bus`) now report as warnings, not errors — documented in the samples.
The started-never-stopped timer stays an error. CI asserts both sides; a new
unit test pins the core's per-finding verdict (no dotnet needed).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
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 `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 178-192: The SubscriptionSourceKind method currently classifies
any local variable receiver as "local" (line 187-188 when recv is ILocalSymbol),
but this misses real leaks when a local aliases an injected or static source.
Instead of immediately returning "local" when you encounter an ILocalSymbol
receiver, you need to perform a conservative provenance check to determine what
that local variable actually references or is initialized from. If the local
aliases an injected or static dependency, return the appropriate classification
for the actual source instead of "local", otherwise fall through to the default
"injected" return value.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c09018e-d79f-4030-8649-7faffd365f3a
📒 Files selected for processing (7)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/CustomerViewModel.csfrontend/roslyn/samples/LambdaHandlerViewModel.csownlang/__main__.pyownlang/ownir.pytests/test_ownir.py
✅ Files skipped from review due to trivial changes (1)
- frontend/roslyn/samples/CustomerViewModel.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
Addresses a false-negative in the source-kind tiering raised in review: SubscriptionSourceKind classified ANY local receiver as "local" and the emission dropped it — but a local that aliases an injected/long-lived source (`var src = _bus; src.X += OnChanged;`) is a real leak, silently lost. A local is now "local" (method-bounded, dropped) ONLY when it is the publisher the scope constructs (`var p = new Publisher()`), which dies with it. A local initialised from anything else (a field, a parameter, a call) has unknown provenance and falls through to "injected" -> WARNING, never dropped. Mirrors the existing constructed/self-owned ownership rule; nothing escalates to error. New AliasedSourceViewModel sample + CI assertions lock both sides: the aliased injected source warns, the locally-new'd publisher stays dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
This was referenced Jun 18, 2026
This was referenced Jul 10, 2026
Merged
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.
Two commits, two follow-ups from the oracle work.
1.
feat(extractor): --stats flow-locals coverageThe Dapper oracle run exposed a blind spot: "0 findings" could mean "clean" or "we didn't reach it" (async / loops /
trywe honestly skip).--statscloses it.Program.cs: over the--flow-localspass, count methods that have a non-escaping disposable local (the denominator worth checking) and split them into flow-analysed vs honestly-skipped (LowerFlowBodybailed on an unmodelled construct).--statsprints a one-linecoverage:to stderr and stamps the same counts into the facts JSON as an additivestatsobject (the core'sload()ignores unknown keys — verified).own-check.sh:--statspassthrough.mine.sh/mine_report.py: mining runs own-check with--stats; the coverage line is captured and surfaced in the report header and the "Clean" note — so a clean mine now reads "analysed N, skipped M" instead of an ambiguous zero. New--coveragearg; selftest 8/8.ci.yml: a step asserts--statsemits the coverage line + the stats object.2.
feat(extractor): lock the lambda-handler event leak in the goldenThe event-leak track already catches
evt += handlerwithout-=, but the nastiest variant wasn't pinned: a lambda handler. A lambda literal has no handle, so it can never be-='d at all — yet the LHS still binds to the event, so it's a real OWN001.LambdaHandlerViewModel.cs(bus.CustomerChanged += (s,e) => _count++— capturesthis, so not a static-handler exemption).Verified
Python/shell locally:
mine_report --selftest8/8,ruffclean,run_tests.pyexit 0,bash -non both scripts,ci.ymlvalid YAML. The extractor C# is validated in CI (no local dotnet here) — the newCoverage summary (--stats)and lambda assertions are the gates.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
--flow-locals --statsreporting with a one-line coverage summary.OWN001severity behavior.OWN001severity tiering (including injected vs static and inline-lambda wording).-=detach messaging.