JSON: recursive entity extraction with suppress-parent display

[nminev]

Why

Today the JSON parser is depth-2: it surfaces top-level keys and one layer of children, then treats everything deeper as opaque text. A package.json edit to scripts.build reports as scripts modified with the entire scripts object as the before/after content — no signal about which script actually changed. For agent and CI consumers parsing the JSON output, this is essentially noise; in the diff-so-fancy integration discussed in #78 it's the difference between "a script changed" and "build was changed."

Builds on the parent_name field merged in #97 and the precision guard merged in #98. Rebased onto current main after both landed.

What changes

Parser

• Recurse into every nested object; every key at every depth becomes an entity.
• JSON Pointer (/scripts/build, /jest/config/testTimeout) is the stable identity.
• Arrays stay opaque (elements have no stable identity), but array-typed keys are still entities and rename-detect via structural hash.
• Entity ID format: {file}::{pointer} instead of {file}::{type}::{pointer}. A key whose value type changes (scalar ↔ object ↔ array) keeps the same ID and matches Phase 1 as Modified instead of Deleted + Added. This is the only breaking output change.

parent_name traversal

• Builds the full ancestor chain by walking parent_id (e.g. jest::config for an entity at /jest/config/testTimeout), instead of returning only the leaf segment of the immediate parent's ID.
• Skips empty ancestor names — package-lock.json uses "" as the root-project key, which previously rendered as packages::::dependencies (double colon). The full pointer is still recoverable from entity_id; only the displayed chain drops the empty segment.

Suppression extends #98's precision check

• Adds \"object\" to CONTAINER_TYPES so a JSON parent isn't reported alongside a child change of its own. With recursion this matters more than before — without it, every leaf change cascades a chain of parent Modifieds.
• The Modified-suppression branch additionally requires entity_type to match across before/after, so scalar↔object value-type transitions keep the parent change (the type change itself is meaningful).
• An additional pass drops Moved entries when the entity's old_parent_id is in the change set. This catches the case where a parent rename (e.g. scripts → tasks) couldn't be matched as Renamed because its children also changed, but the children themselves matched by structural hash and surfaced as Moved. Without this, the rename-failure surfaces both the parent Deleted/Added and the child Moves; with it, only the leaf Moves remain — and consumers can infer the parent rename from the children's old_parent_id + new parent_name.

Spec doc

• JSON_SEMANTIC_DIFF_SPEC.md documents the behavior end-to-end: entity extraction rules, change cases, display format, the matching algorithm overview, and known limitations (chiefly the parent-rename + content change case).

Tests

38 BDD-style tests in json.rs covering top-level changes, 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. They use compute_semantic_diff (the public pipeline) so they assert observable behavior rather than internal state.

All 190 sem-core tests pass (188 pre-existing + 38 new − 36 reorganized).

Notes

• The behavioral change to suppress JSON object parents directly contradicts the previous spec language describing double-reporting as "intentional, both useful." For non-JSON code with depth-1 parent-child structures this is unchanged; the new behavior only kicks in for JSON's recursive depth where double-reporting cascades into 3+ entries per logical change. Open to any framing the maintainers prefer here, including splitting suppression off into a follow-up PR if you'd rather merge the parser improvement first.
• Discussed in Discussion about integrating sem into diff-so-fancy #78 in the context of the diff-so-fancy integration — the behaviors here are most visible in summary headers showing many small changes.

Test plan

• CI green
• Spot-check with a real package.json / tsconfig.json / pnpm-lock.yaml diff to confirm output looks right
• Confirm non-JSON entity extraction is untouched (190 sem-core tests pass)

[inspect-review]

inspect review

Triage: 92 entities analyzed | 0 critical, 0 high, 21 medium, 71 low
Verdict: standard_review

Findings (2)

1. [low] extract_object_value can return an incorrect slice because it decrements depth on both '}' and ']' and returns when depth reaches 0, even though it is documented to extract an object value delimited by '{' ... matching '}'. Evidence: in extract_object_value: match ch { '{' | '[' => depth += 1, '}' | ']' => { depth -= 1; if depth == 0 { return Some(...) }}}. This allows a closing ']' to terminate extraction early if nesting balances to 0 on an array close, truncating the object slice.
2. [low] extract_object_value can panic via usize underflow when encountering a closing '}' or ']' while depth == 0 (malformed/unexpected input), because it does depth -= 1 on a usize without guarding. Evidence: let mut depth = 0usize; ... '}' | ']' => { depth -= 1, if depth == 0 { ... } }.

---

Reviewed by inspect | Entity-level triage found 0 high-risk changes

[rs545837]

could you rebase the PR to main?

[inspect-review]

inspect review

Triage: 85 entities analyzed | 0 critical, 0 high, 14 medium, 71 low
Verdict: standard_review

Findings (0)

---

Reviewed by inspect | Entity-level triage found 0 high-risk changes

[nminev]

Just rebased onto current main — brings in #98's precision guard. Force-pushed a3b96b3.

The Modified-suppression path now uses strip_children_content from #98; the only addition there is an entity_type mismatch check so a JSON value-type transition (scalar ↔ object) keeps the parent change instead of being suppressed. The other two passes (suppress an old parent left behind by a Moved child; drop "stale" Moved entries when the parent renamed) are layered on top of the existing logic.

Re: the inspect bot findings on extract_object_value — both are valid (a stray closing ] could terminate extraction early at depth 0, and the depth -= 1 on usize is a panic risk on malformed input). Happy to push a small fix in this PR or split it out as a follow-up — let me know which you prefer.

[rs545837]

Thanks a lot for the thorough work here, the deep entity extraction for JSON is a clear improvement over the depth-2 approach.

One concern: we recently converted all AST walkers from recursive to iterative (b9384a0, fixing #103) to avoid stack overflows on deeply nested trees. This PR reintroduces recursion for JSON entity extraction, which could hit the same issue on files like package-lock.json or pnpm-lock.yaml that can nest hundreds of levels deep.

Could you convert the recursive traversal to an iterative worklist to stay consistent with the rest of the codebase?

Also I know that the PR might have some long code, so would appreciate if there are things that can be cut down.
I am planning to remove some wordy code here and there in sem, which can help everyone maintain it.

[nminev]

Pushed e8592d3 on top — iterative conversion plus the trim pass. Fast-forward, no force-push.

extract_entries_recursive is now extract_entries with a Vec<Frame> worklist (matches the pattern in b9384a0). Each frame carries a cursor through its entries; encountering an object child pushes the parent frame back with the cursor advanced and the child frame on top, then breaks — so the child is popped next, processed fully, and the parent resumes at the next sibling. DFS pre-order is preserved bit-for-bit, so match_entities sees the same entity Vec ordering as before.

The two inspect-bot findings on extract_object_value are addressed in the same commit: the slice is now returned only on a closing } at depth 0 (a stray ] no longer terminates early), and depth -= 1 is replaced with saturating_sub on both arms. Added malformed_input_does_not_panic that runs seven malformed inputs ({, {"a":, {"a": {, {"a": {] }}, {"a": {"b": [}]}, {"a": }}}}, {"a": {"b": 1}, "c":) through the public API to lock in the safety property — would catch a regression if saturating_sub is removed.

Trim pass: dropped JSON_SEMANTIC_DIFF_SPEC.md and consolidated some redundant tests (three pointer-escape tests into one table-driven, two scalar↔array transitions into one, three root-document-edge tests into one). Net is -459 lines across the two files.

On the spec doc — I removed it to cut down the MR but happy to bring it back, either as-is or in a trimmed form, whichever you prefer. Your call.

186 sem-core tests pass.

[inspect-review]

inspect review

Triage: 55 entities analyzed | 0 critical, 0 high, 15 medium, 40 low
Verdict: standard_review

Findings (2)

1. [low] extract_object_value has incorrect bracket-depth accounting: it increments depth for both { and [ but decrements on both } and ], and it returns only when seeing } with depth == 0. This can return the wrong slice when arrays are nested inside the object value (or when brackets are mismatched), producing incorrect obj_str and thus wrong child extraction/hashes/line offsets. Evidence:

match ch {
  '{' | '[' => depth += 1,
  '}' => { depth = depth.saturating_sub(1); if depth == 0 { return Some(...); } }
  ']' => { depth = depth.saturating_sub(1); }
}

2. [low] parent_name can infinite-loop on cyclic parent_id chains because it traverses parent_id links with loop { ... pid = next ... } and has no visited-set / max-depth guard. Under malformed/corrupted entity graphs (e.g., parent_id points to itself), this will never terminate. Evidence:

let mut pid = entity.parent_id.as_deref()?;
loop {
  match by_id.get(pid) {
    Some(parent) => match parent.parent_id.as_deref() {
      Some(next) => pid = next,
      None => break,
    },
    None => break,
  }
}

---

Reviewed by inspect | Entity-level triage found 0 high-risk changes

[inspect-review]

inspect review

Triage: 57 entities analyzed | 0 critical, 0 high, 16 medium, 41 low
Verdict: standard_review

Findings (1)

1. [low] extract_object_value can misclassify string values containing { as objects because it finds the first { after the colon without string-awareness (let brace_offset = after_colon.find('{')?;). Example: "key": "{not an object}" would be treated as an object, causing incorrect nested entity extraction and wrong line offsets.

---

Reviewed by inspect | Entity-level triage found 0 high-risk changes

[rs545837]

Thanks a lot for this PR @nminev, appreciate the effort.