refactor: adopt dead helpers identified by Titan grind#1793
Conversation
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
…ing (docs check acknowledged) Impact: 12 functions changed, 36 affected
…and modularity.ts Forge phase 3 extracted fget/iget/u8get/taAdd into typed-array-helpers.ts from adapter.ts/index.ts/partition.ts, but two sibling leiden files — cpm.ts and modularity.ts — carried byte-for-byte identical private copies of fget/iget (down to the "see adapter.ts for rationale" comment) that were never migrated to the shared module. Codebase-wide grep/symbol scans found no other duplicates of this pattern outside the leiden directory. Both functions were unexported, file-local helpers with zero external callers (codegraph exports confirms neither appears in either file's export list), so replacing the private declarations with an import from the shared module is a pure dedup with no behavioral change. docs check acknowledged: internal helper adoption within an already-vendored algorithm module, no user-facing feature/language/architecture-table changes.
…actor Forge phase 7 (8fed8bc) added these as reserved DEFAULTS entries but left their consumers on hardcoded literals. db.busyTimeoutMs: openDb and openReadonlyOrFail in src/db/connection.ts now take an optional busyTimeoutMs parameter defaulting to DEFAULTS.db.busyTimeoutMs. Build and watch call sites that already hold a resolved config (pipeline, native orchestrator, native-db-lifecycle, watcher, openRepo and openReadonlyWithNative via the renamed resolveDbSettings helper) pass the user-configured value through explicitly. This partially addresses issue 1749, whose busy-locked regex dedup half remains open; remaining read-only query call sites and the Rust connection.rs mirror are tracked in issue 1763. community.capacityGrowthFactor is threaded through the existing maxLevels and refinementTheta plumbing, from communitiesData through louvainCommunities and detectClusters into makePartition, so ensureCommCapacity in src/graph/algorithms/leiden/partition.ts reads the configured value instead of a hardcoded 1.5 growth multiplier. It is ignored by the native Rust Louvain path, consistent with the other options. largeCodebaseFileThreshold, the third reserved entry from phase 7, was already wired by a later forge commit and needed no action. docs check acknowledged: internal config wiring only, already documented in docs/guides/configuration.md by forge phase 7. Impact: 31 functions changed, 171 affected
…icate sites handleMethodCapture, handleMethodDef, and extractObjectLiteralFunctions each inlined the identical computed_property_name unwrap-and-strip-quotes logic that forge phase 10 already extracted into resolveMethodDefinitionName for pushMethodDefContext. Consolidate onto the shared helper; no behavior change. docs check acknowledged: internal-only duplicate-code consolidation, no new languages/commands/architecture; README.md, CLAUDE.md, and ROADMAP.md are unaffected. Impact: 3 functions changed, 11 affected
Phase 11 decomposed build-edges.ts's resolveFallbackTargets into 15 named
helpers. The codebase-wide duplicate scan on those new helpers surfaced two
pre-existing duplicates the decomposition didn't catch (both predate phase 11
temporally, so the consolidation opportunities weren't nameable until now):
- resolveReflectionKeyExprFallback and resolveDefinePropertyAccessorFallback
each inlined the same string/{type} typeMap-entry unwrap ternary that
strategy.ts's unwrapTypeEntry already implements (extracted one commit
after phase 11, in forge phase 20). Promoted unwrapTypeEntry to an export
and wired both call sites to it.
- build-edges.ts's newly-named resolveSameClassQualifiedMethod was
byte-identical to incremental.ts's pre-existing resolveThisSameClassTarget
(incremental.ts's own docstring already noted it mirrors "the full-build
counterpart"). Moved the shared implementation into call-resolver.ts --
the module whose stated purpose is holding call-resolution logic shared
between build-edges.ts and incremental.ts exactly once -- and wired both
consumers to it.
Zero behavior change: all replaced logic was verified byte-identical before
relocating. Resolution benchmark (206/206) and full suite (3340 tests) pass
identically before/after.
Two follow-on divergences the scan also surfaced between the full-build and
incremental resolution paths (not fixed here -- would change behavior, not
just relocate it): incremental.ts has no same-class bare-call fallback
(#1765), and the Object.defineProperty same-file fallback's kind-filter
differs between the two paths (#1766).
docs check acknowledged: pure internal dedup/extract-method refactor, no
new features, commands, languages, or architecture changes -- README/CLAUDE/
ROADMAP do not need updates.
Impact: 5 functions changed, 20 affected
…docs check acknowledged) insertDefinitionsAndExports (insert-nodes.ts) and insertBackfilledNodes (native-orchestrator.ts) duplicated the exact same chunked-UPDATE loop for marking exported symbols -- the latter's own comment even said "mirrors insertDefinitionsAndExports". Extract markExportedSymbols(db, exportKeys) in builder/helpers.ts, built on the existing getOrCreateBatchStmt/runBatchInsert primitives from the batch-insert dedupe, and wire both call sites to it. Impact: 4 functions changed, 20 affected
…-timing lib Forge phase 28 extracted timeMedian in token-benchmark.ts, already wired within that file. The codebase-wide duplicate scan found median()/round1() duplicated verbatim across query-benchmark.ts, incremental-benchmark.ts (twice), and benchmark.ts, plus the "loop N times, time each run, median()" pattern repeated 8 more times across those files. Promotes median/round1/timeMedian into scripts/lib/bench-timing.ts (matching the existing bench-config.ts/fork-engine.ts shared-module pattern) and wires median/round1 into all 4 scripts. Adopts timeMedian at the 3 call sites that were already unconditionally async with no new async boundary (incremental-benchmark.ts fullBuildMs/noopRebuildMs worker, benchmark.ts noopRebuildMs worker). Left 5 remaining duplicate sites unconverted since adopting timeMedian there requires making currently-synchronous functions async, which is a control-flow change rather than a mechanical adoption -- tracked in #1774. Impact: 4 functions changed, 14 affected
Greptile SummaryThis PR wires previously-extracted-but-unused helpers into real call sites across 25 files, reducing duplication without altering observable behaviour. All changes are mechanical adoptions: promoting shared helpers (
Confidence Score: 5/5Safe to merge — all changes are mechanical deduplication of helpers that were already proven correct at their original sites. Every adoption is a like-for-like substitution: the extracted functions reproduce the exact same logic as the inlined code they replace. The busyTimeoutMs threading correctly defaults to DEFAULTS.db.busyTimeoutMs at both the parameter level and the resolveDbSettings fallback. The resolveMethodDefinitionName consolidation preserves the per-site early-exit variant (return vs continue). The markExportedSymbols refactor upgrades from a per-call Map to a module-level WeakMap cache, which is strictly better. No logic changes were made — only consolidation of duplicates. No files require special attention beyond the minor style inconsistency in scripts/lib/bench-timing.ts (missing explicit performance import). Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CD as communitiesData()
participant LC as louvainCommunities()
participant LJ as louvainJS() / detectClusters()
participant MP as makePartition()
participant EC as ensureCommCapacity()
CD->>LC: capacityGrowthFactor (opts ?? config)
LC->>LJ: "capacityGrowthFactor (if != null)"
LJ->>LJ: normalizeOptions() → capacityGrowthFactor (or DEFAULT_CAPACITY_GROWTH_FACTOR)
LJ->>MP: "makePartition(graph, { capacityGrowthFactor })"
MP->>EC: s.capacityGrowthFactor stored in PartitionState
EC->>EC: "Math.ceil(length * s.capacityGrowthFactor) — previously hardcoded 1.5"
%%{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"}}}%%
sequenceDiagram
participant CD as communitiesData()
participant LC as louvainCommunities()
participant LJ as louvainJS() / detectClusters()
participant MP as makePartition()
participant EC as ensureCommCapacity()
CD->>LC: capacityGrowthFactor (opts ?? config)
LC->>LJ: "capacityGrowthFactor (if != null)"
LJ->>LJ: normalizeOptions() → capacityGrowthFactor (or DEFAULT_CAPACITY_GROWTH_FACTOR)
LJ->>MP: "makePartition(graph, { capacityGrowthFactor })"
MP->>EC: s.capacityGrowthFactor stored in PartitionState
EC->>EC: "Math.ceil(length * s.capacityGrowthFactor) — previously hardcoded 1.5"
Reviews (3): Last reviewed commit: "fix(leiden): dedupe DEFAULT_CAPACITY_GRO..." | Re-trigger Greptile |
| // via .codegraphrc.json. Callers (e.g. louvain.ts) can pass overrides through options. | ||
| const DEFAULT_MAX_LEVELS: number = 50; | ||
| const DEFAULT_MAX_LOCAL_PASSES: number = 20; | ||
| const DEFAULT_CAPACITY_GROWTH_FACTOR: number = 1.5; |
There was a problem hiding this comment.
Three copies of the same
1.5 default now exist independently: DEFAULT_CAPACITY_GROWTH_FACTOR here, DEFAULT_CAPACITY_GROWTH_FACTOR in partition.ts, and DEFAULTS.community.capacityGrowthFactor in config.ts. The value in optimiser.ts normalises the option before it reaches makePartition, but partition.ts also keeps its own copy as a last-resort fallback for callers that skip the optimiser layer. If the tuning value ever changes, all three sites must be updated atomically — any miss silently produces mismatched growth behaviour depending on which code path ran. Importing the constant from one canonical location would close that gap.
| const DEFAULT_CAPACITY_GROWTH_FACTOR: number = 1.5; | |
| import { DEFAULT_CAPACITY_GROWTH_FACTOR } from './partition.js'; |
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 — exported DEFAULT_CAPACITY_GROWTH_FACTOR from partition.ts and imported it in optimiser.ts instead of keeping an independent literal. The third copy (DEFAULTS.community.capacityGrowthFactor in config.ts) is intentionally kept separate — it follows the same established pattern as DEFAULT_MAX_LEVELS/DEFAULT_MAX_LOCAL_PASSES in this file (module-level fallback mirrored from the user-facing config default, documented via comment), so no further action needed there.
Codegraph Impact Analysis47 functions changed → 231 callers affected across 114 files
|
…ledged) optimiser.ts kept its own copy of the 1.5 fallback instead of importing partition.ts's constant, so the two could silently drift apart. Export it from partition.ts and import it in optimiser.ts. Addresses Greptile review feedback on #1793.
|
Addressed Greptile's review feedback: exported |
Summary
Adopt dead helpers identified by the Titan GRIND phase — wiring previously-extracted-but-unused helpers into real call sites, and promoting/deduping a few forge-phase helpers that turned out to have further adoption opportunities:
fget/igetfromtyped-array-helpers.tsincpm.ts/modularity.ts(leiden).db.busyTimeoutMsandcommunity.capacityGrowthFactorconfig constants through their real call sites (db/connection, builder pipeline, watcher, communities, leiden).resolveMethodDefinitionNameacross 3 duplicate call sites inextractors/javascript.ts.unwrapTypeEntry,resolveSameClassQualifiedMethod) acrossbuild-edges.ts/incremental.ts.markExportedSymbols, dedupe a batch UPDATE in builder helpers.timeMedian/median/round1into a sharedscripts/lib/bench-timing.ts, deduping the same logic across 3 benchmark scripts.Titan Audit Context
Changes
src/graph/algorithms/leiden/{cpm,modularity}.tssrc/db/connection.ts,src/domain/graph/builder/{pipeline,watcher}...,src/domain/graph/builder/stages/{native-db-lifecycle,native-orchestrator}.ts,src/features/communities.ts,src/graph/algorithms/leiden/{optimiser,partition}.ts,src/graph/algorithms/louvain.ts,src/infrastructure/config.ts,src/types.tssrc/extractors/javascript.tssrc/domain/graph/builder/{call-resolver,incremental}.ts,src/domain/graph/builder/stages/build-edges.ts,src/domain/graph/resolver/strategy.tssrc/domain/graph/builder/{helpers,stages/insert-nodes,stages/native-orchestrator}.tsscripts/{benchmark,incremental-benchmark,query-benchmark,token-benchmark}.ts, newscripts/lib/bench-timing.tsMetrics Impact
The Titan GRIND phase triaged 24 candidate dead-symbol targets: 6 were genuinely adoptable (this PR), 3 were promoted/deduped further (also this PR), and the remaining 15 were confirmed false positives caused by known codegraph role-classifier bugs (documented in
generated/titan/report and filed as GitHub issues).Test plan