diff --git a/internal/diff/resolver.go b/internal/diff/resolver.go index ee981a0..bc97f64 100644 --- a/internal/diff/resolver.go +++ b/internal/diff/resolver.go @@ -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 → @@ -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 + } + 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 @@ -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 diff --git a/internal/diff/resolver_test.go b/internal/diff/resolver_test.go index dd8effd..a6cea7a 100644 --- a/internal/diff/resolver_test.go +++ b/internal/diff/resolver_test.go @@ -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}, @@ -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 // ---------------------------------------------------------------------------