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

* 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>
2026-07-09 17:28:58 +00:00 · 2026-06-24 22:07:25 +08:00 · 2026-06-24 22:07:25 +08:00 · 18797f8c05
commit 18797f8c05
parent 4a2370f9cd
38 changed files with 4627 additions and 646 deletions
--- a/cmd/opencodereview/shared.go
+++ b/cmd/opencodereview/shared.go
@ -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
+}