mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-07-10 01:39:12 +00:00
* 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>
333 lines
9.9 KiB
Go
333 lines
9.9 KiB
Go
package llmloop
|
|
|
|
import (
|
|
"context"
|
|
"fmt"
|
|
"strings"
|
|
"sync/atomic"
|
|
"time"
|
|
|
|
"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"
|
|
)
|
|
|
|
// Compression thresholds, as fractions of MaxTokens.
|
|
const (
|
|
tokenSoftThreshold = 0.60 // async background compression
|
|
tokenWarningThreshold = 0.80 // immediate sync compression
|
|
)
|
|
|
|
// round groups consecutive messages starting with an assistant message
|
|
// followed by zero or more tool result messages.
|
|
type round struct {
|
|
assistantIdx int
|
|
toolIdxs []int
|
|
}
|
|
|
|
// partitionResult describes how messages should be split for compression.
|
|
type partitionResult struct {
|
|
frozenEnd int
|
|
compressEnd int
|
|
rounds []round
|
|
activeCount int
|
|
}
|
|
|
|
// compressionJob tracks an in-flight background compression operation.
|
|
type compressionJob struct {
|
|
done chan struct{}
|
|
rebuilt []llm.Message
|
|
cancel context.CancelFunc
|
|
snapshotLen int // message count when the snapshot was taken
|
|
}
|
|
|
|
// 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
|
|
for i < len(messages) {
|
|
if messages[i].Role == "assistant" {
|
|
r := round{assistantIdx: i}
|
|
i++
|
|
for i < len(messages) && messages[i].Role == "tool" {
|
|
r.toolIdxs = append(r.toolIdxs, i)
|
|
i++
|
|
}
|
|
rounds = append(rounds, r)
|
|
} else {
|
|
i++
|
|
}
|
|
}
|
|
return rounds
|
|
}
|
|
|
|
// 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 {
|
|
return 0
|
|
}
|
|
|
|
count := 0
|
|
tokensUsed := 0
|
|
for i := len(rounds) - 1; i >= 0; i-- {
|
|
roundTokens := llm.CountTokens(messages[rounds[i].assistantIdx].ExtractText())
|
|
for _, ti := range rounds[i].toolIdxs {
|
|
roundTokens += llm.CountTokens(messages[ti].ExtractText())
|
|
}
|
|
if tokensUsed+roundTokens > budget {
|
|
break
|
|
}
|
|
tokensUsed += roundTokens
|
|
count++
|
|
}
|
|
return count
|
|
}
|
|
|
|
// 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.
|
|
func partitionMessages(messages []llm.Message, maxTokens int, prevSummaryTokenEstimate int) partitionResult {
|
|
result := partitionResult{frozenEnd: 2}
|
|
if len(messages) <= 2 {
|
|
result.compressEnd = len(messages)
|
|
return result
|
|
}
|
|
|
|
result.rounds = groupIntoRounds(messages, 2)
|
|
if len(result.rounds) == 0 {
|
|
result.compressEnd = len(messages)
|
|
return result
|
|
}
|
|
|
|
result.activeCount = computeActiveZoneSize(result.rounds, messages, maxTokens, prevSummaryTokenEstimate)
|
|
if result.activeCount >= len(result.rounds) {
|
|
// Everything fits — no compression needed.
|
|
result.compressEnd = len(messages)
|
|
result.activeCount = 0
|
|
return result
|
|
}
|
|
|
|
// compressEnd = index after the last round NOT in active zone.
|
|
activeStartIdx := len(result.rounds) - result.activeCount
|
|
lastCompressRound := result.rounds[activeStartIdx-1]
|
|
if len(lastCompressRound.toolIdxs) > 0 {
|
|
result.compressEnd = lastCompressRound.toolIdxs[len(lastCompressRound.toolIdxs)-1] + 1
|
|
} else {
|
|
result.compressEnd = lastCompressRound.assistantIdx + 1
|
|
}
|
|
|
|
return result
|
|
}
|
|
|
|
// 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) }
|
|
|
|
// 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, "```")
|
|
}
|
|
}
|
|
s = strings.TrimSpace(s)
|
|
if strings.HasSuffix(s, "```") {
|
|
s = strings.TrimSuffix(s, "```")
|
|
s = strings.TrimSpace(s)
|
|
}
|
|
return s
|
|
}
|
|
|
|
// 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, 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(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 := r.deps.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{
|
|
Model: r.deps.Model,
|
|
Messages: compressionMsgs,
|
|
MaxTokens: r.deps.Template.MaxTokens,
|
|
})
|
|
duration := time.Since(startTime)
|
|
|
|
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)
|
|
// 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(&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
|
|
}
|
|
|
|
rebuilt := make([]llm.Message, 2)
|
|
copy(rebuilt, msgs[:2])
|
|
|
|
userMsg := rebuilt[1]
|
|
currentText := userMsg.ExtractText()
|
|
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++ {
|
|
rebuilt = append(rebuilt, msgs[i])
|
|
}
|
|
|
|
return rebuilt, nil
|
|
}
|
|
|
|
// triggerAsyncCompression kicks off a background compression job.
|
|
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)}
|
|
r.compressionMu.Lock()
|
|
r.pendingJob = job
|
|
r.compressionMu.Unlock()
|
|
|
|
go func() {
|
|
defer cancel()
|
|
rebuilt, err := r.runCompression(asyncCtx, msgSnapshot, filePath)
|
|
|
|
r.compressionMu.Lock()
|
|
defer r.compressionMu.Unlock()
|
|
|
|
if r.pendingJob != job {
|
|
return // cancelled or superseded
|
|
}
|
|
if err != nil {
|
|
// Compression failed — abandon the job rather than applying a
|
|
// truncated/unmodified snapshot over live messages.
|
|
r.pendingJob = nil
|
|
close(job.done)
|
|
return
|
|
}
|
|
job.rebuilt = rebuilt
|
|
close(job.done)
|
|
}()
|
|
}
|
|
|
|
// 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
|
|
}
|
|
|
|
select {
|
|
case <-job.done:
|
|
applied := false
|
|
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 r.pendingJob == job {
|
|
r.pendingJob = nil
|
|
}
|
|
r.compressionMu.Unlock()
|
|
return applied
|
|
default:
|
|
return false
|
|
}
|
|
}
|
|
|
|
// cancelPendingCompression aborts any in-flight background compression.
|
|
func (r *Runner) cancelPendingCompression() {
|
|
r.compressionMu.Lock()
|
|
defer r.compressionMu.Unlock()
|
|
|
|
if r.pendingJob != nil {
|
|
r.pendingJob.cancel()
|
|
r.pendingJob = nil
|
|
}
|
|
}
|