open-code-review/cmd/opencodereview/flags.go
css521 18797f8c05
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-06-24 22:07:25 +08:00

315 lines
11 KiB
Go

package main
import (
"flag"
"fmt"
"os"
"time"
)
// --- custom flag set that supports short flags (-c, -f etc.) ---
type ocrFlagSet struct {
fs *flag.FlagSet
shortMap map[string]string // maps short key "c" -> full name "commit"
showHelp bool
}
func newOcrFlagSet(name string) *ocrFlagSet {
return &ocrFlagSet{
fs: flag.NewFlagSet(name, flag.ContinueOnError),
shortMap: make(map[string]string),
}
}
// StringVarP registers --name with optional short form -s.
func (a *ocrFlagSet) StringVarP(p *string, name, shorthand string, value, usage string) {
suffix := ""
if shorthand != "" {
a.shortMap[shorthand] = name
suffix = fmt.Sprintf(" (shorthand: -%s)", shorthand)
}
a.fs.StringVar(p, name, value, usage+suffix)
}
// BoolVarP registers --name with optional short form -s.
func (a *ocrFlagSet) BoolVarP(p *bool, name, shorthand string, value bool, usage string) {
suffix := ""
if shorthand != "" {
a.shortMap[shorthand] = name
suffix = fmt.Sprintf(" (shorthand: -%s)", shorthand)
}
a.fs.BoolVar(p, name, value, usage+suffix)
}
func (a *ocrFlagSet) StringVar(p *string, name string, value string, usage string) {
a.fs.StringVar(p, name, value, usage)
}
func (a *ocrFlagSet) BoolVar(p *bool, name string, value bool, usage string) {
a.fs.BoolVar(p, name, value, usage)
}
func (a *ocrFlagSet) IntVar(p *int, name string, value int, usage string) {
a.fs.IntVar(p, name, value, usage)
}
func (a *ocrFlagSet) DurationVar(p *time.Duration, name string, value time.Duration, usage string) {
a.fs.DurationVar(p, name, value, usage)
}
func (a *ocrFlagSet) PrintDefaults() {
a.fs.PrintDefaults()
}
func (a *ocrFlagSet) Parse(arguments []string) error {
expanded := expandShortFlags(arguments, a.shortMap)
for _, arg := range expanded {
if arg == "-h" || arg == "--help" {
a.showHelp = true
return nil
}
}
return a.fs.Parse(expanded)
}
// expandShortFlags replaces standalone -X args with their long equivalents.
// Only triggers when the arg is exactly -N (single char after dash).
func expandShortFlags(args []string, shortMap map[string]string) []string {
out := make([]string, 0, len(args))
for _, arg := range args {
if len(arg) == 2 && arg[0] == '-' && arg[1] != '-' {
key := string(arg[1])
if full, ok := shortMap[key]; ok {
out = append(out, "--"+full)
continue
}
}
out = append(out, arg)
}
return out
}
// --- review subcommand options ---
type reviewOptions struct {
toolConfigPath string
rulePath string
repoDir string
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
model string // --model: override resolved LLM model for this review
concurrency int
perFileTimeout int
maxTools int
maxGitProcs int
preview bool
showHelp bool
}
func parseReviewFlags(args []string) (reviewOptions, error) {
a := newOcrFlagSet("ocr review")
opts := reviewOptions{}
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.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")
a.StringVar(&opts.audience, "audience", "human", "output audience: human (show progress) or agent (summary only)")
a.StringVarP(&opts.background, "background", "b", "", "optional requirement/business context for the review")
a.StringVar(&opts.model, "model", "", "override LLM model for this review (e.g., claude-opus-4-6)")
a.IntVar(&opts.maxTools, "max-tools", 0, "max tool call rounds per file (0 = template default; min 10)")
a.IntVar(&opts.maxGitProcs, "max-git-procs", 16, "max concurrent git subprocesses")
a.BoolVarP(&opts.preview, "preview", "p", false, "preview which files will be reviewed without running the LLM")
if err := a.Parse(args); err != nil {
return opts, fmt.Errorf("parse flags: %w", err)
}
opts.showHelp = a.showHelp
if opts.showHelp {
return opts, nil
}
modeCount := 0
if opts.from != "" || opts.to != "" {
modeCount++
}
if opts.commit != "" {
modeCount++
}
// modeCount == 0 → workspace mode (no error, allowed)
if modeCount > 1 {
return opts, fmt.Errorf("only one review mode allowed (--from/--to or --commit)")
}
if opts.from != "" && opts.to == "" {
return opts, fmt.Errorf("--to is required when --from is specified")
}
if opts.to != "" && opts.from == "" {
return opts, fmt.Errorf("--from is required when --to is specified")
}
switch opts.audience {
case "human", "agent":
default:
return opts, fmt.Errorf("invalid --audience value %q: must be 'human' or 'agent'", opts.audience)
}
const minMaxTools = 10
if opts.maxTools < 0 {
return opts, fmt.Errorf("--max-tools must be a non-negative integer (0 means use template default)")
}
if opts.maxTools > 0 && opts.maxTools < minMaxTools {
fmt.Fprintf(os.Stderr, "[ocr] --max-tools %d is below minimum %d, using %d\n", opts.maxTools, minMaxTools, minMaxTools)
opts.maxTools = minMaxTools
}
if opts.maxGitProcs < 0 {
return opts, fmt.Errorf("--max-git-procs must be a non-negative integer (0 means use default 16)")
}
return opts, nil
}
func printReviewUsage() {
fmt.Println(`OpenCodeReview - AI-Powered Code Review CLI
Usage:
ocr review [flags]
ocr r [flags] (alias)
Examples:
# Review staged + unstaged + untracked changes in current workspace
ocr review
# Review a branch against its base (merge-base mode)
ocr review --from master --to dev-ref
# Review a specific commit
ocr review --commit abc123
ocr review -c abc123
# Output JSON format
ocr review --format json
ocr review -f json
# Agent mode (summary only, no progress lines)
ocr review --audience agent
# Preview which files will be reviewed
ocr review --preview
ocr review -c abc123 -p
Flags:
--audience string output audience: human (show progress) or agent (summary only) (default "human")
-b, --background string optional requirement/business context for the review
-c, --commit string single commit hash or tag to review (vs its parent)
-f, --format string output format: text or json (default "text")
--concurrency int max concurrent file reviews (default 8)
--max-git-procs int max concurrent git subprocesses (default 16)
--from string source ref to start diff from (e.g., 'main')
--max-tools int max tool call rounds per file (0 = template default; min 10)
--model string override LLM model for this review (e.g., claude-opus-4-6)
-p, --preview preview which files will be reviewed 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)
--to string target ref to end diff at (e.g., 'feature-branch')
--tools string path to JSON tools config file (default: embedded)`)
}
// --- config subcommand ---
type configAction struct {
subCmd string // "set", "unset"
key string
value string
}
func parseConfigArgs(args []string) (configAction, error) {
if len(args) == 0 {
return configAction{}, fmt.Errorf("usage: ocr config set <key> <value>\ne.g., ocr config set llm.model claude-opus-4-6")
}
subCmd := args[0]
switch subCmd {
case "set":
if len(args) < 3 {
return configAction{}, fmt.Errorf("usage: ocr config set <key> <value>\ne.g., ocr config set llm.model claude-opus-4-6")
}
return configAction{
subCmd: "set",
key: args[1],
value: args[2],
}, nil
case "unset":
if len(args) < 2 {
return configAction{}, fmt.Errorf("usage: ocr config unset custom_providers.<name>\ne.g., ocr config unset custom_providers.my-gateway")
}
return configAction{
subCmd: "unset",
key: args[1],
}, nil
default:
return configAction{}, fmt.Errorf("unknown config sub-command: %s\nAvailable: set, unset, provider, model", subCmd)
}
}
func printConfigUsage() {
fmt.Println(`Configuration management.
Usage:
ocr config set <key> <value>
ocr config unset custom_providers.<name> Delete a custom provider
ocr config provider Interactive provider setup
ocr config model Interactive model selection
Examples:
# Provider setup (interactive)
ocr config provider
ocr config model
# Provider setup (non-interactive)
ocr config set provider anthropic
ocr config set model claude-opus-4-6
# Set API key via environment variable (recommended) or config:
# export ANTHROPIC_API_KEY=sk-ant-xxx
ocr config set providers.anthropic.api_key "$ANTHROPIC_API_KEY"
# Custom provider
ocr config set provider my-gateway
ocr config set custom_providers.my-gateway.url https://gateway.internal.com/v1
ocr config set custom_providers.my-gateway.protocol openai
ocr config set custom_providers.my-gateway.model llama-3-70b
ocr config set custom_providers.my-gateway.models '["llama-3-70b","llama-3-8b"]'
ocr config set custom_providers.my-gateway.api_key "$MY_API_KEY"
# Delete a custom provider
ocr config unset custom_providers.my-gateway
# Legacy endpoint configuration
ocr config set llm.url https://xx/v1/openai/chat/completions
ocr config set llm.auth_token xxxxxxxxxx
ocr config set llm.auth_header x-api-key
ocr config set llm.model claude-opus-4-6
ocr config set llm.extra_body '{"thinking":{"type":"disabled"}}'
ocr config set language English
ocr config set telemetry.enabled true
Supported keys: provider, model, providers.<name>.<field>, custom_providers.<name>.<field>, llm.url, llm.auth_token, llm.auth_header, llm.model, llm.use_anthropic, llm.extra_body, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging
Provider fields: api_key, url, protocol, model, models, auth_header, extra_body`)
}