From a3b96b3c48a7466d336e1db0868645eab4918f02 Mon Sep 17 00:00:00 2001 From: nminev Date: Fri, 1 May 2026 00:43:36 +0200 Subject: [PATCH 1/3] JSON: recursive entity extraction with suppress-parent display MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The JSON parser was depth-2: it surfaced top-level keys and one layer of children, then treated everything deeper as opaque text. A change to package.json's scripts.build surfaced as "scripts modified" with the entire scripts object as before/after content. Builds on the parent_name field merged in #97 and the precision guard merged in #98. Parser is now fully recursive — every key at every depth is an entity identified by JSON Pointer (/scripts/build). Arrays remain opaque (elements have no stable identity), but array-typed keys themselves are entities. JSON entity IDs are file::pointer (entity_type dropped) so a key whose value type changes (scalar↔object↔array) keeps the same ID and matches Phase 1 as Modified instead of Deleted+Added. parent_name traverses parent_id to build the full ancestor chain (e.g. jest::config for an entity at /jest/config/timeout). Empty ancestor names — package-lock.json's "" root-package key — are skipped so displayed paths stay clean. Suppression extends #98's precision check: - "object" joins CONTAINER_TYPES so JSON parents are eligible - The Modified-suppression branch additionally requires entity_type to match across before/after, so scalar↔object value transitions keep the parent change - An additional pass drops Moved entries when the entity's old_parent_id is itself in the change set, catching parent-rename failures where children matched by structural hash and the parent did not Behavior is documented in JSON_SEMANTIC_DIFF_SPEC.md, including the remaining limitation around parent-rename plus content change in the same commit. Adds 38 BDD-style tests in json.rs covering top-level, nested rename/ add/delete, deep nesting, type transitions, array opacity, parent- rename fallbacks, document edge cases, and the empty-string-key case from package-lock.json. All 190 sem-core tests pass. --- JSON_SEMANTIC_DIFF_SPEC.md | 420 ++++++++ crates/sem-core/src/model/identity.rs | 59 +- crates/sem-core/src/parser/differ.rs | 56 +- crates/sem-core/src/parser/plugins/json.rs | 1003 +++++++++++++++----- 4 files changed, 1271 insertions(+), 267 deletions(-) create mode 100644 JSON_SEMANTIC_DIFF_SPEC.md diff --git a/JSON_SEMANTIC_DIFF_SPEC.md b/JSON_SEMANTIC_DIFF_SPEC.md new file mode 100644 index 00000000..a963ec16 --- /dev/null +++ b/JSON_SEMANTIC_DIFF_SPEC.md @@ -0,0 +1,420 @@ +# JSON Semantic Diff — Behaviour Spec + +## What is a JSON entity? + +An entity is a single key-value pair anywhere inside a JSON object. +It has: +- A **JSON Pointer path** as its stable identity within the file (e.g. `/scripts/build`) +- A **parent** — the enclosing entity (or none for top-level keys) +- **content** — the raw `"key": value` text, used for content hashing +- **structural_hash** — a hash of the *value only* (key name stripped), used to detect renames + +--- + +## What we extract entities from + +| JSON structure | Extract entities? | Recurse into children? | +|---|---|---| +| Root object `{ }` | No (root itself is not an entity) | Yes — all top-level keys become entities | +| Object value `"key": { }` | Yes (the key is an entity) | Yes — recurse into the nested object | +| Array value `"key": [ ]` | Yes (the key is an entity) | **No** — array elements have no stable key name | +| Scalar value `"key": "val"` (string, number, boolean, or `null`) | Yes | N/A | +| Root is an array `[ ]` | — | File produces no entities at all | + +--- + +## Entity types + +| Value type | `entity_type` | +|---|---| +| String, number, boolean, null | `property` | +| Object `{ }` | `object` | +| Array `[ ]` | `array` | + +Note: the `entity_type` field is set on each entity but is **not** part of the +entity ID. Two entities at the same JSON Pointer path with different value types +(e.g. scalar → object) share the same ID and are matched as the same entity. + +--- + +## Display format + +Changes are displayed with the **full ancestor chain** as context: + +``` +⊕ property scripts::build [added] +∆ property jest::config::testTimeout [modified] +``` + +The `parent_name` field on a change holds the full `::`-joined chain of ancestor +names (e.g. `"jest::config"` for an entity at `/jest/config/testTimeout`). The +entity's own name is **not** included in `parent_name` — the terminal formatter +combines `parent_name` and `entity_name` to produce the display path. + +For `Renamed` and `Moved` changes, `entity_name` and `parent_name` always +reflect the **after** state, while `old_entity_name` carries the before key +(when changed) and `old_parent_id` carries the before parent (when changed). +Display format: + +| Change type | Display | +|---|---| +| Renamed (same parent, key changed) | `parent_name::old_entity_name -> entity_name` | +| Moved (parent changed, key unchanged) | `parent_name::entity_name`, footer `moved from ` | +| Moved (parent changed, key also changed) | `parent_name::old_entity_name -> entity_name`, footer `moved from ` | + +`` is derived from the `old_parent_id` field by resolving +the ID against the **before** entity set and reading that entity's `name`. +For top-level entities (no parent), the footer is omitted. + +--- + +## Parent suppression + +Object entities (`entity_type = "object"`) act as **containers**. When any child +changes, the parent object is **not** reported as a separate change — only the +children are. The full display path (`parent::child`) gives sufficient context. + +This keeps output focused on what actually changed for large files. + +```json +// before // after +{ "scripts": { "build": "tsc" } } { "scripts": { "build": "webpack" } } +``` +→ `scripts::build` **Modified** +(Not: `scripts Modified` + `scripts::build Modified`) + +Same rule applies to Add/Delete — when a whole object section is added or removed, +only its leaf children are reported, not the container object itself. + +--- + +## Child move suppression + +When a child entity moves only because its parent was renamed (and the child +itself is otherwise unchanged), the child move is **suppressed**. Only the +parent rename is reported. + +A child is "otherwise unchanged" when its key name and value content are the +same; only its `parent_id` changed. A child whose key was also renamed is +**not** suppressed. A child whose value also changed is governed by the +[parent rename + child value change](#parent-rename--child-value-change) +limitation below — its connection to the before entity is lost and it is +reported as Deleted + Added in the new parent path. + +--- + +## Change detection — all cases + +### Top-level scalar + +```json +// before // after +{ "name": "foo" } { "name": "bar" } +``` +→ `name` **Modified** + +```json +{ "name": "foo" } { } +``` +→ `name` **Deleted** + +```json +{ } { "name": "foo" } +``` +→ `name` **Added** + +```json +{ "timeout": 30 } { "testTimeout": 30 } +``` +→ `testTimeout` **Renamed** from `timeout` (structural_hash matches — same value, different key) + +--- + +### Top-level object + +```json +{ "scripts": { "build": "tsc" } } { "scripts": { "build": "webpack" } } +``` +→ `scripts::build` **Modified** + +```json +{ "scripts": { "build": "tsc" } } { } +``` +→ `scripts::build` **Deleted** + +```json +{ } { "scripts": { "build": "tsc" } } +``` +→ `scripts::build` **Added** + +```json +{ "scripts": { "dev": "vite" } } { "tasks": { "dev": "vite" } } +``` +→ `tasks` **Renamed** from `scripts` (structural_hash of object value matches) +(`tasks::dev` is suppressed — `dev` only "moved" because its parent was renamed.) + +--- + +### Nested scalar — rename + +```json +// before // after +{ "scripts": { "run": "node ." } } { "scripts": { "start": "node ." } } +``` +→ `scripts::start` **Renamed** from `run` + +--- + +### Nested scalar — add/delete + +```json +{ "scripts": { "build": "tsc" } } { "scripts": { "build": "tsc", "test": "jest" } } +``` +→ `scripts::test` **Added** + +```json +{ "scripts": { "build": "tsc", "test": "jest" } } { "scripts": { "build": "tsc" } } +``` +→ `scripts::test` **Deleted** + +--- + +### Parent rename + child also renamed + +This case is governed by the +[Parent rename when content also changed](#parent-rename-when-content-also-changed) +limitation — the renamed child key changes the parent's structural_hash, so +the parent rename itself is not detected. The child move surfaces with both +`old_entity_name` and `old_parent_id` populated, conveying the rename. + +--- + +### Scalar ↔ object type change + +A key whose value changes from scalar to object (or vice versa) is reported +as **Modified** — same key path, different value. When the new value is an +object with children (or the old value was), those children are reported +separately as Added/Deleted. Container suppression does **not** apply across +a type transition — both the parent change and the child changes are visible +because the type change itself is meaningful. + +```json +{ "build": "tsc" } { "build": { "command": "tsc" } } +``` +→ `build` **Modified** +→ `build::command` **Added** + +```json +{ "config": { "watch": true } } { "config": "auto" } +``` +→ `config` **Modified** +→ `config::watch` **Deleted** + +The `entity_type` of the change reflects the **after** type (`object` becomes +`property` or vice versa). + +--- + +### Deep nesting (3+ levels) + +```json +// before +{ + "jest": { + "config": { + "testTimeout": 5000 + } + } +} + +// after +{ + "jest": { + "config": { + "testTimeout": 10000 + } + } +} +``` +→ `jest::config::testTimeout` **Modified** +(Intermediate container objects `jest` and `jest::config` are not reported separately.) + +--- + +### Array value — always treated as opaque + +```json +{ "deps": ["react", "vue"] } { "deps": ["react", "vue", "lodash"] } +``` +→ `deps` **Modified** +(No child entities. Array elements are not tracked.) + +```json +{ "deps": [{"name": "react"}] } { "deps": [{"name": "react-dom"}] } +``` +→ `deps` **Modified** +(Array contains objects — we still do not recurse. The whole array is opaque.) + +```json +{ "deps": [{"name": "react"}] } { "dependencies": [{"name": "react"}] } +``` +→ `dependencies` **Renamed** from `deps` (structural_hash of array content matches) + +--- + +### Null and empty object values + +```json +{ "key": null } { "key": "value" } +``` +→ `key` **Modified** + +```json +{ "key": {} } { "key": { "build": "tsc" } } +``` +→ `key` **Modified**, `key::build` **Added** +(The precision guard preserves `key` because its declaration shape changed +from `{}` to `{...}`.) + +--- + +## Matching algorithm (overview) + +Entities in before/after are matched in three phases: + +1. **Phase 1 — exact ID match.** Same entity ID in both sides. If `content_hash` differs → Modified, otherwise unchanged. +2. **Phase 2 — structural_hash match.** Unmatched entities are paired by equal `structural_hash` (same value, different ID). Used for rename and move detection. +3. **Phase 3 — fuzzy similarity.** Remaining unmatched entities are paired by Jaccard similarity above a threshold. Used to recover renames where both the key and value changed slightly. + +Whatever remains unmatched after phase 3 is Deleted (before only) or Added (after only). + +--- + +## Structural hash rules (rename detection) + +The `structural_hash` is computed from the **value only** — the key name is stripped. +This is what allows rename detection. + +| Before | After | content_hash | structural_hash | +|---|---|---|---| +| `"build": "tsc"` | `"compile": "tsc"` | different (key name changed) | **same** → Renamed | +| `"build": "tsc"` | `"build": "webpack"` | different | different → Modified | +| `"scripts": {"dev": "vite"}` | `"tasks": {"dev": "vite"}` | different | **same** → Renamed | +| `"scripts": {"dev": "vite"}` | `"scripts": {"dev": "rollup"}` | different | different → Modified | + +### Tie-breaking on duplicate structural_hash + +When multiple sibling keys share the same value (e.g. several flags all set to +`true`, or several scripts all running the same command), and one or more are +renamed, the spec **does not** guarantee a specific pairing between +identical-value before/after entities. Any pairing produces semantically +equivalent output (same set of names disappeared, same set of names appeared), +so callers MUST treat the result as equivalent regardless of which old name was +paired with which new name. Implementations are free to be stable across runs +on the same input but the spec does not require it. + +--- + +## Entity ID format + +IDs are stable across runs and unique within a file. + +Format: `{file_path}::{json_pointer}` + +Examples: +- `package.json::/name` +- `package.json::/scripts` +- `package.json::/scripts/build` +- `package.json::/deps` + +Rules: +- The JSON Pointer is always the **full absolute path** from the root (e.g. `/scripts/build`, not just `/build`) +- Key names are JSON Pointer-escaped: `~` → `~0`, `/` → `~1` +- The entity type is **not** part of the ID — a key whose value changed type + (scalar ↔ object) keeps the same ID and is matched as Modified +- The parent ID is **not** embedded in the child ID — the full pointer is sufficient to uniquely identify any entity + +--- + +## Known limitations + +### Parent rename when content also changed + +When a parent object is renamed **and** any of its content also changes in +the same commit (a sibling added/removed, a child renamed, or a child value +changed), the parent rename itself cannot be detected. The implementation +falls back to whatever leaf-level matches Phase 2/3 can recover, then +container-suppresses the parent Deleted/Added entries. + +The user can usually still infer the parent rename from a child's +`old_parent_id` (footer "moved from ...") and current `parent_name`. + +#### Sub-case: sibling added/removed + +```json +// before // after +{ { + "scripts": { "tasks": { + "build": "tsc" "build": "tsc", + } "test": "jest" +} } + } +``` + +Output: +``` +→ property tasks::build [moved] moved from scripts +⊕ property tasks::test [added] +``` + +`build` matches by structural_hash → Moved (parent_id changed). `scripts` +Deleted and `tasks` Added are container-suppressed because `build`'s +`old_parent_id` is `scripts` and `test`'s `parent_id` is `tasks`. + +#### Sub-case: child key also renamed + +```json +{ "scripts": { "dev": "vite" } } { "tasks": { "develop": "vite" } } +``` + +Output: +``` +→ property tasks::dev -> develop [moved] moved from scripts +``` + +The renamed child key changes the parent's structural_hash, so the parent +rename is missed. The child still matches by structural_hash (value `"vite"` +unchanged) and surfaces with both `old_entity_name` (the old key) and +`old_parent_id` (the old parent) populated. + +#### Sub-case: child value also changed + +```json +{ "scripts": { "dev": "vite" } } { "tasks": { "dev": "rollup" } } +``` + +Output: +``` +- property scripts::dev [deleted] ++ property tasks::dev [added] +``` + +Both the parent's structural_hash and the child's structural_hash differ; +no Phase 2 match is possible at either level. Phase 3 fuzzy matching may +recover the connection if the surrounding content is similar enough but is +not guaranteed. + +--- + +## Edge cases + +| Case | Behaviour | +|---|---| +| Key name contains `/` e.g. `"a/b": 1` | Pointer-escaped to `/a~1b`. Entity ID: `file::/a~1b` | +| Key name contains `~` e.g. `"a~b": 1` | Pointer-escaped to `/a~0b` | +| Root document is `[]` | No entities produced | +| Root document is a scalar `"hello"` | No entities produced | +| Empty object `{}` | No entities produced | +| Object with empty nested object `{"key": {}}` | One entity: `key` (type `object`, no children) | +| Object with `null` value `{"key": null}` | One entity: `key` (type `property`) | +| Same key, value type changes (scalar ↔ object ↔ array, any combination) | The key is reported as **Modified** (entity_type reflects the after value). Children of the side that is an object — old children if before was an object, new children if after is an object — are reported as Added or Deleted. Container suppression does not apply across a type transition. Arrays remain opaque (no children either side). See [Scalar ↔ object type change](#scalar--object-type-change). | diff --git a/crates/sem-core/src/model/identity.rs b/crates/sem-core/src/model/identity.rs index 5b150b33..87849d98 100644 --- a/crates/sem-core/src/model/identity.rs +++ b/crates/sem-core/src/model/identity.rs @@ -3,10 +3,35 @@ 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 pid = entity.parent_id.as_deref()?; + loop { + 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 +54,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 +75,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 +120,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 +135,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 +205,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 +303,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 +311,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 +367,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 +418,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)); } } } diff --git a/crates/sem-core/src/parser/differ.rs b/crates/sem-core/src/parser/differ.rs index 9d0f7603..3581fe62 100644 --- a/crates/sem-core/src/parser/differ.rs +++ b/crates/sem-core/src/parser/differ.rs @@ -141,12 +141,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], @@ -160,6 +154,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> = @@ -200,11 +195,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 @@ -220,11 +216,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..46c4c41d 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,208 @@ 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('{') { return Vec::new(); } - let lines: Vec<&str> = content.lines().collect(); - let entries = find_top_level_entries(content); - let closing = find_closing_brace_line(&lines); - 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) - }; - - 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); - - entities.push(SemanticEntity { - id: parent_id.clone(), - file_path: file_path.to_string(), - entity_type: entry.entity_type.clone(), - name: entry.key.clone(), - parent_id: None, - content_hash: content_hash(&entity_content), - structural_hash, - content: entity_content.clone(), - start_line: entry.start_line, - end_line, - 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) - }; - - 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, - }); + extract_entries_recursive(content, file_path, 1, None, None, &mut entities); + entities + } +} + +/// Recursively extract entities from a JSON object string. +/// +/// - `content`: the full text of the object (including surrounding `{` `}`) +/// - `file_path`: original file path, threaded through for entity IDs +/// - `line_offset`: 1-based absolute line number of the first line of `content` +/// - `parent_pointer`: JSON Pointer prefix for children, e.g. `Some("/scripts")` +/// - `parent_entity_id`: the entity id of the enclosing entity (for `parent_id` field) +/// - `out`: collected entities, appended in-place (DFS pre-order) +fn extract_entries_recursive( + content: &str, + file_path: &str, + line_offset: usize, + parent_pointer: Option<&str>, + parent_entity_id: Option<&str>, + out: &mut Vec, +) { + let lines: Vec<&str> = content.lines().collect(); + let entries = find_top_level_entries(content); + + 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 { + let closing = find_closing_brace_line(&lines); + trim_trailing_blanks(&lines, entry.start_line, closing) + }; + + 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)); + + // Build JSON Pointer path: parent_pointer + "/" + escaped_key + let pointer = match parent_pointer { + Some(pp) => format!("{pp}{}", entry.pointer), + None => entry.pointer.clone(), + }; + + let abs_start = line_offset + entry.start_line - 1; + let abs_end = line_offset + end_line - 1; + + // JSON entity IDs are file::pointer — entity_type is intentionally not + // part of the ID so that scalar↔object value-type changes match as Modified. + let entity_id = format!("{}::{}", file_path, pointer); + + out.push(SemanticEntity { + id: entity_id.clone(), + file_path: file_path.to_string(), + entity_type: entry.entity_type.clone(), + name: entry.key.clone(), + parent_id: parent_entity_id.map(str::to_string), + content_hash: content_hash(&entity_content), + structural_hash, + content: entity_content.clone(), + start_line: abs_start, + end_line: abs_end, + metadata: None, + }); + + // If this entry is an object, recurse into its value + if entry.entity_type == "object" { + if let Some(obj_str) = extract_object_value(&entity_content) { + // The object value starts at the line with the opening `{`. + // We need to find the absolute line of that `{` inside entity_content. + let obj_line_in_entity = find_value_start_line(&entity_content); + let obj_abs_line = abs_start + obj_line_in_entity - 1; + extract_entries_recursive( + obj_str, + file_path, + obj_abs_line, + Some(&pointer), + Some(&entity_id), + out, + ); + } + } + } +} + +/// 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 `}` + let mut 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 { + '{' | '[' => depth += 1, + '}' | ']' => { + depth -= 1; + if depth == 0 { + return Some(&content[obj_start..obj_start + i + 1]); + } } + _ => {} } } + } + None +} - entities +/// 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; + + 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 +223,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 +267,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 +275,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 +290,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 +300,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 +313,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 +322,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 +376,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 +424,566 @@ 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!(top[0].name, "name"); + assert_eq!(top[0].start_line, 2); + assert_eq!(top[0].end_line, 2); + + assert_eq!(top[1].name, "version"); + assert_eq!(top[1].start_line, 3); + assert_eq!(top[1].end_line, 3); + + 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); + + 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[0].name, "name"); - assert_eq!(entities[0].start_line, 2); - assert_eq!(entities[0].end_line, 2); - assert!(entities[0].parent_id.is_none()); + let scripts = entities.iter().find(|e| e.name == "scripts").unwrap(); + assert!(scripts.parent_id.is_none()); - assert_eq!(entities[1].name, "version"); - assert_eq!(entities[1].start_line, 3); - assert_eq!(entities[1].end_line, 3); + 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); - 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); + 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); + } - // 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)); + // ───────────────────────────────────────────────────────────────────────── + // Top-level scalars + // ───────────────────────────────────────────────────────────────────────── - 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)); + #[test] + 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); + } - assert_eq!(entities[5].name, "description"); - assert_eq!(entities[5].start_line, 8); - assert_eq!(entities[5].end_line, 8); + #[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 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 test_rename_detected_end_to_end() { - let before_content = "{\n \"timeout\": 30\n}\n"; - let after_content = "{\n \"request_timeout\": 30\n}\n"; + 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"); + } + + #[test] + fn key_with_slash_is_pointer_escaped_in_entity_id() { + let changes = json_diff( + "{\n \"a/b\": 1\n}", + "{\n \"a/b\": 2\n}", + ); + assert_eq!(changes.len(), 1); + assert_eq!(changes[0].entity_id, "test.json::/a~1b"); + } + + #[test] + fn key_with_tilde_is_pointer_escaped_in_entity_id() { + let changes = json_diff( + "{\n \"a~b\": 1\n}", + "{\n \"a~b\": 2\n}", + ); + assert_eq!(changes.len(), 1); + assert_eq!(changes[0].entity_id, "test.json::/a~0b"); + } + + // ───────────────────────────────────────────────────────────────────────── + // 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_to_array_transition_reports_modified_only() { + // Arrays are opaque so no children are produced on either side. + let changes = json_diff( + "{\n \"deps\": \"react\"\n}", + "{\n \"deps\": [\"react\", \"vue\"]\n}", + ); + assert_eq!(names(&changes), vec![("deps".into(), ChangeType::Modified)]); + assert_eq!(changes[0].entity_type, "array"); + } + + #[test] + fn array_to_scalar_transition_reports_modified_only() { + let changes = json_diff( + "{\n \"deps\": [\"react\", \"vue\"]\n}", + "{\n \"deps\": \"react\"\n}", + ); + assert_eq!(names(&changes), vec![("deps".into(), ChangeType::Modified)]); + assert_eq!(changes[0].entity_type, "property"); + } + + #[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 key_with_both_tilde_and_slash_is_pointer_escaped_in_correct_order() { + // Per RFC 6901, '~' must be escaped before '/' so 'a~/b' becomes + // 'a~0~1b' — not 'a~01b' which would happen if '/' were escaped first. + let changes = json_diff( + "{\n \"a~/b\": 1\n}", + "{\n \"a~/b\": 2\n}", + ); + assert_eq!(changes.len(), 1); + assert_eq!(changes[0].entity_id, "test.json::/a~0~1b"); + } + + // ───────────────────────────────────────────────────────────────────────── + // Document-level edge cases + // ───────────────────────────────────────────────────────────────────────── + + #[test] + fn root_array_document_produces_no_entities() { 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"); + let entities = plugin.extract_entities("[1, 2, 3]", "test.json"); + assert!(entities.is_empty()); } #[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 root_scalar_document_produces_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); + assert!(plugin.extract_entities("\"hello\"", "test.json").is_empty()); + assert!(plugin.extract_entities("42", "test.json").is_empty()); + assert!(plugin.extract_entities("null", "test.json").is_empty()); } #[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 empty_root_object_produces_no_entities() { 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); + assert!(plugin.extract_entities("{}", "test.json").is_empty()); + } + + #[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)); } } From e8592d3de4df2c6f5ae69409f4d48b4eb8776d6d Mon Sep 17 00:00:00 2001 From: nminev Date: Sat, 9 May 2026 14:27:40 +0300 Subject: [PATCH 2/3] Convert JSON entity extraction to iterative; address review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per @rs545837's request, convert the recursive `extract_entries_recursive` to iterative (matches the pattern in b9384a0). The new `extract_entries` uses a Vec worklist with cursor-based mid-iteration suspension, so DFS pre-order is preserved bit-for-bit — entity Vec output is identical to the previous recursive version on all existing test inputs. Also fixes the two inspect bot findings on `extract_object_value`: the function now returns Some(slice) only on a closing '}' (a stray ']' no longer terminates extraction at depth 0), and the depth decrement uses saturating_sub on both arms. Trim pass per the maintainer's "things that can be cut down" request: - Drop JSON_SEMANTIC_DIFF_SPEC.md (-420 lines) - Three pointer-escape tests collapse into one table-driven test - Two scalar↔array transition tests collapse into one - Three root-document-edge tests collapse into one (with `{}` added) - Add malformed_input_does_not_panic covering 7 malformed inputs to pin down the saturating_sub safety property Net: -459 lines across the two files. 186 sem-core tests pass. --- JSON_SEMANTIC_DIFF_SPEC.md | 420 --------------------- crates/sem-core/src/parser/plugins/json.rs | 273 +++++++------- 2 files changed, 132 insertions(+), 561 deletions(-) delete mode 100644 JSON_SEMANTIC_DIFF_SPEC.md diff --git a/JSON_SEMANTIC_DIFF_SPEC.md b/JSON_SEMANTIC_DIFF_SPEC.md deleted file mode 100644 index a963ec16..00000000 --- a/JSON_SEMANTIC_DIFF_SPEC.md +++ /dev/null @@ -1,420 +0,0 @@ -# JSON Semantic Diff — Behaviour Spec - -## What is a JSON entity? - -An entity is a single key-value pair anywhere inside a JSON object. -It has: -- A **JSON Pointer path** as its stable identity within the file (e.g. `/scripts/build`) -- A **parent** — the enclosing entity (or none for top-level keys) -- **content** — the raw `"key": value` text, used for content hashing -- **structural_hash** — a hash of the *value only* (key name stripped), used to detect renames - ---- - -## What we extract entities from - -| JSON structure | Extract entities? | Recurse into children? | -|---|---|---| -| Root object `{ }` | No (root itself is not an entity) | Yes — all top-level keys become entities | -| Object value `"key": { }` | Yes (the key is an entity) | Yes — recurse into the nested object | -| Array value `"key": [ ]` | Yes (the key is an entity) | **No** — array elements have no stable key name | -| Scalar value `"key": "val"` (string, number, boolean, or `null`) | Yes | N/A | -| Root is an array `[ ]` | — | File produces no entities at all | - ---- - -## Entity types - -| Value type | `entity_type` | -|---|---| -| String, number, boolean, null | `property` | -| Object `{ }` | `object` | -| Array `[ ]` | `array` | - -Note: the `entity_type` field is set on each entity but is **not** part of the -entity ID. Two entities at the same JSON Pointer path with different value types -(e.g. scalar → object) share the same ID and are matched as the same entity. - ---- - -## Display format - -Changes are displayed with the **full ancestor chain** as context: - -``` -⊕ property scripts::build [added] -∆ property jest::config::testTimeout [modified] -``` - -The `parent_name` field on a change holds the full `::`-joined chain of ancestor -names (e.g. `"jest::config"` for an entity at `/jest/config/testTimeout`). The -entity's own name is **not** included in `parent_name` — the terminal formatter -combines `parent_name` and `entity_name` to produce the display path. - -For `Renamed` and `Moved` changes, `entity_name` and `parent_name` always -reflect the **after** state, while `old_entity_name` carries the before key -(when changed) and `old_parent_id` carries the before parent (when changed). -Display format: - -| Change type | Display | -|---|---| -| Renamed (same parent, key changed) | `parent_name::old_entity_name -> entity_name` | -| Moved (parent changed, key unchanged) | `parent_name::entity_name`, footer `moved from ` | -| Moved (parent changed, key also changed) | `parent_name::old_entity_name -> entity_name`, footer `moved from ` | - -`` is derived from the `old_parent_id` field by resolving -the ID against the **before** entity set and reading that entity's `name`. -For top-level entities (no parent), the footer is omitted. - ---- - -## Parent suppression - -Object entities (`entity_type = "object"`) act as **containers**. When any child -changes, the parent object is **not** reported as a separate change — only the -children are. The full display path (`parent::child`) gives sufficient context. - -This keeps output focused on what actually changed for large files. - -```json -// before // after -{ "scripts": { "build": "tsc" } } { "scripts": { "build": "webpack" } } -``` -→ `scripts::build` **Modified** -(Not: `scripts Modified` + `scripts::build Modified`) - -Same rule applies to Add/Delete — when a whole object section is added or removed, -only its leaf children are reported, not the container object itself. - ---- - -## Child move suppression - -When a child entity moves only because its parent was renamed (and the child -itself is otherwise unchanged), the child move is **suppressed**. Only the -parent rename is reported. - -A child is "otherwise unchanged" when its key name and value content are the -same; only its `parent_id` changed. A child whose key was also renamed is -**not** suppressed. A child whose value also changed is governed by the -[parent rename + child value change](#parent-rename--child-value-change) -limitation below — its connection to the before entity is lost and it is -reported as Deleted + Added in the new parent path. - ---- - -## Change detection — all cases - -### Top-level scalar - -```json -// before // after -{ "name": "foo" } { "name": "bar" } -``` -→ `name` **Modified** - -```json -{ "name": "foo" } { } -``` -→ `name` **Deleted** - -```json -{ } { "name": "foo" } -``` -→ `name` **Added** - -```json -{ "timeout": 30 } { "testTimeout": 30 } -``` -→ `testTimeout` **Renamed** from `timeout` (structural_hash matches — same value, different key) - ---- - -### Top-level object - -```json -{ "scripts": { "build": "tsc" } } { "scripts": { "build": "webpack" } } -``` -→ `scripts::build` **Modified** - -```json -{ "scripts": { "build": "tsc" } } { } -``` -→ `scripts::build` **Deleted** - -```json -{ } { "scripts": { "build": "tsc" } } -``` -→ `scripts::build` **Added** - -```json -{ "scripts": { "dev": "vite" } } { "tasks": { "dev": "vite" } } -``` -→ `tasks` **Renamed** from `scripts` (structural_hash of object value matches) -(`tasks::dev` is suppressed — `dev` only "moved" because its parent was renamed.) - ---- - -### Nested scalar — rename - -```json -// before // after -{ "scripts": { "run": "node ." } } { "scripts": { "start": "node ." } } -``` -→ `scripts::start` **Renamed** from `run` - ---- - -### Nested scalar — add/delete - -```json -{ "scripts": { "build": "tsc" } } { "scripts": { "build": "tsc", "test": "jest" } } -``` -→ `scripts::test` **Added** - -```json -{ "scripts": { "build": "tsc", "test": "jest" } } { "scripts": { "build": "tsc" } } -``` -→ `scripts::test` **Deleted** - ---- - -### Parent rename + child also renamed - -This case is governed by the -[Parent rename when content also changed](#parent-rename-when-content-also-changed) -limitation — the renamed child key changes the parent's structural_hash, so -the parent rename itself is not detected. The child move surfaces with both -`old_entity_name` and `old_parent_id` populated, conveying the rename. - ---- - -### Scalar ↔ object type change - -A key whose value changes from scalar to object (or vice versa) is reported -as **Modified** — same key path, different value. When the new value is an -object with children (or the old value was), those children are reported -separately as Added/Deleted. Container suppression does **not** apply across -a type transition — both the parent change and the child changes are visible -because the type change itself is meaningful. - -```json -{ "build": "tsc" } { "build": { "command": "tsc" } } -``` -→ `build` **Modified** -→ `build::command` **Added** - -```json -{ "config": { "watch": true } } { "config": "auto" } -``` -→ `config` **Modified** -→ `config::watch` **Deleted** - -The `entity_type` of the change reflects the **after** type (`object` becomes -`property` or vice versa). - ---- - -### Deep nesting (3+ levels) - -```json -// before -{ - "jest": { - "config": { - "testTimeout": 5000 - } - } -} - -// after -{ - "jest": { - "config": { - "testTimeout": 10000 - } - } -} -``` -→ `jest::config::testTimeout` **Modified** -(Intermediate container objects `jest` and `jest::config` are not reported separately.) - ---- - -### Array value — always treated as opaque - -```json -{ "deps": ["react", "vue"] } { "deps": ["react", "vue", "lodash"] } -``` -→ `deps` **Modified** -(No child entities. Array elements are not tracked.) - -```json -{ "deps": [{"name": "react"}] } { "deps": [{"name": "react-dom"}] } -``` -→ `deps` **Modified** -(Array contains objects — we still do not recurse. The whole array is opaque.) - -```json -{ "deps": [{"name": "react"}] } { "dependencies": [{"name": "react"}] } -``` -→ `dependencies` **Renamed** from `deps` (structural_hash of array content matches) - ---- - -### Null and empty object values - -```json -{ "key": null } { "key": "value" } -``` -→ `key` **Modified** - -```json -{ "key": {} } { "key": { "build": "tsc" } } -``` -→ `key` **Modified**, `key::build` **Added** -(The precision guard preserves `key` because its declaration shape changed -from `{}` to `{...}`.) - ---- - -## Matching algorithm (overview) - -Entities in before/after are matched in three phases: - -1. **Phase 1 — exact ID match.** Same entity ID in both sides. If `content_hash` differs → Modified, otherwise unchanged. -2. **Phase 2 — structural_hash match.** Unmatched entities are paired by equal `structural_hash` (same value, different ID). Used for rename and move detection. -3. **Phase 3 — fuzzy similarity.** Remaining unmatched entities are paired by Jaccard similarity above a threshold. Used to recover renames where both the key and value changed slightly. - -Whatever remains unmatched after phase 3 is Deleted (before only) or Added (after only). - ---- - -## Structural hash rules (rename detection) - -The `structural_hash` is computed from the **value only** — the key name is stripped. -This is what allows rename detection. - -| Before | After | content_hash | structural_hash | -|---|---|---|---| -| `"build": "tsc"` | `"compile": "tsc"` | different (key name changed) | **same** → Renamed | -| `"build": "tsc"` | `"build": "webpack"` | different | different → Modified | -| `"scripts": {"dev": "vite"}` | `"tasks": {"dev": "vite"}` | different | **same** → Renamed | -| `"scripts": {"dev": "vite"}` | `"scripts": {"dev": "rollup"}` | different | different → Modified | - -### Tie-breaking on duplicate structural_hash - -When multiple sibling keys share the same value (e.g. several flags all set to -`true`, or several scripts all running the same command), and one or more are -renamed, the spec **does not** guarantee a specific pairing between -identical-value before/after entities. Any pairing produces semantically -equivalent output (same set of names disappeared, same set of names appeared), -so callers MUST treat the result as equivalent regardless of which old name was -paired with which new name. Implementations are free to be stable across runs -on the same input but the spec does not require it. - ---- - -## Entity ID format - -IDs are stable across runs and unique within a file. - -Format: `{file_path}::{json_pointer}` - -Examples: -- `package.json::/name` -- `package.json::/scripts` -- `package.json::/scripts/build` -- `package.json::/deps` - -Rules: -- The JSON Pointer is always the **full absolute path** from the root (e.g. `/scripts/build`, not just `/build`) -- Key names are JSON Pointer-escaped: `~` → `~0`, `/` → `~1` -- The entity type is **not** part of the ID — a key whose value changed type - (scalar ↔ object) keeps the same ID and is matched as Modified -- The parent ID is **not** embedded in the child ID — the full pointer is sufficient to uniquely identify any entity - ---- - -## Known limitations - -### Parent rename when content also changed - -When a parent object is renamed **and** any of its content also changes in -the same commit (a sibling added/removed, a child renamed, or a child value -changed), the parent rename itself cannot be detected. The implementation -falls back to whatever leaf-level matches Phase 2/3 can recover, then -container-suppresses the parent Deleted/Added entries. - -The user can usually still infer the parent rename from a child's -`old_parent_id` (footer "moved from ...") and current `parent_name`. - -#### Sub-case: sibling added/removed - -```json -// before // after -{ { - "scripts": { "tasks": { - "build": "tsc" "build": "tsc", - } "test": "jest" -} } - } -``` - -Output: -``` -→ property tasks::build [moved] moved from scripts -⊕ property tasks::test [added] -``` - -`build` matches by structural_hash → Moved (parent_id changed). `scripts` -Deleted and `tasks` Added are container-suppressed because `build`'s -`old_parent_id` is `scripts` and `test`'s `parent_id` is `tasks`. - -#### Sub-case: child key also renamed - -```json -{ "scripts": { "dev": "vite" } } { "tasks": { "develop": "vite" } } -``` - -Output: -``` -→ property tasks::dev -> develop [moved] moved from scripts -``` - -The renamed child key changes the parent's structural_hash, so the parent -rename is missed. The child still matches by structural_hash (value `"vite"` -unchanged) and surfaces with both `old_entity_name` (the old key) and -`old_parent_id` (the old parent) populated. - -#### Sub-case: child value also changed - -```json -{ "scripts": { "dev": "vite" } } { "tasks": { "dev": "rollup" } } -``` - -Output: -``` -- property scripts::dev [deleted] -+ property tasks::dev [added] -``` - -Both the parent's structural_hash and the child's structural_hash differ; -no Phase 2 match is possible at either level. Phase 3 fuzzy matching may -recover the connection if the surrounding content is similar enough but is -not guaranteed. - ---- - -## Edge cases - -| Case | Behaviour | -|---|---| -| Key name contains `/` e.g. `"a/b": 1` | Pointer-escaped to `/a~1b`. Entity ID: `file::/a~1b` | -| Key name contains `~` e.g. `"a~b": 1` | Pointer-escaped to `/a~0b` | -| Root document is `[]` | No entities produced | -| Root document is a scalar `"hello"` | No entities produced | -| Empty object `{}` | No entities produced | -| Object with empty nested object `{"key": {}}` | One entity: `key` (type `object`, no children) | -| Object with `null` value `{"key": null}` | One entity: `key` (type `property`) | -| Same key, value type changes (scalar ↔ object ↔ array, any combination) | The key is reported as **Modified** (entity_type reflects the after value). Children of the side that is an object — old children if before was an object, new children if after is an object — are reported as Added or Deleted. Container suppression does not apply across a type transition. Arrays remain opaque (no children either side). See [Scalar ↔ object type change](#scalar--object-type-change). | diff --git a/crates/sem-core/src/parser/plugins/json.rs b/crates/sem-core/src/parser/plugins/json.rs index 46c4c41d..7b472ded 100644 --- a/crates/sem-core/src/parser/plugins/json.rs +++ b/crates/sem-core/src/parser/plugins/json.rs @@ -14,95 +14,94 @@ impl SemanticParserPlugin for JsonParserPlugin { } fn extract_entities(&self, content: &str, file_path: &str) -> Vec { - let trimmed = content.trim(); - if !trimmed.starts_with('{') { + if !content.trim().starts_with('{') { return Vec::new(); } - - let mut entities = Vec::new(); - extract_entries_recursive(content, file_path, 1, None, None, &mut entities); - entities + extract_entries(content, file_path) } } -/// Recursively extract entities from a JSON object string. -/// -/// - `content`: the full text of the object (including surrounding `{` `}`) -/// - `file_path`: original file path, threaded through for entity IDs -/// - `line_offset`: 1-based absolute line number of the first line of `content` -/// - `parent_pointer`: JSON Pointer prefix for children, e.g. `Some("/scripts")` -/// - `parent_entity_id`: the entity id of the enclosing entity (for `parent_id` field) -/// - `out`: collected entities, appended in-place (DFS pre-order) -fn extract_entries_recursive( - content: &str, - file_path: &str, +struct Frame { + content: String, + entries: Vec, + cursor: usize, line_offset: usize, - parent_pointer: Option<&str>, - parent_entity_id: Option<&str>, - out: &mut Vec, -) { - let lines: Vec<&str> = content.lines().collect(); - let entries = find_top_level_entries(content); - - 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 { - let closing = find_closing_brace_line(&lines); - trim_trailing_blanks(&lines, entry.start_line, closing) - }; - - 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)); - - // Build JSON Pointer path: parent_pointer + "/" + escaped_key - let pointer = match parent_pointer { - Some(pp) => format!("{pp}{}", entry.pointer), - None => entry.pointer.clone(), - }; - - let abs_start = line_offset + entry.start_line - 1; - let abs_end = line_offset + end_line - 1; - - // JSON entity IDs are file::pointer — entity_type is intentionally not - // part of the ID so that scalar↔object value-type changes match as Modified. - let entity_id = format!("{}::{}", file_path, pointer); - - out.push(SemanticEntity { - id: entity_id.clone(), - file_path: file_path.to_string(), - entity_type: entry.entity_type.clone(), - name: entry.key.clone(), - parent_id: parent_entity_id.map(str::to_string), - content_hash: content_hash(&entity_content), - structural_hash, - content: entity_content.clone(), - start_line: abs_start, - end_line: abs_end, - metadata: None, - }); - - // If this entry is an object, recurse into its value - if entry.entity_type == "object" { - if let Some(obj_str) = extract_object_value(&entity_content) { - // The object value starts at the line with the opening `{`. - // We need to find the absolute line of that `{` inside entity_content. - let obj_line_in_entity = find_value_start_line(&entity_content); - let obj_abs_line = abs_start + obj_line_in_entity - 1; - extract_entries_recursive( - obj_str, - file_path, - obj_abs_line, - Some(&pointer), - Some(&entity_id), - out, - ); + parent_pointer: Option, + parent_entity_id: Option, +} + +/// 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); + + 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 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: entity_id.clone(), + file_path: file_path.to_string(), + entity_type: entry.entity_type.clone(), + name: entry.key.clone(), + parent_id: frame.parent_entity_id.clone(), + content_hash: content_hash(&entity_content), + structural_hash: Some(content_hash(value_content)), + content: entity_content.clone(), + start_line: abs_start, + end_line: abs_end, + metadata: None, + }); + + if entry.entity_type == "object" { + 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; + } } } } + + entities } /// Given an entity content string like ` "scripts": {\n "build": "tsc"\n }`, @@ -158,12 +157,15 @@ fn extract_object_value(content: &str) -> Option<&str> { if !in_string { match ch { '{' | '[' => depth += 1, - '}' | ']' => { - depth -= 1; + '}' => { + depth = depth.saturating_sub(1); if depth == 0 { return Some(&content[obj_start..obj_start + i + 1]); } } + ']' => { + depth = depth.saturating_sub(1); + } _ => {} } } @@ -794,25 +796,6 @@ mod tests { assert_eq!(build.entity_id, "test.json::/scripts/build"); } - #[test] - fn key_with_slash_is_pointer_escaped_in_entity_id() { - let changes = json_diff( - "{\n \"a/b\": 1\n}", - "{\n \"a/b\": 2\n}", - ); - assert_eq!(changes.len(), 1); - assert_eq!(changes[0].entity_id, "test.json::/a~1b"); - } - - #[test] - fn key_with_tilde_is_pointer_escaped_in_entity_id() { - let changes = json_diff( - "{\n \"a~b\": 1\n}", - "{\n \"a~b\": 2\n}", - ); - assert_eq!(changes.len(), 1); - assert_eq!(changes[0].entity_id, "test.json::/a~0b"); - } // ───────────────────────────────────────────────────────────────────────── // Phase 3 fuzzy matching @@ -883,24 +866,18 @@ mod tests { } #[test] - fn scalar_to_array_transition_reports_modified_only() { - // Arrays are opaque so no children are produced on either side. - let changes = json_diff( - "{\n \"deps\": \"react\"\n}", - "{\n \"deps\": [\"react\", \"vue\"]\n}", - ); - assert_eq!(names(&changes), vec![("deps".into(), ChangeType::Modified)]); - assert_eq!(changes[0].entity_type, "array"); - } - - #[test] - fn array_to_scalar_transition_reports_modified_only() { - let changes = json_diff( - "{\n \"deps\": [\"react\", \"vue\"]\n}", - "{\n \"deps\": \"react\"\n}", - ); - assert_eq!(names(&changes), vec![("deps".into(), ChangeType::Modified)]); - assert_eq!(changes[0].entity_type, "property"); + 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] @@ -939,15 +916,22 @@ mod tests { } #[test] - fn key_with_both_tilde_and_slash_is_pointer_escaped_in_correct_order() { - // Per RFC 6901, '~' must be escaped before '/' so 'a~/b' becomes - // 'a~0~1b' — not 'a~01b' which would happen if '/' were escaped first. - let changes = json_diff( - "{\n \"a~/b\": 1\n}", - "{\n \"a~/b\": 2\n}", - ); - assert_eq!(changes.len(), 1); - assert_eq!(changes[0].entity_id, "test.json::/a~0~1b"); + 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}"); + } } // ───────────────────────────────────────────────────────────────────────── @@ -955,24 +939,31 @@ mod tests { // ───────────────────────────────────────────────────────────────────────── #[test] - fn root_array_document_produces_no_entities() { - let plugin = JsonParserPlugin; - let entities = plugin.extract_entities("[1, 2, 3]", "test.json"); - assert!(entities.is_empty()); - } - - #[test] - fn root_scalar_document_produces_no_entities() { + fn documents_without_extractable_keys_produce_no_entities() { let plugin = JsonParserPlugin; - assert!(plugin.extract_entities("\"hello\"", "test.json").is_empty()); - assert!(plugin.extract_entities("42", "test.json").is_empty()); - assert!(plugin.extract_entities("null", "test.json").is_empty()); + for input in ["[1, 2, 3]", "\"hello\"", "42", "null", "{}"] { + assert!( + plugin.extract_entities(input, "test.json").is_empty(), + "input: {input}" + ); + } } #[test] - fn empty_root_object_produces_no_entities() { + fn malformed_input_does_not_panic() { let plugin = JsonParserPlugin; - assert!(plugin.extract_entities("{}", "test.json").is_empty()); + 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] From 3828bc149c52f90fe0a7c7cafc04e9c3574c0523 Mon Sep 17 00:00:00 2001 From: nminev Date: Sat, 9 May 2026 14:35:31 +0300 Subject: [PATCH 3/3] Address two new inspect-bot findings - extract_object_value: track brace and bracket depth separately, so a stray ']' can't terminate object extraction at brace_depth == 0. Returns Some(slice) only when both depths are zero on a closing '}'. - parent_name: guard the parent_id walk with a visited HashSet so a cyclic chain in malformed entity graphs can't loop forever. Adds parent_name_terminates_on_cyclic_parent_id to lock in the cycle guard. The bracket-depth fix is exercised by the existing tests that mix arrays with nested objects (top-level coverage); malformed inputs are covered by malformed_input_does_not_panic. --- crates/sem-core/src/model/identity.rs | 22 ++++++++++++++++++++++ crates/sem-core/src/parser/plugins/json.rs | 17 +++++++++-------- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/crates/sem-core/src/model/identity.rs b/crates/sem-core/src/model/identity.rs index 87849d98..32fc874f 100644 --- a/crates/sem-core/src/model/identity.rs +++ b/crates/sem-core/src/model/identity.rs @@ -8,8 +8,12 @@ fn parent_name( 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 @@ -765,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/plugins/json.rs b/crates/sem-core/src/parser/plugins/json.rs index 7b472ded..f06dc753 100644 --- a/crates/sem-core/src/parser/plugins/json.rs +++ b/crates/sem-core/src/parser/plugins/json.rs @@ -136,8 +136,10 @@ fn extract_object_value(content: &str) -> Option<&str> { let brace_offset = after_colon.find('{')?; let obj_start = colon_pos? + 1 + brace_offset; - // Find the matching `}` - let mut depth = 0usize; + // 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; @@ -156,16 +158,15 @@ fn extract_object_value(content: &str) -> Option<&str> { } if !in_string { match ch { - '{' | '[' => depth += 1, + '{' => brace_depth += 1, + '[' => bracket_depth += 1, '}' => { - depth = depth.saturating_sub(1); - if depth == 0 { + brace_depth = brace_depth.saturating_sub(1); + if brace_depth == 0 && bracket_depth == 0 { return Some(&content[obj_start..obj_start + i + 1]); } } - ']' => { - depth = depth.saturating_sub(1); - } + ']' => bracket_depth = bracket_depth.saturating_sub(1), _ => {} } }