Start runtime layer: leak-harness ingest bridge + C# harness skeleton (Plan.md §4)#102
Conversation
…lan.md §4) The runtime layer confirms static findings by observing the running app, and its findings flow through the SAME normalize -> score -> report pipeline as the static tiers — so a runtime-confirmed leak in the same file as a static finding clusters with it into one high-confidence result (the static->runtime confirmation in Plan.md §3.5). Testable core (pure Python, Linux-CI-gated): - audit/runtime/ingest.py converts the leak-harness JSON result into SARIF; only findings whose deterministic growth assertion tripped (leaked:true) become results, type-based findings stay file-level (reusing the #101 region fix). Its --selftest proves runtime rules categorize correctly AND that a static OWN014 + a runtime leak in the same file form one high-confidence {own-check, leak-harness} cluster. - categories.yml gains RUNTIME-* rule mappings for categories 2/3/4/11 (4 and 11 were NO-TOOL for static — the runtime layer is now their only tool) plus their severity baselines. - CI's audit-selftests job runs the ingest selftest. Windows / build-required skeleton (NOT CI-gated, per the build-required tier): - audit/runtime/LeakHarness/ — net472 C# harness (FlaUI + procdump + ClrMD): the deterministic GC+snapshot loop, growth assertion, and JSON result. Program.cs / Scenario.cs / HeapCounter.cs / LeakHarness.csproj. - audit/runtime/scenarios/open-close-declaration.yml — one declarative scenario with its schema documented inline. - audit/runtime/README.md documents the stack, flow, and how it plugs into the unified report. All five audit selftests green (normalize/score/report/run_static/ingest), ruff clean tree-wide. 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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds a runtime leak-harness pipeline for Own.NET, including YAML scenario loading, heap-dump counting, SARIF conversion, taxonomy mappings, CI selftests, and README updates describing the runtime layer and its status. ChangesRuntime audit pipeline
Sequence Diagram(s)sequenceDiagram
participant CI
participant ingest.py
participant normalize_results
participant parse_sarif
participant score
participant run_static.py
CI->>ingest.py: --selftest
ingest.py->>normalize_results: normalize runtime + static SARIF
ingest.py->>parse_sarif: parse combined results
ingest.py->>score: score clusters
CI->>run_static.py: include leak-harness.sarif
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 4af07f6969
ℹ️ 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".
…SARIF into run_static Two Codex P2 findings on the runtime layer: - LeakHarness Program.cs: a UI step whose control is missing (typo / not loaded) or whose action is unknown now THROWS a ScenarioException instead of being silently skipped by a null-propagating chain. A skipped step would baseline and re-snapshot the wrong screen and emit a clean result — a broken scenario masquerading as "no leak". Main maps ScenarioException to exit code 2 (distinct from clean=0 and leak-found=1) and writes no result file. - run_static.run() now folds in the runtime leak-harness SARIF: it picks up <out>/leak-harness.sarif (produced by ingest.py) alongside the static SARIFs, so a runtime-confirmed leak clusters with its static OWN014/OWN001 -> high confidence (§3.5) through the ORCHESTRATOR, matching the documented path — not only the lower-level aggregate(). Extended the run_static selftest to drop a leak-harness SARIF in the same file as a static finding and assert the leak-harness tier is picked up and a high-confidence cluster forms. run_static selftest 15/15, ingest 10/10, 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: 4
🧹 Nitpick comments (1)
audit/runtime/LeakHarness/LeakHarness.csproj (1)
16-16: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
<Platforms>may not force the build architecture you intend.
<Platforms>only declares the available platforms; it does not setPlatformTarget. Without<PlatformTarget>x64</PlatformTarget>(or building with-p:Platform=x64), the default net472 Exe build resolves to AnyCPU, which can run as 32-bit underPrefer32Bit. Since ClrMD reads a procdump full dump and FlaUI/heap inspection of the legacy target depend on predictable bitness, pin the target explicitly.♻️ Proposed change
<Platforms>x64</Platforms> + <PlatformTarget>x64</PlatformTarget>🤖 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 `@audit/runtime/LeakHarness/LeakHarness.csproj` at line 16, The LeakHarness project currently only declares available platforms via Platforms, which does not guarantee the build will run as x64. Update the LeakHarness.csproj configuration to explicitly pin the target architecture with PlatformTarget in the project settings so the net472 Exe is built and run as x64 instead of defaulting to AnyCPU; use the existing project property block around Platforms to locate the change.
🤖 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/LeakHarness/HeapCounter.cs`:
- Around line 65-77: The procdump invocation in HeapCounter’s dump-creation flow
can deadlock because both redirected streams are left unread before calling
p.WaitForExit(). Update this logic to drain StandardOutput and StandardError
while the process runs (or read them asynchronously), add a bounded wait/timeout
instead of an unbounded WaitForExit(), and then verify the process exit code
before relying on File.Exists(dumpPath). Use the existing _procdump,
ProcessStartInfo, and p variables to keep the fix localized.
- Around line 44-45: Guard against an empty ClrVersions collection in
HeapCounter when creating the runtime from the dump. In the HeapCounter logic
that loads the dump with DataTarget.LoadDump and accesses
dataTarget.ClrVersions[0], switch to a safe lookup such as FirstOrDefault, and
if no CLR version is present, throw a clear InvalidOperationException before
calling CreateRuntime().
In `@audit/runtime/LeakHarness/Program.cs`:
- Around line 75-79: The teardown in the finally block can throw and mask the
original failure when the target app is already exited or crashed. Update the
LeakHarness cleanup around app.Close() and app.Dispose() to be defensive by
handling teardown errors separately from the main try/finally flow, and use the
app symbol consistently so any cleanup exception is swallowed or logged without
replacing the original exception.
In `@audit/runtime/LeakHarness/Scenario.cs`:
- Around line 45-50: The Suspect model is silently defaulting Rule to
RUNTIME-LEAK-SUBSCRIPTION, which can misclassify malformed scenarios and emit
the wrong finding taxonomy; update the Suspect class in Scenario.cs so Rule is
required and has no fallback default, and make the scenario loading/validation
path fail fast when a Rule is missing, using the Suspect type and the Program.cs
finding emission flow as the key touchpoints.
---
Nitpick comments:
In `@audit/runtime/LeakHarness/LeakHarness.csproj`:
- Line 16: The LeakHarness project currently only declares available platforms
via Platforms, which does not guarantee the build will run as x64. Update the
LeakHarness.csproj configuration to explicitly pin the target architecture with
PlatformTarget in the project settings so the net472 Exe is built and run as x64
instead of defaulting to AnyCPU; use the existing project property block around
Platforms to locate the change.
🪄 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: d6917ae8-1e43-4506-8a78-f16a0d0c7203
📒 Files selected for processing (10)
.github/workflows/ci.ymlaudit/README.mdaudit/runtime/LeakHarness/HeapCounter.csaudit/runtime/LeakHarness/LeakHarness.csprojaudit/runtime/LeakHarness/Program.csaudit/runtime/LeakHarness/Scenario.csaudit/runtime/README.mdaudit/runtime/ingest.pyaudit/runtime/scenarios/open-close-declaration.ymlaudit/static/taxonomy/categories.yml
…rdown, required Rule, x64 CodeRabbit review on PR #102 (all on the Windows/build-required harness): - HeapCounter.RunProcdump: drain StandardOutput/StandardError asynchronously before waiting (an unread full pipe + WaitForExit() is a classic deadlock), bound the wait to 120s (kill on timeout), and check the exit code — not just whether a dump file appeared. - HeapCounter: guard against an empty ClrVersions (a non-managed dump) with FirstOrDefault() + a clear InvalidOperationException instead of indexing [0]. - Program.cs: wrap app.Close() in the finally so a teardown throw (target already exited/crashed) can't mask the real exception. - Scenario.cs: Rule no longer defaults to RUNTIME-LEAK-SUBSCRIPTION — a missing YAML `rule` would silently misclassify the finding. It defaults to "" and Scenario.Load() fails fast when a suspect is missing `rule` or `type`. - LeakHarness.csproj: add <PlatformTarget>x64</PlatformTarget> so the net472 Exe builds/runs as x64 (predictable bitness for ClrMD/FlaUI), not AnyCPU. C# verified by reading (braces/parens balance + tokens); it is Windows/build- required and not compiled in CI. Python ingest 10/10, run_static 15/15, ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QDPpNT9Uh8RoTrKPvgcoRE
A heap full of identical immutable values (the same "Country"/unit/currency string held by thousands of separate instances) is wasted memory that interning / a flyweight / reference-by-id would collapse. This adds that detector to the runtime layer, flowing through the same normalize -> score -> report pipeline. Testable core (pure Python, Linux-CI-gated): - ingest.py refactored around a shared _runtime_sarif() core, with a new duplicate_detector_to_sarif() adapter: type/value-based findings stay file-level, SARIF level warning (a P2 memory/perf finding), and a below-threshold finding (report:false) is dropped. --selftest extended to prove the dup path maps to category 11 and carries wastedBytes; ingest 15/15. - run_static.run() folds <out>/duplicate-detector.sarif into aggregation as a runtime tier (alongside leak-harness.sarif), so a cat-11 finding reaches the report through the orchestrator; run_static 15/15. - RUNTIME-DUP-IMMUTABLE was already mapped to category 11 (P2) in #102, so no taxonomy change is needed. Windows / build-required skeleton (NOT CI-gated): - audit/runtime/DuplicateDetector/ (net472 C#, ClrMD): walks a full dump, groups live strings by value, computes per-group wasted bytes ((count-1)*bytesPerInstance), and emits JSON findings above --min-wasted-bytes. Reuses the hardened procdump pattern (drain pipes, bounded wait, exit-code check) and the ClrVersions guard. Strings first (the highest-value case); arbitrary immutable types deferred. All audit selftests green (normalize/score/report/run_static/ingest), ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QDPpNT9Uh8RoTrKPvgcoRE
Starts the runtime layer (Plan.md §4). The runtime layer confirms static
findings by observing the running app; its findings flow through the same
normalize → score → reportpipeline as the static tiers, so a runtime-confirmedleak in the same file as a static finding clusters with it into one
high-confidence result — the static→runtime confirmation in Plan.md §3.5.
Testable core — pure Python, Linux-CI-gated
audit/runtime/ingest.pyconverts the leak-harness JSON result into SARIF.Only findings whose deterministic growth assertion tripped (
leaked: true)become results; type-based findings (e.g. duplicate-immutable) stay file-level,
reusing the Add merged-SARIF and HTML renderers — complete Phase 1 audit outputs #101 region fix. Its
--selftestproves the runtime rules categorizecorrectly and that a static
OWN014plus a runtime leak in the same file formone high-confidence
{own-check, leak-harness}cluster.categories.ymlgainsRUNTIME-*rule mappings for categories 2/3/4/11 — and4 (
DependencyPropertyDescriptor.AddValueChanged) and 11 (duplicated-immutabledata, the project's "gold") were
NO-TOOLfor static analysis, so the runtimelayer is now their only tool — plus their severity baselines.
audit-selftestsjob runs the ingest selftest.Windows / build-required skeleton — NOT CI-gated
Per the build-required tier (runs on the local Windows machine, never in CI):
audit/runtime/LeakHarness/— net472 C# harness (FlaUI + procdump + ClrMD):the deterministic GC+snapshot loop, growth assertion, and JSON result
(
Program.cs/Scenario.cs/HeapCounter.cs/LeakHarness.csproj).audit/runtime/scenarios/open-close-declaration.yml— one declarativeleak-harness scenario with its schema documented inline.
audit/runtime/README.md— the stack, the deterministic loop, and how itplugs into the unified report.
Tests
End-to-end (through the real orchestrator): a static
OWN014(line 122) + a runtimeleak-harness finding (line 123) in the same file collapse into 1 high-confidence
finding — runtime confirms static.
🤖 Generated with Claude Code
https://claude.ai/code/session_01QDPpNT9Uh8RoTrKPvgcoRE
Generated by Claude Code
Summary by CodeRabbit