alibaba / alibaba/open-code-review

bug(diff): CRLF line endings cause corrupted NewPath and missing IsNew/IsDeleted metadata

Open
#933 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
24.4k
Forks
1.8k
Avg merge
2d 4m
Merged PRs (30d)
102

Description

### OpenCodeReview Version
main (latest)

### Operating System
Windows / Cross-platform with CRLF

### Installation Method
go build / npm

### LLM Provider
Any

### Bug Description
When unified diff text contains Windows-style CRLF (`\r\n`) line endings (common with `core.autocrlf=true` or repositories configured with autocrlf / CRLF checkout), `ParseDiffText` and `ParseHunks` in `internal/diff` split lines on `\n` without stripping trailing `\r`.

This causes several severe regressions:
1. `diffHeaderRe` captures `b/(.+)`, polluting `d.NewPath` with a trailing `\r` (e.g. `"file.go\r"`).
2. Subsequent calls to read `d.NewPath` in `finalizeDiff` fail with `stat file "file.go\r": CreateFile ... The filename, directory name, or volume label syntax is incorrect` / `file not found`, leaving `d.NewFileContent` completely empty during review.
3. Metadata checks `line == "--- /dev/null"` and `line == "+++ /dev/null"` fail because the string contains `\r`, losing `IsNew` and `IsDeleted` flags.
4. `ParseHunks` leaves trailing `\r` in `HunkLine.Content`.

### Steps to Reproduce
Pass a unified diff containing CRLF line endings to `ParseDiffText` (or run `ocr review` in an environment where diffs output CRLF):

```go
diffText := "diff --git a/fresh.go b/fresh.go\r\n" +
"new file mode 100644\r\n" +
"--- /dev/null\r\n" +
"+++ b/fresh.go\r\n" +
"@@ -0,0 +1,1 @@\r\n" +
"+line1\r\n"

diffs, err := ParseDiffText(context.Background(), diffText, dir, "", nil)
// Output:
// [ocr] WARNING: cannot read file fresh.go for review: stat file "fresh.go\r": CreateFile ...: The filename, directory name, or volume label syntax is incorrect.
// d.NewPath == "fresh.go\r" (corrupted)
// d.IsNew == false (lost)
```

### Expected Behavior
Trailing `\r` should be stripped when splitting diff lines, so file paths are clean and metadata markers match accurately.

### Additional Context
Additionally, in `internal/tool/code_search.go`, `hasTraversalPathComponent` only checks `/` separators and misses Windows backslashes (`..\secret`), which can be addressed together.

Contributor guide

Open the contributing guide

Research direction

Start in internal/diff at ParseDiffText and ParseHunks, then trace finalizeDiff and the diffHeaderRe and metadata checks described in the report. Verify that CRLF input leaves clean paths and HunkLine.Content and preserves IsNew and IsDeleted; also inspect internal/tool/code_search.go for Windows backslash traversal handling.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
devtools
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.