fix(tool): resolve file_read paths against git top-level in monorepos#2
fix(tool): resolve file_read paths against git top-level in monorepos#2chethanuk wants to merge 2 commits into
Conversation
|
CodeAnt AI is reviewing your PR. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a stdout-only git helper, anchors review-mode working-directory resolution to the git repository top-level, reuses that resolution in ChangesMonorepo Subdirectory Path Resolution
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue alibaba#287 by anchoring the repository directory at the git top-level when running ocr review from a subdirectory of a monorepo, ensuring root-relative paths resolve correctly. It also adds comprehensive tests to verify this behavior. The reviewer pointed out a potential issue where using runGitCmd to retrieve the top-level path could pollute the path string with stderr warnings or errors. They suggested executing the command directly with exec.Command and capturing only stdout, which is a safer and more robust approach.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
CodeAnt AI finished reviewing your PR. |
Greptile SummaryThis PR fixes a double-prefix path bug (alibaba#287) that caused
Confidence Score: 4/5Safe to merge for the core monorepo path-fix; a known silent-fallback edge case and a rule.json lookup behavior change in subdirectory-based workflows should be reviewed before landing. The path-resolution fix is correct and well-tested. Two open concerns carry real user-facing impact: (1) if cmd/opencodereview/shared.go — the Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/opencodereview/git.go (1)
15-23: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout via
exec.CommandContext(noctx).Static analysis flags
exec.Commandon line 20;os/exec.Commandshould be replaced withos/exec.CommandContextto allow cancellation/timeout. Without it, this call can block indefinitely (e.g. hung git process, credential prompt) on the review request path.🔧 Proposed fix (bounded timeout, no signature change)
+import ( + "context" + "os/exec" + "time" +) + // 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...) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "git", fullArgs...) return cmd.Output() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/opencodereview/git.go` around lines 15 - 23, The runGitCmdStdout helper currently uses exec.Command directly, so the git process cannot be cancelled and may hang the review path. Update runGitCmdStdout in git.go to create the command with exec.CommandContext using a bounded context timeout, while keeping the same function signature and preserving the current stdout-only behavior. Make sure the context is created locally inside runGitCmdStdout and applied to the existing git invocation built from repoDir and args.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/opencodereview/git.go`:
- Around line 15-23: The runGitCmdStdout helper currently uses exec.Command
directly, so the git process cannot be cancelled and may hang the review path.
Update runGitCmdStdout in git.go to create the command with exec.CommandContext
using a bounded context timeout, while keeping the same function signature and
preserving the current stdout-only behavior. Make sure the context is created
locally inside runGitCmdStdout and applied to the existing git invocation built
from repoDir and args.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04a29c23-2d2d-406b-a519-d623173d12b6
📒 Files selected for processing (4)
cmd/opencodereview/git.gocmd/opencodereview/shared.gocmd/opencodereview/shared_test.gointernal/tool/filereader_read_test.go
a27296e to
4cc0b24
Compare
There was a problem hiding this comment.
chethanuk has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Hi @chethanuk 👋 We noticed you're also fixing alibaba#287 — and it turns out we landed on the exact same approach independently: anchoring Your version also handles a detail ours doesn't — reading the top-level through a stdout-only git helper so stderr notices can't pollute the resolved path. Nice touch. Since your fix is effectively a superset, we'd be happy to close ours in favor of yours. Could you open your PR against |
ocr review from a monorepo subdirectory failed with "file not found" (alibaba#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.
4cc0b24 to
1462a5e
Compare
There was a problem hiding this comment.
chethanuk has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/opencodereview/git.go (1)
15-22: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
exec.CommandContextwith a timeout for the git subprocess.Static analysis flags Line 20 for using
exec.Commandinstead ofexec.CommandContext. Since this call has no timeout, a hung/slow git process (e.g. due to FS issues) blocks callers indefinitely. This mirrors the existing pattern inrunGitCmd(Lines 9-13), so consider addressing both together.♻️ Proposed fix
-func runGitCmdStdout(repoDir string, args ...string) ([]byte, error) { - fullArgs := append([]string{"-C", repoDir}, args...) - cmd := exec.Command("git", fullArgs...) - return cmd.Output() -} +func runGitCmdStdout(repoDir string, args ...string) ([]byte, error) { + ctx, cancel := context.WithTimeout(context.Background(), gitCmdTimeout) + defer cancel() + fullArgs := append([]string{"-C", repoDir}, args...) + cmd := exec.CommandContext(ctx, "git", fullArgs...) + return cmd.Output() +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/opencodereview/git.go` around lines 15 - 22, runGitCmdStdout currently uses exec.Command without any cancellation, so a stuck git process can hang callers indefinitely. Update runGitCmdStdout to take a context and use exec.CommandContext, matching the timeout/cancellation pattern already used by runGitCmd, and then adjust any call sites as needed so both helpers share the same subprocess lifecycle behavior.Source: Linters/SAST tools
internal/tool/filereader_read_test.go (1)
220-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't actually exercise the
#287anchoring bug it claims to reproduce.The docstring says this "reproduces
#287at the git-show layer," but the setup never simulates the buggy scenario: git isinited directly indir, andFileReader{RepoDir: dir, ...}usesdiras-is — already the correct top-level. The original bug requiredRepoDirto be mis-anchored to a subdirectory (causing double-prefixed paths likesubproject1/subproject1/...); this test would pass identically even without this PR's fix, as long asRepoDirhappened to equal the repo root. It does add useful new coverage for multi-segment root-relative paths in commit mode, but it doesn't guard the actual anchoring regression — that's only covered byTestResolveWorkingDir_MonorepoSubdirinshared_test.go. Sinceinternal/toolcan't import the unexportedresolveWorkingDirfromcmd/opencodereview(package main) to derive a realistically mis/correctly-anchoredRepoDir, true end-to-end coverage isn't feasible here — consider softening the docstring to avoid overstating what's being protected.✏️ Suggested docstring tweak
-// 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. +// TestFileReader_Read_CommitMode_MonorepoSubdirPath verifies that, once RepoDir +// is anchored at the git top-level (see TestResolveWorkingDir_MonorepoSubdir for +// the actual `#287` anchoring regression guard), `git show HEAD:<root-relative-path>` +// resolves correctly for multi-segment paths in both commit and workspace modes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tool/filereader_read_test.go` around lines 220 - 273, The test name/comment overstates that TestFileReader_Read_CommitMode_MonorepoSubdirPath reproduces the `#287` anchoring regression, but the current setup uses RepoDir at the git root so it would pass even without the fix. Update the docstring in TestFileReader_Read_CommitMode_MonorepoSubdirPath (and any nearby comments) to describe it as coverage for root-relative commit/workspace paths in a monorepo, not the anchoring bug itself, and avoid claiming it exercises the exact git-show failure from `#287`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/opencodereview/git.go`:
- Around line 15-22: runGitCmdStdout currently uses exec.Command without any
cancellation, so a stuck git process can hang callers indefinitely. Update
runGitCmdStdout to take a context and use exec.CommandContext, matching the
timeout/cancellation pattern already used by runGitCmd, and then adjust any call
sites as needed so both helpers share the same subprocess lifecycle behavior.
In `@internal/tool/filereader_read_test.go`:
- Around line 220-273: The test name/comment overstates that
TestFileReader_Read_CommitMode_MonorepoSubdirPath reproduces the `#287` anchoring
regression, but the current setup uses RepoDir at the git root so it would pass
even without the fix. Update the docstring in
TestFileReader_Read_CommitMode_MonorepoSubdirPath (and any nearby comments) to
describe it as coverage for root-relative commit/workspace paths in a monorepo,
not the anchoring bug itself, and avoid claiming it exercises the exact git-show
failure from `#287`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee81d5c6-4694-412e-acd7-ea6f86612fa0
📒 Files selected for processing (5)
cmd/opencodereview/git.gocmd/opencodereview/review_cmd.gocmd/opencodereview/shared.gocmd/opencodereview/shared_test.gointernal/tool/filereader_read_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/opencodereview/shared.go
Since alibaba#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).
|
CodeAnt AI is running Incremental review |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
There was a problem hiding this comment.
chethanuk has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
2 participants
Description
Running
ocr reviewfrom a subdirectory of a monorepo failed withfile not found(alibaba#287). Git reports diff paths andgit show HEAD:<path>paths relative to the repository root, butRepoDirwas scoped to the invocation subdirectory. Joining a root-relative path onto a subdirectoryRepoDirproduced a double prefix (e.g.repo/subproject1/subproject1/src/...), so both disk reads and git-show reads missed the file.Fix:
resolveWorkingDirnow anchorsRepoDirat the git top-level viagit rev-parse --show-toplevelon the review path (requireGit=true).ocr scan(requireGit=false) deliberately keeps the CWD so itsgit ls-fileswalk stays scoped to the subdirectory.Two robustness details:
runGitCmdStdout, usingcmd.Output()) so git's stderr notices (permissions, deprecations, config warnings) can't pollute the resolved path.--show-toplevelerrors or returns nothing — e.g. a bare repo, where--git-dirsucceeds but there is no work tree — resolution now fails loudly instead of silently reusing the subdirectory and reproducing the original bug.flowchart TD A[resolveWorkingDir input, requireGit] --> B[filepath.Abs + os.Stat] B --> C[git rev-parse --git-dir] C --> D{is git repo?} D -- no, requireGit=true --> E[error: not a git repository] D -- no, requireGit=false --> F[return subdir, isGit=false] D -- yes, requireGit=false scan --> G[return subdir, isGit=true<br/>git ls-files stays scoped] D -- yes, requireGit=true review --> H[git rev-parse --show-toplevel<br/>stdout only] H --> I{ok and non-empty?} I -- yes --> J[RepoDir = git top-level<br/>fix for #287] I -- no --> K[return error<br/>fail loudly, no silent fallback] J --> L[return repo root, isGit=true]Type of Change
How Has This Been Tested?
go test ./cmd/opencodereview/ ./internal/tool/— 592 tests pass;gofmt -lclean;go vetclean.New regression tests:
TestResolveWorkingDir_MonorepoSubdir— review path hoists to the git top-level; scan path keeps the subdirectory.TestResolveWorkingDir_BareRepoFailsLoudly— bare repo (--git-dirok,--show-toplevelfails) returns an error rather than silently falling back.TestFileReader_Read_CommitMode_MonorepoSubdirPath—git show HEAD:<root-relative-path>and workspace disk reads both resolve, the exact command from the issue.make testpasses locallyManual testing (described above)
Checklist
go fmt,go vet)Related Issues
closes alibaba#287
Summary by CodeRabbit