feat(cmd):Add a method to initiate OCR based on a single branch and its corresponding patch - #1063
feat(cmd):Add a method to initiate OCR based on a single branch and its corresponding patch#1063Strke wants to merge 18 commits into
Conversation
…容且 resume身份不一致的问题
|
Based on the fact that it seems inconvenient to resolve conflicts on the main branch, I have submitted a new pull request |
|
🔍 OpenCodeReview found 7 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| head := diff.NewCommitProvider(cc.RepoDir, ref, cc.GitRunner).ResolveInput(ctx).ResolvedHead | ||
| if head == "" { | ||
| return fmt.Errorf("resolve patch post-image ref %q in --repo", ref) | ||
| } |
There was a problem hiding this comment.
The error message here loses all diagnostic context about why resolution failed. ResolveInput silently returns an empty string when git rev-parse fails (e.g., unborn repo, corrupt ref, permission error), so the user sees only resolve patch post-image ref "HEAD" in --repo with no underlying cause.
The equivalent code in internal/agent/identity.go:resolveInputBeforeDiff calls resolveCommitHead which at least returns a wrappable error. Consider either:
- Using
resolveCommitHead-style logic that preserves the git stderr output, or - Running
git rev-parsedirectly here to capture the actual failure reason.
This matters because HEAD resolution can fail for non-obvious reasons (unborn repository, detached HEAD edge cases, corrupted refs), and the current message gives the user no actionable information.
| // so a provider that changed via config file or environment stays implicit — | ||
| // which is the transition this check exists to reject. | ||
| func validateResumeIdentity(ctx context.Context, cc *commonContext, opts reviewOptions, rt *llmRuntime, state *session.ResumeState) (*agent.SealedInput, error) { | ||
| func validateResumeIdentity(ctx context.Context, cc *commonContext, opts reviewOptions, rt *llmRuntime, state *session.ResumeState, patchInputs ...*diff.InputResolution) (*agent.SealedInput, error) { |
There was a problem hiding this comment.
Using a variadic parameter here to accommodate existing test callers that omit this argument obscures the production contract: exactly one *diff.InputResolution is expected, and passing zero or multiple values silently does the wrong thing (nil or ignored extras). Consider making this a regular pointer parameter and updating the test call sites to pass nil explicitly — this makes the API self-documenting and prevents future callers from accidentally passing multiple values.
| patchRef := a.args.PatchRef | ||
| ref = diff.NewCommitProvider(a.args.RepoDir, patchRef, a.args.GitRunner).ResolveInput(ctx).ResolvedHead | ||
| if ref == "" { | ||
| return fmt.Errorf("resolve patch post-image ref %q", patchRef) |
There was a problem hiding this comment.
The error message reads as a phrase fragment ("resolve patch post-image ref ...") rather than describing what went wrong. Unlike the equivalent path in identity.go which wraps an underlying error with %w, here ResolveInput returns an empty string with no error to wrap. Consider adding a verb to make the failure actionable, e.g. "cannot resolve patch post-image ref %q".
| if err := os.Remove(indexPath); err != nil { | ||
| return "", fmt.Errorf("prepare temporary patch index: %w", err) | ||
| } | ||
| defer os.Remove(indexPath) |
There was a problem hiding this comment.
If os.Remove(indexPath) fails here, the function returns without the deferred cleanup (which is set up on line 135, after this check), leaking the temporary file. Move the defer os.Remove(indexPath) before this removal attempt, or add an explicit os.Remove(indexPath) in this error branch.
Suggestion:
| if err := os.Remove(indexPath); err != nil { | |
| return "", fmt.Errorf("prepare temporary patch index: %w", err) | |
| } | |
| defer os.Remove(indexPath) | |
| defer os.Remove(indexPath) | |
| if err := os.Remove(indexPath); err != nil { | |
| return "", fmt.Errorf("prepare temporary patch index: %w", err) | |
| } |
| cmd.Stdin = bytes.NewReader(input) | ||
| return cmd.CombinedOutput() |
There was a problem hiding this comment.
Bug: The fallback path uses cmd.CombinedOutput() which merges stdout and stderr. For write-tree and commit-tree, the returned bytes are parsed as a git object hash via strings.TrimSpace. If git emits any stderr warnings or diagnostics alongside the hash, the trimmed result will contain non-hash text, causing downstream failures (e.g., commit-tree receiving an invalid tree hash, or the returned commit hash being malformed).
The runner path (OutputWithInputEnv) correctly returns stdout only. The fallback should match this behavior by separating stdout and stderr.
Suggestion:
| cmd.Stdin = bytes.NewReader(input) | |
| return cmd.CombinedOutput() | |
| cmd.Stdin = bytes.NewReader(input) | |
| var stderr bytes.Buffer | |
| cmd.Stderr = &stderr | |
| out, err := cmd.Output() | |
| if err != nil && stderr.Len() > 0 { | |
| return nil, fmt.Errorf("%w: %s", err, stderr.String()) | |
| } | |
| return out, err |
| if err != nil && stderr.Len() > 0 { | ||
| return nil, fmt.Errorf("%w: %s", err, stderr.String()) | ||
| } | ||
| return out, err |
There was a problem hiding this comment.
When the command fails and stderr is non-empty, this returns nil for stdout. However, callers (e.g., MaterializePatchCommit in internal/diff/patch.go) use the returned out in error messages (strings.TrimSpace(string(out))) expecting diagnostic content even on failure. With this code, those error messages will display empty strings whenever stderr has content.
Additionally, Go's cmd.Output() does return stdout bytes alongside an *exec.ExitError when the process exits non-zero. Consider returning out along with the wrapped error so callers retain access to any stdout produced before the failure.
Suggestion:
| if err != nil && stderr.Len() > 0 { | |
| return nil, fmt.Errorf("%w: %s", err, stderr.String()) | |
| } | |
| return out, err | |
| if err != nil && stderr.Len() > 0 { | |
| return out, fmt.Errorf("%w: %s", err, stderr.String()) | |
| } | |
| return out, err |
|
These issues have been corrected |
|
1. NewFileContent semantic mismatch (want to confirm your intended workflow) In my understanding, Currently, I've outlined my logic and modified the code, which involves two execution logics: With the Without the 2. Missing upfront validation for --diff path An advance check for --patch has now been added. 3. Should --diff require --repo? The 4. PatchProvider.ResolveInput returns empty InputResolution
Then it is fixed as the commit SHA and passed to 5. Flag naming: --diff → --patch? Now diff has been replaced with patch. |
lizhengfeng101
left a comment
lizhengfeng101
left a comment
There was a problem hiding this comment.
Two minor suggestions:
-
validateResumeIdentityvariadic: Nit: the variadicpatchInputs ...*diff.InputResolutionfeels like a workaround to avoid touching callers — just make it an explicit param, it's cleaner and only two call sites need updating. -
review_cmd.gopatch logic: The patch-specific branches scattered throughexecuteReviewContextare getting hard to follow — would you mind pulling them into a helper likeresolvePatchInput(ctx, cc, opts)so the main flow reads top-to-bottom again?
2 participants
Description
By adding the --repo, --branch, --patch, and --apply-patch methods, the data preparation space required for testing OCR review has been reduced.
Running ocr review with --from and --to requires downloading the complete code of the target repository, which can lead to long download times or significant local storage usage when testing a large codebase. Therefore, a new method for running ocr review has been added, which only requires using the repository's git shallow clone code and the patch in the pr to adapt the ocr review backend running program for execution. Local testing has shown that the storage space required to test 50 repositories has been reduced from 25.31GB to 4.87GB (using the code-review-benchmark dataset)
--repo: directory address of the code repository
--branch: The target branch of the code repository
--patch: Directory address of the .patch file
--apply-patch: Whether to apply the patch to the current branch
Type of Change
How Has This Been Tested?
make testpasses locallyChecklist
go fmt,go vet)Related Issues