Skip to content

fix(diff): replace naive index-based comparison with LCS algorithm - #2668

Closed
guyoron1 wants to merge 1 commit into
rtk-ai:developfrom
guyoron1:fix/diff-naive-algorithm
Closed

guyoron1 wants to merge 1 commit into
rtk-ai:developfrom
guyoron1:fix/diff-naive-algorithm

Conversation

@guyoron1

Copy link
Copy Markdown
Contributor

Summary

  • rtk diff file1 file2 used a naive index-based line comparison that caused cascading false positives on any insertion/deletion
  • Replaced with standard LCS (Longest Common Subsequence) dynamic programming
  • Inserting 1 line in a 20-line file now correctly reports 1 added line instead of 16 removed + 17 added
  • No new dependencies — the algorithm is implemented inline
  • All 25 existing tests pass unchanged; 2 new regression tests added

Root Cause

compute_diff() compared lines1[i] with lines2[i] by index position. Any insertion shifted all subsequent indices, causing every line after the insertion point to be reported as a remove+add pair. The LCS approach finds the longest set of common lines between the two files, then correctly identifies only the actual differences.

Reproduction (from #1869)

seq 1 20 > a.txt
cp a.txt b.txt
sed -i '5i\INSERTED' b.txt

# Before: 36 lines of output, 16 removed + 17 added (all false positives)
# After: correctly reports 1 added line
rtk diff a.txt b.txt

Test plan

  • New test_compute_diff_insertion_no_cascade — verifies single insertion produces exactly 1 added change
  • New test_compute_diff_deletion_no_cascade — verifies single deletion produces exactly 1 removed change
  • All 23 existing tests pass (identical, added, removed, modified, similarity, render, condense, truncation)
  • cargo fmt && cargo clippy --all-targets && cargo test --all passes

Fixes #1869

@EvolveAegis

Copy link
Copy Markdown

Tested this against the 1,000-line fixture from #1869 — the LCS output is a clean single-line diff. Nice work.

One edge case I noticed while testing: the never_worse guard in guard.rs can still override the LCS result and fall back to the raw before + sep + after concatenation. This happens when the filtered output is larger than the raw baseline.

I don't think it triggers with the default filters on a simple insertion, but if any filter expands the diff output beyond the concatenated file size, the guard silently swaps it for both full files — no indication that the diff was discarded.

Probably not in scope for this PR, just flagging it in case it's worth a follow-up.

compute_diff() compared lines by index position, causing cascading
false positives when lines were inserted or deleted. A single
insertion in a 20-line file would report 16 removed + 17 added
lines instead of 1 added.

Replace with standard LCS (Longest Common Subsequence) dynamic
programming. Adjacent remove+add pairs are still merged into
"modified" when lines have >50% similarity (Jaccard).

No new dependencies — the DP table is computed inline.

Fixes rtk-ai#1869
@guyoron1
guyoron1 force-pushed the fix/diff-naive-algorithm branch from c95964c to b75875a Compare August 14, 2026 19:03
@KuSh

KuSh commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Closing as a duplicate of #3268, which replaces the same naive positional comparison in compute_diff with an LCS alignment.

The bug you're fixing is real and worse in production than the issue describes: on a 40-line file with one line inserted, the positional compare marks everything after the insertion as modified, the rendered output grows past the raw baseline, and never_worse then discards it and prints both files in full — 0% savings on exactly the case the command exists for. Your test_compute_diff_insertion_no_cascade is the right regression test for it.

#3268 gets the nod because it already carries a cell cap on the DP table. This PR allocates vec![vec![0u32; n + 1]; m + 1] unconditionally, so a pair of 50k-line files asks for roughly 10 GB before it does any work — that has to be bounded before an LCS can ship.

One thing worth flagging, because it applies to #3268 too and it's the reason neither is merged yet: matched lines emit no op, so a deletion and an insertion that are far apart in the file end up adjacent in the ops vector and get paired by the similarity check. Sorting a three-line import block is enough to trigger it. The fix is an explicit "keep" op (or tracking the last matched index) so runs can't merge across matched regions.

Your #2652 is unaffected by any of this and is still on the list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rtk diff produces cascading false positives on insertions/deletions

3 participants