diff --git a/cmd/opencodereview/git.go b/cmd/opencodereview/git.go index 0185b56..dba1d6f 100644 --- a/cmd/opencodereview/git.go +++ b/cmd/opencodereview/git.go @@ -12,6 +12,15 @@ func runGitCmd(repoDir string, args ...string) ([]byte, error) { return cmd.CombinedOutput() } +// runGitCmdStdout is like runGitCmd but returns stdout only. Use it when the +// output is consumed as data (e.g. a resolved path) so git's stderr warnings +// (permissions, deprecations, config notices) can't pollute the result. +func runGitCmdStdout(repoDir string, args ...string) ([]byte, error) { + fullArgs := append([]string{"-C", repoDir}, args...) + cmd := exec.Command("git", fullArgs...) + return cmd.Output() +} + func getCommitMessage(repoDir, commit string) (string, error) { out, err := runGitCmd(repoDir, "log", "-1", "--format=%B", "--end-of-options", commit) if err != nil { diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index d76951b..118d398 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -115,23 +115,13 @@ func runReview(args []string) error { return emitRunResult(ctx, ag, comments, startTime, opts.outputFormat, opts.audience, q) } +// resolveRepoDir resolves the repo dir for `ocr rules check`. It delegates to +// resolveWorkingDir(requireGit=true) so it anchors at the git top-level just +// like the review path — keeping rule resolution consistent when run from a +// monorepo subdirectory (#287). func resolveRepoDir(input string) (string, error) { - if input == "" { - var err error - input, err = os.Getwd() - if err != nil { - return "", fmt.Errorf("get working directory: %w", err) - } - } - absPath, err := filepath.Abs(input) - 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 { - return "", fmt.Errorf("%s is not a git repository", absPath) - } - return absPath, nil + absPath, _, err := resolveWorkingDir(input, true) + return absPath, err } // 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..eaacf83 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" @@ -100,6 +101,25 @@ func resolveWorkingDir(input string, requireGit bool) (string, bool, error) { if !isGit && requireGit { return "", false, fmt.Errorf("%s is not a git repository", absPath) } + // #287: git reports diff and `git show HEAD:` paths relative to the + // repository root, not the current directory. When `ocr review` runs from a + // subdirectory of a monorepo, anchor RepoDir at the git top-level so those + // root-relative paths resolve for both disk reads and git-show reads. + // requireGit is true only for the review path; scan (requireGit=false) keeps + // the CWD so its `git ls-files` walk stays scoped to the subdirectory. + if isGit && requireGit { + // runGitCmdStdout captures stdout only so git stderr notices can't + // pollute the resolved path. --show-toplevel fails (or is empty) when + // there is no work tree — e.g. a bare repo, where --git-dir succeeds so + // isGit is true. Fail loudly there instead of silently reusing the + // subdir, which would reproduce the #287 root-relative-path bug. + top, topErr := runGitCmdStdout(absPath, "rev-parse", "--show-toplevel") + t := strings.TrimSpace(string(top)) + if topErr != nil || t == "" { + return "", false, fmt.Errorf("%s is a git repository without a work tree (bare repo?); cannot resolve its top level for review", absPath) + } + absPath = t + } return absPath, isGit, nil } diff --git a/cmd/opencodereview/shared_test.go b/cmd/opencodereview/shared_test.go index 8c9b819..21e7553 100644 --- a/cmd/opencodereview/shared_test.go +++ b/cmd/opencodereview/shared_test.go @@ -2,6 +2,7 @@ package main import ( "os" + "os/exec" "path/filepath" "testing" @@ -109,6 +110,89 @@ func TestResolveWorkingDir_NonExistent(t *testing.T) { } } +// TestResolveWorkingDir_MonorepoSubdir reproduces #287: running `ocr review` +// from a subdirectory of a git repo must anchor RepoDir at the git top-level +// (git reports diff / `git show HEAD:` paths relative to the repo root), +// while `ocr scan` (requireGit=false) must keep the subdirectory so its walk +// stays scoped. +func TestResolveWorkingDir_MonorepoSubdir(t *testing.T) { + root := t.TempDir() + git := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = root + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + git("init") + git("config", "user.email", "t@t.co") + git("config", "user.name", "t") + + sub := filepath.Join(root, "subproject1", "src") + if err := os.MkdirAll(sub, 0o755); err != nil { + t.Fatal(err) + } + + // macOS /var -> /private/var symlink means t.TempDir() differs from the + // canonicalized toplevel git returns; compare via EvalSymlinks. + wantRoot, err := filepath.EvalSymlinks(root) + if err != nil { + t.Fatalf("EvalSymlinks(%q): %v", root, err) + } + + // review path: hoisted to the git top-level. + got, isGit, err := resolveWorkingDir(sub, true) + if err != nil { + t.Fatalf("resolveWorkingDir(sub, true) error: %v", err) + } + if !isGit { + t.Error("expected isGit=true for a git subdirectory") + } + gotResolved, err := filepath.EvalSymlinks(got) + if err != nil { + t.Fatalf("EvalSymlinks(%q): %v", got, err) + } + if gotResolved != wantRoot { + t.Errorf("review RepoDir = %q, want git top-level %q", gotResolved, wantRoot) + } + + // scan path: keeps the subdirectory unchanged. + gotScan, _, err := resolveWorkingDir(sub, false) + if err != nil { + t.Fatalf("resolveWorkingDir(sub, false) error: %v", err) + } + gotScanResolved, err := filepath.EvalSymlinks(gotScan) + if err != nil { + t.Fatalf("EvalSymlinks(%q): %v", gotScan, err) + } + wantSub, err := filepath.EvalSymlinks(sub) + if err != nil { + t.Fatalf("EvalSymlinks(%q): %v", sub, err) + } + if gotScanResolved != wantSub { + t.Errorf("scan RepoDir = %q, want subdir %q (must stay scoped)", gotScanResolved, wantSub) + } +} + +// TestResolveWorkingDir_BareRepoFailsLoudly guards the #287 fix: a bare repo has +// no work tree, so `git rev-parse --git-dir` succeeds (isGit=true) but +// `--show-toplevel` fails. The review path (requireGit=true) must return an +// error rather than silently reusing the input dir, which would reproduce the +// original root-relative-path bug. +func TestResolveWorkingDir_BareRepoFailsLoudly(t *testing.T) { + bare := t.TempDir() + cmd := exec.Command("git", "init", "--bare", bare) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git init --bare: %v\n%s", err, out) + } + + _, _, err := resolveWorkingDir(bare, true) + if err == nil { + t.Fatal("expected error for a bare repo (no work tree), got nil") + } +} + func TestResolveWorkingDir_GitRepo(t *testing.T) { dir := t.TempDir() gitDir := filepath.Join(dir, ".git") diff --git a/internal/config/rules/system_rules.go b/internal/config/rules/system_rules.go index 2da4d14..6f4cf8d 100644 --- a/internal/config/rules/system_rules.go +++ b/internal/config/rules/system_rules.go @@ -344,6 +344,11 @@ func loadRuleFile(path string) (*ProjectRule, error) { return &pr, nil } +// loadProjectRule reads /.opencodereview/rule.json. Since #287 anchored +// RepoDir at the git top-level, `ocr review` from a monorepo subdirectory loads +// the repo-root rule file — which is consistent, since rule entries match against +// root-relative diff paths. A subproject-local rule.json under the subdirectory is +// intentionally not consulted; put shared rules at the repo root, or pass --rule. func loadProjectRule(repoDir string) (*ProjectRule, error) { path := filepath.Join(repoDir, ".opencodereview", "rule.json") data, err := os.ReadFile(path) diff --git a/internal/tool/filereader_read_test.go b/internal/tool/filereader_read_test.go index 96e2956..7093dd0 100644 --- a/internal/tool/filereader_read_test.go +++ b/internal/tool/filereader_read_test.go @@ -3,6 +3,7 @@ package tool import ( "context" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -215,3 +216,58 @@ func TestFileReader_Read_SubdirectoryFile(t *testing.T) { t.Errorf("Read() = %q, want %q", got, "package main") } } + +// TestFileReader_Read_CommitMode_MonorepoSubdirPath reproduces #287 at the +// git-show layer: in a monorepo, git reports paths relative to the repo root +// (e.g. "subproject1/src/models/request_meta.py"). With RepoDir anchored at the +// git top-level (the fix), `git show HEAD:` must resolve — +// this is the exact command that failed in the issue. +func TestFileReader_Read_CommitMode_MonorepoSubdirPath(t *testing.T) { + dir := t.TempDir() + git := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + git("init") + git("config", "user.email", "t@t.co") + git("config", "user.name", "t") + + rel := filepath.Join("subproject1", "src", "models", "request_meta.py") + if err := os.MkdirAll(filepath.Join(dir, filepath.Dir(rel)), 0o755); err != nil { + t.Fatal(err) + } + content := "class RequestMeta:\n id = 1\n" + if err := os.WriteFile(filepath.Join(dir, rel), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + git("add", ".") + git("commit", "-m", "init") + commit := getHeadCommit(t, dir) + + // Use the git-style forward-slash path the diff/LLM would supply. + gitPath := "subproject1/src/models/request_meta.py" + + // git-show (commit mode): the exact path from the issue error. + frShow := &FileReader{RepoDir: dir, Mode: ModeCommit, Ref: commit} + got, err := frShow.Read(context.Background(), gitPath) + if err != nil { + t.Fatalf("commit-mode Read(%q) error: %v", gitPath, err) + } + if got != content { + t.Errorf("commit-mode Read = %q, want %q", got, content) + } + + // disk (workspace mode): same root-relative path resolves too. + frDisk := &FileReader{RepoDir: dir, Mode: ModeWorkspace} + gotDisk, err := frDisk.Read(context.Background(), gitPath) + if err != nil { + t.Fatalf("workspace-mode Read(%q) error: %v", gitPath, err) + } + if gotDisk != content { + t.Errorf("workspace-mode Read = %q, want %q", gotDisk, content) + } +}