2 commits

Author	SHA1	Message	Date
kite	9a11fc1302	test: improve coverage for cmd/opencodereview package from 42% to 70% ... Add comprehensive unit tests covering config dispatch, emit run result, git helpers, provider commands, provider TUI pure functions and View rendering, shared utilities, flags parsing, scan command flags, and small file entry points (version, viewer, llm, rules commands).	2026-06-27 01:47:48 +08:00
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