fix: resolve barrel resolution quality and cycle regression#848
fix: resolve barrel resolution quality and cycle regression#848carlos-alm merged 7 commits intomainfrom
Conversation
Extract hardcoded magic numbers to named constants in constants.rs: - Louvain: MAX_LEVELS=50, MAX_PASSES=20, MIN_GAIN=1e-12, DEFAULT_SEED=42 - Dataflow: TRUNCATION_LIMIT=120 - Build pipeline: FAST_PATH_MAX_CHANGED_FILES=5, FAST_PATH_MIN_EXISTING_FILES=20 Also extract DEFAULT_RANDOM_SEED=42 in TS louvain.ts.
|
Claude finished @carlos-alm's task —— View job PR Review CompletePR #848: fix: resolve barrel resolution quality and cycle regression Root Cause AnalysisProblem Being Solved: This PR addresses a +1 function cycle regression (8→9 cycles) introduced during barrel resolution extraction. The root cause was mutual recursion between Why This Approach: The fix eliminates the mutual recursion by inlining Risk Assessment: ✅ LOW RISK - This is a genuine algorithmic fix that removes unnecessary indirection without changing the core logic. Backlog Compliance
Code Quality Assessment✅ Excellent Changes
|
Codegraph Impact Analysis33 functions changed → 65 callers affected across 36 files
|
Greptile SummaryThis PR addresses two independent issues in the native Rust engine: (1) a +1 function-cycle regression (9→8) caused by mutual recursion between Key changes:
The Rust Confidence Score: 5/5Safe to merge — both the cycle regression fix and the barrel-resolution refactor are correct, well-tested, and behaviour-preserving. All changed logic has been verified: the TS cycle fix removes a genuine mutual recursion, the Rust barrel_resolution module faithfully mirrors the TS algorithm with unit tests covering four scenarios, the helpers.rs early-return refactor preserves identical behaviour, and constant extraction is a pure rename. No P0 or P1 findings were identified. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant EB as edge_builder.rs
participant IE as import_edges.rs
participant BR as barrel_resolution.rs
participant TS as resolve-imports.ts
Note over EB,IE: Before PR — duplicate resolve_barrel_export in each file
Note over EB,IE: After PR — shared BarrelContext trait + generic fn
EB->>BR: resolve_barrel_export(ctx, barrel, sym, visited)
IE->>BR: resolve_barrel_export(ctx, barrel, sym, visited)
BR->>BR: recurse (visited guards cycles)
BR-->>EB: Option<String>
BR-->>IE: Option<String>
Note over TS: resolve-imports.ts (cycle fix)
TS->>TS: resolveBarrelExport(ctx, barrel, sym, visited)
TS->>TS: sourceDefinesSymbol(ctx, source, sym)
Note over TS: sourceDefinesSymbol is now a pure lookup — no back-call into resolveBarrelExport
TS-->>TS: string | null
Reviews (2): Last reviewed commit: "fix: address Greptile review feedback on..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Wildcard or empty-names reexports | ||
| if re.wildcard_reexport || re.names.is_empty() { |
There was a problem hiding this comment.
true at this point
After the first if block (lines 49–62) either returns or continues for every non-wildcard named re-export, execution only reaches line 65 when the entry is a wildcard or has empty names — meaning re.wildcard_reexport || re.names.is_empty() is structurally guaranteed to be true here. The guard is harmless, but it creates a false impression that the branch might be skipped and adds a mismatch with the TypeScript counterpart (which has no guard at this point).
Consider removing the redundant condition and keeping the block flat, mirroring the TypeScript:
// Wildcard or empty-names reexports
if ctx.has_definition(re.source, symbol_name) {
return Some(re.source.to_string());
}
let deeper = resolve_barrel_export(ctx, re.source, symbol_name, visited);
if deeper.is_some() {
return deeper;
}There was a problem hiding this comment.
Fixed — removed the redundant if re.wildcard_reexport || re.names.is_empty() guard since execution can only reach that point when the condition is already true. The block is now flat, matching the TypeScript counterpart.
| if let Some(ast_kind) = classify_ast_node(node.kind(), config) { | ||
| let skip_children_on_short_string; | ||
| match ast_kind { | ||
| "new" => { | ||
| ast_nodes.push(build_new_node(node, source)); | ||
| skip_children_on_short_string = false; | ||
| } | ||
| "throw" => { | ||
| ast_nodes.push(build_throw_node(node, source, config)); | ||
| skip_children_on_short_string = false; | ||
| } | ||
| "await" => { | ||
| ast_nodes.push(build_await_node(node, source)); | ||
| skip_children_on_short_string = false; | ||
| } | ||
| "string" => { | ||
| if let Some(ast_node) = build_string_node(node, source, config) { | ||
| ast_nodes.push(ast_node); | ||
| skip_children_on_short_string = false; | ||
| } else { | ||
| // Short string: recurse children then return early | ||
| skip_children_on_short_string = true; | ||
| } | ||
| } | ||
| "regex" => { | ||
| ast_nodes.push(build_regex_node(node, source)); | ||
| skip_children_on_short_string = false; | ||
| } | ||
| _ => { skip_children_on_short_string = false; } | ||
| } | ||
| if skip_children_on_short_string { | ||
| for i in 0..node.child_count() { | ||
| if let Some(child) = node.child(i) { | ||
| walk_ast_nodes_with_config_depth(&child, source, ast_nodes, config, depth + 1); | ||
| } | ||
| } | ||
| return; | ||
| } | ||
| let name = truncate(content, 100); | ||
| let text = truncate(raw, AST_TEXT_MAX); | ||
| ast_nodes.push(AstNode { | ||
| kind: "string".to_string(), | ||
| name, | ||
| line: start_line(node), | ||
| text: Some(text), | ||
| receiver: None, | ||
| }); | ||
| // Fall through to recurse children (template strings may have nested expressions) | ||
| } else if config.regex_types.contains(&kind) { | ||
| let raw = node_text(node, source); | ||
| let name = if raw.is_empty() { "?".to_string() } else { raw.to_string() }; | ||
| let text = truncate(raw, AST_TEXT_MAX); | ||
| ast_nodes.push(AstNode { | ||
| kind: "regex".to_string(), | ||
| name, | ||
| line: start_line(node), | ||
| text: Some(text), | ||
| receiver: None, | ||
| }); | ||
| // Fall through to recurse children | ||
| } |
There was a problem hiding this comment.
skip_children_on_short_string flag creates subtle dual-loop control flow
The boolean flag is set inside match ast_kind then checked immediately afterwards, with a separate outer for loop that always runs when the flag is false. This works correctly, but the interaction is non-obvious: any reader must trace that all non-short-string recognized nodes ("new", "throw", "await", "regex") fall through to the unconditional outer child-recursion loop at lines 542–546.
The short-string case is the only special path — it recurses children inline then returns early to prevent the outer loop from running again. A cleaner pattern would be to return early directly from the match arm and eliminate the flag entirely:
"string" => {
if build_string_node(node, source, config).map(|n| ast_nodes.push(n)).is_none() {
// Short string: recurse children then skip outer loop
for i in 0..node.child_count() {
if let Some(child) = node.child(i) {
walk_ast_nodes_with_config_depth(&child, source, ast_nodes, config, depth + 1);
}
}
return;
}
}This makes the early-return intent explicit without needing the flag, and avoids the reader having to reason about what happens when the flag remains false across both the if check and the outer loop.
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 — eliminated the skip_children_on_short_string flag entirely. The string match arm now returns early directly when build_string_node returns None, making the early-return intent explicit without needing the boolean flag.
The constant is compared against existing_file_count which returns i64 from SQLite. Using usize caused a Rust compile error.
#848) Remove redundant guard in barrel_resolution.rs that was always true at the wildcard/empty-names branch. Eliminate skip_children_on_short_string flag in helpers.rs by using early return directly from the string match arm.
|
Addressed feedback from all reviewers: CI fix: Fixed Rust compile error — Greptile P2 (barrel_resolution.rs:65): Removed the redundant Greptile P2 (helpers.rs:540): Eliminated the Claude condition 1 (engine parity): Both engines share the same barrel resolution algorithm now via the extracted Claude condition 2 ( |
|
Addressed feedback from all reviewers: CI fix: Fixed Rust compile error — FAST_PATH_MIN_EXISTING_FILES was usize but compared against existing_file_count which is i64. Changed the constant type to i64. Greptile P2 (barrel_resolution.rs:65): Removed the redundant guard — execution only reaches that point when the condition is structurally guaranteed true. The block is now flat, matching the TypeScript counterpart. Greptile P2 (helpers.rs:540): Eliminated the skip_children_on_short_string boolean flag. The string match arm now returns early directly when build_string_node returns None. Claude condition 1 (engine parity): Both engines share the same barrel resolution algorithm now via the extracted BarrelContext trait. Claude condition 2 (deeper ?? re.source fallback): The fallback to re.source for named exports is intentional — when a named reexport explicitly lists the symbol, we know the source file is the correct target even if we cannot find the definition (it may be a type-only export or not yet parsed). Returning null would break valid resolution chains. |
|
Claude finished @carlos-alm's task —— View job Re-reviewing PR #848Carlos-alm has addressed previous reviewer feedback. Here's my updated review: Todo List
CI Status ✅CI is currently running (4 workflows in progress) with no failed runs. The previous Rust compile error appears to be resolved. Verified Fixes ✅✅ CI Compile Error Fixed:
✅ Greptile P2 Issue #1 - Redundant Guard Removed:
✅ Greptile P2 Issue #2 - Eliminated
✅ Barrel Resolution Quality Improvement:
✅ Cycle Regression Fix:
Final Assessment ⭐⭐⭐⭐⭐Rating: 5/5 - EXCELLENT Recommendation: APPROVE - All issues have been properly addressed Why This is Now Excellent:
Impact: This directly improves codegraph's accuracy in cycle detection, helping AI agents avoid false positives when analyzing blast radius. The shared implementation also ensures consistent behavior across engines. Carlos-alm has demonstrated excellent responsiveness to feedback and thorough implementation of the requested changes. This PR is ready for merge. |
1 participant
Summary
Titan Audit Context
Changes
crates/codegraph-core/src/extractors/helpers.rs-- flatten walk_ast_nodes_with_config_depth, extract depth trackingsrc/domain/graph/builder/stages/resolve-imports.ts-- fix barrel resolution mutual recursion causing +1 cycle, improve qualityMetrics Impact
Test plan