Improve parent dedup: handle renamed/added/deleted children + show parent context in output - #94
Conversation
) * fix sem diff replaying the last commit after commit (Ataraxy-Labs#14) When sem diff found no staged or working tree changes, it fell back to HEAD and returned the last commit diff. That made a clean repo look like it still had pending changes and read like stale cached output. Repro: 1. Modify a tracked file 2. Run sem diff 3. Commit the change 4. Run sem diff again Expected: no changes detected Actual: the same semantic diff is shown again from HEAD Stop the clean-worktree path from auto-switching to commit scope. Explicit history queries still go through --commit, --from, and --to. Also add a regression test covering the clean-worktree case and preserving explicit commit diff behavior. * nested function extraction + release binaries CI, v0.3.7 Add function body node types to container_node_types for all languages so nested functions are extracted as child entities with parent_id. Add GitHub Actions workflow to build and attach binaries (linux-x86_64, darwin-x86_64, darwin-arm64) on tag push. * fix release CI: use macos-14 for x86_64 cross-compile * drop macOS x86_64 target, add fail-fast: false * update README: add file diff, Vue/Swift/Elixir/Bash to language table * fix version string to use CARGO_PKG_VERSION instead of hardcoded value * Suppress parent Modified when only child entities changed When a method is renamed inside a class (or a key renamed inside a JSON object), the parent entity's content_hash changes because parent content includes child content. This caused spurious "Modified" entries for the parent alongside the accurate child-level Renamed/Added/Deleted event. Add a post-processing pass in `match_entities` that strips child content from both before/after parent content (using entity line numbers), then compares the remainder. If the parent's own declaration/fields are unchanged, the Modified is suppressed — only the child change is reported. The parent still appears as Modified when its own content changes (e.g. a class gains a new base class) even if children also changed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Show parent context in terminal output for nested entities Previously nested entities (methods inside classes, keys inside JSON objects) were displayed with just their own name, making it impossible to tell which class a modified method belonged to. Add `parent_name: Option<String>` to `SemanticChange`, populated from the entity's parent_id at match time. The terminal formatter renders nested entities as "ParentName::entity_name". Top-level entities (classes, top-level functions, root JSON keys) are unaffected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Address review findings: truncate long names, add debug_assert, expand tests - Truncate display_name to 24 chars + ellipsis in terminal formatter so long Parent::method_name strings don't break column alignment - Add debug_assert in strip_children_content to surface any future extractor bug where a child start_line < parent start_line - Add test asserting parent_name is Some("Svc") for nested entities and None for top-level entities - Add test confirming parent stays Modified when all children are deleted (body changes to `pass`, normalized content differs — suppression correctly does not fire) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Mathews Fernando <mathewsfpereira@gmail.com> Co-authored-by: Rohan Sharma <rs545837@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Conflicts resolved:
- identity.rs: kept our suppress_redundant_parent_modified (handles
Renamed/Added/Deleted children, not just Modified) over upstream's
simpler Modified-only dedup. Took upstream's optimised Phase 3
(pre-computed token sets grouped by type). Added parent_name alongside
upstream's new entity_line field in all SemanticChange constructions.
Updated upstream's test fixture to use multi-line content so our
line-number-based child stripping works correctly.
- change.rs: kept both our parent_name and upstream's entity_line +
old_entity_name fields.
- terminal.rs: combined upstream's verbose mode + old_entity_name
display ("old -> new") with our parent context display
("Parent::entity_name") and name truncation.
- Other files (bridge.rs, mod.rs, Cargo.toml, release.yml, README.md,
bridge.ts): accepted upstream versions unchanged.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
inspect review
Triage: 34 entities analyzed | 0 critical, 0 high, 4 medium, 30 low
Verdict: standard_review
Findings (2)
- [low] UTF-8 unsafe truncation in terminal formatter can panic at runtime:
format_terminalslicesdisplay_nameby byte index (&display_name[..24]) after checkingdisplay_name.len() > 25(byte length). Ifdisplay_namecontains any multi-byte UTF-8 characters and byte 24 is not a char boundary, Rust will panic when slicing. Evidence:let truncated = if display_name.len() > 25 { format!("{}…", &display_name[..24]) } else { display_name };incrates/sem-cli/src/formatters/terminal.rs. - [low]
strip_children_contentcan panic when child line metadata is inverted: it uses an inclusive rangefor i in start_idx..=end_idx, which panics ifend_idx < start_idx(e.g., ifchild.end_line < child.start_lineor other malformed/stale metadata). There is no guard for this case. Evidence:let start_idx = ...; let end_idx = ...; for i in start_idx..=end_idx { ... }incrates/sem-core/src/model/identity.rs.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
There was a problem hiding this comment.
inspect review
Triage: 18 entities analyzed | 0 critical, 0 high, 4 medium, 14 low
Verdict: standard_review
Findings (2)
- [low] UTF-8 unsafe truncation in terminal formatter can panic at runtime:
&display_name[..24]slices a RustStringby byte index, which will panic if index 24 is not a UTF-8 char boundary (e.g., non-ASCII entity/parent names). Evidence:let truncated = if display_name.len() > 25 { format!("{}…", &display_name[..24]) } else { display_name };incrates/sem-cli/src/formatters/terminal.rs. - [low] Off-by-one bug in
strip_children_contentline index conversion: comment says converting absolute 1-based line numbers to 0-based indices, but code does not subtract 1. This shifts excluded ranges and can exclude the wrong lines, breaking suppression decisions. Evidence:let start_idx = child.start_line.saturating_sub(parent_start_line); let end_idx = child.end_line.saturating_sub(parent_start_line); for i in start_idx..=end_idx { ... }incrates/sem-core/src/model/identity.rs.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
There was a problem hiding this comment.
inspect review
Triage: 19 entities analyzed | 0 critical, 0 high, 4 medium, 15 low
Verdict: standard_review
Findings (1)
- [low] UTF-8 unsafe truncation in terminal formatter can panic at runtime. In
crates/sem-cli/src/formatters/terminal.rs, truncation uses byte slicing:format!("{}…", &display_name[..24]). Ifdisplay_namecontains non-ASCII/multibyte UTF-8 characters, index 24 may not be a char boundary and Rust will panic when slicing aString.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
There was a problem hiding this comment.
inspect review
Triage: 15 entities analyzed | 0 critical, 0 high, 4 medium, 11 low
Verdict: standard_review
Findings (1)
- [low] Terminal formatter can panic on non-ASCII names due to invalid UTF-8 slicing. In
crates/sem-cli/src/formatters/terminal.rs, truncation uses&display_name[..24]whendisplay_name.len() > 25. Rust string slicing requires a char boundary; if byte 24 falls inside a multi-byte UTF-8 character, this will panic at runtime:let truncated = if display_name.len() > 25 { format!("{}…", &display_name[..24]) } else { display_name };
Reviewed by inspect | Entity-level triage found 0 high-risk changes
|
Thanks for mentioning that it's generated with claude code, I will review it. |
|
The suppress_redundant_parents (line 148) already checks if ANY child has a change (changed_ids.contains(entity.id.as_str())), regardless of whether it's Modified, Renamed, Added, or Deleted. Then it suppresses the parent if the parent is Modified. This already covers all the cases the PR claims to fix. I think the PR was based on an older version of the code (before the fix in ee94c9e). |
|
Closing it for now, reopen if there's a case that I missed. |
|
Hey, fair point on the suppression — you're right that Two things though: 1. The precision gap Your approach suppresses the parent whenever any child has a change. But consider the case where a parent's own declaration changes at the same time as a child change — e.g. a class gains a base class and a method gets renamed in the same commit. Your code would suppress the parent It's an edge case, but it's real and the current behavior would silently drop a meaningful change. 2. The This part wasn't addressed at all in the close. Right now the output shows: With the change it shows: When you have multiple classes in a file with similarly named methods, the current output gives you no idea which class the change is in without going and checking the file. The Feel free to keep it closed if neither of these is something you want, just wanted to make sure both points were on the table. Happy to split into two separate PRs if that's easier to review. |
|
Opened two smaller PRs instead:
|
Summary
This PR builds on the dedup work in #81 with two improvements:
1. Broader suppression — handles Renamed, Added, and Deleted children (not just Modified)
The current implementation in
a03f7e4only suppresses a parentModifiedwhen a child is alsoModified. It misses the case where a child is renamed — which is arguably the most common scenario (renaming a method inside a class). In that case the child shows asRenamedbut the parent still shows asModified, which is still noise.The new approach strips child content from both sides of the parent using entity line numbers, then compares what remains. If the parent's own declaration/signature is unchanged, the
Modifiedis suppressed regardless of what type of change the children had.The parent is not suppressed if its own declaration changed (e.g. a class gains a base class), even if children also changed.
2. Parent context in terminal output
Nested entities now display as
Parent::entity_nameso you can immediately tell which class a changed method belongs to. Works alongside the existingold -> newrename display.Adds
parent_name: Option<String>toSemanticChange(serialised asparentNamein JSON, skipped when null — fully additive).Test plan
test_parent_child_dedup_class_methodto use multi-line content with consistent line numbers (the original single-line fixture was incompatible with line-number-based stripping)parent_namepopulated correctly; all-children-deleted → parent still shown🤖 Generated with Claude Code