Files
Zhiming Wang 152cc6d585 fix(agent): make --preview apply the same selection the review applies (#801)
* fix(agent): make --preview apply the same selection the review applies

`--preview` applied only the static file filters, but a real review also
ran filterLargeDiffs before it froze coverage, dropping any diff whose
token count exceeded 80% of max_tokens. That filter ran before
registerCoverage sealed the selected set, so a file could be listed
under "Will review" and then never be reviewed — and preview could not
apply the gate even in principle, because runPreviewContext passed no
Template and Template.MaxTokens was therefore zero on that path.

Extract the deterministic pre-dispatch selection into one pure
operation, selectFiles, that returns every input diff with a decision
and a stable reason. Preview and Run both consume it, so neither can
re-derive the rules on its own: reimplementing the size check inside
preview would just let the two drift again. Run keeps its previous log
output by reporting the static exclusions and the size exclusions as two
groups.

Add too_large as the exclusion reason for an oversized diff, so
consumers need not reproduce the tokenizer or the 80% threshold to
explain the drop. The human preview renders it through the existing
reason column.

Give preview the real limit without initializing a provider:
resolveMaxTokens needs only AppCfg, which loads independently of
llm.ResolveEndpointWithOptions, so preview still requires no API key.
`ocr scan --preview` already resolved its limit this way, through
resolveScanPreviewMaxTokens; that helper had the same body as
previewMaxTokens, so scan now calls previewMaxTokens instead.
`ocr delegate preview` passes no template and keeps the gate
disabled — the host agent reviews with its own context window, not
OCR's max_tokens.

One behavior delta beyond the fix: a deletion is decided before the size
gate, so an oversized deletion now stays in the working diff set instead
of being dropped by filterLargeDiffs. It was never dispatched or
registered either way, and it now behaves like any other deletion.

ResolveIdentity, the resume pre-flight, must hash the same selection
the run records in source_artifact_sha256, so it now replays
selectFiles as well. That digest skips deletions, so the delta above
does not move it.

The skip line for an emptied selection now names the count and drops its
"All changed files" claim, which was wrong whenever the static filters
had already removed some.

Selection also decides the run-level skip signal now. review.skipped
carries the cause in an attribute — too_large, deleted or
no_supported_files, most actionable first — and is keyed on the
selected set, the same quantity registerCoverage seals, so a
deletion-only run counts as skipped too. Two judgement calls there:
no.files.changed keeps firing exactly where it did, so existing
consumers are unaffected, and a deletion-only run emits review.skipped
before review.started rather than suppressing review.started, which
would drop a signal main emits. An oversized changeset used to emit
review.started with a review.count that included the files it then
dropped, and no skip event at all.

Deliberately out of scope because the outcomes are execution-specific
or unavailable to preview: aggregate --max-tokens-budget exhaustion,
resume reuse and LLM failures.

The regression test pins that equality directly: preview's will_review
set equals the set a fresh, non-resumed, unbudgeted run registers as
selected coverage, over a changeset that mixes reviewable,
extension-filtered, deleted and oversized files, asserting each of those
outcomes is really represented. The CLI test covers both sources of the
limit, since --max-tokens short-circuits config resolution; the
all-oversized case drives Run, which is where the gate now runs; and the
model constant table pins too_large's wire value. The unit tests that
covered filterDiffs / filterLargeDiffs / countReviewable now cover the
one selection they were folded into.

Update architecture and review-rule docs for selection.go and the two
post-gate outcomes, add too_large to the FAQ, and document the new
review.skipped telemetry event in every locale.

Fixes #782

* test(cli): pin preview's max_tokens to the value the run applies

Selection is one operation that preview and the run both consume, but
each resolves the max_tokens it hands selection on its own path: the run
from the app config its LLM runtime loaded, preview through
previewMaxTokens, which loads the config without building a runtime. A
source or precedence change on either path alone would put preview's
will_review set out of step with the run again, with nothing failing.

Drive a real review through the CLI for each source of the limit — the
template default, the saved max_tokens, --max-tokens, and the flag
overriding a saved value — and assert previewMaxTokens returns what the
run handed its agent. The change set holds only a file the static gates
exclude, so the run skips before any LLM request.

Read the run's value off agent.Args through a newReviewAgent variable
rather than recomputing it in the test, so the assertion follows what
executeReviewContext actually applies instead of restating its
precedence.

* test(review): drop agent.New seam from max-tokens parity test

Assert previewMaxTokens parity by calling resolveMaxTokens directly (the
same function the run uses) instead of monkeypatching agent.New through a
package-level var that leaked into review_cmd.go. Removes global mutable
test state and the production indirection.

---------

Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>
2026-09-11 12:52:12 +08:00

326 lines
9.8 KiB
Go

// SPDX-License-Identifier: Apache-2.0
// Copyright 2026 alibaba/open-code-review Contributors
package main
import (
"context"
"encoding/json"
"fmt"
"os"
"github.com/alibaba/open-code-review/internal/agent"
"github.com/alibaba/open-code-review/internal/config/rules"
"github.com/alibaba/open-code-review/internal/delegate"
"github.com/alibaba/open-code-review/internal/diff"
"github.com/alibaba/open-code-review/internal/tool"
"github.com/spf13/cobra"
)
type delegateOptions struct {
repoDir string
from string
to string
commit string
excludes string
rulePath string
background string
backgroundFile string
maxGitProcs int
format string
}
var delegatePreviewOpts delegateOptions
var delegateRuleOpts delegateOptions
var delegateCmd = &cobra.Command{
Use: "delegate",
Aliases: []string{"d"},
Short: "Output review spec for host-agent delegation (no LLM required)",
Long: `OpenCodeReview - Delegation Mode
Output review spec for host-agent delegation (no LLM required).`,
Example: ` # Preview which files will be reviewed
ocr delegate preview --from main --to feature
# Preview workspace changes
ocr delegate preview
# Get rules for multiple files (grouped by content)
ocr delegate rule internal/agent/agent.go internal/llm/client.go`,
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, args []string) error {
return cmd.Help()
},
}
var delegatePreviewCmd = &cobra.Command{
Use: "preview [flags]",
Short: "Preview reviewable files with mode/ref metadata",
Long: "Outputs reviewable file list with mode/ref metadata for the host agent to construct git commands.",
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, args []string) error {
if err := validateDelegateOptions(&delegatePreviewOpts); err != nil {
return err
}
return executeDelegatePreview(delegatePreviewOpts)
},
}
var delegateRuleCmd = &cobra.Command{
Use: "rule [flags] <path...>",
Short: "Output resolved review rules grouped by content",
Long: "Outputs resolved review rules grouped by content. Accepts multiple paths.",
Args: minimumArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
if err := validateDelegateOptions(&delegateRuleOpts); err != nil {
return err
}
return executeDelegateRule(delegateRuleOpts, args)
},
}
func init() {
registerDelegateFlags(delegatePreviewCmd, &delegatePreviewOpts)
registerDelegateFlags(delegateRuleCmd, &delegateRuleOpts)
delegateCmd.AddCommand(delegatePreviewCmd)
delegateCmd.AddCommand(delegateRuleCmd)
}
// delegateContext holds the shared state for delegate sub-commands.
type delegateContext struct {
cc *commonContext
opts delegateOptions
}
func loadDelegateContext(opts delegateOptions) (*delegateContext, error) {
contentRef, _ := tool.ParseReviewMode(opts.from, opts.to, opts.commit).RefValue(opts.to, opts.commit)
cc, err := loadCommonContext(opts.repoDir, opts.rulePath, contentRef, 0, opts.maxGitProcs, true)
if err != nil {
return nil, err
}
applyCLIExcludes(cc, splitPaths(opts.excludes))
// Security: reject ref-option injection.
reviewOpts := reviewOptions{from: opts.from, to: opts.to, commit: opts.commit}
if err := validateReviewRefs(cc.RepoDir, reviewOpts); err != nil {
return nil, err
}
bg, err := resolveBackground(cc.RepoDir, opts.background, opts.backgroundFile, opts.commit)
if err != nil {
return nil, err
}
opts.background = bg
return &delegateContext{cc: cc, opts: opts}, nil
}
// preview runs the agent's file-selection logic and returns the preview result.
//
// No Template is passed, which leaves the per-file diff-size ceiling disabled:
// the host agent reviews with its own context window, so OCR's max_tokens is
// not the limit that applies to delegated work.
func (dc *delegateContext) preview(ctx context.Context) (*agent.DiffPreview, error) {
return agent.Preview(ctx, agent.Args{
RepoDir: dc.cc.RepoDir,
From: dc.opts.from,
To: dc.opts.to,
Commit: dc.opts.commit,
FileFilter: dc.cc.FileFilter,
GitRunner: dc.cc.GitRunner,
})
}
// mergeBase computes the merge-base for range mode. Returns "" for other modes.
func (dc *delegateContext) mergeBase(ctx context.Context) string {
if dc.opts.from == "" || dc.opts.to == "" {
return ""
}
provider := diff.NewProvider(dc.cc.RepoDir, dc.opts.from, dc.opts.to, dc.cc.GitRunner)
return provider.MergeBase(ctx)
}
// reviewMode returns the string mode identifier.
func (dc *delegateContext) reviewMode() string {
switch {
case dc.opts.commit != "":
return "commit"
case dc.opts.from != "" && dc.opts.to != "":
return "range"
default:
return "workspace"
}
}
// resolver returns the rules Resolver, asserting DetailResolver if available.
func (dc *delegateContext) resolver() rules.Resolver {
return dc.cc.Resolver
}
func executeDelegatePreview(opts delegateOptions) error {
dc, err := loadDelegateContext(opts)
if err != nil {
return err
}
ctx := context.Background()
preview, err := dc.preview(ctx)
if err != nil {
return fmt.Errorf("preview failed: %w", err)
}
mergeBase := dc.mergeBase(ctx)
if opts.format == "json" {
return writeDelegateJSON(delegatePreviewJSON{
SchemaVersion: delegateSchemaVersion,
Mode: dc.reviewMode(),
Repository: dc.cc.RepoDir,
From: dc.opts.from,
To: dc.opts.to,
Commit: dc.opts.commit,
MergeBase: mergeBase,
Background: dc.opts.background,
TotalFiles: preview.TotalFiles,
ReviewableCount: preview.ReviewableCount,
ExcludedCount: preview.ExcludedCount,
TotalInsertions: preview.TotalInsertions,
TotalDeletions: preview.TotalDeletions,
ReviewableFiles: previewFiles(preview, true),
ExcludedFiles: previewFiles(preview, false),
})
}
fmt.Printf("# Files (%d reviewable / %d total)\n\n", preview.ReviewableCount, preview.TotalFiles)
fmt.Printf("- mode: %s\n", dc.reviewMode())
if dc.opts.from != "" {
fmt.Printf("- from: %s\n", dc.opts.from)
}
if dc.opts.to != "" {
fmt.Printf("- to: %s\n", dc.opts.to)
}
if dc.opts.commit != "" {
fmt.Printf("- commit: %s\n", dc.opts.commit)
}
if mergeBase != "" {
fmt.Printf("- merge_base: %s\n", mergeBase)
}
if dc.opts.background != "" {
fmt.Printf("- background: %s\n", dc.opts.background)
}
fmt.Printf("- total_insertions: %d\n", preview.TotalInsertions)
fmt.Printf("- total_deletions: %d\n\n", preview.TotalDeletions)
for _, entry := range preview.Entries {
marker := " "
if !entry.WillReview {
marker = "~~"
}
fmt.Printf("%s- `%s` [%s] +%d/-%d", marker, entry.Path, entry.Status, entry.Insertions, entry.Deletions)
if !entry.WillReview {
fmt.Printf(" (excluded: %s)", entry.ExcludeReason)
fmt.Print("~~")
}
fmt.Println()
}
return nil
}
func executeDelegateRule(opts delegateOptions, paths []string) error {
dc, err := loadDelegateContext(opts)
if err != nil {
return err
}
groups := delegate.GroupRules(dc.resolver(), paths)
if opts.format == "json" {
return writeDelegateJSON(delegateRulesJSON{
SchemaVersion: delegateSchemaVersion,
Groups: ruleGroupsJSON(groups),
})
}
fmt.Print(delegate.RuleGroupsMarkdown(groups))
return nil
}
const delegateSchemaVersion = "1"
type delegatePreviewFileJSON struct {
Path string `json:"path"`
Status string `json:"status"`
Insertions int64 `json:"insertions"`
Deletions int64 `json:"deletions"`
ExcludeReason string `json:"exclude_reason,omitempty"`
}
type delegatePreviewJSON struct {
SchemaVersion string `json:"schema_version"`
Mode string `json:"mode"`
Repository string `json:"repository"`
From string `json:"from,omitempty"`
To string `json:"to,omitempty"`
Commit string `json:"commit,omitempty"`
MergeBase string `json:"merge_base,omitempty"`
Background string `json:"background,omitempty"`
TotalFiles int `json:"total_files"`
ReviewableCount int `json:"reviewable_count"`
ExcludedCount int `json:"excluded_count"`
TotalInsertions int64 `json:"total_insertions"`
TotalDeletions int64 `json:"total_deletions"`
ReviewableFiles []delegatePreviewFileJSON `json:"reviewable_files"`
ExcludedFiles []delegatePreviewFileJSON `json:"excluded_files"`
}
type delegateRuleGroupJSON struct {
GroupID int `json:"group_id"`
Source string `json:"source"`
Pattern string `json:"pattern"`
Files []string `json:"files"`
Rule string `json:"rule"`
}
type delegateRulesJSON struct {
SchemaVersion string `json:"schema_version"`
Groups []delegateRuleGroupJSON `json:"groups"`
}
func previewFiles(preview *agent.DiffPreview, reviewable bool) []delegatePreviewFileJSON {
files := make([]delegatePreviewFileJSON, 0)
for _, entry := range preview.Entries {
if entry.WillReview != reviewable {
continue
}
files = append(files, delegatePreviewFileJSON{
Path: entry.Path,
Status: entry.Status,
Insertions: entry.Insertions,
Deletions: entry.Deletions,
ExcludeReason: string(entry.ExcludeReason),
})
}
return files
}
func ruleGroupsJSON(groups []delegate.RuleGroup) []delegateRuleGroupJSON {
out := make([]delegateRuleGroupJSON, 0, len(groups))
for _, group := range groups {
files := make([]string, 0, len(group.Files))
files = append(files, group.Files...)
out = append(out, delegateRuleGroupJSON{
GroupID: group.ID,
Source: group.Source,
Pattern: group.Pattern,
Files: files,
Rule: group.Text,
})
}
return out
}
func writeDelegateJSON(value any) error {
encoder := json.NewEncoder(os.Stdout)
encoder.SetIndent("", " ")
encoder.SetEscapeHTML(false)
return encoder.Encode(value)
}