Fix parent suppression when container's own declaration also changes - #98
Merged
rs545837 merged 1 commit intoApr 30, 2026
Merged
Conversation
The existing suppression fires whenever any child entity changes, which incorrectly hides a parent Modified when both the parent's own declaration and a child changed in the same commit (e.g. class gains a base class while a method is also renamed). Fix: for Modified containers, strip child content by line number from both sides and compare what remains. If the parent's own declaration changed, keep it in the output. Added/Deleted containers are still suppressed unconditionally since their children carry all the detail. Also removes the older Modified-only dedup from match_entities — it was superseded by suppress_redundant_parents in differ.rs.
Closed
3 tasks
![inspect-review[bot]](https://avatars.githubusercontent.com/in/3177512?s=60&v=4){kind=small}
There was a problem hiding this comment.
inspect review
Triage: 10 entities analyzed | 0 critical, 0 high, 5 medium, 5 low
Verdict: standard_review
Findings (2)
- [low] strip_children_content can exclude the wrong lines due to 1-based vs 0-based indexing mismatch. It computes indices as
child.start_line.saturating_sub(parent_start_line)/child.end_line.saturating_sub(parent_start_line)and then compares them directly tolines.iter().enumerate()indices (0-based). Ifstart_line/end_lineare 1-based (typical for line numbers), the excluded range will be shifted by 1, makingbefore_own == after_owncomparisons incorrect and causing wrong suppression decisions. Evidence:let start_idx = child.start_line.saturating_sub(parent_start_line); ... excluded.insert(i);withlines.iter().enumerate(). - [low] strip_children_content can incorrectly exclude the parent's first line when a child is outside/above the parent range because it uses
saturating_subwithout validating that the child span is within the parent content. Ifchild.start_line < parent_start_line,start_idxbecomes 0 and the loop will exclude line index 0 of the parent content even though the child is not actually inside the parent snippet. Evidence:let start_idx = child.start_line.saturating_sub(parent_start_line); ... for i in start_idx..=end_idx.max(start_idx) { if i < lines.len() { excluded.insert(i); } }.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
3 tasks
nminev
added a commit
to nminev/sem
that referenced
this pull request
Apr 30, 2026
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 Ataraxy-Labs#97 and the precision guard merged in Ataraxy-Labs#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 Ataraxy-Labs#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #94 (closed). This covers the suppression precision fix only.
The bug
The current
suppress_redundant_parentsfires whenever any child entity changes — which is correct in most cases. But if a container's own declaration also changed in the same commit (e.g. a class gains a base class while a method is also renamed), the parentModifiedgets suppressed anyway, silently dropping a real change from the output.Fix
For
Modifiedcontainers, strip the child entity lines from both sides of the parent content and compare what's left. If the parent's own declaration changed, keep it in the output.Added/Deletedcontainers are still suppressed unconditionally since their children carry all the detail.Two files changed:
differ.rs— replaces the blanket child-check insuppress_redundant_parentswith the line-number-based own-content comparison for Modified containers. Adds astrip_children_contenthelper and two integration tests that pin both behaviours (basic suppression still works, precision case now works).identity.rs— removes the older Modified-only dedup block frommatch_entities. It was added beforesuppress_redundant_parentsexisted indiffer.rsand is now dead code.Tests