From 25a32d3ce12262c2682b341c8268f33866899ee9 Mon Sep 17 00:00:00 2001 From: Wolfvin Date: Sun, 12 Jul 2026 19:41:26 +0700 Subject: [PATCH] feat(audit): dead-code findings auto-annotated with impact deletion_safety (closes #238) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `audit --check dead-code` reported `status: dead` from the registry with no signal about whether a finding is actually safe to delete — an agent had to manually chain a separate `context --check trace --direction up` call per finding to rule out entry points (exactly the caveat already documented in CONTEXT.md: "status: dead != aman dihapus"). `impact_engine.analyze_impact(name, action="delete")` already computes this exact signal (risk level from real dependents), it just wasn't wired into the dead-code command output. Each finding (capped at --verify-impact-limit, default 20, across all categories combined) now gets a `deletion_safety` field: safe / caution / entry_point_likely / unknown (on a per-item analyze_impact failure — never crashes the whole report). Opt out entirely with --no-verify-impact. Verified on a real workspace: AdGate.tsx's default export (flagged unused_exports — genuinely never imported directly, confirmed earlier this session) is correctly tagged entry_point_likely because analyze_impact finds 6 real direct dependents through other paths, exactly the false-confidence trap this issue set out to close. --- scripts/commands/audit.py | 9 +++ scripts/commands/dead_code.py | 50 +++++++++++++ tests/test_dead_code_command.py | 126 ++++++++++++++++++++++++++++++++ 3 files changed, 185 insertions(+) create mode 100644 tests/test_dead_code_command.py diff --git a/scripts/commands/audit.py b/scripts/commands/audit.py index 6f498d7d..6b904a31 100644 --- a/scripts/commands/audit.py +++ b/scripts/commands/audit.py @@ -110,6 +110,13 @@ def add_args(parser): help="perf-hint: single category filter") parser.add_argument("--no-confirm-hash", action="store_true", default=False, help="staleness: skip content-hash confirmation") + parser.add_argument("--no-verify-impact", dest="verify_impact", + action="store_false", default=True, + help="dead-code: skip per-finding deletion_safety cross-check " + "against impact analysis (issue #238)") + parser.add_argument("--verify-impact-limit", type=int, default=None, + help="dead-code: max findings to cross-check with impact " + "analysis (default: 20)") def _parse_checks(check_arg: str) -> List[str]: @@ -138,6 +145,8 @@ def _build_namespace(base_args, check_name: str) -> argparse.Namespace: ns.categories = getattr(base_args, "categories", None) ns.max_files = getattr(base_args, "max_files", None) or 3000 ns.max_results = getattr(base_args, "max_results", None) or 100 + ns.verify_impact = getattr(base_args, "verify_impact", True) + ns.verify_impact_limit = getattr(base_args, "verify_impact_limit", None) or 20 elif check_name == "complexity": ns.name = getattr(base_args, "name", None) ns.file = getattr(base_args, "file", None) diff --git a/scripts/commands/dead_code.py b/scripts/commands/dead_code.py index 156bb632..6c1f0081 100644 --- a/scripts/commands/dead_code.py +++ b/scripts/commands/dead_code.py @@ -13,6 +13,15 @@ def add_args(parser): help="Max files to scan (default: 3000)") parser.add_argument("--max-results", type=int, default=100, help="Max results per category (default: 100)") + parser.add_argument("--no-verify-impact", dest="verify_impact", + action="store_false", default=True, + help="Skip per-finding deletion_safety cross-check against " + "impact analysis (issue #238). On by default; disable " + "on very large result sets if it's too slow.") + parser.add_argument("--verify-impact-limit", type=int, default=20, + help="Max number of findings to cross-check with impact " + "analysis per run (default: 20, highest-confidence " + "findings first)") def execute(args, workspace): @@ -82,6 +91,47 @@ def execute(args, workspace): if "stats" not in result: result["stats"] = {} result["stats"]["confidence_distribution"] = dist + + # Issue #238: per-finding deletion_safety cross-check. + # + # "status: dead" in the registry only means "no CALLS edge found" — it + # does NOT mean safe to delete (entry points like HTTP handlers, CLI + # subcommands, and exported APIs routinely have zero inbound edges but + # are still critical). Previously an agent had to manually chain + # `audit --check dead-code` -> `context --check trace --direction up` + # to verify this per finding; analyze_impact() already computes exactly + # this signal for the "delete" action, it just wasn't wired in here. + if getattr(args, "verify_impact", True): + try: + from impact_engine import analyze_impact + all_items = [] + for cat_items in result.get("results", {}).values(): + if isinstance(cat_items, list): + all_items.extend(cat_items) + limit = max(0, getattr(args, "verify_impact_limit", 20) or 0) + _risk_to_safety = { + "low": "safe", + "medium": "caution", + "high": "entry_point_likely", + "critical": "entry_point_likely", + } + for item in all_items[:limit]: + if not isinstance(item, dict): + continue + name = item.get("name") + if not name: + continue + try: + impact_result = analyze_impact( + name, workspace, action="delete", depth=3 + ) + risk = impact_result.get("risk", "low") + item["deletion_safety"] = _risk_to_safety.get(risk, "caution") + item["deletion_impact_stats"] = impact_result.get("stats") + except Exception: + item["deletion_safety"] = "unknown" + except ImportError: + pass return result # Issue #199: deprecated "dead-code" alias registration removed; this module is now an implementation module imported by the "audit" umbrella command. diff --git a/tests/test_dead_code_command.py b/tests/test_dead_code_command.py new file mode 100644 index 00000000..fdac6488 --- /dev/null +++ b/tests/test_dead_code_command.py @@ -0,0 +1,126 @@ +"""Tests for the dead-code command's deletion_safety cross-check (issue #238). + +`audit --check dead-code` previously reported `status: dead` from the +registry with no signal about whether a finding is actually safe to delete +— an agent had to manually chain a separate `context --check trace +--direction up` call per finding to check for entry points. This wires +`impact_engine.analyze_impact(action="delete")` (which already computes +exactly this signal) directly into the dead-code command output. +""" + +import argparse +import os +import sys +from unittest import mock + +import pytest + +SCRIPT_DIR = os.path.join( + os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "scripts" +) +if SCRIPT_DIR not in sys.path: + sys.path.insert(0, SCRIPT_DIR) + +from commands import dead_code # noqa: E402 + + +def _base_args(**overrides): + ns = argparse.Namespace( + workspace=".", + categories=None, + max_files=3000, + max_results=100, + verify_impact=True, + verify_impact_limit=20, + ) + for k, v in overrides.items(): + setattr(ns, k, v) + return ns + + +def _fake_dead_code_result(): + return { + "status": "ok", + "stats": {"total_dead_code": 2}, + "results": { + "unused_exports": [ + {"file": "AdGate.tsx", "line": 39, "name": "AdGate", "type": "default_export"}, + ], + "registry_dead": [ + {"file": "utils.ts", "line": 10, "name": "reallyUnusedHelper", "type": "function"}, + ], + }, + } + + +class TestDeletionSafetyCrossCheck: + def test_high_risk_symbol_flagged_entry_point_likely(self): + """A dead-code finding that analyze_impact reports as high-risk + (real dependents exist) must not be silently labeled safe.""" + with mock.patch("commands.dead_code.detect_dead_code", return_value=_fake_dead_code_result()), \ + mock.patch("hybrid_engine.create_hybrid_engine", side_effect=ImportError), \ + mock.patch( + "impact_engine.analyze_impact", + side_effect=lambda name, ws, **kw: { + "risk": "high" if name == "AdGate" else "low", + "stats": {"direct_dependents": 6 if name == "AdGate" else 0}, + }, + ): + result = dead_code.execute(_base_args(), ".") + + items_by_name = { + item["name"]: item + for cat in result["results"].values() + for item in cat + } + assert items_by_name["AdGate"]["deletion_safety"] == "entry_point_likely" + assert items_by_name["reallyUnusedHelper"]["deletion_safety"] == "safe" + + def test_no_verify_impact_flag_skips_cross_check(self): + """--no-verify-impact must not call analyze_impact at all.""" + with mock.patch("commands.dead_code.detect_dead_code", return_value=_fake_dead_code_result()), \ + mock.patch("hybrid_engine.create_hybrid_engine", side_effect=ImportError), \ + mock.patch("impact_engine.analyze_impact") as mock_analyze: + result = dead_code.execute(_base_args(verify_impact=False), ".") + + mock_analyze.assert_not_called() + for cat in result["results"].values(): + for item in cat: + assert "deletion_safety" not in item + + def test_verify_impact_limit_caps_calls(self): + """Only the first N findings (across all categories combined) are + cross-checked, to bound cost on large dead-code result sets.""" + many_findings = { + "status": "ok", + "stats": {"total_dead_code": 10}, + "results": { + "unused_exports": [ + {"file": f"f{i}.ts", "line": i, "name": f"fn{i}", "type": "function"} + for i in range(10) + ], + }, + } + with mock.patch("commands.dead_code.detect_dead_code", return_value=many_findings), \ + mock.patch("hybrid_engine.create_hybrid_engine", side_effect=ImportError), \ + mock.patch( + "impact_engine.analyze_impact", + return_value={"risk": "low", "stats": {}}, + ) as mock_analyze: + dead_code.execute(_base_args(verify_impact_limit=3), ".") + + assert mock_analyze.call_count == 3 + + def test_analyze_impact_failure_does_not_crash_command(self): + """If analyze_impact raises for a given symbol, the command must + still return successfully with an 'unknown' safety label — not + propagate the exception and break the whole dead-code report.""" + with mock.patch("commands.dead_code.detect_dead_code", return_value=_fake_dead_code_result()), \ + mock.patch("hybrid_engine.create_hybrid_engine", side_effect=ImportError), \ + mock.patch("impact_engine.analyze_impact", side_effect=RuntimeError("boom")): + result = dead_code.execute(_base_args(), ".") + + assert result["status"] == "ok" + for cat in result["results"].values(): + for item in cat: + assert item["deletion_safety"] == "unknown"