vrr/open-code-review
2
0
Fork 0
mirror of https://github.com/alibaba/open-code-review.git synced 2026-07-09 17:28:58 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

feat: add ocr scan for full-file code review (#93)

Browse source 
* feat: add ocr scan for full-file code review

Introduce a new top-level subcommand `ocr scan` (alias `s`) that reviews
whole files instead of git diffs. Use cases include reviewing unfamiliar
codebases, pre-migration audits, and ad-hoc per-directory reviews.

Architecture splits scan and diff review at the package level so the two
pipelines can evolve independently:

- internal/scan/      new package: file enumeration via `git ls-files`,
                      full-scan agent, FULL_SCAN_TASK rendering, preview
- internal/llmloop/   new package: shared LLM tool-use loop, three-zone
                      memory compression, CommentWorkerPool, AgentWarning.
                      Both internal/agent and internal/scan delegate to
                      llmloop.Runner; agent and scan never import each other
- internal/agent/     slimmed: LLM loop / compression / token aggregation
                      moved to llmloop; review-only orchestration remains
- internal/model/     new ScanItem (full-file payload) + Preview /
                      PreviewEntry / ExcludeReason shared by both modes
- internal/diff/      new gitignore.go exporting helpers reused by scan
- cmd/opencodereview/ new scan_cmd.go; shared.go consolidates startup
                      (loadCommonContext / loadLLMRuntime), output
                      (emitRunResult, ResultProvider) and stdout silencing
                      (quietHandle); review_cmd.go follows the same shape

Template additions:
- FULL_SCAN_TASK: dedicated prompt with Tool-call discipline guidance to
  reduce gratuitous tool calls per file
- FULL_SCAN_MAX_TOOL_REQUEST_TIMES (default 60): scan-only per-file budget,
  raised over diff's 30 to fit multi-finding files; --max-tools still
  composes (only raise, never lower)

In scan mode, file_read_diff is filtered out of MainToolDefs since it has
no useful semantics without a diff.

Tests cover provider enumeration (with temp git repo), template rendering,
filter passes, dependency budget, flag validation, and excludeToolDef.

* feat(scan): v2 — exclude / non-git / split template / plan / batch / dedup / project-summary

Address design-review feedback by evolving `ocr scan` along seven axes
while keeping `ocr review` behavior unchanged:

1. File size cap is now configurable (ScanTemplate.MaxFileSizeBytes,
   default 2 MiB; previously a hard-coded 5 MiB). The cap exists only to
   bound memory reading; the real review-feasibility gate is the per-file
   token budget downstream.

2. Drop the `--all` flag. Bare `ocr scan` now scans the whole repo;
   `--path` narrows. Less ceremony, fewer redundant flags.

3. New `--exclude` flag on both review and scan. Comma-separated
   gitignore-style patterns; merged with rule.json's exclude layer via
   the new shared.applyCLIExcludes helper.

4. Scan supports non-git directories. internal/scan.Provider chooses
   between `git ls-files` (full .gitignore semantics) and a
   filepath.WalkDir fallback (root .gitignore + ExcludedDirs blocklist)
   per isGitRepo probe. loadCommonContext takes a requireGit bool; review
   keeps the hard requirement, scan relaxes it.

5. Scan configuration lives in its own file. internal/config/template:
   - new ScanTemplate type with LoadScanDefault/ApplyLanguage/Validate
   - new embedded scan_template.json
   - Template loses the FULL_SCAN_* fields (review template unaffected)
   scan.Agent.Args.Template now holds a ScanTemplate; toLoopTemplate
   adapts it for llmloop.Runner.

6. New scan phases — each nil-able in the template and toggleable via a
   CLI flag, so users can revert to v1 behavior trivially:

   * PLAN_TASK (--no-plan): per-file pre-pass that outputs a JSON
     summary + checkpoints, embedded into MAIN_TASK as {{plan_guidance}}.
     formatPlanGuidance renders to markdown; malformed JSON falls back
     to raw text. PLAN_TASK failure never blocks the main loop.

   * BATCH_STRATEGY (--batch): files are grouped before dispatch.
     "none" preserves v1, "by-language" (default) groups by extension,
     "by-directory" groups by first-level subdir. BatchSize caps natural
     groups so a single language with 500 files doesn't form one giant
     batch. Batches are processed sequentially; files within a batch
     remain concurrent up to MaxConcurrency.

   * DEDUP_TASK (--no-dedup): per-batch postprocess that asks the LLM
     to cluster near-duplicate comments. Output is a `groups` JSON;
     every input id must appear exactly once or the result is rejected
     and originals are kept (safety: never silently lose comments).
     CommentCollector grows Snapshot/Since/ReplaceSince for this.

   * PROJECT_SUMMARY_TASK (--no-summary): once-per-run cross-file
     summary appended to text output and surfaced as `project_summary`
     in JSON output. ResultProvider grows ProjectSummary(); agent.Agent
     returns "" (review mode has no project summary).

   All four new LLM steps record token usage via runner.RecordUsage so
   aggregate counters stay accurate.

7. Tests cover the new pure code paths:
   - batch_test.go: 3 strategies, BatchSize cap, language-key edge cases
   - dedup_test.go: groups parser, malformed shapes, fence stripping,
     payload field selection
   - agent_test.go: formatPlanGuidance variants, buildSummaryCommentsList
     truncation, maybeRunPlan skip paths
   - provider_test.go: non-git directory walker fallback
   - template_test.go: ScanTemplate loads / ApplyLanguage / review
     template no longer contains scan fields

The seven phases can be reverted independently by toggling flags or
clearing the corresponding optional template fields; nothing forces the
new behavior on existing review users.

* fix(scan): three real bugs surfaced by SCAN_PLAN_TASK self-review

A v2 end-to-end test (ocr scan --path internal/scan/preview.go) had the
PLAN_TASK phase flag three concrete bugs in the scan package itself.
This commit fixes them and adds regression tests.

1. Preview() mutated a.items as a side-effect.
   Both Preview and Run wrote to a.items. Calling Preview before Run
   silently primed Run with the preview's enumeration instead of
   triggering a fresh listFiles. Preview is documented as a read-only
   dry-run; uphold that. Local variable now; a.items stays nil after
   Preview returns.

2. Preview.result.Entries was nil when there were no items.
   With no items at all the loop never ran, so Entries remained nil and
   JSON marshalling produced "files":null. Pre-allocate to a non-nil
   empty slice so the JSON contract stays "files":[] regardless.

3. Provider.Enumerate and listFilesViaWalk never checked ctx.Done().
   On a large repo a cancelled context would still complete the full
   walk before the caller saw an error (every iteration costs a stat or
   ReadFile syscall). Add the check at the top of each iteration in
   both the git-ls-files path and the walker fallback path; the walker
   returns ctx.Err() so filepath.WalkDir propagates the cancellation.

Three new regression tests pin the contracts:
- TestPreview_DoesNotMutateAgentItems
- TestPreview_EmptyResultEntriesIsNonNilSlice
- TestProvider_Enumerate_RespectsContextCancellation

* feat(scan): cost estimate + token budget cap; fix file_find on non-git dirs

Two cost-control features and one robustness fix, all surfaced by running
the scanner against a real ~870K-token repository.

Cost estimate (internal/scan/estimate.go):
- Before dispatch, Run prints an order-of-magnitude projection of token
  usage (input/output/total), derived from per-file content size × an
  assumed round count, plus the optional plan/dedup/summary phases.
- Deliberately reports tokens only, not dollars — pricing varies per
  provider/model and a precise figure would mislead. Actual usage is still
  reported from the API after the run.

Token budget cap (--max-tokens-budget / ScanTemplate.MaxTokensBudget):
- Caps total token usage for one scan. The gate is checked per file inside
  dispatchBatch, right before acquiring a concurrency slot: if tokens
  already spent plus a look-ahead estimate of the next file would exceed
  the budget, that file and all remaining files are skipped and a
  token_budget_reached warning is recorded.
- An earlier batch-level gate was too coarse: with the default by-language
  batching, a Go-heavy repo puts most files in one batch, so the gate only
  fired between batches and overran the budget ~2.4×. The per-file gate
  bounds overrun to roughly one in-flight file per worker (~1.3× at
  concurrency=1 in testing).
- 0 = unlimited (unchanged default behavior).

Phase-gate helpers (planEnabled/dedupEnabled/summaryEnabled) consolidate
the "template defines it AND --no-* flag not set" checks so the cost
estimate and the dispatch path agree on which phases will actually run.

file_find non-git fallback (internal/tool/file_find.go):
- `git ls-files` exits 128 in a non-git directory, which spammed failures
  when scanning plain directories (scan already supports non-git repos via
  the provider's walker, but the file_find tool did not). Now falls back
  to filepath.WalkDir honoring the root .gitignore and the default
  excluded-dir blocklist when git fails and no specific ref is requested.

Tests:
- estimate_test.go: humanTokens formatting, per-file vs aggregate estimate
  consistency, phase scaling, phase-gate tri-state.
- budget_test.go: fake LLM client drives the gate deterministically —
  verifies dispatch stops before exceeding budget and that 0 = unlimited.
- file_find_test.go: non-git directory fallback finds files, honors
  .gitignore / blocklist, and returns the not-found sentinel correctly.

* docs(readme): document ocr scan subcommand and flags

ocr scan existed but was undiscoverable from the README. Add it to the
intro blurb, Quick Start, the Commands table, Examples, and a dedicated
flags table (path / exclude / preview / max-tokens-budget / no-plan /
no-dedup / no-summary / batch / format / concurrency / rule / repo).
Note non-git support and the pre-run cost estimate. Also backfill the
--exclude flag in the ocr review flags table (added during the v1.3 merge
but never documented).

Flag names and defaults verified against `ocr scan -h`.

* fix(scan): code_search works in non-git directories via git grep --no-index

code_search relied on `git grep`, which exits 128 in a non-git directory —
so `ocr scan` on a plain directory (already supported by file enumeration and
file_find) silently returned errors instead of search results. Detect that
failure and retry with `git grep --no-index --exclude-standard`, which searches
the working tree directly while still honoring .gitignore. Reuses all existing
grep flag/parsing logic; ref-based search still requires a real repo.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(scan): preserve context on compression failure; fix NUL parsing in gitLs

Address three real bugs from the PR #93 automated review that regressed when
compression moved into internal/llmloop:

- Sync compression failure / empty summary now return the original messages
  instead of truncating to the frozen zone, which discarded the whole
  per-file conversation context.
- Async compression now abandons the job on error instead of applying a
  truncated snapshot, and re-applies messages appended while it ran
  (snapshotLen), so concurrent tool results are no longer lost.
- scan Provider.gitLs uses cmd.Output() instead of CombinedOutput() so
  stderr can't corrupt the NUL-delimited (-z) filename parsing.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
css521 2026-06-24 22:07:25 +08:00 • committed by GitHub
parent 4a2370f9cd
commit 18797f8c05
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
38 changed files with 4627 additions and 646 deletions
Download patch file Download diff file Expand all files Collapse all files

2
.gitignore vendored
View file

@ -17,9 +17,11 @@ CHANGELOG.md
vendor/
temp/
.codegraph/
# Ignore downloaded Go binary, but keep JS entry point
!bin/ocr.js
bin/ocr
bin/opencodereview
/opencodereview

49
README.md
View file

@ -35,7 +35,7 @@
Open Code Review is an AI-powered code review CLI tool. It originated as Alibaba Group's internal official AI code review assistant — over the past two years, it has served tens of thousands of developers and identified millions of code defects. After thorough validation at massive scale, we incubated it into an open source project for the community. Simply configure a model endpoint to get started.
It reads Git diffs, sends changed files to a configurable LLM via an agent with tool-use capabilities, and generates structured review comments with line-level precision. The agent can read full file contents, search the codebase, inspect other changed files for context, and produce deep reviews — not just surface-level diff feedback.
It reads Git diffs, sends changed files to a configurable LLM via an agent with tool-use capabilities, and generates structured review comments with line-level precision. The agent can read full file contents, search the codebase, inspect other changed files for context, and produce deep reviews — not just surface-level diff feedback. Beyond diff review, `ocr scan` reviews entire files for auditing unfamiliar codebases or directories that have no meaningful diff.
![Highlights](imgs/highlights-en.png)
@ -227,6 +227,10 @@ ocr review --from main --to feature-branch
# Single commit
ocr review --commit abc123
# Full-file scan — review whole files instead of a diff (no git history needed)
ocr scan # scan the entire repository
ocr scan --path internal/agent # scan a directory or specific files
```
### Integrate with Coding Agents
@ -338,7 +342,8 @@ See the [`examples/`](./examples/) directory for integration examples:
| Command | Alias | Description |
|---------|-------|-------------|
| `ocr review` | `ocr r` | Start a code review |
| `ocr review` | `ocr r` | Start a diff-based code review |
| `ocr scan` | `ocr s` | Review whole files (no diff required) |
| `ocr rules check <file>` | — | Preview which review rule applies to a file path |
| `ocr config provider` | — | Interactive provider setup (built-in, custom, or manual) |
| `ocr config model` | — | Interactive model selection for the active provider |
@ -357,6 +362,7 @@ See the [`examples/`](./examples/) directory for integration examples:
| `--from` | — | — | Source ref (e.g., `main`) |
| `--to` | — | — | Target ref (e.g., `feature-branch`) |
| `--commit` | `-c` | — | Single commit to review |
| `--exclude` | — | — | Comma-separated gitignore-style patterns to skip; merged with rule.json excludes |
| `--preview` | `-p` | `false` | Preview which files will be reviewed without running the LLM |
| `--format` | `-f` | `text` | Output format: `text` or `json` |
| `--concurrency` | — | `8` | Max concurrent file reviews |
@ -369,6 +375,30 @@ See the [`examples/`](./examples/) directory for integration examples:
| `--max-git-procs` | — | built-in | Max concurrent git subprocesses |
| `--tools` | — | — | Path to custom JSON tools config |
### `ocr scan` Flags
`ocr scan` reviews entire files rather than a diff — useful for auditing an unfamiliar
codebase, a pre-migration sweep, or any directory with no meaningful diff. It works in
non-git directories too (it falls back to a filesystem walk that honors `.gitignore`).
| Flag | Shorthand | Default | Description |
|------|-----------|---------|-------------|
| `--path` | — | whole repo | Comma-separated dirs/files to scan |
| `--exclude` | — | — | Comma-separated gitignore-style patterns to skip; merged with rule.json excludes |
| `--preview` | `-p` | `false` | List which files would be scanned without running the LLM |
| `--max-tokens-budget` | — | `0` (unlimited) | Cap total token usage; dispatch stops once exceeded |
| `--no-plan` | — | `false` | Skip the per-file planning pre-pass |
| `--no-dedup` | — | `false` | Skip per-batch de-duplication of similar comments |
| `--no-summary` | — | `false` | Skip the project-level summary |
| `--batch` | — | `by-language` | Batching strategy: `none`, `by-language`, or `by-directory` |
| `--format` | `-f` | `text` | Output format: `text` or `json` (JSON includes a `project_summary` field) |
| `--concurrency` | — | `8` | Max concurrent file scans |
| `--rule` | — | — | Path to custom JSON review rules |
| `--repo` | — | current dir | Repository or directory root to scan |
Before each run, `ocr scan` prints a rough token-cost estimate. Use `--preview` to see the
file list first, and `--max-tokens-budget` to cap spend on large repositories.
## Examples
```bash
@ -407,6 +437,21 @@ ocr review --rule /path/to/my-rules.json
ocr rules check src/main/java/com/example/Foo.java
ocr rules check --rule custom.json src/main/resources/mapper/UserMapper.xml
# Full-file scan: preview the file list first (no LLM calls)
ocr scan --preview
# Scan the whole repo, cap spend at ~500k tokens
ocr scan --max-tokens-budget 500000
# Scan a subdirectory, skipping generated/test files
ocr scan --path internal --exclude '**/*_test.go,**/generated/**'
# Scan a non-git directory with JSON output (includes project_summary)
ocr scan --repo /path/to/plain/dir --format json
# Fastest scan: skip planning, dedup, and the project summary
ocr scan --no-plan --no-dedup --no-summary
# View review session history in browser
ocr viewer
ocr viewer --addr :3000

2
cmd/opencodereview/flags.go
View file

@ -101,6 +101,7 @@ type reviewOptions struct {
from string
to string
commit string
excludes string // --exclude: comma-separated gitignore-style patterns
outputFormat string
audience string // --audience: "human" (default) or "agent"
background string // --background: optional requirement context
@ -124,6 +125,7 @@ func parseReviewFlags(args []string) (reviewOptions, error) {
a.StringVar(&opts.from, "from", "", "source ref to start diff from (e.g., 'main')")
a.StringVar(&opts.to, "to", "", "target ref to end diff at (e.g., 'feature-branch')")
a.StringVarP(&opts.commit, "commit", "c", "", "single commit hash or tag to review (vs its parent)")
a.StringVar(&opts.excludes, "exclude", "", "comma-separated gitignore-style patterns to exclude; merged with rule.json excludes")
a.StringVarP(&opts.outputFormat, "format", "f", "text", "output format: text or json")
a.IntVar(&opts.concurrency, "concurrency", 8, "max concurrent file reviews")
a.IntVar(&opts.perFileTimeout, "timeout", 10, "concurrent task timeout in minutes")

8
cmd/opencodereview/main.go
View file

@ -46,6 +46,8 @@ func dispatch() error {
return nil
case "review", "r":
return runReview(args[1:])
case "scan", "s":
return runScan(args[1:])
case "config":
return runConfig(args[1:])
case "llm":
@ -69,7 +71,8 @@ Usage:
ocr [command]
Commands:
review, r Start a code review
review, r Start a diff-based code review
scan, s Scan entire files (no diff required)
rules Inspect and debug review rules
config Manage configuration settings
llm LLM utility commands
@ -79,6 +82,8 @@ Commands:
Examples:
ocr review --from master --to dev Review diff range
ocr review --commit abc123 Review a single commit
ocr scan Scan every reviewable file in the repo
ocr scan --path internal/agent Scan a single directory
ocr config provider Interactive provider setup
ocr config model Interactive model selection
ocr config set llm.model opus-4-6 Set a config value
@ -87,6 +92,7 @@ Examples:
ocr version Show version info
Use "ocr review -h" for more information about review.
Use "ocr scan -h" for more information about scan.
Use "ocr rules -h" for more information about rules.
Use "ocr config" for more information about config.
Use "ocr llm" for more information about LLM utilities.

17
cmd/opencodereview/output.go
View file

@ -187,11 +187,12 @@ type jsonSummary struct {
}
type jsonOutput struct {
Status string `json:"status"`
Message string `json:"message,omitempty"`
Summary *jsonSummary `json:"summary,omitempty"`
Comments []model.LlmComment `json:"comments"`
Warnings []agent.AgentWarning `json:"warnings,omitempty"`
Status string `json:"status"`
Message string `json:"message,omitempty"`
Summary *jsonSummary `json:"summary,omitempty"`
Comments []model.LlmComment `json:"comments"`
Warnings []agent.AgentWarning `json:"warnings,omitempty"`
ProjectSummary string `json:"project_summary,omitempty"`
}
func outputJSON(comments []model.LlmComment) error {
@ -208,7 +209,8 @@ func outputJSON(comments []model.LlmComment) error {
}
func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentWarning,
filesReviewed, inputTokens, outputTokens, totalTokens, cacheReadTokens, cacheWriteTokens int64, duration time.Duration) error {
filesReviewed, inputTokens, outputTokens, totalTokens, cacheReadTokens, cacheWriteTokens int64,
duration time.Duration, projectSummary string) error {
out := jsonOutput{
Status: "success",
Comments: comments,
@ -222,6 +224,7 @@ func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentW
CacheWriteTokens: cacheWriteTokens,
Elapsed: duration.Round(time.Second).String(),
},
ProjectSummary: projectSummary,
}
if len(comments) == 0 {
if hasSubtaskErrors(warnings) {
@ -311,6 +314,8 @@ func statusBadge(status string) string {
return "\033[36m[R]\033[0m"
case "binary":
return "\033[35m[B]\033[0m"
case "scan":
return "\033[34m[S]\033[0m"
default:
return "[?]"
}

159
cmd/opencodereview/review_cmd.go
View file

@ -9,13 +9,6 @@ import (
"time"
"github.com/open-code-review/open-code-review/internal/agent"
"github.com/open-code-review/open-code-review/internal/config/rules"
"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"
"github.com/open-code-review/open-code-review/internal/tool"
)
@ -23,125 +16,76 @@ import (
func runReview(args []string) error {
opts, err := parseReviewFlags(args)
if err != nil {
return fmt.Errorf("parse flags: %w", err)
// parseReviewFlags already wraps with "parse flags: %w" — return as-is.
return err
}
if opts.showHelp {
printReviewUsage()
return nil
}
if err := requireGitRepo(opts.repoDir); err != nil {
// review path: git repo is required (diff concepts depend on it).
cc, err := loadCommonContext(opts.repoDir, opts.rulePath, opts.maxTools, opts.maxGitProcs, true)
if err != nil {
return err
}
applyCLIExcludes(cc, splitPaths(opts.excludes))
tpl, err := template.LoadDefault()
if err != nil {
return fmt.Errorf("load default template: %w", err)
}
if opts.maxTools > 0 {
tpl.MaxToolRequestTimes = opts.maxTools
}
if err := tpl.Validate(); err != nil {
return fmt.Errorf("invalid config: %w", err)
}
repoDir, err := resolveRepoDir(opts.repoDir)
if err != nil {
return fmt.Errorf("resolve repo: %w", err)
}
if err := validateReviewRefs(repoDir, opts); err != nil {
// Security (#112): reject ref-option injection before any git invocation.
if err := validateReviewRefs(cc.RepoDir, opts); err != nil {
return err
}
if opts.commit != "" && opts.background == "" {
if msg, err := getCommitMessage(repoDir, opts.commit); err == nil && msg != "" {
if msg, err := getCommitMessage(cc.RepoDir, opts.commit); err == nil && msg != "" {
opts.background = msg
}
}
resolver, fileFilter, err := rules.NewResolver(repoDir, opts.rulePath)
if err != nil {
return fmt.Errorf("load rules: %w", err)
}
if opts.preview {
return runPreview(repoDir, opts, fileFilter)
return runPreview(cc, opts)
}
toolEntries, err := toolsconfig.Load(opts.toolConfigPath)
if err != nil {
return fmt.Errorf("load tools: %w", err)
}
planToolDefs := agent.BuildToolDefs(toolEntries, true)
mainToolDefs := agent.BuildToolDefs(toolEntries, false)
cfgPath, err := defaultConfigPath()
rt, err := loadLLMRuntime(cc.Template, opts.toolConfigPath, opts.model)
if err != nil {
return err
}
appCfg, err := LoadAppConfig(cfgPath)
if err != nil {
return fmt.Errorf("load app config: %w", err)
}
var lang string
if appCfg != nil {
lang = appCfg.Language
}
tpl.ApplyLanguage(lang)
ep, err := llm.ResolveEndpointWithModelOverride(cfgPath, opts.model)
if err != nil {
return fmt.Errorf("resolve LLM endpoint: %w", err)
}
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)
fileReader := &tool.FileReader{
RepoDir: repoDir,
RepoDir: cc.RepoDir,
Mode: mode,
Ref: ref,
Runner: gitRunner,
Runner: cc.GitRunner,
}
tools := buildToolRegistry(collector, fileReader)
tools := buildToolRegistry(rt.Collector, fileReader)
ag := agent.New(agent.Args{
RepoDir: repoDir,
RepoDir: cc.RepoDir,
From: opts.from,
To: opts.to,
Commit: opts.commit,
Template: *tpl,
SystemRule: resolver,
FileFilter: fileFilter,
LLMClient: llmClient,
Template: *cc.Template,
SystemRule: cc.Resolver,
FileFilter: cc.FileFilter,
LLMClient: rt.Client,
Tools: tools,
PlanToolDefs: planToolDefs,
MainToolDefs: mainToolDefs,
CommentCollector: collector,
PlanToolDefs: rt.PlanToolDefs,
MainToolDefs: rt.MainToolDefs,
CommentCollector: rt.Collector,
CommentWorkerPool: agent.NewCommentWorkerPool(opts.concurrency),
MaxConcurrency: opts.concurrency,
ConcurrentTaskTimeout: opts.perFileTimeout,
Model: model,
Model: rt.Model,
Background: opts.background,
GitRunner: gitRunner,
GitRunner: cc.GitRunner,
})
// Silence progress output during execution; restore before Summary in agent mode.
var unsilence func()
if opts.outputFormat == "json" || opts.audience == "agent" {
unsilence = stdout.Quiet()
defer func() {
if unsilence != nil {
unsilence()
}
}()
}
// Silence progress output during execution; restored before the trace
// summary in agent-text mode (and on function exit otherwise).
q := newQuietHandle(opts.outputFormat, opts.audience)
defer q.Restore()
ctx, span := telemetry.StartSpan(context.Background(), "review.run")
defer span.End()
@ -153,41 +97,7 @@ func runReview(args []string) error {
return fmt.Errorf("review failed: %w", err)
}
// Resolve line numbers by matching existing_code against diff hunks.
comments = diff.ResolveLineNumbers(comments, ag.Diffs())
// Record summary metrics (files_reviewed is refined by agent.Run).
duration := time.Since(startTime)
telemetry.RecordReviewDuration(ctx, duration)
if len(comments) > 0 {
telemetry.RecordCommentsGenerated(ctx, int64(len(comments)))
}
// If no files were reviewed (e.g. workspace has no changes), inform the caller in JSON mode.
if opts.outputFormat == "json" && len(comments) == 0 && ag.FilesReviewed() == 0 {
return outputJSONNoFiles()
}
// In agent mode (text output), restore stdout so Summary reaches the terminal.
if opts.audience == "agent" && opts.outputFormat != "json" && unsilence != nil {
unsilence()
unsilence = nil
}
if opts.outputFormat != "json" {
telemetry.PrintTraceSummary(ag.FilesReviewed(), int64(len(comments)), ag.TotalInputTokens(), ag.TotalOutputTokens(), ag.TotalTokensUsed(), ag.TotalCacheReadTokens(), ag.TotalCacheWriteTokens(), duration)
}
if opts.outputFormat == "json" {
return outputJSONWithWarnings(comments, ag.Warnings(), ag.FilesReviewed(), ag.TotalInputTokens(), ag.TotalOutputTokens(), ag.TotalTokensUsed(), ag.TotalCacheReadTokens(), ag.TotalCacheWriteTokens(), duration)
}
if opts.audience == "agent" {
outputTextWithWarnings(comments, ag.Warnings())
return nil
}
outputTextWithWarnings(comments, ag.Warnings())
return nil
return emitRunResult(ctx, ag, comments, startTime, opts.outputFormat, opts.audience, q)
}
func resolveRepoDir(input string) (string, error) {
@ -222,6 +132,8 @@ func requireGitRepo(dir string) error {
return nil
}
// validateReviewRefs rejects ref-option injection (#112): any --from/--to/
// --commit value must be a real commit ref and must not start with '-'.
func validateReviewRefs(repoDir string, opts reviewOptions) error {
refs := []struct {
flag string
@ -249,15 +161,14 @@ func validateReviewRefs(repoDir string, opts reviewOptions) error {
return nil
}
func runPreview(repoDir string, opts reviewOptions, fileFilter *rules.FileFilter) error {
gitRunner := gitcmd.New(opts.maxGitProcs)
func runPreview(cc *commonContext, opts reviewOptions) error {
ag := agent.New(agent.Args{
RepoDir: repoDir,
RepoDir: cc.RepoDir,
From: opts.from,
To: opts.to,
Commit: opts.commit,
FileFilter: fileFilter,
GitRunner: gitRunner,
FileFilter: cc.FileFilter,
GitRunner: cc.GitRunner,
})
preview, err := ag.Preview(context.Background())

288
cmd/opencodereview/scan_cmd.go Normal file
View file

@ -0,0 +1,288 @@
package main
import (
"context"
"fmt"
"strings"
"time"
"github.com/open-code-review/open-code-review/internal/config/template"
"github.com/open-code-review/open-code-review/internal/llmloop"
"github.com/open-code-review/open-code-review/internal/scan"
"github.com/open-code-review/open-code-review/internal/telemetry"
"github.com/open-code-review/open-code-review/internal/tool"
)
// scanOptions mirrors reviewOptions for the full-scan subcommand. The two
// types are kept separate so the scan flag set can evolve independently of
// the diff-based review flags (e.g. --from/--to/--commit make no sense here).
//
// Bare `ocr scan` (no --path) scans the entire repository; --path narrows.
type scanOptions struct {
toolConfigPath string
rulePath string
repoDir string
paths string // comma-separated relative paths; empty = whole repo
excludes string // comma-separated gitignore-style exclude patterns
outputFormat string
audience string
background string
concurrency int
perFileTimeout int
maxTools int
maxGitProcs int
preview bool
noPlan bool // --no-plan: skip the PLAN_TASK pre-pass per file
noDedup bool // --no-dedup: skip the per-batch DEDUP_TASK
noSummary bool // --no-summary: skip the post-run PROJECT_SUMMARY_TASK
batch string // --batch: override scan template's BATCH_STRATEGY
maxTokensBudget int // --max-tokens-budget: cap total token usage; 0 = unlimited
model string // --model: override resolved LLM model for this scan
showHelp bool
}
func parseScanFlags(args []string) (scanOptions, error) {
a := newOcrFlagSet("ocr scan")
opts := scanOptions{}
a.StringVar(&opts.toolConfigPath, "tools", "", "path to JSON tools config file (default: embedded)")
a.StringVar(&opts.rulePath, "rule", "", "path to JSON file with system review rules")
a.StringVar(&opts.repoDir, "repo", "", "root directory of the git repository (default: current dir)")
a.StringVar(&opts.paths, "path", "", "comma-separated repo-relative directories or files to scan (default: whole repo)")
a.StringVar(&opts.excludes, "exclude", "", "comma-separated gitignore-style patterns to exclude; merged with rule.json excludes")
a.StringVarP(&opts.outputFormat, "format", "f", "text", "output format: text or json")
a.IntVar(&opts.concurrency, "concurrency", 8, "max concurrent file scans")
a.IntVar(&opts.perFileTimeout, "timeout", 10, "concurrent task timeout in minutes")
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 scan")
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 scanned without running the LLM")
a.BoolVar(&opts.noPlan, "no-plan", false, "skip the per-file PLAN_TASK pre-pass (one fewer LLM call per file; may reduce review focus)")
a.BoolVar(&opts.noDedup, "no-dedup", false, "skip the per-batch DEDUP_TASK (keeps raw comments; one fewer LLM call per batch)")
a.BoolVar(&opts.noSummary, "no-summary", false, "skip the post-run PROJECT_SUMMARY_TASK (no project-level markdown summary)")
a.StringVar(&opts.batch, "batch", "", "override BATCH_STRATEGY from scan template: none | by-language | by-directory")
a.IntVar(&opts.maxTokensBudget, "max-tokens-budget", 0, "cap total token usage (input+output); dispatch stops once exceeded (0 = unlimited)")
a.StringVar(&opts.model, "model", "", "override LLM model for this scan (e.g., claude-opus-4-6)")
if err := a.Parse(args); err != nil {
return opts, fmt.Errorf("parse flags: %w", err)
}
opts.showHelp = a.showHelp
if opts.showHelp {
return opts, nil
}
switch opts.audience {
case "human", "agent":
default:
return opts, fmt.Errorf("invalid --audience value %q: must be 'human' or 'agent'", opts.audience)
}
if opts.maxTools < 0 {
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)")
}
if opts.maxTokensBudget < 0 {
return opts, fmt.Errorf("--max-tokens-budget must be a non-negative integer (0 means unlimited)")
}
return opts, nil
}
func splitPaths(raw string) []string {
if raw == "" {
return nil
}
parts := strings.Split(raw, ",")
out := make([]string, 0, len(parts))
for _, p := range parts {
p = strings.TrimSpace(p)
if p != "" {
out = append(out, p)
}
}
return out
}
func runScan(args []string) error {
opts, err := parseScanFlags(args)
if err != nil {
// parseScanFlags already wraps with "parse flags: %w" — return as-is.
return err
}
if opts.showHelp {
printScanUsage()
return nil
}
// scan path: git is preferred (more accurate .gitignore handling) but not required;
// provider falls back to filepath.Walk when the dir is not a git repo.
cc, err := loadCommonContext(opts.repoDir, opts.rulePath, opts.maxTools, opts.maxGitProcs, false)
if err != nil {
return err
}
applyCLIExcludes(cc, splitPaths(opts.excludes))
// scan owns its own template (scan_template.json) independent from the
// diff-review template loaded by loadCommonContext above. Apply --max-tools
// as an "only raise" override to the scan template's per-file budget.
scanTpl, err := template.LoadScanDefault()
if err != nil {
return fmt.Errorf("load scan template: %w", err)
}
if err := scanTpl.Validate(); err != nil {
return fmt.Errorf("invalid scan template: %w", err)
}
if opts.maxTools > scanTpl.MaxToolRequestTimes {
scanTpl.MaxToolRequestTimes = opts.maxTools
}
if opts.batch != "" {
// CLI override of BATCH_STRATEGY; validated downstream by parseBatchStrategy
// (unknown values silently fall back to "none").
scanTpl.BatchStrategy = opts.batch
}
// Token budget: --max-tokens-budget overrides the template value when set.
budget := scanTpl.MaxTokensBudget
if opts.maxTokensBudget > 0 {
budget = int64(opts.maxTokensBudget)
}
scanPaths := splitPaths(opts.paths)
if opts.preview {
return runScanPreview(cc, scanTpl, scanPaths)
}
rt, err := loadLLMRuntime(cc.Template, opts.toolConfigPath, opts.model)
if err != nil {
return err
}
// Apply language to the scan template too (loadLLMRuntime only mutates
// the diff-review template it was handed).
if rt.AppCfg != nil {
scanTpl.ApplyLanguage(rt.AppCfg.Language)
}
// file_read_diff is meaningless in scan mode (no diff exists). Hiding it
// from MainToolDefs stops the LLM from burning tool-call rounds probing
// for diff content that does not exist.
scanToolDefs := excludeToolDef(rt.MainToolDefs, "file_read_diff")
// Scan mode always reads file contents from the working tree.
fileReader := &tool.FileReader{
RepoDir: cc.RepoDir,
Mode: tool.ModeWorkspace,
Runner: cc.GitRunner,
}
tools := buildToolRegistry(rt.Collector, fileReader)
ag := scan.NewAgent(scan.Args{
RepoDir: cc.RepoDir,
Paths: scanPaths,
Template: *scanTpl,
SystemRule: cc.Resolver,
FileFilter: cc.FileFilter,
LLMClient: rt.Client,
Tools: tools,
MainToolDefs: scanToolDefs,
CommentCollector: rt.Collector,
CommentWorkerPool: llmloop.NewCommentWorkerPool(opts.concurrency),
MaxConcurrency: opts.concurrency,
ConcurrentTaskTimeout: opts.perFileTimeout,
Model: rt.Model,
Background: opts.background,
GitRunner: cc.GitRunner,
MaxFileSizeBytes: scanTpl.MaxFileSizeBytes,
MaxTokensBudget: budget,
SkipPlan: opts.noPlan,
SkipDedup: opts.noDedup,
SkipSummary: opts.noSummary,
})
q := newQuietHandle(opts.outputFormat, opts.audience)
defer q.Restore()
ctx, span := telemetry.StartSpan(context.Background(), "scan.run")
defer span.End()
startTime := time.Now()
comments, err := ag.Run(ctx)
if err != nil {
telemetry.SetAttr(span, "error", err.Error())
return fmt.Errorf("scan failed: %w", err)
}
return emitRunResult(ctx, ag, comments, startTime, opts.outputFormat, opts.audience, q)
}
func runScanPreview(cc *commonContext, scanTpl *template.ScanTemplate, scanPaths []string) error {
ag := scan.NewAgent(scan.Args{
RepoDir: cc.RepoDir,
Paths: scanPaths,
FileFilter: cc.FileFilter,
GitRunner: cc.GitRunner,
MaxFileSizeBytes: scanTpl.MaxFileSizeBytes,
// Template's prompt fields are unused by Preview; pass the same
// value so MaxFileSizeBytes is consistent.
Template: *scanTpl,
})
preview, err := ag.Preview(context.Background())
if err != nil {
return fmt.Errorf("scan preview failed: %w", err)
}
outputPreviewText(preview)
return nil
}
func printScanUsage() {
fmt.Println(`OpenCodeReview - Full-File Scan
Usage:
ocr scan [flags]
ocr s [flags] (alias)
Examples:
# Scan the entire repository (default when no --path is given)
ocr scan
# Scan a single directory
ocr scan --path internal/agent
# Scan multiple files
ocr scan --path internal/agent/agent.go,internal/diff/scan.go
# Exclude generated files / fixtures
ocr scan --exclude '**/generated/*,**/testdata/*'
# Preview which files would be scanned without calling the LLM
ocr scan --preview
# Skip the per-file PLAN_TASK pre-pass (saves ~1 LLM call per file, may
# reduce review focus)
ocr scan --no-plan
Flags:
--path string comma-separated repo-relative dirs/files to scan (default: whole repo)
--exclude string comma-separated gitignore-style patterns to exclude (merged with rule.json)
--no-plan skip the per-file PLAN_TASK pre-pass (faster, less focused)
--no-dedup skip the per-batch DEDUP_TASK (keeps raw comments)
--no-summary skip the post-run PROJECT_SUMMARY_TASK
--batch string override BATCH_STRATEGY: none | by-language | by-directory
--max-tokens-budget int cap total token usage; dispatch stops once exceeded (0 = unlimited)
--model string override LLM model for this scan (e.g., claude-opus-4-6)
--audience string output audience: human (show progress) or agent (summary only) (default "human")
-b, --background string optional requirement/business context for the scan
-f, --format string output format: text or json (default "text")
--concurrency int max concurrent file scans (default 8)
--max-git-procs int max concurrent git subprocesses (default 16)
--max-tools int max tool call rounds per file; only takes effect when greater than template default
-p, --preview preview which files will be scanned without running the LLM
--repo string root directory of the git repository (default: current dir)
--rule string path to JSON file with system review rules
--timeout int concurrent task timeout in minutes (default 10)
--tools string path to JSON tools config file (default: embedded)`)
}

142
cmd/opencodereview/scan_cmd_test.go Normal file
View file

@ -0,0 +1,142 @@
package main
import (
"reflect"
"strings"
"testing"
"github.com/open-code-review/open-code-review/internal/llm"
)
func TestExcludeToolDef(t *testing.T) {
defs := []llm.ToolDef{
{Type: "function", Function: llm.FunctionDef{Name: "task_done"}},
{Type: "function", Function: llm.FunctionDef{Name: "file_read"}},
{Type: "function", Function: llm.FunctionDef{Name: "file_read_diff"}},
{Type: "function", Function: llm.FunctionDef{Name: "code_comment"}},
}
got := excludeToolDef(defs, "file_read_diff")
if len(got) != 3 {
t.Fatalf("expected 3 defs, got %d", len(got))
}
for _, d := range got {
if d.Function.Name == "file_read_diff" {
t.Errorf("file_read_diff should have been removed")
}
}
// Input slice must not be mutated.
if len(defs) != 4 {
t.Errorf("input slice was mutated: len=%d, want 4", len(defs))
}
}
func TestExcludeToolDef_AbsentName(t *testing.T) {
defs := []llm.ToolDef{
{Type: "function", Function: llm.FunctionDef{Name: "task_done"}},
}
got := excludeToolDef(defs, "does_not_exist")
if !reflect.DeepEqual(got, defs) {
t.Errorf("removing absent name should return identical content")
}
}
func TestSplitPaths(t *testing.T) {
tests := []struct {
name string
in string
want []string
}{
{"empty", "", nil},
{"single", "internal/agent", []string{"internal/agent"}},
{"multiple", "a.go,b.go,c.go", []string{"a.go", "b.go", "c.go"}},
{"trims whitespace", " a.go , b.go ", []string{"a.go", "b.go"}},
{"drops empty segments", "a.go,,b.go,", []string{"a.go", "b.go"}},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := splitPaths(tt.in)
if !reflect.DeepEqual(got, tt.want) {
t.Errorf("splitPaths(%q) = %v, want %v", tt.in, got, tt.want)
}
})
}
}
func TestParseScanFlags_BareCommandScansWholeRepo(t *testing.T) {
opts, err := parseScanFlags([]string{}) // no flags
if err != nil {
t.Fatalf("bare `ocr scan` should not error, got: %v", err)
}
if opts.paths != "" {
t.Errorf("default paths should be empty (= whole repo), got %q", opts.paths)
}
}
func TestParseScanFlags_RejectsInvalidAudience(t *testing.T) {
_, err := parseScanFlags([]string{"--audience", "robot"})
if err == nil {
t.Fatal("expected error for invalid --audience")
}
if !strings.Contains(err.Error(), "invalid --audience") {
t.Errorf("error message = %q; want invalid --audience", err.Error())
}
}
func TestParseScanFlags_RejectsNegativeMaxTools(t *testing.T) {
_, err := parseScanFlags([]string{"--max-tools", "-1"})
if err == nil {
t.Fatal("expected error for negative --max-tools")
}
if !strings.Contains(err.Error(), "--max-tools") {
t.Errorf("error message = %q; want it to mention --max-tools", err.Error())
}
}
func TestParseScanFlags_RejectsNegativeMaxGitProcs(t *testing.T) {
_, err := parseScanFlags([]string{"--max-git-procs", "-3"})
if err == nil {
t.Fatal("expected error for negative --max-git-procs")
}
if !strings.Contains(err.Error(), "--max-git-procs") {
t.Errorf("error message = %q; want it to mention --max-git-procs", err.Error())
}
}
func TestParseScanFlags_DefaultsValid(t *testing.T) {
opts, err := parseScanFlags([]string{}) // bare command
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if opts.paths != "" {
t.Errorf("opts = %+v; want paths empty (whole repo)", opts)
}
if opts.audience != "human" {
t.Errorf("default audience = %q, want \"human\"", opts.audience)
}
if opts.outputFormat != "text" {
t.Errorf("default outputFormat = %q, want \"text\"", opts.outputFormat)
}
if opts.concurrency != 8 {
t.Errorf("default concurrency = %d, want 8", opts.concurrency)
}
}
func TestParseScanFlags_PathNarrowsScope(t *testing.T) {
opts, err := parseScanFlags([]string{"--path", "internal/agent,internal/diff"})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if got := splitPaths(opts.paths); !reflect.DeepEqual(got, []string{"internal/agent", "internal/diff"}) {
t.Errorf("splitPaths(opts.paths) = %v", got)
}
}
func TestParseScanFlags_HelpFlag(t *testing.T) {
opts, err := parseScanFlags([]string{"-h"})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if !opts.showHelp {
t.Error("opts.showHelp should be true when -h is supplied")
}
}

285
cmd/opencodereview/shared.go Normal file
View file

@ -0,0 +1,285 @@
package main
import (
"context"
"fmt"
"os"
"path/filepath"
"time"
"github.com/open-code-review/open-code-review/internal/agent"
"github.com/open-code-review/open-code-review/internal/config/rules"
"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/stdout"
"github.com/open-code-review/open-code-review/internal/telemetry"
"github.com/open-code-review/open-code-review/internal/tool"
)
// commonContext bundles the state that both `ocr review` and `ocr scan`
// need to load *before* deciding whether to dispatch a preview or a real
// LLM session: a validated template, the resolved repo path, review rules,
// and a shared git subprocess limiter.
type commonContext struct {
Template *template.Template
RepoDir string
Resolver rules.Resolver
FileFilter *rules.FileFilter
GitRunner *gitcmd.Runner
// IsGitRepo reports whether RepoDir is inside a git repository. Always
// true when requireGit was set; may be false when scan accepts non-git
// directories.
IsGitRepo bool
}
// loadCommonContext validates the working directory, loads the embedded
// template, raises MaxToolRequestTimes when maxTools exceeds the default,
// resolves the absolute repo path, loads system review rules, and creates
// the global git subprocess limiter. Both review and scan callers go
// through this so the startup sequence stays consistent.
//
// requireGit=true fails fast when the directory is not a git repo (review
// path: diff concept requires git). requireGit=false allows non-git
// directories (scan path: provider falls back to filepath.Walk).
func loadCommonContext(repoDirInput, rulePath string, maxTools, maxGitProcs int, requireGit bool) (*commonContext, error) {
tpl, err := template.LoadDefault()
if err != nil {
return nil, fmt.Errorf("load default template: %w", err)
}
if maxTools > tpl.MaxToolRequestTimes {
tpl.MaxToolRequestTimes = maxTools
}
if err := tpl.Validate(); err != nil {
return nil, fmt.Errorf("invalid config: %w", err)
}
repoDir, isGit, err := resolveWorkingDir(repoDirInput, requireGit)
if err != nil {
return nil, err
}
resolver, fileFilter, err := rules.NewResolver(repoDir, rulePath)
if err != nil {
return nil, fmt.Errorf("load rules: %w", err)
}
return &commonContext{
Template: tpl,
RepoDir: repoDir,
Resolver: resolver,
FileFilter: fileFilter,
GitRunner: gitcmd.New(maxGitProcs),
IsGitRepo: isGit,
}, nil
}
// resolveWorkingDir returns (absPath, isGitRepo, err). When requireGit is
// true, returns an error if the directory is not a git repo. When false,
// returns IsGitRepo=false instead of erroring (scan path uses this).
func resolveWorkingDir(input string, requireGit bool) (string, bool, error) {
if input == "" {
wd, err := os.Getwd()
if err != nil {
return "", false, fmt.Errorf("get working directory: %w", err)
}
input = wd
}
absPath, err := filepath.Abs(input)
if err != nil {
return "", false, fmt.Errorf("resolve absolute path: %w", err)
}
if _, statErr := os.Stat(absPath); statErr != nil {
return "", false, fmt.Errorf("stat %s: %w", absPath, statErr)
}
out, err := runGitCmd(absPath, "rev-parse", "--git-dir")
isGit := err == nil && len(out) > 0
if !isGit && requireGit {
return "", false, fmt.Errorf("%s is not a git repository", absPath)
}
return absPath, isGit, nil
}
// llmRuntime bundles the LLM-side state both subcommands need once they've
// decided to actually run a session: tool definitions, an app-language
// adjusted template (mutated in place via ApplyLanguage), the LLM client,
// the resolved model name, and a fresh comment collector.
type llmRuntime struct {
Client llm.LLMClient
Model string
PlanToolDefs []llm.ToolDef
MainToolDefs []llm.ToolDef
Collector *tool.CommentCollector
AppCfg *Config
}
// loadLLMRuntime loads tool defs from toolConfigPath, reads the app config
// from the user's default config path (applying the configured language to
// tpl — defaulting when the config file is absent), resolves the LLM
// endpoint (honoring modelOverride from --model when non-empty), and
// returns the runtime bundle. tpl is mutated in place.
func loadLLMRuntime(tpl *template.Template, toolConfigPath, modelOverride string) (*llmRuntime, error) {
toolEntries, err := toolsconfig.Load(toolConfigPath)
if err != nil {
return nil, fmt.Errorf("load tools: %w", err)
}
planToolDefs := agent.BuildToolDefs(toolEntries, true)
mainToolDefs := agent.BuildToolDefs(toolEntries, false)
cfgPath, err := defaultConfigPath()
if err != nil {
return nil, err
}
appCfg, err := LoadAppConfig(cfgPath)
if err != nil {
return nil, fmt.Errorf("load app config: %w", err)
}
// Apply the language directive even when the config file is missing
// (upstream #fix: ApplyLanguage with empty lang falls back to default).
var lang string
if appCfg != nil {
lang = appCfg.Language
}
tpl.ApplyLanguage(lang)
ep, err := llm.ResolveEndpointWithModelOverride(cfgPath, modelOverride)
if err != nil {
return nil, fmt.Errorf("resolve LLM endpoint: %w", err)
}
return &llmRuntime{
Client: llm.NewLLMClient(ep),
Model: ep.Model,
PlanToolDefs: planToolDefs,
MainToolDefs: mainToolDefs,
Collector: tool.NewCommentCollector(),
AppCfg: appCfg,
}, nil
}
// applyCLIExcludes appends user-supplied --exclude patterns (already split
// into a []string) onto cc.FileFilter.Exclude. Creates the FileFilter if
// none was returned by rule.json layers. Idempotent on empty input.
func applyCLIExcludes(cc *commonContext, patterns []string) {
if len(patterns) == 0 {
return
}
if cc.FileFilter == nil {
cc.FileFilter = &rules.FileFilter{}
}
cc.FileFilter.Exclude = append(cc.FileFilter.Exclude, patterns...)
}
// excludeToolDef returns a copy of defs with any entries whose function name
// matches name removed. Used by `ocr scan` to hide tools that don't make
// sense in full-scan mode (e.g. file_read_diff).
func excludeToolDef(defs []llm.ToolDef, name string) []llm.ToolDef {
out := make([]llm.ToolDef, 0, len(defs))
for _, d := range defs {
if d.Function.Name == name {
continue
}
out = append(out, d)
}
return out
}
// quietHandle wraps a stdout.Quiet() restorer so callers can `defer
// q.Restore()` for safety while emitRunResult restores it early when the
// agent-text audience needs the trace summary on the user's terminal.
// Restore is idempotent.
type quietHandle struct {
fn func()
}
// newQuietHandle silences stdout when outputFormat=="json" or
// audience=="agent"; otherwise the returned handle is a no-op restorer.
func newQuietHandle(outputFormat, audience string) *quietHandle {
h := &quietHandle{}
if outputFormat == "json" || audience == "agent" {
h.fn = stdout.Quiet()
}
return h
}
// Restore re-enables stdout. Safe to call multiple times.
func (h *quietHandle) Restore() {
if h == nil || h.fn == nil {
return
}
h.fn()
h.fn = nil
}
// ResultProvider abstracts the metadata both internal/agent.Agent and
// internal/scan.Agent expose post-run, so emitRunResult can finalize
// either without knowing which kind it has.
type ResultProvider interface {
Diffs() []model.Diff
FilesReviewed() int64
TotalInputTokens() int64
TotalOutputTokens() int64
TotalTokensUsed() int64
TotalCacheReadTokens() int64
TotalCacheWriteTokens() int64
Warnings() []agent.AgentWarning
// ProjectSummary is the markdown project-level summary produced by
// scan's PROJECT_SUMMARY_TASK. Empty for review mode and for scans
// that skipped / failed the summary phase.
ProjectSummary() string
}
// emitRunResult is the post-LLM-run finalization shared by `ocr review` and
// `ocr scan`: resolves comment line numbers, records telemetry, restores
// stdout early for agent-text audiences so the summary is visible, prints
// the trace summary, and writes the result in the requested format.
//
// q is the silencing handle returned by newQuietHandle; pass nil if no
// silencing was set up (in which case the early restore is a no-op).
func emitRunResult(
ctx context.Context,
ag ResultProvider,
comments []model.LlmComment,
startTime time.Time,
outputFormat, audience string,
q *quietHandle,
) error {
comments = diff.ResolveLineNumbers(comments, ag.Diffs())
duration := time.Since(startTime)
telemetry.RecordReviewDuration(ctx, duration)
if len(comments) > 0 {
telemetry.RecordCommentsGenerated(ctx, int64(len(comments)))
}
if outputFormat == "json" && len(comments) == 0 && ag.FilesReviewed() == 0 {
return outputJSONNoFiles()
}
// Agent-text audiences need stdout back before PrintTraceSummary so the
// summary line lands on their terminal.
if audience == "agent" && outputFormat != "json" {
q.Restore()
}
if outputFormat != "json" {
telemetry.PrintTraceSummary(ag.FilesReviewed(), int64(len(comments)),
ag.TotalInputTokens(), ag.TotalOutputTokens(), ag.TotalTokensUsed(),
ag.TotalCacheReadTokens(), ag.TotalCacheWriteTokens(), duration)
}
if outputFormat == "json" {
return outputJSONWithWarnings(comments, ag.Warnings(), ag.FilesReviewed(),
ag.TotalInputTokens(), ag.TotalOutputTokens(), ag.TotalTokensUsed(),
ag.TotalCacheReadTokens(), ag.TotalCacheWriteTokens(), duration,
ag.ProjectSummary())
}
outputTextWithWarnings(comments, ag.Warnings())
if summary := ag.ProjectSummary(); summary != "" {
fmt.Printf("\n\n──────── Project Summary ────────\n\n%s\n", summary)
}
return nil
}

428
internal/agent/agent.go
View file

@ -15,6 +15,7 @@ import (
"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/llmloop"
"github.com/open-code-review/open-code-review/internal/model"
"github.com/open-code-review/open-code-review/internal/session"
"github.com/open-code-review/open-code-review/internal/stdout"
@ -22,6 +23,19 @@ import (
"github.com/open-code-review/open-code-review/internal/tool"
)
// AgentWarning is re-exported from llmloop for backwards compatibility with
// existing callers (cmd/opencodereview/output.go).
type AgentWarning = llmloop.AgentWarning
// CommentWorkerPool is re-exported from llmloop for backwards compatibility.
type CommentWorkerPool = llmloop.CommentWorkerPool
// NewCommentWorkerPool delegates to llmloop.NewCommentWorkerPool.
func NewCommentWorkerPool(workerCount int) *CommentWorkerPool {
return llmloop.NewCommentWorkerPool(workerCount)
}
// Args holds all dependencies and configuration needed to run a review session.
type Args struct {
// RepoDir is the root of the git repository.
@ -36,6 +50,7 @@ type Args struct {
// ReviewMode is one of "workspace", "range", or "commit".
// When empty, it is derived from From/To/Commit at session creation time.
// Full-scan reviews are owned by internal/scan and never reach this Args.
ReviewMode string
// Template loaded from YAML config file.
@ -98,81 +113,18 @@ type Args struct {
Session *session.SessionHistory
}
// AgentWarning describes a non-fatal warning recorded during review.
type AgentWarning struct {
File string `json:"file"`
Message string `json:"message"`
Type string `json:"type"`
}
// Agent orchestrates the AI-powered code review.
// Agent orchestrates the AI-powered code review. LLM tool-use loop / memory
// compression / token aggregation now live in internal/llmloop.Runner; this
// struct holds the diff-side state and orchestrates per-file subtasks.
type Agent struct {
args Args
diffs []model.Diff // parsed diffs
totalInsertions int64
totalDeletions int64
currentDate string
session *session.SessionHistory
totalInputTokens int64 // accumulated input/prompt tokens, accessed atomically
totalOutputTokens int64 // accumulated completion tokens, accessed atomically
totalCacheReadTokens int64 // accumulated cache read tokens, accessed atomically
totalCacheWriteTokens int64 // accumulated cache write tokens, accessed atomically
subtaskFailed int64 // count of failed subtasks, accessed atomically
warningsMu sync.Mutex
warnings []AgentWarning
compressionMu sync.Mutex
pendingJob *compressionJob
}
// CommentWorkerPool manages a fixed-size pool of workers dedicated to
// processing code-review comment post-steps (line-range tracking,
// re-tracking, reflection, suggestion validation) asynchronously.
//
// These steps can be time-consuming (network calls to LLM, external APIs,
// heavy computation). By offloading them to a worker pool the main LLM
// tool-use loop stays unblocked, reducing overall latency - just like the
// Java implementation uses a dedicated subtaskExecutor for the CODE_COMMENT
// tool (see CodeReviewNativeAgent.executeToolCall ~L640-642).
type CommentWorkerPool struct {
semaphore chan struct{}
wg sync.WaitGroup
resultsMu sync.Mutex
results []model.LlmComment
}
// NewCommentWorkerPool creates a pool with the given concurrency limit.
func NewCommentWorkerPool(workerCount int) *CommentWorkerPool {
if workerCount <= 0 {
workerCount = 8
}
return &CommentWorkerPool{
semaphore: make(chan struct{}, workerCount),
}
}
// Submit runs f in a background goroutine bounded by the semaphore.
// When f completes its return value is collected internally.
func (p *CommentWorkerPool) Submit(f func() ([]model.LlmComment, error)) {
p.wg.Add(1)
go func() {
defer p.wg.Done()
p.semaphore <- struct{}{} // acquire
defer func() { <-p.semaphore }() // release
comments, err := f()
if err != nil {
fmt.Fprintf(stdout.Writer(), "[ocr] CommentWorkerPool error: %v\n", err)
}
p.resultsMu.Lock()
p.results = append(p.results, comments...)
p.resultsMu.Unlock()
}()
}
// Await blocks until all submitted work has completed and returns aggregated results.
func (p *CommentWorkerPool) Await() []model.LlmComment {
p.wg.Wait()
return p.results
args Args
diffs []model.Diff // parsed diffs
totalInsertions int64
totalDeletions int64
currentDate string
session *session.SessionHistory
subtaskFailed int64 // count of failed subtasks, accessed atomically
runner *llmloop.Runner
}
// New creates a new Agent from the given arguments.
@ -196,10 +148,25 @@ func New(args Args) *Agent {
DiffCommit: args.Commit,
})
}
return &Agent{
a := &Agent{
args: args,
session: args.Session,
}
// DiffLookup closure captures a so the runner can resolve per-file
// model.Diff records lazily (a.diffs is only populated by loadDiffs,
// after New returns).
a.runner = llmloop.NewRunner(llmloop.Deps{
LLMClient: args.LLMClient,
Model: args.Model,
Template: args.Template,
Tools: args.Tools,
MainToolDefs: args.MainToolDefs,
CommentCollector: args.CommentCollector,
CommentWorkerPool: args.CommentWorkerPool,
Session: args.Session,
DiffLookup: a.findDiff,
})
return a
}
// Run executes the full review pipeline: parse diffs -> plan per file -> LLM tool-loop -> collect comments.
@ -268,48 +235,31 @@ func (a *Agent) Diffs() []model.Diff {
// TotalTokensUsed returns PromptTokens + CompletionTokens across all LLM calls.
// For Anthropic, PromptTokens already includes cache read/write tokens.
func (a *Agent) TotalTokensUsed() int64 {
return atomic.LoadInt64(&a.totalInputTokens) + atomic.LoadInt64(&a.totalOutputTokens)
}
func (a *Agent) TotalTokensUsed() int64 { return a.runner.TotalTokensUsed() }
// TotalInputTokens returns the accumulated input/prompt tokens from all LLM calls.
func (a *Agent) TotalInputTokens() int64 {
return atomic.LoadInt64(&a.totalInputTokens)
}
func (a *Agent) TotalInputTokens() int64 { return a.runner.TotalInputTokens() }
// TotalOutputTokens returns the accumulated completion tokens from all LLM calls.
func (a *Agent) TotalOutputTokens() int64 {
return atomic.LoadInt64(&a.totalOutputTokens)
}
func (a *Agent) TotalOutputTokens() int64 { return a.runner.TotalOutputTokens() }
// TotalCacheReadTokens returns the accumulated cache read tokens from all LLM calls.
func (a *Agent) TotalCacheReadTokens() int64 {
return atomic.LoadInt64(&a.totalCacheReadTokens)
}
func (a *Agent) TotalCacheReadTokens() int64 { return a.runner.TotalCacheReadTokens() }
// TotalCacheWriteTokens returns the accumulated cache write tokens from all LLM calls.
func (a *Agent) TotalCacheWriteTokens() int64 {
return atomic.LoadInt64(&a.totalCacheWriteTokens)
}
func (a *Agent) TotalCacheWriteTokens() int64 { return a.runner.TotalCacheWriteTokens() }
// ProjectSummary returns the markdown project-level summary. Always empty
// for the diff-review path; defined so *Agent satisfies the
// cmd/opencodereview.ResultProvider interface that scan.Agent also implements.
func (a *Agent) ProjectSummary() string { return "" }
// Warnings returns a copy of non-fatal warnings recorded during review.
func (a *Agent) Warnings() []AgentWarning {
a.warningsMu.Lock()
defer a.warningsMu.Unlock()
out := make([]AgentWarning, len(a.warnings))
copy(out, a.warnings)
return out
}
func (a *Agent) Warnings() []AgentWarning { return a.runner.Warnings() }
// recordWarning adds a non-fatal warning to the agent's warning list.
func (a *Agent) recordWarning(warningType, file, message string) {
a.warningsMu.Lock()
a.warnings = append(a.warnings, AgentWarning{
File: file,
Message: message,
Type: warningType,
})
a.warningsMu.Unlock()
a.runner.RecordWarning(warningType, file, message)
}
// loadDiffs populates the diff-related fields.
@ -360,16 +310,6 @@ func (a *Agent) injectDiffMap() {
}
}
// lookupTool returns the provider for a given tool from the registry,
// or nil if the tool is not registered.
func lookupTool(reg *tool.Registry, t tool.Tool) tool.Provider {
p, ok := reg.Get(t.Name())
if !ok {
return nil
}
return p
}
// dispatchSubtasks runs the Plan + Main phases for each changed file concurrently.
func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error) {
startTime := time.Now()
@ -515,7 +455,7 @@ func (a *Agent) executeSubtask(ctx context.Context, d model.Diff) error {
messages = append(messages, llm.NewTextMessage(m.Role, content))
}
tokenCount := countMessagesTokens(messages)
tokenCount := llmloop.CountMessagesTokens(messages)
maxAllowed := a.args.Template.MaxTokens
tokenLimit := maxAllowed * 4 / 5 // 80% of MaxTokens
if tokenCount > tokenLimit {
@ -529,8 +469,11 @@ func (a *Agent) executeSubtask(ctx context.Context, d model.Diff) error {
return nil
}
err := a.performLlmCodeReview(ctx, messages, newPath)
err := a.runner.RunPerFile(ctx, messages, newPath)
if err == nil {
// REVIEW_FILTER_TASK runs after the main loop and decides which of the
// just-collected comments to drop. It needs to see comments produced by
// the async CommentWorkerPool, so wait for that to drain first.
if a.args.CommentWorkerPool != nil {
a.args.CommentWorkerPool.Await()
}
@ -584,12 +527,7 @@ func (a *Agent) executeReviewFilter(ctx context.Context, d model.Diff, newPath s
return
}
rec.SetResponse(resp, time.Since(startTime))
if resp.Usage != nil {
atomic.AddInt64(&a.totalInputTokens, resp.Usage.PromptTokens)
atomic.AddInt64(&a.totalOutputTokens, resp.Usage.CompletionTokens)
atomic.AddInt64(&a.totalCacheReadTokens, resp.Usage.CacheReadTokens)
atomic.AddInt64(&a.totalCacheWriteTokens, resp.Usage.CacheWriteTokens)
}
a.runner.RecordUsage(resp.Usage)
indices := parseFilterResponse(resp.Content(), len(comments))
if len(indices) == 0 {
@ -622,7 +560,7 @@ func buildFilterCommentsJSON(comments []model.LlmComment) string {
// parseFilterResponse extracts comment indices from the LLM filter response.
// Returns a set of 0-based indices. Invalid IDs or out-of-range indices are ignored.
func parseFilterResponse(raw string, total int) map[int]struct{} {
raw = stripMarkdownFences(raw)
raw = llmloop.StripMarkdownFences(raw)
var ids []string
if err := json.Unmarshal([]byte(raw), &ids); err != nil {
preview := raw
@ -794,12 +732,7 @@ func (a *Agent) executePlanPhase(ctx context.Context, newPath, rawDiff, changeFi
return "", fmt.Errorf("plan request: %w", err)
}
rec.SetResponse(resp, time.Since(startTime))
if resp.Usage != nil {
atomic.AddInt64(&a.totalInputTokens, resp.Usage.PromptTokens)
atomic.AddInt64(&a.totalOutputTokens, resp.Usage.CompletionTokens)
atomic.AddInt64(&a.totalCacheReadTokens, resp.Usage.CacheReadTokens)
atomic.AddInt64(&a.totalCacheWriteTokens, resp.Usage.CacheWriteTokens)
}
a.runner.RecordUsage(resp.Usage)
fmt.Fprintf(stdout.Writer(), "[ocr] Plan completed for %s\n", newPath)
return resp.Content(), nil
}
@ -842,236 +775,6 @@ func formatToolDefs(toolDefs []llm.ToolDef) string {
return sb.String()
}
// performLlmCodeReview drives the main LLM conversation loop for a single file.
// It sends messages with tool definitions, handles tool calls returned by the model,
// and collects review comments until task_done is called or limits are reached.
func (a *Agent) performLlmCodeReview(ctx context.Context, messages []llm.Message, newPath string) error {
toolReqCount := a.args.Template.MaxToolRequestTimes
const maxConsecutiveEmptyRounds = 3
consecutiveEmptyRounds := 0
for toolReqCount > 0 {
select {
case <-ctx.Done():
return ctx.Err()
default:
}
toolReqCount--
fs := a.session.GetOrCreateFileSession(newPath)
rec := fs.AppendTaskRecord(session.MainTask, append([]llm.Message(nil), messages...))
startTime := time.Now()
resp, err := a.args.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{
Model: a.args.Model,
Messages: messages,
Tools: a.args.MainToolDefs,
MaxTokens: a.args.Template.MaxTokens,
})
duration := time.Since(startTime)
if err != nil {
rec.SetError(err, duration)
telemetry.RecordLLMRequest(ctx, a.args.Model, duration, 0, "error")
return fmt.Errorf("LLM completion error: %w", err)
}
rec.SetResponse(resp, duration)
// Record LLM metrics with token info from API response usage field.
totalTokens := int64(0)
if resp.Usage != nil {
totalTokens = resp.Usage.TotalTokens
atomic.AddInt64(&a.totalInputTokens, resp.Usage.PromptTokens)
atomic.AddInt64(&a.totalOutputTokens, resp.Usage.CompletionTokens)
atomic.AddInt64(&a.totalCacheReadTokens, resp.Usage.CacheReadTokens)
atomic.AddInt64(&a.totalCacheWriteTokens, resp.Usage.CacheWriteTokens)
}
telemetry.RecordLLMRequest(ctx, a.args.Model, duration, totalTokens, "ok")
content := resp.Content()
calls := resp.ToolCalls()
if len(calls) == 0 {
// No tool calls - remind the model
fmt.Fprintf(stdout.Writer(), "[ocr] No tool calls parsed for %s, retrying...\n", newPath)
messages = append(messages, llm.NewTextMessage("user", "You did not successfully call any tools. Please try again or use task_done if finished."))
if content != "" {
messages = append(messages[:len(messages)-1], llm.NewTextMessage("assistant", content), messages[len(messages)-1])
}
continue
}
var results []tool.ToolCallResult
taskCompleted := false
hasValidResult := false
for _, call := range calls {
cp := a.executeToolCall(ctx, newPath, call, rec)
if cp.Completed {
results = append(results, tool.ToolCallResult{
ToolCallID: call.ID,
Name: call.Function.Name,
Result: "Task completed successfully.",
})
taskCompleted = true
} else if cp.Data != "" {
results = append(results, tool.ToolCallResult{
ToolCallID: call.ID,
Name: call.Function.Name,
Result: cp.Data,
})
hasValidResult = true
} else {
results = append(results, tool.ToolCallResult{
ToolCallID: call.ID,
Name: call.Function.Name,
Result: "Error: Tool execution returned no result.",
})
}
}
if taskCompleted {
break
}
if !hasValidResult {
consecutiveEmptyRounds++
if consecutiveEmptyRounds >= maxConsecutiveEmptyRounds {
fmt.Fprintf(stdout.Writer(), "[ocr] Too many empty retries for %s, stopping.\n", newPath)
break
}
fmt.Fprintf(stdout.Writer(), "[ocr] No valid tool results for %s, retrying...\n", newPath)
} else {
consecutiveEmptyRounds = 0
}
succeed := a.addNextMessage(ctx, content, calls, results, &messages, newPath)
if !succeed {
fmt.Fprintf(stdout.Writer(), "[ocr] Context compression exceeded threshold for %s, stopping.\n", newPath)
break
}
}
if toolReqCount <= 0 {
fmt.Fprintf(stdout.Writer(), "[ocr] Max tool requests reached for %s.\n", newPath)
}
return nil
}
// executeToolCall executes a single tool call from the LLM response and records
// the result in session history. It handles async dispatch for code_comment when
// a worker pool is configured, otherwise runs synchronously.
func (a *Agent) executeToolCall(ctx context.Context, newPath string, call llm.ToolCall, rec *session.TaskRecord) tool.TaskCheckpoint {
t := tool.OfName(call.Function.Name)
if !t.IsKnown() {
return tool.Of(tool.NotAvailableMsg)
}
if t == tool.TaskDone {
return tool.Complete()
}
p := lookupTool(a.args.Tools, t)
if p == nil {
return tool.Of(tool.NotAvailableMsg)
}
var args map[string]any
if err := json.Unmarshal([]byte(call.Function.Arguments), &args); err != nil {
return tool.Of(fmt.Sprintf("Error parsing tool arguments for %s: %v", t.Name(), err))
}
// Inject current file path as default for code_comment when not provided.
// The model already knows which file it's reviewing, so it omits path.
if t == tool.CodeComment && newPath != "" {
if _, ok := args["path"]; !ok {
args["path"] = newPath
}
}
startTime := time.Now()
// code_comment: parse → resolve line numbers → re-locate if needed → add to collector
if t == tool.CodeComment {
telemetry.PrintToolCallStarted(t.Name(), args)
comments, errMsg := tool.ParseComments(args)
if errMsg != "" {
telemetry.RecordToolCall(ctx, t.Name(), time.Since(startTime), false)
return tool.Of(errMsg)
}
resolveAndCollect := func(rctx context.Context) {
for i := range comments {
cm := &comments[i]
d := a.findDiff(cm.Path)
if d != nil {
if !diff.ResolveComment(cm, d) && a.args.Template.ReLocationTask != nil {
rlStart := time.Now()
_, resp, msgs := diff.ReLocateComment(rctx, cm, d, a.args.LLMClient, a.args.Template.ReLocationTask, a.args.Model, a.args.Template.MaxTokens)
if msgs != nil {
fs := a.session.GetOrCreateFileSession(cm.Path)
rlRec := fs.AppendTaskRecord(session.ReLocationTask, msgs)
if resp != nil {
rlRec.SetResponse(resp, time.Since(rlStart))
if resp.Usage != nil {
atomic.AddInt64(&a.totalInputTokens, resp.Usage.PromptTokens)
atomic.AddInt64(&a.totalOutputTokens, resp.Usage.CompletionTokens)
atomic.AddInt64(&a.totalCacheReadTokens, resp.Usage.CacheReadTokens)
atomic.AddInt64(&a.totalCacheWriteTokens, resp.Usage.CacheWriteTokens)
}
} else {
rlRec.SetError(fmt.Errorf("re-location LLM call failed"), time.Since(rlStart))
}
}
}
}
a.args.CommentCollector.Add(*cm)
}
}
if a.args.CommentWorkerPool != nil {
if rec != nil {
rec.AddToolResult(t.Name(), call.Function.Arguments, "(async)")
}
pool := a.args.CommentWorkerPool
asyncCtx := context.WithoutCancel(ctx)
toolName := t.Name()
pool.Submit(func() ([]model.LlmComment, error) {
resolveAndCollect(asyncCtx)
telemetry.PrintToolCallFinished(toolName, time.Since(startTime))
return []model.LlmComment{}, nil
})
telemetry.RecordToolCall(asyncCtx, toolName, time.Since(startTime), true)
return tool.Of(tool.CommentSucceed)
}
resolveAndCollect(ctx)
dur := time.Since(startTime)
telemetry.RecordToolCall(ctx, t.Name(), dur, true)
telemetry.PrintToolCallFinished(t.Name(), dur)
if rec != nil {
rec.AddToolResult(t.Name(), call.Function.Arguments, tool.CommentSucceed)
}
return tool.Of(tool.CommentSucceed)
}
// Synchronous path for all other tools
telemetry.PrintToolCallStarted(t.Name(), args)
result, err := p.Execute(ctx, args)
dur := time.Since(startTime)
ok := err == nil
telemetry.RecordToolCall(ctx, t.Name(), dur, ok)
if err != nil {
telemetry.PrintToolCallError(t.Name(), err)
return tool.Of(fmt.Sprintf("Error executing tool %s: %v", t.Name(), err))
}
telemetry.PrintToolCallFinished(t.Name(), dur)
if rec != nil {
rec.AddToolResult(t.Name(), call.Function.Arguments, result)
}
return tool.Of(result)
}
// findDiff returns the Diff for the given file path, or nil if not found.
func (a *Agent) findDiff(path string) *model.Diff {
@ -1083,13 +786,6 @@ func (a *Agent) findDiff(path string) *model.Diff {
return nil
}
// collectPendingComments waits for any async workers then returns aggregated comments from the collector.
func (a *Agent) collectPendingComments() []model.LlmComment {
if a.args.CommentWorkerPool != nil {
a.args.CommentWorkerPool.Await()
}
return a.args.CommentCollector.Comments()
}
// BuildToolDefs converts toolsconfig.ToolConfigEntry slice into []llm.ToolDef,
// filtering by phase (planOnly=true for plan_task, false for main_task).

42
internal/agent/preview.go
View file

@ -8,38 +8,24 @@ import (
"github.com/open-code-review/open-code-review/internal/model"
)
// ExcludeReason describes why a file was excluded from review.
type ExcludeReason string
// ExcludeReason / DiffPreview / DiffPreviewEntry are now type aliases of
// the mode-agnostic preview types in internal/model. Kept for backwards
// compatibility with existing call sites; internal/scan returns the same
// model.Preview shape directly.
type ExcludeReason = model.ExcludeReason
type DiffPreview = model.Preview
type DiffPreviewEntry = model.PreviewEntry
// Re-export the constants so callers can keep writing agent.ExcludeBinary.
const (
ExcludeNone ExcludeReason = ""
ExcludeUserRule ExcludeReason = "user_exclude"
ExcludeExtension ExcludeReason = "unsupported_ext"
ExcludeDefaultPath ExcludeReason = "default_path"
ExcludeDeleted ExcludeReason = "deleted"
ExcludeBinary ExcludeReason = "binary"
ExcludeNone = model.ExcludeNone
ExcludeUserRule = model.ExcludeUserRule
ExcludeExtension = model.ExcludeExtension
ExcludeDefaultPath = model.ExcludeDefaultPath
ExcludeDeleted = model.ExcludeDeleted
ExcludeBinary = model.ExcludeBinary
)
// DiffPreviewEntry is one file's preview record.
type DiffPreviewEntry struct {
Path string `json:"path"`
Status string `json:"status"`
Insertions int64 `json:"insertions"`
Deletions int64 `json:"deletions"`
WillReview bool `json:"will_review"`
ExcludeReason ExcludeReason `json:"exclude_reason,omitempty"`
}
// DiffPreview is the full preview result.
type DiffPreview struct {
Entries []DiffPreviewEntry `json:"files"`
TotalInsertions int64 `json:"total_insertions"`
TotalDeletions int64 `json:"total_deletions"`
TotalFiles int `json:"total_files"`
ReviewableCount int `json:"reviewable_count"`
ExcludedCount int `json:"excluded_count"`
}
// whyExcluded applies the filter algorithm as shouldReview but
// returns the specific reason a file is excluded.
func (a *Agent) whyExcluded(d model.Diff) ExcludeReason {

88
internal/config/template/scan_template.json Normal file
View file

@ -0,0 +1,88 @@
{
"MAIN_TASK": {
"messages": [
{
"role": "system",
"content": "## Role\nYou are a code review assistant developed by Alibaba. Unlike diff-based review, in this task you are reviewing an ENTIRE existing source file (no diff context). Provide professional review feedback identifying real defects, bugs, security risks, performance issues, concurrency hazards, and maintainability problems in the file as it stands today.\n\n## Capabilities\n- Think step by step.\n- Read the full file content carefully before commenting.\n- Be objective and grounded in the source. When context is insufficient, call context tools instead of guessing.\n- Focus on substantive correctness/security/performance/maintainability issues. Avoid stylistic nitpicks unless the project's review rules call them out.\n- Do not comment on code in other files — context tools are reference only; findings outside the current file MUST NOT become the subject of your comments.\n- Avoid commenting on auto-generated code, code comments, or metadata annotations unless the user's review rules require it.\n\n## Tool-call discipline (IMPORTANT — your tool-call budget per file is limited)\n- The file content is ALREADY in <current_file_content>; do NOT call `file_read` to re-fetch the current file.\n- Limit context-gathering to AT MOST 2-3 tool calls per finding. Avoid calling the same tool with the same arguments more than once.\n- Once you have enough evidence for an issue, call `code_comment` immediately rather than gathering more context.\n- Batch multiple comments in a single `code_comment` call (its `comments` parameter accepts an array) instead of issuing one call per finding.\n- If no further issues are evident after a quick sweep of the file, call `task_done` immediately — do not keep probing for marginal findings.\n\n## Reply limit\n- If the review is complete, call `task_done` to end the task.\n- For each confirmed issue, call `code_comment` to record feedback (prefer batching).\n- If additional context is genuinely needed, call the appropriate context tool, but stay within the budget above."
},
{
"role": "user",
"content": "<current_file_path>{{current_file_path}}</current_file_path>\n\n<current_file_content>\n{{file_content}}\n</current_file_content>\n\nCurrent time in the real world: {{current_system_date_time}}\n\n<user_task>\n### Requirement Background (Optional)\n{{requirement_background}}\n\n### Review Checklist\n{{system_rule}}\n\n### Pre-scan Focus Areas\n{{plan_guidance}}\n\nReview the entire source file in <current_file_content> and report all findings via code_comment, then call task_done.\n</user_task>"
}
],
"timeout": 180
},
"PLAN_TASK": {
"messages": [
{
"role": "system",
"content": "## Role\nYou are a scan-mode review planner. Given a whole source file you produce a focused review checklist that a downstream reviewer will use as guidance.\n\n## Output\nStrictly output a single JSON object with this shape (no prose around it):\n\n```\n{\n \"summary\": \"one or two sentences summarizing what this file does and its key design\",\n \"checkpoints\": [\n {\"focus\": \"<short label of the concern>\", \"lines\": \"<line range like 45-78, optional>\", \"why\": \"<one sentence why it warrants attention>\"}\n ]\n}\n```\n\n## Discipline\n- At most 5 checkpoints. Prefer fewer high-signal ones over many shallow ones.\n- Severity bias: prioritise correctness / concurrency / security / data-integrity / error-handling over style.\n- Each checkpoint must be grounded in the file content; if unsure, omit it.\n- Do NOT call any tools. Output JSON only."
},
{
"role": "user",
"content": "<current_file_path>{{current_file_path}}</current_file_path>\n\n<current_file_content>\n{{file_content}}\n</current_file_content>\n\nCurrent time: {{current_system_date_time}}\n\n### Review Checklist (project rules)\n{{system_rule}}\n\nProduce the JSON focus checklist now."
}
],
"timeout": 90
},
"DEDUP_TASK": {
"messages": [
{
"role": "system",
"content": "## Role\nYou consolidate near-duplicate review comments produced within a single scan batch into a smaller, deduplicated set.\n\n## Output\nStrictly output one JSON object, no prose:\n\n```\n{\n \"groups\": [\n {\"members\": [\"c-0\"]},\n {\"members\": [\"c-1\",\"c-3\"], \"merged_content\": \"<optional rewritten content for the group\\u0027s canonical comment>\"}\n ]\n}\n```\n\n## Rules\n- Every input id MUST appear in exactly one group's members list.\n- A single-member group means \"keep as-is\".\n- A multi-member group means: keep members[0] as canonical, drop the rest. If merged_content is present, replace the canonical comment's content with it; otherwise keep the canonical's original content unchanged.\n- Cluster aggressively only when comments make the SAME claim (e.g. \"missing error check\" repeated across files). Do NOT merge two findings just because they touch the same area.\n- Prefer keeping per-file detail over merging across files when severity differs.\n- Output JSON only. No commentary."
},
{
"role": "user",
"content": "<batch_comments>\n{{batch_comments}}\n</batch_comments>\n\nProduce the dedup groups JSON now."
}
],
"timeout": 90
},
"PROJECT_SUMMARY_TASK": {
"messages": [
{
"role": "system",
"content": "## Role\nYou are a senior reviewer producing a project-level summary of a full-repo scan.\n\nYou are given the union of per-file review comments. Identify cross-cutting patterns and the most important findings; do NOT restate every individual comment.\n\n## Output\nA single markdown document with these sections (omit a section if it has nothing to say):\n\n### Top Issues\nThe 5–10 most consequential findings, ranked by impact. Group when the same root cause repeats across files.\n\n### Module Hotspots\nDirectories / modules where comment density or severity is abnormally high. Cite paths.\n\n### Cross-Cutting Concerns\nPatterns that appear across many files (e.g. inconsistent error handling, missing tests, unsafe concurrency primitives). Cite a few representative paths each.\n\n### Quick Wins\nFixes that are low-effort and high-leverage based on the comments.\n\nDo not include a generic \"overall the codebase is X\" paragraph. Be concrete; reference paths."
},
{
"role": "user",
"content": "Total comments: {{comment_count}} across {{file_count}} files.\n\n<all_comments>\n{{all_comments}}\n</all_comments>\n\nProduce the project summary markdown now."
}
],
"timeout": 120
},
"MEMORY_COMPRESSION_TASK": {
"messages": [
{
"role": "system",
"content": "## Goal\nYou are a professional code review conversation summarization assistant. You will receive a conversation history between a code review assistant and an LLM model (including tool calls and their results). Compress this conversation into a structured summary so that the code review assistant can continue from the current state without restarting.\n\n## Output Format Requirements\nOrganize the summary using the following five dimensions, separated by explicit headings:\n\n### Identified Code Issues\nList all confirmed issues sorted by severity (HIGH / MEDIUM / LOW). Each entry should include: file path, issue type, severity, brief description.\n\n### Tool Call Conclusions\nSummarize key findings and conclusions from each tool invocation.\n\n### Completed Tasks\nList items that have been completed and require no further follow-up.\n\n### Pending Tasks\nList items that have been started but not yet completed and still need attention.\n\n### Current Focus\nSummarize in one sentence the core matter currently being investigated or handled.\n\n## Rules\n1. Do not include specific code details; only reference file paths and issue types\n2. Avoid repetitive or redundant information\n3. Omit any dimension that has no relevant content\n4. Completed/pending task list items should be described as complete sentences\n5. current_focus should be concise, no more than one sentence"
},
{
"role": "user",
"content": "{{context}}"
}
],
"timeout": 120
},
"RE_LOCATION_TASK": {
"messages": [
{
"role": "system",
"content": "You are a code location assistant. Given a unified diff and a review comment, your sole task is to extract the exact code snippet from the diff that the comment refers to. /no_think"
},
{
"role": "user",
"content": "Below is a unified diff and a review comment. Identify the minimal contiguous code range in the diff that the comment targets.\n\nRules:\n1. Copy the relevant lines VERBATIM from the diff — do not rewrite, reformat, or add anything.\n2. Strip leading diff markers (`+`, `-`, ` `) from each line before outputting.\n3. Include only the lines directly related to the issue — no surrounding context.\n4. If multiple disjoint locations apply, pick the single most relevant one.\n5. Output ONLY a fenced code block. No explanation, no commentary.\n\n**Diff:**\n```diff\n{diff}```\n\n**Original code snippet (failed to match):**\n```\n{existing_code}\n```\n\n**Review comment:**\n{suggestion_content}"
}
],
"timeout": 60
},
"MAX_TOKENS": 58888,
"MAX_TOOL_REQUEST_TIMES": 60,
"MAX_FILE_SIZE_BYTES": 2097152,
"BATCH_STRATEGY": "by-language",
"BATCH_SIZE": 50,
"DEDUP_MIN_COMMENTS": 4,
"TOOL_REQUEST_WAIT_TIME_MS": 10000,
"MAX_SUBTASK_EXECUTION_TIME_MINUTES": 5
}

67
internal/config/template/template.go
View file

@ -9,6 +9,7 @@ import (
)
// Template holds the native agent task template configuration.
// Scan-mode fields live in ScanTemplate, not here.
type Template struct {
MainTask LlmConversation `json:"MAIN_TASK"`
PlanTask *LlmConversation `json:"PLAN_TASK,omitempty"`
@ -20,9 +21,33 @@ type Template struct {
ReviewFilterTask *LlmConversation `json:"REVIEW_FILTER_TASK,omitempty"`
}
// ScanTemplate holds the full-file scan task template configuration loaded
// from scan_template.json. Kept entirely separate from Template so the two
// pipelines can evolve their prompts and budgets independently.
type ScanTemplate struct {
MainTask LlmConversation `json:"MAIN_TASK"`
PlanTask *LlmConversation `json:"PLAN_TASK,omitempty"`
MemoryCompressionTask LlmConversation `json:"MEMORY_COMPRESSION_TASK"`
ReLocationTask *LlmConversation `json:"RE_LOCATION_TASK,omitempty"`
MaxTokens int `json:"MAX_TOKENS"`
ToolRequestWaitTimeMs int `json:"TOOL_REQUEST_WAIT_TIME_MS"`
MaxToolRequestTimes int `json:"MAX_TOOL_REQUEST_TIMES"`
MaxSubtaskExecMinutes int `json:"MAX_SUBTASK_EXECUTION_TIME_MINUTES"`
MaxFileSizeBytes int64 `json:"MAX_FILE_SIZE_BYTES,omitempty"`
MaxTokensBudget int64 `json:"MAX_TOKENS_BUDGET,omitempty"`
BatchStrategy string `json:"BATCH_STRATEGY,omitempty"`
BatchSize int `json:"BATCH_SIZE,omitempty"`
DedupTask *LlmConversation `json:"DEDUP_TASK,omitempty"`
DedupMinComments int `json:"DEDUP_MIN_COMMENTS,omitempty"`
ProjectSummaryTask *LlmConversation `json:"PROJECT_SUMMARY_TASK,omitempty"`
}
//go:embed task_template.json prompts/*
var templateFS embed.FS
//go:embed scan_template.json
var defaultScanTemplate []byte
type manifestMessage struct {
Role string `json:"role"`
PromptFile string `json:"prompt_file"`
@ -105,6 +130,15 @@ func LoadDefault() (*Template, error) {
return &tpl, nil
}
// LoadScanDefault parses the embedded scan_template.json.
func LoadScanDefault() (*ScanTemplate, error) {
var tpl ScanTemplate
if err := json.Unmarshal(defaultScanTemplate, &tpl); err != nil {
return nil, fmt.Errorf("unmarshal default scan template: %w", err)
}
return &tpl, nil
}
// applyLanguage appends instruction to all system-role messages in conv.
func applyLanguage(conv *LlmConversation, instruction string) {
for i := range conv.Messages {
@ -132,6 +166,25 @@ func (t *Template) ApplyLanguage(lang string) {
}
applyLanguage(&t.MemoryCompressionTask, instruction)
}
// ApplyLanguage injects a language directive into all system-role messages
// of the scan template (MAIN_TASK, PLAN_TASK if set, DEDUP_TASK if set,
// and MEMORY_COMPRESSION_TASK).
func (t *ScanTemplate) ApplyLanguage(lang string) {
instruction := "\n\nAlways respond in " + resolveLang(lang) + "."
applyLanguage(&t.MainTask, instruction)
if t.PlanTask != nil {
applyLanguage(t.PlanTask, instruction)
}
if t.DedupTask != nil {
applyLanguage(t.DedupTask, instruction)
}
if t.ProjectSummaryTask != nil {
applyLanguage(t.ProjectSummaryTask, instruction)
}
applyLanguage(&t.MemoryCompressionTask, instruction)
}
func (t *Template) Validate() error {
if t.MaxTokens <= 0 {
return fmt.Errorf("max_tokens must be positive")
@ -145,6 +198,20 @@ func (t *Template) Validate() error {
return nil
}
// Validate checks that a ScanTemplate has the minimum fields populated.
func (t *ScanTemplate) Validate() error {
if t.MaxTokens <= 0 {
return fmt.Errorf("scan: max_tokens must be positive")
}
if t.MaxToolRequestTimes <= 0 {
return fmt.Errorf("scan: max_tool_request_times must be positive")
}
if len(t.MainTask.Messages) == 0 {
return fmt.Errorf("scan: main_task.messages must not be empty")
}
return nil
}
// LlmConversation mirrors LlmConversation from the Java side — a preset prompt with settings.
type LlmConversation struct {
Timeout int `json:"timeout"`

46
internal/config/template/template_test.go
View file

@ -5,6 +5,52 @@ import (
"testing"
)
func TestLoadScanDefault_BudgetParsed(t *testing.T) {
tpl, err := LoadScanDefault()
if err != nil {
t.Fatalf("LoadScanDefault: %v", err)
}
if tpl.MaxToolRequestTimes < 60 {
t.Errorf("scan MaxToolRequestTimes(%d) should be >= 60", tpl.MaxToolRequestTimes)
}
if len(tpl.MainTask.Messages) == 0 {
t.Fatal("scan MainTask must be populated from the embedded scan_template.json")
}
if tpl.MaxFileSizeBytes <= 0 {
t.Errorf("scan MaxFileSizeBytes(%d) should be > 0 (defaults to 2 MiB in JSON)", tpl.MaxFileSizeBytes)
}
}
func TestApplyLanguage_ScanTemplate(t *testing.T) {
tpl, err := LoadScanDefault()
if err != nil {
t.Fatalf("LoadScanDefault: %v", err)
}
tpl.ApplyLanguage("Spanish")
for _, m := range tpl.MainTask.Messages {
if m.Role != "system" {
continue
}
if !strings.Contains(m.Content, "Always respond in Spanish.") {
t.Errorf("language directive missing from scan MainTask system message")
}
}
}
func TestLoadDefault_HasNoScanFields(t *testing.T) {
tpl, err := LoadDefault()
if err != nil {
t.Fatalf("LoadDefault: %v", err)
}
if len(tpl.MainTask.Messages) == 0 {
t.Fatal("review MainTask must be populated")
}
if tpl.MaxToolRequestTimes <= 0 {
t.Errorf("review MaxToolRequestTimes invalid: %d", tpl.MaxToolRequestTimes)
}
}
func TestLoadDefault_FieldsPopulated(t *testing.T) {
tpl, err := LoadDefault()
if err != nil {

14
internal/diff/git.go
View file

@ -7,6 +7,7 @@ import (
"os"
"os/exec"
"path/filepath"
"slices"
"strings"
"github.com/open-code-review/open-code-review/internal/gitcmd"
@ -167,7 +168,7 @@ func (p *Provider) loadGitignorePatterns() []string {
return nil
}
var patterns []string
for _, line := range strings.Split(string(data), "\n") {
for line := range strings.SplitSeq(string(data), "\n") {
line = strings.TrimSpace(line)
if line == "" || strings.HasPrefix(line, "#") {
continue
@ -200,16 +201,11 @@ func (p *Provider) isPathExcluded(relPath string, gitignorePatterns []string) bo
// matchGitignorePattern checks if relPath matches a single .gitignore pattern.
func matchGitignorePattern(relPath, pat string) bool {
// Directory-only patterns (trailing /)
if strings.HasSuffix(pat, "/") {
dirName := strings.TrimSuffix(pat, "/")
if before, ok := strings.CutSuffix(pat, "/"); ok {
dirName := before
// Match if any path segment equals the dir name
segments := strings.Split(relPath, "/")
for _, seg := range segments {
if seg == dirName {
return true
}
}
return false
return slices.Contains(segments, dirName)
}
// Negation patterns are not needed for exclusion purposes

33
internal/diff/gitignore.go Normal file
View file

@ -0,0 +1,33 @@
package diff
// ExcludedDirs is the list of directory prefixes that scanners and diff
// providers should always skip. Exposed so internal/scan and other consumers
// can reuse the same blocklist.
func ExcludedDirs() []string {
out := make([]string, len(providerDirIgnoreDirs))
copy(out, providerDirIgnoreDirs)
return out
}
// LoadGitignorePatterns reads and parses .gitignore patterns from the given
// repository root. Returns nil if the file is missing or unreadable.
func LoadGitignorePatterns(repoDir string) []string {
stub := &Provider{repoDir: repoDir}
return stub.loadGitignorePatterns()
}
// IsPathExcluded returns true when relPath matches any of the supplied
// gitignore-style patterns or any default excluded directory prefix
// (see ExcludedDirs).
func IsPathExcluded(repoDir, relPath string, patterns []string) bool {
stub := &Provider{repoDir: repoDir}
return stub.isPathExcluded(relPath, patterns)
}
// MatchGitignorePattern reports whether relPath matches a single
// gitignore-style pattern, using the simplified semantics that diff.Provider
// already implements (basename match, prefix match, directory-only suffix).
// Useful when callers want to test a single pattern in isolation.
func MatchGitignorePattern(relPath, pat string) bool {
return matchGitignorePattern(relPath, pat)
}

217
internal/agent/compression.go → internal/llmloop/compression.go
View file

@ -1,4 +1,4 @@
package agent
package llmloop
import (
"context"
@ -10,10 +10,9 @@ import (
"github.com/open-code-review/open-code-review/internal/llm"
"github.com/open-code-review/open-code-review/internal/session"
"github.com/open-code-review/open-code-review/internal/stdout"
"github.com/open-code-review/open-code-review/internal/tool"
)
// compression thresholds as fractions of MaxTokens.
// Compression thresholds, as fractions of MaxTokens.
const (
tokenSoftThreshold = 0.60 // async background compression
tokenWarningThreshold = 0.80 // immediate sync compression
@ -42,7 +41,19 @@ type compressionJob struct {
snapshotLen int // message count when the snapshot was taken
}
// groupIntoRounds parses messages[start:] into logical (assistant + tool_results) pairs.
// CountMessagesTokens returns the rough token count of msgs by summing the
// per-message text token count. Exported because both review and scan top
// layers may want it for pre-flight checks.
func CountMessagesTokens(msgs []llm.Message) int {
var total int
for _, m := range msgs {
total += llm.CountTokens(m.ExtractText())
}
return total
}
// groupIntoRounds parses messages[start:] into logical
// (assistant + tool_results) pairs.
func groupIntoRounds(messages []llm.Message, start int) []round {
var rounds []round
i := start
@ -62,8 +73,9 @@ func groupIntoRounds(messages []llm.Message, start int) []round {
return rounds
}
// computeActiveZoneSize returns how many trailing rounds fit within the remaining
// token budget after accounting for frozen zone and the compressed summary.
// computeActiveZoneSize returns how many trailing rounds fit within the
// remaining token budget after accounting for the frozen zone and the
// compressed summary.
func computeActiveZoneSize(rounds []round, messages []llm.Message, maxTokens int, reservedTokens int) int {
budget := int(float64(maxTokens)*tokenWarningThreshold) - reservedTokens
if budget <= 0 {
@ -87,8 +99,8 @@ func computeActiveZoneSize(rounds []round, messages []llm.Message, maxTokens int
}
// partitionMessages divides messages into frozen, compress, and active zones.
// Frozen zone is always messages[0:2]. Active zone preserves the K most recent
// complete rounds based on available token budget.
// Frozen zone is always messages[0:2]. Active zone preserves the K most
// recent complete rounds based on available token budget.
func partitionMessages(messages []llm.Message, maxTokens int, prevSummaryTokenEstimate int) partitionResult {
result := partitionResult{frozenEnd: 2}
if len(messages) <= 2 {
@ -122,104 +134,107 @@ func partitionMessages(messages []llm.Message, maxTokens int, prevSummaryTokenEs
return result
}
// addNextMessage adds assistant + tool response messages to the conversation history.
// Implements dual-threshold compression:
// - 60% of MaxTokens: trigger async background compression (non-blocking)
// - 80% of MaxTokens: perform synchronous compression immediately
func (a *Agent) addNextMessage(ctx context.Context, assistantContent string, toolCalls []llm.ToolCall, results []tool.ToolCallResult, messages *[]llm.Message, filePath string) bool {
maxAllowed := a.args.Template.MaxTokens
softLimit := int(float64(maxAllowed) * tokenSoftThreshold)
warnLimit := int(float64(maxAllowed) * tokenWarningThreshold)
// StripMarkdownFences removes ```json and ``` wrappers some models add
// around structured outputs. Exposed so callers (e.g. agent's review-filter
// post-step) that parse LLM JSON output can reuse the same heuristic.
func StripMarkdownFences(s string) string { return stripMarkdownFences(s) }
// Try to apply any completed async compression from a previous iteration.
a.tryApplyPendingCompression(messages)
tokenCount := countMessagesTokens(*messages)
// Hard threshold: synchronous compression.
if tokenCount > warnLimit {
a.cancelPendingCompression()
*messages, _ = a.runCompression(ctx, *messages, filePath)
tokenCount = countMessagesTokens(*messages)
// stripMarkdownFences is the package-private workhorse used by the
// internal compression code paths.
func stripMarkdownFences(s string) string {
s = strings.TrimSpace(s)
if strings.HasPrefix(s, "```") {
if nl := strings.IndexByte(s, '\n'); nl >= 0 {
s = s[nl+1:]
} else {
s = strings.TrimPrefix(s, "```json")
s = strings.TrimPrefix(s, "```")
}
}
// Soft threshold: async compression.
if tokenCount > softLimit && a.pendingJob == nil {
a.triggerAsyncCompression(ctx, *messages, filePath)
s = strings.TrimSpace(s)
if strings.HasSuffix(s, "```") {
s = strings.TrimSuffix(s, "```")
s = strings.TrimSpace(s)
}
// Add assistant message with tool_calls when present.
if len(toolCalls) > 0 {
*messages = append(*messages, llm.NewToolCallMessage(assistantContent, toolCalls))
} else if assistantContent != "" {
*messages = append(*messages, llm.NewTextMessage("assistant", assistantContent))
}
// Add tool response messages using Claude's tool_result format.
for _, r := range results {
*messages = append(*messages, llm.NewToolResultMessage(r.ToolCallID, r.Result))
}
// Final check: compress synchronously if still over warning limit.
finalCount := countMessagesTokens(*messages)
if finalCount > warnLimit {
a.cancelPendingCompression()
*messages, _ = a.runCompression(ctx, *messages, filePath)
finalCount = countMessagesTokens(*messages)
}
return finalCount < warnLimit
return s
}
// runCompression performs three-zone memory compression on the given messages.
// It summarizes the compress zone while preserving the active zone intact.
// Returns the rebuilt messages slice: [frozen] + [compressed_summary] + [active].
func (a *Agent) runCompression(ctx context.Context, msgs []llm.Message, filePath string) ([]llm.Message, error) {
if len(a.args.Template.MemoryCompressionTask.Messages) == 0 || len(msgs) <= 2 {
// buildMessageXML serializes msgs into the <message><content> form expected
// by the MEMORY_COMPRESSION_TASK prompt template.
func buildMessageXML(msgs []llm.Message) string {
var sb strings.Builder
for i, m := range msgs {
sb.WriteString(fmt.Sprintf("<message id=\"%d\" role=\"%s\">\n", i, m.Role))
sb.WriteString(" <content>\n")
sb.WriteString(fmt.Sprintf(" %s\n", m.ExtractText()))
sb.WriteString(" </content>\n")
sb.WriteString("</message>")
if i < len(msgs)-1 {
sb.WriteString("\n")
}
}
return sb.String()
}
// copyMessages creates a shallow copy of a message slice.
func copyMessages(msgs []llm.Message) []llm.Message {
out := make([]llm.Message, len(msgs))
copy(out, msgs)
return out
}
// runCompression performs three-zone memory compression on the given
// messages, summarizing the compress zone while preserving the active zone
// intact. Returns rebuilt as [frozen] + [compressed_summary appended to
// the user prompt] + [active].
func (r *Runner) runCompression(ctx context.Context, msgs []llm.Message, filePath string) ([]llm.Message, error) {
if len(r.deps.Template.MemoryCompressionTask.Messages) == 0 || len(msgs) <= 2 {
return msgs[:min(len(msgs), 2)], nil
}
part := partitionMessages(msgs, a.args.Template.MaxTokens, 0)
part := partitionMessages(msgs, r.deps.Template.MaxTokens, 0)
if part.compressEnd <= part.frozenEnd {
return msgs, nil
}
contextXML := buildMessageXML(msgs[part.frozenEnd:part.compressEnd])
compressionMsgs := make([]llm.Message, 0, len(a.args.Template.MemoryCompressionTask.Messages))
for _, m := range a.args.Template.MemoryCompressionTask.Messages {
compressionMsgs := make([]llm.Message, 0, len(r.deps.Template.MemoryCompressionTask.Messages))
for _, m := range r.deps.Template.MemoryCompressionTask.Messages {
content := strings.ReplaceAll(m.Content, "{{context}}", contextXML)
compressionMsgs = append(compressionMsgs, llm.NewTextMessage(m.Role, content))
}
startTime := time.Now()
resp, err := a.args.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{
Model: a.args.Model,
resp, err := r.deps.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{
Model: r.deps.Model,
Messages: compressionMsgs,
MaxTokens: a.args.Template.MaxTokens,
MaxTokens: r.deps.Template.MaxTokens,
})
duration := time.Since(startTime)
fs := a.session.GetOrCreateFileSession(filePath)
fs := r.deps.Session.GetOrCreateFileSession(filePath)
rec := fs.AppendTaskRecord(session.MemoryCompressionTask, compressionMsgs)
if err != nil {
rec.SetError(err, duration)
fmt.Fprintf(stdout.Writer(), "[ocr] Memory compression failed: %v\n", err)
// Intentionally return unmodified msgs: truncating to frozenEnd would
// discard all conversation context, which is worse than staying over
// the token limit temporarily.
// Return msgs unchanged: truncating to frozenEnd would discard all
// conversation context, which is worse than staying over the token
// limit temporarily.
return msgs, fmt.Errorf("memory compression: %w", err)
}
rec.SetResponse(resp, duration)
if resp.Usage != nil {
atomic.AddInt64(&a.totalInputTokens, resp.Usage.PromptTokens)
atomic.AddInt64(&a.totalOutputTokens, resp.Usage.CompletionTokens)
atomic.AddInt64(&a.totalCacheReadTokens, resp.Usage.CacheReadTokens)
atomic.AddInt64(&a.totalCacheWriteTokens, resp.Usage.CacheWriteTokens)
atomic.AddInt64(&r.totalInputTokens, resp.Usage.PromptTokens)
atomic.AddInt64(&r.totalOutputTokens, resp.Usage.CompletionTokens)
atomic.AddInt64(&r.totalCacheReadTokens, resp.Usage.CacheReadTokens)
atomic.AddInt64(&r.totalCacheWriteTokens, resp.Usage.CacheWriteTokens)
}
rawSummary := stripMarkdownFences(resp.Content())
if rawSummary == "" {
// Empty summary: keep the original conversation rather than dropping
// everything below the frozen zone.
return msgs, nil
}
@ -228,9 +243,6 @@ func (a *Agent) runCompression(ctx context.Context, msgs []llm.Message, filePath
userMsg := rebuilt[1]
currentText := userMsg.ExtractText()
if idx := strings.Index(currentText, "\n\n<previous_review_summary>"); idx >= 0 {
currentText = currentText[:idx]
}
rebuilt[1] = llm.NewTextMessage(userMsg.Role, currentText+"\n\n<previous_review_summary>\n"+rawSummary+"\n</previous_review_summary>")
for i := part.compressEnd; i < len(msgs); i++ {
@ -241,28 +253,30 @@ func (a *Agent) runCompression(ctx context.Context, msgs []llm.Message, filePath
}
// triggerAsyncCompression kicks off a background compression job.
func (a *Agent) triggerAsyncCompression(ctx context.Context, messages []llm.Message, filePath string) {
func (r *Runner) triggerAsyncCompression(ctx context.Context, messages []llm.Message, filePath string) {
msgSnapshot := copyMessages(messages)
asyncCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Minute)
job := &compressionJob{done: make(chan struct{}), cancel: cancel, snapshotLen: len(messages)}
a.compressionMu.Lock()
a.pendingJob = job
a.compressionMu.Unlock()
r.compressionMu.Lock()
r.pendingJob = job
r.compressionMu.Unlock()
go func() {
defer cancel()
rebuilt, err := a.runCompression(asyncCtx, msgSnapshot, filePath)
rebuilt, err := r.runCompression(asyncCtx, msgSnapshot, filePath)
a.compressionMu.Lock()
defer a.compressionMu.Unlock()
r.compressionMu.Lock()
defer r.compressionMu.Unlock()
if a.pendingJob != job {
if r.pendingJob != job {
return // cancelled or superseded
}
if err != nil {
a.pendingJob = nil
// Compression failed — abandon the job rather than applying a
// truncated/unmodified snapshot over live messages.
r.pendingJob = nil
close(job.done)
return
}
@ -271,12 +285,13 @@ func (a *Agent) triggerAsyncCompression(ctx context.Context, messages []llm.Mess
}()
}
// tryApplyPendingCompression checks if a background compression completed
// and swaps the rebuilt messages into place. Returns true if applied.
func (a *Agent) tryApplyPendingCompression(messages *[]llm.Message) bool {
a.compressionMu.Lock()
job := a.pendingJob
a.compressionMu.Unlock()
// tryApplyPendingCompression checks whether a background compression has
// completed and swaps the rebuilt messages into place. Returns true if
// applied.
func (r *Runner) tryApplyPendingCompression(messages *[]llm.Message) bool {
r.compressionMu.Lock()
job := r.pendingJob
r.compressionMu.Unlock()
if job == nil {
return false
@ -285,19 +300,21 @@ func (a *Agent) tryApplyPendingCompression(messages *[]llm.Message) bool {
select {
case <-job.done:
applied := false
a.compressionMu.Lock()
if a.pendingJob == job && job.rebuilt != nil {
r.compressionMu.Lock()
if r.pendingJob == job && job.rebuilt != nil {
rebuilt := job.rebuilt
// Preserve any messages appended after the snapshot was taken —
// the background job only compressed messages[:snapshotLen].
if job.snapshotLen < len(*messages) {
rebuilt = append(rebuilt, (*messages)[job.snapshotLen:]...)
}
*messages = rebuilt
applied = true
}
if a.pendingJob == job {
a.pendingJob = nil
if r.pendingJob == job {
r.pendingJob = nil
}
a.compressionMu.Unlock()
r.compressionMu.Unlock()
return applied
default:
return false
@ -305,12 +322,12 @@ func (a *Agent) tryApplyPendingCompression(messages *[]llm.Message) bool {
}
// cancelPendingCompression aborts any in-flight background compression.
func (a *Agent) cancelPendingCompression() {
a.compressionMu.Lock()
defer a.compressionMu.Unlock()
func (r *Runner) cancelPendingCompression() {
r.compressionMu.Lock()
defer r.compressionMu.Unlock()
if a.pendingJob != nil {
a.pendingJob.cancel()
a.pendingJob = nil
if r.pendingJob != nil {
r.pendingJob.cancel()
r.pendingJob = nil
}
}

404
internal/llmloop/loop.go Normal file
View file

@ -0,0 +1,404 @@
package llmloop
import (
"context"
"encoding/json"
"fmt"
"sync"
"sync/atomic"
"time"
"github.com/open-code-review/open-code-review/internal/config/template"
"github.com/open-code-review/open-code-review/internal/diff"
"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"
"github.com/open-code-review/open-code-review/internal/stdout"
"github.com/open-code-review/open-code-review/internal/telemetry"
"github.com/open-code-review/open-code-review/internal/tool"
)
// Deps bundles all per-call dependencies the Runner needs. Both
// internal/agent (diff review) and internal/scan (full-file scan) build a
// Deps from their own state and hand it to NewRunner.
type Deps struct {
LLMClient llm.LLMClient
Model string
Template template.Template
Tools *tool.Registry
MainToolDefs []llm.ToolDef
CommentCollector *tool.CommentCollector
CommentWorkerPool *CommentWorkerPool
Session *session.SessionHistory
// DiffLookup is consulted by the code_comment tool path to resolve
// line numbers against the file's diff (or against full file content
// in scan mode — scan adapters return a synthetic Diff whose
// NewFileContent is the whole file and Diff is empty).
DiffLookup func(path string) *model.Diff
}
// Runner is a per-session (across files) executor of the LLM tool-use
// loop. Token counters, warnings, and the optional background compression
// job are aggregated across every RunPerFile call.
type Runner struct {
deps Deps
totalInputTokens int64 // atomically updated
totalOutputTokens int64
totalCacheReadTokens int64
totalCacheWriteTokens int64
warningsMu sync.Mutex
warnings []AgentWarning
compressionMu sync.Mutex
pendingJob *compressionJob
}
// NewRunner returns a Runner bound to the given dependencies.
func NewRunner(deps Deps) *Runner {
return &Runner{deps: deps}
}
// TotalInputTokens returns the accumulated input/prompt tokens from all LLM calls.
func (r *Runner) TotalInputTokens() int64 { return atomic.LoadInt64(&r.totalInputTokens) }
// TotalOutputTokens returns the accumulated completion tokens from all LLM calls.
func (r *Runner) TotalOutputTokens() int64 { return atomic.LoadInt64(&r.totalOutputTokens) }
// TotalCacheReadTokens returns the accumulated cache read tokens.
func (r *Runner) TotalCacheReadTokens() int64 { return atomic.LoadInt64(&r.totalCacheReadTokens) }
// TotalCacheWriteTokens returns the accumulated cache write tokens.
func (r *Runner) TotalCacheWriteTokens() int64 { return atomic.LoadInt64(&r.totalCacheWriteTokens) }
// TotalTokensUsed returns input + output.
func (r *Runner) TotalTokensUsed() int64 {
return r.TotalInputTokens() + r.TotalOutputTokens()
}
// Warnings returns a copy of the accumulated warnings.
func (r *Runner) Warnings() []AgentWarning {
r.warningsMu.Lock()
defer r.warningsMu.Unlock()
out := make([]AgentWarning, len(r.warnings))
copy(out, r.warnings)
return out
}
// RecordWarning adds a non-fatal warning.
func (r *Runner) RecordWarning(warningType, file, message string) {
r.warningsMu.Lock()
r.warnings = append(r.warnings, AgentWarning{
File: file,
Message: message,
Type: warningType,
})
r.warningsMu.Unlock()
}
// RecordUsage adds the prompt/completion/cache tokens reported by an LLM
// response to the runner's aggregate counters. Used by callers (plan phase
// in agent / future scan phases) that perform their own LLM calls outside
// RunPerFile.
func (r *Runner) RecordUsage(u *llm.UsageInfo) {
if u == nil {
return
}
atomic.AddInt64(&r.totalInputTokens, u.PromptTokens)
atomic.AddInt64(&r.totalOutputTokens, u.CompletionTokens)
atomic.AddInt64(&r.totalCacheReadTokens, u.CacheReadTokens)
atomic.AddInt64(&r.totalCacheWriteTokens, u.CacheWriteTokens)
}
// CollectPendingComments awaits any async comment-processing workers and
// returns the aggregated comments from the collector. Safe to call once
// per session at the end.
func (r *Runner) CollectPendingComments() []model.LlmComment {
if r.deps.CommentWorkerPool != nil {
r.deps.CommentWorkerPool.Await()
}
return r.deps.CommentCollector.Comments()
}
// RunPerFile drives the main LLM conversation loop for a single file.
// It sends messages with the configured tool definitions, executes any
// tool calls returned by the model, and collects review comments until
// task_done is called or limits are reached. Token usage and warnings
// are aggregated on the Runner across all files.
func (r *Runner) RunPerFile(ctx context.Context, messages []llm.Message, newPath string) error {
toolReqCount := r.deps.Template.MaxToolRequestTimes
const maxConsecutiveEmptyRounds = 3
consecutiveEmptyRounds := 0
for toolReqCount > 0 {
select {
case <-ctx.Done():
return ctx.Err()
default:
}
toolReqCount--
fs := r.deps.Session.GetOrCreateFileSession(newPath)
rec := fs.AppendTaskRecord(session.MainTask, append([]llm.Message(nil), messages...))
startTime := time.Now()
resp, err := r.deps.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{
Model: r.deps.Model,
Messages: messages,
Tools: r.deps.MainToolDefs,
MaxTokens: r.deps.Template.MaxTokens,
})
duration := time.Since(startTime)
if err != nil {
rec.SetError(err, duration)
telemetry.RecordLLMRequest(ctx, r.deps.Model, duration, 0, "error")
return fmt.Errorf("LLM completion error: %w", err)
}
rec.SetResponse(resp, duration)
totalTokens := int64(0)
if resp.Usage != nil {
totalTokens = resp.Usage.TotalTokens
atomic.AddInt64(&r.totalInputTokens, resp.Usage.PromptTokens)
atomic.AddInt64(&r.totalOutputTokens, resp.Usage.CompletionTokens)
atomic.AddInt64(&r.totalCacheReadTokens, resp.Usage.CacheReadTokens)
atomic.AddInt64(&r.totalCacheWriteTokens, resp.Usage.CacheWriteTokens)
}
telemetry.RecordLLMRequest(ctx, r.deps.Model, duration, totalTokens, "ok")
content := resp.Content()
calls := resp.ToolCalls()
if len(calls) == 0 {
fmt.Fprintf(stdout.Writer(), "[ocr] No tool calls parsed for %s, retrying...\n", newPath)
messages = append(messages, llm.NewTextMessage("user", "You did not successfully call any tools. Please try again or use task_done if finished."))
if content != "" {
messages = append(messages[:len(messages)-1], llm.NewTextMessage("assistant", content), messages[len(messages)-1])
}
continue
}
var results []tool.ToolCallResult
taskCompleted := false
hasValidResult := false
for _, call := range calls {
cp := r.executeToolCall(ctx, newPath, call, rec)
if cp.Completed {
results = append(results, tool.ToolCallResult{
ToolCallID: call.ID,
Name: call.Function.Name,
Result: "Task completed successfully.",
})
taskCompleted = true
} else if cp.Data != "" {
results = append(results, tool.ToolCallResult{
ToolCallID: call.ID,
Name: call.Function.Name,
Result: cp.Data,
})
hasValidResult = true
} else {
results = append(results, tool.ToolCallResult{
ToolCallID: call.ID,
Name: call.Function.Name,
Result: "Error: Tool execution returned no result.",
})
}
}
if taskCompleted {
break
}
if !hasValidResult {
consecutiveEmptyRounds++
if consecutiveEmptyRounds >= maxConsecutiveEmptyRounds {
fmt.Fprintf(stdout.Writer(), "[ocr] Too many empty retries for %s, stopping.\n", newPath)
break
}
fmt.Fprintf(stdout.Writer(), "[ocr] No valid tool results for %s, retrying...\n", newPath)
} else {
consecutiveEmptyRounds = 0
}
succeed := r.addNextMessage(ctx, content, calls, results, &messages, newPath)
if !succeed {
fmt.Fprintf(stdout.Writer(), "[ocr] Context compression exceeded threshold for %s, stopping.\n", newPath)
break
}
}
if toolReqCount <= 0 {
fmt.Fprintf(stdout.Writer(), "[ocr] Max tool requests reached for %s.\n", newPath)
}
return nil
}
// executeToolCall dispatches a single tool call from the LLM response and
// records the result in session history. code_comment handling includes
// optional async dispatch through CommentWorkerPool plus line-number
// resolution / re-location.
func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.ToolCall, rec *session.TaskRecord) tool.TaskCheckpoint {
t := tool.OfName(call.Function.Name)
if !t.IsKnown() {
return tool.Of(tool.NotAvailableMsg)
}
if t == tool.TaskDone {
return tool.Complete()
}
p := lookupTool(r.deps.Tools, t)
if p == nil {
return tool.Of(tool.NotAvailableMsg)
}
var args map[string]any
if err := json.Unmarshal([]byte(call.Function.Arguments), &args); err != nil {
return tool.Of(fmt.Sprintf("Error parsing tool arguments for %s: %v", t.Name(), err))
}
// Inject current file path as default for code_comment when not provided.
if t == tool.CodeComment && newPath != "" {
if _, ok := args["path"]; !ok {
args["path"] = newPath
}
}
startTime := time.Now()
if t == tool.CodeComment {
telemetry.PrintToolCallStarted(t.Name(), args)
comments, errMsg := tool.ParseComments(args)
if errMsg != "" {
telemetry.RecordToolCall(ctx, t.Name(), time.Since(startTime), false)
return tool.Of(errMsg)
}
resolveAndCollect := func(rctx context.Context) {
for i := range comments {
cm := &comments[i]
var d *model.Diff
if r.deps.DiffLookup != nil {
d = r.deps.DiffLookup(cm.Path)
}
if d != nil {
if !diff.ResolveComment(cm, d) && r.deps.Template.ReLocationTask != nil {
rlStart := time.Now()
_, resp, msgs := diff.ReLocateComment(rctx, cm, d, r.deps.LLMClient, r.deps.Template.ReLocationTask, r.deps.Model, r.deps.Template.MaxTokens)
if msgs != nil {
fs := r.deps.Session.GetOrCreateFileSession(cm.Path)
rlRec := fs.AppendTaskRecord(session.ReLocationTask, msgs)
if resp != nil {
rlRec.SetResponse(resp, time.Since(rlStart))
if resp.Usage != nil {
atomic.AddInt64(&r.totalInputTokens, resp.Usage.PromptTokens)
atomic.AddInt64(&r.totalOutputTokens, resp.Usage.CompletionTokens)
atomic.AddInt64(&r.totalCacheReadTokens, resp.Usage.CacheReadTokens)
atomic.AddInt64(&r.totalCacheWriteTokens, resp.Usage.CacheWriteTokens)
}
} else {
rlRec.SetError(fmt.Errorf("re-location LLM call failed"), time.Since(rlStart))
}
}
}
}
r.deps.CommentCollector.Add(*cm)
}
}
if r.deps.CommentWorkerPool != nil {
if rec != nil {
rec.AddToolResult(t.Name(), call.Function.Arguments, "(async)")
}
pool := r.deps.CommentWorkerPool
asyncCtx := context.WithoutCancel(ctx)
toolName := t.Name()
pool.Submit(func() ([]model.LlmComment, error) {
resolveAndCollect(asyncCtx)
telemetry.PrintToolCallFinished(toolName, time.Since(startTime))
return []model.LlmComment{}, nil
})
telemetry.RecordToolCall(asyncCtx, toolName, time.Since(startTime), true)
return tool.Of(tool.CommentSucceed)
}
resolveAndCollect(ctx)
dur := time.Since(startTime)
telemetry.RecordToolCall(ctx, t.Name(), dur, true)
telemetry.PrintToolCallFinished(t.Name(), dur)
if rec != nil {
rec.AddToolResult(t.Name(), call.Function.Arguments, tool.CommentSucceed)
}
return tool.Of(tool.CommentSucceed)
}
// Synchronous path for all other tools
telemetry.PrintToolCallStarted(t.Name(), args)
result, err := p.Execute(ctx, args)
dur := time.Since(startTime)
ok := err == nil
telemetry.RecordToolCall(ctx, t.Name(), dur, ok)
if err != nil {
telemetry.PrintToolCallError(t.Name(), err)
return tool.Of(fmt.Sprintf("Error executing tool %s: %v", t.Name(), err))
}
telemetry.PrintToolCallFinished(t.Name(), dur)
if rec != nil {
rec.AddToolResult(t.Name(), call.Function.Arguments, result)
}
return tool.Of(result)
}
// addNextMessage extends the conversation with the assistant message and
// tool responses, applying three-zone compression at the soft (60%) and
// warning (80%) MaxTokens thresholds. Returns false when even after
// synchronous compression the conversation is still over the warning
// threshold — caller should stop the loop in that case.
func (r *Runner) addNextMessage(ctx context.Context, assistantContent string, toolCalls []llm.ToolCall, results []tool.ToolCallResult, messages *[]llm.Message, filePath string) bool {
maxAllowed := r.deps.Template.MaxTokens
softLimit := int(float64(maxAllowed) * tokenSoftThreshold)
warnLimit := int(float64(maxAllowed) * tokenWarningThreshold)
r.tryApplyPendingCompression(messages)
tokenCount := CountMessagesTokens(*messages)
if tokenCount > warnLimit {
r.cancelPendingCompression()
*messages, _ = r.runCompression(ctx, *messages, filePath)
tokenCount = CountMessagesTokens(*messages)
}
if tokenCount > softLimit && r.pendingJob == nil {
r.triggerAsyncCompression(ctx, *messages, filePath)
}
if len(toolCalls) > 0 {
*messages = append(*messages, llm.NewToolCallMessage(assistantContent, toolCalls))
} else if assistantContent != "" {
*messages = append(*messages, llm.NewTextMessage("assistant", assistantContent))
}
for _, rs := range results {
*messages = append(*messages, llm.NewToolResultMessage(rs.ToolCallID, rs.Result))
}
finalCount := CountMessagesTokens(*messages)
if finalCount > warnLimit {
r.cancelPendingCompression()
*messages, _ = r.runCompression(ctx, *messages, filePath)
}
return CountMessagesTokens(*messages) < warnLimit
}
// lookupTool returns the provider for a given tool from the registry, or
// nil when not registered.
func lookupTool(reg *tool.Registry, t tool.Tool) tool.Provider {
p, ok := reg.Get(t.Name())
if !ok {
return nil
}
return p
}

74
internal/llmloop/pool.go Normal file
View file

@ -0,0 +1,74 @@
// Package llmloop carries the per-file LLM tool-use loop shared by `ocr
// review` (diff-based) and `ocr scan` (full-file). It owns the chat
// completion conversation state, three-zone memory compression, tool-call
// dispatch (including async comment post-processing), and aggregate token /
// warning bookkeeping. Callers above this package render the initial
// messages (review uses MAIN_TASK, scan uses FULL_SCAN_TASK) and hand them
// in via Runner.RunPerFile.
package llmloop
import (
"fmt"
"sync"
"github.com/open-code-review/open-code-review/internal/model"
"github.com/open-code-review/open-code-review/internal/stdout"
)
// AgentWarning describes a non-fatal warning recorded during a per-file
// review/scan. The name is kept for backwards compatibility with the
// previous internal/agent package.
type AgentWarning struct {
File string `json:"file"`
Message string `json:"message"`
Type string `json:"type"`
}
// CommentWorkerPool manages a fixed-size pool of workers dedicated to
// processing code-review comment post-steps (line-range tracking,
// re-tracking, reflection, suggestion validation) asynchronously.
//
// Offloading them to a worker pool keeps the main LLM tool-use loop
// unblocked, reducing overall latency — mirroring the Java side's dedicated
// subtaskExecutor for the CODE_COMMENT tool.
type CommentWorkerPool struct {
semaphore chan struct{}
wg sync.WaitGroup
resultsMu sync.Mutex
results []model.LlmComment
}
// NewCommentWorkerPool creates a pool with the given concurrency limit.
// workerCount <= 0 defaults to 8.
func NewCommentWorkerPool(workerCount int) *CommentWorkerPool {
if workerCount <= 0 {
workerCount = 8
}
return &CommentWorkerPool{
semaphore: make(chan struct{}, workerCount),
}
}
// Submit runs f in a background goroutine bounded by the semaphore.
// When f completes its return value is collected internally.
func (p *CommentWorkerPool) Submit(f func() ([]model.LlmComment, error)) {
p.wg.Go(func() {
p.semaphore <- struct{}{}
defer func() { <-p.semaphore }()
comments, err := f()
if err != nil {
fmt.Fprintf(stdout.Writer(), "[ocr] CommentWorkerPool error: %v\n", err)
}
p.resultsMu.Lock()
p.results = append(p.results, comments...)
p.resultsMu.Unlock()
})
}
// Await blocks until all submitted work has completed and returns
// aggregated results from every Submit call so far.
func (p *CommentWorkerPool) Await() []model.LlmComment {
p.wg.Wait()
return p.results
}

35
internal/model/preview.go Normal file
View file

@ -0,0 +1,35 @@
package model
// ExcludeReason describes why a file was excluded from review. Shared by
// both diff review (internal/agent) and full-file scan (internal/scan).
type ExcludeReason string
const (
ExcludeNone ExcludeReason = ""
ExcludeUserRule ExcludeReason = "user_exclude"
ExcludeExtension ExcludeReason = "unsupported_ext"
ExcludeDefaultPath ExcludeReason = "default_path"
ExcludeDeleted ExcludeReason = "deleted"
ExcludeBinary ExcludeReason = "binary"
)
// PreviewEntry is one file's preview record (mode-agnostic).
type PreviewEntry struct {
Path string `json:"path"`
Status string `json:"status"`
Insertions int64 `json:"insertions"`
Deletions int64 `json:"deletions"`
WillReview bool `json:"will_review"`
ExcludeReason ExcludeReason `json:"exclude_reason,omitempty"`
}
// Preview is the full preview result, mode-agnostic so cmd/opencodereview
// can render it the same way for review and scan.
type Preview struct {
Entries []PreviewEntry `json:"files"`
TotalInsertions int64 `json:"total_insertions"`
TotalDeletions int64 `json:"total_deletions"`
TotalFiles int `json:"total_files"`
ReviewableCount int `json:"reviewable_count"`
ExcludedCount int `json:"excluded_count"`
}

30
internal/model/scan.go Normal file
View file

@ -0,0 +1,30 @@
package model
// ScanItem represents a single file enumerated by full-scan mode. Unlike
// model.Diff (which carries a unified diff text), ScanItem carries the
// entire file content because scan reviews whole files with no diff
// context.
type ScanItem struct {
Path string `json:"path"`
Content string `json:"content"`
IsBinary bool `json:"is_binary,omitempty"`
LineCount int `json:"line_count,omitempty"`
}
// AsDiff returns a Diff suitable for handing to code that expects the
// diff-based shape (line-number resolver, file_read_diff tool). The Diff
// field stays empty since scan mode has no unified diff; NewFileContent
// carries the whole file so resolver.resolveFromFileContent and similar
// fallbacks can still find the source lines.
func (s *ScanItem) AsDiff() *Diff {
if s == nil {
return nil
}
return &Diff{
OldPath: s.Path,
NewPath: s.Path,
NewFileContent: s.Content,
IsBinary: s.IsBinary,
Insertions: int64(s.LineCount),
}
}

890
internal/scan/agent.go Normal file
View file

@ -0,0 +1,890 @@
package scan
import (
"context"
"encoding/json"
"fmt"
"strings"
"sync"
"sync/atomic"
"time"
allowedext "github.com/open-code-review/open-code-review/internal/config/allowlist"
"github.com/open-code-review/open-code-review/internal/config/rules"
"github.com/open-code-review/open-code-review/internal/config/template"
"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/llmloop"
"github.com/open-code-review/open-code-review/internal/model"
"github.com/open-code-review/open-code-review/internal/session"
"github.com/open-code-review/open-code-review/internal/stdout"
"github.com/open-code-review/open-code-review/internal/telemetry"
"github.com/open-code-review/open-code-review/internal/tool"
)
// changeFilesScanLiteral substitutes for the {{change_files}} placeholder.
// Full-scan has no "other changed files" concept; using a fixed sentinel is
// less misleading than leaving the placeholder empty.
const changeFilesScanLiteral = "(not applicable in full-scan mode)"
// Args bundles all dependencies needed for one scan session.
//
// Note: Template is the scan-specific template.ScanTemplate (loaded from
// scan_template.json), not the diff-review template.Template. The two are
// intentionally separate so review/scan prompts evolve independently.
//
// MaxFileSizeBytes overrides the default 2 MiB per-file size cap; it is
// usually populated from ScanTemplate.MaxFileSizeBytes via scan_cmd.
type Args struct {
RepoDir string
Paths []string // empty = whole repo
Template template.ScanTemplate
SystemRule rules.Resolver
FileFilter *rules.FileFilter
LLMClient llm.LLMClient
Tools *tool.Registry
MainToolDefs []llm.ToolDef
CommentCollector *tool.CommentCollector
CommentWorkerPool *llmloop.CommentWorkerPool
MaxConcurrency int
ConcurrentTaskTimeout int
Model string
Background string
GitRunner *gitcmd.Runner
Session *session.SessionHistory
MaxFileSizeBytes int64
// SkipPlan disables the PLAN_TASK pre-pass even when the template
// defines one. Set via the --no-plan CLI flag.
SkipPlan bool
// SkipDedup disables the per-batch DEDUP_TASK even when the template
// defines one. Set via the --no-dedup CLI flag.
SkipDedup bool
// SkipSummary disables the post-run PROJECT_SUMMARY_TASK even when the
// template defines one. Set via the --no-summary CLI flag.
SkipSummary bool
// MaxTokensBudget, when > 0, caps total token usage (input+output, as
// reported by the API). Once the running total exceeds it, no further
// batches are dispatched. 0 = unlimited. Set via --max-tokens-budget
// or ScanTemplate.MaxTokensBudget.
MaxTokensBudget int64
}
// planEnabled / dedupEnabled / summaryEnabled report whether each optional
// phase will actually run: template must define it AND the corresponding
// --no-* flag must not be set. Used by both cost estimation and dispatch.
func (a *Agent) planEnabled() bool {
return !a.args.SkipPlan && a.args.Template.PlanTask != nil && len(a.args.Template.PlanTask.Messages) > 0
}
func (a *Agent) dedupEnabled() bool {
return !a.args.SkipDedup && a.args.Template.DedupTask != nil && len(a.args.Template.DedupTask.Messages) > 0
}
func (a *Agent) summaryEnabled() bool {
return !a.args.SkipSummary && a.args.Template.ProjectSummaryTask != nil && len(a.args.Template.ProjectSummaryTask.Messages) > 0
}
// Agent orchestrates full-file code review. It delegates the per-file LLM
// tool-use loop to llmloop.Runner and owns only scan-specific concerns
// (file enumeration, FULL_SCAN_TASK rendering, per-file filtering).
type Agent struct {
args Args
items []model.ScanItem
currentDate string
session *session.SessionHistory
subtaskFailed int64 // atomic
runner *llmloop.Runner
projectSummary string // populated post-run by maybeRunProjectSummary
}
// ProjectSummary returns the markdown project-level summary produced after
// all batches finish. Empty when SkipSummary is set, PROJECT_SUMMARY_TASK
// is absent, no comments were collected, or the summary LLM call failed.
func (a *Agent) ProjectSummary() string { return a.projectSummary }
// NewAgent creates a scan Agent from the given args. The Session is
// auto-created (review_mode = full_scan) when not supplied.
func NewAgent(args Args) *Agent {
if args.Tools == nil {
args.Tools = tool.NewRegistry()
}
if args.CommentCollector == nil {
args.CommentCollector = tool.NewCommentCollector()
}
if args.Session == nil {
args.Session = session.New(args.RepoDir, "", args.Model, session.SessionOptions{
ReviewMode: session.ReviewModeFullScan,
})
}
a := &Agent{
args: args,
session: args.Session,
}
a.runner = llmloop.NewRunner(llmloop.Deps{
LLMClient: args.LLMClient,
Model: args.Model,
Template: toLoopTemplate(args.Template),
Tools: args.Tools,
MainToolDefs: args.MainToolDefs,
CommentCollector: args.CommentCollector,
CommentWorkerPool: args.CommentWorkerPool,
Session: args.Session,
// DiffLookup returns a synthetic Diff so the code_comment tool's
// line-number resolver (resolveFromFileContent) can match against
// the full file content of the scanned file.
DiffLookup: a.lookupDiff,
})
return a
}
// toLoopTemplate maps the scan-specific ScanTemplate onto the subset of
// fields llmloop.Runner reads from template.Template. llmloop only needs
// MaxTokens / MaxToolRequestTimes / MemoryCompressionTask / ReLocationTask,
// so we leave the diff-only fields (MainTask / PlanTask / ReviewFilterTask)
// at their zero values.
func toLoopTemplate(s template.ScanTemplate) template.Template {
return template.Template{
MemoryCompressionTask: s.MemoryCompressionTask,
MaxTokens: s.MaxTokens,
MaxToolRequestTimes: s.MaxToolRequestTimes,
ReLocationTask: s.ReLocationTask,
}
}
// Session returns the session history associated with this Agent.
func (a *Agent) Session() *session.SessionHistory { return a.session }
// FilesReviewed returns the number of items included in this scan.
func (a *Agent) FilesReviewed() int64 { return int64(len(a.items)) }
// Diffs returns the scanned items adapted to model.Diff form so callers
// (e.g. cmd/opencodereview's outputJSON / ResolveLineNumbers) can treat
// both review and scan results uniformly.
func (a *Agent) Diffs() []model.Diff {
out := make([]model.Diff, len(a.items))
for i := range a.items {
out[i] = *a.items[i].AsDiff()
}
return out
}
// TotalTokensUsed / TotalInputTokens / ... delegate to the underlying runner.
func (a *Agent) TotalTokensUsed() int64 { return a.runner.TotalTokensUsed() }
func (a *Agent) TotalInputTokens() int64 { return a.runner.TotalInputTokens() }
func (a *Agent) TotalOutputTokens() int64 { return a.runner.TotalOutputTokens() }
func (a *Agent) TotalCacheReadTokens() int64 { return a.runner.TotalCacheReadTokens() }
func (a *Agent) TotalCacheWriteTokens() int64 {
return a.runner.TotalCacheWriteTokens()
}
// Warnings returns the warnings recorded by the LLM runner.
func (a *Agent) Warnings() []llmloop.AgentWarning { return a.runner.Warnings() }
func (a *Agent) recordWarning(warningType, file, message string) {
a.runner.RecordWarning(warningType, file, message)
}
// Run executes the full-scan pipeline: enumerate → filter → token-filter →
// dispatch one subtask per file → collect comments.
func (a *Agent) Run(ctx context.Context) ([]model.LlmComment, error) {
if len(a.args.Template.MainTask.Messages) == 0 {
return nil, fmt.Errorf("scan template MAIN_TASK is missing or empty")
}
ctx, scanSpan := telemetry.StartSpan(ctx, "scan.enumerate")
provider := NewProvider(a.args.RepoDir, a.args.Paths, a.args.GitRunner, a.args.MaxFileSizeBytes)
items, err := provider.Enumerate(ctx)
if err != nil {
scanSpan.End()
return nil, fmt.Errorf("enumerate files: %w", err)
}
telemetry.SetAttr(scanSpan, "files.enumerated", len(items))
scanSpan.End()
a.items = items
a.injectScanContentMap()
a.args.Tools.Freeze()
totalDiscovered := len(a.items)
a.items = a.filterScanItems(a.items)
a.items = a.filterLargeScans(a.items)
reviewable := len(a.items)
fmt.Fprintf(stdout.Writer(), "[ocr] full-scan: %d file(s) discovered, reviewing %d in %s\n",
totalDiscovered, reviewable, a.args.RepoDir)
if reviewable == 0 {
fmt.Fprintln(stdout.Writer(), "[ocr] No reviewable files. Skipping scan.")
telemetry.Event(ctx, "scan.no.files")
a.session.Finalize()
return []model.LlmComment{}, nil
}
// Pre-run cost projection so users aren't surprised by a large scan.
est := estimateCost(a.items, a.planEnabled(), a.dedupEnabled(), a.summaryEnabled())
fmt.Fprintf(stdout.Writer(), "[ocr] estimated cost: %s\n", est)
if a.args.MaxTokensBudget > 0 {
fmt.Fprintf(stdout.Writer(), "[ocr] token budget: %s (dispatch stops once exceeded)\n", humanTokens(a.args.MaxTokensBudget))
if est.TotalTokens > a.args.MaxTokensBudget {
fmt.Fprintf(stdout.Writer(), "[ocr] WARNING: estimate (%s) exceeds budget (%s); scan will stop partway\n",
humanTokens(est.TotalTokens), humanTokens(a.args.MaxTokensBudget))
}
}
a.currentDate = time.Now().Format("2006-01-02 15:04")
telemetry.Event(ctx, "scan.started",
telemetry.AnyToAttr("file.count", totalDiscovered),
telemetry.AnyToAttr("review.count", reviewable),
telemetry.AnyToAttr("est.total.tokens", est.TotalTokens),
telemetry.AnyToAttr("repo.dir", a.args.RepoDir))
telemetry.RecordFilesReviewed(ctx, int64(reviewable))
comments, err := a.dispatchSubtasks(ctx)
if len(comments) > 0 {
telemetry.RecordCommentsGenerated(ctx, int64(len(comments)))
}
// Project-level summary runs after all batches; never blocks return.
a.maybeRunProjectSummary(ctx, comments)
a.session.Finalize()
return comments, err
}
// lookupDiff returns the synthetic Diff for a path, used by llmloop.Runner
// to resolve code_comment line numbers against the scanned file content.
func (a *Agent) lookupDiff(path string) *model.Diff {
for i := range a.items {
if a.items[i].Path == path {
return a.items[i].AsDiff()
}
}
return nil
}
// injectScanContentMap fills the file_read_diff tool's DiffMap with full
// file content keyed by path, so if the model calls it the tool returns
// the whole file rather than failing.
func (a *Agent) injectScanContentMap() {
m := make(map[string]string, len(a.items))
for i := range a.items {
it := &a.items[i]
if it.Path != "" {
m[it.Path] = it.Content
}
}
dm := tool.NewDiffMap(m)
if p, ok := a.args.Tools.Get(tool.FileReadDiff.Name()); ok {
if frd, ok := p.(*tool.FileReadDiffProvider); ok {
frd.SetDiffMap(dm)
}
}
}
// filterScanItems drops items that should not be reviewed under the standard
// reviewability rules (binary, extension allowlist, user include/exclude,
// default excluded paths).
func (a *Agent) filterScanItems(items []model.ScanItem) []model.ScanItem {
var kept []model.ScanItem
skipped := 0
for _, it := range items {
if reason := a.whyExcluded(it); reason != model.ExcludeNone {
if it.IsBinary {
fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — binary file\n", it.Path)
} else {
fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — filtered by path/extension rules\n", it.Path)
}
skipped++
continue
}
kept = append(kept, it)
}
if skipped > 0 {
fmt.Fprintf(stdout.Writer(), "[ocr] Filtered %d file(s) by include/exclude rules\n", skipped)
}
return kept
}
// filterLargeScans drops items whose content exceeds 80% of MaxTokens.
func (a *Agent) filterLargeScans(items []model.ScanItem) []model.ScanItem {
limit := a.args.Template.MaxTokens * 4 / 5
if limit <= 0 {
return items
}
var kept []model.ScanItem
skipped := 0
for _, it := range items {
tokens := llm.CountTokens(it.Content)
if tokens > limit {
fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s (~%d tokens exceeds 80%% of max_tokens(%d))\n",
it.Path, tokens, a.args.Template.MaxTokens)
skipped++
continue
}
kept = append(kept, it)
}
if skipped > 0 {
fmt.Fprintf(stdout.Writer(), "[ocr] Pre-filtered %d file(s) exceeding 80%% of max_tokens\n", skipped)
}
return kept
}
// whyExcluded mirrors agent.whyExcluded but for ScanItem inputs.
func (a *Agent) whyExcluded(it model.ScanItem) model.ExcludeReason {
if it.IsBinary {
return model.ExcludeBinary
}
path := it.Path
if a.args.FileFilter != nil && a.args.FileFilter.IsUserExcluded(path) {
return model.ExcludeUserRule
}
ext := extFromPath(path)
if ext != "" && !allowedext.IsAllowedExt(ext) {
return model.ExcludeExtension
}
if a.args.FileFilter != nil && a.args.FileFilter.HasInclude() && a.args.FileFilter.IsUserIncluded(path) {
return model.ExcludeNone
}
if allowedext.IsExcludedPath(path) {
return model.ExcludeDefaultPath
}
return model.ExcludeNone
}
func extFromPath(path string) string {
basename := path
if idx := strings.LastIndex(path, "/"); idx >= 0 {
basename = path[idx+1:]
}
dot := strings.LastIndex(basename, ".")
if dot <= 0 {
return ""
}
return strings.ToLower(basename[dot:])
}
// dispatchSubtasks groups items into batches per the configured strategy,
// then processes batches sequentially while running files within each
// batch concurrently up to MaxConcurrency. Sequential batches enable
// future per-batch hooks (e.g. Phase 6 dedup) and improve LLM prompt-cache
// hit rate by keeping same-language files adjacent in time.
func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error) {
startTime := time.Now()
defer func() {
telemetry.RecordReviewDuration(ctx, time.Since(startTime))
}()
if len(a.items) == 0 {
return []model.LlmComment{}, nil
}
atomic.StoreInt64(&a.subtaskFailed, 0)
strategy := a.resolveBatchStrategy()
batches := groupBatches(a.items, strategy, a.args.Template.BatchSize)
fmt.Fprintf(stdout.Writer(), "[ocr] scan dispatch: %d batch(es) by %s strategy\n", len(batches), strategy)
var dispatched int64
for bi, batch := range batches {
if err := ctx.Err(); err != nil {
return a.args.CommentCollector.Comments(), err
}
// Snapshot the collector so we can isolate comments added by *this*
// batch and feed them into the per-batch dedup hook.
batchStart := a.args.CommentCollector.Snapshot()
n, budgetHit, err := a.dispatchBatch(ctx, bi, batch)
dispatched += n
if err != nil {
// ctx cancelled mid-batch: stop scheduling further batches but
// still return whatever we've collected so far.
return a.args.CommentCollector.Comments(), err
}
// Drain async comment workers BEFORE dedup so all of this batch's
// comments are visible. CommentWorkerPool.Await is cumulative
// across batches — that's fine since batches are sequential here.
if a.args.CommentWorkerPool != nil {
a.args.CommentWorkerPool.Await()
}
a.maybeRunDedup(ctx, bi, batchStart)
// The per-file budget gate inside dispatchBatch tripped — stop
// scheduling any remaining batches.
if budgetHit {
break
}
}
failed := atomic.LoadInt64(&a.subtaskFailed)
if failed > 0 && failed == dispatched {
return nil, fmt.Errorf("all %d file scan(s) failed — check your LLM configuration and API key", dispatched)
}
return a.args.CommentCollector.Comments(), nil
}
// resolveBatchStrategy reads the strategy from the scan template, defaulting
// to BatchNone for unrecognized / empty values.
func (a *Agent) resolveBatchStrategy() BatchStrategy {
return parseBatchStrategy(a.args.Template.BatchStrategy)
}
// dispatchBatch fans out the files of a single batch concurrently and
// blocks until they all finish (or ctx is cancelled). Returns the number
// of files dispatched, whether the token budget was hit mid-batch, and
// ctx.Err() if cancelled.
//
// The budget gate is checked per file, right after acquiring the
// concurrency slot and before launching the subtask: if the tokens already
// spent PLUS a look-ahead estimate of this file's cost would exceed the
// budget, the file (and all remaining files in the batch) are skipped.
// This keeps overrun bounded by roughly one in-flight file per worker,
// instead of a whole batch as the coarse batch-level gate did.
func (a *Agent) dispatchBatch(ctx context.Context, batchIdx int, batch []model.ScanItem) (int64, bool, error) {
concurrency := a.args.MaxConcurrency
if concurrency <= 0 {
concurrency = 8
}
sem := make(chan struct{}, concurrency)
timeout := time.Duration(a.args.ConcurrentTaskTimeout) * time.Minute
var (
wg sync.WaitGroup
dispatched int64
budgetHit bool
)
for i := range batch {
// Per-file budget look-ahead. Stop before acquiring a slot so we
// don't even queue work that would blow the budget.
if a.args.MaxTokensBudget > 0 {
used := a.runner.TotalTokensUsed()
projected := used + estimateFileTokens(batch[i], a.planEnabled())
if projected > a.args.MaxTokensBudget {
fmt.Fprintf(stdout.Writer(), "[ocr] token budget reached (used %s + next-file est ≈ %s > budget %s) — skipping %s and remaining files\n",
humanTokens(used), humanTokens(projected), humanTokens(a.args.MaxTokensBudget), batch[i].Path)
a.recordWarning("token_budget_reached", batch[i].Path,
fmt.Sprintf("stopped in batch #%d: used %d tokens + next-file estimate exceeds budget %d", batchIdx, used, a.args.MaxTokensBudget))
budgetHit = true
break
}
}
select {
case sem <- struct{}{}:
case <-ctx.Done():
wg.Wait()
return dispatched, budgetHit, ctx.Err()
}
dispatched++
wg.Add(1)
go func(it model.ScanItem) {
defer wg.Done()
defer func() { <-sem }()
var fileCtx context.Context
var cancel context.CancelFunc
if timeout > 0 {
fileCtx, cancel = context.WithTimeout(ctx, timeout)
defer cancel()
} else {
fileCtx = ctx
}
if err := a.executeSubtask(fileCtx, it); err != nil {
atomic.AddInt64(&a.subtaskFailed, 1)
fmt.Fprintf(stdout.Writer(), "[ocr] Scan subtask error for %s (batch #%d): %v\n", it.Path, batchIdx, err)
telemetry.ErrorEvent(fileCtx, "scan.subtask.error", err,
telemetry.AnyToAttr("file.path", it.Path),
telemetry.AnyToAttr("batch.index", batchIdx))
a.recordWarning("scan_subtask_error", it.Path, err.Error())
}
}(batch[i])
}
wg.Wait()
return dispatched, budgetHit, nil
}
// executeSubtask runs the scan pipeline for one file:
// 1. Optional PLAN_TASK: produce a JSON checklist of focus areas.
// 2. MAIN_TASK: review the file with the plan's checkpoints embedded as
// {{plan_guidance}}.
//
// Plan phase is skipped (and {{plan_guidance}} is filled with a "no plan"
// sentinel) when Template.PlanTask is nil, args.SkipPlan is true, the file
// is small enough that planning overhead outweighs gain, or the plan call
// itself fails. Plan failure never blocks the main review — it falls back
// to v1 (plan-less) behavior.
func (a *Agent) executeSubtask(ctx context.Context, it model.ScanItem) error {
ctx, span := telemetry.StartSpan(ctx, "scan.subtask."+it.Path)
defer span.End()
telemetry.SetAttr(span, "file.path", it.Path)
if ctx.Err() != nil {
return ctx.Err()
}
rule := ""
if a.args.SystemRule != nil {
rule = a.args.SystemRule.Resolve(strings.ToLower(it.Path))
}
planGuidance := a.maybeRunPlan(ctx, it, rule)
messages := a.renderMessages(it, rule, planGuidance)
tokenCount := llmloop.CountMessagesTokens(messages)
maxAllowed := a.args.Template.MaxTokens
tokenLimit := maxAllowed * 4 / 5
if tokenCount > tokenLimit {
msg := fmt.Sprintf("prompt tokens (%d) exceed %d%% of max_tokens(%d)", tokenCount, 80, maxAllowed)
fmt.Fprintf(stdout.Writer(), "[ocr] WARNING: %s for %s\n", msg, it.Path)
a.recordWarning("token_threshold_exceeded", it.Path, msg)
telemetry.Event(ctx, "token.threshold.exceeded",
telemetry.AnyToAttr("file.path", it.Path),
telemetry.AnyToAttr("tokens", tokenCount),
telemetry.AnyToAttr("max_tokens", maxAllowed))
return nil
}
return a.runner.RunPerFile(ctx, messages, it.Path)
}
// maybeRunPlan invokes PLAN_TASK on the file and returns a human-readable
// guidance string suitable for {{plan_guidance}} substitution. Returns "(no
// pre-scan plan; review the entire file as usual)" when planning is
// disabled or fails — that sentinel is intentionally non-empty so the
// surrounding "### Pre-scan Focus Areas" header in MAIN_TASK has content
// instead of dangling.
func (a *Agent) maybeRunPlan(ctx context.Context, it model.ScanItem, rule string) string {
const noPlan = "(no pre-scan plan; review the entire file as usual)"
if !a.planEnabled() {
return noPlan
}
pt := a.args.Template.PlanTask
// Render plan messages.
messages := make([]llm.Message, 0, len(pt.Messages))
for _, m := range pt.Messages {
content := m.Content
content = strings.ReplaceAll(content, "{{current_system_date_time}}", a.currentDate)
content = strings.ReplaceAll(content, "{{current_file_path}}", it.Path)
content = strings.ReplaceAll(content, "{{system_rule}}", rule)
content = strings.ReplaceAll(content, "{{file_content}}", it.Content)
messages = append(messages, llm.NewTextMessage(m.Role, content))
}
fs := a.session.GetOrCreateFileSession(it.Path)
rec := fs.AppendTaskRecord(session.PlanTask, messages)
startTime := time.Now()
resp, err := a.args.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{
Model: a.args.Model,
Messages: messages,
MaxTokens: a.args.Template.MaxTokens,
})
if err != nil {
rec.SetError(err, time.Since(startTime))
fmt.Fprintf(stdout.Writer(), "[ocr] scan plan failed for %s: %v (falling back to plan-less)\n", it.Path, err)
return noPlan
}
rec.SetResponse(resp, time.Since(startTime))
a.runner.RecordUsage(resp.Usage)
guidance := formatPlanGuidance(resp.Content())
if guidance == "" {
return noPlan
}
return guidance
}
// maybeRunProjectSummary runs the post-batch PROJECT_SUMMARY_TASK over the
// union of all collected comments. Best-effort: any error / empty input
// / no-template silently leaves projectSummary unset.
func (a *Agent) maybeRunProjectSummary(ctx context.Context, comments []model.LlmComment) {
if !a.summaryEnabled() {
return
}
pt := a.args.Template.ProjectSummaryTask
if len(comments) == 0 {
return
}
// Distinct file count for header context.
fileSet := make(map[string]struct{}, len(comments))
for _, c := range comments {
fileSet[c.Path] = struct{}{}
}
payload := buildSummaryCommentsList(comments)
messages := make([]llm.Message, 0, len(pt.Messages))
for _, m := range pt.Messages {
content := m.Content
content = strings.ReplaceAll(content, "{{comment_count}}", fmt.Sprintf("%d", len(comments)))
content = strings.ReplaceAll(content, "{{file_count}}", fmt.Sprintf("%d", len(fileSet)))
content = strings.ReplaceAll(content, "{{all_comments}}", payload)
messages = append(messages, llm.NewTextMessage(m.Role, content))
}
const pathKey = "__scan_project_summary__"
fs := a.session.GetOrCreateFileSession(pathKey)
rec := fs.AppendTaskRecord(session.MemoryCompressionTask, messages) // reuse existing task type
startTime := time.Now()
resp, err := a.args.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{
Model: a.args.Model,
Messages: messages,
MaxTokens: a.args.Template.MaxTokens,
})
if err != nil {
rec.SetError(err, time.Since(startTime))
fmt.Fprintf(stdout.Writer(), "[ocr] scan project summary failed: %v\n", err)
return
}
rec.SetResponse(resp, time.Since(startTime))
a.runner.RecordUsage(resp.Usage)
body := strings.TrimSpace(llmloop.StripMarkdownFences(resp.Content()))
if body == "" {
return
}
a.projectSummary = body
}
// buildSummaryCommentsList renders comments as a compact path-anchored
// markdown list suitable for embedding in the PROJECT_SUMMARY_TASK prompt.
// Format: "- `path/to/file.go`: <one-line content (truncated)>".
// Content is truncated to ~280 chars to bound prompt growth on large scans.
func buildSummaryCommentsList(comments []model.LlmComment) string {
const maxLine = 280
var sb strings.Builder
for _, c := range comments {
sb.WriteString("- `")
sb.WriteString(c.Path)
sb.WriteString("`: ")
oneLine := strings.ReplaceAll(c.Content, "\n", " ")
if len(oneLine) > maxLine {
oneLine = oneLine[:maxLine] + "..."
}
sb.WriteString(oneLine)
sb.WriteString("\n")
}
return sb.String()
}
// maybeRunDedup, when the template has a DedupTask and the batch produced
// at least DedupMinComments comments, invokes the DEDUP_TASK LLM to merge
// near-duplicate findings. On any failure (LLM error / malformed JSON /
// invalid grouping) the original batch comments are kept unchanged — dedup
// is a best-effort optimization, never a correctness gate.
func (a *Agent) maybeRunDedup(ctx context.Context, batchIdx, batchStart int) {
if !a.dedupEnabled() {
return
}
dt := a.args.Template.DedupTask
minN := a.args.Template.DedupMinComments
if minN <= 0 {
minN = 2
}
batchComments := a.args.CommentCollector.Since(batchStart)
if len(batchComments) < minN {
return
}
payload := buildDedupCommentsJSON(batchComments)
messages := make([]llm.Message, 0, len(dt.Messages))
for _, m := range dt.Messages {
content := strings.ReplaceAll(m.Content, "{{batch_comments}}", payload)
messages = append(messages, llm.NewTextMessage(m.Role, content))
}
// Use a synthetic file path keyed by batch index so the session JSONL
// keeps dedup records distinct from per-file plan/main records.
pathKey := fmt.Sprintf("__scan_dedup_batch_%d__", batchIdx)
fs := a.session.GetOrCreateFileSession(pathKey)
rec := fs.AppendTaskRecord(session.MemoryCompressionTask, messages) // reuse existing task type; no scan-specific type to invent
startTime := time.Now()
resp, err := a.args.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{
Model: a.args.Model,
Messages: messages,
MaxTokens: a.args.Template.MaxTokens,
})
if err != nil {
rec.SetError(err, time.Since(startTime))
fmt.Fprintf(stdout.Writer(), "[ocr] scan dedup failed for batch #%d: %v (keeping originals)\n", batchIdx, err)
return
}
rec.SetResponse(resp, time.Since(startTime))
a.runner.RecordUsage(resp.Usage)
deduped, ok := applyDedupGroups(resp.Content(), batchComments)
if !ok {
fmt.Fprintf(stdout.Writer(), "[ocr] scan dedup batch #%d: malformed groups, keeping originals\n", batchIdx)
return
}
if len(deduped) == len(batchComments) {
// No-op result — don't bother rewriting the collector.
return
}
a.args.CommentCollector.ReplaceSince(batchStart, deduped)
fmt.Fprintf(stdout.Writer(), "[ocr] scan dedup batch #%d: %d → %d comments\n", batchIdx, len(batchComments), len(deduped))
}
// buildDedupCommentsJSON renders the batch comments as a JSON list with
// stable c-N ids that the LLM groups by. Only fields the LLM needs to
// judge similarity are included (path / content / existing_code), keeping
// the prompt compact.
func buildDedupCommentsJSON(comments []model.LlmComment) string {
type wire struct {
ID string `json:"id"`
Path string `json:"path"`
Content string `json:"content"`
ExistingCode string `json:"existing_code,omitempty"`
}
items := make([]wire, len(comments))
for i, cm := range comments {
items[i] = wire{
ID: fmt.Sprintf("c-%d", i),
Path: cm.Path,
Content: cm.Content,
ExistingCode: cm.ExistingCode,
}
}
data, _ := json.Marshal(items)
return string(data)
}
// applyDedupGroups parses the DEDUP_TASK output and returns the deduped
// comment slice. Returns (nil, false) when the response is malformed OR
// when the groups don't cover every input id exactly once (safety: we
// refuse to silently drop comments we can't account for).
func applyDedupGroups(rawJSON string, originals []model.LlmComment) ([]model.LlmComment, bool) {
stripped := llmloop.StripMarkdownFences(rawJSON)
stripped = strings.TrimSpace(stripped)
if stripped == "" {
return nil, false
}
var parsed struct {
Groups []struct {
Members []string `json:"members"`
MergedContent string `json:"merged_content,omitempty"`
} `json:"groups"`
}
if err := json.Unmarshal([]byte(stripped), &parsed); err != nil {
return nil, false
}
idToIdx := make(map[string]int, len(originals))
for i := range originals {
idToIdx[fmt.Sprintf("c-%d", i)] = i
}
seen := make(map[string]bool, len(originals))
var out []model.LlmComment
for _, g := range parsed.Groups {
if len(g.Members) == 0 {
return nil, false
}
canonicalIdx, ok := idToIdx[g.Members[0]]
if !ok {
return nil, false
}
for _, id := range g.Members {
if _, exists := idToIdx[id]; !exists {
return nil, false // unknown id
}
if seen[id] {
return nil, false // duplicate assignment
}
seen[id] = true
}
canonical := originals[canonicalIdx]
if len(g.Members) > 1 && g.MergedContent != "" {
canonical.Content = g.MergedContent
}
out = append(out, canonical)
}
if len(seen) != len(originals) {
return nil, false // some id missing
}
return out, true
}
// formatPlanGuidance parses the PLAN_TASK JSON output into a markdown
// snippet suitable for embedding in MAIN_TASK. On parse failure it returns
// the raw content so the model still gets *something* (better than the
// "no plan" fallback when the LLM did say something useful but in the
// wrong shape).
func formatPlanGuidance(raw string) string {
stripped := llmloop.StripMarkdownFences(raw)
stripped = strings.TrimSpace(stripped)
if stripped == "" {
return ""
}
var plan struct {
Summary string `json:"summary"`
Checkpoints []struct {
Focus string `json:"focus"`
Lines string `json:"lines,omitempty"`
Why string `json:"why,omitempty"`
} `json:"checkpoints"`
}
if err := json.Unmarshal([]byte(stripped), &plan); err != nil {
// Fallback: hand the raw text to the main task; it's better than
// nothing and lets us debug bad outputs from session JSONL.
return stripped
}
var sb strings.Builder
if plan.Summary != "" {
sb.WriteString("**Summary**: ")
sb.WriteString(plan.Summary)
sb.WriteString("\n\n")
}
if len(plan.Checkpoints) == 0 {
// Summary-only plan still has value as orientation.
return strings.TrimRight(sb.String(), "\n")
}
sb.WriteString("**Focus areas (give these extra attention; not exhaustive):**\n")
for i, cp := range plan.Checkpoints {
fmt.Fprintf(&sb, "%d. `%s`", i+1, cp.Focus)
if cp.Lines != "" {
fmt.Fprintf(&sb, " (lines %s)", cp.Lines)
}
if cp.Why != "" {
fmt.Fprintf(&sb, " — %s", cp.Why)
}
sb.WriteString("\n")
}
return strings.TrimRight(sb.String(), "\n")
}
// renderMessages substitutes placeholders in the scan template's MainTask
// for a single scan item. planGuidance is the output of maybeRunPlan and
// gets substituted into {{plan_guidance}}; callers should pass a non-empty
// sentinel when planning is disabled so the surrounding section header in
// the prompt template doesn't dangle.
func (a *Agent) renderMessages(it model.ScanItem, rule, planGuidance string) []llm.Message {
rawMsgs := a.args.Template.MainTask.Messages
messages := make([]llm.Message, 0, len(rawMsgs))
for _, m := range rawMsgs {
content := m.Content
content = strings.ReplaceAll(content, "{{plan_guidance}}", planGuidance)
content = strings.ReplaceAll(content, "{{current_system_date_time}}", a.currentDate)
content = strings.ReplaceAll(content, "{{current_file_path}}", it.Path)
content = strings.ReplaceAll(content, "{{system_rule}}", rule)
content = strings.ReplaceAll(content, "{{change_files}}", changeFilesScanLiteral)
content = strings.ReplaceAll(content, "{{file_content}}", it.Content)
content = strings.ReplaceAll(content, "{{requirement_background}}", a.args.Background)
messages = append(messages, llm.NewTextMessage(m.Role, content))
}
return messages
}

323
internal/scan/agent_test.go Normal file
View file

@ -0,0 +1,323 @@
package scan
import (
"strings"
"testing"
"github.com/open-code-review/open-code-review/internal/config/template"
"github.com/open-code-review/open-code-review/internal/llmloop"
"github.com/open-code-review/open-code-review/internal/model"
"github.com/open-code-review/open-code-review/internal/session"
"github.com/open-code-review/open-code-review/internal/tool"
)
func newAgentForTest(t *testing.T, tpl template.ScanTemplate) *Agent {
t.Helper()
return NewAgent(Args{
Template: tpl,
CommentCollector: tool.NewCommentCollector(),
Tools: tool.NewRegistry(),
Session: session.New(t.TempDir(), "main", "test-model", session.SessionOptions{
ReviewMode: session.ReviewModeFullScan,
}),
})
}
func makeTemplateWithFullScan() template.ScanTemplate {
return template.ScanTemplate{
MaxTokens: 1000,
MaxToolRequestTimes: 5,
MainTask: template.LlmConversation{
Messages: []template.ChatMessage{
{Role: "system", Content: "scan system rule={{system_rule}}"},
{
Role: "user",
Content: "path={{current_file_path}}\n" +
"date={{current_system_date_time}}\n" +
"siblings=[{{change_files}}]\n" +
"bg={{requirement_background}}\n" +
"plan={{plan_guidance}}\n" +
"<content>\n{{file_content}}\n</content>",
},
},
},
}
}
func TestFormatPlanGuidance_FullJSON(t *testing.T) {
raw := "```json\n" + `{
"summary": "this file orchestrates X.",
"checkpoints": [
{"focus": "race in cache", "lines": "45-78", "why": "writes under read lock"},
{"focus": "error swallowing", "lines": "120-130", "why": "ignored Err return"}
]
}` + "\n```"
got := formatPlanGuidance(raw)
for _, want := range []string{
"**Summary**: this file orchestrates X.",
"1. `race in cache` (lines 45-78) — writes under read lock",
"2. `error swallowing` (lines 120-130) — ignored Err return",
} {
if !strings.Contains(got, want) {
t.Errorf("missing %q\nfull:\n%s", want, got)
}
}
}
func TestFormatPlanGuidance_EmptyAndMalformed(t *testing.T) {
if got := formatPlanGuidance(""); got != "" {
t.Errorf("empty input should yield empty guidance, got %q", got)
}
// Malformed JSON falls back to the raw text so we don't lose what the
// model said — better to feed bad text to the reviewer than nothing.
raw := "the LLM forgot to use JSON: focus on error handling"
if got := formatPlanGuidance(raw); got != raw {
t.Errorf("malformed input should pass through raw, got %q", got)
}
}
func TestFormatPlanGuidance_SummaryOnly(t *testing.T) {
raw := `{"summary": "small helper file", "checkpoints": []}`
got := formatPlanGuidance(raw)
if !strings.Contains(got, "**Summary**: small helper file") {
t.Errorf("missing summary header, got %q", got)
}
if strings.Contains(got, "Focus areas") {
t.Errorf("should not render focus header when no checkpoints, got %q", got)
}
}
// TestPreview_DoesNotMutateAgentItems guards against re-introducing a
// side-effect that pre-populated a.items, which made subsequent Run calls
// on the same Agent silently observe stale state.
func TestPreview_DoesNotMutateAgentItems(t *testing.T) {
repo := initTestRepo(t)
writeFile(t, repo, "a.go", []byte("package a\n"))
writeFile(t, repo, "b.go", []byte("package b\n"))
gitCommit(t, repo, "init")
a := NewAgent(Args{
RepoDir: repo,
GitRunner: nil,
Template: makeTemplateWithFullScan(),
})
if got := a.items; got != nil {
t.Fatalf("pre-Preview items should be nil, got %v", got)
}
if _, err := a.Preview(t.Context()); err != nil {
t.Fatalf("Preview: %v", err)
}
if a.items != nil {
t.Errorf("Preview must not mutate a.items; got %d items", len(a.items))
}
}
// TestPreview_EmptyResultEntriesIsNonNilSlice prevents `"files":null` in
// JSON output when there is nothing reviewable to enumerate.
func TestPreview_EmptyResultEntriesIsNonNilSlice(t *testing.T) {
// Empty repo → empty Entries
repo := initTestRepo(t)
a := NewAgent(Args{
RepoDir: repo,
Template: makeTemplateWithFullScan(),
})
got, err := a.Preview(t.Context())
if err != nil {
t.Fatalf("Preview: %v", err)
}
if got.Entries == nil {
t.Errorf("Entries must be non-nil even when empty (JSON would emit null)")
}
if len(got.Entries) != 0 {
t.Errorf("expected 0 entries, got %d", len(got.Entries))
}
}
func TestBuildSummaryCommentsList_TruncatesAndOneLines(t *testing.T) {
long := strings.Repeat("x", 400)
cs := []model.LlmComment{
{Path: "a.go", Content: "line one\nline two\nline three"},
{Path: "b.go", Content: long},
}
got := buildSummaryCommentsList(cs)
// Newlines in content should be collapsed to spaces.
if strings.Contains(got, "line one\nline two") {
t.Errorf("expected content newlines to be flattened, got:\n%s", got)
}
if !strings.Contains(got, "- `a.go`: line one line two line three") {
t.Errorf("expected path-anchored prefix, got:\n%s", got)
}
// Long content truncated to ~280 + "..." marker.
if !strings.Contains(got, "...") {
t.Errorf("expected truncation marker on long content, got:\n%s", got)
}
for _, line := range strings.Split(got, "\n") {
if len(line) > 320 { // 280 content + small path/prefix overhead
t.Errorf("line not capped: len=%d %q", len(line), line)
}
}
}
func TestMaybeRunPlan_SkipPathsDoNotCallLLM(t *testing.T) {
tpl := makeTemplateWithFullScan()
// no PlanTask attached → must return sentinel without crashing
a := newAgentForTest(t, tpl)
guidance := a.maybeRunPlan(t.Context(), model.ScanItem{Path: "x.go", Content: "package x"}, "rule")
if !strings.Contains(guidance, "no pre-scan plan") {
t.Errorf("expected fallback sentinel, got %q", guidance)
}
// PlanTask attached but SkipPlan set
tpl.PlanTask = &template.LlmConversation{
Messages: []template.ChatMessage{{Role: "user", Content: "plan {{file_content}}"}},
}
a2 := NewAgent(Args{
Template: tpl,
CommentCollector: tool.NewCommentCollector(),
Tools: tool.NewRegistry(),
Session: session.New(t.TempDir(), "main", "test-model", session.SessionOptions{
ReviewMode: session.ReviewModeFullScan,
}),
SkipPlan: true,
})
guidance2 := a2.maybeRunPlan(t.Context(), model.ScanItem{Path: "x.go", Content: "package x"}, "rule")
if !strings.Contains(guidance2, "no pre-scan plan") {
t.Errorf("SkipPlan should suppress plan, got %q", guidance2)
}
}
func TestRenderMessages(t *testing.T) {
tpl := makeTemplateWithFullScan()
a := newAgentForTest(t, tpl)
a.currentDate = "2026-06-09 10:00"
a.args.Background = "ticket-123"
it := model.ScanItem{
Path: "internal/foo/bar.go",
Content: "package foo\n\nfunc Bar() {}\n",
}
msgs := a.renderMessages(it, "rule-text", "(no pre-scan plan; review the entire file as usual)")
if len(msgs) != 2 {
t.Fatalf("expected 2 messages, got %d", len(msgs))
}
sysText := msgs[0].ExtractText()
if !strings.Contains(sysText, "rule=rule-text") {
t.Errorf("system missing system_rule: %q", sysText)
}
userText := msgs[1].ExtractText()
checks := map[string]string{
"path": "path=internal/foo/bar.go",
"date": "date=2026-06-09 10:00",
"siblings": "siblings=[" + changeFilesScanLiteral + "]",
"bg": "bg=ticket-123",
"content": "<content>\npackage foo\n\nfunc Bar() {}\n\n</content>",
}
for label, want := range checks {
if !strings.Contains(userText, want) {
t.Errorf("%s missing %q\nfull: %q", label, want, userText)
}
}
for _, leak := range []string{"{{diff}}", "{{file_content}}", "{{change_files}}", "{{plan_guidance}}"} {
if strings.Contains(userText, leak) {
t.Errorf("placeholder %s leaked into prompt", leak)
}
}
}
func TestFilterLargeScans(t *testing.T) {
tpl := makeTemplateWithFullScan()
tpl.MaxTokens = 40 // threshold = 32
a := newAgentForTest(t, tpl)
short := strings.Repeat("a ", 5)
huge := strings.Repeat("token ", 200)
in := []model.ScanItem{
{Path: "a.go", Content: short},
{Path: "huge.go", Content: huge},
{Path: "b.go", Content: short},
}
out := a.filterLargeScans(in)
if len(out) != 2 {
t.Fatalf("expected 2 kept, got %d", len(out))
}
for _, it := range out {
if it.Path == "huge.go" {
t.Errorf("huge.go should have been filtered")
}
}
}
func TestFilterLargeScans_NoLimit(t *testing.T) {
tpl := makeTemplateWithFullScan()
tpl.MaxTokens = 0
a := newAgentForTest(t, tpl)
in := []model.ScanItem{
{Path: "a.go", Content: "anything"},
{Path: "b.go", Content: strings.Repeat("x ", 1000)},
}
out := a.filterLargeScans(in)
if len(out) != 2 {
t.Errorf("with MaxTokens=0 nothing should be filtered, got %d", len(out))
}
}
func TestInjectScanContentMap(t *testing.T) {
tpl := makeTemplateWithFullScan()
a := newAgentForTest(t, tpl)
a.args.Tools.Register(tool.NewFileReadDiff(tool.DiffMap{}))
a.items = []model.ScanItem{
{Path: "x.go", Content: "package x"},
{Path: "y.go", Content: "package y"},
}
a.injectScanContentMap()
p, ok := a.args.Tools.Get(tool.FileReadDiff.Name())
if !ok {
t.Fatal("file_read_diff not registered")
}
frd := p.(*tool.FileReadDiffProvider)
res, err := frd.Execute(t.Context(), map[string]any{
"path_array": []any{"x.go", "y.go", "missing.go"},
})
if err != nil {
t.Fatalf("Execute: %v", err)
}
if !strings.Contains(res, "package x") || !strings.Contains(res, "package y") {
t.Errorf("missing scan content:\n%s", res)
}
}
func TestNewAgent_SetsSessionMode(t *testing.T) {
a := NewAgent(Args{Template: makeTemplateWithFullScan()})
if a.session.ReviewMode != session.ReviewModeFullScan {
t.Errorf("ReviewMode = %q, want %q", a.session.ReviewMode, session.ReviewModeFullScan)
}
}
func TestRunner_Warnings_RoundTrip(t *testing.T) {
a := newAgentForTest(t, makeTemplateWithFullScan())
a.recordWarning("foo", "x.go", "boom")
ws := a.Warnings()
if len(ws) != 1 || ws[0].Type != "foo" || ws[0].File != "x.go" {
t.Errorf("warnings = %+v", ws)
}
}
// Ensure llmloop.Runner is the underlying source of token counters so the
// public methods on scan.Agent are not stale (preventing accidental refactor
// regressions).
func TestTokenCountersDelegateToRunner(t *testing.T) {
a := newAgentForTest(t, makeTemplateWithFullScan())
if a.TotalInputTokens() != a.runner.TotalInputTokens() ||
a.TotalOutputTokens() != a.runner.TotalOutputTokens() ||
a.TotalCacheReadTokens() != a.runner.TotalCacheReadTokens() ||
a.TotalCacheWriteTokens() != a.runner.TotalCacheWriteTokens() {
t.Fatal("scan.Agent token getters must mirror runner")
}
_ = llmloop.AgentWarning{} // keep llmloop import meaningful
}

116
internal/scan/batch.go Normal file
View file

@ -0,0 +1,116 @@
package scan
import (
"sort"
"strings"
"github.com/open-code-review/open-code-review/internal/model"
)
// BatchStrategy enumerates the grouping policies for scan dispatch.
type BatchStrategy string
const (
// BatchNone treats every file as its own batch (v1 behavior).
BatchNone BatchStrategy = "none"
// BatchByLanguage groups files by extension (case-insensitive).
BatchByLanguage BatchStrategy = "by-language"
// BatchByDirectory groups files by their first-level directory under
// the repo root. Files directly in the root form their own batch.
BatchByDirectory BatchStrategy = "by-directory"
)
// parseBatchStrategy normalizes a user-supplied strategy string. Unknown
// or empty values fall back to BatchNone (safe v1 behavior).
func parseBatchStrategy(s string) BatchStrategy {
switch BatchStrategy(strings.ToLower(strings.TrimSpace(s))) {
case BatchByLanguage:
return BatchByLanguage
case BatchByDirectory:
return BatchByDirectory
default:
return BatchNone
}
}
// groupBatches partitions items according to strategy, then slices each
// natural group into BatchSize-sized chunks (when size > 0). Within a batch
// the input order is preserved; batches themselves are sorted by their
// group key for determinism.
//
// Returns nil when items is empty.
func groupBatches(items []model.ScanItem, strategy BatchStrategy, size int) [][]model.ScanItem {
if len(items) == 0 {
return nil
}
// Bucket by group key.
keyFn := batchKeyFunc(strategy)
buckets := make(map[string][]model.ScanItem)
for _, it := range items {
key := keyFn(it)
buckets[key] = append(buckets[key], it)
}
// Deterministic batch order via sorted keys.
keys := make([]string, 0, len(buckets))
for k := range buckets {
keys = append(keys, k)
}
sort.Strings(keys)
var out [][]model.ScanItem
for _, k := range keys {
group := buckets[k]
if size <= 0 || len(group) <= size {
out = append(out, group)
continue
}
// Chunk the natural group into BatchSize-sized slices.
for start := 0; start < len(group); start += size {
end := start + size
if end > len(group) {
end = len(group)
}
out = append(out, group[start:end])
}
}
return out
}
// batchKeyFunc returns the grouping key extractor for a strategy.
func batchKeyFunc(strategy BatchStrategy) func(model.ScanItem) string {
switch strategy {
case BatchByLanguage:
return languageKey
case BatchByDirectory:
return firstLevelDirKey
default:
// BatchNone: each file is its own batch.
return func(it model.ScanItem) string { return it.Path }
}
}
// languageKey returns the lowercased file extension (with the leading dot)
// or "<no-ext>" for extensionless files.
func languageKey(it model.ScanItem) string {
base := it.Path
if i := strings.LastIndex(base, "/"); i >= 0 {
base = base[i+1:]
}
dot := strings.LastIndex(base, ".")
if dot <= 0 {
return "<no-ext>"
}
return strings.ToLower(base[dot:])
}
// firstLevelDirKey returns the first path segment of a repo-relative path,
// or "<root>" for files directly in the repo root.
func firstLevelDirKey(it model.ScanItem) string {
idx := strings.IndexByte(it.Path, '/')
if idx < 0 {
return "<root>"
}
return it.Path[:idx]
}

135
internal/scan/batch_test.go Normal file
View file

@ -0,0 +1,135 @@
package scan
import (
"reflect"
"testing"
"github.com/open-code-review/open-code-review/internal/model"
)
func itemList(paths ...string) []model.ScanItem {
out := make([]model.ScanItem, len(paths))
for i, p := range paths {
out[i] = model.ScanItem{Path: p}
}
return out
}
func batchPaths(b [][]model.ScanItem) [][]string {
out := make([][]string, len(b))
for i, batch := range b {
ps := make([]string, len(batch))
for j, it := range batch {
ps[j] = it.Path
}
out[i] = ps
}
return out
}
func TestParseBatchStrategy(t *testing.T) {
cases := map[string]BatchStrategy{
"": BatchNone,
" ": BatchNone,
"by-language": BatchByLanguage,
"BY-LANGUAGE": BatchByLanguage,
"by-directory": BatchByDirectory,
"none": BatchNone,
"by-author": BatchNone, // unknown → safe default
" by-language ": BatchByLanguage,
}
for in, want := range cases {
if got := parseBatchStrategy(in); got != want {
t.Errorf("parseBatchStrategy(%q) = %q, want %q", in, got, want)
}
}
}
func TestGroupBatches_ByLanguage(t *testing.T) {
items := itemList(
"cmd/main.go",
"internal/scan/agent.go",
"docs/README.md",
"scripts/build.sh",
"internal/scan/preview.go",
"docs/intro.md",
)
got := batchPaths(groupBatches(items, BatchByLanguage, 0))
// Batches are emitted in lexicographic key order: .go < .md < .sh.
// Within a batch, input order is preserved.
want := [][]string{
{"cmd/main.go", "internal/scan/agent.go", "internal/scan/preview.go"}, // .go
{"docs/README.md", "docs/intro.md"}, // .md
{"scripts/build.sh"}, // .sh
}
if !reflect.DeepEqual(got, want) {
t.Errorf("got %v\nwant %v", got, want)
}
}
func TestGroupBatches_ByDirectory(t *testing.T) {
items := itemList(
"README.md", // <root>
"cmd/main.go", // cmd
"internal/a/x.go", // internal
"internal/b/y.go", // internal
"cmd/scan.go", // cmd
"LICENSE", // <root>
)
got := batchPaths(groupBatches(items, BatchByDirectory, 0))
want := [][]string{
{"README.md", "LICENSE"}, // <root>
{"cmd/main.go", "cmd/scan.go"}, // cmd
{"internal/a/x.go", "internal/b/y.go"}, // internal
}
if !reflect.DeepEqual(got, want) {
t.Errorf("got %v\nwant %v", got, want)
}
}
func TestGroupBatches_None(t *testing.T) {
items := itemList("a.go", "b.go", "c.py")
got := batchPaths(groupBatches(items, BatchNone, 0))
want := [][]string{{"a.go"}, {"b.go"}, {"c.py"}}
if !reflect.DeepEqual(got, want) {
t.Errorf("got %v\nwant %v", got, want)
}
}
func TestGroupBatches_BatchSizeCap(t *testing.T) {
items := itemList("a.go", "b.go", "c.go", "d.go", "e.go")
// All .go in one natural group; size=2 → 3 chunks of 2,2,1
got := batchPaths(groupBatches(items, BatchByLanguage, 2))
want := [][]string{
{"a.go", "b.go"},
{"c.go", "d.go"},
{"e.go"},
}
if !reflect.DeepEqual(got, want) {
t.Errorf("got %v\nwant %v", got, want)
}
}
func TestGroupBatches_Empty(t *testing.T) {
if got := groupBatches(nil, BatchByLanguage, 0); got != nil {
t.Errorf("expected nil for empty input, got %v", got)
}
}
func TestLanguageKey_ExtensionlessAndDotfiles(t *testing.T) {
cases := map[string]string{
"Makefile": "<no-ext>",
"src/Dockerfile": "<no-ext>",
".gitignore": "<no-ext>", // dotfile w/o extension
".github/CODEOWNERS": "<no-ext>",
"cmd/main.go": ".go",
"docs/README.MD": ".md",
"a/b/c.Test.go": ".go",
}
for path, want := range cases {
got := languageKey(model.ScanItem{Path: path})
if got != want {
t.Errorf("languageKey(%q) = %q, want %q", path, got, want)
}
}
}

146
internal/scan/budget_test.go Normal file
View file

@ -0,0 +1,146 @@
package scan
import (
"context"
"sync/atomic"
"testing"
"github.com/open-code-review/open-code-review/internal/config/template"
"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"
"github.com/open-code-review/open-code-review/internal/tool"
)
// fakeBudgetClient returns a task_done tool call on every request and
// reports a fixed token usage, so each file completes in exactly one round
// and consumes a predictable number of tokens. Used to drive the budget
// gate deterministically.
type fakeBudgetClient struct {
perCallTokens int64
calls int64 // atomic
}
func (f *fakeBudgetClient) CompletionsWithCtx(_ context.Context, _ llm.ChatRequest) (*llm.ChatResponse, error) {
atomic.AddInt64(&f.calls, 1)
return &llm.ChatResponse{
Choices: []llm.Choice{{
Message: llm.ResponseMessage{
Role: "assistant",
ToolCalls: []llm.ToolCall{{
ID: "1",
Type: "function",
Function: llm.FunctionCall{Name: "task_done", Arguments: "{}"},
}},
},
FinishReason: "tool_calls",
}},
Usage: &llm.UsageInfo{
PromptTokens: f.perCallTokens,
CompletionTokens: 0,
TotalTokens: f.perCallTokens,
},
}, nil
}
func budgetTestTemplate() template.ScanTemplate {
return template.ScanTemplate{
MaxTokens: 100000,
MaxToolRequestTimes: 5,
MainTask: template.LlmConversation{
Messages: []template.ChatMessage{
{Role: "system", Content: "scan"},
{Role: "user", Content: "review {{file_content}}"},
},
},
}
}
func makeScanItems(n int) []model.ScanItem {
items := make([]model.ScanItem, n)
for i := range items {
items[i] = model.ScanItem{
Path: "f" + string(rune('0'+i)) + ".go",
Content: "package x\n",
LineCount: 1,
}
}
return items
}
// TestBudgetGate_StopsBeforeExceeding verifies the per-file gate stops
// dispatch once the running token total + next-file look-ahead would blow
// the budget — overrun is bounded by at most (concurrency) in-flight files,
// not a whole batch.
func TestBudgetGate_StopsBeforeExceeding(t *testing.T) {
const perCall = 50_000
fake := &fakeBudgetClient{perCallTokens: perCall}
a := NewAgent(Args{
Template: budgetTestTemplate(),
LLMClient: fake,
CommentCollector: tool.NewCommentCollector(),
Tools: tool.NewRegistry(),
MaxConcurrency: 1, // serialize so the gate is deterministic
MaxTokensBudget: 120_000,
Session: session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: session.ReviewModeFullScan}),
SkipPlan: true,
SkipDedup: true,
SkipSummary: true,
})
a.items = makeScanItems(10)
a.args.Tools.Freeze()
if _, err := a.dispatchSubtasks(context.Background()); err != nil {
t.Fatalf("dispatchSubtasks: %v", err)
}
// Budget 120K, each file ~50K actual. estimateFileTokens for a 1-line
// file is dominated by promptOverhead×rounds, so the look-ahead will be
// large; the gate should stop well before all 10 files run.
calls := atomic.LoadInt64(&fake.calls)
if calls == 0 {
t.Fatal("expected at least one file to be dispatched")
}
if calls >= 10 {
t.Errorf("budget gate did not stop dispatch: all %d files ran (budget should have cut it short)", calls)
}
// A token_budget_reached warning must be recorded.
var found bool
for _, w := range a.Warnings() {
if w.Type == "token_budget_reached" {
found = true
break
}
}
if !found {
t.Error("expected a token_budget_reached warning")
}
}
// TestBudgetGate_Unlimited verifies MaxTokensBudget=0 runs every file.
func TestBudgetGate_Unlimited(t *testing.T) {
fake := &fakeBudgetClient{perCallTokens: 50_000}
a := NewAgent(Args{
Template: budgetTestTemplate(),
LLMClient: fake,
CommentCollector: tool.NewCommentCollector(),
Tools: tool.NewRegistry(),
MaxConcurrency: 1,
MaxTokensBudget: 0, // unlimited
Session: session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: session.ReviewModeFullScan}),
SkipPlan: true,
SkipDedup: true,
SkipSummary: true,
})
a.items = makeScanItems(5)
a.args.Tools.Freeze()
if _, err := a.dispatchSubtasks(context.Background()); err != nil {
t.Fatalf("dispatchSubtasks: %v", err)
}
if calls := atomic.LoadInt64(&fake.calls); calls != 5 {
t.Errorf("unlimited budget should run all 5 files, ran %d", calls)
}
}

111
internal/scan/dedup_test.go Normal file
View file

@ -0,0 +1,111 @@
package scan
import (
"strings"
"testing"
"github.com/open-code-review/open-code-review/internal/model"
)
func cmt(path, content string) model.LlmComment {
return model.LlmComment{Path: path, Content: content}
}
func TestApplyDedupGroups_MergeAndKeep(t *testing.T) {
originals := []model.LlmComment{
cmt("a.go", "missing nil check"),
cmt("b.go", "missing nil check"),
cmt("c.go", "race on shared map"),
cmt("d.go", "missing nil check"),
}
raw := `{
"groups": [
{"members": ["c-0", "c-1", "c-3"], "merged_content": "missing nil check (3 files)"},
{"members": ["c-2"]}
]
}`
got, ok := applyDedupGroups(raw, originals)
if !ok {
t.Fatal("expected ok=true")
}
if len(got) != 2 {
t.Fatalf("expected 2 deduped comments, got %d", len(got))
}
if got[0].Path != "a.go" {
t.Errorf("canonical should be members[0] = a.go, got %s", got[0].Path)
}
if got[0].Content != "missing nil check (3 files)" {
t.Errorf("canonical content not merged: %q", got[0].Content)
}
if got[1].Path != "c.go" || got[1].Content != "race on shared map" {
t.Errorf("singleton group should be passed through: %+v", got[1])
}
}
func TestApplyDedupGroups_KeepCanonicalContentWhenNoMergedContent(t *testing.T) {
originals := []model.LlmComment{
cmt("a.go", "original A"),
cmt("b.go", "original B"),
}
// Multi-member but no merged_content → keep members[0]'s original content
raw := `{"groups": [{"members": ["c-0", "c-1"]}]}`
got, ok := applyDedupGroups(raw, originals)
if !ok || len(got) != 1 {
t.Fatalf("unexpected: ok=%v len=%d", ok, len(got))
}
if got[0].Content != "original A" {
t.Errorf("expected canonical's original content, got %q", got[0].Content)
}
}
func TestApplyDedupGroups_RejectsBadShapes(t *testing.T) {
originals := []model.LlmComment{cmt("a.go", "x"), cmt("b.go", "y")}
cases := map[string]string{
"empty input": ``,
"non-json": `not json at all`,
"missing id": `{"groups": [{"members": ["c-0"]}]}`, // c-1 not covered
"duplicate id": `{"groups": [{"members": ["c-0", "c-0"]}, {"members": ["c-1"]}]}`,
"unknown id": `{"groups": [{"members": ["c-0"]}, {"members": ["c-99"]}]}`,
"empty members": `{"groups": [{"members": []}, {"members": ["c-0", "c-1"]}]}`,
"missing one": `{"groups": [{"members": ["c-1"]}]}`, // c-0 missing
}
for name, raw := range cases {
t.Run(name, func(t *testing.T) {
if _, ok := applyDedupGroups(raw, originals); ok {
t.Errorf("expected ok=false for %s, raw=%q", name, raw)
}
})
}
}
func TestApplyDedupGroups_AcceptsMarkdownFences(t *testing.T) {
originals := []model.LlmComment{cmt("a.go", "x"), cmt("b.go", "y")}
raw := "```json\n" + `{"groups":[{"members":["c-0","c-1"],"merged_content":"merged"}]}` + "\n```"
got, ok := applyDedupGroups(raw, originals)
if !ok || len(got) != 1 || got[0].Content != "merged" {
t.Errorf("expected merged single comment, ok=%v got=%+v", ok, got)
}
}
func TestBuildDedupCommentsJSON_IncludesIDsAndKeyFields(t *testing.T) {
cs := []model.LlmComment{
{Path: "a.go", Content: "first", ExistingCode: "x := nil"},
{Path: "b.go", Content: "second"},
}
got := buildDedupCommentsJSON(cs)
for _, want := range []string{
`"id":"c-0"`,
`"id":"c-1"`,
`"path":"a.go"`,
`"content":"first"`,
`"existing_code":"x := nil"`,
} {
if !strings.Contains(got, want) {
t.Errorf("missing %q in payload: %s", want, got)
}
}
// existing_code omitted when empty
if strings.Contains(got, `"existing_code":""`) {
t.Errorf("empty existing_code should be omitted, got: %s", got)
}
}

120
internal/scan/estimate.go Normal file
View file

@ -0,0 +1,120 @@
package scan
import (
"fmt"
"github.com/open-code-review/open-code-review/internal/llm"
"github.com/open-code-review/open-code-review/internal/model"
)
// Cost-estimation heuristics. These are deliberately rough — their job is
// to give the user an order-of-magnitude warning before a large scan, not
// to be billing-accurate. Real usage is always reported from the API
// response after the run.
const (
// promptOverheadTokens approximates the fixed prompt scaffolding per LLM
// call (system prompt + template wrappers + tool definitions).
promptOverheadTokens = 2000
// avgMainRoundsPerFile is the assumed number of MAIN_TASK tool-use
// rounds for a typical file. Observed ~6 on real repos; round up.
avgMainRoundsPerFile = 7
// avgOutputTokensPerRound approximates completion tokens per round.
avgOutputTokensPerRound = 700
)
// Estimate is a pre-run, order-of-magnitude projection of scan cost.
type Estimate struct {
Files int
InputTokens int64
OutputTokens int64
TotalTokens int64
}
// estimateCost projects token usage for reviewing the given items under the
// supplied scan template. planEnabled / dedupEnabled / summaryEnabled
// reflect the effective runtime toggles (template field present AND not
// disabled by a --no-* flag).
// estimateFileTokens projects the input+output token cost of reviewing a
// single file (PLAN_TASK + MAIN_TASK rounds). Excludes the run-level dedup/
// summary phases. Returns 0 for files that are skipped before dispatch
// (binary / empty). Used both by the aggregate estimate and by the
// per-file budget look-ahead in dispatch.
func estimateFileTokens(it model.ScanItem, planEnabled bool) int64 {
if it.IsBinary || it.Content == "" {
return 0
}
fileTokens := int64(llm.CountTokens(it.Content))
var total int64
if planEnabled {
total += fileTokens + promptOverheadTokens // PLAN input
total += 400 // PLAN output (small JSON)
}
// MAIN_TASK: file content carried across rounds + per-round overhead.
total += (fileTokens + promptOverheadTokens) * avgMainRoundsPerFile
total += avgOutputTokensPerRound * avgMainRoundsPerFile
return total
}
func estimateCost(items []model.ScanItem, planEnabled, dedupEnabled, summaryEnabled bool) Estimate {
var est Estimate
var allCommentsApprox int64
for i := range items {
it := &items[i]
if it.IsBinary || it.Content == "" {
continue // skipped before dispatch
}
est.Files++
// Per-file cost folds into InputTokens for the aggregate; we don't
// split input/output here since the look-ahead only needs the total.
// Recompute the input/output split inline to keep the headline
// numbers meaningful.
fileTokens := int64(llm.CountTokens(it.Content))
if planEnabled {
est.InputTokens += fileTokens + promptOverheadTokens
est.OutputTokens += 400
}
est.InputTokens += (fileTokens + promptOverheadTokens) * avgMainRoundsPerFile
est.OutputTokens += avgOutputTokensPerRound * avgMainRoundsPerFile
// Rough comment yield used to size dedup/summary inputs downstream.
allCommentsApprox += 3
}
// DEDUP_TASK: one call per batch; approximate as a single pass over all
// comments (batches partition them, so total dedup input ≈ all comments).
if dedupEnabled && allCommentsApprox > 0 {
est.InputTokens += allCommentsApprox*120 + promptOverheadTokens
est.OutputTokens += allCommentsApprox * 20
}
// PROJECT_SUMMARY_TASK: one call over all comments.
if summaryEnabled && allCommentsApprox > 0 {
est.InputTokens += allCommentsApprox*120 + promptOverheadTokens
est.OutputTokens += 2000
}
est.TotalTokens = est.InputTokens + est.OutputTokens
return est
}
// String renders a one-line human-readable estimate. Money is intentionally
// omitted — pricing varies per provider/model and we don't want to imply a
// precise dollar figure.
func (e Estimate) String() string {
return fmt.Sprintf("~%d file(s), est. %s input + %s output ≈ %s total tokens (rough; actual reported after run)",
e.Files, humanTokens(e.InputTokens), humanTokens(e.OutputTokens), humanTokens(e.TotalTokens))
}
// humanTokens formats a token count as e.g. "1.2M" / "850K" / "420".
func humanTokens(n int64) string {
switch {
case n >= 1_000_000:
return fmt.Sprintf("%.1fM", float64(n)/1_000_000)
case n >= 1_000:
return fmt.Sprintf("%.0fK", float64(n)/1_000)
default:
return fmt.Sprintf("%d", n)
}
}

139
internal/scan/estimate_test.go Normal file
View file

@ -0,0 +1,139 @@
package scan
import (
"strings"
"testing"
"github.com/open-code-review/open-code-review/internal/config/template"
"github.com/open-code-review/open-code-review/internal/model"
)
func TestHumanTokens(t *testing.T) {
cases := map[int64]string{
0: "0",
420: "420",
999: "999",
1000: "1K",
1500: "2K", // rounds
850_000: "850K",
1_000_000: "1.0M",
2_400_000: "2.4M",
}
for in, want := range cases {
if got := humanTokens(in); got != want {
t.Errorf("humanTokens(%d) = %q, want %q", in, got, want)
}
}
}
func TestEstimateCost_ScalesWithContentAndPhases(t *testing.T) {
items := []model.ScanItem{
{Path: "a.go", Content: strings.Repeat("token ", 500)}, // ~non-trivial
{Path: "b.go", Content: strings.Repeat("x ", 300)},
{Path: "bin.dat", IsBinary: true}, // skipped
{Path: "empty.go", Content: ""}, // skipped
}
// Plan off, dedup off, summary off → only MAIN_TASK cost.
base := estimateCost(items, false, false, false)
if base.Files != 2 {
t.Fatalf("expected 2 reviewable files, got %d", base.Files)
}
if base.TotalTokens <= 0 {
t.Fatal("expected positive total")
}
// Turning plan on must increase the estimate.
withPlan := estimateCost(items, true, false, false)
if withPlan.TotalTokens <= base.TotalTokens {
t.Errorf("plan should raise estimate: base=%d withPlan=%d", base.TotalTokens, withPlan.TotalTokens)
}
// Summary + dedup on top must increase further.
full := estimateCost(items, true, true, true)
if full.TotalTokens <= withPlan.TotalTokens {
t.Errorf("dedup+summary should raise estimate: withPlan=%d full=%d", withPlan.TotalTokens, full.TotalTokens)
}
// TotalTokens must equal input + output.
if full.TotalTokens != full.InputTokens+full.OutputTokens {
t.Errorf("total %d != input %d + output %d", full.TotalTokens, full.InputTokens, full.OutputTokens)
}
}
func TestEstimateFileTokens(t *testing.T) {
// Binary / empty → 0 (skipped before dispatch).
if got := estimateFileTokens(model.ScanItem{Path: "x", IsBinary: true}, true); got != 0 {
t.Errorf("binary file should estimate 0, got %d", got)
}
if got := estimateFileTokens(model.ScanItem{Path: "x", Content: ""}, true); got != 0 {
t.Errorf("empty file should estimate 0, got %d", got)
}
it := model.ScanItem{Path: "a.go", Content: strings.Repeat("token ", 400)}
withPlan := estimateFileTokens(it, true)
noPlan := estimateFileTokens(it, false)
if withPlan <= 0 || noPlan <= 0 {
t.Fatalf("expected positive estimates, got plan=%d noplan=%d", withPlan, noPlan)
}
if withPlan <= noPlan {
t.Errorf("plan-enabled estimate (%d) should exceed plan-disabled (%d)", withPlan, noPlan)
}
// Per-file estimate must equal the aggregate single-file MAIN+PLAN cost
// (sanity that the aggregate and look-ahead share the same model).
agg := estimateCost([]model.ScanItem{it}, true, false, false)
if agg.TotalTokens != withPlan {
t.Errorf("aggregate single-file total (%d) != per-file estimate (%d)", agg.TotalTokens, withPlan)
}
}
func TestEstimateCost_EmptyItems(t *testing.T) {
est := estimateCost(nil, true, true, true)
if est.Files != 0 || est.TotalTokens != 0 {
t.Errorf("empty input should yield zero estimate, got %+v", est)
}
}
func TestEstimate_StringMentionsTokens(t *testing.T) {
est := Estimate{Files: 3, InputTokens: 1_200_000, OutputTokens: 90_000, TotalTokens: 1_290_000}
s := est.String()
for _, want := range []string{"3 file", "1.2M", "90K", "1.3M"} {
if !strings.Contains(s, want) {
t.Errorf("String() missing %q: %s", want, s)
}
}
}
func TestPhaseEnabled_GatedByTemplateAndFlag(t *testing.T) {
tpl := makeTemplateWithFullScan()
tpl.PlanTask = &template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "plan {{file_content}}"}}}
tpl.DedupTask = &template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "dedup {{batch_comments}}"}}}
tpl.ProjectSummaryTask = &template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "summary {{all_comments}}"}}}
a := newAgentForTest(t, tpl)
if !a.planEnabled() {
t.Error("planEnabled should be true when template has PlanTask and SkipPlan is false")
}
if !a.dedupEnabled() {
t.Error("dedupEnabled should be true")
}
if !a.summaryEnabled() {
t.Error("summaryEnabled should be true")
}
// Flags disable each phase independently.
a.args.SkipPlan = true
a.args.SkipDedup = true
a.args.SkipSummary = true
if a.planEnabled() || a.dedupEnabled() || a.summaryEnabled() {
t.Error("--no-* flags must disable the corresponding phase")
}
// Nil template field disables regardless of flag.
a2 := newAgentForTest(t, makeTemplateWithFullScan())
a2.args.Template.DedupTask = nil
if a2.dedupEnabled() {
t.Error("nil DedupTask must disable dedup")
}
}

54
internal/scan/preview.go Normal file
View file

@ -0,0 +1,54 @@
package scan
import (
"context"
"fmt"
"github.com/open-code-review/open-code-review/internal/model"
)
// Preview enumerates files and applies the standard reviewability filter
// without dispatching any LLM calls. Returns a *model.Preview ready for
// cmd/opencodereview.outputPreviewText to render.
//
// Preview is read-only with respect to the Agent: it does not mutate
// a.items. (Earlier versions did, which made a subsequent Run on the same
// Agent silently observe the preview's enumeration instead of re-running
// it.) Callers that want to reuse the enumeration should call Run once.
func (a *Agent) Preview(ctx context.Context) (*model.Preview, error) {
provider := NewProvider(a.args.RepoDir, a.args.Paths, a.args.GitRunner, a.args.MaxFileSizeBytes)
items, err := provider.Enumerate(ctx)
if err != nil {
return nil, fmt.Errorf("enumerate files: %w", err)
}
// Pre-allocate Entries to a non-nil empty slice so JSON marshalling
// emits `"files":[]` rather than `"files":null` when there is nothing
// to review — important for downstream API consumers expecting an array.
result := &model.Preview{
TotalFiles: len(items),
Entries: make([]model.PreviewEntry, 0, len(items)),
}
for _, it := range items {
if err := ctx.Err(); err != nil {
return nil, err
}
entry := model.PreviewEntry{
Path: it.Path,
Status: "scan",
Insertions: int64(it.LineCount),
}
reason := a.whyExcluded(it)
entry.WillReview = reason == model.ExcludeNone
entry.ExcludeReason = reason
if entry.WillReview {
result.ReviewableCount++
result.TotalInsertions += entry.Insertions
} else {
result.ExcludedCount++
}
result.Entries = append(result.Entries, entry)
}
return result, nil
}

313
internal/scan/provider.go Normal file
View file

@ -0,0 +1,313 @@
// Package scan implements `ocr scan` — full-file code review. It owns the
// file-enumeration provider, the per-file orchestrator, and the FULL_SCAN
// prompt-template plumbing. Shared LLM tool-use loop / memory compression
// lives in internal/llmloop; this package only handles scan-specific
// concerns (enumeration, FULL_SCAN_TASK rendering, scan-specific filter).
package scan
import (
"bytes"
"context"
"errors"
"fmt"
"io"
"os"
"os/exec"
"path/filepath"
"strings"
"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/model"
)
// binarySniffWindow is the number of leading bytes inspected to decide
// whether a file is binary. Matches common heuristics (git, less).
const binarySniffWindow = 8000
// DefaultMaxFileSizeBytes is the default hard cap on how large a single
// file may be before the scanner skips it. The real review-feasibility
// limit is the per-file token budget (filterLargeScans, ~188 KB at
// MaxTokens=58888) — this byte cap exists only to stop us from reading
// multi-MB dumps into memory. Callers can override via NewProvider.
const DefaultMaxFileSizeBytes int64 = 2 << 20 // 2 MiB
// Provider enumerates source files in a repository for full-file review.
// Unlike diff.Provider it produces no unified diffs — each ScanItem carries
// the full file content via Content, and binaries are emitted as placeholder
// entries (Content empty, IsBinary=true) so callers can still surface them
// in previews without spending memory on their bytes.
type Provider struct {
repoDir string
paths []string // empty = whole repo
runner *gitcmd.Runner
maxFileSizeBytes int64
}
// NewProvider creates a Provider that enumerates the repository at repoDir.
// If paths is non-empty each element must be a repo-relative path (file or
// directory); only matching files are returned. maxFileSizeBytes <= 0 falls
// back to DefaultMaxFileSizeBytes.
func NewProvider(repoDir string, paths []string, runner *gitcmd.Runner, maxFileSizeBytes int64) *Provider {
cleaned := make([]string, 0, len(paths))
for _, p := range paths {
p = strings.TrimSpace(p)
if p == "" {
continue
}
// Normalize: strip leading "./" and trailing "/" so prefix matching
// against `git ls-files` output (which never has leading "./") works.
p = strings.TrimPrefix(p, "./")
p = strings.TrimSuffix(p, "/")
cleaned = append(cleaned, filepath.ToSlash(p))
}
if maxFileSizeBytes <= 0 {
maxFileSizeBytes = DefaultMaxFileSizeBytes
}
return &Provider{
repoDir: repoDir,
paths: cleaned,
runner: runner,
maxFileSizeBytes: maxFileSizeBytes,
}
}
// Enumerate returns one ScanItem per reviewable file. Binaries are emitted
// with empty Content + IsBinary=true so previews can show them as excluded.
func (p *Provider) Enumerate(ctx context.Context) ([]model.ScanItem, error) {
files, err := p.listFiles(ctx)
if err != nil {
return nil, err
}
if len(p.paths) > 0 {
files = filterByPaths(files, p.paths)
}
gitignorePatterns := diff.LoadGitignorePatterns(p.repoDir)
var out []model.ScanItem
for _, rel := range files {
// Per-iteration cancellation check: a large repo with thousands of
// files may take seconds to walk, and downstream Lstat / ReadFile
// each cost a syscall — abort early when ctx is cancelled.
if err := ctx.Err(); err != nil {
return nil, err
}
if rel == "" {
continue
}
if diff.IsPathExcluded(p.repoDir, rel, gitignorePatterns) {
continue
}
full := filepath.Join(p.repoDir, rel)
info, err := os.Lstat(full)
if err != nil {
fmt.Fprintf(os.Stderr, "[ocr] WARNING: cannot stat %s: %v\n", rel, err)
continue
}
if !info.Mode().IsRegular() {
continue
}
if info.Size() > p.maxFileSizeBytes {
fmt.Fprintf(os.Stderr, "[ocr] WARNING: skipping %s (%d bytes exceeds %d-byte scan limit; raise MaxTokens if the real concern is token budget, not memory)\n",
rel, info.Size(), p.maxFileSizeBytes)
continue
}
binary, err := isBinaryFile(full)
if err != nil {
fmt.Fprintf(os.Stderr, "[ocr] WARNING: cannot sniff %s: %v\n", rel, err)
continue
}
if binary {
// Emit placeholder so preview can display [B], but do not
// read the file body — saves memory on large binaries.
out = append(out, model.ScanItem{
Path: rel,
IsBinary: true,
})
continue
}
content, err := os.ReadFile(full)
if err != nil {
fmt.Fprintf(os.Stderr, "[ocr] WARNING: cannot read %s: %v\n", rel, err)
continue
}
out = append(out, model.ScanItem{
Path: rel,
Content: string(content),
IsBinary: false,
LineCount: countLines(content),
})
}
return out, nil
}
// listFiles returns all source files under repoDir. In a git repo it uses
// `git ls-files` for full .gitignore semantics (nested + global excludes +
// negation rules). In a non-git directory it falls back to filepath.WalkDir
// with the simpler in-process gitignore handling (root .gitignore + the
// internal ExcludedDirs blocklist).
func (p *Provider) listFiles(ctx context.Context) ([]string, error) {
if p.isGitRepo(ctx) {
return p.listFilesViaGit(ctx)
}
return p.listFilesViaWalk(ctx)
}
// isGitRepo reports whether p.repoDir is inside a git working tree.
func (p *Provider) isGitRepo(ctx context.Context) bool {
cmd := exec.CommandContext(ctx, "git", "-C", p.repoDir, "rev-parse", "--git-dir")
return cmd.Run() == nil
}
func (p *Provider) listFilesViaGit(ctx context.Context) ([]string, error) {
tracked, err := p.gitLs(ctx, "-z")
if err != nil {
return nil, fmt.Errorf("git ls-files (tracked): %w", err)
}
untracked, err := p.gitLs(ctx, "-z", "--others", "--exclude-standard")
if err != nil {
return nil, fmt.Errorf("git ls-files (untracked): %w", err)
}
seen := make(map[string]struct{}, len(tracked)+len(untracked))
all := make([]string, 0, len(tracked)+len(untracked))
for _, f := range append(tracked, untracked...) {
if f == "" {
continue
}
if _, ok := seen[f]; ok {
continue
}
seen[f] = struct{}{}
all = append(all, f)
}
return all, nil
}
// listFilesViaWalk recursively walks p.repoDir collecting regular files.
// Honors:
// - the internal ExcludedDirs blocklist (.git, node_modules, vendor, ...)
// - the root .gitignore (simplified semantics; nested .gitignore is NOT
// supported in this mode)
//
// Skips entire subtrees via filepath.SkipDir for performance.
func (p *Provider) listFilesViaWalk(ctx context.Context) ([]string, error) {
gitignorePatterns := diff.LoadGitignorePatterns(p.repoDir)
var files []string
err := filepath.WalkDir(p.repoDir, func(path string, d os.DirEntry, err error) error {
if cerr := ctx.Err(); cerr != nil {
// Abort the walk; filepath.WalkDir propagates this back as the
// returned error so the caller sees ctx.Err().
return cerr
}
if err != nil {
fmt.Fprintf(os.Stderr, "[ocr] WARNING: walk error at %s: %v\n", path, err)
return nil // continue walking; skip this entry
}
if path == p.repoDir {
return nil
}
rel, relErr := filepath.Rel(p.repoDir, path)
if relErr != nil {
return nil
}
rel = filepath.ToSlash(rel)
if d.IsDir() {
// Skip the whole subtree if the dir itself is excluded.
if diff.IsPathExcluded(p.repoDir, rel, gitignorePatterns) {
return filepath.SkipDir
}
return nil
}
// Regular files only; skip symlinks / sockets / etc.
if !d.Type().IsRegular() {
return nil
}
if diff.IsPathExcluded(p.repoDir, rel, gitignorePatterns) {
return nil
}
files = append(files, rel)
return nil
})
if err != nil {
return nil, fmt.Errorf("walk %s: %w", p.repoDir, err)
}
return files, nil
}
func (p *Provider) gitLs(ctx context.Context, args ...string) ([]string, error) {
cmdArgs := append([]string{"-c", "core.quotepath=false", "ls-files"}, args...)
var out string
var err error
if p.runner != nil {
out, err = p.runner.Run(ctx, p.repoDir, cmdArgs...)
} else {
cmd := exec.CommandContext(ctx, "git", cmdArgs...)
cmd.Dir = p.repoDir
// Use Output (stdout only), not CombinedOutput: with -z, git emits
// NUL-delimited paths on stdout, and merging stderr in would corrupt
// the filename parsing below.
raw, runErr := cmd.Output()
out, err = string(raw), runErr
}
if err != nil {
return nil, err
}
raw := strings.Split(strings.TrimRight(out, "\x00"), "\x00")
files := make([]string, 0, len(raw))
for _, f := range raw {
f = strings.TrimSpace(f)
if f != "" {
files = append(files, f)
}
}
return files, nil
}
// filterByPaths keeps only entries whose path equals a user-supplied path
// (for exact files) or lies under it (for directories).
func filterByPaths(all []string, paths []string) []string {
var out []string
for _, f := range all {
for _, want := range paths {
if f == want || strings.HasPrefix(f, want+"/") {
out = append(out, f)
break
}
}
}
return out
}
// countLines returns the number of lines in content. A file without a
// trailing newline still counts its final line. Empty input → 0.
func countLines(content []byte) int {
if len(content) == 0 {
return 0
}
n := bytes.Count(content, []byte{'\n'})
if content[len(content)-1] != '\n' {
n++
}
return n
}
// isBinaryFile reads up to binarySniffWindow bytes from path and reports
// whether they contain a NUL byte (git's "binary" heuristic).
func isBinaryFile(path string) (bool, error) {
f, err := os.Open(path)
if err != nil {
return false, err
}
defer f.Close()
buf := make([]byte, binarySniffWindow)
n, err := io.ReadFull(f, buf)
if err != nil && !errors.Is(err, io.EOF) && !errors.Is(err, io.ErrUnexpectedEOF) {
return false, err
}
return bytes.IndexByte(buf[:n], 0) >= 0, nil
}

241
internal/scan/provider_test.go Normal file
View file

@ -0,0 +1,241 @@
package scan
import (
"context"
"errors"
"os"
"os/exec"
"path/filepath"
"reflect"
"sort"
"strings"
"testing"
)
func TestCountLines(t *testing.T) {
tests := []struct {
name string
in []byte
want int
}{
{"empty", []byte(""), 0},
{"single line no newline", []byte("foo"), 1},
{"single line trailing newline", []byte("foo\n"), 1},
{"two lines no trailing newline", []byte("foo\nbar"), 2},
{"two lines trailing newline", []byte("foo\nbar\n"), 2},
{"only newline", []byte("\n"), 1},
{"three lines mixed", []byte("a\n\nb"), 3},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := countLines(tt.in); got != tt.want {
t.Errorf("countLines(%q) = %d, want %d", tt.in, got, tt.want)
}
})
}
}
func TestFilterByPaths(t *testing.T) {
all := []string{
"cmd/main.go",
"internal/agent/agent.go",
"internal/agent/fullscan.go",
"internal/scan/provider.go",
"README.md",
}
tests := []struct {
name string
paths []string
want []string
}{
{"exact file", []string{"README.md"}, []string{"README.md"}},
{"dir prefix", []string{"internal/agent"}, []string{"internal/agent/agent.go", "internal/agent/fullscan.go"}},
{"multi", []string{"cmd/main.go", "internal/scan"}, []string{"cmd/main.go", "internal/scan/provider.go"}},
{"prefix not at boundary", []string{"internal/age"}, nil},
{"no match", []string{"does/not/exist"}, nil},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := filterByPaths(all, tt.paths)
if !reflect.DeepEqual(got, tt.want) {
t.Errorf("filterByPaths() = %v, want %v", got, tt.want)
}
})
}
}
func TestNewProvider_NormalizesPaths(t *testing.T) {
p := NewProvider("/tmp/repo", []string{
" ",
"./internal/agent/",
"cmd",
" internal/diff ",
filepath.FromSlash("a/b"),
}, nil, 0)
want := []string{"internal/agent", "cmd", "internal/diff", "a/b"}
if !reflect.DeepEqual(p.paths, want) {
t.Errorf("paths = %v, want %v", p.paths, want)
}
}
func initTestRepo(t *testing.T) string {
t.Helper()
repo := t.TempDir()
run := func(args ...string) {
cmd := exec.Command("git", args...)
cmd.Dir = repo
cmd.Env = append(os.Environ(),
"GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@example.com",
"GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@example.com",
)
if out, err := cmd.CombinedOutput(); err != nil {
t.Fatalf("git %v: %v\n%s", args, err, out)
}
}
run("init", "-b", "main")
run("config", "user.email", "test@example.com")
run("config", "user.name", "test")
run("config", "commit.gpgsign", "false")
return repo
}
func writeFile(t *testing.T, root, rel string, content []byte) {
t.Helper()
full := filepath.Join(root, rel)
if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil {
t.Fatalf("mkdir: %v", err)
}
if err := os.WriteFile(full, content, 0o644); err != nil {
t.Fatalf("write %s: %v", rel, err)
}
}
func gitCommit(t *testing.T, repo, msg string) {
t.Helper()
for _, args := range [][]string{{"add", "-A"}, {"commit", "-m", msg}} {
cmd := exec.Command("git", args...)
cmd.Dir = repo
cmd.Env = append(os.Environ(),
"GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@example.com",
"GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@example.com",
)
if out, err := cmd.CombinedOutput(); err != nil {
t.Fatalf("git %v: %v\n%s", args, err, out)
}
}
}
func TestProvider_Enumerate_FullRepo(t *testing.T) {
repo := initTestRepo(t)
writeFile(t, repo, "main.go", []byte("package main\n\nfunc main() {}\n"))
writeFile(t, repo, "pkg/util.go", []byte("package pkg\n"))
writeFile(t, repo, "image.bin", []byte{0x00, 0x01, 0x02})
writeFile(t, repo, ".gitignore", []byte("ignored.txt\n"))
writeFile(t, repo, "ignored.txt", []byte("should not appear\n"))
gitCommit(t, repo, "init")
got, err := NewProvider(repo, nil, nil, 0).Enumerate(context.Background())
if err != nil {
t.Fatalf("Enumerate: %v", err)
}
paths := make([]string, 0, len(got))
for _, it := range got {
paths = append(paths, it.Path)
}
sort.Strings(paths)
want := []string{".gitignore", "image.bin", "main.go", "pkg/util.go"}
if !reflect.DeepEqual(paths, want) {
t.Errorf("paths = %v, want %v", paths, want)
}
for _, it := range got {
switch it.Path {
case "main.go":
if it.IsBinary {
t.Errorf("main.go must not be binary")
}
if !strings.Contains(it.Content, "package main") {
t.Errorf("main.go content unexpected: %q", it.Content)
}
if it.LineCount != 3 {
t.Errorf("main.go LineCount = %d, want 3", it.LineCount)
}
case "image.bin":
if !it.IsBinary {
t.Errorf("image.bin must be binary")
}
if it.Content != "" {
t.Errorf("binary item must not store content, got %d bytes", len(it.Content))
}
}
}
}
func TestProvider_Enumerate_NonGitDirectory(t *testing.T) {
// Plain temp dir — no `git init`. Walker fallback should kick in.
repo := t.TempDir()
writeFile(t, repo, "main.go", []byte("package main\n"))
writeFile(t, repo, "pkg/util.go", []byte("package pkg\n"))
writeFile(t, repo, ".gitignore", []byte("ignored.txt\n"))
writeFile(t, repo, "ignored.txt", []byte("should be excluded by root .gitignore\n"))
writeFile(t, repo, "node_modules/lib/foo.js", []byte("module.exports = 1;\n")) // should be skipped via ExcludedDirs
got, err := NewProvider(repo, nil, nil, 0).Enumerate(context.Background())
if err != nil {
t.Fatalf("Enumerate (non-git): %v", err)
}
paths := make([]string, 0, len(got))
for _, it := range got {
paths = append(paths, it.Path)
}
sort.Strings(paths)
want := []string{".gitignore", "main.go", "pkg/util.go"}
if !reflect.DeepEqual(paths, want) {
t.Errorf("paths = %v, want %v (ignored.txt must be filtered by .gitignore, node_modules/* by ExcludedDirs)", paths, want)
}
}
// TestProvider_Enumerate_RespectsContextCancellation guards the
// per-iteration ctx.Err() check that was previously missing.
func TestProvider_Enumerate_RespectsContextCancellation(t *testing.T) {
repo := initTestRepo(t)
for i := 0; i < 30; i++ {
writeFile(t, repo, "pkg/"+strings.Repeat("a", i+1)+".go", []byte("package pkg\n"))
}
gitCommit(t, repo, "init")
ctx, cancel := context.WithCancel(context.Background())
cancel() // pre-cancelled
_, err := NewProvider(repo, nil, nil, 0).Enumerate(ctx)
if err == nil {
t.Fatal("expected ctx-cancelled error, got nil")
}
if !errors.Is(err, context.Canceled) {
t.Errorf("expected context.Canceled, got %v", err)
}
}
func TestProvider_Enumerate_PathFilter(t *testing.T) {
repo := initTestRepo(t)
writeFile(t, repo, "a.go", []byte("package a\n"))
writeFile(t, repo, "pkg/b.go", []byte("package pkg\n"))
writeFile(t, repo, "pkg/sub/c.go", []byte("package sub\n"))
gitCommit(t, repo, "init")
got, err := NewProvider(repo, []string{"pkg"}, nil, 0).Enumerate(context.Background())
if err != nil {
t.Fatalf("Enumerate: %v", err)
}
paths := make([]string, 0, len(got))
for _, it := range got {
paths = append(paths, it.Path)
}
sort.Strings(paths)
want := []string{"pkg/b.go", "pkg/sub/c.go"}
if !reflect.DeepEqual(paths, want) {
t.Errorf("paths = %v, want %v", paths, want)
}
}

1
internal/session/history.go
View file

@ -27,6 +27,7 @@ const (
ReviewModeWorkspace = "workspace"
ReviewModeRange = "range"
ReviewModeCommit = "commit"
ReviewModeFullScan = "full_scan"
)
// SessionHistory is the top-level container for an entire CR run.

19
internal/tool/code_search.go
View file

@ -49,9 +49,15 @@ func (p *CodeSearchProvider) Execute(ctx context.Context, args map[string]any) (
return result, nil
}
func (p *CodeSearchProvider) buildGrepArgs(searchText string, caseSensitive bool, usePerlRegexp bool, pathspec []string) []string {
func (p *CodeSearchProvider) buildGrepArgs(searchText string, caseSensitive bool, usePerlRegexp bool, noIndex bool, pathspec []string) []string {
cmdArgs := []string{"--no-pager", "grep"}
if noIndex {
// Non-git directory: search the working tree directly while still
// honoring .gitignore and skipping .git (via --exclude-standard).
cmdArgs = append(cmdArgs, "--no-index", "--exclude-standard")
}
if !caseSensitive {
cmdArgs = append(cmdArgs, "-i")
}
@ -104,10 +110,19 @@ func (p *CodeSearchProvider) runGitGrep(parentCtx context.Context, cmdArgs []str
}
func (p *CodeSearchProvider) gitGrep(ctx context.Context, searchText string, caseSensitive bool, usePerlRegexp bool, pathspec []string) (string, error) {
cmdArgs := p.buildGrepArgs(searchText, caseSensitive, usePerlRegexp, pathspec)
cmdArgs := p.buildGrepArgs(searchText, caseSensitive, usePerlRegexp, false, pathspec)
outStr, errStr, err := p.runGitGrep(ctx, cmdArgs)
// Non-git directory: `git grep` exits 128 with "not a git repository".
// `ocr scan` supports plain directories, so retry in --no-index mode, which
// searches the working tree directly while still honoring .gitignore.
// Ref-based search needs a real repo, so it is not retried.
if err != nil && p.FileReader.Ref == "" && strings.Contains(errStr, "not a git repository") {
cmdArgs = p.buildGrepArgs(searchText, caseSensitive, usePerlRegexp, true, pathspec)
outStr, errStr, err = p.runGitGrep(ctx, cmdArgs)
}
if err != nil {
if errors.Is(err, context.DeadlineExceeded) {
return "code_search timed out. Try narrowing file_patterns to a more specific path.", nil

68
internal/tool/code_search_test.go
View file

@ -12,7 +12,7 @@ import (
func TestBuildGrepArgs_WorkspaceMode(t *testing.T) {
p := NewCodeSearch(&FileReader{RepoDir: "/tmp", Ref: ""})
args := p.buildGrepArgs("myFunc", false, false, nil)
args := p.buildGrepArgs("myFunc", false, false, false, nil)
assertContainsInOrder(t, args, "-e", "myFunc", "--")
assertContains(t, args, "-i")
@ -27,21 +27,21 @@ func TestBuildGrepArgs_WorkspaceMode(t *testing.T) {
func TestBuildGrepArgs_CommitMode(t *testing.T) {
p := NewCodeSearch(&FileReader{RepoDir: "/tmp", Ref: "abc1234"})
args := p.buildGrepArgs("myFunc", false, false, []string{"pkg/"})
args := p.buildGrepArgs("myFunc", false, false, false, []string{"pkg/"})
assertContainsInOrder(t, args, "-e", "myFunc", "--end-of-options", "abc1234", "--", "pkg/")
}
func TestBuildGrepArgs_RefUsesEndOfOptions(t *testing.T) {
p := NewCodeSearch(&FileReader{RepoDir: "/tmp", Ref: "-O./pwn.sh"})
args := p.buildGrepArgs("myFunc", false, false, nil)
args := p.buildGrepArgs("myFunc", false, false, false, nil)
assertContainsInOrder(t, args, "-e", "myFunc", "--end-of-options", "-O./pwn.sh", "--")
}
func TestBuildGrepArgs_PatternStartingWithDash(t *testing.T) {
p := NewCodeSearch(&FileReader{RepoDir: "/tmp", Ref: ""})
args := p.buildGrepArgs("-myOption", false, false, nil)
args := p.buildGrepArgs("-myOption", false, false, false, nil)
idx := slices.Index(args, "-e")
if idx < 0 || idx+1 >= len(args) || args[idx+1] != "-myOption" {
@ -51,21 +51,21 @@ func TestBuildGrepArgs_PatternStartingWithDash(t *testing.T) {
func TestBuildGrepArgs_CaseSensitive(t *testing.T) {
p := NewCodeSearch(&FileReader{RepoDir: "/tmp", Ref: ""})
args := p.buildGrepArgs("foo", true, false, nil)
args := p.buildGrepArgs("foo", true, false, false, nil)
assertNotContains(t, args, "-i")
}
func TestBuildGrepArgs_CaseInsensitive(t *testing.T) {
p := NewCodeSearch(&FileReader{RepoDir: "/tmp", Ref: ""})
args := p.buildGrepArgs("foo", false, false, nil)
args := p.buildGrepArgs("foo", false, false, false, nil)
assertContains(t, args, "-i")
}
func TestBuildGrepArgs_PerlRegexp(t *testing.T) {
p := NewCodeSearch(&FileReader{RepoDir: "/tmp", Ref: ""})
args := p.buildGrepArgs("foo", false, true, nil)
args := p.buildGrepArgs("foo", false, true, false, nil)
assertContains(t, args, "-P")
assertNotContains(t, args, "-F")
@ -73,7 +73,7 @@ func TestBuildGrepArgs_PerlRegexp(t *testing.T) {
func TestBuildGrepArgs_FixedString(t *testing.T) {
p := NewCodeSearch(&FileReader{RepoDir: "/tmp", Ref: ""})
args := p.buildGrepArgs("foo", false, false, nil)
args := p.buildGrepArgs("foo", false, false, false, nil)
assertContains(t, args, "-F")
assertNotContains(t, args, "-E")
@ -306,3 +306,55 @@ func assertContainsInOrder(t *testing.T, args []string, vals ...string) {
t.Errorf("expected args to contain %v in order, got %v (matched up to index %d)", vals, args, idx)
}
}
// TestGitGrep_NonGitDirectoryFallback verifies code_search works in a plain
// (non-git) directory by retrying git grep in --no-index mode instead of
// failing with git's exit 128, while still honoring .gitignore.
func TestGitGrep_NonGitDirectoryFallback(t *testing.T) {
dir := t.TempDir() // plain dir, no `git init`
write := func(rel, content string) {
full := filepath.Join(dir, rel)
if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(full, []byte(content), 0o644); err != nil {
t.Fatal(err)
}
}
write("server.go", "package main\n\nfunc Handler() {}\n")
write("internal/svc.go", "package internal\n\nfunc Handler() {}\n")
write(".gitignore", "node_modules/\n")
write("node_modules/lib.js", "function Handler() {}\n") // excluded by .gitignore
p := NewCodeSearch(&FileReader{RepoDir: dir, Ref: "", Mode: ModeWorkspace})
out, err := p.gitGrep(context.Background(), "Handler", false, false, nil)
if err != nil {
t.Fatalf("gitGrep should not error in a non-git dir, got: %v", err)
}
if !strings.Contains(out, "server.go") || !strings.Contains(out, "internal/svc.go") {
t.Errorf("expected matches in tracked-like files, got:\n%s", out)
}
if strings.Contains(out, "node_modules") {
t.Errorf("node_modules should be excluded via --exclude-standard, got:\n%s", out)
}
}
// TestGitGrep_NonGitDirectoryNoMatch verifies the no-match path in a non-git
// dir returns the sentinel rather than an error.
func TestGitGrep_NonGitDirectoryNoMatch(t *testing.T) {
dir := t.TempDir()
if err := os.WriteFile(filepath.Join(dir, "a.go"), []byte("package a\n"), 0o644); err != nil {
t.Fatal(err)
}
p := NewCodeSearch(&FileReader{RepoDir: dir, Ref: "", Mode: ModeWorkspace})
out, err := p.gitGrep(context.Background(), "nonexistentXYZ", false, false, nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if out != "No matches found" {
t.Errorf("expected 'No matches found', got: %q", out)
}
}

41
internal/tool/comment_collector.go
View file

@ -47,6 +47,47 @@ func (c *CommentCollector) CommentsForPath(path string) []model.LlmComment {
return out
}
// Snapshot returns the current count of stored comments. Pair with Since /
// ReplaceSince to operate on the comments added between two points in time
// (e.g. before / after a scan batch).
func (c *CommentCollector) Snapshot() int {
c.mu.Lock()
defer c.mu.Unlock()
return len(c.comments)
}
// Since returns a copy of all comments stored at index ≥ start. Returns nil
// when no new comments have been added since the snapshot.
func (c *CommentCollector) Since(start int) []model.LlmComment {
c.mu.Lock()
defer c.mu.Unlock()
if start < 0 {
start = 0
}
if start >= len(c.comments) {
return nil
}
out := make([]model.LlmComment, len(c.comments)-start)
copy(out, c.comments[start:])
return out
}
// ReplaceSince replaces comments[start:] with the given replacements.
// Useful for batch-level dedup: take a Snapshot, run a batch, dedup the
// new comments, then apply the deduped list back. Indices ≥ len(comments)
// are ignored (no-op).
func (c *CommentCollector) ReplaceSince(start int, replacements []model.LlmComment) {
c.mu.Lock()
defer c.mu.Unlock()
if start < 0 {
start = 0
}
if start > len(c.comments) {
return
}
c.comments = append(c.comments[:start:start], replacements...)
}
// RemoveByPathAndIndices removes comments for a given path whose per-path index
// (0-based position among all comments with that path) is in the indices set.
func (c *CommentCollector) RemoveByPathAndIndices(path string, indices map[int]struct{}) {

60
internal/tool/file_find.go
View file

@ -3,9 +3,13 @@ package tool
import (
"bytes"
"context"
"io/fs"
"os/exec"
"path/filepath"
"strings"
"time"
"github.com/open-code-review/open-code-review/internal/diff"
)
const (
@ -89,6 +93,13 @@ func (p *FileFindProvider) listGitFiles(parentCtx context.Context) ([]string, er
if ctx.Err() != nil {
return nil, ctx.Err()
}
// Non-git directory (git ls-files exits 128) and no specific ref:
// fall back to a filesystem walk so file_find still works for
// `ocr scan` on plain directories. Ref-based lookups can't be
// satisfied without git, so those still error.
if p.FileReader.Ref == "" {
return p.listWalkFiles(ctx)
}
return nil, err
}
@ -108,6 +119,55 @@ func (p *FileFindProvider) listGitFiles(parentCtx context.Context) ([]string, er
return files, nil
}
// listWalkFiles is the non-git fallback for listGitFiles: it walks the repo
// directory honoring the root .gitignore and the default excluded-dir
// blocklist (.git, node_modules, vendor, ...). Used when `git ls-files`
// fails because the directory is not a git repository.
func (p *FileFindProvider) listWalkFiles(ctx context.Context) ([]string, error) {
root := p.FileReader.RepoDir
gitignorePatterns := diff.LoadGitignorePatterns(root)
var files []string
err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error {
if cerr := ctx.Err(); cerr != nil {
return cerr
}
if err != nil {
return nil // skip unreadable entries
}
if path == root {
return nil
}
rel, relErr := filepath.Rel(root, path)
if relErr != nil {
return nil
}
rel = filepath.ToSlash(rel)
if d.IsDir() {
if diff.IsPathExcluded(root, rel, gitignorePatterns) {
return filepath.SkipDir
}
return nil
}
if !d.Type().IsRegular() {
return nil
}
if diff.IsPathExcluded(root, rel, gitignorePatterns) {
return nil
}
if shouldSkipFile(rel) {
return nil
}
files = append(files, rel)
return nil
})
if err != nil {
return nil, err
}
return files, nil
}
// shouldSkipFile returns true if a git ls-files output path should be skipped.
// Keeps only widely useful files (those with recognizable extensions).
func shouldSkipFile(path string) bool {

66
internal/tool/file_find_test.go Normal file
View file

@ -0,0 +1,66 @@
package tool
import (
"context"
"os"
"path/filepath"
"strings"
"testing"
)
// TestFileFind_NonGitDirectoryFallback verifies file_find works in a plain
// (non-git) directory by falling back to a filesystem walk instead of
// failing with git's exit 128.
func TestFileFind_NonGitDirectoryFallback(t *testing.T) {
dir := t.TempDir() // plain dir, no `git init`
write := func(rel, content string) {
full := filepath.Join(dir, rel)
if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(full, []byte(content), 0o644); err != nil {
t.Fatal(err)
}
}
write("server.go", "package main\n")
write("internal/handler.go", "package internal\n")
write("node_modules/lib/index.js", "x\n") // excluded by blocklist
write(".gitignore", "ignored.go\n")
write("ignored.go", "package x\n") // excluded by root .gitignore
p := NewFileFind(&FileReader{RepoDir: dir, Mode: ModeWorkspace})
out, err := p.Execute(context.Background(), map[string]any{"query_name": ".go"})
if err != nil {
t.Fatalf("Execute should not error in a non-git dir, got: %v", err)
}
if !strings.Contains(out, "server.go") || !strings.Contains(out, "internal/handler.go") {
t.Errorf("expected go files in result, got:\n%s", out)
}
if strings.Contains(out, "node_modules") {
t.Errorf("node_modules should be excluded, got:\n%s", out)
}
if strings.Contains(out, "ignored.go") {
t.Errorf("ignored.go should be excluded by .gitignore, got:\n%s", out)
}
}
// TestFileFind_NonGitDirectoryNoMatch verifies the not-found path in a
// non-git dir returns the sentinel rather than an error.
func TestFileFind_NonGitDirectoryNoMatch(t *testing.T) {
dir := t.TempDir()
if err := os.WriteFile(filepath.Join(dir, "a.go"), []byte("package a\n"), 0o644); err != nil {
t.Fatal(err)
}
p := NewFileFind(&FileReader{RepoDir: dir, Mode: ModeWorkspace})
out, err := p.Execute(context.Background(), map[string]any{"query_name": "nonexistent_xyz"})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if !strings.Contains(out, "not found") {
t.Errorf("expected not-found sentinel, got: %q", out)
}
}