fix: address quality issues in resolver, search, graph model, and features#1789
Merged
carlos-alm merged 27 commits intoJul 5, 2026
Merged
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
Contributor
Greptile SummaryThis PR is a large-scale quality refactoring across the resolver, search, graph model, and features domains, driven by a Titan GAUNTLET audit. The primary correctness fix is the
Confidence Score: 5/5Safe to merge — all changes are structural refactoring or targeted bug fixes with no logic regressions identified. The merge() aliasing fix in model.ts is correct and necessary. The Vitest mock update in complexity.test.ts is required for the new DEFAULTS reference not to throw. All other changes extract existing logic into named helpers without altering semantics: cascade order in the resolver is preserved, try/catch placement in branch-compare is improved, and co-change defaults now come from a single authoritative source rather than duplicated magic values. No files require special attention. The most semantically significant change (model.ts merge fix) is straightforward and well-isolated. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph resolver["Domain: Graph Resolver"]
PT["points-to.ts"]
ST["strategy.ts"]
TS["ts-resolver.ts"]
end
subgraph search["Domain: Search"]
PR["prepare.ts"]
end
subgraph model["Graph Model"]
GM["model.ts - merge() aliasing fix"]
end
subgraph leiden["Leiden Algorithm"]
AD["adapter.ts"]
PA["partition.ts"]
end
subgraph features["Features"]
BC["branch-compare.ts"]
CC["cochange.ts"]
CQ["complexity-query.ts"]
end
subgraph tests["Tests"]
TT["complexity.test.ts"]
end
CC --> CQ
CQ --> TT
%%{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 resolver["Domain: Graph Resolver"]
PT["points-to.ts"]
ST["strategy.ts"]
TS["ts-resolver.ts"]
end
subgraph search["Domain: Search"]
PR["prepare.ts"]
end
subgraph model["Graph Model"]
GM["model.ts - merge() aliasing fix"]
end
subgraph leiden["Leiden Algorithm"]
AD["adapter.ts"]
PA["partition.ts"]
end
subgraph features["Features"]
BC["branch-compare.ts"]
CC["cochange.ts"]
CQ["complexity-query.ts"]
end
subgraph tests["Tests"]
TT["complexity.test.ts"]
end
CC --> CQ
CQ --> TT
Reviews (4): 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}
Comment on lines
683
to
705
| const changedFiles = getChangedFilesBetweenRefs(repoRoot, baseSha, targetSha); | ||
|
|
||
| if (changedFiles.length === 0) { | ||
| return emptyBranchCompareResult(baseRef, targetRef, baseSha, targetSha); | ||
| } | ||
|
|
||
| const { tmpBase, baseDir, targetDir } = createCompareTempDirs(); | ||
|
|
||
| try { | ||
| return await runBranchCompareInWorktrees( | ||
| { baseRef, targetRef, baseSha, targetSha }, | ||
| { repoRoot, baseDir, targetDir, engine }, | ||
| changedFiles, | ||
| noTests, | ||
| maxDepth, | ||
| ); | ||
| } catch (err) { | ||
| return { error: toErrorMessage(err) }; | ||
| } finally { | ||
| removeWorktree(repoRoot, baseDir); | ||
| removeWorktree(repoRoot, targetDir); | ||
| try { | ||
| fs.rmSync(tmpBase, { recursive: true, force: true }); | ||
| } catch (cleanupErr) { | ||
| debug(`branchCompareData: temp cleanup failed: ${toErrorMessage(cleanupErr)}`); | ||
| } | ||
| cleanupCompareTempDirs(repoRoot, baseDir, targetDir, tmpBase); | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
getChangedFilesBetweenRefs moved outside the error boundary
getChangedFilesBetweenRefs calls execFileSync('git', ['diff', '--name-only', ...]) without any try/catch, and is now invoked before the try { … } catch (err) { return { error: toErrorMessage(err) } } block. In the original code, the entire body was wrapped in that handler, so any execFileSync failure was converted to { error: string } without raising. After the refactor, a git error at this step (e.g. corrupted object, hook failure, or signal) will instead throw an unhandled rejection from the otherwise-infallible branchCompareData function. createCompareTempDirs is similarly outside the catch block, though that failure path is far less likely.
Contributor
Author
There was a problem hiding this comment.
Fixed in 4a7f106 — wrapped getChangedFilesBetweenRefs and createCompareTempDirs inside branchCompareData's try/catch, with a nested try/finally around the worktree-dependent work so cleanup only runs once temp dirs actually exist. All failure paths from the git-diff step (or mkdtempSync) now return { error: string } instead of propagating as an unhandled rejection.
Comment on lines
+86
to
+90
| const fc = buildFileConditionSQL(fpArr, 'n.file'); | ||
| if (fc.sql) { | ||
| // Strip leading ' AND ' since we're using conditions array | ||
| conditions.push(fc.sql.replace(/^ AND /, '')); | ||
| params.push(...fc.params); |
Contributor
There was a problem hiding this comment.
/^ AND / strips " AND " from buildFileConditionSQL's output before pushing into conditions. This is correct today because the helper always prefixes with AND, but the coupling is invisible — if the helper's contract ever changes, this will silently produce malformed SQL. A small inline comment makes the invariant explicit.
Suggested change
| const fc = buildFileConditionSQL(fpArr, 'n.file'); | |
| if (fc.sql) { | |
| // Strip leading ' AND ' since we're using conditions array | |
| conditions.push(fc.sql.replace(/^ AND /, '')); | |
| params.push(...fc.params); | |
| const fc = buildFileConditionSQL(fpArr, 'n.file'); | |
| if (fc.sql) { | |
| // buildFileConditionSQL always prefixes with ' AND '; strip it since | |
| // we accumulate fragments in a conditions[] array joined below. | |
| conditions.push(fc.sql.replace(/^ AND /, '')); | |
| params.push(...fc.params); |
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!
Contributor
Author
There was a problem hiding this comment.
Fixed in 320d6e9 — expanded the comment to spell out the invariant (buildFileConditionSQL always prefixes with ' AND ', see src/db/query-builder.ts) and why the strip+accumulate pattern is safe.
Contributor
Codegraph Impact Analysis161 functions changed → 109 callers affected across 35 files
|
…ndary (#1789) git diff-tree failures at this step previously escaped as unhandled rejections instead of the { error: string } shape callers expect. Impact: 1 functions changed, 3 affected
…Search (#1789) Makes explicit why the ' AND ' prefix strip is safe, so a future change to buildFileConditionSQL's output shape doesn't silently break the conditions[] accumulation here. Impact: 1 functions changed, 8 affected
Contributor
Author
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Address quality issues (complexity, DRY, error handling) flagged by the Titan GAUNTLET audit across the resolver, search, graph model, and features domains:
src/domain/graph/resolver/{points-to,ts-resolver,strategy}.tssrc/domain/search/search/prepare.ts— adopt existingbuildFileConditionSQLhelper, move aconsole.logout of the domain layersrc/graph/model.ts+ leidenpartition.ts/adapter.ts— fixmerge()aliasing bug, reduce leiden complexitysrc/features/{complexity-query,cochange,branch-compare}.tsTitan Audit Context
Changes
src/domain/graph/resolver/{points-to,ts-resolver,strategy}.tssrc/domain/search/search/prepare.tssrc/graph/model.ts,src/graph/algorithms/leiden/{partition,adapter,index}.tssrc/features/complexity-query.ts,tests/integration/complexity.test.tssrc/features/cochange.tssrc/features/branch-compare.tsMetrics Impact
branchCompareDataandcomplexityDatawere both on the Titan RECON worst-Maintainability-Index list; see the report's "Complexity Improvement: Top Movers" table for exact before/after numbers.Test plan