fix(mine_report): only absorb multi-line continuations while brackets are unbalanced The multi-line parser fix (85cdded) absorbed ANY non-matching line after a finding, so a stray line following a COMPLETE finding (e.g. build chatter leaking to stdout) was folded into its message instead of counted — breaking mine_report's selftest ("expected 1 unparsed, got 0"), the lint-job failure on the last two pushes. Track the running bracket balance of the finding's message: a real multi-line lambda body leaves it unbalanced, a complete finding is balanced. Absorb continuation lines only while unbalanced; once balanced, a further non-matching line is stray drift again. Both selftests green (mine_report 8/8, oracle_compare 19/19); the multi-line ScreenToGif findings still parse and a trailing stray line is still counted.#29
Merged
Conversation
The mine workflow only surfaced its report to the run summary and an artifact. The eval loop (API/agent) reads job logs, not summaries or artifacts, so also echo report.md and findings.txt to stdout (grouped) — no behavior change to the mining itself, just makes the result readable from the run log. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Adds .github/workflows/mine-on-push.yml + corpus/mine-target.txt so the corpus eval loop runs without manual workflow_dispatch (which the agent token can't do). On a push that changes the sentinel, the workflow allowlist-validates the target (owner/repo or https URL), runs scripts/mine.sh on it, and echoes the report to the job log. Dev-branch scoped; remove before any merge to main. First target: DapperLib/Dapper (baseline signal/noise read). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Baseline mine on DapperLib/Dapper validated the loop: 1 verified true-positive (BenchmarkBase._connection, an undisposed SqlConnection field) + 1 OWN050. Hunt continues on a shipping-code library for a higher-impact field/subscription leak. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
To unlock the flagship WPF subscription/timer detectors on real apps, the extractor must resolve WPF framework types (it currently loads only the runtime TPA -> Button.Click/DispatcherTimer.Tick fall out as OWN050). This adds a TEMP probe step that restores a net8.0-windows/UseWPF stub (EnableWindowsTargeting) and reports where PresentationFramework.dll lands on the Linux runner, so the extractor change can point at the right ref dir. Sentinel -> a real WPF app (NickeManarin/ScreenToGif) for a no-refs baseline in the same run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
…RS (WPF unlock) The extractor built its reference set purely from the runtime TPA, so WPF framework events/timers (Button.Click, DispatcherTimer.Tick) couldn't bind and fell out as OWN050 on real WPF apps — blinding the flagship subscription/timer detectors. Now it also loads *.dll from each dir in OWN_EXTRA_REF_DIRS (colon- separated), deduped by simple name against the TPA so System.* isn't double- referenced. Unset => unchanged behaviour (the whole existing suite is the guard). mine-on-push materializes the WindowsDesktop ref pack on Linux (a net8.0-windows /UseWPF stub restores it via EnableWindowsTargeting -> 47 ref dlls) and exports the dir as OWN_EXTRA_REF_DIRS. Re-mining ScreenToGif with the pack loaded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
… + write-up
Milestone 1 ("find 1-3 real subscription/timer leaks in real code") is met:
mining real OSS C# surfaced a real WPF view->view-model subscription leak in
ScreenToGif (VideoSource.xaml.cs — four inline-lambda handlers wired in
Window_Loaded, never detached). Capture it and write up the run.
- corpus/real-world/screentogif-loaded-subscription/: the finding reduced to a
tested regression (case.own -> OWN001, before/after .cs, provenance in notes).
- docs/notes/real-world-mining.md: the run write-up — the autonomous push-miner
loop, the 4-repo findings table + precision verdict, the flagship finding, the
WPF reference unlock (OWN_EXTRA_REF_DIRS: OWN050 210->37), and the next gap it
revealed (self-owned controls built via ref-params / template parts).
- docs/ROADMAP.md: mark milestone 1 progressed, link the write-up.
Full suite green (corpus 3/3).
…-built fields and template parts
The WPF mining run (docs/notes/real-world-mining.md) showed that loading the WPF
reference pack jumps OWN001 from 8 to 123, dominated by FALSE positives on
self-owned controls: a class subscribing to a control it owns is a collectable
cycle, not a leak. The existing exemption only recognised fields built by a direct
`field = new ...`, so two real ownership shapes slipped through:
* indirect construction — `BuildCorner(ref _thumb, ...)`: a helper the class
calls populates the field (the adorner pattern);
* template parts — `_part = GetTemplateChild("PART_x") as T` / FindName: a
control owns the parts of its own template.
Fold both into a separate `selfOwned` set used by the subscription exemption only.
It is deliberately kept OUT of `constructed`, so the WPF003 disposal detector keeps
demanding disposal of `new`'d fields only (you do not dispose a borrowed-by-ref
value or a template part). Detection is AST-based — the ref/out argument keyword,
and a GetTemplateChild/FindName call name behind an optional cast / `as` — in the
spirit of the rest of the extractor.
Verification: new sample SelfOwnedControlParts.cs exercises both shapes; the ci.yml
wpf-extractor job asserts it produces no finding, alongside the existing self-owned
silence checks.
…ed-control fix Dev-loop only (mine-on-push, dev-branch scaffolding). Re-run the WPF-profile mine over ScreenToGif now that the self-owned exemption covers ref/out-built fields and GetTemplateChild template parts, to confirm the OWN001 self-owned-control false positives drop from the 123 baseline.
…> 36 on ScreenToGif) Re-mining ScreenToGif with the WPF profile after the exemption extension confirms OWN001 drops from the 123 baseline to 36 findings: the adorner / template-part false positives are gone, while the real leaks survive — two SystemEvents.DisplaySettingsChanged subscriptions (flagged error, the classic static-source leak, in GraphicsConfigurationDialog / Troubleshoot) and the four VideoSource view->view-model lambdas (warning). Close the documented "next gap" and note the new SystemEvents finding; update milestone 1.
…regression The WPF-profile mine surfaced a second real leak: GraphicsConfigurationDialog and Troubleshoot each subscribe to Microsoft.Win32.SystemEvents.DisplaySettingsChanged (a static, process-lifetime source) and never unsubscribe — the classic SystemEvents leak. Because the source is static it provably outlives the window, so the extractor rates it an ERROR (vs VideoSource's injected -> warning). Add corpus/real-world/screentogif-systemevents-leak/ (case.own -> OWN001, before/after .cs, provenance + tiering in notes.md); the corpus test now runs 4/4. Point the write-up at both locked regressions.
…ents cross-check) The automation token can't workflow_dispatch oracle.yml (403), so add a push trigger gated on a sentinel (corpus/oracle-target.txt), mirroring mine-on-push. A "Resolve target" step feeds repo/ref/paths/build/include_tests from inputs (on dispatch) or the sentinel (on push); every downstream step now reads steps.t.outputs.*, so both triggers share the same tested CodeQL/Infer#/diff pipeline. The dispatch path is unchanged. Dev-branch scaffolding — remove before merging to main. Sentinel points at NickeManarin/ScreenToGif @27a49c3 to cross-check the SystemEvents.DisplaySettingsChanged finding: does CodeQL (and Infer#, if the WPF target builds on Linux) also flag it, or is it Own.NET-only (differentiated)?
…n ScreenToGif) The oracle's clone did `git clone --depth 1` then `git fetch --depth 1 origin $REF` — which fails for an abbreviated SHA (servers reject unadvertised/short SHAs in a want request), so the ScreenToGif cross-check died at the Clone step. Use a blobless clone (`--filter=blob:none`, full commit/ref graph, blobs on demand) and `checkout --detach "$REF"`, which resolves any ref — branch, tag, or short SHA — locally. Keep the fast `--depth 1` path when no ref is given. Bump the sentinel to re-run on NickeManarin/ScreenToGif @27a49c3.
…check framework refs The ScreenToGif oracle run mis-reported "Own.NET = 3" with "38 own-check line(s) did not parse" — two bugs, so the diff's Own.NET side was unusable (the SystemEvents / VideoSource findings never showed): 1. Parser drift (scripts/mine_report.py, shared with the miner): _LINE *required* a trailing `[resource: kind]` tag and matched single lines only, so findings without the tag — and multi-line findings (an inline lambda handler echoed across lines, the VideoSource shape) — failed to parse. Make the tag optional (msg non-greedy) and fold continuation lines into the current finding's message, recovering a late `[resource:]` kind from the last line. Add a multi-line / no-tag regression to the oracle_compare selftest (16 -> 19 checks). 2. Config (.github/workflows/oracle.yml): own-check ran at the default --severity error and without OWN_EXTRA_REF_DIRS, so SystemEvents (unresolved -> OWN050) and the warning-tier leaks (VideoSource) were never emitted. Materialize the WindowsDesktop ref pack and run own-check at --severity warning, so framework events resolve and warning-tier subscription leaks are included — putting own-check on equal footing with CodeQL (which resolves types from source). Bump the sentinel to re-run on ScreenToGif @27a49c3 for a clean Own.NET-vs-CodeQL diff.
…ubscription leaks) The oracle run on ScreenToGif @27a49c3 (Own.NET vs CodeQL; Infer# skipped — WPF won't build on Linux) shows the two tools' leak findings are nearly disjoint: SystemEvents (error) and VideoSource (warning) land in "Own.NET only" alongside a pile of own-control subscriptions, CodeQL's 33 leak findings are entirely Dispose/RAII (cs/local-not-disposed, cs/dispose-not-called-on-throw), and the leak files overlap on exactly one. So the differentiation is confirmed by the oracle, not just argued — the tools are complementary. Note the honest caveats (Infer# skipped; our Dispose-class recall gap) and the two oracle bugs fixed to get a trustworthy diff.
… run (incl. Infer#) ScreenToGif is WPF and won't dotnet build on the Linux oracle runner, so Infer# was skipped — the cross-tool diff was Own.NET vs CodeQL only. Add a minimal net8.0 console fixture (corpus/fixtures/systemevents-console/) with two leak classes: * SystemEvents.DisplaySettingsChanged subscribed, never -= (subscription/lifetime) * a new FileStream local never disposed (Dispose/RAII — the control) and teach the oracle a `local:<in-repo path>` target (copied into target/ instead of cloned) so the fixture builds and Infer# actually runs. Expected 2x2: the FileStream leak agrees across all three; the SystemEvents leak is Own.NET-only — the final nail in the differentiation (and a check of whether Infer# catches the subscription class). Point the sentinel at the fixture to run it.
… leak The Linux-buildable fixture (corpus/fixtures/systemevents-console/) got all three tools running through the oracle. Clean 2x2: the FileStream Dispose leak (Program.cs:41) is Agree across Own.NET + CodeQL + Infer# (the control — both oracles run and catch the RAII class), while the SystemEvents subscription leak (Program.cs:20) is Own.NET only — Infer# misses it too. Both mature oracles cover Dispose/RAII and neither has the subscription-leak class. Record the result in the write-up and ROADMAP.
…r-loop recall slice) Digging into the oracle's "Oracle only 33" on ScreenToGif (CodeQL Dispose-class findings Own.NET misses): it's a method-coverage gap, NOT type recognition. The --flow-locals detector bails on any method with an unmodelled construct, and the undisposed locals sit in those methods — the tell is the StringReader/XmlReader cases (a recognised disposable type) being missed only because their method had one. `for (init; cond; incr) body` has the same 0+-iteration shape as while/foreach, so lower its body to a `while` flow op (init/cond/incr opaque; the tracked locals are the body's — a resource in the for-INITIALIZER is not a method-body local, so never a tracked candidate -> sound, no new FPs). FlowLocalsSample gains a ForLeak case (per-iteration leak -> flagged) and HasLoop's `looped` stays silent (now balanced rather than skipped); the --stats coverage invariant still holds. `do`/try/switch remain unmodelled — `try` (the dispose-not-called-on-throw class) is the next step.
… are unbalanced The multi-line parser fix (85cdded) absorbed ANY non-matching line after a finding, so a stray line following a COMPLETE finding (e.g. build chatter leaking to stdout) was folded into its message instead of counted — breaking mine_report's selftest ("expected 1 unparsed, got 0"), the lint-job failure on the last two pushes. Track the running bracket balance of the finding's message: a real multi-line lambda body leaves it unbalanced, a complete finding is balanced. Absorb continuation lines only while unbalanced; once balanced, a further non-matching line is stray drift again. Both selftests green (mine_report 8/8, oracle_compare 19/19); the multi-line ScreenToGif findings still parse and a trailing stray line is still counted.
|
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 (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR completes the "WPF leak spike" milestone by expanding the OwnSharp extractor's self-owned subscription exemption to cover ChangesExtractor Logic and Sample Validation
Real-World Corpus Cases and Oracle Fixture
Mining and Oracle Automation
Sequence DiagramsequenceDiagram
Developer->>mine-on-push.yml: Push to corpus/mine-target.txt
mine-on-push.yml->>GitHub: Checkout repo
mine-on-push.yml->>WPF Ref: Materialize refs on Linux
mine-on-push.yml->>mine.sh: Run with --ref and --paths
mine.sh->>OwnSharp.Extractor: Extract leak patterns
OwnSharp.Extractor->>OwnSharp.Extractor: Apply selfOwned expansion<br/>and loop lowering
mine.sh->>mine_report.py: Parse own-check findings
mine_report.py->>mine_report.py: Handle multi-line findings<br/>with bracket balance
mine-on-push.yml->>GitHub: Upload report.md/findings.txt
Developer->>oracle.yml: Push to corpus/oracle-target.txt
oracle.yml->>Resolve Target: Unify dispatch/push inputs
oracle.yml->>Local Fixture: Clone systemevents-console
oracle.yml->>WPF Ref: Materialize refs
oracle.yml->>Own.NET: Run own-check
oracle.yml->>CodeQL: Run CodeQL
oracle.yml->>Infer#: Run Infer# (Linux)
oracle_compare.py->>oracle_compare.py: Parse multi-line findings
oracle.yml->>GitHub: Publish cross-tool diff
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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: d3d25c2e04
ℹ️ 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".
Comment on lines
+536
to
+540
| foreach (var arg in cls.DescendantNodes().OfType<ArgumentSyntax>()) | ||
| if ((arg.RefKindKeyword.IsKind(SyntaxKind.RefKeyword) | ||
| || arg.RefKindKeyword.IsKind(SyntaxKind.OutKeyword)) | ||
| && FieldName(arg.Expression) is { } rf) | ||
| selfOwned.Add(rf); |
There was a problem hiding this comment.
When a class passes an injected or otherwise long-lived publisher field by ref/out (for example TryResolve(out _bus) or Interlocked.Exchange(ref _bus, ...)) and then subscribes to _bus.Event, this unconditionally adds the field to selfOwned, so IsSelfOwnedSource suppresses the subscription entirely. Ref/out only proves the callee can assign the field, not that it constructs an object owned by this class, so these real subscriptions become false negatives instead of warnings/errors.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Good catch — fixed in afe8676. The ref/out → self-owned path now requires the receiving method to be declared in this same class (the adorner's own BuildCorner(ref _thumb, …)), so an external container.TryResolve(out _bus) / Interlocked.Exchange(ref _bus, …) no longer suppresses the subscription — it stays a warning, exactly as you described. I also gated both the ref/out and template-part paths on IFieldSymbol so a local/parameter can't slip in.
Locked it with a negative-test sample (frontend/roslyn/samples/ExternalRefSubscription.cs): a field populated by an external class's ref method, then subscribed, must still report OWN001 — CI now asserts that, while SelfOwnedControlParts (its own same-class BuildCorner) stays silent.
Generated by Claude Code
![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 @.github/workflows/mine-on-push.yml:
- Around line 49-55: The find command on line 49 that searches for the WPF
ref-pack directory will cause the workflow step to abort if the directory
doesn't exist, because the script runs with pipefail enabled and a non-zero exit
code from find will fail the entire command substitution. To fix this, add ||
true to the end of the command substitution for the variable d assignment so
that if find returns no results, the assignment will still succeed with an empty
string, allowing the fallback logic in the if-else block to handle the case
gracefully and continue the mining step without WPF refs instead of aborting.
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 536-545: The code currently adds identifiers to the selfOwned
collection from both ref/out arguments and template-part assignments without
verifying they are actually fields, which can incorrectly include locals and
parameters. You need to modify both foreach loops: in the first loop handling
ArgumentSyntax (where FieldName(arg.Expression) is checked), add a validation
step to ensure the resolved field from FieldName() is actually an IFieldSymbol
before adding to selfOwned.Add(). Similarly, in the second foreach loop handling
assignments (where FieldName(a.Left) is checked), add the same IFieldSymbol
validation before calling selfOwned.Add(tf). This ensures only actual field
declarations are marked as self-owned, not local variables or parameters.
🪄 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: ee61cfa6-8cae-4477-8267-c06f211b9953
📒 Files selected for processing (26)
.github/workflows/ci.yml.github/workflows/mine-on-push.yml.github/workflows/mine.yml.github/workflows/oracle.ymlcorpus/fixtures/systemevents-console/Program.cscorpus/fixtures/systemevents-console/README.mdcorpus/fixtures/systemevents-console/SystemEventsLeak.csprojcorpus/mine-target.txtcorpus/oracle-target.txtcorpus/real-world/screentogif-loaded-subscription/after.cscorpus/real-world/screentogif-loaded-subscription/before.cscorpus/real-world/screentogif-loaded-subscription/case.owncorpus/real-world/screentogif-loaded-subscription/expected-diagnostics.txtcorpus/real-world/screentogif-loaded-subscription/notes.mdcorpus/real-world/screentogif-systemevents-leak/after.cscorpus/real-world/screentogif-systemevents-leak/before.cscorpus/real-world/screentogif-systemevents-leak/case.owncorpus/real-world/screentogif-systemevents-leak/expected-diagnostics.txtcorpus/real-world/screentogif-systemevents-leak/notes.mddocs/ROADMAP.mddocs/notes/real-world-mining.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FlowLocalsSample.csfrontend/roslyn/samples/SelfOwnedControlParts.csscripts/mine_report.pyscripts/oracle_compare.py
…ld-only (PR #29 review) Two valid review findings on the self-owned subscription exemption: * Codex (P2): treating ANY ref/out-passed field as self-owned suppresses real subscription leaks — an external `container.TryResolve(out _bus)` or `Interlocked.Exchange(ref _bus, ...)` only proves the callee can assign the field, not that this class owns it. Restrict the ref/out path to callees declared IN THIS CLASS (the adorner's own BuildCorner); external resolution/exchange stays a leak candidate (warning), not a false negative. * CodeRabbit: both ref/out and template-part paths added identifiers by name, so a ref/out local/param or a template-part assigned to a local could be wrongly marked self-owned. Require an IFieldSymbol on both. New sample ExternalRefSubscription.cs locks the Codex case (external-ref field + sub -> must still report OWN001; CI asserts it); SelfOwnedControlParts (its own same-class BuildCorner) stays silent. Also (CodeRabbit): guard the WPF ref-pack `find` probe with `|| true` in mine-on-push.yml and oracle.yml — under `bash -eo pipefail` a missing ref-pack dir made `find` non-zero and aborted the step before the fallback. Docs note the narrowing.
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.
Summary by CodeRabbit
Release Notes
for), plus improved oracle/miner workflow behavior and report parsing for richer multi-line outputs.