fix(tool): resolve file_read paths against git top-level in monorepos (#309)

* fix(tool): resolve file_read paths against git top-level in monorepos

ocr review from a monorepo subdirectory failed with "file not found" (#287):
git reports diff and `git show HEAD:<path>` paths relative to the repo root,
but RepoDir was scoped to the invocation subdirectory, producing a double
prefix. resolveWorkingDir now anchors RepoDir at `git rev-parse
--show-toplevel` on the review path (requireGit=true); scan keeps the CWD so
its `git ls-files` walk stays scoped.

The top-level lookup uses a stdout-only git helper so stderr notices can't
pollute the path, and fails loudly if --show-toplevel errors or is empty
(e.g. a bare repo) instead of silently reusing the subdirectory. Adds
regression tests for the subdir hoist, the scan-path scoping, git-show
resolution of root-relative paths, and the bare-repo failure.

* docs(rules): document repo-root rule.json resolution in monorepos

Since #287 anchored RepoDir at the git top-level, ocr review from a
monorepo subdirectory loads the repo-root .opencodereview/rule.json
rather than a subdir-local one. Call out this user-visible behavior at
loadProjectRule so the scope change isn't a surprise (review feedback).

Author: chethanuk

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 {

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.

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:<path>` 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
 }
 

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:<path>` 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")

internal/config/rules/system_rules.go

@@ -344,6 +344,11 @@ func loadRuleFile(path string) (*ProjectRule, error) {
 	return &pr, nil
 }
 
+// loadProjectRule reads <repoDir>/.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)

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:<root-relative-path>` 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)
+	}
+}