fix(dead-code,smell): 2 false-positive classes found dogfooding on real KAW81 API codebase - #325
Merged
Merged
Conversation
…W81 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).
|
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
1 participant
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.
Summary
Dogfooding
codelens auditon a real ~29K-line TS/Express codebase (Coretax-Auto-Downloader,vps-deploy-kaw81/api/) surfaced 3 false-positive classes. 2 are fixed here with verified root cause + regression tests. The 3rd (registry_dead, deeper parser issue) is filed as #324 instead of guessed at.1.
unused_vars(deadcode_engine.py) — same-file-only scan false-flags exported valuesThis detector only counts occurrences within the same file. An
export const X = ...is by definition meant for cross-file use, so it always found exactly 1 occurrence (the declaration) and flagged it. Real case: 7/7 Express rate-limiters (export const orderCreateRateLimiter = rateLimit(...)) used asapp.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 isunused_exports' job (correct import-graph walk), this same-file heuristic shouldn't duplicate a weaker version of it.2.
magic_values(smell_engine.py) — only excludes//, never/** */JSDoc block-comment continuation lines start with
*, not//, so they were never excluded. Every number in doc-comment prose got scanned as live code. Real case:routes/public/orders/create.tsflagged 17 "magic numbers" that were 100% GitHub issue references (#775,#1194) and range docs ([-90, 90]) inside JSDoc blocks.Fix: track
/* ... */block-comment state, same pattern already used for Python'sin_docstring.Verification methodology
Both bugs verified with an A/B test, not just code reading: minimal repro fixture →
git stash(bug reproduces) →git stash pop(bug gone). Repro fixtures matched the exact real-world nesting (Express router + asyncHandler wrapper for #1, JSDoc block structure for #2), not simplified guesses.deadcode_engine/smell_enginefailures.test_exported_var_used_only_in_other_file_not_flagged(test_deadcode_engine.py),test_magic_values_ignores_numbers_in_block_comments(test_smell_engine.py).test_unused_variable_detection,test_python_unused_variable).Not fixed here
registry_deadfalse-positive on same-file calls nested insideasyncHandler-wrapped route callbacks — filed as #324 with SQL evidence (malformed graph edges:source_iduses a raw line number instead of the<file>:0:<module>convention,target_idalways NULL, 5 duplicate rows). 2 repro attempts failed to isolate a minimal case, so a fix here would've been a guess rather than a verified change.Test plan
pytest tests/test_deadcode_engine.py -v— 25/25 pass (24 existing + 1 new)pytest tests/test_smell_engine.py -v— 10/10 pass (9 existing + 1 new)pytest tests/ --ignore=tests/test_integration.py --timeout=120— 19 failures, all cross-checked against pre-existing baseline, zero new🤖 Generated with Claude Code