From de95f16b697d80c424fba4e5b3a03de3f669f1aa Mon Sep 17 00:00:00 2001 From: Wolfvin Date: Thu, 13 Aug 2026 09:30:45 +0700 Subject: [PATCH] fix(dead-code,smell): 2 false-positive classes found dogfooding on KAW81 API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while auditing a real ~29K-line TS/Express codebase (Coretax-Auto- Downloader/vps-deploy-kaw81/api) with `codelens audit`. Both verified via minimal repro + stash/pop A-B test (bug present pre-fix, gone post-fix), not just inferred from reading the detector code. 1. unused_vars (deadcode_engine.py, _detect_unused_variables): this detector only counts occurrences WITHIN THE SAME FILE. An `export const X = ...` is by definition meant to be used from OTHER files, so it always found exactly 1 occurrence (the declaration) and false-flagged it. Real case: 7/7 Express rate-limiters (`export const orderCreateRateLimiter = rateLimit(...)`) used as `app.post(path, orderCreateRateLimiter)` in a different file — passed by reference, never re-mentioned in their own file. Fix: skip any declaration immediately preceded by `export` — cross-file usage is `unused_exports`' job (it walks the import graph correctly), this same-file heuristic must defer to it instead of duplicating a weaker version of the same check. 2. magic_values (smell_engine.py, _detect_magic_values): the line-skip logic only excluded single-line `//` comments. JSDoc block comments (`/** ... */`) were never tracked — continuation lines start with `*`, not `//`, so every number in doc-comment prose was scanned as if it were live code. Real case: routes/public/orders/create.ts flagged 17 "magic numbers" that were 100% GitHub issue references (`#775`, `#1194`) and range docs (`[-90, 90]`) inside JSDoc. Fix: track /* ... */ block-comment state the same way in_docstring is already tracked for Python's """/'''. Both fixes are pure line-skip additions to existing detectors — no category removed, no threshold changed, existing true positives untouched (test_unused_variable_detection / test_python_unused_variable still pass unmodified). New regression tests added per-engine (test_deadcode_engine.py, test_smell_engine.py), matching existing test file structure. Full suite: 19 failures, ALL pre-existing (Windows path separator / LSP-URI env issues, listed in CONTEXT.md "Sudah kelar" baseline) — zero new failures, zero deadcode_engine/smell_engine failures. Not fixed here — filed as issue instead (needs deeper parser investigation, didn't want to guess): registry_dead false-positive on same-file function calls nested inside asyncHandler-wrapped Express route callbacks. SQL evidence: graph_edges rows for the affected file have source_id using a raw line number (`file.ts:30`) instead of the established `:0:` synthetic-caller convention, target_id always NULL, 5 duplicate rows, line=0 — strong signal of a parser bug in how module-level/nested-callback calls get attributed, distinct from the already-fixed #220 same-file-usage exemption path (which only covers non-call references like const/static usage, not actual function calls). --- scripts/deadcode_engine.py | 17 +++++++++++++++++ scripts/smell_engine.py | 21 +++++++++++++++++++++ tests/test_deadcode_engine.py | 23 +++++++++++++++++++++++ tests/test_smell_engine.py | 31 +++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+) diff --git a/scripts/deadcode_engine.py b/scripts/deadcode_engine.py index 4dcf2c2c..31ffb5f4 100755 --- a/scripts/deadcode_engine.py +++ b/scripts/deadcode_engine.py @@ -643,6 +643,23 @@ def _detect_unused_variables(content: str, ext: str, rel_path: str) -> List[Dict if re.match(r'^\d[\d_]*$', var_name): continue + # This detector only counts occurrences WITHIN THE SAME FILE + # (clean_content is this file's content). An `export const X = ...` + # is by definition meant to be used from OTHER files — this + # same-file heuristic has no way to see that usage and will always + # find exactly 1 occurrence (the declaration itself), false-flagging + # every exported value passed by reference elsewhere (e.g. Express + # middleware: `export const fooLimiter = rateLimit(...)` used as + # `app.post(path, fooLimiter)` in a different file — fooLimiter is + # never "called" or re-mentioned in its own file, so it looked + # unused here even though 3+ other files import and use it). + # Cross-file usage is `unused_exports`' job (it walks the import + # graph); this same-file scan must defer to it, not duplicate a + # weaker version of the same check. + _export_prefix = clean_content[max(0, start_pos - 20):start_pos] + if re.search(r'\bexport\s*$', _export_prefix): + continue + # Skip common patterns that are used indirectly skip_names = {'_', 'e', 'err', 'error', 'res', 'req', 'ctx', 'props', 'state', 'ref', 'config', 'module'} if var_name in skip_names or var_name.startswith('_'): diff --git a/scripts/smell_engine.py b/scripts/smell_engine.py index b441d412..13dfb9d4 100755 --- a/scripts/smell_engine.py +++ b/scripts/smell_engine.py @@ -1379,9 +1379,30 @@ def _detect_magic_values(content: str, ext: str, rel_path: str) -> List[Dict]: in_docstring = False + in_block_comment = False for i, line in enumerate(lines): stripped = line.strip() + # Track /* ... */ and /** ... */ block comments (JS/TS/Java/C/C++/ + # Rust/Go/CSS). Only single-line `//` was excluded before this fix — + # a JSDoc block's continuation lines never start with `//`, they + # start with `/*` (opening) or `*` (continuation), so every number + # written in doc-comment prose (issue refs like "#1091", coordinate + # ranges like "[-90, 90]", ISO-format examples like "8601") was + # scanned as if it were live code. Real false positive: KAW81 API's + # routes/public/orders/create.ts flagged 17 "magic numbers" that were + # 100% issue-number references inside JSDoc (`#775`, `#1194`, ...). + if in_block_comment: + if '*/' in stripped: + in_block_comment = False + continue + if stripped.startswith('/*'): + if '*/' not in stripped: + in_block_comment = True + continue + if stripped.startswith('*'): # JSDoc continuation line: " * text" + continue + # Track docstring boundaries if '"""' in stripped or "'''" in stripped: count = stripped.count('"""') + stripped.count("'''") diff --git a/tests/test_deadcode_engine.py b/tests/test_deadcode_engine.py index f33a9979..c321ba03 100644 --- a/tests/test_deadcode_engine.py +++ b/tests/test_deadcode_engine.py @@ -64,6 +64,29 @@ def test_unused_variable_detection(self): finally: shutil.rmtree(ws, ignore_errors=True) + def test_exported_var_used_only_in_other_file_not_flagged(self): + """An `export const X = ...` that is never re-mentioned in its OWN + file must NOT be flagged unused_vars, even though this detector only + scans same-file occurrences. Real-world false positive: Express + middleware exported and passed by reference in a different file + (`app.post(path, fooLimiter)`) — fooLimiter is never called or + re-mentioned in the file that declares it, so the same-file count + was always 1 (the declaration itself). Cross-file usage is + `unused_exports`' job, not this detector's — an exported symbol + must always be exempted here regardless of same-file usage count.""" + code = """ +import rateLimit from 'express-rate-limit'; +export const fooLimiter = rateLimit({ windowMs: 60000, max: 10 }); +""" + ws = self._create_workspace(code, "ratelimit.ts") + try: + result = detect_dead_code(ws) + assert result["status"] == "ok" + unused_names = [v["variable"] for v in result["results"].get("unused_vars", [])] + assert "fooLimiter" not in unused_names + finally: + shutil.rmtree(ws, ignore_errors=True) + def test_return_structure(self): """Verify the complete return structure of detect_dead_code.""" code = "function test() { return true; }" diff --git a/tests/test_smell_engine.py b/tests/test_smell_engine.py index c65bf8e5..3f17a664 100644 --- a/tests/test_smell_engine.py +++ b/tests/test_smell_engine.py @@ -49,6 +49,37 @@ def test_many_parameters(self): finally: shutil.rmtree(ws, ignore_errors=True) + def test_magic_values_ignores_numbers_in_block_comments(self): + """Numbers inside /* */ and /** */ block comments must NOT be + flagged as magic numbers. Before this fix, only single-line `//` + comments were excluded — JSDoc continuation lines (starting with + `*`, not `//`) were scanned as live code. Real false positive: + KAW81 API's routes/public/orders/create.ts flagged 17 "magic + numbers" that were 100% GitHub issue references (`#775`, `#1194`) + and coordinate-range docs (`[-90, 90]`) inside JSDoc blocks.""" + code = """interface Payload { + /** + * #1091: Voucher applied to this order. Optional — null/undefined = + * no voucher (backward compat). + */ + voucherId?: string | null; + /** + * #775: Delivery latitude — must be a finite number in range [-90, 90]. + */ + deliveryLat?: number; +} +""" + ws = self._create_workspace(code, "payload.ts") + try: + result = detect_smells(ws, categories=["magic_values"]) + magic = result["by_category"].get("magic_values", []) + flagged_values = [m["value"] for m in magic] + assert 1091 not in flagged_values, f"False positive: {magic}" + assert 775 not in flagged_values, f"False positive: {magic}" + assert 90 not in flagged_values, f"False positive: {magic}" + finally: + shutil.rmtree(ws, ignore_errors=True) + def test_clean_code_high_score(self): code = """ function add(a, b) { return a + b; }