mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-09-14 19:59:52 +08:00
152cc6d585
* 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>
326 lines
9.8 KiB
Go
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)
|
|
}
|