fix: address quality issues in ast-analysis, presentation, extractors, and cli#1790
fix: address quality issues in ast-analysis, presentation, extractors, and cli#1790carlos-alm wants to merge 32 commits into
Conversation
…docs check acknowledged) Impact: 1 functions changed, 2 affected
…n algorithm files docs check acknowledged: internal helper extraction only, no user-facing feature/language/architecture-table changes. Impact: 10 functions changed, 27 affected
Impact: 11 functions changed, 11 affected
…n readFileSafe readFileSafe's Atomics.wait busy-block froze the entire Node.js event loop (all I/O and timer callbacks) for up to 100ms per retry on the watch-mode hot path. Extracts journal.ts's existing sleepSync busy-spin helper into src/shared/sleep.ts so both readFileSafe and journal.ts's lock-retry loop share one implementation instead of duplicating it. docs check acknowledged: internal bug fix, no feature/language/architecture table changes warranted in README.md, CLAUDE.md, or ROADMAP.md. Impact: 2 functions changed, 31 affected
…complexity.ts Impact: 14 functions changed, 10 affected
Registers resolveSecrets' execFileSync timeout/maxBuffer in DEFAULTS.llm (apiKeyCommandTimeoutMs, apiKeyCommandMaxBufferBytes) and wires resolveSecrets to read them from config instead of hardcoding. Also adds three purely-additive @reserved DEFAULTS entries for constants hardcoded elsewhere in the codebase (build. largeCodebaseFileThreshold, db.busyTimeoutMs, community. capacityGrowthFactor) so their consumer files can be wired to them in follow-up commits. docs check acknowledged — no new feature/language/architecture change; docs/guides/configuration.md (the actual config reference) is already updated in this commit. Impact: 5 functions changed, 87 affected
…presentation/plot.ts Impact: 4 functions changed, 3 affected
…l/runContextCollectorWalk Pure extract-method decomposition of the three highest-complexity functions in extractors/javascript.ts (Titan phase 10, sync.json commit message shortened to fit the 100-char commitlint header limit). No extraction logic, node-type handling, or edge-case behavior changed -- verified byte-identical resolution-benchmark precision/recall across all 34 fixture languages and byte-identical codegraph query/where output for 3 real non-fixture files before/after. No Rust files touched, so native/WASM parity is unaffected by construction. docs check acknowledged: internal-only refactor, no new languages/commands/ architecture; README.md, CLAUDE.md, and ROADMAP.md are unaffected. Impact: 20 functions changed, 15 affected
…ative in build-edges.ts docs check acknowledged: pure internal extract-method refactor, no new features, commands, languages, or architecture changes — README/CLAUDE/ROADMAP do not need updates. Impact: 20 functions changed, 15 affected
…or in native-orchestrator.ts Pure extract-class decomposition of the DECOMPOSE-flagged worst offender in Titan phase 12 (halstead.bugs 1.17). tryNativeOrchestrator's native-DB lifecycle steps (open/build/backfill/handoff/close) are now owned by a NativeOrchestrationSession class; tryNativeOrchestrator becomes a thin sequencer of session method calls. No dispatch logic, fallback conditions, or error handling changed -- verified via full test suite (200/200 files, 3330/3330 tests), byte-identical resolution-benchmark output across all 34 fixture languages, and byte-identical native-engine DB dumps (full build + incremental early-exit) on tests/fixtures/sample-project before/after. tryNativeOrchestrator: cognitive 35->24, cyclomatic 34->25, halstead.bugs 1.17->0.83, mi 49.7->54.1. docs check acknowledged: pure internal extract-class refactor, no new features, commands, languages, or architecture changes -- README/CLAUDE/ ROADMAP do not need updates. Impact: 9 functions changed, 6 affected Impact: 9 functions changed, 6 affected
…tor in remote.ts Extract-method refactor only, no behavior change. embedRemote's per-batch body is split into executeRemoteEmbeddingRequest (build request body, fetch-with-timeout, map network/timeout/status failures to EngineError) and mapRemoteEmbeddingResponse (shape-check, index-sort, embedding-field check, cross-batch dimension-consistency check, Float32Array conversion), with the outer loop calling both in the same order as before. Drops embedRemote from cognitive=36/halstead.bugs=1.10 (DECOMPOSE-flagged worst offender in GAUNTLET) to cognitive=6/bugs=0.38; both new helpers are well within thresholds (cognitive 11 and 9, bugs 0.38 and 0.33). Deliberately does not fix the gauntlet's secondary finding that response.json() sits outside error handling (a malformed body throws a raw SyntaxError instead of EngineError) -- that's a behavior change, out of scope for this pure decomposition. Filed as #1745. docs check acknowledged: internal refactor only, no CLI/feature/language/ architecture surface changed -- README/CLAUDE.md/ROADMAP untouched by design. Impact: 3 functions changed, 10 affected
…dedupe consent glob-matching applyExcludeTestsShorthand mutated merged.query in place, which was still a live reference to the shared DEFAULTS.query singleton whenever no config layer had already overridden `query`. In long-running processes (e.g. `codegraph mcp --multi-repo`) this permanently leaked one repo's excludeTests setting into every subsequent loadConfig() call for any other repo. loadConfig now deep-clones DEFAULTS before merging so no layer can ever write onto a live DEFAULTS reference, and DEFAULTS itself is now deep-frozen so any future regression of this kind throws immediately instead of silently corrupting shared state. applyExcludeTestsShorthand was also hardened to copy-on-write its `query` key directly. Also dedupes the appliesTo-glob-matching logic (previously copy-pasted between resolveConsent and promptForConsentIfNeeded) into a shared matchesAppliesTo helper. No user-facing behavior, CLI surface, or language support changed — docs check acknowledged. Fixes #1725 Impact: 6 functions changed, 140 affected
…pe engine resolution openReadonlyWithNative opened the better-sqlite3 handle before resolving the engine, and engine resolution calls loadConfig(), which can throw (e.g. ConfigError from resolveSecrets on a malformed llm.apiKeyCommand). If that throw happened, the already-open DB handle was never closed -- a real leak on the hot path used by dataflow/hotspots/stats commands. Fix: resolve the engine (and thus loadConfig) before opening the DB, mirroring openRepo's existing, correct ordering. Extracted the shared engine-resolution logic (customDbPath > rootDir > loadConfig priority chain) into resolveDbEngine(), used by both openRepo and openReadonlyWithNative so the two call sites can't drift again. Added tests/unit/openReadonlyWithNative-leak.test.ts: tracks every better-sqlite3 Database instantiation and asserts zero occur when loadConfig throws. Verified this test fails against the pre-fix ordering (it recorded a leaked instance) and passes against the fix. docs check acknowledged: internal bug fix + dedup, no CLI surface, language support, or documented architecture/design decision changed. Impact: 3 functions changed, 38 affected
…ne and cli/commands/info.ts
Converts 13 comment-only/silent catch blocks in pipeline.ts, native-orchestrator.ts,
detect-changes.ts, helpers.ts, and info.ts from `catch { /* comment */ }` to
`catch (e) { debug(...) }`, using the existing infrastructure/logger.ts debug()
utility. Purely additive observability -- no control-flow changes, no change to
what errors are swallowed vs rethrown.
docs check acknowledged: internal logging-only change, no new feature/language/
architecture/command surface to document in README/CLAUDE.md/ROADMAP.md.
Impact: 13 functions changed, 17 affected
…(docs check acknowledged) Impact: 4 functions changed, 4 affected
…nfig (docs check acknowledged) Impact: 6 functions changed, 12 affected
…docs check acknowledged) Impact: 6 functions changed, 22 affected
…owledged) Decompose gauntlet-flagged FAIL-level complexity in points-to.ts, strategy.ts, and ts-resolver.ts via pure extract-method refactoring. No resolution-behavior change (verified byte-for-byte identical resolution-benchmark output across all 34 fixture languages). Impact: 41 functions changed, 32 affected
…ut of domain layer Impact: 1 functions changed, 8 affected
…aliasing, leiden complexity)
model.ts: merge() aliased NodeAttrs/EdgeAttrs objects by reference instead
of cloning, unlike subgraph()/filterEdges()/clone() which all defensively
copy with { ...attrs }. A caller merging graph B into graph A could
silently leak mutations across graphs via the shared attrs object. No
production caller exists today (verified via fn-impact: 0 callers besides
the test), so this was a latent defect, not a demonstrated one -- now
fixed to match the file's established convention.
leiden/partition.ts + leiden/adapter.ts: decomposed the remaining
cognitive/cyclomatic-exceeding functions left after phase 3's shared
aggregate/typed-array helper extraction (commit 0f9bbe6), following the
same directed/undirected-branch-splitting pattern gauntlet recommended:
- moveNode (cognitive 16->2, cyclomatic 13->3): split into
applyMoveStrengthTotals + applyMoveInternalEdgeWeightDelta[Directed/Undirected]
- buildSortedCommunityIds (cognitive 17->3): extracted compareBySizeDesc/
compareByPreserveMap comparators
- computeDeltaCPM (cognitive 17->4): extracted computeCpmEdgeWeights[Directed/Undirected]
- makeGraphAdapter (cognitive 27->3): extracted resolveAdapterOptions,
buildNodeIndex, computeNodeSizes, makeForEachNeighbor
- populateUndirectedEdges (cognitive 28->0): extracted
aggregateUndirectedPairs/recordUndirectedPairWeight, emitUndirectedPairs,
applyUndirectedSelfLoops
Pure behavior-preserving decomposition -- no algorithm changes. Verified:
full test suite (201/201 files, 3336 tests), leiden-specific suite
(22/22), graph suite (177/177 incl. merge()), typecheck, and lint all
green. Community-detection output on the leiden-specific directories is
byte-identical before/after (confirmed via codegraph communities --drift
split-candidates, controlling for the known #1734 run-to-run noise).
docs check acknowledged: internal refactor + bug fix only, no user-facing
feature/language/architecture-table changes.
Impact: 28 functions changed, 31 affected
…ck acknowledged) Extract getExceededMetrics as the single source of truth for which manifesto thresholds a row exceeds, shared by mapComplexityRow and exceedsAnyThreshold — cuts mapComplexityRow's cyclomatic complexity from 23 (fail) to 10 and removes the duplicated 4-branch check. Replace the hardcoded default-threshold object with DEFAULTS.manifesto.rules (config.ts is already the source of truth for these values). Decompose complexityData/computeComplexitySummary (resolveComplexityQueryOptions, buildComplexityResult, queryComplexityRows, fetchAllComplexityMetrics, summarizeComplexityMetrics, average) to bring halstead.effort for every function in the file under the 15000 fail threshold. Pure decomposition, zero behavior change — verified via clean rebuild + full test suite. Widen tests/integration/complexity.test.ts's config.js mock to preserve real exports via importOriginal (it previously replaced the whole module, which broke once this file started importing DEFAULTS). Impact: 24 functions changed, 8 affected
…wledged) Wire computeCoChanges/analyzeCoChanges's minSupport/maxFilesPerCommit/since fallback literals through DEFAULTS.coChange instead of re-declaring the same magic numbers in two places (extended the same fix to minJaccard in coChangeData/coChangeTopData/coChangeForFiles for consistency). Decompose computeCoChanges' three passes (per-file counts, pair generation, Jaccard filtering) into named helpers (updateFileCommitCounts, updatePairCounts, buildCoChangeResults), plus scanGitHistory, analyzeCoChanges, coChangeData, coChangeTopData, and coChangeForFiles — bringing halstead.effort for every one of the 26 functions in the file under the 15000 fail threshold (worst was computeCoChanges at 65249.68). Fix the loadLastAnalyzedSha/loadKnownFiles silent catches to log via debug(), matching scanGitHistory's existing error-visibility pattern. Pure decomposition + config wiring, zero behavior change — verified via clean rebuild + full test suite (including the real git-history integration tests in cochange.test.ts). Impact: 23 functions changed, 15 affected
… acknowledged) This was the run's worst gauntlet offender (halstead.bugs 1.585 on branchCompareData). Pure decomposition per the gauntlet recommendation: extract git-ref validation (validateBranchCompareRefs), dual-worktree + dual-buildGraph setup (setupCompareWorktrees), and output-shape cleanup (shapeBranchCompareSymbolLists) out of branchCompareData; unify attachImpactToSymbols/attachImpactToChanged into one generic attachImpact(symbols, resolveId, dbPath, maxDepth, noTests) parameterized by id-resolution strategy. Extended the same treatment to the file's other named-FAIL functions (loadSymbolsFromDb: halstead.effort 123718.05->12326.18, bugs 0.9546->0.2182; branchCompareMermaid: cyclomatic 22->6) and to pre-existing effort-fails gauntlet's summary didn't name explicitly (loadCallersFromDb, compareSymbols) -- consistent with this phase's cochange.ts/complexity-query.ts fixes, where the file-level FAIL verdict covers every function over threshold, not just the 2-3 worst examples cited in the audit detail text. Zero behavior change: both exported functions (branchCompareData, branchCompareMermaid) keep byte-identical signatures; every extraction preserves exact call order, error-handling scope (the try/catch/finally around worktree creation is untouched), and the existing mutate-in-place impact-attachment pattern. Verified via tests/integration/branch-compare.test.ts, which exercises real git worktrees + buildGraph + DB comparison end-to-end (not mocked), plus the full suite, both before and after each incremental edit. Impact: 44 functions changed, 15 affected
Impact: 16 functions changed, 14 affected
…ck acknowledged) Impact: 48 functions changed, 9 affected
…nowledged) Decomposes the highest-complexity functions across 7 independent single-language extractor files (sync.json phase 26): r, dart, groovy, csharp, elixir, scala, julia. Pure behavior-preserving decomposition — no extraction-logic changes. Resolution benchmark precision/recall/ TP/FP/FN confirmed byte-for-byte identical to baseline for all 7 languages. Impact: 22 functions changed, 29 affected
…/printBuildMetadata docs check acknowledged — pure internal decomposition of CLI output logic, no new features/languages/architecture changes; README/CLAUDE/ ROADMAP do not need updates. Impact: 5 functions changed, 2 affected
Impact: 2 functions changed, 3 affected
Greptile SummaryThis PR addresses quality issues from a GAUNTLET audit by extracting helpers, building dispatch tables, and adopting concrete types across
Confidence Score: 5/5Safe to merge — all changes are structural refactors with no business logic modifications; behavior is preserved throughout. Every changed file is a mechanical extraction: helpers pulled out of long functions, dispatch tables replacing switch/if-else cascades, and type declarations promoted from inline No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph cfg-conditionals ["cfg-conditionals.ts (new shared helper)"]
processBranch["processBranch(condBlock, joinBlock, S, branchKind, label, runBranchBody)"]
B1[makeBlock] --> B2[addEdge condBlock→branchBlock]
B2 --> B3[runBranchBody → branchEnd]
B3 --> B4{branchEnd non-null?}
B4 -->|yes| B5[addEdge branchEnd→joinBlock fallthrough]
processBranch --> B1
end
subgraph callers ["Previously inlined 6+ times — now delegating"]
processIf -->|true branch| processBranch
processAlternative -->|else-if branch| processBranch
processAlternative -->|else branch| processBranch
processElifSiblings -->|elif true branch| processBranch
processElifSiblings -->|elif else branch| processBranch
end
subgraph audit-presentation ["presentation/audit.ts decomposition"]
renderAuditFunction --> renderFunctionHeader
renderAuditFunction --> renderHealthMetrics
renderAuditFunction --> renderThresholdBreaches
renderAuditFunction --> renderImpactSection
renderAuditFunction --> renderCallRefs_calls["renderCallRefs('Calls')"]
renderAuditFunction --> renderCallRefs_calledby["renderCallRefs('Called by')"]
renderAuditFunction --> renderRelatedTests
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
subgraph cfg-conditionals ["cfg-conditionals.ts (new shared helper)"]
processBranch["processBranch(condBlock, joinBlock, S, branchKind, label, runBranchBody)"]
B1[makeBlock] --> B2[addEdge condBlock→branchBlock]
B2 --> B3[runBranchBody → branchEnd]
B3 --> B4{branchEnd non-null?}
B4 -->|yes| B5[addEdge branchEnd→joinBlock fallthrough]
processBranch --> B1
end
subgraph callers ["Previously inlined 6+ times — now delegating"]
processIf -->|true branch| processBranch
processAlternative -->|else-if branch| processBranch
processAlternative -->|else branch| processBranch
processElifSiblings -->|elif true branch| processBranch
processElifSiblings -->|elif else branch| processBranch
end
subgraph audit-presentation ["presentation/audit.ts decomposition"]
renderAuditFunction --> renderFunctionHeader
renderAuditFunction --> renderHealthMetrics
renderAuditFunction --> renderThresholdBreaches
renderAuditFunction --> renderImpactSection
renderAuditFunction --> renderCallRefs_calls["renderCallRefs('Calls')"]
renderAuditFunction --> renderCallRefs_calledby["renderCallRefs('Called by')"]
renderAuditFunction --> renderRelatedTests
end
Reviews (4): Last reviewed commit: "refactor: address warnings in benchmark ..." | Re-trigger Greptile |
Codegraph Impact Analysis120 functions changed → 98 callers affected across 26 files
|
| const data: AuditResult = auditData(target, customDbPath, opts); | ||
|
|
||
| if (outputResult(data, null, opts)) return; | ||
| if (outputResult(data as unknown as Record<string, unknown>, null, opts)) return; |
There was a problem hiding this comment.
Double cast bypasses well-typed
AuditResult
data as unknown as Record<string, unknown> is required only because outputResult takes a Record<string, unknown> parameter. Since AuditResult is now a concrete, exported interface, widening outputResult's signature to accept object would remove the escape hatch and let the type system verify callers. The cast is not a bug today, but it silently defeats the type improvements introduced elsewhere in this PR.
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 — removed the double cast entirely. Turns out it wasn't load-bearing: AuditResult assigns cleanly to outputResult's Record<string, any> parameter without any cast (verified with tsc --noEmit). Widening outputResult to accept object would be a good follow-up but touches ~20 call sites across the presentation layer that use the same as unknown as Record<string, unknown> pattern (queries-cli/impact.ts, path.ts, inspect.ts, overview.ts, triage.ts, owners.ts, exports.ts) — out of scope for this PR, which is limited to ast-analysis/presentation/extractors/cli. Filed #1803 for the repo-wide signature widening.
| levels: Record<number, ImpactLevelEntry[]>; | ||
| }; | ||
| health: { | ||
| cognitive: number; | ||
| cyclomatic: number; | ||
| maxNesting: number; | ||
| maintainabilityIndex: number | null; | ||
| halstead: HalsteadMetrics | null; | ||
| loc: number; | ||
| sloc: number; | ||
| commentLines: number; | ||
| thresholdBreaches: string[]; | ||
| }; | ||
| riskScore: number; | ||
| complexityNotes: string[]; | ||
| sideEffects: string[]; | ||
| health: AuditHealthMetrics; | ||
| riskScore: number | null; | ||
| complexityNotes: string | null; | ||
| sideEffects: string | null; | ||
| } |
There was a problem hiding this comment.
AuditFunctionEntry field types corrected from incorrect prior declarations
callees/callers change from string[] to Array<{name, kind, file, line}>, and relatedTests from string[] to Array<{file: string}>. These are corrections — the actual runtime values were always objects — but any external consumer of codegraph audit --json that treated these as string arrays will silently break. Worth calling out in release notes if the JSON output is considered a public API.
There was a problem hiding this comment.
Acknowledged, but verified this is not an actual behavior/output change. I traced callees/callers/relatedTests back through features/audit.ts history (e.g. commit 4a6787e) — they were always populated with SymbolRef[] and {file}[] objects at runtime; only the TS declaration was wrong. Since codegraph audit --json was already emitting objects (never strings), no external consumer that actually inspected the real output could have relied on the incorrect string-array shape, so no release-note callout is needed here — the JSON contract itself hasn't changed, just the type-checker's understanding of it.
|
Addressed Claude's/Greptile's review feedback:
|
…nfo (#1790) printNativeVersionInfo now takes a resolved NativeAddon directly instead of re-invoking loadNative() and asserting it non-null. isNativeAvailable() and loadNative() are two independent code paths; if they ever diverge the assertion would crash instead of degrading gracefully. Impact: 2 functions changed, 2 affected
AuditResult assigns cleanly to outputResult's Record<string, any> parameter without a cast; the `as unknown as Record<string, unknown>` was dead weight left over from before AuditResult was a concrete type. Impact: 1 functions changed, 1 affected
…is naming (#1791) * refactor: address warnings in benchmark tracer tooling Split loader-hooks.mjs's instrumentSource (cognitive 34, cyclomatic 22) into single-purpose matcher/tracker helpers. Extract the duplicated line-scanning loop shared by native-tracer.sh's trace_rust/trace_swift/ trace_dart/trace_zig into a parameterized inject_trace_calls plus small maybe_close_context/maybe_close_finally_scope/maybe_inject_declaration/ instrument_one_file helpers, and rename the LANG variable to TRACE_LANG so it no longer clobbers the POSIX locale env var inherited by every spawned compiler toolchain. Log lua-tracer.lua's swallowed pcall error to stderr so a genuine fixture failure is visible instead of looking like a silent successful trace. Impact: 17 functions changed, 11 affected * refactor: address warnings in ast-analysis and extractors/helpers naming (docs check acknowledged) Impact: 12 functions changed, 36 affected * fix: use compgen -G for glob expansion in inject_trace_calls (#1791) Unquoted `for srcfile in $glob_pattern` word-splits before globbing, so a TMP_DIR containing a space would silently skip every file instead of instrumenting it. compgen -G expands the glob pattern as a single argument, avoiding the split. Also documents the concrete failure mode of calling a maybe_* helper outside the inject_trace_calls -> instrument_one_file chain. Impact: 1 functions changed, 5 affected
Summary
Address remaining quality issues from the Titan GAUNTLET audit across ast-analysis, presentation, extractors, cli, and scripts:
src/ast-analysis/visitor-utils.ts,cfg-conditionals.ts— dispatch table + processBranch dedupesrc/presentation/audit.ts— decomposerenderAuditFunction, adopt typedAuditResultexecute()incli/commands/info.tsintoprintEngineInfo/printNativeVersionInfo/printBuildMetadatatimeMedianhelper inscripts/token-benchmark.tsTitan Audit Context
Changes
src/ast-analysis/visitor-utils.ts,src/ast-analysis/visitors/cfg-conditionals.tssrc/features/audit.ts,src/presentation/audit.ts,src/types.tssrc/extractors/{r,dart,groovy,csharp,elixir,scala,julia}.tssrc/cli/commands/info.tsscripts/token-benchmark.tsMetrics Impact
Part of the quality-fix pass addressing GAUNTLET fail-level violations; see
generated/titan/report for full metrics.Test plan