fix(git): anchor deletion-only hunks so dependents of deleted symbols are detected by noam-aharon20 · Pull Request #72 · frontops-dev/domino

noam-aharon20 · 2026-06-10T07:22:11Z

Closes #74.

Summary

parse_diff in src/git.rs expands each diff hunk header to the new-side range start..start + count. With --unified=0 (which get_diff always uses, src/git.rs:111), removing a line produces a @@ -X,Y +Z,0 @@ hunk — new-side count W == 0 — so start..start + count is the empty range Z..Z and the deletion contributes nothing to changed_lines. Downstream in src/core.rs, an empty changed_lines yields no symbols, unique_symbols is empty, and reference traversal is skipped (No traceable symbols found … skipping reference traversal). The file's owning package is still marked, but every project that depends on the deleted symbol is silently dropped.

This is the deletion-only counterpart of #62. #63 expanded changed_lines to the full hunk range, fixing the addition / multi-line case, but a W == 0 deletion still expands to an empty range — #63 codified that in test_parse_diff_pure_deletion_hunk_contributes_zero. This PR closes that remaining gap (direction (1) from #74).

Change

src/git.rs, in the parse_diff hunk-range loop — anchor a W == 0 deletion hunk to its new-side line Z instead of emitting an empty range:

let count: usize = caps
    .get(2)
    .and_then(|m| m.as_str().parse().ok())
    .unwrap_or(1);
if count == 0 {
    // Deletion-only hunk (`+Z,0`). The removed lines are gone from the new
    // file, but the top-level symbol that *enclosed* them (e.g. an exported
    // object losing a property, a switch losing a case) is still changed, and
    // its dependents are affected. Anchor to the new-side line `Z` — the line
    // just before the deletion point, still inside the enclosing symbol — so
    // the downstream AST lookup can resolve that symbol and trace its refs.
    // Clamp to 1 for deletions at the top of the file (git emits `+0,0`).
    let anchor = start.max(1);
    return Some(anchor..anchor + 1);
}
Some(start..start + count)

Z is the line immediately before the deletion point, which is still inside the enclosing top-level symbol, so find_node_at_line(Z) resolves it (e.g. TABLE) and the existing cascade traces its consumers.

Tests

test_parse_diff_deletion_only_hunk_anchors_to_new_side_line (new) — regression mirroring the real hunk @@ -495 +494,0 @@; asserts changed_lines == [494].
test_parse_diff_deletion_hunk_anchors_alongside_addition (was test_parse_diff_pure_deletion_hunk_contributes_zero, added in fix(git): expand changed_lines to full hunk range #63) — the deletion at +5,0 now anchors line 5 alongside the addition hunk's lines: [5, 21, 22] (was [21, 22]).
test_parse_diff_deletion_only_file_anchors_to_enclosing_symbol_line (was ..._kept_with_empty_lines) — a deletion-only file now yields [5] instead of an empty changed_lines.

Both renamed tests had encoded the "deletion contributes nothing" behavior this PR corrects.

cargo test --lib — 208 passed, 0 failed. cargo fmt --check and cargo clippy --lib clean.

Caller impact

Reviewed every read of ChangedFile.changed_lines:

core.rs:208-223 — each line is mapped to a symbol via find_node_at_line, then deduplicated into unique_symbols. The new anchor line resolves to the enclosing symbol; if a deletion ever anchors to a line outside any declaration it contributes an empty Vec<String> and does nothing — i.e. no worse than today. No behavioural change except the bug is fixed.
core.rs:194-201, 235-257 — AffectCause::DirectChange { line } reporting now records one cause for the anchor line of a deletion (previously zero). A correctness improvement; previously under-reported.
lockfile.rs — reads only file_path, never changed_lines.

No change to ChangedFile, no new public API.

Manual validation

Two-package Rush fixture (anonymized; same shape as #74's repro), package-b calls lookup() from package-a, which reads an exported TABLE object:

Branch	Before (`@1.4.0`)	After (fix)
`edit` (in-place body change)	`[package-a, package-b]`	`[package-a, package-b]`
`delete` (remove one `TABLE` entry)	`[package-a]` ❌	`[package-a, package-b]` ✅

The edit control confirms the cascade itself works; only the delete shape regressed, and the fix restores it. Also validated on a real Rush monorepo: a one-line deletion from an exported lookup object consumed by ~26 projects went from 1 affected project to 22 (the projects that actually reference it — the semantic cascade stays precise, it's just no longer short-circuited).

Out of scope

Direction (2) from #74 — recovering a deleted entire top-level symbol by parsing the base revision at the old-side range — is deferred. When a deletion removes a whole symbol, the anchor line falls outside any declaration and behaviour falls back to today's package-level marking (no regression, just not improved).

Summary by CodeRabbit

Release Notes

Bug Fixes
- Improved detection and tracking of deleted content. The system now properly processes deletions during change analysis, ensuring all removed code is accurately captured and identified for downstream symbol and reference detection. Updated regression tests verify correct deletion handling at both file and change level.

`git diff --unified=0` emits a `+Z,0` hunk when lines are removed. These deletion-only hunks contributed nothing to `changed_lines`, so downstream symbol extraction in `core.rs` found no traceable symbol and skipped the reference-traversal cascade entirely ("No traceable symbols found ... skipping reference traversal"). The file's owning package was still marked affected, but every *dependent* of the deleted symbol was missed. Impact: any change whose diff is purely a deletion — removing a property from an exported object, a case from a switch, an enum member, an overload — under-reports affected projects to just the package the file lives in. In a real monorepo, deleting one entry from an exported lookup object that 26 projects transitively consume reported `["@lama/selectors"]` (1 project) instead of the 22 projects that actually reference it. Fix: a `W == 0` deletion hunk now anchors to its new-side line `Z` — the line just before the deletion point, which is still inside the enclosing top-level symbol — so the AST lookup resolves that symbol and traces its references normally. Clamped to 1 for deletions at the top of a file (`+0,0`). Known limitation (left for a follow-up): deleting an *entire* top-level symbol leaves no enclosing node at the anchor line, so the AST lookup finds nothing and behavior falls back to today's package-level marking. Catching that requires parsing the base revision at the old-side range. Adds a regression test and corrects two existing tests that had encoded the buggy "deletion contributes nothing" assumption.

coderabbitai · 2026-06-10T07:22:19Z

📝 Walkthrough

Walkthrough

parse_diff in src/git.rs is updated so that pure deletion hunks (+Z,0) emit a single anchored changed_lines entry (clamped to line ≥1) on the new side instead of contributing nothing. Inline documentation and three regression tests are updated or added to cover this behavior.

Changes

Deletion Hunk Anchoring in parse_diff

Layer / File(s)	Summary
Anchoring logic and doc update `src/git.rs`	Inline documentation is updated to describe `W == 0` anchoring semantics. The hunk line-number extraction returns `anchor..anchor+1` (with `max(Z, 1)` clamping) when the new-side count is `0`, replacing the prior empty-range path.
Regression tests `src/git.rs`	Mixed deletion+addition hunk test is rewritten to assert both the anchored deletion line and the addition lines appear in `changed_lines`. The deletion-only file test is rewritten to assert the file is retained with the anchored line. A new regression test asserts a deletion-only hunk alone contributes the anchored new-side line.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

affected misses dependents on deletion-only changes (+Z,0 hunk → empty changed_lines) #74: Directly describes the same fix — pure deletion hunks (+Z,0) must emit an anchored changed_lines value to prevent downstream symbol/reference detection failures.

Possibly related PRs

frontops-dev/domino#63: Modifies the same parse_diff / changed_lines logic in src/git.rs for deletion-only hunk handling, making it the direct predecessor of this change.

Suggested reviewers

EladBezalel

Poem

🐇 A deletion once vanished without a trace,
No anchor, no line, just an empty space.
Now clamped to at least one, it stays in the list,
No symbol or reference shall ever be missed.
Hippity-hop, the diff is now right! 🎉

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and accurately summarizes the main change: fixing deletion-only hunks to properly anchor and enable dependent symbol detection.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

🧹 Nitpick comments (1)

src/git.rs (1)
235-240: 💤 Low value

Stale comment and unreachable code path after deletion-hunk fix.

With the new anchoring logic, deletion hunks (+Z,0) now contribute an anchor line to changed_lines. This means a "deletion-only file" will no longer have empty changed_lines — it will contain the anchor lines. The condition changed_lines.is_empty() && has_hunks is now unreachable for deletion-only files, making this branch dead code and the comment misleading.

Consider removing this branch or updating the comment to clarify it's defensive code for hypothetical edge cases (e.g., malformed hunks that parse but produce empty ranges).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/git.rs` around lines 235 - 240, The condition `changed_lines.is_empty()
&& has_hunks` is now unreachable after the deletion-hunk anchoring fix, since
deletion hunks now contribute anchor lines to `changed_lines`, making this
branch dead code. Either remove the entire `else if has_hunks` branch in the
git.rs deletion-file handling logic, or update the misleading comment to clarify
that this is defensive code for edge cases like malformed hunks rather than
actual deletion-only files. Whichever approach you choose, ensure the remaining
code still properly marks affected packages when deletions occur.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/git.rs`:
- Around line 235-240: The condition `changed_lines.is_empty() && has_hunks` is
now unreachable after the deletion-hunk anchoring fix, since deletion hunks now
contribute anchor lines to `changed_lines`, making this branch dead code. Either
remove the entire `else if has_hunks` branch in the git.rs deletion-file
handling logic, or update the misleading comment to clarify that this is
defensive code for edge cases like malformed hunks rather than actual
deletion-only files. Whichever approach you choose, ensure the remaining code
still properly marks affected packages when deletions occur.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e1ff5952-1dd6-40ff-b4bd-d2e218f2ca3c

📥 Commits

Reviewing files that changed from the base of the PR and between 8b64617 and 4c53a66.

📒 Files selected for processing (1)

src/git.rs

EladBezalel · 2026-07-08T13:39:45Z

+            // the enclosing symbol — so the downstream AST lookup can resolve
+            // that symbol and trace its references. Clamp to 1 for deletions at
+            // the top of the file (git emits `+0,0`).
+            let anchor = start.max(1);


this branch is dead now — every hunk yields a non-empty range (count == 0 returns anchor..anchor+1, everything else has count >= 1), so changed_lines can't be empty while has_hunks is true. drop it (and has_hunks)? the comment describes a state that can no longer happen, and no test exercises it anymore.

) (#79) * fix(git): anchor deletion-only hunks to the enclosing symbol's line `git diff --unified=0` emits a `+Z,0` hunk when lines are removed. These deletion-only hunks contributed nothing to `changed_lines`, so downstream symbol extraction in `core.rs` found no traceable symbol and skipped the reference-traversal cascade entirely ("No traceable symbols found ... skipping reference traversal"). The file's owning package was still marked affected, but every *dependent* of the deleted symbol was missed. Impact: any change whose diff is purely a deletion — removing a property from an exported object, a case from a switch, an enum member, an overload — under-reports affected projects to just the package the file lives in. In a real monorepo, deleting one entry from an exported lookup object that 26 projects transitively consume reported `["@lama/selectors"]` (1 project) instead of the 22 projects that actually reference it. Fix: a `W == 0` deletion hunk now anchors to its new-side line `Z` — the line just before the deletion point, which is still inside the enclosing top-level symbol — so the AST lookup resolves that symbol and traces its references normally. Clamped to 1 for deletions at the top of a file (`+0,0`). Known limitation (left for a follow-up): deleting an *entire* top-level symbol leaves no enclosing node at the anchor line, so the AST lookup finds nothing and behavior falls back to today's package-level marking. Catching that requires parsing the base revision at the old-side range. Adds a regression test and corrects two existing tests that had encoded the buggy "deletion contributes nothing" assumption. * fix(git): recover deleted symbols from the base revision Reworks the deletion-only fix in response to review. The previous commit anchored a `+Z,0` deletion hunk to its new-side line `Z` and fed that into the current-file AST lookup. That heuristic is incorrect: after a deletion, line `Z` belongs to whatever symbol now occupies it in the *new* file, so deleting an entire top-level symbol either mis-attributes the change to the next symbol or resolves nothing — the deleted symbol's real dependents were still dropped. Instead, capture the hunk's *old-side* range `-X,Y` in a new `ChangedFile.deleted_lines` field and, downstream, re-parse the file at the base revision (`git show <merge-base>:./path`) to resolve the top-level symbol that enclosed each removed line. This handles both shapes uniformly: - removing a member of a still-present declaration (object property, switch case, interface field) resolves to the enclosing symbol, and - removing an entire exported declaration resolves to that symbol itself — the case the anchor heuristic could not solve. The recovered symbols are traced through the *current* import graph, where consumers still import them, so their projects are correctly reported affected. Deletions whose lines resolve to no declaration contribute nothing (no regression). Also drops the now-unreachable `has_hunks` empty-`changed_lines` branch that review flagged: every non-deletion hunk yields new-side lines and every deletion yields `deleted_lines`, so a file with hunks is never empty on both. analyzer: split the top-level-symbol resolution out of `find_node_at_line` into `find_top_level_symbols(&FileSemanticData, ...)` so it can run against a one-off base-revision parse (`parse_source`) that never enters `self.files`; profiler timing now wraps the call in `find_node_at_line`. Known limitation (unchanged from before): a whole-file deletion, and a deleted symbol reachable only via a local reference from another symbol whose binding the deletion breaks, still fall back to package-level marking. Tests: rewrite the three git unit tests for `deleted_lines` semantics, add a leading-symbol-deletion case, add analyzer unit tests for `find_deleted_symbols`, and add integration tests for whole-symbol and member deletion tracing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: address CodeRabbit review on #79 - git.rs: fix stale symmetric-hunk test comment that referenced the old greedy `.*` in `LINE_RE`; describe the current explicit capture groups. - integration: assert the directly-changed package (`lib`) is also affected in the member-deletion test, mirroring the whole-symbol-deletion test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: address maintainer review on #79 - types.rs: drop the unused `Default` derive on `ChangedFile` — every construction (src + tests) sets both fields explicitly. - report.rs: deletion causes carry `line: 0` (no surviving new-side line); render that as "(deleted)" instead of the odd "(line 0)", and omit the locator entirely for the symbol-less whole-file/asset line-0 case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Noam Aharon <noam.a@lama.ai> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

EladBezalel · 2026-07-09T07:06:01Z

Superseded by #79, which took the correct base-revision recovery approach (this PR's new-side anchor heuristic couldn't handle whole-symbol deletion). #79 is merged. Closing.

noam-aharon20 closed this Jun 10, 2026

noam-aharon20 reopened this Jun 15, 2026

noam-aharon20 marked this pull request as ready for review June 15, 2026 13:47

coderabbitai Bot reviewed Jun 15, 2026

View reviewed changes

EladBezalel reviewed Jul 8, 2026

View reviewed changes

itayper mentioned this pull request Jul 8, 2026

fix(git): recover deleted symbols from the base revision (supersedes #72) #79

Merged

EladBezalel closed this Jul 9, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(git): anchor deletion-only hunks so dependents of deleted symbols are detected#72

fix(git): anchor deletion-only hunks so dependents of deleted symbols are detected#72
noam-aharon20 wants to merge 1 commit into
frontops-dev:mainfrom
noam-aharon20:fix/deletion-only-hunk-anchors-enclosing-symbol

noam-aharon20 commented Jun 10, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 10, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

coderabbitai Bot left a comment

Uh oh!

EladBezalel Jul 8, 2026

Uh oh!

EladBezalel commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

noam-aharon20 commented Jun 10, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change

Tests

Caller impact

Manual validation

Out of scope

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai Bot commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

EladBezalel Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

EladBezalel commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

noam-aharon20 commented Jun 10, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 10, 2026 •

edited

Loading