Add duplicate-immutable detector — the project's "gold" (Plan.md §2 cat. 11)#103
Conversation
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
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new DuplicateDetector runtime tool that analyzes duplicate strings from dumps, emits JSON findings, converts runtime outputs to SARIF, and teaches the static pipeline to load duplicate-detector results. ChangesDuplicate detector runtime pipeline
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: 1c3ec01601
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
audit/static/run_static.py (1)
144-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a selftest for the new duplicate-detector pickup.
These lines introduce a second runtime SARIF path, but
_selftest()still exercises onlyleak-harness.sarif. A typo in this filename/tool mapping would miss CI until a Windows run drops the new artifact.🤖 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/static/run_static.py` around lines 144 - 154, Add a selftest for the new duplicate-detector runtime pickup in _selftest() so both leak-harness.sarif and duplicate-detector.sarif are covered. Update the selftest setup to create/verify the duplicate-detector artifact and assert that run_static.py’s runtime import logic maps the duplicate-detector tool name to the correct SARIF filename, alongside the existing leak-harness check.
🤖 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/DuplicateDetector/Program.cs`:
- Around line 69-75: The grouping in DuplicateDetector.Program is using
obj.AsString(opts.MaxStringLength), which can collapse distinct long strings
into the same bucket. Update the logic around the val/groups.TryGetValue path to
group by the full string value instead of the truncated display value, and apply
truncation only when formatting the output fields like value/message.
In `@audit/runtime/ingest.py`:
- Around line 135-153: Make the CLI inputs in ingest.py mutually exclusive so
only one of --leak-harness or --duplicate-detector can be provided at a time.
Update the argparse setup near ap.add_argument / args parsing to place these
options in a mutually exclusive group (or otherwise validate and reject both
being set), and keep the existing ap.error path for invalid combinations so the
tool fails fast instead of silently choosing leak_harness over
duplicate_detector.
---
Nitpick comments:
In `@audit/static/run_static.py`:
- Around line 144-154: Add a selftest for the new duplicate-detector runtime
pickup in _selftest() so both leak-harness.sarif and duplicate-detector.sarif
are covered. Update the selftest setup to create/verify the duplicate-detector
artifact and assert that run_static.py’s runtime import logic maps the
duplicate-detector tool name to the correct SARIF filename, alongside the
existing leak-harness check.
🪄 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: 79c54600-0368-46e4-9cf1-33c5e1dd7e4b
📒 Files selected for processing (5)
audit/runtime/DuplicateDetector/DuplicateDetector.csprojaudit/runtime/DuplicateDetector/Program.csaudit/runtime/README.mdaudit/runtime/ingest.pyaudit/static/run_static.py
Address Codex + CodeRabbit findings on the duplicate-immutable detector: - DuplicateDetector/Program.cs: group strings by their FULL value, not the --max-string-length-truncated read. Distinct long strings sharing a prefix (JSON/XML blobs) no longer collapse into one bogus group with inflated count/wastedBytes. Overlong strings key on length + a SHA-256 of the full content so the dictionary never retains large blobs; a short sample is kept only for the value/message display. - ingest.py: the C# emits no source location for heap-wide duplicate groups, so every finding previously fell back to (type, line 0) and the scorer collapsed Country/Currency/... into a single cluster, corrupting totals/heatmap. Synthesize a UNIQUE per-value synthetic uri (heap://<type>/<n>-<slug>); all groups still roll up under one heap://<type> module. Harden the selftest with a realistic location-less fixture proving two distinct values stay two clusters. - ingest.py: make --leak-harness / --duplicate-detector mutually exclusive so a stray flag fails fast instead of silently converting the wrong file. - run_static.py: extend _selftest() to also drop a duplicate-detector.sarif and assert the runtime pickup folds it in, so a filename/tool-name typo fails on Linux CI, not only on a Windows run. Selftests: ingest 18/18, run_static 16/16, normalize/score/report unchanged; 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 / currencystring held by thousands of separate instances — is wasted memory that interning, a
flyweight, or a reference-by-id would collapse. Plan.md calls category 11 the
project's "gold". This adds that detector to the runtime layer; its findings flow
through the same
normalize → score → reportpipeline as everything else.Testable core — pure Python, Linux-CI-gated
ingest.pyrefactored around a shared_runtime_sarif()core, with a newduplicate_detector_to_sarif()adapter: type/value-based findings stayfile-level (reusing the Add merged-SARIF and HTML renderers — complete Phase 1 audit outputs #101 region handling), SARIF level
warning(a P2memory/perf finding, not a correctness error), and a below-threshold finding
(
report: false) is dropped.--selftestextended to prove the dup path maps tocategory 11 and carries
wastedBytes(ingest 15/15).run_static.run()folds<out>/duplicate-detector.sarifinto aggregation asa
runtimetier (alongsideleak-harness.sarif), so a cat-11 finding reaches thereport through the orchestrator (run_static 15/15).
RUNTIME-DUP-IMMUTABLEwas already mapped to category 11 (P2) in Start runtime layer: leak-harness ingest bridge + C# harness skeleton (Plan.md §4) #102 — notaxonomy change 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 from Start runtime layer: leak-harness ingest bridge + C# harness skeleton (Plan.md §4) #102 (drain pipes,bounded wait, exit-code check) and the
ClrVersionsguard. Strings first — thehighest-value case; arbitrary immutable types (field-by-field equality) are a noted
later refinement.
Tests
End-to-end: a duplicate-detector JSON → SARIF → report renders the finding as
[P2 · duplicate-immutable](category 11). C# verified by reading (it'sWindows/build-required and not compiled in CI).
🤖 Generated with Claude Code
https://claude.ai/code/session_01QDPpNT9Uh8RoTrKPvgcoRE
Generated by Claude Code
Summary by CodeRabbit