Filter cosmetic orphan changes from semantic diff#294
Closed
Iron-Ham wants to merge 1 commit into
Closed
Conversation
There was a problem hiding this comment.
inspect review
Triage: 38 entities analyzed | 0 critical, 0 high, 17 medium, 21 low
Verdict: standard_review
Findings (6)
- [low] In
recalculate_diff_summary, orphan changes are now double-counted in change-type buckets. Whenentity_type == "orphan", the code incrementsorphan_countbut no longer returns early, so it falls through to the match statement and increments the change-type bucket again (e.g.,added_count). This means orphans contribute to bothorphan_countAND their respective change-type count, which breaks the invariant thatorphan_countshould be a subset cross-cutting metric. - [low] In
recalculate_diff_summary, the logic for Moved/Renamed/Reordered changes double-counts modifications. When a change is Moved/Renamed/Reordered AND has content changes, it increments both the move/rename/reorder count ANDmodified_count. However, if the change is also an orphan, it will be triple-counted (orphan_count + move/rename/reorder count + modified_count), leading to incorrect summary totals. - [low] In
retain_non_cosmetic_changes, the filter logic is inverted for compound changes. The code retains changes wherestructural_change != Some(false)OR the change is Moved/Renamed/Reordered. This means a cosmetic (structural_change == Some(false)) Moved/Renamed/Reordered change will be retained, which contradicts the intent to drop purely cosmetic changes. - [low] In test
no_cosmetics_filter_recomputes_summary_from_remaining_changes, the expectedmodified_countchanged from 1 to 2, but the test setup only has one Modified change (the structural one). The orphan has ChangeType::Modified but should not be counted separately in modified_count if the old behavior was correct, suggesting the new counting logic is wrong or the test expectation is incorrect. - [low] In
recalculate_diff_summary, the testorphan_changes_count_toward_change_type_bucketsexpects orphans to count toward change-type buckets (assertsadded_count == 1anddeleted_count == 2with 1 orphan added and 2 entities deleted). However, the comment in the AFTER code says 'orphan_count is cross-cutting metadata, while retained orphans still contribute to change-type buckets', which contradicts the BEFORE comment that said 'Orphans only skip the entity change-type buckets'. The removal of thecontinuestatement changes the behavior to make orphans count in both places, but this appears to be inconsistent with the original design where orphans were separate. - [low] In
retain_non_cosmetic_changes, the filter logic now allows cosmetic changes (wherestructural_change == Some(false)) to be retained if they are Moved, Renamed, or Reordered. However, inrecalculate_diff_summary, these cosmetic compound changes will increment both their primary bucket (moved_count, renamed_count, reordered_count) AND modified_count ifhas_content_change()returns true. This creates a logical inconsistency: a cosmetic change (structural_change=false) should not have a content change, but the code will still check and potentially double-count it.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
Contributor
Author
|
Superseded by #311. |
rs545837
pushed a commit
that referenced
this pull request
Jun 4, 2026
## Summary - consolidate binary file diff reporting, diff output escaping, and cosmetic orphan filtering into one branch - keep diff formatter, JSON, CLI, and MCP reporting behavior aligned across the related changes Supersedes #237, #290, #294. Closes #181. Closes #273. Fixes #272. ## Test plan - cargo test --manifest-path crates/Cargo.toml -p sem-cli --test diff_file_compare - cargo test --manifest-path crates/Cargo.toml -p sem-cli --test diff_patch - cargo test --manifest-path crates/Cargo.toml -p sem-cli --test diff_output_hardening - cargo test --manifest-path crates/Cargo.toml -p sem-cli --test diff_no_cosmetics - cargo test --manifest-path crates/Cargo.toml -p sem-cli commands::diff::tests - cargo test --manifest-path crates/Cargo.toml -p sem-core parser::differ::tests - cargo check --manifest-path crates/Cargo.toml -p sem-core -p sem-cli -p sem-mcp
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
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.
Summary
--no-cosmeticsand preserve their summary bucketsFixes #272
Test plan
cargo test --workspaceclaude -previewer launched twice; final embedded-diff run exited successfully but produced no review text