fix(edgevec): normalize file paths to forward slashes at browse + Edg… - #151
Open
marlon-costa-dc wants to merge 2 commits into
Open
fix(edgevec): normalize file paths to forward slashes at browse + Edg…#151marlon-costa-dc wants to merge 2 commits into
marlon-costa-dc wants to merge 2 commits into
Conversation
…eVec comparison boundaries so backslash queries match stored chunks (mcb-ns8z)
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
PR Summary by QodoNormalize EdgeVec Paths for Cross-Platform Chunk Lookup
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
36 rules 1. Test imports incorrectly ordered
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+418
to
+507
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use mcb_domain::value_objects::Embedding; | ||
| use mcb_utils::utils::path::normalize_path_separators; | ||
| use tokio::sync::mpsc; | ||
|
|
||
| type TestResult<T = ()> = std::result::Result<T, Box<dyn std::error::Error>>; | ||
|
|
||
| const TEST_DIMS: usize = 8; | ||
|
|
||
| /// Build a minimal `EdgeVecActor` with a small dimension for fast tests. | ||
| fn make_actor() -> TestResult<EdgeVecActor> { | ||
| let (_tx, rx) = mpsc::channel(1); | ||
| let cfg = EdgeVecConfig { | ||
| dimensions: TEST_DIMS, | ||
| ..EdgeVecConfig::default() | ||
| }; | ||
| Ok(EdgeVecActor::new(rx, cfg)?) | ||
| } | ||
|
|
||
| /// Insert one vector with a `file_path` metadata field stored with forward slashes. | ||
| /// | ||
| /// Returns the collection name used, so callers can query it. | ||
| fn insert_forward_slash_chunk(actor: &mut EdgeVecActor) -> TestResult<String> { | ||
| let collection = "test_col"; | ||
| actor.handle_create_collection(collection.to_owned())?; | ||
|
|
||
| let embedding = Embedding { | ||
| vector: vec![1.0_f32; TEST_DIMS], | ||
| model: "test".to_owned(), | ||
| dimensions: TEST_DIMS, | ||
| }; | ||
|
|
||
| let metadata = vec![{ | ||
| let mut m = std::collections::HashMap::new(); | ||
| m.insert( | ||
| VECTOR_FIELD_FILE_PATH.to_owned(), | ||
| serde_json::json!("src/lib.rs"), | ||
| ); | ||
| m.insert("content".to_owned(), serde_json::json!("fn foo() {}")); | ||
| m.insert("language".to_owned(), serde_json::json!("rust")); | ||
| m.insert("start_line".to_owned(), serde_json::json!(1_u32)); | ||
| m.insert("end_line".to_owned(), serde_json::json!(3_u32)); | ||
| m.insert("chunk_index".to_owned(), serde_json::json!(0_u32)); | ||
| m | ||
| }]; | ||
|
|
||
| let ids = actor.handle_insert_vectors(collection, vec![embedding], metadata)?; | ||
| assert_eq!(ids.len(), 1, "expected one inserted id"); | ||
| Ok(collection.to_owned()) | ||
| } | ||
|
|
||
| /// Regression test for mcb-ns8z: forward-slash and backslash queries must | ||
| /// return the same chunks, regardless of the separator used by the caller. | ||
| #[test] | ||
| fn get_chunks_by_file_backslash_and_forward_slash_are_equivalent() -> TestResult { | ||
| let mut actor = make_actor()?; | ||
| let collection = insert_forward_slash_chunk(&mut actor)?; | ||
|
|
||
| let forward = actor.handle_get_chunks_by_file(&collection, "src/lib.rs")?; | ||
| let backward = actor.handle_get_chunks_by_file(&collection, "src\\lib.rs")?; | ||
|
|
||
| assert_eq!( | ||
| forward.len(), | ||
| 1, | ||
| "forward-slash query must return 1 chunk, got {}: {forward:?}", | ||
| forward.len() | ||
| ); | ||
| assert_eq!( | ||
| backward.len(), | ||
| forward.len(), | ||
| "backslash query must return same number of chunks as forward-slash query" | ||
| ); | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Confirm that `normalize_path_separators` is a no-op for paths without backslashes. | ||
| #[test] | ||
| fn normalize_path_separators_forward_slash_unchanged() { | ||
| assert_eq!(normalize_path_separators("src/lib.rs"), "src/lib.rs"); | ||
| } | ||
|
|
||
| /// Confirm that `normalize_path_separators` replaces backslashes. | ||
| #[test] | ||
| fn normalize_path_separators_backslash_replaced() { | ||
| assert_eq!(normalize_path_separators("src\\lib.rs"), "src/lib.rs"); | ||
| assert_eq!(normalize_path_separators("a\\b\\c.rs"), "a/b/c.rs"); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
1. Tests bloat actor.rs 📘 Rule violation ⚙ Maintainability
crates/mcb-providers/src/vector_store/edgevec/actor.rs is now ~507 lines after adding a large in-file test module, exceeding the ~200-line limit. This increases maintenance burden and makes the module harder to navigate.
Agent Prompt
## Issue description
`crates/mcb-providers/src/vector_store/edgevec/actor.rs` exceeds the ~200-line limit after adding an in-file `#[cfg(test)] mod tests` block.
## Issue Context
The compliance rule requires modified source files to stay under ~200 lines by splitting into submodules. The newly added tests can live in a dedicated test module/file without increasing the production module size.
## Fix Focus Areas
- crates/mcb-providers/src/vector_store/edgevec/actor.rs[418-507]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+302
to
311
| // Normalize to forward slashes for cross-platform path matching. | ||
| let normalized_query = normalize_path_separators(file_path); | ||
| if let Some(collection_metadata) = self.get_collection_metadata(collection) { | ||
| for (ext_id, meta_val) in collection_metadata.iter() { | ||
| if let Some(meta) = meta_val.as_object() | ||
| && meta | ||
| .get(VECTOR_FIELD_FILE_PATH) | ||
| .and_then(|v| v.as_str()) | ||
| .is_some_and(|p| p.replace('\\', "/") == normalized_query) | ||
| .is_some_and(|p| normalize_path_separators(p) == normalized_query) | ||
| { |
Contributor
There was a problem hiding this comment.
2. Non-canonical file_path returned 🐞 Bug ≡ Correctness
After normalizing separators for matching in handle_get_chunks_by_file, the code still overwrites SearchResult.file_path with the raw query string, so a src\lib.rs query returns results whose file_path contains backslashes. This violates the workspace’s forward-slash canonical path convention and can cause mismatches when callers reuse or key/group on returned file_path values.
Agent Prompt
### Issue description
`EdgeVecActor::handle_get_chunks_by_file` now normalizes path separators so backslash queries match stored forward-slash metadata, but it still returns `SearchResult.file_path` as the **raw** query string. This can leak backslashes into results even though the system canonicalizes workspace-relative paths to forward slashes.
### Issue Context
- The indexing pipeline canonicalizes paths to forward slashes (via `workspace_relative_path` → `path_to_utf8_string`).
- `handle_get_chunks_by_file` should return results with canonical `file_path` (preferably the stored metadata path or the normalized query), not whatever separator the caller used.
### Fix Focus Areas
- crates/mcb-providers/src/vector_store/edgevec/actor.rs[296-316]
### Suggested fix
- Prefer **not** overriding `result.file_path` at all (keep `search_result_from_json_metadata`’s metadata-derived canonical path), OR set it to the normalized form:
- Replace `result.file_path = file_path.to_owned();` with `result.file_path.clone_from(&normalized_query);` (or `result.file_path = normalized_query.clone();`).
- Strengthen the regression test to assert the returned `file_path` is normalized (e.g., both queries return `"src/lib.rs"`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+54
to
+64
| /// Normalizes path separators to forward slashes. | ||
| /// | ||
| /// Replaces every backslash (`\`) with a forward slash (`/`) so that | ||
| /// paths stored or compared inside vector-store metadata are consistent | ||
| /// across Windows and Unix platforms. | ||
| /// | ||
| /// This is a pure string operation and does **not** access the filesystem. | ||
| #[must_use] | ||
| pub fn normalize_path_separators(path: &str) -> String { | ||
| path.replace('\\', "/") | ||
| } |
Contributor
There was a problem hiding this comment.
1. Path docs drift 🐞 Bug ⚙ Maintainability
mcb_utils::utils::path’s module docs state that all functions return Result, but this PR adds normalize_path_separators(&str) -> String, making that contract literally untrue. The new function’s doc comment also references “vector-store metadata”, which conflicts with mcb-utils’ stated “zero domain knowledge” principle.
Agent Prompt
## Issue description
`crates/mcb-utils/src/utils/path.rs` contains module-level documentation claiming all functions return `Result`, but this PR adds `normalize_path_separators(&str) -> String`. Additionally, the new function’s documentation mentions “vector-store metadata”, which is domain-specific and clashes with the `mcb-utils` crate’s stated goal of having zero domain knowledge.
## Issue Context
This is not a runtime bug: `normalize_path_separators` is infallible by design. The problem is that module/crate documentation and layering guidance become misleading/inconsistent after this addition.
## Fix Focus Areas
- crates/mcb-utils/src/utils/path.rs[1-65]
- crates/mcb-utils/src/lib.rs[16-21]
Suggested changes:
- Update the module-level comment to clarify that *fallible* path operations return `Result`, while infallible string transforms may return plain values.
- Rewrite the `normalize_path_separators` doc comment to be domain-agnostic (avoid mentioning vector-store metadata).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 58215b1 |
marlon-costa-dc
enabled auto-merge (squash)
July 29, 2026 03:07
Comment on lines
+420
to
+423
| use super::*; | ||
| use mcb_domain::value_objects::Embedding; | ||
| use mcb_utils::utils::path::normalize_path_separators; | ||
| use tokio::sync::mpsc; |
Contributor
There was a problem hiding this comment.
1. Test imports incorrectly ordered 📘 Rule violation ⚙ Maintainability
The new test module orders local and mcb_* imports before the external tokio import, contrary to the required external → mcb_* → local grouping. This makes the changed Rust file noncompliant with the repository import-order rule.
Agent Prompt
## Issue description
The new test module's imports are not grouped in the required order: external crates, then `mcb_*` crates, then local modules.
## Issue Context
Move `tokio` first, keep the `mcb_domain` and `mcb_utils` imports together next, and place `super::*` last.
## Fix Focus Areas
- crates/mcb-providers/src/vector_store/edgevec/actor.rs[420-423]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 1ad7cb8 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 5/5
- In
crates/mcb-utils/src/utils/path.rs, having separate path-separator normalization logic for string andPathflows creates a drift risk where callers get inconsistent normalized paths over time; this could cause subtle cross-platform behavior differences—havepath_to_utf8_stringdelegate to the shared helper so normalization rules stay in one place.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/mcb-utils/src/utils/path.rs">
<violation number="1" location="crates/mcb-utils/src/utils/path.rs:63">
P3: Path-separator normalization now has two implementations in the same module, so future normalization changes can diverge between string and `Path` callers. Consider making `path_to_utf8_string` delegate to this helper so there is one normalization boundary.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
| /// This is a pure string operation and does **not** access the filesystem. | ||
| #[must_use] | ||
| pub fn normalize_path_separators(path: &str) -> String { | ||
| path.replace('\\', "/") |
There was a problem hiding this comment.
P3: Path-separator normalization now has two implementations in the same module, so future normalization changes can diverge between string and Path callers. Consider making path_to_utf8_string delegate to this helper so there is one normalization boundary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-utils/src/utils/path.rs, line 63:
<comment>Path-separator normalization now has two implementations in the same module, so future normalization changes can diverge between string and `Path` callers. Consider making `path_to_utf8_string` delegate to this helper so there is one normalization boundary.</comment>
<file context>
@@ -51,6 +51,18 @@ pub fn path_to_utf8_string(path: &Path) -> Result<String, UtilsError> {
+/// This is a pure string operation and does **not** access the filesystem.
+#[must_use]
+pub fn normalize_path_separators(path: &str) -> String {
+ path.replace('\\', "/")
+}
+
</file context>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
…eVec comparison boundaries so backslash queries match stored chunks (mcb-ns8z)