feat: implement findings quality improvements (#84, #85, #112) - #130
Merged
Conversation
📝 WalkthroughWalkthroughThis PR implements Wave 1 of the findings quality initiative: it adds structural risk scoring to all detectors, introduces a hybrid severity derivation model combining risk floors with confidence-weighted cost escalation, centralizes suggestion deduplication with source tracking, fixes guard detection to ignore comment text, and adds UI filtering/badging for confidence thresholds and cross-detector consensus. ChangesFindings Quality & Severity Derivation
Sequence DiagramsequenceDiagram
participant Detector as Detectors
participant Builder as Builders<br/>(local/remote)
participant Collapse as collapseSuggestions
participant UI as UI<br/>(Filter & Badge)
Detector->>Detector: compute score → riskScore
Detector->>Builder: emit LocalWasteFinding
Builder->>Builder: costImpactUsd = compute(baseline, frequency)
Builder->>Builder: severity = deriveSeverity(signals)
Builder->>Builder: sources = ["local-rule"]
Builder->>Collapse: merged suggestions
Collapse->>Collapse: dedupe by type+file+endpoint
Collapse->>Collapse: sources = union(sources)
Collapse->>Collapse: confidence = max(confidence)
Collapse->>Collapse: recompute severity
Collapse->>UI: collapsed suggestions
UI->>UI: filter by minConfidence
UI->>UI: badge: "detected by N"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webview/src/components/ResultsPage.tsx (1)
689-757:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing empty state for filtered results.
When all suggestions are filtered out by the confidence threshold, the UI renders blank content with no explanation. The empty state check on line 689 tests
suggestions.length, notvisibleSuggestions.length, so users who set a high confidence threshold might see an empty panel and assume the UI is broken.📋 Proposed fix to add filtered empty state
) : ( <> <div style={{ padding: "5px 12px", borderBottom: "1px solid var(--vscode-panel-border)", display: "flex", alignItems: "center", gap: "6px" }}> ... </div> + {visibleSuggestions.length === 0 ? ( + <div style={{ display: "flex", flexDirection: "column", alignItems: "center", justifyContent: "center", padding: "40px 16px", gap: "8px", color: "var(--vscode-descriptionForeground)" }}> + <span className="codicon codicon-filter" style={{ fontSize: "24px" }} /> + <span>No issues match the current filters</span> + <span style={{ fontSize: "10px", opacity: 0.6 }}>Try adjusting the confidence or type filter</span> + </div> + ) : ( {(() => { const paidIssues = visibleSuggestions.filter((s) => s.pricingClass === "paid"); ... })()} + )} </> )}🤖 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 `@webview/src/components/ResultsPage.tsx` around lines 689 - 757, The empty-state check uses suggestions.length instead of the filtered list, so change the top conditional in ResultsPage (the ternary that currently tests suggestions.length) to test visibleSuggestions.length and render a clear "No issues match filters" empty state when visibleSuggestions is 0; also update any UI counts shown in the type/select header (e.g., the "All ({suggestions.length})" option and presentTypes counts) to reflect visibleSuggestions where appropriate so the counts match the filtered view and the PricingSection(s) receive the same filtered suggestion arrays (paidIssues/freeIssues/unknownIssues).
🤖 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 `@src/test/local-waste-detector.test.ts`:
- Around line 119-133: The test titled "`#112`: bare 'cleanup'/'guard' word in a
comment does not suppress" currently only includes a comment with "cleanup", so
update the test input in the run block to also include a comment containing the
bare word "guard" (e.g., add "// guard this endpoint later" or replace the
single comment line with two comment lines "// cleanup..." and "// guard...") so
the detectLocalWasteFindingsInText("src/items.ts", text) assertion exercises
both comment tokens; no changes to detection logic needed—only modify the test
text in this test case.
---
Outside diff comments:
In `@webview/src/components/ResultsPage.tsx`:
- Around line 689-757: The empty-state check uses suggestions.length instead of
the filtered list, so change the top conditional in ResultsPage (the ternary
that currently tests suggestions.length) to test visibleSuggestions.length and
render a clear "No issues match filters" empty state when visibleSuggestions is
0; also update any UI counts shown in the type/select header (e.g., the "All
({suggestions.length})" option and presentTypes counts) to reflect
visibleSuggestions where appropriate so the counts match the filtered view and
the PricingSection(s) receive the same filtered suggestion arrays
(paidIssues/freeIssues/unknownIssues).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 842ba2e0-0465-4a6c-a640-c617c4f9b82d
📒 Files selected for processing (26)
docs/accuracy/findings.mddocs/superpowers/plans/2026-05-28-wave1-findings-quality.mddocs/superpowers/plans/PROGRESS.mdpackage.jsonsrc/analysis/types.tssrc/ast/waste/batch-detector.tssrc/ast/waste/cache-detector.tssrc/ast/waste/concurrency-detector.tssrc/intelligence/__tests__/builder.test.tssrc/intelligence/__tests__/clusters.test.tssrc/intelligence/__tests__/compression.test.tssrc/intelligence/__tests__/export.test.tssrc/intelligence/__tests__/scorer.test.tssrc/intelligence/builder.tssrc/intelligence/types.tssrc/scan-results.tssrc/scanner/local-waste-detector.tssrc/scanner/python-waste-detector.tssrc/simulator/engine.tssrc/test/chat-handler-merge.test.tssrc/test/local-waste-detector.test.tssrc/test/scan-results.test.tssrc/webview/chat-handler.tssrc/webview/scan-publishing-handler.tswebview/src/components/ResultsPage.tsxwebview/src/types.ts
Comment on lines
+119
to
+133
| run("#112: bare 'cleanup'/'guard' word in a comment does not suppress", () => { | ||
| const text = [ | ||
| "// cleanup this endpoint later", | ||
| "export async function loadItems(ids) {", | ||
| " const out = [];", | ||
| " for (const id of ids) {", | ||
| " out.push(await fetch(`https://api.example.com/items/${id}`));", | ||
| " }", | ||
| " return out;", | ||
| "}", | ||
| ].join("\n"); | ||
| const findings = detectLocalWasteFindingsInText("src/items.ts", text); | ||
| assert.ok( | ||
| findings.some((f) => f.type === "n_plus_one" || f.type === "cache"), | ||
| "comment-only 'cleanup' must not suppress findings; expected n_plus_one or cache finding" |
There was a problem hiding this comment.
Cover the 'guard' comment-token case explicitly.
Line 121 only exercises cleanup; it never places guard in comment text, so the test does not validate the full case described in its title.
Proposed test-input fix
- "// cleanup this endpoint later",
+ "// cleanup this endpoint later; add guard next",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| run("#112: bare 'cleanup'/'guard' word in a comment does not suppress", () => { | |
| const text = [ | |
| "// cleanup this endpoint later", | |
| "export async function loadItems(ids) {", | |
| " const out = [];", | |
| " for (const id of ids) {", | |
| " out.push(await fetch(`https://api.example.com/items/${id}`));", | |
| " }", | |
| " return out;", | |
| "}", | |
| ].join("\n"); | |
| const findings = detectLocalWasteFindingsInText("src/items.ts", text); | |
| assert.ok( | |
| findings.some((f) => f.type === "n_plus_one" || f.type === "cache"), | |
| "comment-only 'cleanup' must not suppress findings; expected n_plus_one or cache finding" | |
| run("`#112`: bare 'cleanup'/'guard' word in a comment does not suppress", () => { | |
| const text = [ | |
| "// cleanup this endpoint later; add guard next", | |
| "export async function loadItems(ids) {", | |
| " const out = [];", | |
| " for (const id of ids) {", | |
| " out.push(await fetch(`https://api.example.com/items/${id}`));", | |
| " }", | |
| " return out;", | |
| "}", | |
| ].join("\n"); | |
| const findings = detectLocalWasteFindingsInText("src/items.ts", text); | |
| assert.ok( | |
| findings.some((f) => f.type === "n_plus_one" || f.type === "cache"), | |
| "comment-only 'cleanup' must not suppress findings; expected n_plus_one or cache finding" |
🤖 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 `@src/test/local-waste-detector.test.ts` around lines 119 - 133, The test
titled "`#112`: bare 'cleanup'/'guard' word in a comment does not suppress"
currently only includes a comment with "cleanup", so update the test input in
the run block to also include a comment containing the bare word "guard" (e.g.,
add "// guard this endpoint later" or replace the single comment line with two
comment lines "// cleanup..." and "// guard...") so the
detectLocalWasteFindingsInText("src/items.ts", text) assertion exercises both
comment tokens; no changes to detection logic needed—only modify the test text
in this test case.
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.
Closes #84
Closes #85
Closes #112
Summary
Implements three coupled findings quality improvements in a single PR:
Key Changes
#112: Comment-Word Guard Leak Fix
stripComments()helper inlocal-waste-detector.tsto remove line/block comments before testing guard regexesCACHE_GUARD,BATCH_GUARD, etc.) now test against comment-stripped window text, preventing false suppressions from prose mentions like// TODO: batch these later//line-comment rule uses a(?<[:/])lookbehind sohttps://URLs aren't mangled#85: Severity Derivation + Cost Impact
FREQUENCY_CLASS_MULTIPLIERSfrom simulator as single source of truthriskScorefield toLocalWasteFindinginterface; emitted from all detectors (local, Python, AST-based)deriveSeverity()function: hybrid model combining structural floor (riskScore thresholds) with cost amplifier (confidence × costImpactUsd)computeCostImpact(): endpoint monthlyCost amplified by frequency-class multiplierscan-results.ts,scan-publishing-handler.ts,chat-handler.ts#84: Finding Deduplication + Source Tracking
sources?: string[]andcostImpactUsd?: number | nullfields toSuggestiontypecollapseSuggestions()function: merges findings by type + file + endpoint/line-bucketscan-publishing-handler.tspath)FindingNodefor graph/export parityImplementation Details
Math.max(structuralTier, costTier)where structural tier is based on riskScore (≥5→high, ≥3→medium, <3→low) and cost tier is based on confidence × costImpactUsd (≥100→high, ≥10→medium, <10→low). Never demotes — only escalates above the C1-calibrated structural floor.monthlyCostfromLOCAL_PRICING×FREQUENCY_CLASS_MULTIPLIERS;costImpactUsdis internal (drives severity + ordering) and never rendered to users.type | file | endpointId(or 5-line bucket when no endpoint); findings on same endpoint always collapse regardless of line distance.Tests
src/test/scan-results.test.ts: unit tests forderiveSeverity()(incl. exact threshold boundaries),computeCostImpact(),collapseSuggestions(),buildRemoteScanResultsaggressive-suggestion labelingsrc/test/chat-handler-merge.test.ts: AI finding merge + collapsesrc/test/local-waste-detector.test.ts: [Findings] Tighten CACHE_GUARD / BATCH_GUARD bare-word leak #112 regression cases (comment-word suppressions, https:// URL preservation, explicitGuard comment leak)node:assert/strictcompiled bytscand run vianpm run test:scannerGates
npm run buildclean (extension + webview)npm run test:scanner— FAIL count 0 (full suite)npm run benchmarkagainst the 7-fixture v1 corpus — Δ +0.00pp on all five metrics (detection P/R, provider attribution, finding P/R); every per-type finding precision held at 100%Pending manual verification
The two webview UI bits build cleanly and typecheck, but require interactive F5 verification:
Annotated as
[~]indocs/accuracy/findings.mdrather than[x].Tracked follow-ups (out of scope for this PR)
scope === "internal"guard divergence between the twobuildAggressiveSuggestionscopies (scan-results.tslacks the guardscan-publishing-handler.tshas)SourceBadgecomponent + duplicated badge inline-style inResultsPage.tsx— extract a shared secondary-badgestripCommentsedge cases: protocol-relative URLs//cdn...and#inside string literals (under-suppress only — never over-suppress; benchmark-clean)FindingNode.severitystill reflects raw structural severity, not the cost-amplified value (intentional per plan; flag if a follow-up wants ranking alignment with the sidebar)https://claude.ai/code/session_01DPQ8sonp1j85mKqDQefc8T
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests
[[Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/recost-dev/extension/pull/130?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)