alibaba / alibaba/open-code-review
bug(diff): CRLF line endings cause corrupted NewPath and missing IsNew/IsDeleted metadata
- 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
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