Repository navigation
fix(rules): allow ocr rules check in non-git directories - #1705
akushonkamen wants to merge 1 commit into
Conversation
) resolveRepoDir delegated to resolveWorkingDir(requireGit=true), so `ocr rules check` failed fast outside a git repo even though rule resolution has no git dependency: with ResolverOptions{} the ref is empty and the sniffer reads the working tree via os.Open, never via git show. The git gate was an artifact of reusing the review path's resolver. Resolve with requireGit=false (matching the scan/session precedents), but keep anchoring at the git top-level inside a git repo so rule resolution stays consistent with the review path when run from a monorepo subdirectory (alibaba#287). Bare repos still fail loudly, as before.
|
|
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| top, topErr := runGitCmdStdout(absPath, "rev-parse", "--show-toplevel") | ||
| t := strings.TrimSpace(string(top)) | ||
| if topErr != nil || t == "" { | ||
| return "", fmt.Errorf("%s is a git repository without a work tree (bare repo?); cannot resolve its top level", absPath) | ||
| } | ||
| absPath = t |
There was a problem hiding this comment.
The bare-repo / top-level resolution logic here is an exact duplicate of what resolveWorkingDir already does when requireGit=true (shared.go:179-191). If that logic is ever updated (e.g., different error message, additional validation), this copy will silently drift.
Consider extracting the "anchor to git top-level" step into a shared helper (e.g., anchorToGitTopLevel(absPath) (string, error)) that both resolveWorkingDir and resolveRepoDir can call. This keeps the non-git passthrough behavior of resolveRepoDir while eliminating the duplication.
This branch has not been deployed
2 participants
Description
ocr rules checkfailed fast in any directory that is not a git repository (Error: <dir> is not a git repository), even though rule resolution itself has no git dependency. The git requirement was an artifact of the command reusing the review path's directory resolver:runRulesCheckcallsresolveRepoDir(cmd/opencodereview/rules_cmd.go:46), which delegated toresolveWorkingDir(input, true)—requireGithardcoded to true, which is the review path's semantics where the diff concept requires git (cmd/opencodereview/review_cmd.go:457-460before this change).rules.ResolverOptions{}(cmd/opencodereview/rules_cmd.go:51), soRefis empty and the sniffer reads file content from the working tree viaos.Open(internal/config/rules/sniffer.go:99ref gate; thegit showbranch atsniffer.go:120-134never triggers without a ref).This change resolves the rules-check directory with
requireGit=false, matching the existing scan (cmd/opencodereview/shared.go:104-105) and session (cmd/opencodereview/session_cmd.go:623-628) precedents. Inside a git repo,resolveRepoDirstill anchors at the git top-level (git rev-parse --show-toplevel), so rule resolution stays consistent with the review path when run from a monorepo subdirectory (#287); bare repos keep failing loudly instead of silently reusing the subdirectory.Single-function change in
cmd/opencodereview/review_cmd.go, plus tests: two behavior-locked tests flipped to the corrected behavior and two new guard tests added for the non-git case (cmd/opencodereview/git_test.go,cmd/opencodereview/rules_check_test.go).Type of Change
How Has This Been Tested?
make testpasses locally (race enabled; Go 1.26.9, macOS arm64): 24 packagesok, 0 FAILRed/green: with the one-function fix reverted and the new tests kept,
go test ./cmd/opencodereview/ -run 'TestResolveRepoDir|TestRunRulesCheck' -count=1fails exactly the two new assertions (TestResolveRepoDir_NotGitRepo,TestRunRulesCheck/non-git_repo_dir_still_resolves_(#1704)); with the fix applied the same command passes 12/12, and the fullcmd/opencodereviewsuite reports 839 top-level tests passed / 0 failed (~21s, plus 546 subtests).go build ./...andmake check(license headers, English-only,go mod tidy,gofmt,go vet) pass with a clean working tree.End-to-end with the locally built CLI:
ocr rules check --repo /tmp/nogit foo.javaprintedError: /tmp/nogit is not a git repositoryand exited 1 before the fix; after the fix it exits 0 and prints the matching System built-in rule.file_readcall failures in a monorepo #287 top-level anchoring and existing behavior are preserved.Notes: the
ocr review --audience agentpre-commit review command suggested in AGENTS.md was attempted with the locally built CLI but could not resolve an LLM endpoint because none is configured in this environment. Module downloads went throughGOPROXY=https://goproxy.cnbecause proxy.golang.org was unreachable from this network.Checklist
go fmt,go vet)AI/LLM disclosure: this fix was produced with AI assistance on behalf of @akushonkamen, as already disclosed on issue #1704 — triage and root-cause analysis with Claude Code, and the patch, tests, and pre-submission validation prepared with Claude Code and a GLM (Z.ai) coding agent. The commit carries no AI attribution.
Related Issues
Fixes #1704