Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 71 additions & 2 deletions crates/codegraph-core/src/import_resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,33 @@ fn file_exists(path: &str, known: Option<&HashSet<String>>) -> bool {
known.map_or_else(|| Path::new(path).exists(), |set| set.contains(path))
}

/// 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 {
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
}
Comment on lines +22 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 No unit tests for 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +22 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Leading .. 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:

Suggested change
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 {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// Normalize a path to use forward slashes and clean `.` / `..` segments
/// (cross-platform consistency).
fn normalize_path(p: &str) -> String {
let cleaned: PathBuf = Path::new(p).components().collect();
let cleaned = clean_path(Path::new(p));
cleaned.display().to_string().replace('\\', "/")
}

Expand Down Expand Up @@ -111,7 +134,7 @@ fn resolve_import_path_inner(

// Relative import — normalize immediately to remove `.` / `..` segments
let dir = Path::new(from_file).parent().unwrap_or(Path::new(""));
let resolved: PathBuf = dir.join(import_source).components().collect();
let resolved = clean_path(&dir.join(import_source));
let resolved_str = resolved.display().to_string().replace('\\', "/");

// .js → .ts remap
Expand Down Expand Up @@ -246,3 +269,49 @@ pub fn resolve_imports_batch(
})
.collect()
}

#[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")
);
}

#[test]
fn clean_path_mixed_segments() {
assert_eq!(
clean_path(Path::new("a/b/../c/./d/../e.js")),
PathBuf::from("a/c/e.js")
);
}

#[test]
fn clean_path_excess_parent_dirs_silently_dropped() {
// Documents the known limitation: excess leading `..` are dropped
assert_eq!(
clean_path(Path::new("../../foo")),
PathBuf::from("foo")
);
}
}
2 changes: 1 addition & 1 deletion tests/resolution/parity.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ describe.skipIf(!hasNative)('Import resolution parity', () => {
assertParity(path.join(rootDir, 'index.js'), 'lodash', noAliases);
});

// Known native engine limitation: does not normalize ../ or append extensions
// Un-skip once the native binary is published with the clean_path fix (#600)
it.skip('resolves parent directory traversal', () => {
// Create a temporary nested structure
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-parity-'));
Expand Down
Loading