refactor: Titan audit — decompose, reduce complexity, remove dead code#699
refactor: Titan audit — decompose, reduce complexity, remove dead code#699carlos-alm merged 40 commits intomainfrom
Conversation
Extract hasFuncBody, setupAstVisitor, setupComplexityVisitorForFile, and setupCfgVisitorForFile from the monolithic setupVisitors function. Each helper encapsulates one visitor's setup logic, reducing cognitive complexity and improving readability.
Extract handler functions from extractSymbolsQuery (cog 78, bugs 2.43): handleFnCapture, handleVarFnCapture, handleClassCapture, handleMethodCapture, handleExportCapture, and dispatchQueryMatch. Extract from extractGoTypeMapDepth (cog 143, bugs 1.15): handleTypedIdentifiers, inferShortVarType, handleShortVarDecl.
Extract from complexityData (cog 72, bugs 2.65): buildComplexityWhere, buildThresholdHaving, mapComplexityRow, exceedsAnyThreshold, computeComplexitySummary, checkHasGraph. Extract from prepareFunctionLevelData (cog 66, bugs 2.54): buildNodeMapFromEdges, loadComplexityMap, loadFanMaps, buildEnrichedVisNode, selectSeedNodes.
…rmatters Extract formatPredicateViolations from check (cog 62). Extract renderAuditFunction and renderHealthMetrics from audit (cog 55). Extract formatAddedSection, formatRemovedSection, formatChangedSection, and formatImpactLine from branch-compare formatText (cog 48).
…elpers Extract buildDirFilesMap, buildFileToAncestorDirs, countDirectoryEdges, and countSymbolsInFiles from computeDirectoryMetrics (cog 73, bugs 0.92).
Extract renderHealthTable and renderDefaultTable from complexity (cog 40, bugs 1.59), separating the two table-rendering modes.
Extract focused helper functions from monolithic walk/build functions: - edge_builder: split build_call_edges into EdgeContext + 8 handlers - complexity: extract classify_branch, detect_else_if, is_pattern_c_else, handle_logical_op, classify_halstead shared by walk and walk_all - cfg: decompose process_statement into try_unwrap_expr_stmt, try_process_labeled, try_process_control_flow, try_process_terminal, process_break, process_continue
Extract DataflowOutput struct to bundle output vectors, visit_children helper to eliminate repeated DFS boilerplate, and handle_return_stmt from inline visit logic.
javascript.ts: extract handleVarDeclaratorTypeMap, handleParamTypeMap, extractMemberExprCallInfo, extractSubscriptCallInfo, extractRequireReexport, extractSpreadRequireReexports from monolithic extractTypeMapWalk, extractCallInfo, handleCommonJSAssignment. go.ts: extract handleGoStructType, handleGoInterfaceType from handleGoTypeDecl.
Extract shared indexByLine and matchResultToDef helpers from duplicated code in storeComplexityResults and storeCfgResults. Extract classifyLogicalOp from inline logic in createComplexityVisitor's enterNode method.
Extract focused helper functions from buildCFGData (cognitive 86), dataflowPathData (cognitive 40), dataflowData (halstead.bugs 1.21), dataflowImpactData (nesting 6), and checkData (cyclomatic 26).
Extract focused helpers from do_classify_incremental (196 SLOC, halstead.bugs 2.9) and walk_node_depth (cognitive 91, nesting 11). Share classify_rows and test_file_filter between full and incremental classification paths.
|
Claude finished @carlos-alm's task —— View job PR Review: Titan Audit RefactoringTodo List:
Root Cause AnalysisProblem Being Solved: Genuine code quality issues identified by Titan audit: high complexity functions ( Why This Approach: Legitimate refactoring following established patterns: function decomposition for high-complexity targets, shared abstraction extraction to eliminate duplication, dead code removal based on actual usage analysis. No rule relaxation or test weakening detected. Risk Assessment: ✅ LOW RISK - This is genuine refactoring, not symptom hiding. The Titan report shows actual improvements in objective metrics (MI scores, function counts, cycle reduction). Backlog Compliance
Critical Concerns❌ NONE - This PR passes all phases:
Final Recommendation
|
Greptile SummaryThis PR is a comprehensive Titan pipeline audit of the codegraph codebase (v3.5.0), touching 63 files across 13 domains. The changes are purely structural — decomposition of oversized functions, extraction of shared abstractions, and dead code removal — with no new features or behavior changes. All 2131 tests pass and all 22 gate validations pass. Key structural improvements:
Confidence Score: 5/5Safe to merge — all changes are structural refactoring with preserved semantics, 2131 tests pass, and the single finding is a P2 style nit. The only finding is three src/domain/graph/watcher.ts — three new catch blocks use unsafe Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph finalize["finalize.ts"]
F[finalize] --> RW[releaseWasmTrees]
F --> DI[detectIncrementalDrift]
F --> PB[persistBuildMetadata]
F --> RA[runAdvisoryChecks]
end
subgraph leiden["Leiden"]
MP[makePartition] --> ANA[accumulateNodeAggregates]
MP --> AIEW[accumulateInternalEdgeWeights]
BOP[buildOriginalPartition] --> ANA
RLUM[runLouvain] --> FBCM[findBestCommunityMove]
end
subgraph engine["AST Engine"]
SV[setupVisitors] --> SAV[setupAstVisitor]
SV --> SCVF[setupComplexityVisitor]
SV --> SCFVF[setupCfgVisitor]
end
Reviews (3): Last reviewed commit: "fix: resolve merge conflicts with main (..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function compactCommunityIds(opts: CompactOptions = {}): void { | ||
| const ids: number[] = []; | ||
| for (let c = 0; c < communityCount; c++) if (iget(communityNodeCount, c) > 0) ids.push(c); | ||
| if (opts.keepOldOrder) { | ||
| ids.sort((a, b) => a - b); | ||
| } else if (opts.preserveMap instanceof Map) { | ||
| const preserveMap = opts.preserveMap; | ||
| ids.sort((a, b) => { | ||
| const pa = preserveMap.get(a); | ||
| const pb = preserveMap.get(b); | ||
| if (pa != null && pb != null && pa !== pb) return pa - pb; | ||
| if (pa != null && pb == null) return -1; | ||
| if (pb != null && pa == null) return 1; | ||
| return ( | ||
| fget(communityTotalSize, b) - fget(communityTotalSize, a) || | ||
| iget(communityNodeCount, b) - iget(communityNodeCount, a) || | ||
| a - b | ||
| ); | ||
| }); | ||
| } else { | ||
| ids.sort( | ||
| (a, b) => | ||
| fget(communityTotalSize, b) - fget(communityTotalSize, a) || | ||
| iget(communityNodeCount, b) - iget(communityNodeCount, a) || | ||
| a - b, | ||
| ); | ||
| } | ||
| buildSortedCommunityIds(ids, opts, communityTotalSize, communityNodeCount); |
There was a problem hiding this comment.
buildSortedCommunityIds
buildSortedCommunityIds sorts ids in-place and returns the sorted array, but the return value is discarded at the call site. The function signature implies the caller should use the return value, which creates a subtle API confusion. Since Array.prototype.sort mutates in place the behavior is correct — but the misleading signature could cause a future caller to forget the in-place mutation and use the return value thinking it's a new array.
Consider either:
- Making the return type
void(since the in-place sort is the actual contract), or - Using the return value:
ids = buildSortedCommunityIds(ids, opts, communityTotalSize, communityNodeCount);
| function compactCommunityIds(opts: CompactOptions = {}): void { | |
| const ids: number[] = []; | |
| for (let c = 0; c < communityCount; c++) if (iget(communityNodeCount, c) > 0) ids.push(c); | |
| if (opts.keepOldOrder) { | |
| ids.sort((a, b) => a - b); | |
| } else if (opts.preserveMap instanceof Map) { | |
| const preserveMap = opts.preserveMap; | |
| ids.sort((a, b) => { | |
| const pa = preserveMap.get(a); | |
| const pb = preserveMap.get(b); | |
| if (pa != null && pb != null && pa !== pb) return pa - pb; | |
| if (pa != null && pb == null) return -1; | |
| if (pb != null && pa == null) return 1; | |
| return ( | |
| fget(communityTotalSize, b) - fget(communityTotalSize, a) || | |
| iget(communityNodeCount, b) - iget(communityNodeCount, a) || | |
| a - b | |
| ); | |
| }); | |
| } else { | |
| ids.sort( | |
| (a, b) => | |
| fget(communityTotalSize, b) - fget(communityTotalSize, a) || | |
| iget(communityNodeCount, b) - iget(communityNodeCount, a) || | |
| a - b, | |
| ); | |
| } | |
| buildSortedCommunityIds(ids, opts, communityTotalSize, communityNodeCount); | |
| const sortedIds = buildSortedCommunityIds(ids, opts, communityTotalSize, communityNodeCount); | |
| const newId = new Int32Array(communityCount).fill(-1); | |
| sortedIds.forEach((c, i) => { |
There was a problem hiding this comment.
Fixed — changed buildSortedCommunityIds return type to void since the sort is in-place and the return value was unused at the call site. This makes the API contract explicit.
| code = readFileSafe(absPath); | ||
| } catch { | ||
| } catch (e: unknown) { | ||
| debug(`parseReverseDep: cannot read ${absPath}: ${(e as Error).message}`); | ||
| return null; |
There was a problem hiding this comment.
(e as Error).message assumes the caught value is always an Error instance. If readFileSafe throws a string or a non-Error object, .message will be undefined and the debug log will print "parseReverseDep: cannot read <path>: undefined". Consider using a safer accessor:
| code = readFileSafe(absPath); | |
| } catch { | |
| } catch (e: unknown) { | |
| debug(`parseReverseDep: cannot read ${absPath}: ${(e as Error).message}`); | |
| return null; | |
| debug(`parseReverseDep: cannot read ${absPath}: ${e instanceof Error ? e.message : String(e)}`); |
There was a problem hiding this comment.
Fixed — replaced (e as Error).message with e instanceof Error ? e.message : String(e) to safely handle non-Error rejections.
src/domain/analysis/query-helpers.ts
Outdated
| config: any; | ||
| displayOpts: Record<string, unknown>; | ||
| } { | ||
| const noTests = opts.noTests || false; | ||
| const config = opts.config || loadConfig(); | ||
| const displayOpts = config.display || {}; | ||
| return { noTests, config, displayOpts }; | ||
| } |
There was a problem hiding this comment.
config typed as any leaks into the shared helper
resolveAnalysisOpts returns config: any, which gives callers untyped access to the config object. The individual callers (context.ts, fn-impact.ts, exports.ts, dependencies.ts) already know they're working with the project's CodegraphConfig shape. Typing the return as config: CodegraphConfig (or a minimal interface) would prevent silent typos on properties like config.analysis?.fnImpactDepth and config.display elsewhere in the codebase.
There was a problem hiding this comment.
Fixed — typed config as CodegraphConfig (imported from types.ts) instead of any in both the parameter and return type of resolveAnalysisOpts. All callers type-check cleanly.
|
Claude finished @carlos-alm's task —— View job PR Review: Titan Audit RefactoringComprehensive review of the 62-file refactoring with objective quality improvements. Todo List:
Assessment: ⭐⭐⭐⭐⭐ EXCELLENT REFACTORING✅ What This PR Does Well1. Objective Quality Improvements
2. Systematic Approach
3. High-Quality Refactoring Patterns Dead Code Removal (src/db/connection.ts:158) // REMOVED: _resetVersionWarning (no remaining callers)
// EXTRACTED: warnOnVersionMismatch helper with proper deduplicationShared Abstraction Extraction (src/domain/analysis/query-helpers.ts:1-36) // NEW: withReadonlyDb<T> - eliminates 4x try/finally DB patterns
// NEW: resolveAnalysisOpts - eliminates config loading duplicationFunction Decomposition (src/graph/algorithms/leiden/partition.ts:419-429) // DECOMPOSED: makePartition (MI 5→13.4) into focused helpers:
// - accumulateNodeAggregates
// - accumulateInternalEdgeWeights
// - buildSortedCommunityIds4. Greptile P2 Issues: All Fixed ✅
5. Behavioral Preservation
6. Follows Repository Guidelines
|
1 participant
Summary
Full Titan pipeline audit of the codegraph codebase (v3.5.0). 122 files audited across 13 domains, 34 targets addressed in 32 commits.
helpers.rs), WASM extractor visitor utilities, and analysis query-building patternsmakePartition(MI 5 -> 13.4),walk_node_depthacross 9 native extractors,build_call_edges, MCP server, graph builder finalize stage, AST engine, and presentation formattersnoUncheckedIndexedAccesserrors and exported missing interface typesMetrics
All 2131 tests pass. 22 gate validations: 14 pass, 8 warn, 0 fail, 0 rollbacks.
Titan Audit Context
generated/titan/titan-report-v3.5.0-2026-03-30T03-04-14.mdTest plan