diff --git a/crates/sem-core/src/model/identity.rs b/crates/sem-core/src/model/identity.rs index 5b150b33..32fc874f 100644 --- a/crates/sem-core/src/model/identity.rs +++ b/crates/sem-core/src/model/identity.rs @@ -3,10 +3,39 @@ use std::collections::{HashMap, HashSet}; use super::change::{ChangeType, SemanticChange}; use super::entity::SemanticEntity; -/// Extracts the leaf name from a parent_id string (last "::" segment). -fn parent_name(entity: &SemanticEntity) -> Option { - let pid = entity.parent_id.as_ref()?; - pid.rsplit("::").next().map(String::from) +fn parent_name( + entity: &SemanticEntity, + by_id: &HashMap<&str, &SemanticEntity>, +) -> Option { + let mut parts: Vec<&str> = Vec::new(); + let mut visited: HashSet<&str> = HashSet::new(); + let mut pid = entity.parent_id.as_deref()?; + loop { + if !visited.insert(pid) { + break; + } + match by_id.get(pid) { + Some(parent) => { + // Skip ancestors with empty names (e.g. JSON's empty-string + // root-package key in package-lock.json). The full path is + // still recoverable from entity_id; the displayed chain is + // for human readability. + if !parent.name.is_empty() { + parts.push(parent.name.as_str()); + } + match parent.parent_id.as_deref() { + Some(next) => pid = next, + None => break, + } + } + None => break, + } + } + if parts.is_empty() { + return None; + } + parts.reverse(); + Some(parts.join("::")) } pub struct MatchResult { @@ -29,6 +58,7 @@ fn make_change( before_entity: Option<&SemanticEntity>, commit_sha: Option<&str>, author: Option<&str>, + by_id: &HashMap<&str, &SemanticEntity>, ) -> SemanticChange { let prefix = match change_type { ChangeType::Added => "added::", @@ -49,7 +79,7 @@ fn make_change( entity_type: primary.entity_type.clone(), entity_name: primary.name.clone(), entity_line: primary.start_line, - parent_name: parent_name(primary), + parent_name: parent_name(primary, by_id), file_path: primary.file_path.clone(), old_entity_name: before_entity.and_then(|b| { (b.name != after_entity.name).then(|| b.name.clone()) @@ -94,6 +124,14 @@ pub fn match_entities( let after_by_id: HashMap<&str, &SemanticEntity> = after.iter().map(|e| (e.id.as_str(), e)).collect(); + // Combined map for ancestor-chain lookup: after takes precedence so the + // displayed path reflects the post-change tree for non-deleted entities. + let combined_by_id: HashMap<&str, &SemanticEntity> = before + .iter() + .map(|e| (e.id.as_str(), e)) + .chain(after.iter().map(|e| (e.id.as_str(), e))) + .collect(); + // Phase 1: Exact ID match for (&id, after_entity) in &after_by_id { if let Some(before_entity) = before_by_id.get(id) { @@ -101,7 +139,7 @@ pub fn match_entities( matched_after.insert(id); if before_entity.content_hash != after_entity.content_hash { - let mut change = make_change(after_entity, ChangeType::Modified, Some(before_entity), commit_sha, author); + let mut change = make_change(after_entity, ChangeType::Modified, Some(before_entity), commit_sha, author, &combined_by_id); change.structural_change = match (&before_entity.structural_hash, &after_entity.structural_hash) { (Some(before_sh), Some(after_sh)) => Some(before_sh != after_sh), _ => None, @@ -171,7 +209,7 @@ pub fn match_entities( continue; } - changes.push(make_change(after_entity, classify_match(before_entity, after_entity), Some(before_entity), commit_sha, author)); + changes.push(make_change(after_entity, classify_match(before_entity, after_entity), Some(before_entity), commit_sha, author, &combined_by_id)); } } @@ -269,7 +307,7 @@ pub fn match_entities( continue; } - changes.push(make_change(after_entity, classify_match(matched, after_entity), Some(matched), commit_sha, author)); + changes.push(make_change(after_entity, classify_match(matched, after_entity), Some(matched), commit_sha, author, &combined_by_id)); } } } @@ -277,16 +315,16 @@ pub fn match_entities( // Phase 4: Intra-file reorder detection // For entities that matched by exact ID with identical content (unchanged), // check if their relative ordering changed within the file. - detect_reorders(before, after, &matched_before, &matched_after, &mut changes, commit_sha, author); + detect_reorders(before, after, &matched_before, &matched_after, &mut changes, commit_sha, author, &combined_by_id); // Remaining unmatched before = deleted for entity in before.iter().filter(|e| !matched_before.contains(e.id.as_str())) { - changes.push(make_change(entity, ChangeType::Deleted, Some(entity), commit_sha, author)); + changes.push(make_change(entity, ChangeType::Deleted, Some(entity), commit_sha, author, &combined_by_id)); } // Remaining unmatched after = added for entity in after.iter().filter(|e| !matched_after.contains(e.id.as_str())) { - changes.push(make_change(entity, ChangeType::Added, None, commit_sha, author)); + changes.push(make_change(entity, ChangeType::Added, None, commit_sha, author, &combined_by_id)); } MatchResult { changes } @@ -333,6 +371,7 @@ fn detect_reorders( changes: &mut Vec, commit_sha: Option<&str>, author: Option<&str>, + by_id: &HashMap<&str, &SemanticEntity>, ) { // Collect unchanged entities: matched by ID with same content_hash let before_by_id: HashMap<&str, &SemanticEntity> = @@ -383,7 +422,7 @@ fn detect_reorders( if lis_set.contains(&i) { continue; } - changes.push(make_change(after_entity, ChangeType::Reordered, None, commit_sha, author)); + changes.push(make_change(after_entity, ChangeType::Reordered, None, commit_sha, author, by_id)); } } } @@ -730,4 +769,22 @@ mod tests { assert!(score > 0.5); assert!(score < 1.0); } + + #[test] + fn parent_name_terminates_on_cyclic_parent_id() { + // Two entities whose parent_id chains form a cycle. parent_name + // would loop forever without the visited-set guard. + let a = make_entity_with_parent("A", "A", "", "f", Some("B")); + let b = make_entity_with_parent("B", "B", "", "f", Some("A")); + let mut by_id: HashMap<&str, &SemanticEntity> = HashMap::new(); + by_id.insert("A", &a); + by_id.insert("B", &b); + // Synthesize a leaf whose parent_id enters the cycle via A. + let leaf = make_entity_with_parent("L", "L", "", "f", Some("A")); + let chain = parent_name(&leaf, &by_id); + // Must terminate. We don't assert exact contents — order/composition + // depends on which side of the cycle is reached first; the safety + // property is "this returns at all." + assert!(chain.is_some()); + } } diff --git a/crates/sem-core/src/parser/differ.rs b/crates/sem-core/src/parser/differ.rs index 3158bb79..9f9bea6b 100644 --- a/crates/sem-core/src/parser/differ.rs +++ b/crates/sem-core/src/parser/differ.rs @@ -145,12 +145,6 @@ pub fn compute_semantic_diff( } } -/// Remove "Modified" parent entities from the change list when at least one -/// child entity also appears as a change. This avoids showing e.g. an impl -/// block as modified when the real change is in a method inside it. -/// Only suppresses container entity types (impl, trait, module) where the -/// parent is just a wrapper. Functions, structs, etc. are never suppressed -/// because they have independent meaningful content. fn suppress_redundant_parents( changes: &mut Vec, before: &[SemanticEntity], @@ -164,6 +158,7 @@ fn suppress_redundant_parents( "impl", "trait", "module", "class", "interface", "mixin", "extension", "namespace", "export", "package", "svelte_instance_script", "svelte_module_script", + "object", ]; let before_by_id: HashMap<&str, &SemanticEntity> = @@ -204,11 +199,12 @@ fn suppress_redundant_parents( continue; } - // For Added/Deleted containers: suppress unconditionally — the children carry the detail. - // For Modified: only suppress if the container's own declaration didn't change. + // Added/Deleted: suppress unconditionally; the children carry the detail. + // Modified: only suppress if the container's own declaration is unchanged + // and the value type didn't transition. let should_suppress = if change.change_type == ChangeType::Modified { match (before_by_id.get(eid), after_by_id.get(eid)) { - (Some(bp), Some(ap)) => { + (Some(bp), Some(ap)) if bp.entity_type == ap.entity_type => { let before_own = strip_children_content(&bp.content, bp.start_line, b_children); let after_own = strip_children_content(&ap.content, ap.start_line, a_children); before_own == after_own @@ -224,11 +220,47 @@ fn suppress_redundant_parents( } } + // Suppress an old parent that a Moved child left behind when the old + // parent itself appears as a change — handles the parent-rename case + // where the parent itself failed to match. + for change in changes.iter() { + if change.change_type == ChangeType::Moved { + if let Some(ref old_pid) = change.old_parent_id { + if changed_ids.contains(old_pid.as_str()) { + suppress.insert(old_pid.clone()); + } + } + } + } + if !suppress.is_empty() { + changes.retain(|c| !suppress.contains(&c.entity_id)); + } + + // Drop a Moved child whose key is unchanged and whose old parent matches + // a Renamed entity — the child only "moved" because the parent renamed. + let renamed_before_ids: HashSet<&str> = changes + .iter() + .filter(|c| c.change_type == ChangeType::Renamed) + .filter_map(|c| { + let old_name = c.old_entity_name.as_deref()?; + let after_entity = after_by_id.get(c.entity_id.as_str())?; + before.iter() + .find(|e| { + e.name == old_name + && e.entity_type == after_entity.entity_type + && e.parent_id == after_entity.parent_id + }) + .map(|e| e.id.as_str()) + }) + .collect(); + + if !renamed_before_ids.is_empty() { changes.retain(|c| { - !(matches!(c.change_type, ChangeType::Modified | ChangeType::Added | ChangeType::Deleted) - && suppress.contains(&c.entity_id) - && CONTAINER_TYPES.contains(&c.entity_type.as_str())) + !(c.change_type == ChangeType::Moved + && c.old_entity_name.is_none() + && c.old_parent_id.as_deref() + .map_or(false, |pid| renamed_before_ids.contains(pid))) }); } } diff --git a/crates/sem-core/src/parser/plugins/json.rs b/crates/sem-core/src/parser/plugins/json.rs index 9fce84ea..f06dc753 100644 --- a/crates/sem-core/src/parser/plugins/json.rs +++ b/crates/sem-core/src/parser/plugins/json.rs @@ -1,4 +1,4 @@ -use crate::model::entity::{build_entity_id, SemanticEntity}; +use crate::model::entity::SemanticEntity; use crate::parser::plugin::SemanticParserPlugin; use crate::utils::hash::content_hash; @@ -14,93 +14,211 @@ impl SemanticParserPlugin for JsonParserPlugin { } fn extract_entities(&self, content: &str, file_path: &str) -> Vec { - // Extract top-level properties from JSON objects, plus depth-2 children - // for "object" entities (e.g. scripts, dependencies in package.json). - // We scan the source text directly to get accurate line positions, - // which weave needs for entity-level merge reconstruction. - let trimmed = content.trim(); - if !trimmed.starts_with('{') { + if !content.trim().starts_with('{') { return Vec::new(); } + extract_entries(content, file_path) + } +} - let lines: Vec<&str> = content.lines().collect(); - let entries = find_top_level_entries(content); - let closing = find_closing_brace_line(&lines); +struct Frame { + content: String, + entries: Vec, + cursor: usize, + line_offset: usize, + parent_pointer: Option, + parent_entity_id: Option, +} - let mut entities = Vec::new(); - for (i, entry) in entries.iter().enumerate() { - let end_line = if i + 1 < entries.len() { - let next_start = entries[i + 1].start_line; - trim_trailing_blanks(&lines, entry.start_line, next_start) - } else { - trim_trailing_blanks(&lines, entry.start_line, closing) - }; +/// Iterative walk of the JSON tree, emitting entities in DFS pre-order. +/// Frames track a cursor through their entries; encountering an +/// object-valued entry pushes both the parent frame (resumed after) and the +/// child frame (visited next), so children appear before later siblings. +fn extract_entries(content: &str, file_path: &str) -> Vec { + let mut entities = Vec::new(); + let root_entries = find_top_level_entries(content); + let mut worklist: Vec = vec![Frame { + content: content.to_string(), + entries: root_entries, + cursor: 0, + line_offset: 1, + parent_pointer: None, + parent_entity_id: None, + }]; + + while let Some(mut frame) = worklist.pop() { + let lines: Vec<&str> = frame.content.lines().collect(); + let closing = find_closing_brace_line(&lines); - let entity_content = lines[entry.start_line - 1..end_line] - .join("\n"); + while frame.cursor < frame.entries.len() { + let i = frame.cursor; + frame.cursor += 1; + let entry = &frame.entries[i]; + let next_boundary = frame.entries.get(i + 1).map(|e| e.start_line).unwrap_or(closing); + let end_line = trim_trailing_blanks(&lines, entry.start_line, next_boundary); + let entity_content = lines[entry.start_line - 1..end_line].join("\n"); let value_content = extract_value_content(&entity_content); - let structural_hash = Some(content_hash(value_content)); - let parent_id = build_entity_id(file_path, &entry.entity_type, &entry.pointer, None); + let pointer = match &frame.parent_pointer { + Some(pp) => format!("{pp}{}", entry.pointer), + None => entry.pointer.clone(), + }; + let entity_id = format!("{}::{}", file_path, pointer); + let abs_start = frame.line_offset + entry.start_line - 1; + let abs_end = frame.line_offset + end_line - 1; entities.push(SemanticEntity { - id: parent_id.clone(), + id: entity_id.clone(), file_path: file_path.to_string(), entity_type: entry.entity_type.clone(), name: entry.key.clone(), - parent_id: None, + parent_id: frame.parent_entity_id.clone(), content_hash: content_hash(&entity_content), - structural_hash, + structural_hash: Some(content_hash(value_content)), content: entity_content.clone(), - start_line: entry.start_line, - end_line, + start_line: abs_start, + end_line: abs_end, metadata: None, }); - // Extract depth-2 children from "object" entities if entry.entity_type == "object" { - let nested = find_nested_object_entries(&entity_content, entry.start_line); - for (j, nentry) in nested.iter().enumerate() { - let child_end = if j + 1 < nested.len() { - trim_trailing_blanks(&lines, nentry.start_line, nested[j + 1].start_line) - } else { - trim_trailing_blanks(&lines, nentry.start_line, end_line) + if let Some(obj_str) = extract_object_value(&entity_content) { + let obj_line_in_entity = find_value_start_line(&entity_content); + let child = Frame { + content: obj_str.to_string(), + entries: find_top_level_entries(obj_str), + cursor: 0, + line_offset: abs_start + obj_line_in_entity - 1, + parent_pointer: Some(pointer), + parent_entity_id: Some(entity_id), }; + worklist.push(frame); + worklist.push(child); + break; + } + } + } + } - let child_content = lines[nentry.start_line - 1..child_end].join("\n"); - let child_value = extract_value_content(&child_content); - - entities.push(SemanticEntity { - id: build_entity_id(file_path, &nentry.entity_type, &nentry.key, Some(&parent_id)), - file_path: file_path.to_string(), - entity_type: nentry.entity_type.clone(), - name: nentry.key.clone(), - parent_id: Some(parent_id.clone()), - content_hash: content_hash(&child_content), - structural_hash: Some(content_hash(child_value)), - content: child_content, - start_line: nentry.start_line, - end_line: child_end, - metadata: None, - }); + entities +} + +/// Given an entity content string like ` "scripts": {\n "build": "tsc"\n }`, +/// return a slice that starts at the opening `{` of the value and ends at (and +/// including) the matching closing `}`. +fn extract_object_value(content: &str) -> Option<&str> { + // Skip past the first `:` (outside strings) to find the value + let mut in_string = false; + let mut escape_next = false; + let mut colon_pos: Option = None; + + for (i, ch) in content.char_indices() { + if escape_next { + escape_next = false; + continue; + } + if ch == '\\' && in_string { + escape_next = true; + continue; + } + if ch == '"' { + in_string = !in_string; + } + if ch == ':' && !in_string { + colon_pos = Some(i); + break; + } + } + + let after_colon = &content[colon_pos? + 1..]; + // Find the opening `{` + let brace_offset = after_colon.find('{')?; + let obj_start = colon_pos? + 1 + brace_offset; + + // Find the matching `}`. Track brace and bracket depth separately so + // that a `}` only terminates extraction when no array is still open. + let mut brace_depth = 0usize; + let mut bracket_depth = 0usize; + in_string = false; + escape_next = false; + + for (i, ch) in content[obj_start..].char_indices() { + if escape_next { + escape_next = false; + continue; + } + if ch == '\\' && in_string { + escape_next = true; + continue; + } + if ch == '"' { + in_string = !in_string; + continue; + } + if !in_string { + match ch { + '{' => brace_depth += 1, + '[' => bracket_depth += 1, + '}' => { + brace_depth = brace_depth.saturating_sub(1); + if brace_depth == 0 && bracket_depth == 0 { + return Some(&content[obj_start..obj_start + i + 1]); + } } + ']' => bracket_depth = bracket_depth.saturating_sub(1), + _ => {} } } + } + None +} + +/// Return the 1-based line number (relative to the entity content) where the +/// object value's `{` appears. +fn find_value_start_line(content: &str) -> usize { + let mut in_string = false; + let mut escape_next = false; + let mut past_colon = false; + let mut line = 1usize; - entities + for ch in content.chars() { + if ch == '\n' { + line += 1; + continue; + } + if escape_next { + escape_next = false; + continue; + } + if ch == '\\' && in_string { + escape_next = true; + continue; + } + if ch == '"' { + in_string = !in_string; + continue; + } + if ch == ':' && !in_string { + past_colon = true; + continue; + } + if past_colon && ch == '{' { + return line; + } } + 1 } struct JsonEntry { key: String, pointer: String, entity_type: String, - start_line: usize, // 1-based + start_line: usize, // 1-based, relative to the content passed in } /// Scan the source text to find each top-level key in the root JSON object. -/// Returns entries with accurate start_line positions. +/// Returns entries with accurate start_line positions (1-based, relative to `content`). fn find_top_level_entries(content: &str) -> Vec { let mut entries = Vec::new(); let mut depth = 0; @@ -108,7 +226,6 @@ fn find_top_level_entries(content: &str) -> Vec { let mut escape_next = false; let mut line_num: usize = 1; - // State for tracking when we find a key at depth 1 let mut current_key: Option = None; let mut key_start = false; let mut key_buf = String::new(); @@ -153,7 +270,6 @@ fn find_top_level_entries(content: &str) -> Vec { match ch { '"' => { in_string = true; - // At depth 1, a string could be a key (before ':') or value (after ':') if depth == 1 && current_key.is_none() && !key_start { reading_key = true; key_buf.clear(); @@ -162,13 +278,12 @@ fn find_top_level_entries(content: &str) -> Vec { ':' => { if depth == 1 { if let Some(ref key) = current_key { - // Found a key: value pair at depth 1 let escaped_key = key.replace('~', "~0").replace('/', "~1"); let pointer = format!("/{escaped_key}"); entries.push(JsonEntry { key: key.clone(), pointer, - entity_type: String::new(), // filled in below + entity_type: String::new(), start_line: line_num, }); key_start = true; @@ -178,9 +293,8 @@ fn find_top_level_entries(content: &str) -> Vec { '{' | '[' => { depth += 1; if depth == 2 && key_start { - // The value for this key is an object/array if let Some(entry) = entries.last_mut() { - entry.entity_type = "object".to_string(); + entry.entity_type = if ch == '{' { "object" } else { "array" }.to_string(); } } } @@ -189,7 +303,6 @@ fn find_top_level_entries(content: &str) -> Vec { } ',' => { if depth == 1 { - // End of a top-level entry if let Some(entry) = entries.last_mut() { if entry.entity_type.is_empty() { entry.entity_type = "property".to_string(); @@ -203,7 +316,6 @@ fn find_top_level_entries(content: &str) -> Vec { } } - // Handle last entry (no trailing comma) if let Some(entry) = entries.last_mut() { if entry.entity_type.is_empty() { entry.entity_type = "property".to_string(); @@ -213,117 +325,6 @@ fn find_top_level_entries(content: &str) -> Vec { entries } -/// Find keys inside a depth-1 object value within an entity's content. -/// Returns entries with absolute line numbers computed from `base_line`. -fn find_nested_object_entries(entity_content: &str, base_line: usize) -> Vec { - let mut entries = Vec::new(); - let mut in_string = false; - let mut escape_next = false; - let mut line_num: usize = 0; // 0-based offset from base_line - let mut found_outer_colon = false; - let mut found_value_start = false; - let mut value_depth: usize = 0; - let mut current_key: Option = None; - let mut reading_key = false; - let mut key_buf = String::new(); - let mut key_start = false; - - for ch in entity_content.chars() { - if ch == '\n' { - line_num += 1; - continue; - } - - if escape_next { - if reading_key { - key_buf.push(ch); - } - escape_next = false; - continue; - } - - if ch == '\\' && in_string { - if reading_key { - key_buf.push(ch); - } - escape_next = true; - continue; - } - - if in_string { - if ch == '"' { - in_string = false; - if reading_key { - reading_key = false; - current_key = Some(key_buf.clone()); - key_buf.clear(); - } - } else if reading_key { - key_buf.push(ch); - } - continue; - } - - if !found_value_start { - match ch { - '"' => { - in_string = true; - } - ':' => { - found_outer_colon = true; - } - '{' if found_outer_colon => { - found_value_start = true; - value_depth = 1; - } - _ => {} - } - continue; - } - - match ch { - '"' => { - in_string = true; - if value_depth == 1 && current_key.is_none() && !key_start { - reading_key = true; - key_buf.clear(); - } - } - ':' => { - if value_depth == 1 { - if let Some(ref key) = current_key { - entries.push(JsonEntry { - key: key.clone(), - pointer: String::new(), - entity_type: "property".to_string(), - start_line: base_line + line_num, - }); - key_start = true; - } - } - } - '{' | '[' => { - value_depth += 1; - } - '}' | ']' => { - value_depth -= 1; - if value_depth == 0 { - break; - } - } - ',' => { - if value_depth == 1 { - current_key = None; - key_start = false; - } - } - _ => {} - } - } - - entries -} - /// Extract just the value portion of a `"key": value` entity content string, /// stripping the key name so that renamed keys with identical values share the /// same structural_hash and are detected as renames rather than delete + add. @@ -378,8 +379,38 @@ fn trim_trailing_blanks(lines: &[&str], start: usize, next_start: usize) -> usiz #[cfg(test)] mod tests { use super::*; - use crate::model::change::ChangeType; - use crate::model::identity::match_entities; + use crate::git::types::{FileChange, FileStatus}; + use crate::model::change::{ChangeType, SemanticChange}; + use crate::parser::differ::compute_semantic_diff; + use crate::parser::registry::ParserRegistry; + + /// Run the full pipeline and drop orphan changes (which represent line-level + /// noise outside entity spans like the root `{` `}` brackets). + fn json_diff(before: &str, after: &str) -> Vec { + let mut registry = ParserRegistry::new(); + registry.register(Box::new(JsonParserPlugin)); + let changes = vec![FileChange { + file_path: "test.json".to_string(), + status: FileStatus::Modified, + old_file_path: None, + before_content: Some(before.to_string()), + after_content: Some(after.to_string()), + }]; + compute_semantic_diff(&changes, ®istry, None, None) + .changes + .into_iter() + .filter(|c| c.entity_type != "orphan") + .collect() + } + + fn names(changes: &[SemanticChange]) -> Vec<(String, ChangeType)> { + changes.iter().map(|c| (c.entity_name.clone(), c.change_type)).collect() + } + + fn find_change<'a>(changes: &'a [SemanticChange], name: &str, kind: ChangeType) -> &'a SemanticChange { + changes.iter().find(|c| c.entity_name == name && c.change_type == kind) + .unwrap_or_else(|| panic!("expected {:?} {} in changes; got: {:?}", kind, name, names(changes))) + } #[test] fn test_json_line_positions() { @@ -396,77 +427,555 @@ mod tests { let plugin = JsonParserPlugin; let entities = plugin.extract_entities(content, "package.json"); - assert_eq!(entities.len(), 6); + // Top-level entities + let top: Vec<_> = entities.iter().filter(|e| e.parent_id.is_none()).collect(); + assert_eq!(top.len(), 4); - assert_eq!(entities[0].name, "name"); - assert_eq!(entities[0].start_line, 2); - assert_eq!(entities[0].end_line, 2); - assert!(entities[0].parent_id.is_none()); + assert_eq!(top[0].name, "name"); + assert_eq!(top[0].start_line, 2); + assert_eq!(top[0].end_line, 2); - assert_eq!(entities[1].name, "version"); - assert_eq!(entities[1].start_line, 3); - assert_eq!(entities[1].end_line, 3); + assert_eq!(top[1].name, "version"); + assert_eq!(top[1].start_line, 3); + assert_eq!(top[1].end_line, 3); - assert_eq!(entities[2].name, "scripts"); - assert_eq!(entities[2].entity_type, "object"); - assert_eq!(entities[2].start_line, 4); - assert_eq!(entities[2].end_line, 7); + assert_eq!(top[2].name, "scripts"); + assert_eq!(top[2].entity_type, "object"); + assert_eq!(top[2].start_line, 4); + assert_eq!(top[2].end_line, 7); - // Depth-2 children of "scripts" - assert_eq!(entities[3].name, "build"); - assert_eq!(entities[3].start_line, 5); - assert_eq!(entities[3].end_line, 5); - assert_eq!(entities[3].parent_id.as_deref(), Some(&entities[2].id as &str)); + assert_eq!(top[3].name, "description"); + assert_eq!(top[3].start_line, 8); + assert_eq!(top[3].end_line, 8); + } + + #[test] + fn test_nested_entities_extracted() { + let content = r#"{ + "scripts": { + "build": "tsc", + "test": "jest" + } +} +"#; + let plugin = JsonParserPlugin; + let entities = plugin.extract_entities(content, "package.json"); + + // Should have "scripts" (top-level) + "build" and "test" (nested) + assert_eq!(entities.len(), 3); - assert_eq!(entities[4].name, "test"); - assert_eq!(entities[4].start_line, 6); - assert_eq!(entities[4].end_line, 6); - assert_eq!(entities[4].parent_id.as_deref(), Some(&entities[2].id as &str)); + let scripts = entities.iter().find(|e| e.name == "scripts").unwrap(); + assert!(scripts.parent_id.is_none()); - assert_eq!(entities[5].name, "description"); - assert_eq!(entities[5].start_line, 8); - assert_eq!(entities[5].end_line, 8); + let build = entities.iter().find(|e| e.name == "build").unwrap(); + assert_eq!(build.parent_id, Some(scripts.id.clone())); + assert_eq!(build.start_line, 3); + + let test = entities.iter().find(|e| e.name == "test").unwrap(); + assert_eq!(test.parent_id, Some(scripts.id.clone())); + assert_eq!(test.start_line, 4); } + // ───────────────────────────────────────────────────────────────────────── + // Top-level scalars + // ───────────────────────────────────────────────────────────────────────── + #[test] - fn test_rename_detected_end_to_end() { - let before_content = "{\n \"timeout\": 30\n}\n"; - let after_content = "{\n \"request_timeout\": 30\n}\n"; - let plugin = JsonParserPlugin; - let before = plugin.extract_entities(before_content, "config.json"); - let after = plugin.extract_entities(after_content, "config.json"); - let result = match_entities(&before, &after, "config.json", None, None, None); - assert_eq!(result.changes.len(), 1); - assert_eq!(result.changes[0].change_type, ChangeType::Renamed); - assert_eq!(result.changes[0].entity_name, "request_timeout"); + fn scalar_value_change_reports_modified() { + let changes = json_diff( + "{\n \"name\": \"foo\"\n}", + "{\n \"name\": \"bar\"\n}", + ); + assert_eq!(names(&changes), vec![("name".into(), ChangeType::Modified)]); + assert_eq!(changes[0].parent_name, None); + } + + #[test] + fn scalar_added_to_empty_object_reports_only_the_scalar() { + let changes = json_diff("{}", "{\n \"name\": \"foo\"\n}"); + assert_eq!(names(&changes), vec![("name".into(), ChangeType::Added)]); } #[test] - fn test_renamed_scalar_property_shares_structural_hash() { - let before_content = "{\n \"timeout\": 30\n}\n"; - let after_content = "{\n \"request_timeout\": 30\n}\n"; + fn scalar_deleted_from_object_reports_only_the_scalar() { + let changes = json_diff("{\n \"name\": \"foo\"\n}", "{}"); + assert_eq!(names(&changes), vec![("name".into(), ChangeType::Deleted)]); + } + + #[test] + fn scalar_key_renamed_with_unchanged_value_reports_renamed() { + let changes = json_diff( + "{\n \"timeout\": 30\n}", + "{\n \"testTimeout\": 30\n}", + ); + assert_eq!(changes.len(), 1); + assert_eq!(changes[0].change_type, ChangeType::Renamed); + assert_eq!(changes[0].entity_name, "testTimeout"); + assert_eq!(changes[0].old_entity_name.as_deref(), Some("timeout")); + } + + // ───────────────────────────────────────────────────────────────────────── + // Parent suppression — object containers don't surface when children change + // ───────────────────────────────────────────────────────────────────────── + + #[test] + fn child_modified_inside_object_only_child_reported() { + let changes = json_diff( + "{\n \"scripts\": {\n \"build\": \"tsc\"\n }\n}", + "{\n \"scripts\": {\n \"build\": \"webpack\"\n }\n}", + ); + assert!(!changes.iter().any(|c| c.entity_name == "scripts"), + "scripts should be suppressed; got: {:?}", names(&changes)); + let build = find_change(&changes, "build", ChangeType::Modified); + assert_eq!(build.parent_name.as_deref(), Some("scripts")); + } + + #[test] + fn child_added_inside_object_only_child_reported() { + let changes = json_diff( + "{\n \"scripts\": {\n \"build\": \"tsc\"\n }\n}", + "{\n \"scripts\": {\n \"build\": \"tsc\",\n \"test\": \"jest\"\n }\n}", + ); + assert!(!changes.iter().any(|c| c.entity_name == "scripts" && c.change_type == ChangeType::Modified), + "scripts should be suppressed; got: {:?}", names(&changes)); + let test = find_change(&changes, "test", ChangeType::Added); + assert_eq!(test.parent_name.as_deref(), Some("scripts")); + } + + #[test] + fn child_deleted_inside_object_only_child_reported() { + let changes = json_diff( + "{\n \"scripts\": {\n \"build\": \"tsc\",\n \"test\": \"jest\"\n }\n}", + "{\n \"scripts\": {\n \"build\": \"tsc\"\n }\n}", + ); + assert!(!changes.iter().any(|c| c.entity_name == "scripts" && c.change_type == ChangeType::Modified), + "scripts should be suppressed; got: {:?}", names(&changes)); + let test = find_change(&changes, "test", ChangeType::Deleted); + assert_eq!(test.parent_name.as_deref(), Some("scripts")); + } + + #[test] + fn whole_object_added_only_leaf_children_reported() { + let changes = json_diff( + "{}", + "{\n \"scripts\": {\n \"build\": \"tsc\"\n }\n}", + ); + assert!(!changes.iter().any(|c| c.entity_name == "scripts"), + "scripts (container) should be suppressed; got: {:?}", names(&changes)); + let build = find_change(&changes, "build", ChangeType::Added); + assert_eq!(build.parent_name.as_deref(), Some("scripts")); + } + + #[test] + fn whole_object_deleted_only_leaf_children_reported() { + let changes = json_diff( + "{\n \"scripts\": {\n \"build\": \"tsc\"\n }\n}", + "{}", + ); + assert!(!changes.iter().any(|c| c.entity_name == "scripts"), + "scripts (container) should be suppressed; got: {:?}", names(&changes)); + find_change(&changes, "build", ChangeType::Deleted); + } + + // ───────────────────────────────────────────────────────────────────────── + // Deep nesting — full ancestor chain in parent_name + // ───────────────────────────────────────────────────────────────────────── + + #[test] + fn deep_nested_value_change_reports_only_the_leaf_with_full_chain() { + let before = r#"{ + "jest": { + "config": { + "testTimeout": 5000 + } + } +}"#; + let after = r#"{ + "jest": { + "config": { + "testTimeout": 10000 + } + } +}"#; + let changes = json_diff(before, after); + assert_eq!(names(&changes), vec![("testTimeout".into(), ChangeType::Modified)]); + assert_eq!(changes[0].parent_name.as_deref(), Some("jest::config")); + } + + #[test] + fn empty_string_key_ancestor_is_skipped_in_parent_name() { + // package-lock.json uses "" as a key for the root project. + // Walking the parent chain for a deeply-nested change must not emit + // the empty name (would render as "::::") in the displayed path. + let before = r#"{ + "packages": { + "": { + "dependencies": { + "jose": "^6.1.3" + } + } + } +}"#; + let after = r#"{ + "packages": { + "": { + "dependencies": { + "jose": "^6.1.4" + } + } + } +}"#; + let changes = json_diff(before, after); + let jose = find_change(&changes, "jose", ChangeType::Modified); + // The empty-string key ancestor is dropped from the displayed chain. + assert_eq!(jose.parent_name.as_deref(), Some("packages::dependencies")); + } + + // ───────────────────────────────────────────────────────────────────────── + // Renames at the object level + // ───────────────────────────────────────────────────────────────────────── + + #[test] + fn nested_scalar_rename_with_unchanged_value() { + // Same value → structural_hash matches → Renamed. + let before = r#"{ + "scripts": { + "run": "node .", + "test": "jest" + } +}"#; + let after = r#"{ + "scripts": { + "start": "node .", + "test": "jest" + } +}"#; + let changes = json_diff(before, after); + let renames: Vec<_> = changes.iter().filter(|c| c.change_type == ChangeType::Renamed).collect(); + assert_eq!(renames.len(), 1); + assert_eq!(renames[0].entity_name, "start"); + assert_eq!(renames[0].old_entity_name.as_deref(), Some("run")); + assert_eq!(renames[0].parent_name.as_deref(), Some("scripts")); + } + + #[test] + fn parent_object_renamed_unchanged_child_move_suppressed() { + // scripts → tasks, dev unchanged: only the parent rename is reported. + let before = "{\n \"scripts\": {\n \"dev\": \"vite\"\n }\n}\n"; + let after = "{\n \"tasks\": {\n \"dev\": \"vite\"\n }\n}\n"; + let changes = json_diff(before, after); + let tasks = find_change(&changes, "tasks", ChangeType::Renamed); + assert_eq!(tasks.old_entity_name.as_deref(), Some("scripts")); + assert!(!changes.iter().any(|c| c.entity_name == "dev"), + "child 'dev' should be suppressed (only moved due to parent rename); got: {:?}", names(&changes)); + } + + #[test] + fn parent_object_renamed_and_child_renamed_only_child_surfaces() { + // scripts → tasks AND dev → develop. Parent rename cannot be detected + // because the renamed child key changes the parent's structural_hash. + // The child move alone conveys the move + rename via: + // parent_name="tasks", old_entity_name="dev", old_parent_id= + let before = "{\n \"scripts\": {\n \"dev\": \"vite\"\n }\n}\n"; + let after = "{\n \"tasks\": {\n \"develop\": \"vite\"\n }\n}\n"; + let changes = json_diff(before, after); + assert_eq!(names(&changes), vec![("develop".into(), ChangeType::Moved)]); + let develop = &changes[0]; + assert_eq!(develop.old_entity_name.as_deref(), Some("dev")); + assert_eq!(develop.parent_name.as_deref(), Some("tasks")); + assert!(develop.old_parent_id.is_some(), "child Moved should carry old_parent_id"); + } + + // ───────────────────────────────────────────────────────────────────────── + // Type transitions — scalar ↔ object + // ───────────────────────────────────────────────────────────────────────── + + #[test] + fn scalar_to_object_transition_reports_modified_plus_new_children_added() { + let changes = json_diff( + "{\n \"build\": \"tsc\"\n}", + "{\n \"build\": {\n \"command\": \"tsc\"\n }\n}", + ); + let build = find_change(&changes, "build", ChangeType::Modified); + assert_eq!(build.entity_type, "object", "after type should reflect new value"); + let command = find_change(&changes, "command", ChangeType::Added); + assert_eq!(command.parent_name.as_deref(), Some("build")); + } + + #[test] + fn object_to_scalar_transition_reports_modified_plus_old_children_deleted() { + let changes = json_diff( + "{\n \"config\": {\n \"watch\": true\n }\n}", + "{\n \"config\": \"auto\"\n}", + ); + let config = find_change(&changes, "config", ChangeType::Modified); + assert_eq!(config.entity_type, "property", "after type should reflect new value"); + find_change(&changes, "watch", ChangeType::Deleted); + } + + // ───────────────────────────────────────────────────────────────────────── + // Arrays — opaque (no recursion into elements) + // ───────────────────────────────────────────────────────────────────────── + + #[test] + fn array_modified_reports_only_the_array_key() { + let changes = json_diff( + "{\n \"deps\": [\"react\", \"vue\"]\n}", + "{\n \"deps\": [\"react\", \"vue\", \"lodash\"]\n}", + ); + assert_eq!(names(&changes), vec![("deps".into(), ChangeType::Modified)]); + } + + #[test] + fn array_renamed_when_contents_unchanged() { + let changes = json_diff( + "{\n \"deps\": [\"react\", \"vue\"]\n}", + "{\n \"dependencies\": [\"react\", \"vue\"]\n}", + ); + assert_eq!(changes.len(), 1); + assert_eq!(changes[0].change_type, ChangeType::Renamed); + assert_eq!(changes[0].entity_name, "dependencies"); + } + + #[test] + fn array_element_keys_are_not_tracked_as_entities() { + let before = r#"{ + "deps": [ + {"name": "react"}, + {"name": "vue"} + ] +}"#; + let after = r#"{ + "deps": [ + {"package": "react"}, + {"name": "vue"} + ] +}"#; + let changes = json_diff(before, after); + assert_eq!(names(&changes), vec![("deps".into(), ChangeType::Modified)], + "array elements have no stable identity; only the array key should change"); + } + + // ───────────────────────────────────────────────────────────────────────── + // Null and empty values + // ───────────────────────────────────────────────────────────────────────── + + #[test] + fn null_to_string_value_reports_modified() { + let changes = json_diff( + "{\n \"key\": null\n}", + "{\n \"key\": \"value\"\n}", + ); + assert_eq!(names(&changes), vec![("key".into(), ChangeType::Modified)]); + } + + #[test] + fn empty_object_gains_child_reports_both_parent_and_child() { + // The precision guard keeps `key` Modified — its declaration shape + // changed from `{}` to `{...}`. + let changes = json_diff( + "{\n \"key\": {}\n}", + "{\n \"key\": {\n \"build\": \"tsc\"\n }\n}", + ); + let key = find_change(&changes, "key", ChangeType::Modified); + assert_eq!(key.parent_name, None); + let build = find_change(&changes, "build", ChangeType::Added); + assert_eq!(build.parent_name.as_deref(), Some("key")); + } + + // ───────────────────────────────────────────────────────────────────────── + // Entity ID format — file::pointer (no entity_type) + // ───────────────────────────────────────────────────────────────────────── + + #[test] + fn entity_id_for_nested_property_uses_full_pointer_only() { + let changes = json_diff( + "{\n \"scripts\": {\n \"build\": \"tsc\"\n }\n}", + "{\n \"scripts\": {\n \"build\": \"webpack\"\n }\n}", + ); + let build = find_change(&changes, "build", ChangeType::Modified); + assert_eq!(build.entity_id, "test.json::/scripts/build"); + } + + + // ───────────────────────────────────────────────────────────────────────── + // Phase 3 fuzzy matching + // ───────────────────────────────────────────────────────────────────────── + + #[test] + fn fuzzy_rename_detected_when_value_mostly_unchanged() { + // config → settings: key rename (Phase 1 & 2 miss). + // testTimeout 30 → 60: small value change rules out structural_hash. + // Many siblings unchanged → Jaccard > 0.8 → Phase 3 catches it. + let before = r#"{ + "config": { + "host": "localhost", + "protocol": "https", + "retries": 3, + "testTimeout": 30, + "keepalive": true, + "compression": true, + "logging": "verbose", + "maxConnections": 100 + } +}"#; + let after = r#"{ + "settings": { + "host": "localhost", + "protocol": "https", + "retries": 3, + "testTimeout": 60, + "keepalive": true, + "compression": true, + "logging": "verbose", + "maxConnections": 100 + } +}"#; + let changes = json_diff(before, after); + assert!(changes.iter().any(|c| c.entity_name == "settings" && c.change_type == ChangeType::Renamed), + "expected fuzzy rename of config → settings; got: {:?}", names(&changes)); + } + + // ───────────────────────────────────────────────────────────────────────── + // Known limitations (documented in spec) + // ───────────────────────────────────────────────────────────────────────── + + #[test] + fn parent_rename_with_sibling_added_surfaces_leaf_moves() { + // Parent renamed AND a new sibling appears: structural_hash diverges, + // Phase 2 misses the parent rename. The unchanged child still matches + // by structural_hash and surfaces as Moved; the parent Deleted/Added + // entries are container-suppressed. + let before = r#"{ + "scripts": { + "build": "tsc" + } +}"#; + let after = r#"{ + "tasks": { + "build": "tsc", + "test": "jest" + } +}"#; + let changes = json_diff(before, after); + let build = find_change(&changes, "build", ChangeType::Moved); + assert_eq!(build.parent_name.as_deref(), Some("tasks")); + assert!(build.old_parent_id.is_some()); + find_change(&changes, "test", ChangeType::Added); + assert!(!changes.iter().any(|c| c.entity_name == "scripts" || c.entity_name == "tasks"), + "parent Deleted/Added should be suppressed; got: {:?}", names(&changes)); + } + + #[test] + fn scalar_array_transitions_report_modified_only() { + // Arrays are opaque, so the type transition surfaces as a single + // Modified entry with entity_type reflecting the after value. + let cases = [ + ("{\n \"deps\": \"react\"\n}", "{\n \"deps\": [\"react\", \"vue\"]\n}", "array"), + ("{\n \"deps\": [\"react\", \"vue\"]\n}", "{\n \"deps\": \"react\"\n}", "property"), + ]; + for (before, after, after_type) in cases { + let changes = json_diff(before, after); + assert_eq!(names(&changes), vec![("deps".into(), ChangeType::Modified)]); + assert_eq!(changes[0].entity_type, after_type); + } + } + + #[test] + fn object_to_array_transition_reports_modified_plus_old_children_deleted() { + let changes = json_diff( + "{\n \"deps\": {\n \"react\": \"18\"\n }\n}", + "{\n \"deps\": [\"react\"]\n}", + ); + let deps = find_change(&changes, "deps", ChangeType::Modified); + assert_eq!(deps.entity_type, "array"); + find_change(&changes, "react", ChangeType::Deleted); + } + + #[test] + fn array_to_object_transition_reports_modified_plus_new_children_added() { + let changes = json_diff( + "{\n \"deps\": [\"react\"]\n}", + "{\n \"deps\": {\n \"react\": \"18\"\n }\n}", + ); + let deps = find_change(&changes, "deps", ChangeType::Modified); + assert_eq!(deps.entity_type, "object"); + let react = find_change(&changes, "react", ChangeType::Added); + assert_eq!(react.parent_name.as_deref(), Some("deps")); + } + + #[test] + fn deep_whole_section_deleted_only_leaf_reported() { + let changes = json_diff( + "{\n \"jest\": {\n \"config\": {\n \"testTimeout\": 5000\n }\n }\n}", + "{}", + ); + let timeout = find_change(&changes, "testTimeout", ChangeType::Deleted); + assert_eq!(timeout.parent_name.as_deref(), Some("jest::config")); + assert!(!changes.iter().any(|c| c.entity_name == "jest" || c.entity_name == "config"), + "intermediate containers should be suppressed; got: {:?}", names(&changes)); + } + + #[test] + fn pointer_escapes_preserve_rfc6901_order() { + // '~' must be escaped before '/'. Otherwise a literal '/' would become + // '~1' and the '~' inside that would then become '~01'. + let cases = [ + ("a/b", "test.json::/a~1b"), + ("a~b", "test.json::/a~0b"), + ("a~/b", "test.json::/a~0~1b"), + ]; + for (key, expected_id) in cases { + let changes = json_diff( + &format!("{{\n \"{key}\": 1\n}}"), + &format!("{{\n \"{key}\": 2\n}}"), + ); + assert_eq!(changes.len(), 1); + assert_eq!(changes[0].entity_id, expected_id, "key {key}"); + } + } + + // ───────────────────────────────────────────────────────────────────────── + // Document-level edge cases + // ───────────────────────────────────────────────────────────────────────── + + #[test] + fn documents_without_extractable_keys_produce_no_entities() { let plugin = JsonParserPlugin; - let before = plugin.extract_entities(before_content, "config.json"); - let after = plugin.extract_entities(after_content, "config.json"); - assert_eq!(before.len(), 1); - assert_eq!(after.len(), 1); - // content_hash differs (key name is part of content) - assert_ne!(before[0].content_hash, after[0].content_hash); - // structural_hash matches (same value) - assert_eq!(before[0].structural_hash, after[0].structural_hash); + for input in ["[1, 2, 3]", "\"hello\"", "42", "null", "{}"] { + assert!( + plugin.extract_entities(input, "test.json").is_empty(), + "input: {input}" + ); + } } #[test] - fn test_renamed_object_property_shares_structural_hash() { - let before_content = "{\n \"config\": {\n \"port\": 8080\n }\n}\n"; - let after_content = "{\n \"settings\": {\n \"port\": 8080\n }\n}\n"; + fn malformed_input_does_not_panic() { let plugin = JsonParserPlugin; - let before = plugin.extract_entities(before_content, "config.json"); - let after = plugin.extract_entities(after_content, "config.json"); - // 1 parent + 1 child ("port") - assert_eq!(before.len(), 2); - assert_eq!(after.len(), 2); - assert_ne!(before[0].content_hash, after[0].content_hash); - assert_eq!(before[0].structural_hash, after[0].structural_hash); + let cases = [ + "{", // unclosed root + "{\"a\":", // dangling colon + "{\"a\": {", // unclosed nested object + "{\"a\": {] }}", // stray ']' inside object value + "{\"a\": {\"b\": [}]}", // mismatched brackets in array + "{\"a\": }}}}", // multiple stray '}' + "{\"a\": {\"b\": 1}, \"c\":", // truncated mid-object + ]; + for input in cases { + let _ = plugin.extract_entities(input, "test.json"); + } + } + + #[test] + fn parent_rename_with_child_value_change_falls_back_to_leaf_delete_add() { + let changes = json_diff( + "{\n \"scripts\": {\n \"dev\": \"vite\"\n }\n}\n", + "{\n \"tasks\": {\n \"dev\": \"rollup\"\n }\n}\n", + ); + find_change(&changes, "dev", ChangeType::Deleted); + find_change(&changes, "dev", ChangeType::Added); + assert!(!changes.iter().any(|c| c.change_type == ChangeType::Renamed), + "rename should not be detectable; got: {:?}", names(&changes)); } }