fix: replace fixed-depth directory-proximity check with symmetric distance#1894
Conversation
…tance (docs check acknowledged) computeConfidence (and its Rust mirror compute_confidence) scored call-edge resolution confidence using a fixed-depth check: same-directory, or dirname(dirname(caller)) === dirname(dirname(target)) for a "sibling directory" tier. That equality only holds when both files sit at the same depth, so a file in a subdirectory calling a method on a class declared in its direct parent directory (e.g. graph/algorithms/bfs.ts calling a method on the CodeGraph class in graph/model.ts) was scored as maximally distant (0.3) even though the receiver's type had already been correctly resolved via typeMap (from the parameter's `graph: CodeGraph` type annotation). That score fell below the 0.5 threshold used by resolveViaTypedMethod, silently dropping the call edge. Replaced both fixed-depth checks with a symmetric, depth-independent directory-tree distance (hops to the nearest common ancestor), preserving the existing same-directory (0.7) and sibling (0.5) tiers exactly and adding a new tier for direct parent/child directory nesting (0.6). Verified via the resolution-benchmark suite before/after the fix: all 42 language fixtures produce byte-identical precision/recall/TP/FP/FN numbers (aggregate recall 63.8%, 355/556 edges) — no regression. Fixes #1769 Impact: 3 functions changed, 26 affected
Greptile SummaryThis PR fixes a silent call-edge drop caused by
Confidence Score: 4/5The change is narrowly scoped to the confidence-scoring helper in both engines and is thoroughly regression-tested; the only concern is a quadratic traversal on the hot path that is harmless at current repository depths. Both implementations of directoryDistance / directory_distance use a linear indexOf / position scan inside a loop over the ancestor chain, making the function O(n^2) where n is the directory depth. For paths up to ~20 levels deep this amounts to at most ~400 comparisons per call-edge candidate and will not be observable in practice. The logic itself is correct, symmetric, and the existing tier boundaries (0.7 / 0.5 / 0.3) are fully preserved; only the new 0.6 parent/child tier is added. No files require special attention. Both resolve.ts and resolve.rs carry the same O(n^2) traversal pattern that could be addressed with a pre-built lookup map. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["computeConfidenceJS / compute_confidence"] --> B{importedFrom == targetFile?}
B -- yes --> C[return 1.0]
B -- no --> D{workspace resolved?}
D -- yes --> E[return 0.95]
D -- no --> F["directoryDistance(dirname(callerFile), dirname(targetFile))"]
F --> G["ancestorChain(a): walk up from dir to root"]
F --> H["ancestorChain(b): walk up from dir to root"]
G & H --> I{LCA hop count}
I -- dist == 0 --> J["0.7 same directory"]
I -- dist == 1 --> K["0.6 direct parent/child NEW"]
I -- dist == 2 --> L["0.5 siblings / grandparent-grandchild"]
I -- dist >= 3 --> M["0.3 unrelated subtrees"]
%%{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
A["computeConfidenceJS / compute_confidence"] --> B{importedFrom == targetFile?}
B -- yes --> C[return 1.0]
B -- no --> D{workspace resolved?}
D -- yes --> E[return 0.95]
D -- no --> F["directoryDistance(dirname(callerFile), dirname(targetFile))"]
F --> G["ancestorChain(a): walk up from dir to root"]
F --> H["ancestorChain(b): walk up from dir to root"]
G & H --> I{LCA hop count}
I -- dist == 0 --> J["0.7 same directory"]
I -- dist == 1 --> K["0.6 direct parent/child NEW"]
I -- dist == 2 --> L["0.5 siblings / grandparent-grandchild"]
I -- dist >= 3 --> M["0.3 unrelated subtrees"]
Reviews (1): Last reviewed commit: "fix: replace fixed-depth directory-proxi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function directoryDistance(a: string, b: string): number { | ||
| const chainA = ancestorChain(a); | ||
| const chainB = ancestorChain(b); | ||
| for (let i = 0; i < chainA.length; i++) { | ||
| const j = chainB.indexOf(chainA[i]!); | ||
| if (j !== -1) return i + j; | ||
| } | ||
| return Infinity; | ||
| } |
There was a problem hiding this comment.
directoryDistance scans chainB linearly (indexOf) for every element of chainA, giving O(|chainA| × |chainB|) string comparisons. A single pre-built Map<string, number> from directory to its index in chainB would reduce this to O(|chainA| + |chainB|). At typical repository path depths (< 20) the difference is invisible, but this function is on the hot path for every call-edge resolution candidate.
| function directoryDistance(a: string, b: string): number { | |
| const chainA = ancestorChain(a); | |
| const chainB = ancestorChain(b); | |
| for (let i = 0; i < chainA.length; i++) { | |
| const j = chainB.indexOf(chainA[i]!); | |
| if (j !== -1) return i + j; | |
| } | |
| return Infinity; | |
| } | |
| function directoryDistance(a: string, b: string): number { | |
| const chainA = ancestorChain(a); | |
| const chainB = ancestorChain(b); | |
| const indexInB = new Map<string, number>(chainB.map((d, idx) => [d, idx])); | |
| for (let i = 0; i < chainA.length; i++) { | |
| const j = indexInB.get(chainA[i]!); | |
| if (j !== undefined) return i + j; | |
| } | |
| return Infinity; | |
| } |
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!
| fn directory_distance(a: &str, b: &str) -> usize { | ||
| let chain_a = ancestor_chain(a); | ||
| let chain_b = ancestor_chain(b); | ||
| for (i, dir_a) in chain_a.iter().enumerate() { | ||
| if let Some(j) = chain_b.iter().position(|dir_b| dir_b == dir_a) { | ||
| return i + j; | ||
| } | ||
| } | ||
| usize::MAX | ||
| } |
There was a problem hiding this comment.
chain_b.iter().position(...) does a linear scan for each element of chain_a. Collecting chain_b into a HashMap<&str, usize> first would bring this to O(n). At typical path depths the cost is negligible, but the Rust engine also runs this on the rayon-parallelised hot path.
| fn directory_distance(a: &str, b: &str) -> usize { | |
| let chain_a = ancestor_chain(a); | |
| let chain_b = ancestor_chain(b); | |
| for (i, dir_a) in chain_a.iter().enumerate() { | |
| if let Some(j) = chain_b.iter().position(|dir_b| dir_b == dir_a) { | |
| return i + j; | |
| } | |
| } | |
| usize::MAX | |
| } | |
| fn directory_distance(a: &str, b: &str) -> usize { | |
| let chain_a = ancestor_chain(a); | |
| let chain_b = ancestor_chain(b); | |
| let index_in_b: std::collections::HashMap<&str, usize> = | |
| chain_b.iter().enumerate().map(|(j, d)| (d.as_str(), j)).collect(); | |
| for (i, dir_a) in chain_a.iter().enumerate() { | |
| if let Some(&j) = index_in_b.get(dir_a.as_str()) { | |
| return i + j; | |
| } | |
| } | |
| usize::MAX | |
| } |
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!
Codegraph Impact Analysis3 functions changed → 26 callers affected across 6 files
|
1 participant
Summary
typeMap['graph'] = {type: 'CodeGraph', confidence: 1}forgraph: CodeGraphparameters.resolveViaTypedMethodcorrectly findsCodeGraph.successorsby name, butcomputeConfidence/compute_confidencescored directory proximity using a fixed-depth equality check (dirname(dirname(caller)) === dirname(dirname(target))for the 0.5 "same grandparent" tier). This only holds when both files sit at the same depth — for callersrc/graph/algorithms/bfs.tsvs. targetsrc/graph/model.ts(one level deeper), the check failed and confidence fell to 0.3, below the resolver's 0.5 threshold, silently dropping a correctly-typed call edge.Closes #1769
Test plan
npm test— full suite green (3549 passed, 0 failed)npm run lint— cleancargo test— 457 passed,cargo check --workspacecleanfn-impact "CodeGraph.successors"now shows 7 real dependents (rolecore);roles --role dead --file src/graph/model.tsdropped from 11 to 5 dead-unresolvedPartial fix, honestly scoped: 5 of the original 11 flagged methods remain dead-unresolved, but investigation showed these are separate, pre-existing, codebase-wide gaps unrelated to this bug:
getEdgeAttrsgenuinely has zero non-test callers (correct classification);CodeGraph.constructor—new X()resolves to the class node for every class in the codebase, filed as #1892; ES6 getter property-reads are never extracted as call edges anywhere (documented as "Phase 2 reserved" inshared/kinds.ts), filed as #1893.Stacked on #1891 (base branch
fix/issue-1768-build-edges-technique-stmt-cache) — only the resolve.ts/resolve.rs/test diff is this issue's change.