mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-07-09 17:28:58 +00:00
fix(cmd): resolve review repo dir to git top level for monorepo subdirs
When ocr review runs from a subdirectory of a monorepo, the working dir differs from the git repository root. git diff emitted subdir-relative paths (with diff.relative) while the file_read tool's `git show <ref>:<path>` resolves paths relative to the repo root, so every file_read failed on the first try and only succeeded after the LLM retried with the repo-root prefix, wasting extra LLM calls (#287). Resolve the review working dir to `git rev-parse --show-toplevel` so diff and file_read agree on repo-root-relative paths. The scan path (requireGit=false) intentionally keeps the working-directory scope so scanning a subdirectory stays limited to it.
This commit is contained in:
parent
10f587594b
commit
82fe45a110
3 changed files with 78 additions and 9 deletions
|
|
@ -127,11 +127,14 @@ func resolveRepoDir(input string) (string, error) {
|
|||
if err != nil {
|
||||
return "", fmt.Errorf("resolve absolute path: %w", err)
|
||||
}
|
||||
out, err := runGitCmd(absPath, "rev-parse", "--git-dir")
|
||||
if err != nil || len(out) == 0 {
|
||||
// Resolve to the repository top level so paths stay repo-root relative
|
||||
// when invoked from a monorepo subdirectory (#287).
|
||||
out, err := runGitCmd(absPath, "rev-parse", "--show-toplevel")
|
||||
toplevel := strings.TrimSpace(string(out))
|
||||
if err != nil || toplevel == "" {
|
||||
return "", fmt.Errorf("%s is not a git repository", absPath)
|
||||
}
|
||||
return absPath, nil
|
||||
return toplevel, nil
|
||||
}
|
||||
|
||||
// requireGitRepo validates that the given directory is part of a git repository.
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@ import (
|
|||
"fmt"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"github.com/open-code-review/open-code-review/internal/agent"
|
||||
|
|
@ -77,9 +78,17 @@ func loadCommonContext(repoDirInput, rulePath string, maxTools, maxGitProcs int,
|
|||
}, nil
|
||||
}
|
||||
|
||||
// resolveWorkingDir returns (absPath, isGitRepo, err). When requireGit is
|
||||
// resolveWorkingDir returns (repoDir, isGitRepo, err). When requireGit is
|
||||
// true, returns an error if the directory is not a git repo. When false,
|
||||
// returns IsGitRepo=false instead of erroring (scan path uses this).
|
||||
//
|
||||
// When requireGit is true (review path) and the directory is a git repo, the
|
||||
// returned path is the repository top level (`git rev-parse --show-toplevel`),
|
||||
// not the raw working directory. This keeps git diff/show paths consistent when
|
||||
// ocr runs from a monorepo subdirectory: otherwise the diff and the file_read
|
||||
// tool disagree on whether paths are relative to the subdir or the repo root
|
||||
// (#287). The scan path (requireGit=false) intentionally keeps the
|
||||
// working-directory scope so scanning a subdirectory stays limited to it.
|
||||
func resolveWorkingDir(input string, requireGit bool) (string, bool, error) {
|
||||
if input == "" {
|
||||
wd, err := os.Getwd()
|
||||
|
|
@ -95,12 +104,20 @@ func resolveWorkingDir(input string, requireGit bool) (string, bool, error) {
|
|||
if _, statErr := os.Stat(absPath); statErr != nil {
|
||||
return "", false, fmt.Errorf("stat %s: %w", absPath, statErr)
|
||||
}
|
||||
out, err := runGitCmd(absPath, "rev-parse", "--git-dir")
|
||||
isGit := err == nil && len(out) > 0
|
||||
if !isGit && requireGit {
|
||||
return "", false, fmt.Errorf("%s is not a git repository", absPath)
|
||||
out, err := runGitCmd(absPath, "rev-parse", "--show-toplevel")
|
||||
toplevel := strings.TrimSpace(string(out))
|
||||
isGit := err == nil && toplevel != ""
|
||||
if !isGit {
|
||||
if requireGit {
|
||||
return "", false, fmt.Errorf("%s is not a git repository", absPath)
|
||||
}
|
||||
return absPath, false, nil
|
||||
}
|
||||
return absPath, isGit, nil
|
||||
// Only the review path resolves to the repo root; scan keeps the subdir.
|
||||
if requireGit {
|
||||
return toplevel, true, nil
|
||||
}
|
||||
return absPath, true, nil
|
||||
}
|
||||
|
||||
// llmRuntime bundles the LLM-side state both subcommands need once they've
|
||||
|
|
|
|||
|
|
@ -109,6 +109,55 @@ func TestResolveWorkingDir_NonExistent(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
// TestResolveWorkingDir_SubdirResolvesToToplevel locks in the #287 fix: when
|
||||
// ocr is invoked from a monorepo subdirectory, the resolved repo dir must be
|
||||
// the repository top level, not the subdir, so diff and file_read agree on
|
||||
// repo-root-relative paths.
|
||||
func TestResolveWorkingDir_SubdirResolvesToToplevel(t *testing.T) {
|
||||
root := initTestGitRepo(t)
|
||||
sub := filepath.Join(root, "subproject", "src")
|
||||
if err := os.MkdirAll(sub, 0o755); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
got, isGit, err := resolveWorkingDir(sub, true)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if !isGit {
|
||||
t.Fatal("expected isGit=true for a subdir of a git repo")
|
||||
}
|
||||
wantRoot, _ := filepath.EvalSymlinks(root)
|
||||
gotResolved, _ := filepath.EvalSymlinks(got)
|
||||
if gotResolved != wantRoot {
|
||||
t.Errorf("resolveWorkingDir(subdir) = %q, want repo root %q", got, wantRoot)
|
||||
}
|
||||
}
|
||||
|
||||
// TestResolveWorkingDir_ScanSubdirKeepsScope guards against regressing scan's
|
||||
// scope: requireGit=false (scan path) must keep the subdirectory, not expand to
|
||||
// the whole repo, even when the subdir lives inside a git repository.
|
||||
func TestResolveWorkingDir_ScanSubdirKeepsScope(t *testing.T) {
|
||||
root := initTestGitRepo(t)
|
||||
sub := filepath.Join(root, "subproject", "src")
|
||||
if err := os.MkdirAll(sub, 0o755); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
got, isGit, err := resolveWorkingDir(sub, false)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if !isGit {
|
||||
t.Fatal("expected isGit=true for a subdir of a git repo")
|
||||
}
|
||||
wantSub, _ := filepath.EvalSymlinks(sub)
|
||||
gotResolved, _ := filepath.EvalSymlinks(got)
|
||||
if gotResolved != wantSub {
|
||||
t.Errorf("resolveWorkingDir(subdir, requireGit=false) = %q, want subdir %q", got, wantSub)
|
||||
}
|
||||
}
|
||||
|
||||
func TestResolveWorkingDir_GitRepo(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
gitDir := filepath.Join(dir, ".git")
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue