Skip to content

Commit de95f16

Browse files
committed
fix(dead-code,smell): 2 false-positive classes found dogfooding on KAW81 API
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 `<file>:0:<module>` 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).
1 parent ba4d0d2 commit de95f16

4 files changed

Lines changed: 92 additions & 0 deletions

File tree

scripts/deadcode_engine.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -643,6 +643,23 @@ def _detect_unused_variables(content: str, ext: str, rel_path: str) -> List[Dict
643643
if re.match(r'^\d[\d_]*$', var_name):
644644
continue
645645

646+
# This detector only counts occurrences WITHIN THE SAME FILE
647+
# (clean_content is this file's content). An `export const X = ...`
648+
# is by definition meant to be used from OTHER files — this
649+
# same-file heuristic has no way to see that usage and will always
650+
# find exactly 1 occurrence (the declaration itself), false-flagging
651+
# every exported value passed by reference elsewhere (e.g. Express
652+
# middleware: `export const fooLimiter = rateLimit(...)` used as
653+
# `app.post(path, fooLimiter)` in a different file — fooLimiter is
654+
# never "called" or re-mentioned in its own file, so it looked
655+
# unused here even though 3+ other files import and use it).
656+
# Cross-file usage is `unused_exports`' job (it walks the import
657+
# graph); this same-file scan must defer to it, not duplicate a
658+
# weaker version of the same check.
659+
_export_prefix = clean_content[max(0, start_pos - 20):start_pos]
660+
if re.search(r'\bexport\s*$', _export_prefix):
661+
continue
662+
646663
# Skip common patterns that are used indirectly
647664
skip_names = {'_', 'e', 'err', 'error', 'res', 'req', 'ctx', 'props', 'state', 'ref', 'config', 'module'}
648665
if var_name in skip_names or var_name.startswith('_'):

scripts/smell_engine.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1379,9 +1379,30 @@ def _detect_magic_values(content: str, ext: str, rel_path: str) -> List[Dict]:
13791379

13801380
in_docstring = False
13811381

1382+
in_block_comment = False
13821383
for i, line in enumerate(lines):
13831384
stripped = line.strip()
13841385

1386+
# Track /* ... */ and /** ... */ block comments (JS/TS/Java/C/C++/
1387+
# Rust/Go/CSS). Only single-line `//` was excluded before this fix —
1388+
# a JSDoc block's continuation lines never start with `//`, they
1389+
# start with `/*` (opening) or `*` (continuation), so every number
1390+
# written in doc-comment prose (issue refs like "#1091", coordinate
1391+
# ranges like "[-90, 90]", ISO-format examples like "8601") was
1392+
# scanned as if it were live code. Real false positive: KAW81 API's
1393+
# routes/public/orders/create.ts flagged 17 "magic numbers" that were
1394+
# 100% issue-number references inside JSDoc (`#775`, `#1194`, ...).
1395+
if in_block_comment:
1396+
if '*/' in stripped:
1397+
in_block_comment = False
1398+
continue
1399+
if stripped.startswith('/*'):
1400+
if '*/' not in stripped:
1401+
in_block_comment = True
1402+
continue
1403+
if stripped.startswith('*'): # JSDoc continuation line: " * text"
1404+
continue
1405+
13851406
# Track docstring boundaries
13861407
if '"""' in stripped or "'''" in stripped:
13871408
count = stripped.count('"""') + stripped.count("'''")

tests/test_deadcode_engine.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,29 @@ def test_unused_variable_detection(self):
6464
finally:
6565
shutil.rmtree(ws, ignore_errors=True)
6666

67+
def test_exported_var_used_only_in_other_file_not_flagged(self):
68+
"""An `export const X = ...` that is never re-mentioned in its OWN
69+
file must NOT be flagged unused_vars, even though this detector only
70+
scans same-file occurrences. Real-world false positive: Express
71+
middleware exported and passed by reference in a different file
72+
(`app.post(path, fooLimiter)`) — fooLimiter is never called or
73+
re-mentioned in the file that declares it, so the same-file count
74+
was always 1 (the declaration itself). Cross-file usage is
75+
`unused_exports`' job, not this detector's — an exported symbol
76+
must always be exempted here regardless of same-file usage count."""
77+
code = """
78+
import rateLimit from 'express-rate-limit';
79+
export const fooLimiter = rateLimit({ windowMs: 60000, max: 10 });
80+
"""
81+
ws = self._create_workspace(code, "ratelimit.ts")
82+
try:
83+
result = detect_dead_code(ws)
84+
assert result["status"] == "ok"
85+
unused_names = [v["variable"] for v in result["results"].get("unused_vars", [])]
86+
assert "fooLimiter" not in unused_names
87+
finally:
88+
shutil.rmtree(ws, ignore_errors=True)
89+
6790
def test_return_structure(self):
6891
"""Verify the complete return structure of detect_dead_code."""
6992
code = "function test() { return true; }"

tests/test_smell_engine.py

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,37 @@ def test_many_parameters(self):
4949
finally:
5050
shutil.rmtree(ws, ignore_errors=True)
5151

52+
def test_magic_values_ignores_numbers_in_block_comments(self):
53+
"""Numbers inside /* */ and /** */ block comments must NOT be
54+
flagged as magic numbers. Before this fix, only single-line `//`
55+
comments were excluded — JSDoc continuation lines (starting with
56+
`*`, not `//`) were scanned as live code. Real false positive:
57+
KAW81 API's routes/public/orders/create.ts flagged 17 "magic
58+
numbers" that were 100% GitHub issue references (`#775`, `#1194`)
59+
and coordinate-range docs (`[-90, 90]`) inside JSDoc blocks."""
60+
code = """interface Payload {
61+
/**
62+
* #1091: Voucher applied to this order. Optional — null/undefined =
63+
* no voucher (backward compat).
64+
*/
65+
voucherId?: string | null;
66+
/**
67+
* #775: Delivery latitude — must be a finite number in range [-90, 90].
68+
*/
69+
deliveryLat?: number;
70+
}
71+
"""
72+
ws = self._create_workspace(code, "payload.ts")
73+
try:
74+
result = detect_smells(ws, categories=["magic_values"])
75+
magic = result["by_category"].get("magic_values", [])
76+
flagged_values = [m["value"] for m in magic]
77+
assert 1091 not in flagged_values, f"False positive: {magic}"
78+
assert 775 not in flagged_values, f"False positive: {magic}"
79+
assert 90 not in flagged_values, f"False positive: {magic}"
80+
finally:
81+
shutil.rmtree(ws, ignore_errors=True)
82+
5283
def test_clean_code_high_score(self):
5384
code = """
5485
function add(a, b) { return a + b; }

0 commit comments

Comments
 (0)