feat(diff): add --diff-file to review a validated subset of hunks#357
feat(diff): add --diff-file to review a validated subset of hunks#357Th0rgal wants to merge 3 commits into
Conversation
Adds an opt-in `ocr review --from BASE --to HEAD --diff-file selected.patch` input. The patch is a standard unified git diff carrying an exact subset of complete hunks from the canonical BASE..HEAD (or --commit) diff. OCR then reviews only those hunks while still reading the complete real HEAD tree and preserving true-HEAD line numbers for comments. ApplySelection narrows the canonical per-file diffs to the selected hunks and fails closed on anything unsafe: invented/renamed/mutated hunks, malformed patches, path traversal, mismatched header ranges, ambiguous or duplicate matches, and binary/submodule shapes. NewFileContent and hunk headers are preserved so downstream line resolution stays anchored at the true HEAD ref. The flag requires an explicit --from/--to or --commit range (rejected in workspace mode) and cannot be combined with --resume, since selection changes the diff fingerprint. Default behavior is unchanged when the flag is absent. Refs alibaba#313
|
|
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func loadSelectedDiff(path string) (string, error) { | ||
| if path == "" { | ||
| return "", nil | ||
| } | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return "", fmt.Errorf("read --diff-file %q: %w", path, err) | ||
| } | ||
| return string(data), nil | ||
| } |
There was a problem hiding this comment.
Unlike loadBackgroundFile which enforces a 1 MB size limit (maxBackgroundFileBytes) and checks for directory paths before reading, loadSelectedDiff reads the entire file into memory without any size constraint. A maliciously crafted or accidentally huge patch file could cause excessive memory consumption. Consider adding a similar size guard (e.g., os.Stat + size check) before calling os.ReadFile, consistent with the existing pattern in loadBackgroundFile.
There was a problem hiding this comment.
Confirmed and fixed in eed66a1: loadSelectedDiff now mirrors loadBackgroundFile with an os.Stat directory check and a size cap (8 MB, sized for diff subsets). It also rejects empty/whitespace-only files, which previously produced an empty selection that silently fell open into a full-range review. Tests added in selected_diff_test.go.
| case binaryRe.MatchString(line): | ||
| sf.isBinary = true |
There was a problem hiding this comment.
The binaryRe regex (Binary files ) is unanchored and uses substring matching via MatchString. Because this loop iterates over ALL lines in the block (including hunk body lines), a hunk body line like +Binary files are compared here would falsely trigger sf.isBinary = true, causing the selection to be rejected with an error about binary files.
The other patterns in this switch are safe because they use HasPrefix or exact equality, and hunk body lines always start with +, -, , or \. But binaryRe.MatchString performs a substring search regardless of position.
Consider either:
- Anchoring the regex to the start of line:
^Binary files - Or limiting this metadata scan to only lines before the first
@@hunk header (which is where git extended headers actually appear).
There was a problem hiding this comment.
Confirmed and fixed in 9cdfe78: binaryRe is now anchored (^Binary files ) in internal/diff/parser.go, which corrects both this selection-side check and the same latent false positive in ParseDiffText. Regression tests added in selection_test.go and parser_test.go.
The unanchored `Binary files ` regex matched hunk body lines whose content merely mentions the phrase (e.g. "+Binary files are compared here"), falsely marking the file binary in both ParseDiffText and the --diff-file selection parser, which then rejected valid selections. Real git binary markers always start the line, and hunk body lines always carry a '+', '-', ' ' or '\' prefix, so anchoring is exact.
An empty --diff-file produced an empty SelectedDiff, which the agent treats as "no selection" — silently falling open into a full-range review instead of the requested subset. Reject empty/whitespace-only files explicitly, and mirror loadBackgroundFile's stat-based directory and size guards (8 MB cap) so oversized or directory paths fail fast.
2 participants
Summary
Adds an opt-in
--diff-fileinput toocr reviewso a caller can review only asubset of the changes in a range, instead of the whole
--from..--to(or--commit) diff.Motivation: on large changesets, teams often want to select which hunks are in
scope for review (e.g. risk-ranked or path-filtered externally) and hand OCR
just that slice. Today OCR reviews the entire range. This change lets an
external tool express that selection as a plain unified git patch while keeping
all selection policy outside OCR.
Usage:
selected.patchis a standard unified git diff (git diffoutput) containingan exact subset of complete hunks from the canonical
BASE..HEADdiff. OCRreviews only those hunks, while still reading the complete real HEAD tree, so
full-file tool reads and GitHub comment line numbers remain anchored to the true
HEAD.
Behavior & safety
internal/diff.ApplySelectionnarrows the canonical per-file diffs down to theselected hunks. It fails closed on anything that isn't an exact subset:
invented/renamed/mutated hunks, malformed or truncated patches, path traversal
(
../absolute), mismatched header ranges, ambiguous or duplicate matches, andbinary/submodule shapes.
NewFileContentand the original hunk headers are preserved, so lineresolution still yields true-HEAD line numbers.
--from/--toor--commitrange (rejected inworkspace mode) and cannot be combined with
--resume, because narrowing thediff changes the resume fingerprint.
--diff-fileis not passed.Tests
internal/diff/selection_test.go: table-driven unit tests (single/multiplehunks, multiple files, dropping unselected files, mutated body, wrong ranges,
ambiguous/duplicate, rename, delete, binary, submodule, CRLF, Unicode,
no-newline marker, and line-resolver accuracy vs true HEAD).
cmd/opencodereview/selected_diff_test.go: CLI flag/mode validation plus twointegration tests on a real temp git repo (multi-hunk HEAD, subset selection)
asserting review/preview scope and that full reads come from true HEAD.
Refs #313