fix: normalize paths in native resolver for .js → .ts remap#600
fix: normalize paths in native resolver for .js → .ts remap#600carlos-alm merged 6 commits intomainfrom
Conversation
Adds clean_path() to properly resolve .. components before extension probing. Also updates normalize_path() to use clean_path() internally. The JS-side defense-in-depth (path.normalize + remapJsToTs) was already in place; this fixes the root cause in the Rust native resolver. Fixes #592 Impact: 3 functions changed, 6 affected
|
Claude finished @carlos-alm's task —— View job Reviewing PR: normalize paths in native resolver for .js → .ts remap Review Progress
Root Cause AnalysisProblem Being Solved: Native Rust resolver fails to handle relative imports containing Why This Approach: This is the correct fix targeting the root cause. The PR adds proper path normalization via Risk Assessment: ✅ Low risk - This fixes a legitimate bug and brings native resolver behavior in line with JavaScript implementation. There's already defensive code on the JS side as fallback. Backlog Compliance
Code Quality Assessment✅ Strengths:
🔍 Minor Observations:
Test Coverage NoteThere's a relevant test already written but skipped ( Final AssessmentConditions for approval:
This is a high-quality fix that addresses the exact issue described in the skipped test case. The implementation is correct and follows Rust best practices. |
Greptile SummaryThis PR fixes a silent failure in the native Rust import resolver where relative imports containing Key observations:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[resolve_import_path_inner] --> B{starts with '.'}
B -- No --> C[alias resolution / bare specifier]
B -- Yes --> D["dir.join(import_source)"]
D --> E["clean_path() — NEW\ncollapses .. and . components"]
E --> F[resolved_str with forward slashes]
F --> G{ends with .js?}
G -- Yes --> H["replace .js → .ts\ncheck file_exists"]
H --> I{strip_prefix root OK?}
I -- Yes --> J[return normalize_path]
I -- No --> K["replace .js → .tsx\ncheck file_exists"]
K --> L{strip_prefix root OK?}
L -- Yes --> J
L -- No --> M[extension probing loop]
G -- No --> M
M --> N{candidate exists?}
N -- Yes --> O{strip_prefix root OK?}
O -- Yes --> J
O -- No --> P[next extension]
N -- No --> P
P --> M
M -- exhausted --> Q{exact match?}
Q -- Yes --> J
Q -- No --> R[fallback: strip_prefix or normalized string]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn clean_path(p: &Path) -> PathBuf { | ||
| let mut result = PathBuf::new(); | ||
| for c in p.components() { | ||
| match c { | ||
| std::path::Component::ParentDir => { | ||
| result.pop(); | ||
| } | ||
| std::path::Component::CurDir => {} | ||
| _ => result.push(c), | ||
| } | ||
| } | ||
| result | ||
| } |
There was a problem hiding this comment.
clean_path
The project has #[cfg(test)] modules in complexity.rs, cycles.rs, and several extractors, but import_resolution.rs has none. The clean_path function contains path-normalization logic that has known edge-case subtleties worth pinning with tests — especially as a regression guard for the bug this PR fixes.
At minimum, a #[cfg(test)] block with cases like these would give confidence:
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn clean_path_collapses_parent_dirs() {
assert_eq!(
clean_path(Path::new("src/cli/commands/../../domain/graph/builder.js")),
PathBuf::from("src/domain/graph/builder.js")
);
}
#[test]
fn clean_path_skips_cur_dir() {
assert_eq!(
clean_path(Path::new("src/./foo.ts")),
PathBuf::from("src/foo.ts")
);
}
#[test]
fn clean_path_handles_absolute_root() {
assert_eq!(
clean_path(Path::new("/src/../foo.ts")),
PathBuf::from("/foo.ts")
);
}
}Without a regression test for the .js → .ts remap across ../ segments, the specific fix from issue #592 can silently break again in future refactors.
There was a problem hiding this comment.
Addressed. Added a #[cfg(test)] module with five unit tests covering: parent dir collapse, cur dir skip, absolute root, mixed segments, and the excess-parent-dir silent-drop edge case. Also un-skipped the resolves parent directory traversal parity test in tests/resolution/parity.test.ts so the specific .js to .ts remap regression from #592 is covered in CI.
| fn clean_path(p: &Path) -> PathBuf { | ||
| let mut result = PathBuf::new(); | ||
| for c in p.components() { | ||
| match c { | ||
| std::path::Component::ParentDir => { | ||
| result.pop(); | ||
| } | ||
| std::path::Component::CurDir => {} | ||
| _ => result.push(c), | ||
| } | ||
| } | ||
| result | ||
| } |
There was a problem hiding this comment.
.. components are silently dropped on an empty base
When result is an empty PathBuf and a ParentDir component is encountered, PathBuf::pop() is a no-op (it returns false and leaves the path unchanged). This means a call like clean_path(Path::new("../../foo")) yields foo rather than ../../foo.
This diverges from path.normalize in Node.js, which preserves leading .. segments for relative paths. In the current call sites the difference doesn't matter — normalize_path is always called on root-relative paths after strip_prefix, and clean_path in resolve_import_path_inner is always called on dir.join(import_source) where dir carries enough depth to absorb the .. pops. However, it is worth documenting this limitation explicitly in the doc comment to prevent future callers from relying on it for pure relative paths:
| fn clean_path(p: &Path) -> PathBuf { | |
| let mut result = PathBuf::new(); | |
| for c in p.components() { | |
| match c { | |
| std::path::Component::ParentDir => { | |
| result.pop(); | |
| } | |
| std::path::Component::CurDir => {} | |
| _ => result.push(c), | |
| } | |
| } | |
| result | |
| } | |
| /// Resolve `.` and `..` components in a path without touching the filesystem. | |
| /// Unlike `PathBuf::components().collect()`, this properly collapses `..` by | |
| /// popping the previous component from the result. | |
| /// | |
| /// NOTE: if the path begins with more `..` components than there are preceding | |
| /// components to pop (e.g. a purely relative `../../foo`), the excess `..` | |
| /// components are silently dropped. This function is therefore only correct | |
| /// when called on paths that have already been joined to a base directory with | |
| /// sufficient depth. | |
| fn clean_path(p: &Path) -> PathBuf { |
There was a problem hiding this comment.
Addressed. Added the documented limitation to the clean_path doc comment, noting that excess leading .. components on a path with insufficient depth are silently dropped, and that the function is only correct when called on paths already joined to a base directory with sufficient depth.
- Add #[cfg(test)] module with unit tests for clean_path covering parent dir collapse, cur dir skip, absolute root, mixed segments, and the known excess-parent-dir limitation. - Document the silent-drop behavior of leading .. on empty base in the clean_path doc comment. - Un-skip the 'resolves parent directory traversal' parity test now that the native engine properly normalizes paths. Impact: 5 functions changed, 0 affected
…zation' into fix/native-resolver-path-normalization-review Impact: 5 functions changed, 14 affected
The parity test requires the native binary to include the clean_path fix, but CI tests run against the pre-built binary. Re-skip with an updated comment explaining when to un-skip.
|
Addressed both Greptile comments and the Claude review feedback:
|
|
Addressed the coverage gap for the exact #592 regression scenario (.js → .ts remap across
|
1 participant
Summary
clean_path()helper inimport_resolution.rsto properly normalize..and.path components by popping parent directories instead of collecting them verbatimstrip_prefixfailures that caused .js → .ts extension remapping to silently fail when import paths contained../segments (e.g.src/cli/commands/../../domain/graph/builder.js)normalize_path()to use the newclean_path()internally, fixing the same issue for all path normalization in the native resolverpath.normalize+remapJsToTs) was already in place; this fixes the root cause in the Rust native resolverFixes #592
Test plan
../codegraph build . --engine nativeon this repo and check edge counts