From 82fe45a110cebde28f3dfc714ee1a72cd6c2306c Mon Sep 17 00:00:00 2001 From: kite Date: Mon, 6 Jul 2026 19:08:47 +0800 Subject: [PATCH] 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 :` 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. --- cmd/opencodereview/review_cmd.go | 9 ++++-- cmd/opencodereview/shared.go | 29 ++++++++++++++---- cmd/opencodereview/shared_test.go | 49 +++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 9 deletions(-) diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index d76951b..1fbb7fa 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -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. diff --git a/cmd/opencodereview/shared.go b/cmd/opencodereview/shared.go index 8d898a1..a36f630 100644 --- a/cmd/opencodereview/shared.go +++ b/cmd/opencodereview/shared.go @@ -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 diff --git a/cmd/opencodereview/shared_test.go b/cmd/opencodereview/shared_test.go index 8c9b819..530202b 100644 --- a/cmd/opencodereview/shared_test.go +++ b/cmd/opencodereview/shared_test.go @@ -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")