Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 35 additions & 20 deletions internal/diff/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ type indexedLine struct {
content string
}

// maxMatchLineSpanFactor prevents matching snippets across excessive blank-line gaps.
const maxMatchLineSpanFactor = 2

// resolveFromHunk tries to find startLine/endLine by matching ExistingCode
// against hunk lines. It tries the new-side first (context + added lines →
// new-file line numbers), then falls back to old-side (context + deleted →
Expand Down Expand Up @@ -149,16 +152,30 @@ func matchConsecutive(sideLines []indexedLine, targetLines []string) (startLine,
if len(targetLines) == 0 || len(sideLines) < len(targetLines) {
return 0, 0, false
}
for i := 0; i <= len(sideLines)-len(targetLines); i++ {
matched := true
for j, target := range targetLines {
if sideLines[i+j].content != target {
matched = false
for i := 0; i < len(sideLines); i++ {
targetIndex := 0
start, end := 0, 0

for j := i; j < len(sideLines) && targetIndex < len(targetLines); j++ {
// ExistingCode normalization drops blank lines. Skip blank lines on the
// diff/file side as well so snippets containing internal blank lines
// can still resolve to their real line range.
if sideLines[j].content == "" {
continue
}
Comment on lines +163 to +165

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential false positive matching: By skipping blank lines, this function now allows matching across arbitrary gaps of blank lines. If two unrelated code sections happen to share the same normalized content and are separated only by blank lines, they could incorrectly match. Consider adding a sanity check that the gap between start and end is not unreasonably larger than len(targetLines) — for example, ensuring (end - start + 1) <= len(targetLines) * someFactor. Without such a guard, a comment could be attached to the wrong code location in files with many blank lines.

Also, consider adding a unit test for matchConsecutive that directly tests the blank-line-skipping behavior (e.g., sideLines with interspersed empty-content entries), since the existing unit tests only cover strictly consecutive matches.

if sideLines[j].content != targetLines[targetIndex] {
break
}
if start == 0 {
start = sideLines[j].lineNum
}
end = sideLines[j].lineNum
targetIndex++
}
if matched {
return sideLines[i].lineNum, sideLines[i+len(targetLines)-1].lineNum, true
if targetIndex == len(targetLines) {
if end-start+1 <= len(targetLines)*maxMatchLineSpanFactor {
return start, end, true
}
}
}
return 0, 0, false
Expand All @@ -177,19 +194,17 @@ func resolveFromFileContent(d *model.Diff, cm *model.LlmComment) bool {
return false
}

for i := 0; i <= len(fileLines)-len(targetLines); i++ {
matched := true
for j, target := range targetLines {
if normalizeLine(strings.TrimRight(fileLines[i+j], "\r")) != target {
matched = false
break
}
}
if matched {
cm.StartLine = i + 1
cm.EndLine = i + len(targetLines)
return true
}
indexed := make([]indexedLine, 0, len(fileLines))
for i, line := range fileLines {
indexed = append(indexed, indexedLine{
lineNum: i + 1,
content: normalizeLine(strings.TrimRight(line, "\r")),
})
}
if start, end, ok := matchConsecutive(indexed, targetLines); ok {
cm.StartLine = start
cm.EndLine = end
return true
}

return false
Expand Down
75 changes: 75 additions & 0 deletions internal/diff/resolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,53 @@ import "fmt"`},
}
}

func TestResolveLineNumbers_NewFileSnippetWithInternalBlankLine(t *testing.T) {
raw := `diff --git a/internal/handler/file/file_handler_base.go b/internal/handler/file/file_handler_base.go
new file mode 100644
index 0000000..1b3ffb7
--- /dev/null
+++ b/internal/handler/file/file_handler_base.go
@@ -0,0 +1,17 @@
+package file
+
+import (
+ "example.com/acme/review-agent/internal/types"
+ "example.com/acme/review-agent/pkg/log"
+)
+
+// processFileTransfer processes file transfer with given direction
+func (h *Handler) processFileTransfer(fileInfo *types.FileInfo, direction string) *types.SystemStatus {
+ log.Infof("processing transfer: direction=%s, src=%s, dst=%s",
+ direction, fileInfo.SrcPath, fileInfo.DstPath)
+
+ if direction == "download" {
+ return h.performDownload(fileInfo.DstPath, nil)
+ }
+ return h.performUpload(fileInfo.SrcPath)
+}`

diffs := []model.Diff{
{NewPath: "internal/handler/file/file_handler_base.go", Diff: raw},
}
comments := []model.LlmComment{
{Path: "internal/handler/file/file_handler_base.go", ExistingCode: `func (h *Handler) processFileTransfer(fileInfo *types.FileInfo, direction string) *types.SystemStatus {
log.Infof("processing transfer: direction=%s, src=%s, dst=%s",
direction, fileInfo.SrcPath, fileInfo.DstPath)

if direction == "download" {
return h.performDownload(fileInfo.DstPath, nil)
}
return h.performUpload(fileInfo.SrcPath)
}`},
}

result := ResolveLineNumbers(comments, diffs)
cm := result[0]
if cm.StartLine != 9 || cm.EndLine != 17 {
t.Errorf("new file with internal blank line: expected 9..17, got %d..%d", cm.StartLine, cm.EndLine)
}
}

func TestResolveLineNumbers_NoMatchKeepsZero(t *testing.T) {
diffs := []model.Diff{
{NewPath: "test.go", Diff: testDiff},
Expand Down Expand Up @@ -429,6 +476,34 @@ func TestMatchConsecutive_ExactFull(t *testing.T) {
}
}

func TestMatchConsecutive_SkipsInternalBlankLines(t *testing.T) {
lines := []indexedLine{
{lineNum: 10, content: "alpha"},
{lineNum: 11, content: ""},
{lineNum: 12, content: "beta"},
}

start, end, ok := matchConsecutive(lines, []string{"alpha", "beta"})
if !ok || start != 10 || end != 12 {
t.Errorf("blank-line match: got (%d, %d, %v), want (10, 12, true)", start, end, ok)
}
}

func TestMatchConsecutive_RejectsExcessiveBlankLineGap(t *testing.T) {
lines := []indexedLine{
{lineNum: 10, content: "alpha"},
{lineNum: 11, content: ""},
{lineNum: 12, content: ""},
{lineNum: 13, content: ""},
{lineNum: 14, content: "beta"},
}

start, end, ok := matchConsecutive(lines, []string{"alpha", "beta"})
if ok {
t.Errorf("expected excessive blank-line gap to be rejected, got (%d, %d, true)", start, end)
}
}

// ---------------------------------------------------------------------------
// resolveFromHunk integration tests
// ---------------------------------------------------------------------------
Expand Down