diff --git a/cmd/opencodereview/flags.go b/cmd/opencodereview/flags.go index 8a73d6d..a15e3fa 100644 --- a/cmd/opencodereview/flags.go +++ b/cmd/opencodereview/flags.go @@ -106,6 +106,7 @@ type reviewOptions struct { concurrency int perFileTimeout int maxTools int + maxGitProcs int preview bool showHelp bool } @@ -127,6 +128,7 @@ func parseReviewFlags(args []string) (reviewOptions, error) { a.StringVar(&opts.audience, "audience", "human", "output audience: human (show progress) or agent (summary only)") a.StringVarP(&opts.background, "background", "b", "", "optional requirement/business context for the review") a.IntVar(&opts.maxTools, "max-tools", 0, "max tool call rounds per file; only takes effect when greater than template default") + a.IntVar(&opts.maxGitProcs, "max-git-procs", 16, "max concurrent git subprocesses") a.BoolVarP(&opts.preview, "preview", "p", false, "preview which files will be reviewed without running the LLM") if err := a.Parse(args); err != nil { @@ -163,6 +165,10 @@ func parseReviewFlags(args []string) (reviewOptions, error) { return opts, fmt.Errorf("--max-tools must be a non-negative integer (0 means use template default)") } + if opts.maxGitProcs < 0 { + return opts, fmt.Errorf("--max-git-procs must be a non-negative integer (0 means use default 16)") + } + return opts, nil } @@ -201,6 +207,7 @@ Flags: -c, --commit string single commit hash or tag to review (vs its parent) -f, --format string output format: text or json (default "text") --concurrency int max concurrent file reviews (default 8) + --max-git-procs int max concurrent git subprocesses (default 16) --from string source ref to start diff from (e.g., 'main') --max-tools int max tool call rounds per file; only takes effect when greater than template default -p, --preview preview which files will be reviewed without running the LLM diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index 16ccdf5..3a8987f 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -12,6 +12,7 @@ import ( "github.com/open-code-review/open-code-review/internal/config/template" "github.com/open-code-review/open-code-review/internal/config/toolsconfig" "github.com/open-code-review/open-code-review/internal/diff" + "github.com/open-code-review/open-code-review/internal/gitcmd" "github.com/open-code-review/open-code-review/internal/llm" "github.com/open-code-review/open-code-review/internal/stdout" "github.com/open-code-review/open-code-review/internal/telemetry" @@ -91,6 +92,8 @@ func runReview(args []string) error { llmClient := llm.NewLLMClient(ep) model := ep.Model + gitRunner := gitcmd.New(opts.maxGitProcs) + collector := tool.NewCommentCollector() mode := tool.ParseReviewMode(opts.from, opts.to, opts.commit) ref, _ := mode.RefValue(opts.to, opts.commit) @@ -98,6 +101,7 @@ func runReview(args []string) error { RepoDir: repoDir, Mode: mode, Ref: ref, + Runner: gitRunner, } tools := buildToolRegistry(collector, fileReader) @@ -119,6 +123,7 @@ func runReview(args []string) error { ConcurrentTaskTimeout: opts.perFileTimeout, Model: model, Background: opts.background, + GitRunner: gitRunner, }) // Silence progress output during execution; restore before Summary in agent mode. @@ -212,15 +217,17 @@ func requireGitRepo(dir string) error { } func runPreview(repoDir string, opts reviewOptions, fileFilter *rules.FileFilter) error { + gitRunner := gitcmd.New(opts.maxGitProcs) ag := agent.New(agent.Args{ RepoDir: repoDir, From: opts.from, To: opts.to, Commit: opts.commit, FileFilter: fileFilter, + GitRunner: gitRunner, }) - preview, err := ag.Preview() + preview, err := ag.Preview(context.Background()) if err != nil { return fmt.Errorf("preview failed: %w", err) } diff --git a/cmd/testdiff/main.go b/cmd/testdiff/main.go index c42fad5..a1c3e77 100644 --- a/cmd/testdiff/main.go +++ b/cmd/testdiff/main.go @@ -3,6 +3,7 @@ package main // go build ./cmd/testdiff/ -o /tmp/testdiff && /tmp/testdiff ... // Or just: go run ./cmd/testdiff/ ... import ( + "context" "encoding/json" "fmt" "os" @@ -28,7 +29,7 @@ func main() { } provider := buildProvider(repoDir, args) - diffs, err := provider.GetDiff() + diffs, err := provider.GetDiff(context.Background()) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) @@ -141,11 +142,11 @@ func resolveRepo(input string) (string, error) { func buildProvider(repoDir string, args cliArgs) *diff.Provider { switch { case args.commit != "": - return diff.NewCommitProvider(repoDir, args.commit) + return diff.NewCommitProvider(repoDir, args.commit, nil) case args.from != "" && args.to != "": - return diff.NewProvider(repoDir, args.from, args.to) + return diff.NewProvider(repoDir, args.from, args.to, nil) default: - return diff.NewWorkspaceProvider(repoDir) + return diff.NewWorkspaceProvider(repoDir, nil) } } diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 2a65d20..d5d8ece 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -15,6 +15,7 @@ import ( "github.com/open-code-review/open-code-review/internal/config/template" "github.com/open-code-review/open-code-review/internal/config/toolsconfig" "github.com/open-code-review/open-code-review/internal/diff" + "github.com/open-code-review/open-code-review/internal/gitcmd" "github.com/open-code-review/open-code-review/internal/llm" "github.com/open-code-review/open-code-review/internal/model" "github.com/open-code-review/open-code-review/internal/session" @@ -111,6 +112,10 @@ type Args struct { // template phases (plan/memory_compression) don't specify one. Model string + // GitRunner limits the total number of concurrent git subprocesses. + // When nil, subprocesses are spawned without a global limit. + GitRunner *gitcmd.Runner + // Session is an optional session history instance for collecting conversation records. // When nil, a default one is created automatically with git branch auto-detected from repoDir. Session *session.SessionHistory @@ -252,7 +257,7 @@ func New(args Args) *Agent { func (a *Agent) Run(ctx context.Context) ([]model.LlmComment, error) { // Step 1: Parse diffs ctx, diffSpan := telemetry.StartSpan(ctx, "diff.parse") - if err := a.loadDiffs(); err != nil { + if err := a.loadDiffs(ctx); err != nil { diffSpan.End() return nil, fmt.Errorf("load diffs: %w", err) } @@ -359,19 +364,19 @@ func (a *Agent) recordWarning(warningType, file, message string) { } // loadDiffs populates the diff-related fields. -func (a *Agent) loadDiffs() error { +func (a *Agent) loadDiffs(ctx context.Context) error { var provider *diff.Provider switch { case a.args.Commit != "": - provider = diff.NewCommitProvider(a.args.RepoDir, a.args.Commit) + provider = diff.NewCommitProvider(a.args.RepoDir, a.args.Commit, a.args.GitRunner) case a.args.From != "" && a.args.To != "": - provider = diff.NewProvider(a.args.RepoDir, a.args.From, a.args.To) + provider = diff.NewProvider(a.args.RepoDir, a.args.From, a.args.To, a.args.GitRunner) default: - provider = diff.NewWorkspaceProvider(a.args.RepoDir) + provider = diff.NewWorkspaceProvider(a.args.RepoDir, a.args.GitRunner) } - parsed, err := provider.GetDiff() + parsed, err := provider.GetDiff(ctx) if err != nil { return fmt.Errorf("get diffs: %w", err) } diff --git a/internal/agent/preview.go b/internal/agent/preview.go index 2966333..0d1003f 100644 --- a/internal/agent/preview.go +++ b/internal/agent/preview.go @@ -1,6 +1,7 @@ package agent import ( + "context" "fmt" allowedext "github.com/open-code-review/open-code-review/internal/config/allowlist" @@ -71,8 +72,8 @@ func (a *Agent) whyExcluded(d model.Diff) ExcludeReason { // Preview loads diffs and applies the filter algorithm, returning structured // preview data without dispatching any LLM calls. -func (a *Agent) Preview() (*DiffPreview, error) { - if err := a.loadDiffs(); err != nil { +func (a *Agent) Preview(ctx context.Context) (*DiffPreview, error) { + if err := a.loadDiffs(ctx); err != nil { return nil, fmt.Errorf("load diffs: %w", err) } diff --git a/internal/diff/git.go b/internal/diff/git.go index dbdc395..b1175a5 100644 --- a/internal/diff/git.go +++ b/internal/diff/git.go @@ -2,12 +2,14 @@ package diff import ( "bytes" + "context" "fmt" "os" "os/exec" "path/filepath" "strings" + "github.com/open-code-review/open-code-review/internal/gitcmd" "github.com/open-code-review/open-code-review/internal/model" ) @@ -43,6 +45,7 @@ const ( type Provider struct { repoDir string mode Mode + runner *gitcmd.Runner // Range mode parameters from, to string // from/to refs for range comparison @@ -54,29 +57,32 @@ type Provider struct { } // NewProvider creates a Provider for range mode: from..to (via merge-base). -func NewProvider(repoDir, from, to string) *Provider { +func NewProvider(repoDir, from, to string, runner *gitcmd.Runner) *Provider { return &Provider{ repoDir: repoDir, mode: ModeRange, from: from, to: to, + runner: runner, } } // NewCommitProvider creates a Provider for commit mode: show changes introduced by a single commit. -func NewCommitProvider(repoDir, commit string) *Provider { +func NewCommitProvider(repoDir, commit string, runner *gitcmd.Runner) *Provider { return &Provider{ repoDir: repoDir, mode: ModeCommit, commit: commit, + runner: runner, } } // NewWorkspaceProvider creates a Provider for workspace mode (current uncommitted changes). -func NewWorkspaceProvider(repoDir string) *Provider { +func NewWorkspaceProvider(repoDir string, runner *gitcmd.Runner) *Provider { return &Provider{ repoDir: repoDir, mode: ModeWorkspace, + runner: runner, } } @@ -91,45 +97,45 @@ func (p *Provider) IsCommitMode() bool { } // MergeBase returns the computed merge-base commit hash for range mode. -func (p *Provider) MergeBase() string { +func (p *Provider) MergeBase(ctx context.Context) string { if p.mode != ModeRange || p.mergeBase != "" { return p.mergeBase } - p.mergeBase = p.computeMergeBase(p.from, p.to) + p.mergeBase = p.computeMergeBase(ctx, p.from, p.to) return p.mergeBase } // GetDiff returns all changes as parsed model.Diff structs. -func (p *Provider) GetDiff() ([]model.Diff, error) { +func (p *Provider) GetDiff(ctx context.Context) ([]model.Diff, error) { var combined strings.Builder switch p.mode { case ModeRange: - base := p.MergeBase() + base := p.MergeBase(ctx) if base == "" { return nil, fmt.Errorf("cannot find merge-base between %s and %s", p.from, p.to) } - out, err := p.runGit("diff", "--no-color", "-U"+fmt.Sprint(DiffContextLines), base, p.to, "--") + out, err := p.runGit(ctx, "diff", "--no-color", "-U"+fmt.Sprint(DiffContextLines), base, p.to, "--") if err != nil { return nil, fmt.Errorf("git diff failed: %w", err) } combined.WriteString(out) case ModeCommit: - out, err := p.runGit("show", "--no-color", "-U"+fmt.Sprint(DiffContextLines), p.commit) + out, err := p.runGit(ctx, "show", "--no-color", "-U"+fmt.Sprint(DiffContextLines), p.commit) if err != nil { return nil, fmt.Errorf("git show failed: %w", err) } combined.WriteString(out) case ModeWorkspace: - tracked, err := p.workspaceTrackedDiff() + tracked, err := p.workspaceTrackedDiff(ctx) if err != nil { return nil, fmt.Errorf("workspace tracked diff failed: %w", err) } combined.WriteString(tracked) - untracked, err := p.untrackedFileDiffs() + untracked, err := p.untrackedFileDiffs(ctx) if err != nil { return nil, fmt.Errorf("untracked file diff failed: %w", err) } @@ -147,7 +153,7 @@ func (p *Provider) GetDiff() ([]model.Diff, error) { ref = p.commit } - diffs, err := ParseDiffText(combined.String(), p.repoDir, ref) + diffs, err := ParseDiffText(ctx, combined.String(), p.repoDir, ref, p.runner) if err != nil { return nil, err } @@ -250,24 +256,27 @@ func (p *Provider) filterDiffs(diffs []model.Diff) []model.Diff { // ---- Internal helpers ---- -func (p *Provider) computeMergeBase(from, to string) string { - out, err := p.runGit("merge-base", from, to) +func (p *Provider) computeMergeBase(ctx context.Context, from, to string) string { + out, err := p.runGit(ctx, "merge-base", from, to) if err != nil { return "" } return strings.TrimSpace(out) } -func (p *Provider) workspaceTrackedDiff() (string, error) { - out, err := p.runGit("diff", "HEAD", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--") +func (p *Provider) workspaceTrackedDiff(ctx context.Context) (string, error) { + out, err := p.runGit(ctx, "diff", "HEAD", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--") if err == nil && out != "" { return out, nil } - return p.runGit("diff", "--staged", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--") + if ctx.Err() != nil { + return "", ctx.Err() + } + return p.runGit(ctx, "diff", "--staged", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--") } -func (p *Provider) untrackedFileDiffs() ([]string, error) { - files, err := p.untrackedFilesList() +func (p *Provider) untrackedFileDiffs(ctx context.Context) ([]string, error) { + files, err := p.untrackedFilesList(ctx) if err != nil { return nil, err } @@ -309,8 +318,8 @@ func (p *Provider) untrackedFileDiffs() ([]string, error) { return results, nil } -func (p *Provider) untrackedFilesList() ([]string, error) { - out, err := p.runGit("ls-files", "--others", "--exclude-standard") +func (p *Provider) untrackedFilesList(ctx context.Context) ([]string, error) { + out, err := p.runGit(ctx, "ls-files", "--others", "--exclude-standard") if err != nil || out == "" { return nil, nil } @@ -328,13 +337,12 @@ func (p *Provider) untrackedFilesList() ([]string, error) { return files, nil } -func (p *Provider) runGit(args ...string) (string, error) { - cmd := exec.Command("git", args...) +func (p *Provider) runGit(ctx context.Context, args ...string) (string, error) { + if p.runner != nil { + return p.runner.Run(ctx, p.repoDir, args...) + } + cmd := exec.CommandContext(ctx, "git", args...) cmd.Dir = p.repoDir out, err := cmd.CombinedOutput() - outStr := string(out) - if err != nil { - return outStr, err - } - return outStr, nil + return string(out), err } diff --git a/internal/diff/parser.go b/internal/diff/parser.go index 2d9ee94..afcea79 100644 --- a/internal/diff/parser.go +++ b/internal/diff/parser.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "github.com/open-code-review/open-code-review/internal/gitcmd" "github.com/open-code-review/open-code-review/internal/model" ) @@ -24,13 +25,15 @@ var ( // ParseDiffText splits the unified diff text into per-file Diff structs. // ref, if non-empty, is a git ref used to read new-file content via // git show instead of reading from the working tree. -func ParseDiffText(diffText string, repoDir string, ref string) ([]model.Diff, error) { +// runner, if non-nil, is used to execute git subprocesses through a +// shared concurrency limiter. +func ParseDiffText(ctx context.Context, diffText string, repoDir string, ref string, runner *gitcmd.Runner) ([]model.Diff, error) { lines := strings.Split(diffText, "\n") var diffs []model.Diff var current *model.Diff var buf strings.Builder - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + ctx, cancel := context.WithTimeout(ctx, 2*time.Minute) defer cancel() for _, line := range lines { @@ -38,7 +41,7 @@ func ParseDiffText(diffText string, repoDir string, ref string) ([]model.Diff, e // Flush previous diff if current != nil { current.Diff = strings.TrimSuffix(buf.String(), "\n") - finalizeDiff(ctx, current, repoDir, ref) + finalizeDiff(ctx, current, repoDir, ref, runner) diffs = append(diffs, *current) buf.Reset() } @@ -74,7 +77,7 @@ func ParseDiffText(diffText string, repoDir string, ref string) ([]model.Diff, e // Flush last diff if current != nil { current.Diff = strings.TrimSuffix(buf.String(), "\n") - finalizeDiff(ctx, current, repoDir, ref) + finalizeDiff(ctx, current, repoDir, ref, runner) diffs = append(diffs, *current) } @@ -83,16 +86,22 @@ func ParseDiffText(diffText string, repoDir string, ref string) ([]model.Diff, e // finalizeDiff reads the new file content. When ref is non-empty it uses // git show to read the file at that ref; otherwise it reads from disk. -func finalizeDiff(ctx context.Context, d *model.Diff, repoDir string, ref string) { +func finalizeDiff(ctx context.Context, d *model.Diff, repoDir string, ref string, runner *gitcmd.Runner) { if d.IsDeleted || d.NewPath == "/dev/null" { d.NewPath = "/dev/null" return } if ref != "" { - cmd := exec.CommandContext(ctx, "git", "-c", "core.quotepath=false", - "show", ref+":"+d.NewPath) - cmd.Dir = repoDir - output, err := cmd.Output() + args := []string{"-c", "core.quotepath=false", "show", ref + ":" + d.NewPath} + var output []byte + var err error + if runner != nil { + output, err = runner.Output(ctx, repoDir, args...) + } else { + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Dir = repoDir + output, err = cmd.Output() + } if err != nil { fmt.Fprintf(os.Stderr, "[ocr] WARNING: cannot read file %s at ref %s: %v\n", d.NewPath, ref, err) diff --git a/internal/gitcmd/runner.go b/internal/gitcmd/runner.go new file mode 100644 index 0000000..c1e76de --- /dev/null +++ b/internal/gitcmd/runner.go @@ -0,0 +1,83 @@ +package gitcmd + +import ( + "bytes" + "context" + "fmt" + "os/exec" +) + +const defaultMaxConcurrent = 16 + +// Runner limits the number of concurrent git subprocesses via an internal +// semaphore. All git command invocations should go through a shared Runner +// instance so that the total system-wide subprocess count stays bounded. +type Runner struct { + sem chan struct{} +} + +// New creates a Runner that allows at most maxConcurrent simultaneous git +// subprocesses. If maxConcurrent <= 0 the default (16) is used. +func New(maxConcurrent int) *Runner { + if maxConcurrent <= 0 { + maxConcurrent = defaultMaxConcurrent + } + return &Runner{sem: make(chan struct{}, maxConcurrent)} +} + +func (r *Runner) acquire(ctx context.Context) error { + if r.sem == nil { + return fmt.Errorf("gitcmd.Runner not initialized; use gitcmd.New()") + } + select { + case r.sem <- struct{}{}: + return nil + case <-ctx.Done(): + return ctx.Err() + } +} + +func (r *Runner) release() { <-r.sem } + +// Run executes a git command and returns the combined stdout+stderr output. +func (r *Runner) Run(ctx context.Context, repoDir string, args ...string) (string, error) { + if err := r.acquire(ctx); err != nil { + return "", err + } + defer r.release() + + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Dir = repoDir + out, err := cmd.CombinedOutput() + return string(out), err +} + +// Output executes a git command and returns stdout only. +func (r *Runner) Output(ctx context.Context, repoDir string, args ...string) ([]byte, error) { + if err := r.acquire(ctx); err != nil { + return nil, err + } + defer r.release() + + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Dir = repoDir + return cmd.Output() +} + +// RunSplit executes a git command and returns stdout and stderr separately. +func (r *Runner) RunSplit(ctx context.Context, repoDir string, args ...string) (string, string, error) { + if err := r.acquire(ctx); err != nil { + return "", "", err + } + defer r.release() + + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Dir = repoDir + + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + err := cmd.Run() + return stdout.String(), stderr.String(), err +} diff --git a/internal/tool/code_search.go b/internal/tool/code_search.go index adc4441..1fbccb5 100644 --- a/internal/tool/code_search.go +++ b/internal/tool/code_search.go @@ -80,6 +80,14 @@ func (p *CodeSearchProvider) runGitGrep(parentCtx context.Context, cmdArgs []str ctx, cancel := context.WithTimeout(parentCtx, gitGrepTimeout) defer cancel() + if p.FileReader.Runner != nil { + stdout, stderr, err := p.FileReader.Runner.RunSplit(ctx, p.FileReader.RepoDir, cmdArgs...) + if ctx.Err() != nil && err != nil { + return "", "", ctx.Err() + } + return stdout, stderr, err + } + cmd := exec.CommandContext(ctx, "git", cmdArgs...) cmd.Dir = p.FileReader.RepoDir diff --git a/internal/tool/file_find.go b/internal/tool/file_find.go index 54cefc6..81a48d7 100644 --- a/internal/tool/file_find.go +++ b/internal/tool/file_find.go @@ -67,14 +67,24 @@ func (p *FileFindProvider) listGitFiles(parentCtx context.Context) ([]string, er ctx, cancel := context.WithTimeout(parentCtx, fileFindTimeout) defer cancel() - var cmd *exec.Cmd + var output []byte + var err error + + var args []string if ref := p.FileReader.Ref; ref != "" { - cmd = exec.CommandContext(ctx, "git", "ls-tree", "-r", "--name-only", ref) + args = []string{"ls-tree", "-r", "--name-only", ref} } else { - cmd = exec.CommandContext(ctx, "git", "ls-files", "--cached", "--others", "--exclude-standard") + args = []string{"ls-files", "--cached", "--others", "--exclude-standard"} } - cmd.Dir = p.FileReader.RepoDir - output, err := cmd.Output() + + if p.FileReader.Runner != nil { + output, err = p.FileReader.Runner.Output(ctx, p.FileReader.RepoDir, args...) + } else { + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Dir = p.FileReader.RepoDir + output, err = cmd.Output() + } + if err != nil { if ctx.Err() != nil { return nil, ctx.Err() diff --git a/internal/tool/filereader.go b/internal/tool/filereader.go index 8609461..655a9ed 100644 --- a/internal/tool/filereader.go +++ b/internal/tool/filereader.go @@ -7,6 +7,8 @@ import ( "os/exec" "path/filepath" "time" + + "github.com/open-code-review/open-code-review/internal/gitcmd" ) // ReviewMode represents the active review mode. @@ -51,7 +53,8 @@ type FileReader struct { Mode ReviewMode // Ref is the git ref to use for ModeRange (--to) or ModeCommit (--commit). // Empty for ModeWorkspace. - Ref string + Ref string + Runner *gitcmd.Runner } // Read returns the full content of a file path (relative to RepoDir), @@ -82,7 +85,16 @@ func (fr *FileReader) readFromGitShow(parentCtx context.Context, path string) (s ctx, cancel := context.WithTimeout(parentCtx, 30*time.Second) defer cancel() - cmd := exec.CommandContext(ctx, "git", "-c", "core.quotepath=false", "show", fr.Ref+":"+path) + args := []string{"-c", "core.quotepath=false", "show", fr.Ref + ":" + path} + if fr.Runner != nil { + output, err := fr.Runner.Output(ctx, fr.RepoDir, args...) + if err != nil { + return "", fmt.Errorf("git show %s:%s: %w", fr.Ref, path, err) + } + return string(output), nil + } + + cmd := exec.CommandContext(ctx, "git", args...) cmd.Dir = fr.RepoDir output, err := cmd.Output() if err != nil {