feat: dead role sub-categories (Phase 4.1)#504
Conversation
Add Phase 4 (Resolution Accuracy) addressing the audit's top priorities: dead role sub-categories, receiver type tracking, interface/trait implementation edges, precision/recall benchmarks, package.json exports resolution, and monorepo workspace support. Reorder phases: TypeScript Migration (now Phase 5) before Native Acceleration (now Phase 6), per audit recommendation. Remove Web UI from Phase 12. Add kill list (5.9), confidence annotations (7.10), shell completion (7.11), and dual-engine architectural note. Backlog: mark ID 45 done, promote IDs 73/74/80 to roadmap, add Tier 1j with 10 audit-identified items (IDs 87-96).
The dual-engine strategy is settled — no need for a hedging note.
Document the rationale, trade-offs, and trajectory for maintaining both Rust native and JS/WASM engines. Covers performance requirements for large codebases, portability for VS Code extensions and CI, parity convergence plan, and alternatives considered.
Native binary v3.1.0 predates the type-map feature added in #501. When the native parser doesn't return typeMap, backfill it by re-extracting from WASM, convert to Map format, and fall back to the JS edge builder (which handles typeMap) instead of the native edge builder (which the old binary ignores). This restores engine parity for the build-parity test until a new native binary ships. Impact: 4 functions changed, 3 affected
- Add PROMOTED annotations to backlog IDs 87, 89, 90, 91, 93 that were simultaneously added to both backlog and roadmap without cross-references - Fix Tier 1j header to note exceptions (IDs 92, 94 are not zero-dep) - Add scope note to Phase 4.6 clarifying it covers resolution layer only, deferring full monorepo graph support to Phase 12.2
Impact: 1 functions changed, 0 affected
…ignment # Conflicts: # src/domain/parser.js Impact: 15 functions changed, 15 affected
Items 71, 73, 74, 80, 83, 87, 89, 90, 91, 93 had strikethrough only on titles but not on description and benefit columns, inconsistent with all other done/promoted items in the table.
…ad-unresolved The coarse "dead" role conflated genuinely different categories. Now the classifier distinguishes: dead-leaf (parameters/properties/constants), dead-entry (CLI commands, MCP tools, routes/handlers), dead-ffi (cross-language FFI like .rs/.c/.go), and dead-unresolved (genuinely unreferenced callables). --role dead still matches all sub-roles for backward compatibility. Risk weights tuned per sub-role. Implements Phase 4.1 of the roadmap. Impact: 10 functions changed, 5 affected
|
Claude finished @carlos-alm's task —— View job Reviewing PR #504: Dead role sub-categories implementation Review Checklist
Phase 1 Analysis: Root Cause ✅Problem Being Solved: The coarse "dead" role classification was too broad and not useful for triage. Dead code spans from truly unused functions (high priority) to unused parameters (normal/expected). This creates noise in audit workflows. Why This Approach: Sub-categorizing dead code allows users to focus on genuinely problematic dead code ( Risk Assessment: This is architectural improvement, not symptom hiding. The logic is sound - parameters can't have callers by definition, making them "dead" by fan-in but not problematic. Phase 2: Configuration Changes ✅No risky configuration changes detected:
Phase 3: Test Integrity ✅Strong test coverage:
Phase 4: Code Quality ✅Clean implementation:
Phase 5: Backlog Compliance ✅Zero-dep: ✓ — No new runtime dependencies added to package.json
Problem-fit: 4/5 — Directly improves agent accuracy by reducing false-positive dead code alerts. High-priority dead code ( Breaking: No — Tier: 1 (zero-dep ✓ + foundation-aligned ✓) Final Assessment |
Greptile SummaryThis PR sub-classifies the coarse
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Node: fanIn=0 AND not exported] --> B{testOnlyFanIn > 0?}
B -- Yes --> C[role = test-only]
B -- No --> D[classifyDeadSubRole]
D --> E{kind in LEAF_KINDS?}
E -- Yes\nparam/prop/const --> F[dead-leaf\nriskWeight=0.0]
E -- No --> G{file exists?}
G -- No --> H[dead-unresolved\nriskWeight=0.15]
G -- Yes --> I{extension in FFI_EXTENSIONS?}
I -- Yes\n.rs/.c/.go/... --> J[dead-ffi\nriskWeight=0.05]
I -- No --> K{path matches ENTRY_PATH_PATTERNS?}
K -- Yes\ncli/mcp/routes/... --> L[dead-entry\nriskWeight=0.3]
K -- No --> H
style F fill:#fffde7
style J fill:#fce4ec
style L fill:#ffcdd2
style H fill:#ef9a9a
Last reviewed commit: "fix: use DEAD_ROLE_P..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (node.file) { | ||
| // Cross-language FFI: compiled-language files in a JS/TS project | ||
| const dotIdx = node.file.lastIndexOf('.'); | ||
| if (dotIdx !== -1 && FFI_EXTENSIONS.has(node.file.slice(dotIdx))) return 'dead-ffi'; | ||
|
|
||
| // Framework-dispatched entry points (CLI commands, MCP tools, routes) | ||
| if (ENTRY_PATH_PATTERNS.some((p) => p.test(node.file))) return 'dead-entry'; |
There was a problem hiding this comment.
dead-ffi priority blinds dead-entry for FFI files in entry-point paths
Because the file-extension check (→ dead-ffi) runs before the path-pattern check (→ dead-entry), a function in, say, src/cli/commands/analyze.rs will always be labelled dead-ffi — even though it lives in a CLI command directory. The existing priority tests cover dead-leaf > dead-ffi and dead-leaf > dead-entry, but there is no test for the dead-ffi vs dead-entry case. This is presumably intentional (the FFI boundary is considered more fundamental than the path hint), but a comment noting the deliberate ordering would prevent future readers from accidentally swapping the two blocks.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 20881fa. Added a comment above the dead-ffi check explaining the deliberate priority: FFI boundary is a more fundamental classification than path-based entry-point hints (e.g. a .so/.dll in a routes/ directory is still FFI, not an entry point).
src/db/query-builder.js
Outdated
| if (role === 'dead') { | ||
| this.#conditions.push("n.role LIKE 'dead%'"); | ||
| } else { |
There was a problem hiding this comment.
LIKE 'dead%' pattern duplicated across three sites
Identical LIKE 'dead%' logic (hardcoded literal) appears in three places:
src/db/query-builder.js(line 250)src/domain/analysis/roles.js(line 17)src/db/repository/in-memory-repository.js(line 265, asstartsWith('dead'))
If the prefix ever changes (e.g., dead roles are renamed to unused-*), three callsites must be updated in sync. A shared constant such as export const DEAD_ROLE_PREFIX = 'dead' in kinds.js (alongside DEAD_SUB_ROLES) would make this a single-point change and clarify the intent.
There was a problem hiding this comment.
Fixed in 3e90e19. Added DEAD_ROLE_PREFIX = 'dead' to src/shared/kinds.js alongside DEAD_SUB_ROLES, and updated all three consumption sites to use it:
src/db/query-builder.js:roleFilter()now usesDEAD_ROLE_PREFIXfor the comparison and LIKE patternsrc/domain/analysis/roles.js:rolesData()uses the constant instead of hardcoded stringsrc/db/repository/in-memory-repository.js:startsWith()check uses the constant
|
All three review comments addressed:
All 1922 tests pass, lint clean on changed files. |
src/db/query-builder.js
Outdated
| this.#conditions.push('n.role = ?'); | ||
| this.#params.push(role); | ||
| if (role === DEAD_ROLE_PREFIX) { | ||
| this.#conditions.push(`n.role LIKE '${DEAD_ROLE_PREFIX}%'`); |
There was a problem hiding this comment.
Every other value in roleFilter (and across the query builder) is pushed to #params and bound via ?. Here the prefix is interpolated directly into the SQL string as a string literal. While DEAD_ROLE_PREFIX = 'dead' is a safe constant today, this deviates from the codebase's consistent parameterized-query pattern. If the constant ever acquired a SQL-special character (e.g., a quote), this would silently break. Consider:
| this.#conditions.push(`n.role LIKE '${DEAD_ROLE_PREFIX}%'`); | |
| this.#conditions.push('n.role LIKE ?'); | |
| this.#params.push(`${DEAD_ROLE_PREFIX}%`); |
The same pattern applies to src/domain/analysis/roles.js line 18.
Add comment explaining why FFI boundary classification takes precedence over entry-point path hints — a .so/.dll in a routes/ directory is fundamentally an FFI boundary, not an entry point. Impact: 1 functions changed, 2 affected
…attern Add shared DEAD_ROLE_PREFIX = 'dead' in shared/kinds.js and use it in query-builder.js, domain/analysis/roles.js, and in-memory-repository.js instead of duplicating the 'dead' string and LIKE 'dead%' pattern. Impact: 5 functions changed, 0 affected
The test graph comment still said deadFn -> dead but the actual classification since Phase 4.1 produces dead-unresolved for nodes with no special sub-category match.
Replace template-literal interpolation of DEAD_ROLE_PREFIX in SQL LIKE patterns with proper parameterized ? bindings, consistent with the rest of the query builder's convention. Impact: 3 functions changed, 0 affected
Replace hardcoded 'dead' string with the shared DEAD_ROLE_PREFIX constant, making it the last consumption site to use the single-source constant from kinds.js. Impact: 1 functions changed, 1 affected
carlos-alm
force-pushed
the
docs/roadmap-audit-alignment
branch
from
aab2ba9 to
55bb468
Compare
|
Addressed the remaining Greptile findings:
All 1922 tests pass, lint clean. |
1 participant
Summary
deadrole into four categories:dead-leaf(parameters/properties/constants),dead-entry(CLI commands, MCP tools, routes/handlers),dead-ffi(cross-language FFI —.rs,.c,.go, etc.), anddead-unresolved(genuinely unreferenced callables)--role deadstill matches all sub-roles for backward compatibility (usesLIKE 'dead%'/ prefix matching)dead-entry: 0.3,dead-unresolved: 0.15,dead-ffi: 0.05,dead-leaf: 0.017 files changed across classifier, DB query layer, CLI, MCP, stats, and tests. Full test suite passes (1922 tests).
Test plan
classifyNodeRoleswith in-memory DBrolesData,statsData,whereData,explainData