diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml index c009dff3..c413a556 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.yml +++ b/.github/ISSUE_TEMPLATE/bug_report.yml @@ -39,8 +39,6 @@ body: label: Installation Method options: - npm (global) - - Homebrew - - MacPorts - GitHub Release binary - Built from source validations: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 23a1067f..c0fb2a3f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,16 +3,8 @@ name: CI on: push: branches: [main] - paths-ignore: - - '**.md' - - '**/LICENSE' - - '**/.gitignore' pull_request: branches: [main] - paths-ignore: - - '**.md' - - '**/LICENSE' - - '**/.gitignore' permissions: contents: read diff --git a/.github/workflows/deploy-pages.yml b/.github/workflows/deploy-pages.yml index 4c3b3c15..204c1014 100644 --- a/.github/workflows/deploy-pages.yml +++ b/.github/workflows/deploy-pages.yml @@ -5,9 +5,6 @@ on: branches: [main] paths: - 'pages/**' - - 'install.sh' - - 'install.ps1' - - '.github/workflows/deploy-pages.yml' workflow_dispatch: permissions: @@ -45,16 +42,6 @@ jobs: mkdir -p _site cp -r pages/dist/* _site/ cp pages/logo.svg _site/logo.svg - cp install.sh _site/install.sh - cp install.ps1 _site/install.ps1 - - - name: Verify install scripts - run: | - test -f _site/install.sh - test -f _site/install.ps1 - sh -n _site/install.sh - cmp install.sh _site/install.sh - cmp install.ps1 _site/install.ps1 - uses: actions/upload-pages-artifact@v5 with: diff --git a/README.ja-JP.md b/README.ja-JP.md index 8038b18e..83d05b06 100644 --- a/README.ja-JP.md +++ b/README.ja-JP.md @@ -50,8 +50,6 @@ Gitのdiffを読み取り、変更されたファイルをツール利用機能 実際のコードレビューに基づくベンチマーク。**50**の人気オープンソースリポジトリから**200**の実際のPull Requestを厳選し、**10**のプログラミング言語をカバー——80人以上のシニアエンジニアによるクロスバリデーション（**1,505**件のアノテーション済み欠陥）。 -Hugging Face Hugging FaceでAACR-Benchデータセットを見る。 - | 指標 | 測定内容 | 重要性 | |------|----------|--------| | **F1** | 精度とリコールの調和平均 | レビュー品質を示す最良の単一指標 | diff --git a/README.ko-KR.md b/README.ko-KR.md index 7baa8fa8..bb90e69b 100644 --- a/README.ko-KR.md +++ b/README.ko-KR.md @@ -50,8 +50,6 @@ Open Code Review는 AI 기반 코드 리뷰 CLI 도구입니다. Alibaba Group 실제 코드 리뷰 기반 벤치마크. **50**개 인기 오픈소스 저장소에서 **200**개 실제 Pull Request를 엄선하고, **10**개 프로그래밍 언어를 커버 — 80명 이상의 시니어 엔지니어가 교차 검증(**1,505**개 어노테이션된 결함). -Hugging Face Hugging Face에서 AACR-Bench 데이터셋 살펴보기. - | 지표 | 측정 내용 | 중요한 이유 | |------|-----------|-------------| | **F1** | 정밀도와 재현율의 조화 평균 | 리뷰 품질을 나타내는 최적의 단일 지표 | diff --git a/README.md b/README.md index 24dbd64d..452e0221 100644 --- a/README.md +++ b/README.md @@ -50,8 +50,6 @@ Visit the [official website](https://open-codereview.ai) for more details. A real-world code review benchmark built from **50** popular open-source repositories, **200** real Pull Requests, and **10** programming languages — cross-validated by 80+ senior engineers (**1,505** annotated ground-truth issues). -Hugging Face Explore the AACR-Bench dataset on Hugging Face. - | Metric | What it measures | Why it matters | |--------|-----------------|----------------| | **F1** | Harmonic mean of precision and recall | Best single number for overall review quality | diff --git a/README.ru-RU.md b/README.ru-RU.md index 821e4f48..8fa4fb80 100644 --- a/README.ru-RU.md +++ b/README.ru-RU.md @@ -50,8 +50,6 @@ Open Code Review — это CLI-инструмент для код-ревью н Бенчмарк собран на основе реальных код-ревью и включает **50** популярных open-source-репозиториев, **200** реальных пул-реквестов и **10** языков программирования. Разметку из **1 505** дефектов перекрёстно проверили более 80 ведущих инженеров. -Hugging Face Открыть датасет AACR-Bench на Hugging Face. - | Метрика | Что измеряет | Почему важна | |---------|-------------|--------------| | **F1** | Гармоническое среднее precision и recall | Лучший единый показатель качества ревью | diff --git a/README.zh-CN.md b/README.zh-CN.md index 8e6d7ef5..93a2df4d 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -50,8 +50,6 @@ Open Code Review 是一款 AI 驱动的代码审查 CLI 工具。它的前身是 基于真实场景的代码审查基准测试，从 **50** 个热门开源仓库中精选 **200** 个真实的 Pull Request，覆盖 **10** 种编程语言——由 80+ 位资深工程师交叉标注验证（共 **1,505** 个标注缺陷）。 -Hugging Face 在 Hugging Face 上探索 AACR-Bench 数据集。 - | 指标 | 含义 | 为什么重要 | |------|------|-----------| | **F1** | 准确率与召回率的调和均值 | 综合衡量审查质量的最佳单一指标 | diff --git a/cmd/opencodereview/compat_test.go b/cmd/opencodereview/compat_test.go index c14a17af..462a8ca5 100644 --- a/cmd/opencodereview/compat_test.go +++ b/cmd/opencodereview/compat_test.go @@ -3,15 +3,7 @@ package main -import ( - "context" - - "github.com/spf13/cobra" -) - -func executeReview(opts reviewOptions) error { - return executeReviewContext(context.Background(), opts) -} +import "github.com/spf13/cobra" // parseReviewFlags provides test compatibility: parses args through a fresh // cobra command instance and returns the resulting reviewOptions. diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index 1d1e8dd2..7b2eae12 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" "os" - "os/signal" "path/filepath" "sort" "strings" @@ -47,8 +46,9 @@ type reviewOptions struct { maxGitProcs int maxTokens int maxTokensBudget int - noFilter bool - preview bool + noFilter bool + preview bool + resumeIncremental bool } var reviewOpts reviewOptions @@ -97,9 +97,7 @@ var reviewCmd = &cobra.Command{ if err := validateReviewOptions(&reviewOpts); err != nil { return err } - ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt) - defer stop() - return executeReviewContext(ctx, reviewOpts) + return executeReview(reviewOpts) }, } @@ -107,7 +105,7 @@ func init() { registerReviewFlags(reviewCmd, &reviewOpts) } -func executeReviewContext(ctx context.Context, opts reviewOptions) error { +func executeReview(opts reviewOptions) error { cc, err := loadCommonContext(opts.repoDir, opts.rulePath, opts.maxTools, opts.maxGitProcs, true) if err != nil { return err @@ -140,7 +138,7 @@ func executeReviewContext(ctx context.Context, opts reviewOptions) error { } if opts.preview { - return runPreviewContext(ctx, cc, opts) + return runPreview(cc, opts) } resumeState, err := loadReviewResumeState(cc.RepoDir, opts) @@ -165,7 +163,7 @@ func executeReviewContext(ctx context.Context, opts reviewOptions) error { // Strictly before agent.New, so a rejected resume persists nothing. The sealed // input it returns pins the run to the very commits this check passed on, so // the decision cannot be undone by a ref moving afterwards. - sealed, err := validateResumeIdentity(ctx, cc, opts, rt, resumeState) + sealed, err := validateResumeIdentity(context.Background(), cc, opts, rt, resumeState) if err != nil { return err } @@ -189,7 +187,7 @@ func executeReviewContext(ctx context.Context, opts reviewOptions) error { } tools := buildToolRegistry(rt.Collector, fileReader) - mcpClients := initMCPClients(ctx, rt.AppCfg, tools, cc.RepoDir, Version) + mcpClients := initMCPClients(context.Background(), rt.AppCfg, tools, cc.RepoDir, Version) defer func() { for _, mc := range mcpClients { if err := mc.Close(); err != nil { @@ -235,7 +233,7 @@ func executeReviewContext(ctx context.Context, opts reviewOptions) error { q := newQuietHandle(opts.outputFormat, opts.audience) defer q.Restore() - runCtx, span := telemetry.StartSpan(telemetry.ContextWithTraceParentFromEnv(ctx), "review.run") + ctx, span := telemetry.StartSpan(telemetry.ContextWithTraceParentFromEnv(context.Background()), "review.run") defer span.End() telemetry.SetAttr(span, "review.repo", cc.RepoDir) telemetry.SetAttr(span, "review.from", opts.from) @@ -250,7 +248,7 @@ func executeReviewContext(ctx context.Context, opts reviewOptions) error { } startTime := time.Now() - comments, runErr := ag.Run(runCtx) + comments, runErr := ag.Run(ctx) manifest := ag.RunManifest() // Freeze the retry report at the same boundary as the manifest: ag.Run has @@ -280,7 +278,7 @@ func executeReviewContext(ctx context.Context, opts reviewOptions) error { var emitErr error emitted := manifest != nil || runErr == nil if emitted { - emitErr = emitRunResult(runCtx, ag, comments, startTime, opts.outputFormat, opts.audience, q, llmIdentity, retryReport) + emitErr = emitRunResult(ctx, ag, comments, startTime, opts.outputFormat, opts.audience, q, llmIdentity, retryReport) if emitErr != nil { emitErr = fmt.Errorf("emit review result: %w", emitErr) } @@ -396,6 +394,9 @@ func validateResumeIdentity(ctx context.Context, cc *commonContext, opts reviewO Model: rt.Model, ProviderExplicit: opts.provider != "", ModelExplicit: opts.model != "", + Incremental: opts.resumeIncremental, + ResolvedBase: sealed.Resolution.ResolvedBase, + PerFileMaxTokens: cc.Template.MaxTokens, }); err != nil { return nil, err } @@ -481,8 +482,8 @@ func validateReviewRefs(repoDir string, opts reviewOptions) error { return nil } -func runPreviewContext(ctx context.Context, cc *commonContext, opts reviewOptions) error { - preview, err := agent.Preview(ctx, agent.Args{ +func runPreview(cc *commonContext, opts reviewOptions) error { + preview, err := agent.Preview(context.Background(), agent.Args{ RepoDir: cc.RepoDir, From: opts.from, To: opts.to, diff --git a/cmd/opencodereview/review_helpers_test.go b/cmd/opencodereview/review_helpers_test.go index c9feac55..4364f911 100644 --- a/cmd/opencodereview/review_helpers_test.go +++ b/cmd/opencodereview/review_helpers_test.go @@ -11,10 +11,6 @@ import ( "github.com/alibaba/open-code-review/internal/tool" ) -func runPreview(cc *commonContext, opts reviewOptions) error { - return runPreviewContext(context.Background(), cc, opts) -} - func TestRunPreview(t *testing.T) { dir := initTestGitRepo(t) gitCommitFile(t, dir, "x.go", "package x\n", "add x") diff --git a/cmd/opencodereview/shared.go b/cmd/opencodereview/shared.go index d52d4cde..bfa89989 100644 --- a/cmd/opencodereview/shared.go +++ b/cmd/opencodereview/shared.go @@ -403,17 +403,9 @@ func emitRunResult( } if !machineReadable { - telemetry.PrintTraceSummary(telemetry.TraceSummary{ - FilesReviewed: ag.FilesReviewed(), - CommentsGenerated: int64(len(comments)), - InputTokens: ag.TotalInputTokens(), - OutputTokens: ag.TotalOutputTokens(), - TotalTokens: ag.TotalTokensUsed(), - CacheReadTokens: ag.TotalCacheReadTokens(), - CacheWriteTokens: ag.TotalCacheWriteTokens(), - Duration: duration, - SessionID: ag.SessionID(), - }) + telemetry.PrintTraceSummary(ag.FilesReviewed(), int64(len(comments)), + ag.TotalInputTokens(), ag.TotalOutputTokens(), ag.TotalTokensUsed(), + ag.TotalCacheReadTokens(), ag.TotalCacheWriteTokens(), duration) } if outputFormat == "json" { diff --git a/cmd/opencodereview/shared_flags.go b/cmd/opencodereview/shared_flags.go index 6698b627..f60981fd 100644 --- a/cmd/opencodereview/shared_flags.go +++ b/cmd/opencodereview/shared_flags.go @@ -109,6 +109,15 @@ func validateReviewOptions(opts *reviewOptions) error { if opts.preview && opts.resume != "" { return fmt.Errorf("--preview and --resume cannot be used together") } + if opts.preview && opts.resumeIncremental { + return fmt.Errorf("--preview and --resume-incremental cannot be used together") + } + if opts.resumeIncremental && opts.resume == "" { + return fmt.Errorf("--resume-incremental requires --resume ") + } + if opts.resumeIncremental && (opts.from == "" || opts.to == "") { + return fmt.Errorf("--resume-incremental is only supported in range mode (--from and --to)") + } if err := validateAudience(opts.audience); err != nil { return err } @@ -172,6 +181,7 @@ func registerReviewFlags(cmd *cobra.Command, opts *reviewOptions) { addDiffFlags(cmd, &opts.from, &opts.to, &opts.commit) cmd.Flags().StringVar(&opts.resume, "resume", "", "resume from a previous review session id") cmd.RegisterFlagCompletionFunc("resume", completeSessionIDs) + cmd.Flags().BoolVar(&opts.resumeIncremental, "resume-incremental", false, "allow resuming range mode when the head commit has moved (reuses unchanged file reviews)") addExcludeFlag(cmd, &opts.excludes) addOutputFlags(cmd, &opts.outputFormat, &opts.audience) addConcurrencyFlags(cmd, &opts.concurrency, &opts.perFileTimeout, &opts.maxTools, &opts.maxGitProcs, &opts.maxTokens, &opts.maxTokensBudget) diff --git a/install.ps1 b/install.ps1 index d042d64a..e74d7e89 100644 --- a/install.ps1 +++ b/install.ps1 @@ -1,7 +1,7 @@ # Install the ocr (Open Code Review) CLI from GitHub releases on Windows. -# irm https://open-codereview.ai/install.ps1 | iex +# irm https://raw.githubusercontent.com/alibaba/open-code-review/main/install.ps1 | iex # Prefer to inspect first: -# irm https://open-codereview.ai/install.ps1 -OutFile install.ps1 +# irm https://raw.githubusercontent.com/alibaba/open-code-review/main/install.ps1 -OutFile install.ps1 # notepad install.ps1 # review, then: .\install.ps1 # Env: OCR_INSTALL_DIR (default $env:LOCALAPPDATA\Programs\ocr), OCR_VERSION (default latest). # Requires PowerShell 5.1+ or PowerShell 7+. diff --git a/install.sh b/install.sh index 8f92a5fa..c85330a7 100755 --- a/install.sh +++ b/install.sh @@ -4,9 +4,9 @@ # Copyright 2026 alibaba/open-code-review Contributors # Install the ocr (Open Code Review) CLI from GitHub releases. -# curl -fsSL https://open-codereview.ai/install.sh | sh +# curl -fsSL https://raw.githubusercontent.com/alibaba/open-code-review/main/install.sh | sh # Prefer to inspect first: -# curl -fsSL https://open-codereview.ai/install.sh -o install.sh +# curl -fsSL https://raw.githubusercontent.com/alibaba/open-code-review/main/install.sh -o install.sh # less install.sh && sh install.sh # Env: OCR_INSTALL_DIR (default /usr/local/bin), OCR_VERSION (default latest). set -eu @@ -23,7 +23,7 @@ main() { os="$(uname -s | tr '[:upper:]' '[:lower:]')" case "$os" in darwin|linux) ;; - *) err "unsupported OS: $os (on Windows use: irm https://open-codereview.ai/install.ps1 | iex)" ;; + *) err "unsupported OS: $os (on Windows use: irm https://raw.githubusercontent.com/alibaba/open-code-review/main/install.ps1 | iex)" ;; esac arch="$(uname -m)" diff --git a/internal/agent/agent.go b/internal/agent/agent.go index c22369e2..0bec165d 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -35,7 +35,6 @@ import ( "github.com/alibaba/open-code-review/internal/telemetry" "github.com/alibaba/open-code-review/internal/tool" - "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" ) @@ -242,7 +241,6 @@ func New(args Args) *Agent { CommentWorkerPool: args.CommentWorkerPool, Session: args.Session, DiffLookup: a.findDiff, - AllDiffs: a.allDiffs, // Non-nil only here: the same Runner serves scan, whose requests must // stay out of the retry report. See newRequestMeta. NewRequestMeta: a.newRequestMeta, @@ -608,7 +606,6 @@ func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error timeout := time.Duration(a.args.ConcurrentTaskTimeout) * time.Minute var dispatched int64 -dispatchLoop: for i := range toDispatch { if toDispatch[i].IsDeleted { continue @@ -658,17 +655,9 @@ dispatchLoop: } } - select { - case sem <- struct{}{}: // acquire semaphore - case <-ctx.Done(): - break dispatchLoop - } - if ctx.Err() != nil { - <-sem // release the slot acquired concurrently with cancellation - break dispatchLoop - } dispatched++ wg.Add(1) + sem <- struct{}{} // acquire semaphore go func(d model.Diff) { fingerprint := reviewItemFingerprint(a.reviewMode(), d) @@ -742,19 +731,16 @@ dispatchLoop: } wg.Wait() - // All subtasks finished — collect comments from the global collector once. - if a.args.CommentWorkerPool != nil { - a.args.CommentWorkerPool.Await() - } - if ctxErr := ctx.Err(); ctxErr != nil { - a.recordContextFailure(ctxErr) - return a.args.CommentCollector.Comments(), ctxErr - } if dispatched == 0 { return a.args.CommentCollector.Comments(), nil } + // All subtasks finished — collect comments from the global collector once. + if a.args.CommentWorkerPool != nil { + a.args.CommentWorkerPool.Await() + } + failed := atomic.LoadInt64(&a.subtaskFailed) reused := int64(0) if a.resumeInfo != nil { @@ -770,22 +756,6 @@ dispatchLoop: return a.args.CommentCollector.Comments(), nil } -func (a *Agent) recordContextFailure(err error) { - if b := a.session.Manifest(); b != nil { - var setErr error - if errors.Is(err, context.DeadlineExceeded) { - // A deadline truncates pending coverage without overriding completed items. - setErr = b.SetPendingFailureCause(session.FailureTimeout, "review deadline exceeded") - } else { - // Explicit cancellation stops the run itself, not just its pending items. - setErr = b.SetRunFailure(session.RunFailureCancelled, "review was cancelled") - } - if setErr != nil { - a.recordWarning("manifest_error", "", setErr.Error()) - } - } -} - func (a *Agent) applyResume(diffs []model.Diff) []model.Diff { resume := a.args.Resume if resume == nil { @@ -877,6 +847,7 @@ func (a *Agent) initManifest() { ConfiguredConcurrency: a.args.MaxConcurrency, RuleConfigSHA256: a.ruleConfigSHA256(), RuntimeConfigSHA256: a.runtimeConfigSHA256(), + PerFileMaxTokens: a.args.Template.MaxTokens, }) } @@ -1340,60 +1311,6 @@ func (a *Agent) executeSubtask(ctx context.Context, d model.Diff) (bool, *subtas return true, nil, nil } -// filterTools defines the two mutually exclusive tools for the review filter. -// The model MUST call exactly one: either report incorrect comments, or approve all. -var filterTools = []llm.ToolDef{ - { - Type: "function", - Function: llm.FunctionDef{ - Name: "report_incorrect_comments", - Description: "Report review comments that this diff proves to be factually wrong: either the code they target is absent from the diff, " + - "or one diff line literally contradicts their central claim. For every id listed you must be able to name that line. " + - "Do not use this for comments you merely find unconvincing, unverifiable, or low-value, nor for comments about memory safety, " + - "concurrency, linkage consistency, unused parameters, or behavioral changes.", - // Field order matters and is load-bearing. Go serializes these - // properties alphabetically, so "analysis" is emitted before - // "comment_ids" and the model reasons before it commits. With the - // order reversed it picks ids first and cannot retract them: replaying - // recorded sessions showed it writing "this is a protected subject, I - // should not remove it" in the later field while the id stayed in the - // earlier one. Do not rename these fields into a different relative - // order. - Parameters: map[string]any{ - "type": "object", - "properties": map[string]any{ - "analysis": map[string]any{ - "type": "array", - "description": "Work through every candidate comment BEFORE deciding. One entry per candidate: its id, " + - "whether its subject hits the protected-subject veto (Step 1) or the value veto (Step 2), " + - "the exact diff line that refutes it if any, and your final call. " + - "Only ids you conclude here as removable may appear in comment_ids.", - "items": map[string]any{"type": "string"}, - }, - "comment_ids": map[string]any{ - "type": "array", - "description": "IDs concluded removable in analysis, e.g. [\"c-0\", \"c-2\"]. Must not be empty.", - "items": map[string]any{"type": "string"}, - }, - }, - "required": []any{"analysis", "comment_ids"}, - }, - }, - }, - { - Type: "function", - Function: llm.FunctionDef{ - Name: "approve_all_comments", - Description: "Keep every review comment. Call this whenever no comment clears the removal bar — including when comments look doubtful, " + - "cannot be verified from the diff alone, or seem minor. This is the expected outcome for most files.", - Parameters: map[string]any{ - "type": "object", - "properties": map[string]any{}, - }, - }, - }, -} - // executeReviewFilter runs the REVIEW_FILTER_TASK to remove comments that are // provably incorrect based solely on the diff. Errors are logged and silently ignored. func (a *Agent) executeReviewFilter(ctx context.Context, d model.Diff, newPath string) { @@ -1438,11 +1355,9 @@ func (a *Agent) executeReviewFilter(ctx context.Context, d model.Diff, newPath s _, llmSpan := telemetry.StartLLMSpan(ctx, a.args.Model) resp, err := a.args.LLMClient.CompletionsWithCtx(reqCtx, llm.ChatRequest{ - Model: a.args.Model, - Messages: messages, - Tools: filterTools, - ToolChoice: "required", - MaxTokens: a.args.Template.CompletionTokenLimit(), + Model: a.args.Model, + Messages: messages, + MaxTokens: a.args.Template.CompletionTokenLimit(), }) duration := time.Since(startTime) if err != nil { @@ -1463,24 +1378,13 @@ func (a *Agent) executeReviewFilter(ctx context.Context, d model.Diff, newPath s rec.SetResponse(resp, duration) a.runner.RecordUsage(resp.Usage) - indices := parseFilterToolCalls(resp.ToolCalls(), len(comments)) - if indices == nil { - indices = parseFilterResponse(resp.Content(), len(comments)) - } + indices := parseFilterResponse(resp.Content(), len(comments)) telemetry.SetAttr(span, "comments.filtered", len(indices)) if len(indices) == 0 { - telemetry.Event(ctx, "review_filter.completed", - attribute.String("file.path", newPath), - attribute.Int("total_comments", len(comments)), - attribute.Int("removed", 0)) return } a.args.CommentCollector.RemoveByPathAndIndices(newPath, indices) - telemetry.Event(ctx, "review_filter.completed", - attribute.String("file.path", newPath), - attribute.Int("total_comments", len(comments)), - attribute.Int("removed", len(indices))) fmt.Fprintf(stdout.Writer(), "[ocr] Review filter removed %d comment(s) for %s\n", len(indices), newPath) } @@ -1503,39 +1407,6 @@ func buildFilterCommentsJSON(comments []model.LlmComment) string { return string(data) } -// parseFilterToolCalls extracts comment indices from the filter tool call response. -// Returns nil if no matching tool call is found, allowing fallback to text-based parsing. -// Returns an empty map (non-nil) for approve_all_comments or an empty comment_ids list. -func parseFilterToolCalls(calls []llm.ToolCall, total int) map[int]struct{} { - var indices map[int]struct{} - for _, call := range calls { - switch call.Function.Name { - case "approve_all_comments": - if indices == nil { - indices = make(map[int]struct{}) - } - case "report_incorrect_comments": - var args struct { - CommentIDs []string `json:"comment_ids"` - } - if err := json.Unmarshal([]byte(call.Function.Arguments), &args); err != nil { - fmt.Fprintf(stdout.Writer(), "[ocr] Review filter: failed to parse tool call arguments: %v\n", err) - continue - } - if indices == nil { - indices = make(map[int]struct{}) - } - for _, id := range args.CommentIDs { - var idx int - if _, err := fmt.Sscanf(id, "c-%d", &idx); err == nil && idx >= 0 && idx < total { - indices[idx] = struct{}{} - } - } - } - } - return indices -} - // parseFilterResponse extracts comment indices from the LLM filter response. // Returns a set of 0-based indices. Invalid IDs or out-of-range indices are ignored. func parseFilterResponse(raw string, total int) map[int]struct{} { @@ -1852,14 +1723,6 @@ func orderedToolParameters(raw json.RawMessage) ([]orderedToolParameter, bool) { return params, true } -// allDiffs exposes the reviewed diff set for cross-file comment re-filing. -// It is read-only and safe to call from the per-file subtask goroutines: every -// mutation of a.diffs (filterDiffs, filterLargeDiffs) completes before dispatch -// begins, so the slice is stable for the rest of the run. -func (a *Agent) allDiffs() []model.Diff { - return a.diffs -} - // findDiff returns the Diff for the given file path, or nil if not found. func (a *Agent) findDiff(path string) *model.Diff { for i := range a.diffs { diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 33968295..a6dbd6a9 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -223,96 +223,6 @@ func TestParseFilterResponse(t *testing.T) { } } -func TestParseFilterToolCalls(t *testing.T) { - tests := []struct { - name string - calls []llm.ToolCall - total int - wantSet map[int]struct{} - }{ - { - name: "no tool calls", - calls: nil, - total: 5, - wantSet: nil, - }, - { - name: "report_incorrect_comments with IDs", - calls: []llm.ToolCall{{ - Function: llm.FunctionCall{ - Name: "report_incorrect_comments", - Arguments: `{"comment_ids": ["c-0", "c-2"]}`, - }, - }}, - total: 5, - wantSet: map[int]struct{}{0: {}, 2: {}}, - }, - { - name: "approve_all_comments returns empty map", - calls: []llm.ToolCall{{ - Function: llm.FunctionCall{ - Name: "approve_all_comments", - Arguments: `{}`, - }, - }}, - total: 5, - wantSet: map[int]struct{}{}, - }, - { - name: "ignores non-matching tool names", - calls: []llm.ToolCall{{ - Function: llm.FunctionCall{ - Name: "other_tool", - Arguments: `{"comment_ids": ["c-0"]}`, - }, - }}, - total: 5, - wantSet: nil, - }, - { - name: "out-of-range indices ignored", - calls: []llm.ToolCall{{ - Function: llm.FunctionCall{ - Name: "report_incorrect_comments", - Arguments: `{"comment_ids": ["c-0", "c-10"]}`, - }, - }}, - total: 5, - wantSet: map[int]struct{}{0: {}}, - }, - { - name: "invalid JSON arguments falls through", - calls: []llm.ToolCall{{ - Function: llm.FunctionCall{ - Name: "report_incorrect_comments", - Arguments: `not json`, - }, - }}, - total: 5, - wantSet: nil, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := parseFilterToolCalls(tt.calls, tt.total) - if tt.wantSet == nil { - if got != nil { - t.Errorf("expected nil, got %v", got) - } - return - } - if len(got) != len(tt.wantSet) { - t.Fatalf("len = %d, want %d; got %v", len(got), len(tt.wantSet), got) - } - for idx := range tt.wantSet { - if _, ok := got[idx]; !ok { - t.Errorf("missing index %d in result", idx) - } - } - }) - } -} - func TestExtFromPath(t *testing.T) { a := New(Args{}) diff --git a/internal/agent/coverage_test.go b/internal/agent/coverage_test.go index e408c71e..5bff00aa 100644 --- a/internal/agent/coverage_test.go +++ b/internal/agent/coverage_test.go @@ -269,19 +269,11 @@ func TestExecuteReviewFilter_RemovesComments(t *testing.T) { tmpDir := t.TempDir() sess := session.New(tmpDir, "main", "test", session.SessionOptions{ReviewMode: "diff"}) + filterResp := `["c-1"]` client := &fakeAgentClient{ responses: []*llm.ChatResponse{{ Choices: []llm.Choice{{ - Message: llm.ResponseMessage{ - ToolCalls: []llm.ToolCall{{ - ID: "call_1", - Type: "function", - Function: llm.FunctionCall{ - Name: "report_incorrect_comments", - Arguments: `{"comment_ids":["c-1"]}`, - }, - }}, - }, + Message: llm.ResponseMessage{Content: &filterResp}, }}, Usage: &llm.UsageInfo{PromptTokens: 10, CompletionTokens: 5}, }}, @@ -426,19 +418,11 @@ func TestExecuteReviewFilter_SkipFilter(t *testing.T) { tmpDir := t.TempDir() sess := session.New(tmpDir, "main", "test", session.SessionOptions{ReviewMode: "diff"}) + filterResp := `["c-1"]` client := &fakeAgentClient{ responses: []*llm.ChatResponse{{ Choices: []llm.Choice{{ - Message: llm.ResponseMessage{ - ToolCalls: []llm.ToolCall{{ - ID: "call_1", - Type: "function", - Function: llm.FunctionCall{ - Name: "report_incorrect_comments", - Arguments: `{"comment_ids":["c-1"]}`, - }, - }}, - }, + Message: llm.ResponseMessage{Content: &filterResp}, }}, Usage: &llm.UsageInfo{PromptTokens: 10, CompletionTokens: 5}, }}, @@ -791,21 +775,11 @@ func TestExecuteReviewFilter_WithTimeout(t *testing.T) { tmpDir := t.TempDir() sess := session.New(tmpDir, "main", "test", session.SessionOptions{ReviewMode: "diff"}) + filterResp := `[]` client := &fakeAgentClient{ responses: []*llm.ChatResponse{{ - Choices: []llm.Choice{{ - Message: llm.ResponseMessage{ - ToolCalls: []llm.ToolCall{{ - ID: "call_1", - Type: "function", - Function: llm.FunctionCall{ - Name: "approve_all_comments", - Arguments: `{}`, - }, - }}, - }, - }}, - Usage: &llm.UsageInfo{PromptTokens: 5, CompletionTokens: 2}, + Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: &filterResp}}}, + Usage: &llm.UsageInfo{PromptTokens: 5, CompletionTokens: 2}, }}, } diff --git a/internal/agent/identity.go b/internal/agent/identity.go index 44d3f19d..8089b555 100644 --- a/internal/agent/identity.go +++ b/internal/agent/identity.go @@ -111,6 +111,7 @@ func (a *Agent) runIdentity() session.RunIdentity { Mode: a.manifestMode(), SourceArtifactSHA256: a.sourceArtifactSHA256(), RuleConfigSHA256: a.ruleConfigSHA256(), + PerFileMaxTokens: a.args.Template.MaxTokens, } if raw := a.repoRemoteIdentity; raw != "" { sum := sha256.Sum256([]byte(raw)) diff --git a/internal/agent/manifest_integration_test.go b/internal/agent/manifest_integration_test.go index a1ca52d9..5a823e35 100644 --- a/internal/agent/manifest_integration_test.go +++ b/internal/agent/manifest_integration_test.go @@ -9,7 +9,6 @@ import ( "fmt" "strings" "sync" - "sync/atomic" "testing" "time" @@ -43,26 +42,6 @@ func (manifestFlowClient) CompletionsWithCtx(_ context.Context, req llm.ChatRequ } } -type cancellationFlowClient struct { - blocked chan struct{} - once sync.Once -} - -func (c *cancellationFlowClient) CompletionsWithCtx(ctx context.Context, req llm.ChatRequest) (*llm.ChatResponse, error) { - var prompt string - for _, message := range req.Messages { - if text, ok := message.Content.(string); ok { - prompt += text - } - } - if strings.Contains(prompt, "blocked.go") { - c.once.Do(func() { close(c.blocked) }) - <-ctx.Done() - return nil, ctx.Err() - } - return agentTaskDoneResponse(), nil -} - func newManifestFlowAgent(t *testing.T, diffs []model.Diff, resume *session.ResumeState) *Agent { t.Helper() return newManifestFlowAgentWithClient(t, diffs, resume, manifestFlowClient{}) @@ -189,101 +168,6 @@ func TestManifestFlowCompleteAndPartial(t *testing.T) { }) } -func TestManifestFlowCancellationPersistsResumableSession(t *testing.T) { - done := model.Diff{OldPath: "done.go", NewPath: "done.go", Diff: "+done", Insertions: 1} - blocked := model.Diff{OldPath: "blocked.go", NewPath: "blocked.go", Diff: "+blocked", Insertions: 1} - pending := model.Diff{OldPath: "pending.go", NewPath: "pending.go", Diff: "+pending", Insertions: 1} - client := &cancellationFlowClient{blocked: make(chan struct{})} - a := newManifestFlowAgentWithClient(t, []model.Diff{done, blocked, pending}, nil, client) - a.args.MaxConcurrency = 1 - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - dispatchErr := make(chan error, 1) - go func() { - _, err := a.dispatchSubtasks(ctx) - dispatchErr <- err - }() - - select { - case <-client.blocked: - cancel() - case <-time.After(5 * time.Second): - t.Fatal("blocked review did not start") - } - if err := <-dispatchErr; !errors.Is(err, context.Canceled) { - t.Fatalf("dispatch error = %v, want context.Canceled", err) - } - - manifest := finishManifestFlow(t, a) - if manifest.RunFailure == nil || manifest.RunFailure.Classification != session.RunFailureCancelled { - t.Fatalf("run failure = %+v, want cancelled", manifest.RunFailure) - } - if len(manifest.Coverage.Completed) != 1 || len(manifest.Coverage.Failed) != 2 { - t.Fatalf("coverage = %+v, want one completed and two failed", manifest.Coverage) - } - for _, item := range manifest.Coverage.Failed { - if item.Classification != session.FailureCancelled { - t.Fatalf("failed item = %+v, want cancelled", item) - } - } - - state, err := session.LoadReviewResumeState(a.args.RepoDir, a.session.SessionID) - if err != nil { - t.Fatalf("load cancelled session: %v", err) - } - identity := session.RunIdentity{ - Mode: manifest.Input.Mode, - SourceArtifactSHA256: manifest.Input.SourceArtifactSHA256, - RuleConfigSHA256: manifest.Execution.RuleConfigSHA256, - RepositorySHA256: manifest.Repository.IdentitySHA256, - } - if err := state.ValidateResume(session.ResumeRequest{ - Identity: identity, - Provider: manifest.Execution.Provider, - Model: manifest.Execution.Model, - }); err != nil { - t.Fatalf("cancelled session should remain resumable: %v", err) - } - if _, ok := state.ReusableItem(reviewItemFingerprint(a.reviewMode(), done)); !ok { - t.Fatal("completed item is not reusable after cancellation") - } - if _, ok := state.ReusableItem(reviewItemFingerprint(a.reviewMode(), blocked)); ok { - t.Fatal("cancelled item must be reviewed again") - } -} - -func TestManifestFlowCancellationBeforeDispatchStartsNoSubtask(t *testing.T) { - pending := model.Diff{OldPath: "blocked.go", NewPath: "blocked.go", Diff: "+blocked", Insertions: 1} - client := &cancellationFlowClient{blocked: make(chan struct{})} - a := newManifestFlowAgentWithClient(t, []model.Diff{pending}, nil, client) - - ctx, cancel := context.WithCancel(context.Background()) - cancel() - if _, err := a.dispatchSubtasks(ctx); !errors.Is(err, context.Canceled) { - t.Fatalf("dispatch error = %v, want context.Canceled", err) - } - if got := atomic.LoadInt64(&a.subtaskFailed); got != 0 { - t.Fatalf("subtask failures = %d, want 0 because no subtask should start", got) - } - select { - case <-client.blocked: - t.Fatal("LLM client was called after cancellation") - default: - } - - manifest := finishManifestFlow(t, a) - if manifest.RunFailure == nil || manifest.RunFailure.Classification != session.RunFailureCancelled { - t.Fatalf("run failure = %+v, want cancelled", manifest.RunFailure) - } - if len(manifest.Coverage.Completed) != 0 || len(manifest.Coverage.Failed) != 1 { - t.Fatalf("coverage = %+v, want one cancelled item and no completed items", manifest.Coverage) - } - if got := manifest.Coverage.Failed[0].Classification; got != session.FailureCancelled { - t.Fatalf("failure classification = %q, want %q", got, session.FailureCancelled) - } -} - func TestManifestFlowRunInputFailureIsPersisted(t *testing.T) { t.Setenv("HOME", t.TempDir()) repoDir := t.TempDir() diff --git a/internal/config/allowlist/allowed_ext_test.go b/internal/config/allowlist/allowed_ext_test.go index 4939318e..48cbd791 100644 --- a/internal/config/allowlist/allowed_ext_test.go +++ b/internal/config/allowlist/allowed_ext_test.go @@ -145,16 +145,6 @@ func TestIsExcludedPath(t *testing.T) { {"julia test nested", "MyPkg/test/unit/foo.jl", true}, {"julia non-test", "src/model.jl", false}, - // Swift test files - {"swift Tests suffix", "MyAppTests/UserTests.swift", true}, - {"swift Tests suffix nested", "Tests/AppTests/UserTests.swift", true}, - {"swift UITests suffix", "MyAppUITests/LaunchTests.swift", true}, - {"swift Test suffix", "MyAppTests/UserTest.swift", true}, - {"swift Test dir helper", "Tests/AppTests/Mocks/MockService.swift", true}, - {"swift tests dir lowercase", "tests/AppTests/Helpers/Helper.swift", true}, - {"swift non-test", "Sources/App/User.swift", false}, - {"swift helper with test in name", "Sources/App/TestSupport.swift", false}, - // Haskell test files {"haskell test directory", "test/Parser.hs", true}, {"haskell nested test directory", "packages/core/test/unit/Parser.hs", true}, diff --git a/internal/config/allowlist/default_exclude_patterns.json b/internal/config/allowlist/default_exclude_patterns.json index 1eb43a1e..637d6616 100644 --- a/internal/config/allowlist/default_exclude_patterns.json +++ b/internal/config/allowlist/default_exclude_patterns.json @@ -29,8 +29,5 @@ "**/*.gen.go", "**/*.pb.go", "**/*.pb.cc", - "**/*.pb.h", - "**/*Test.swift", - "**/*Tests.swift", - "**/Tests/**/*.swift" + "**/*.pb.h" ] diff --git a/internal/config/rules/rule_docs/swift.md b/internal/config/rules/rule_docs/swift.md deleted file mode 100644 index 528e99fa..00000000 --- a/internal/config/rules/rule_docs/swift.md +++ /dev/null @@ -1,105 +0,0 @@ -#### Swift Review Principles -> Favor precision over recall: report only defects likely real in changed code and reachable execution paths. Prioritize crashes, data corruption, security issues, privacy issues, and concurrency bugs. Do not report style preferences. - -Before reporting non-local behavior, use `file_read` and `code_search` to verify ownership, callers, synchronization, lifecycle, and input sources. Do not infer threading, retain cycles, or error contracts only from names or types. Do not duplicate compiler, SwiftLint, or Xcode analyzer findings unless the diff creates concrete correctness impact. - -#### Optionals and Runtime Failures -- Force unwrap, force cast, or `try!` on runtime-derived values (user input, network responses, persistence, decoding, external state) where failure is reachable and not handled. -- Implicitly unwrapped optionals outside controlled framework lifecycle patterns where access can occur before initialization or after invalidation. -- Optional handling that converts required failure into silent incorrect behavior, missing data, or invalid state. - -#### Memory Ownership and ARC -- Escaping closures stored by an object that strongly capture that same object, creating a retain cycle. -- `[unowned]` captures in escaping closures where object lifetime is not guaranteed until execution. -- Delegate, observer, callback, timer, or task relationships that create ownership cycles or continue work after owner destruction. -- Combine subscriptions capturing `self` strongly inside an owner of the cancellable when it prevents expected deallocation. -- Async streams, notifications, timers, or subscriptions started without lifecycle cleanup when they continue after dismissal/deallocation. - -#### Error Handling -- Throwing operations or `Result` failures ignored, replaced with success values, or hidden when failure changes behavior or data correctness. -- `try?` removing required failure information where callers need failure distinction. -- Empty error handling blocks suppressing failures affecting integrity, security, or user-visible behavior. -- Error wrapping removing typed error information required by callers. -- `fatalError`/`preconditionFailure` used for recoverable runtime failures instead of typed propagation. - -#### Swift Concurrency and Isolation -- Mutable state accessed across actor boundaries without isolation or synchronization where concurrent access is possible. -- Non-`Sendable` values crossing isolation boundaries where races or unsafe assumptions are possible. -- `@unchecked Sendable` or `nonisolated(unsafe)` introduced without a proven thread-safety invariant. -- Actor-isolated state accessed from callbacks, delegates, or closures without preserving isolation. -- Fire-and-forget tasks that outlive owners, cannot be cancelled, or continue side effects after lifecycle ends. -- Detached tasks used where inherited actor context, priority, cancellation, or isolation is required. -- Async work ignoring cancellation and continuing expensive computation or side effects. -- Continuation wrappers that can resume multiple times, never resume, or resume after ownership/lifecycle invalidation. -- Locks or synchronous waits used across `await` boundaries. -- Independent async operations introduced sequentially causing measurable user-visible latency regression. - -#### SwiftUI State and Lifecycle -- View-owned reference state recreated across renders because ownership/lifetime is incorrect. -- Dynamic collections using unstable identity causing incorrect row reuse or state association. -- Side effects executed from `body` or computed properties causing repeated execution. -- Lifecycle async work continuing after disappearance when cancellation ownership is required. -- `.task(id:)` missing where replaced inputs can allow stale results to overwrite newer state. -- UI state mutated outside required main actor isolation when concurrent updates are possible. -- Lifecycle effects duplicated or misattributed across remount, presentation, or dismissal paths. -- User-visible strings added or changed without localization coverage. - -#### Persistence and Data Integrity (SwiftData / Core Data) -- Persistence writes leaving stored state partially updated or inconsistent after failure. -- Schema or relationship changes without compatible migration handling for existing data. -- Relationship configuration changes causing orphaned objects, invalid references, or incorrect delete behavior. -- `@Query`/fetch predicates or sort descriptors matching incorrect data or causing avoidable expensive fetches. -- Cached or persisted values treated as authoritative when they can become stale and affect correctness. - -#### Health and Privacy Data -- HealthKit access performed without required authorization handling or safe fallback behavior. -- Health or sensitive data written to logs, analytics, insecure storage, or plaintext persistence. -- Health claims introduced without required supporting source or compliance basis. -- Health queries, observer queries, or background delivery registrations missing lifecycle handling, causing missed updates or unnecessary resource use. - -#### Purchases and Entitlements -- Purchase, restore, or entitlement state failing to handle pending, offline, or verification outcomes. -- Transaction listeners missing, incorrectly scoped, or failing to consume verified transactions. -- Paywall or entitlement UI using stale state instead of canonical entitlement state. -- Trial, restore, or purchase error paths granting or revoking entitlement incorrectly. - -#### Combine and Reactive Streams -- Combine subscriptions causing ownership cycles or continuing after intended lifecycle. -- UI updates delivered without required scheduler guarantees (`receive(on:)`/equivalent), causing incorrect thread execution. -- Expensive upstream work executed on inappropriate schedulers where it blocks UI or causes latency. -- Streams without cancellation/backpressure handling where unbounded work or memory growth is possible. - -#### Networking -- Authentication tokens, credentials, or sensitive data exposed through logs, storage, or requests. -- Signed/authenticated URLs with bypassed expiry, validation, or authorization checks. -- Retry logic causing request storms or missing backoff for transient failures. -- Cache handling serving stale or unauthorized responses. -- Disabled transport protections or weakened certificate validation where an existing security boundary depends on it. -- Client-controlled identity, authorization, or payment values trusted without server validation. - -#### Web Views, Deep Links, and External Input -- WKWebView JavaScript bridges accepting unvalidated messages or exposing privileged actions. -- Navigation handlers allowing untrusted URLs or schemes without validation. -- Deep-link inputs changing authenticated state or sensitive actions without validation. - -#### Performance and Resource Usage -- Expensive synchronous work on the main actor/thread blocking interaction. -- Repeated expensive work on frequently executed paths causing measurable regressions. -- Unbounded memory growth from collections, caches, tasks, streams, or retained objects. -- Inefficient algorithms on demonstrably large collections causing user-visible slowdown. - -#### Security -- Secrets, credentials, tokens, private keys, or sensitive user data added to source, logs, fixtures, or insecure storage. -- User-controlled input passed into executable contexts, unsafe URLs, queries, or commands without validation. - -#### Unsafe Interoperability -- Unsafe pointer, buffer, or memory APIs used without guaranteed lifetime or bounds. -- Objective-C/C bridging violating ownership, nullability, or lifetime assumptions. - -#### Testing Correctness -- Tests relying on arbitrary sleeps or timing delays instead of async expectations or direct awaiting. -- Tests not exercising changed behavior paths where regressions are likely. -- Tests sharing mutable global state causing isolation failures. -- Async tests leaving tasks running after completion. -- Assertions that cannot fail for the regression they intend to detect. -- Tests depending on uncontrolled environment state (network, time, locale, global persistence) where isolation is required. diff --git a/internal/config/rules/system_rules.json b/internal/config/rules/system_rules.json index a469d147..8646103a 100644 --- a/internal/config/rules/system_rules.json +++ b/internal/config/rules/system_rules.json @@ -34,7 +34,6 @@ "**/*.bicep": "bicep.md", "**/*.nix": "nix.md", "**/*.{hs,lhs}": "haskell.md", - "**/*.{nim,nims,nimble}": "nim.md", - "**/*.swift": "swift.md" + "**/*.{nim,nims,nimble}": "nim.md" } } diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index 6afd8a3a..34e95608 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -115,9 +115,6 @@ func TestResolve_DefaultRules(t *testing.T) { {"src/parser.nim", "Memory and Lifetime Safety"}, {"scripts/build.nims", "Memory and Lifetime Safety"}, {"project.nimble", "Memory and Lifetime Safety"}, - {"Sources/App/ContentView.swift", "Swift Review Principles"}, - {"MyApp/Models/UserStore.swift", "Swift Review Principles"}, - {"ChattyFit/ChattyFit/Views/WorkoutSessionView.swift", "SwiftUI State and Lifecycle"}, } for _, tt := range tests { @@ -141,8 +138,8 @@ func TestResolve_FallbackToDefault(t *testing.T) { "readme.md", "docs/architecture.txt", "Makefile", + "ios/ViewController.swift", "ios/ViewController.m", - "ios/ViewController.mm", } for _, path := range paths { diff --git a/internal/config/template/prompts/review_filter_task_system.md b/internal/config/template/prompts/review_filter_task_system.md index 37cc2213..bff2f0dd 100644 --- a/internal/config/template/prompts/review_filter_task_system.md +++ b/internal/config/template/prompts/review_filter_task_system.md @@ -1,12 +1,7 @@ You are a fact-checker for code review comments. -These review comments come from an Agent that could invoke tools to read the full codebase. You can see only a single diff. Anything you cannot see, the Agent may well have seen. +These review comments come from an Agent that can invoke tools to obtain the full code context. You can currently only see the code diff. -Your task is narrow: remove only the comments that this diff **proves** to be factually wrong. You are not judging whether a comment is useful, well-prioritized, or worth a reviewer's time. +Therefore, your task is NOT to verify whether all review comments are correct, but to **filter out only those review comments that can be confirmed as incorrect based solely on the current diff**. -The two mistakes available to you are not equally bad: - -- Keeping an incorrect comment costs a reviewer a few seconds of attention. -- Removing a correct comment silently destroys a real finding. It never reaches anyone, and nobody learns that it was dropped. - -So when your evidence falls short of proof, approve. "Suspicious", "I cannot verify this", "low value", "the flagged code looks fine to me", and "I would not have raised this" all mean approve. +For review comments whose correctness cannot be determined from the diff alone, even if you find them suspicious, you should let them pass — because the Agent may have access to context that you cannot see. diff --git a/internal/config/template/prompts/review_filter_task_user.md b/internal/config/template/prompts/review_filter_task_user.md index d4858a5d..d2b64725 100644 --- a/internal/config/template/prompts/review_filter_task_user.md +++ b/internal/config/template/prompts/review_filter_task_user.md @@ -1,69 +1,34 @@ ### Task -Below is one file's diff and a set of review comments about it. Identify only the comments that this diff **proves** to be wrong. +Given a code diff and a set of review comments, identify those that are **provably incorrect based solely on the diff**. -Your default answer is to approve everything. On most files that is the correct answer. +### Evaluation Principles -### The only two grounds for removal +**Core principle: You need to falsify, not verify.** -**Ground A — the comment targets code that is not in this diff.** +- ✅ Should flag: The diff contains **direct counter-evidence** that proves the key claim of the review comment is wrong +- ❌ Should NOT flag: The review comment references context not visible in the diff (may have been obtained by the Agent via tools) +- ❌ Should NOT flag: You merely "cannot verify" but also cannot disprove the review comment -The symbol, statement, or construct the comment describes appears nowhere in the diff below. Typical shapes: +### Evaluation Method -- it discusses the body of a function, on a file that only declares or references it -- it discusses host-language logic on a file that holds none — a query, build, markup, or configuration file -- it claims code was removed, or an error is handled, and this diff contains no such change +For each review comment, perform the following two steps: -**Ground B — a specific line of the diff literally contradicts the comment's central claim.** +#### Step 1: Fact Check (Veto Rule) -The comment asserts a concrete fact and the diff shows the opposite in plain text. The contradiction must be readable straight off the diff, not derived through a chain of reasoning. Typical shapes: +- Only verify claims that are verifiable within the diff +- Only determine a comment as incorrect when the diff provides counter-evidence. **If a claim involves information outside the diff (such as logic in other files, business semantics, runtime behavior), and the diff contains no evidence contradicting it, do not make a determination.** -- it says an identifier is unused, and the diff shows it in use -- it says a check, assertion, or branch is missing, and the diff contains it -- it says a value is hardcoded, and the diff shows it read from a variable -- it says something is declared twice and shadows an outer name, and the diff holds exactly one declaration -- it states a condition or type relationship that the diff's own text refutes +⚠️ Fact check fails → Immediately determine as incorrect, skip Step 2. -If you cannot point to the specific diff line that establishes Ground A or Ground B, approve the comment. +#### Step 2: Issue Classification -### Protected subjects — never remove +After confirming that the facts visible in the diff are accurate, determine whether the description contains a **significant deviation that can be disproved from the diff**: -These are vetoes, applied before you judge correctness at all. Whatever you conclude about the comment, approve it if its subject is: +- Does it misidentify clearly normal code in the diff as a defect? +- Does it attribute behavior visible in the diff in a way that contradicts the code? -- **Memory safety** — allocation size, buffer length, index bounds, off-by-one, use-after-free, null dereference -- **Concurrency** — locks and lock modes, atomics, data races, synchronization arguments that are not honored -- **Linkage and declaration consistency** — `static` versus non-`static`, a declaration that disagrees with its definition, missing `extern` -- **Behavioral or compatibility change** — a message, field, status, or default that the old code produced and the new code no longer does; an altered error path; a counter whose update moved to a different point in the lifecycle -- **A parameter the function accepts and never uses** - -These are the categories where a wrongly removed comment is most expensive, and where your own confidence is least trustworthy — including confidence that the language, compiler, or runtime does not behave the way the comment claims. On a protected subject you do not get to be confident. Approve. - -### Not grounds for removal - -- The comment is about style, formatting, naming, blank lines, the wording of a code comment, or readability — **provided what it states is true**. Low value is not incorrectness, and filtering by value is not your job. -- The comment reasons about runtime behavior, business semantics, or code in files you cannot see. The Agent had access you do not. -- You disagree with its recommendation, or you consider the flagged code acceptable as written. -- You cannot confirm it. Unverifiable is not incorrect. -- It identifies a real problem but quotes a slightly wrong line or snippet. Judge the claim, not the citation. -- It is imprecise in passing while its central claim holds. - -### Method - -Run these steps in order for every comment. Stop at the first step that applies — do not revisit a decision a later step would have made differently. - -**Step 1 — protected-subject veto.** Is the comment's subject one of the protected categories above (memory safety, concurrency, linkage and declaration consistency, behavioral or compatibility change, an unused parameter)? → **approve and stop.** Do not assess whether it is correct. This veto outranks Ground A and Ground B: a comment on a protected subject stays even when you are confident it is wrong. - -**Step 2 — value veto.** Is the comment about style, formatting, naming, blank lines, the wording of a code comment, or readability, and is what it states true of this diff? → **approve and stop.** Its low value is not your concern. - -**Step 3 — Ground A.** Is the code it describes absent from the diff? → **remove it.** - -**Step 4 — Ground B.** Is there one diff line that literally contradicts its central claim, requiring no chain of reasoning to see? → **remove it.** Steps 3 and 4 are not optional: once a comment reaches them and qualifies, report it. - -Before concluding a contradiction in Step 4, search the whole diff for what the comment describes — not only the snippet it quoted. A comment that cites the wrong line while describing something the diff does contain is correct, and stays. - -**Step 5 —** approve. - -Reaching Step 4 and needing more than a single inferential step to reach the contradiction means there is none. Approve. +⚠️ Only determine as incorrect when the diff can directly prove the description is wrong. ### Code Diff @@ -77,7 +42,14 @@ Reaching Step 4 and needing more than a single inferential step to reach the con ### Output -You must call exactly one tool: +Return all incorrect review comment IDs directly, without any explanation. Use JSON array format: -- `report_incorrect_comments` — only for comments meeting Ground A or Ground B, and only if you could name the diff line that disproves each one. -- `approve_all_comments` — in every other case, including when comments look doubtful, unverifiable, or minor. +```json +["id-xxx", "id-yyy"] +``` + +If there are no review comments that can be confirmed as incorrect, return an empty array: + +```json +[] +``` diff --git a/internal/diff/relocate_across_files_test.go b/internal/diff/relocate_across_files_test.go deleted file mode 100644 index 223a52f8..00000000 --- a/internal/diff/relocate_across_files_test.go +++ /dev/null @@ -1,127 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// Copyright 2026 alibaba/open-code-review Contributors - -package diff - -import ( - "testing" - - "github.com/alibaba/open-code-review/internal/model" -) - -// headerDiff declares the function; sourceDiff implements it. This is the split -// that produces mis-filed comments in practice: the Agent reviews the header, -// reads the source through file_read_diff, and files a comment about the body -// against the header. -const headerDiff = `diff --git a/src/span.h b/src/span.h ---- a/src/span.h -+++ b/src/span.h -@@ -10,3 +10,4 @@ - void span_set_text(span_t * span, const char * text); -+void span_set_text_fmt(span_t * span, const char * fmt, ...); -` - -const sourceDiff = `diff --git a/src/span.c b/src/span.c ---- a/src/span.c -+++ b/src/span.c -@@ -40,4 +40,7 @@ - void span_set_text_fmt(span_t * span, const char * fmt, ...) - { -+ char * text = span_vfmt(fmt, args); -+ if(text == NULL) return; -+ va_end(args); - } -` - -func diffsFixture() []model.Diff { - return []model.Diff{ - {NewPath: "src/span.h", OldPath: "src/span.h", Diff: headerDiff}, - {NewPath: "src/span.c", OldPath: "src/span.c", Diff: sourceDiff}, - } -} - -func TestRelocateAcrossFiles_RefilesToImplementation(t *testing.T) { - cm := &model.LlmComment{ - Path: "src/span.h", - Content: "va_end is skipped on the early return", - ExistingCode: "\tif(text == NULL) return;\n\tva_end(args);", - } - - got, ok := RelocateAcrossFiles(cm, diffsFixture()) - if !ok { - t.Fatal("expected a unique hit in src/span.c") - } - if got != "src/span.c" || cm.Path != "src/span.c" { - t.Fatalf("path = %q, cm.Path = %q; want src/span.c for both", got, cm.Path) - } - // Line numbers must move with the path, or the comment is re-filed onto the - // right file while still pointing at the wrong line. - if cm.StartLine <= 0 || cm.EndLine < cm.StartLine { - t.Fatalf("StartLine/EndLine = %d/%d; want a resolved range", cm.StartLine, cm.EndLine) - } -} - -func TestRelocateAcrossFiles_DeclinesWhenAmbiguous(t *testing.T) { - // The same excerpt in two files: re-filing onto either one would just swap - // one wrong location for another, so the comment must be left alone. - dup := `diff --git a/src/other.c b/src/other.c ---- a/src/other.c -+++ b/src/other.c -@@ -1,2 +1,4 @@ -+ char * text = span_vfmt(fmt, args); -+ if(text == NULL) return; -+ va_end(args); - } -` - diffs := append(diffsFixture(), model.Diff{NewPath: "src/other.c", OldPath: "src/other.c", Diff: dup}) - cm := &model.LlmComment{ - Path: "src/span.h", - ExistingCode: "\tif(text == NULL) return;\n\tva_end(args);", - } - - if _, ok := RelocateAcrossFiles(cm, diffs); ok { - t.Fatal("expected no verdict when the excerpt matches more than one file") - } - if cm.Path != "src/span.h" || cm.StartLine != 0 || cm.EndLine != 0 { - t.Fatalf("cm mutated on decline: path=%q start=%d end=%d", cm.Path, cm.StartLine, cm.EndLine) - } -} - -func TestRelocateAcrossFiles_DeclinesWhenAbsent(t *testing.T) { - cm := &model.LlmComment{ - Path: "src/span.h", - ExistingCode: "int nothing_here = 0;", - } - if _, ok := RelocateAcrossFiles(cm, diffsFixture()); ok { - t.Fatal("expected no hit for code absent from every diff") - } - if cm.Path != "src/span.h" { - t.Fatalf("cm.Path mutated to %q on decline", cm.Path) - } -} - -func TestRelocateAcrossFiles_SkipsOwnFileAndEmptyInputs(t *testing.T) { - // A comment already resolvable in its own file must not be re-filed: the - // caller only reaches here after same-file resolution failed, but the - // function still has to exclude cm.Path so a self-match cannot masquerade - // as a cross-file hit. - cm := &model.LlmComment{ - Path: "src/span.c", - ExistingCode: "\tva_end(args);", - } - if _, ok := RelocateAcrossFiles(cm, diffsFixture()); ok { - t.Fatal("expected the comment's own file to be excluded from the search") - } - - for name, arg := range map[string]*model.LlmComment{ - "nil comment": nil, - "empty ExistingCode": {Path: "src/span.h"}, - } { - if _, ok := RelocateAcrossFiles(arg, diffsFixture()); ok { - t.Fatalf("%s: expected no hit", name) - } - } - if _, ok := RelocateAcrossFiles(&model.LlmComment{ExistingCode: "x"}, nil); ok { - t.Fatal("empty diff set: expected no hit") - } -} diff --git a/internal/diff/resolver.go b/internal/diff/resolver.go index 8443dfd7..d776b259 100644 --- a/internal/diff/resolver.go +++ b/internal/diff/resolver.go @@ -72,72 +72,6 @@ func ResolveComment(cm *model.LlmComment, d *model.Diff) bool { return resolveFromFileContent(d, cm) } -// RelocateAcrossFiles handles the comment whose ExistingCode belongs to a -// different file than the one it was filed against. -// -// The reviewing Agent reads related files through file_read_diff, so it can -// describe code from a file other than the one under review and still file the -// comment against the file under review — typically a declaration/implementation -// split, where the comment lands on the header and its code lives in the source -// file. ResolveComment then fails, and the LLM re-location that follows is given -// only the wrong file's diff and a prompt that demands a code block back, so it -// answers with whatever token in that diff looks closest. That overwrites the -// one piece of evidence pointing at the real code, and the comment ends up -// looking located while pointing at an unrelated line. -// -// So this runs first, and without a model: ExistingCode is a verbatim excerpt, -// which makes finding its true home plain string matching over the diffs that -// are already in memory. On a unique hit the comment is re-filed — Path, -// StartLine and EndLine all move together — and it returns that path. -// -// Zero hits and multiple hits both decline, leaving cm untouched: the same -// boilerplate can legitimately appear in several files, and guessing between -// them would trade one wrong location for another. Callers should treat a -// false return as "still unlocated" rather than as an error. -// -// cm.Path is skipped because its own file has already been tried, and probing -// happens on a copy so a failed candidate cannot leave line numbers behind. -func RelocateAcrossFiles(cm *model.LlmComment, diffs []model.Diff) (string, bool) { - if cm == nil || cm.ExistingCode == "" || len(diffs) == 0 { - return "", false - } - - type hit struct { - path string - start, end int - } - var hits []hit - - for i := range diffs { - d := &diffs[i] - if d.NewPath == cm.Path || d.OldPath == cm.Path { - continue - } - probe := *cm - probe.StartLine, probe.EndLine = 0, 0 - if !ResolveComment(&probe, d) { - continue - } - path := d.NewPath - if path == "" { - path = d.OldPath - } - hits = append(hits, hit{path: path, start: probe.StartLine, end: probe.EndLine}) - if len(hits) > 1 { - // Ambiguous already; no verdict can come from looking further. - return "", false - } - } - - if len(hits) != 1 { - return "", false - } - cm.Path = hits[0].path - cm.StartLine = hits[0].start - cm.EndLine = hits[0].end - return hits[0].path, true -} - // indexedLine pairs a normalized line with its absolute file line number. type indexedLine struct { lineNum int diff --git a/internal/llm/client.go b/internal/llm/client.go index e3ec7190..5e2bb801 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -403,7 +403,6 @@ type ChatRequest struct { Model string `json:"model"` Messages []Message `json:"messages"` Tools []ToolDef `json:"tools,omitempty"` - ToolChoice string `json:"tool_choice,omitempty"` // "auto", "required", or "none"; empty means provider default Temperature *float64 `json:"temperature,omitempty"` MaxTokens int `json:"max_tokens,omitempty"` SessionID string `json:"-"` // per-file agent loop session ID; used as prompt_cache_key by the Responses API client @@ -647,11 +646,6 @@ func (c *OpenAIClient) buildOpenAIParams(model string, req ChatRequest) openai.C if len(tools) > 0 { params.Tools = tools } - if req.ToolChoice != "" && len(tools) > 0 { - params.ToolChoice = openai.ChatCompletionToolChoiceOptionUnionParam{ - OfAuto: openai.String(req.ToolChoice), - } - } if req.MaxTokens > 0 { params.MaxCompletionTokens = openai.Int(int64(req.MaxTokens)) } @@ -952,11 +946,6 @@ func (c *AnthropicClient) buildAnthropicParams(model string, req ChatRequest) (a if len(tools) > 0 { tools[len(tools)-1].OfTool.CacheControl = anthropic.NewCacheControlEphemeralParam() params.Tools = tools - if req.ToolChoice == "required" { - params.ToolChoice = anthropic.ToolChoiceUnionParam{ - OfAny: &anthropic.ToolChoiceAnyParam{}, - } - } } // Dynamic breakpoint on the latest message so multi-turn history is // cached incrementally: read the full previous prefix, write only the delta. diff --git a/internal/llm/providers.go b/internal/llm/providers.go index 71c3afb1..5ddcd015 100644 --- a/internal/llm/providers.go +++ b/internal/llm/providers.go @@ -205,20 +205,6 @@ var registry = []Provider{ "kimi-k2.5", }, }, - { - Name: "kimi-global", - DisplayName: "Kimi Moonshot API (Global)", - Protocol: ProtocolOpenAIChatCompletions, - BaseURL: "https://api.moonshot.ai/v1", - EnvVar: "MOONSHOT_GLOBAL_API_KEY", - Models: []string{ - "kimi-k3", - "kimi-k2.7-code", - "kimi-k2.7-code-highspeed", - "kimi-k2.6", - "kimi-k2.5", - }, - }, { Name: "z-ai", DisplayName: "Z.AI API", @@ -240,7 +226,6 @@ var registry = []Provider{ BaseURL: "https://open.bigmodel.cn/api/coding/paas/v4", EnvVar: "Z_AI_CODING_API_KEY", Models: []string{ - "glm-5.3", "glm-5.2", "glm-5.1", "glm-5-turbo", @@ -344,18 +329,6 @@ var registry = []Provider{ "deepseek/deepseek-v4-flash-0731", }, }, - { - Name: "xai", - DisplayName: "xAI Grok API", - Protocol: ProtocolOpenAIChatCompletions, - BaseURL: "https://api.x.ai/v1", - EnvVar: "XAI_API_KEY", - Models: []string{ - "grok-4.6", - "grok-4.5", - "grok-4.3", - }, - }, { Name: "litellm", DisplayName: "LiteLLM AI Gateway", diff --git a/internal/llm/providers_test.go b/internal/llm/providers_test.go index a66c9dbd..a326d76d 100644 --- a/internal/llm/providers_test.go +++ b/internal/llm/providers_test.go @@ -75,7 +75,7 @@ func TestListProviders_Order(t *testing.T) { if len(providers) < 3 { t.Fatalf("expected at least 3 providers, got %d", len(providers)) } - expected := []string{"anthropic", "baidu-qianfan", "dashscope", "dashscope-tokenplan", "deepseek", "edenai", "hy-tokenplan", "iflytek", "kimi", "kimi-global", "litellm", "mimo", "minimax", "minimax-cn", "mistral", "novita", "ollama-cloud", "openai", "siliconflow", "siliconflow-cn", "tencent-tokenhub", "volcengine", "xai", "z-ai", "z-ai-coding"} + expected := []string{"anthropic", "baidu-qianfan", "dashscope", "dashscope-tokenplan", "deepseek", "edenai", "hy-tokenplan", "iflytek", "kimi", "litellm", "mimo", "minimax", "minimax-cn", "mistral", "novita", "ollama-cloud", "openai", "siliconflow", "siliconflow-cn", "tencent-tokenhub", "volcengine", "z-ai", "z-ai-coding"} if len(providers) != len(expected) { t.Fatalf("expected %d providers, got %d", len(expected), len(providers)) } @@ -314,41 +314,6 @@ func TestLookupProvider_MistralDetails(t *testing.T) { } } -func TestLookupProvider_XAIDetails(t *testing.T) { - p, ok := LookupProvider("xai") - if !ok { - t.Fatal("xai not found") - } - if p.DisplayName != "xAI Grok API" { - t.Errorf("DisplayName = %q, want %q", p.DisplayName, "xAI Grok API") - } - if p.Protocol != ProtocolOpenAIChatCompletions { - t.Errorf("Protocol = %q, want %q", p.Protocol, ProtocolOpenAIChatCompletions) - } - if p.BaseURL != "https://api.x.ai/v1" { - t.Errorf("BaseURL = %q, want %q", p.BaseURL, "https://api.x.ai/v1") - } - if p.EnvVar != "XAI_API_KEY" { - t.Errorf("EnvVar = %q, want %q", p.EnvVar, "XAI_API_KEY") - } - if p.AuthHeader != "" { - t.Errorf("AuthHeader = %q, want empty (OpenAI-compatible uses Bearer by default)", p.AuthHeader) - } - expectedModels := []string{ - "grok-4.6", - "grok-4.5", - "grok-4.3", - } - if len(p.Models) != len(expectedModels) { - t.Fatalf("Models length = %d, want %d", len(p.Models), len(expectedModels)) - } - for i, model := range expectedModels { - if p.Models[i] != model { - t.Errorf("Models[%d] = %q, want %q", i, p.Models[i], model) - } - } -} - // TestProviders_AllProtocolsCanonical verifies every registry entry uses a // canonical protocol constant — no stale "openai" / "anthropic" literals that // would bypass NormalizeProtocol downstream. diff --git a/internal/llm/responses_client.go b/internal/llm/responses_client.go index 3613059e..522e2cc1 100644 --- a/internal/llm/responses_client.go +++ b/internal/llm/responses_client.go @@ -11,7 +11,6 @@ import ( openai "github.com/openai/openai-go/v3" openaiopt "github.com/openai/openai-go/v3/option" - "github.com/openai/openai-go/v3/packages/param" "github.com/openai/openai-go/v3/responses" ) @@ -225,11 +224,6 @@ func (c *OpenAIResponsesClient) buildResponsesParams(model string, req ChatReque } if len(tools) > 0 { params.Tools = tools - if req.ToolChoice == "required" { - params.ToolChoice = responses.ResponseNewParamsToolChoiceUnion{ - OfToolChoiceMode: param.NewOpt(responses.ToolChoiceOptionsRequired), - } - } } if req.MaxTokens > 0 { params.MaxOutputTokens = openai.Int(int64(req.MaxTokens)) diff --git a/internal/llmloop/loop.go b/internal/llmloop/loop.go index e9de0416..9c30ef78 100644 --- a/internal/llmloop/loop.go +++ b/internal/llmloop/loop.go @@ -40,15 +40,6 @@ type Deps struct { // NewFileContent is the whole file and Diff is empty). DiffLookup func(path string) *model.Diff - // AllDiffs returns every diff this run reviews, for re-filing a comment - // whose ExistingCode belongs to a different file than the one it was filed - // against (diff.RelocateAcrossFiles). It is the reviewed set rather than - // every parsed diff on purpose: re-filing a comment onto a path the run - // excluded would point the reader at a file this review never covered. - // When nil, cross-file re-filing is skipped and only same-file resolution - // applies. - AllDiffs func() []model.Diff - // NewRequestMeta builds the retry-report identity for one logical LLM // request. Non-nil only for review: the retry report describes ocr review, // and this Runner is shared with scan (internal/scan.Agent calls RunPerFile), @@ -557,23 +548,8 @@ func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.T if r.deps.DiffLookup != nil { d = r.deps.DiffLookup(cm.Path) } - // Resolution order: the comment's own file, then a cross-file - // search, then the LLM. The cross-file search precedes the LLM - // because it needs the Agent's original ExistingCode, which the - // LLM step overwrites; and it runs even when d is nil, since a - // comment filed against a path this run holds no diff for is - // exactly the case that search can still place. - located := d != nil && diff.ResolveComment(cm, d) - if !located && r.deps.AllDiffs != nil { - from := cm.Path - if to, ok := diff.RelocateAcrossFiles(cm, r.deps.AllDiffs()); ok { - located = true - r.RecordWarning("comment_refiled", to, fmt.Sprintf( - "comment filed against %s describes code in %s; re-filed", from, to)) - } - } if d != nil { - if !located && r.deps.Template.ReLocationTask != nil { + if !diff.ResolveComment(cm, d) && r.deps.Template.ReLocationTask != nil { // rlStart stays ahead of prompt construction, which is // where it sat when ReLocateComment built the messages // itself — moving it would silently change what diff --git a/internal/session/manifest.go b/internal/session/manifest.go index 6a113b24..199f004b 100644 --- a/internal/session/manifest.go +++ b/internal/session/manifest.go @@ -265,6 +265,7 @@ type ManifestExecution struct { ConfiguredConcurrency int `json:"configured_concurrency,omitempty"` RuleConfigSHA256 string `json:"rule_config_sha256,omitempty"` RuntimeConfigSHA256 string `json:"runtime_config_sha256,omitempty"` + PerFileMaxTokens int `json:"per_file_max_tokens,omitempty"` } // RunManifest is the immutable, versioned coverage snapshot of a single run. diff --git a/internal/session/resume_identity.go b/internal/session/resume_identity.go index e318b99d..a65dad66 100644 --- a/internal/session/resume_identity.go +++ b/internal/session/resume_identity.go @@ -14,6 +14,7 @@ type RunIdentity struct { SourceArtifactSHA256 string // manifest input.source_artifact_sha256 RuleConfigSHA256 string // manifest execution.rule_config_sha256 RepositorySHA256 string // manifest repository.identity_sha256; empty when the repo has no remote + PerFileMaxTokens int // manifest execution.per_file_max_tokens } // ResumeRequest is what the resuming command is asking to do: run this input @@ -29,6 +30,9 @@ type ResumeRequest struct { Model string ProviderExplicit bool ModelExplicit bool + Incremental bool + ResolvedBase string + PerFileMaxTokens int } const resumeHint = "start a new review instead of resuming" @@ -51,7 +55,7 @@ func (s *ResumeState) ValidateResume(req ResumeRequest) error { if s == nil { return nil } - if err := s.validateInputIdentity(req.Identity); err != nil { + if err := s.validateInputIdentity(req); err != nil { return err } @@ -76,10 +80,11 @@ func (s *ResumeState) ValidateResume(req ResumeRequest) error { // This runs once, at admission, and is never repeated during the run: the caller // pins the run to the commit endpoints this comparison was made against (see // agent.SealedInput), so a second comparison could only ever confirm the first. -func (s *ResumeState) validateInputIdentity(id RunIdentity) error { +func (s *ResumeState) validateInputIdentity(req ResumeRequest) error { if s == nil { return nil } + id := req.Identity m := s.Manifest switch { @@ -113,7 +118,17 @@ func (s *ResumeState) validateInputIdentity(id RunIdentity) error { if m.Repository.IdentitySHA256 != id.RepositorySHA256 { return fmt.Errorf("resume rejected: repository identity changed, so this is not the repository the parent run reviewed; %s", resumeHint) } - if m.Input.SourceArtifactSHA256 != id.SourceArtifactSHA256 { + if req.Incremental { + if m.Input.Mode != InputModeRange { + return fmt.Errorf("incremental resume is only supported for range mode, got %q; %s", m.Input.Mode, resumeHint) + } + if req.ResolvedBase == "" || m.Input.ResolvedBase != req.ResolvedBase { + return fmt.Errorf("the reviewed input base commit changed (parent: %q, requested: %q); %s", m.Input.ResolvedBase, req.ResolvedBase, resumeHint) + } + if req.PerFileMaxTokens != m.Execution.PerFileMaxTokens { + return fmt.Errorf("per-file max token ceiling changed (parent: %d, requested: %d); %s", m.Execution.PerFileMaxTokens, req.PerFileMaxTokens, resumeHint) + } + } else if m.Input.SourceArtifactSHA256 != id.SourceArtifactSHA256 { return fmt.Errorf("resume rejected: the reviewed input changed since session %q — a ref may now point at a different commit, or the selected file set changed; %s", s.SessionID, resumeHint) } if m.Execution.RuleConfigSHA256 == "" { diff --git a/internal/session/resume_identity_test.go b/internal/session/resume_identity_test.go index 3a605202..cd06a17f 100644 --- a/internal/session/resume_identity_test.go +++ b/internal/session/resume_identity_test.go @@ -75,6 +75,62 @@ func TestValidateResume(t *testing.T) { { name: "identical input, provider and model is accepted", }, + { + name: "incremental resume with moved head and same base is accepted when Incremental is true", + req: func(r *ResumeRequest) { + r.Incremental = true + r.ResolvedBase = "base-commit-123" + r.PerFileMaxTokens = 1000 + r.Identity.SourceArtifactSHA256 = "artifact-child-moved-head" + }, + parent: func(m *RunManifest) { + m.Input.ResolvedBase = "base-commit-123" + m.Execution.PerFileMaxTokens = 1000 + }, + state: func(s *ResumeState) { + s.Manifest.Execution.PerFileMaxTokens = 1000 + }, + }, + { + name: "incremental resume with moved head rejected when base differs", + req: func(r *ResumeRequest) { + r.Incremental = true + r.ResolvedBase = "base-commit-different" + r.PerFileMaxTokens = 1000 + r.Identity.SourceArtifactSHA256 = "artifact-child-moved-head" + }, + parent: func(m *RunManifest) { + m.Input.ResolvedBase = "base-commit-123" + m.Execution.PerFileMaxTokens = 1000 + }, + wantErr: "the reviewed input base commit changed", + }, + { + name: "incremental resume with moved head rejected when per-file max tokens differs", + req: func(r *ResumeRequest) { + r.Incremental = true + r.ResolvedBase = "base-commit-123" + r.PerFileMaxTokens = 2000 + r.Identity.SourceArtifactSHA256 = "artifact-child-moved-head" + }, + parent: func(m *RunManifest) { + m.Input.ResolvedBase = "base-commit-123" + m.Execution.PerFileMaxTokens = 1000 + }, + wantErr: "per-file max token ceiling changed", + }, + { + name: "incremental resume with moved head rejected when Incremental is false", + req: func(r *ResumeRequest) { + r.Incremental = false + r.ResolvedBase = "base-commit-123" + r.Identity.SourceArtifactSHA256 = "artifact-child-moved-head" + }, + parent: func(m *RunManifest) { + m.Input.ResolvedBase = "base-commit-123" + }, + wantErr: "the reviewed input changed", + }, { name: "parent that completed nothing is still resumable", // The parent's own coverage is irrelevant here: admission depends on diff --git a/internal/stdout/stdout.go b/internal/stdout/stdout.go index 58ed4da2..150ad192 100644 --- a/internal/stdout/stdout.go +++ b/internal/stdout/stdout.go @@ -41,25 +41,3 @@ func Quiet() func() { mu.Unlock() } } - -// Swap replaces the stdout writer with replacement and returns a restore -// function. It lets tests capture output written through Writer(). -// Usage: -// -// var buf bytes.Buffer -// defer stdout.Swap(&buf)() -// -// Like Quiet, Swap acquires the package mutex for memory safety, but concurrent -// swaps from multiple goroutines produce non-deterministic restore ordering. -// Keep swapping and restoring on a single goroutine. -func Swap(replacement io.Writer) func() { - mu.Lock() - old := w - w = replacement - mu.Unlock() - return func() { - mu.Lock() - w = old - mu.Unlock() - } -} diff --git a/internal/stdout/stdout_test.go b/internal/stdout/stdout_test.go index 9dfd6d7a..98130db8 100644 --- a/internal/stdout/stdout_test.go +++ b/internal/stdout/stdout_test.go @@ -4,7 +4,6 @@ package stdout import ( - "bytes" "io" "os" "testing" @@ -32,41 +31,3 @@ func TestQuiet(t *testing.T) { t.Error("expected Writer to be os.Stdout after restore") } } - -func TestSwap(t *testing.T) { - var buf bytes.Buffer - restore := Swap(&buf) - - if _, err := io.WriteString(Writer(), "captured"); err != nil { - t.Fatalf("write to swapped writer failed: %v", err) - } - if got := buf.String(); got != "captured" { - t.Errorf("expected swapped writer to capture %q, got %q", "captured", got) - } - - restore() - - if w := Writer(); w != os.Stdout { - t.Error("expected Writer to be os.Stdout after restore") - } -} - -func TestSwap_Composable(t *testing.T) { - var buf bytes.Buffer - outer := Swap(&buf) - - innerRestore := Quiet() - if w := Writer(); w != io.Discard { - t.Error("expected Writer to be io.Discard after nested Quiet()") - } - innerRestore() - - if w := Writer(); w != &buf { - t.Error("expected Writer to be the swapped buffer after nested restore") - } - outer() - - if w := Writer(); w != os.Stdout { - t.Error("expected Writer to be os.Stdout after outer restore") - } -} diff --git a/internal/telemetry/events.go b/internal/telemetry/events.go index 3a30af52..34d3994a 100644 --- a/internal/telemetry/events.go +++ b/internal/telemetry/events.go @@ -68,36 +68,19 @@ func FormatDuration(dur time.Duration) string { return dur.Round(time.Millisecond).String() } -// TraceSummary carries the metrics printed by PrintTraceSummary. -type TraceSummary struct { - FilesReviewed int64 - CommentsGenerated int64 - InputTokens int64 - OutputTokens int64 - TotalTokens int64 - CacheReadTokens int64 - CacheWriteTokens int64 - Duration time.Duration - SessionID string -} - // PrintTraceSummary prints a one-line summary of the review to stdout. -// If SessionID is non-empty, an "[ocr] Session: " line follows the summary. -func PrintTraceSummary(s TraceSummary) { - elapsed := s.Duration.Round(time.Second).String() - if s.InputTokens > 0 || s.OutputTokens > 0 { +func PrintTraceSummary(filesReviewed, commentsGenerated int64, inputTokens, outputTokens, totalTokens int64, cacheReadTokens, cacheWriteTokens int64, duration time.Duration) { + elapsed := duration.Round(time.Second).String() + if inputTokens > 0 || outputTokens > 0 { base := fmt.Sprintf("[ocr] Summary: %d file(s) reviewed, %d comment(s), ~%d token(s) used (input: ~%d, output: ~%d)", - s.FilesReviewed, s.CommentsGenerated, s.TotalTokens, s.InputTokens, s.OutputTokens) - if s.CacheReadTokens > 0 || s.CacheWriteTokens > 0 { - base += fmt.Sprintf(", cache(read: ~%d, write: ~%d)", s.CacheReadTokens, s.CacheWriteTokens) + filesReviewed, commentsGenerated, totalTokens, inputTokens, outputTokens) + if cacheReadTokens > 0 || cacheWriteTokens > 0 { + base += fmt.Sprintf(", cache(read: ~%d, write: ~%d)", cacheReadTokens, cacheWriteTokens) } fmt.Fprintf(stdout.Writer(), "%s, %s elapsed\n", base, elapsed) } else { fmt.Fprintf(stdout.Writer(), "[ocr] Summary: %d file(s) reviewed, %d comment(s), ~%d token(s) used, %s elapsed\n", - s.FilesReviewed, s.CommentsGenerated, s.TotalTokens, elapsed) - } - if s.SessionID != "" { - fmt.Fprintf(stdout.Writer(), "[ocr] Session: %s\n", s.SessionID) + filesReviewed, commentsGenerated, totalTokens, elapsed) } } diff --git a/internal/telemetry/events_test.go b/internal/telemetry/events_test.go index ef34d758..3f6b5dc2 100644 --- a/internal/telemetry/events_test.go +++ b/internal/telemetry/events_test.go @@ -19,8 +19,6 @@ import ( sdkmetric "go.opentelemetry.io/otel/sdk/metric" "go.opentelemetry.io/otel/sdk/resource" sdktrace "go.opentelemetry.io/otel/sdk/trace" - - "github.com/alibaba/open-code-review/internal/stdout" ) func setupEnabledTelemetry(t *testing.T) { @@ -126,88 +124,16 @@ func TestPhaseEvent_WithError(t *testing.T) { PhaseEvent(ctx, "scan", "main.go", 500*time.Millisecond, errors.New("parse error")) } -func captureStdout(t *testing.T, fn func()) string { - t.Helper() - var buf bytes.Buffer - restore := stdout.Swap(&buf) - defer restore() - fn() - return buf.String() -} - func TestPrintTraceSummary_WithTokenDetails(t *testing.T) { - out := captureStdout(t, func() { - PrintTraceSummary(TraceSummary{ - FilesReviewed: 5, CommentsGenerated: 10, - InputTokens: 1000, OutputTokens: 200, TotalTokens: 1200, - Duration: 3 * time.Second, - }) - }) - want := "[ocr] Summary: 5 file(s) reviewed, 10 comment(s), ~1200 token(s) used (input: ~1000, output: ~200), 3s elapsed\n" - if out != want { - t.Errorf("PrintTraceSummary output = %q, want %q", out, want) - } + PrintTraceSummary(5, 10, 1000, 200, 1200, 0, 0, 3*time.Second) } func TestPrintTraceSummary_WithCacheTokens(t *testing.T) { - out := captureStdout(t, func() { - PrintTraceSummary(TraceSummary{ - FilesReviewed: 3, CommentsGenerated: 2, - InputTokens: 500, OutputTokens: 100, TotalTokens: 600, - CacheReadTokens: 200, CacheWriteTokens: 50, - Duration: 2 * time.Second, - }) - }) - want := "[ocr] Summary: 3 file(s) reviewed, 2 comment(s), ~600 token(s) used (input: ~500, output: ~100), cache(read: ~200, write: ~50), 2s elapsed\n" - if out != want { - t.Errorf("PrintTraceSummary output = %q, want %q", out, want) - } + PrintTraceSummary(3, 2, 500, 100, 600, 200, 50, 2*time.Second) } func TestPrintTraceSummary_NoTokenDetails(t *testing.T) { - out := captureStdout(t, func() { - PrintTraceSummary(TraceSummary{ - FilesReviewed: 2, CommentsGenerated: 1, - TotalTokens: 500, - Duration: 1 * time.Second, - }) - }) - want := "[ocr] Summary: 2 file(s) reviewed, 1 comment(s), ~500 token(s) used, 1s elapsed\n" - if out != want { - t.Errorf("PrintTraceSummary output = %q, want %q", out, want) - } -} - -func TestPrintTraceSummary_WithSessionID(t *testing.T) { - out := captureStdout(t, func() { - PrintTraceSummary(TraceSummary{ - FilesReviewed: 5, CommentsGenerated: 10, - InputTokens: 1000, OutputTokens: 200, TotalTokens: 1200, - Duration: 3 * time.Second, - SessionID: "3a7f2b1c-9d4e-4f8a-b2c1-6e7f8a9b0c1d", - }) - }) - want := "[ocr] Summary: 5 file(s) reviewed, 10 comment(s), ~1200 token(s) used (input: ~1000, output: ~200), 3s elapsed\n" + - "[ocr] Session: 3a7f2b1c-9d4e-4f8a-b2c1-6e7f8a9b0c1d\n" - if out != want { - t.Errorf("PrintTraceSummary output = %q, want %q", out, want) - } -} - -func TestPrintTraceSummary_WithoutSessionID(t *testing.T) { - // An empty session ID exercises the omit path: no session line is printed. - // The empty case arises when session persistence is unavailable, not from - // preview mode (preview does not reach PrintTraceSummary). - out := captureStdout(t, func() { - PrintTraceSummary(TraceSummary{ - FilesReviewed: 2, CommentsGenerated: 1, - TotalTokens: 500, - Duration: 1 * time.Second, - }) - }) - if strings.Contains(out, "Session:") { - t.Errorf("expected no session line for empty session ID, got %q", out) - } + PrintTraceSummary(2, 1, 0, 0, 500, 0, 0, 1*time.Second) } func TestPrintToolCallStarted_WithArgs(t *testing.T) { diff --git a/internal/viewer/store.go b/internal/viewer/store.go index d2f04d46..d85abd1d 100644 --- a/internal/viewer/store.go +++ b/internal/viewer/store.go @@ -432,8 +432,6 @@ func LoadSession(root, encodedRepo, sessionID string) (*ViewSession, error) { info := ToolCallInfo{Name: name, Arguments: args} if name == "task_done" { info.Ok = taskDoneSucceeded(args) - } else if name == "report_incorrect_comments" || name == "approve_all_comments" { - info.Ok = true } card.ToolCalls = append(card.ToolCalls, info) } diff --git a/pages/src/assets/icons/apple.svg b/pages/src/assets/icons/apple.svg deleted file mode 100644 index 49ac0e7f..00000000 --- a/pages/src/assets/icons/apple.svg +++ /dev/null @@ -1,3 +0,0 @@ - - - diff --git a/pages/src/assets/icons/linux.svg b/pages/src/assets/icons/linux.svg deleted file mode 100644 index 4664f736..00000000 --- a/pages/src/assets/icons/linux.svg +++ /dev/null @@ -1,15 +0,0 @@ - - - - - - - - - - - - - - - diff --git a/pages/src/assets/icons/macports.svg b/pages/src/assets/icons/macports.svg deleted file mode 100644 index d2f83920..00000000 --- a/pages/src/assets/icons/macports.svg +++ /dev/null @@ -1 +0,0 @@ -MacPorts \ No newline at end of file diff --git a/pages/src/assets/icons/windows.svg b/pages/src/assets/icons/windows.svg deleted file mode 100644 index 7b0d29b4..00000000 --- a/pages/src/assets/icons/windows.svg +++ /dev/null @@ -1,3 +0,0 @@ - - - diff --git a/pages/src/components/HeroSection.test.tsx b/pages/src/components/HeroSection.test.tsx deleted file mode 100644 index 7a15ee78..00000000 --- a/pages/src/components/HeroSection.test.tsx +++ /dev/null @@ -1,96 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// Copyright 2026 alibaba/open-code-review Contributors - -import { describe, it, expect } from 'vitest'; -import { render, screen } from '@testing-library/react'; -import userEvent from '@testing-library/user-event'; -import { MemoryRouter } from 'react-router-dom'; -import { LanguageProvider } from '../i18n'; -import HeroSection from './HeroSection'; - -function renderHero() { - render( - - - - - , - ); -} - -// The panel is found through the id that `aria-controls` already points at, so -// the test leans on the accessibility wiring instead of a test-only hook. -const panel = () => document.getElementById('install-more-panel'); -const trigger = () => screen.getByRole('button', { name: /More|MacPorts/i }); - -describe('HeroSection install channels', () => { - it('starts on the first channel with the panel closed', () => { - renderHero(); - expect(screen.getByText('npm i -g @alibaba-group/open-code-review')).toBeTruthy(); - expect(panel()).toBeNull(); - }); - - it('picking an overflow channel swaps the command and closes the panel', async () => { - const user = userEvent.setup(); - renderHero(); - - await user.click(trigger()); - expect(panel()).not.toBeNull(); - - await user.click(screen.getByRole('button', { name: /MacPorts/i })); - - expect(screen.getByText('sudo port install open-code-review')).toBeTruthy(); - expect(panel()).toBeNull(); - }); - - it('closes when a primary tab is clicked', async () => { - const user = userEvent.setup(); - renderHero(); - - await user.click(trigger()); - await user.click(screen.getByRole('button', { name: /Homebrew/i })); - - expect(screen.getByText('brew install open-code-review')).toBeTruthy(); - expect(panel()).toBeNull(); - }); - - // Keyboard activation dispatches `click` with no preceding `mousedown`, so - // this does not go through the same path as the test above. - it('closes when a primary tab is activated by keyboard', async () => { - const user = userEvent.setup(); - renderHero(); - - await user.click(trigger()); - screen.getByRole('button', { name: /Homebrew/i }).focus(); - await user.keyboard('{Enter}'); - - expect(screen.getByText('brew install open-code-review')).toBeTruthy(); - expect(panel()).toBeNull(); - }); - - it('closes on Escape and on an outside click', async () => { - const user = userEvent.setup(); - renderHero(); - - await user.click(trigger()); - await user.keyboard('{Escape}'); - expect(panel()).toBeNull(); - - await user.click(trigger()); - await user.click(document.body); - expect(panel()).toBeNull(); - }); - - it('reflects the selected overflow channel on the trigger', async () => { - const user = userEvent.setup(); - renderHero(); - expect(screen.getByRole('button', { name: /^More$/i })).toBeTruthy(); - - await user.click(trigger()); - await user.click(screen.getByRole('button', { name: /MacPorts/i })); - - const collapsed = screen.getByRole('button', { name: /MacPorts/i }); - expect(collapsed.getAttribute('aria-expanded')).toBe('false'); - expect(screen.queryByRole('button', { name: /^More$/i })).toBeNull(); - }); -}); diff --git a/pages/src/components/HeroSection.tsx b/pages/src/components/HeroSection.tsx index d657ace1..c3b96962 100644 --- a/pages/src/components/HeroSection.tsx +++ b/pages/src/components/HeroSection.tsx @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 alibaba/open-code-review Contributors -import React, { Suspense, useCallback, useState, useEffect, useRef } from 'react'; +import React, { Suspense, useCallback, useState, useEffect } from 'react'; import ReactDOM from 'react-dom'; import { Link } from 'react-router-dom'; import { useTranslation } from '../i18n'; @@ -9,12 +9,7 @@ import { useResponsive } from '../hooks/useResponsive'; import ErrorBoundary from './ErrorBoundary'; import npmIcon from '../assets/icons/npm.svg'; import brewIcon from '../assets/icons/brew.svg'; -import macportsIcon from '../assets/icons/macports.svg'; -import appleIcon from '../assets/icons/apple.svg'; -import linuxIcon from '../assets/icons/linux.svg'; -import windowsIcon from '../assets/icons/windows.svg'; import copyIcon from '../assets/icons/icon-copy.svg'; -import chevronDownIcon from '../assets/icons/icon-chevron-down.svg'; const ColorBends = React.lazy(() => import(/* webpackChunkName: "color-bends" */ './ColorBends')); @@ -127,26 +122,11 @@ const terminalLines = [ { num: 12, content: ｜ }, // allow-non-english: fullwidth bar renders the terminal cursor ]; -interface InstallChannel { - key: string; - labelKey: string; - cmd: string; - icons: string[]; - primary: boolean; -} - -const INSTALL_CHANNELS: InstallChannel[] = [ - { key: 'npm', labelKey: 'hero.installNpm', cmd: 'npm i -g @alibaba-group/open-code-review', icons: [npmIcon], primary: true }, - { key: 'brew', labelKey: 'hero.installBrew', cmd: 'brew install open-code-review', icons: [brewIcon], primary: true }, - { key: 'macos', labelKey: 'hero.installMacOS', cmd: 'curl -fsSL https://open-codereview.ai/install.sh | sh', icons: [appleIcon], primary: false }, - { key: 'linux', labelKey: 'hero.installLinux', cmd: 'curl -fsSL https://open-codereview.ai/install.sh | sh', icons: [linuxIcon], primary: false }, - { key: 'windows', labelKey: 'hero.installWindows', cmd: 'irm https://open-codereview.ai/install.ps1 | iex', icons: [windowsIcon], primary: false }, - { key: 'macports', labelKey: 'hero.installMacPorts', cmd: 'sudo port install open-code-review', icons: [macportsIcon], primary: false }, +const INSTALL_CHANNELS = [ + { key: 'npm', labelKey: 'hero.installNpm', cmd: 'npm i -g @alibaba-group/open-code-review', icons: [npmIcon] }, + { key: 'brew', labelKey: 'hero.installBrew', cmd: 'brew install open-code-review', icons: [brewIcon] }, ]; -const PRIMARY_CHANNELS = INSTALL_CHANNELS.filter((ch) => ch.primary); -const SECONDARY_CHANNELS = INSTALL_CHANNELS.filter((ch) => !ch.primary); - const HeroSection: React.FC = () => { const { t } = useTranslation(); const { isMobile, isTablet, isDesktop } = useResponsive(); @@ -154,12 +134,7 @@ const HeroSection: React.FC = () => { const [toastVisible, setToastVisible] = useState(false); const [toastMessage, setToastMessage] = useState(''); const [showShaderBackground, setShowShaderBackground] = useState(false); - const [activeChannelKey, setActiveChannelKey] = useState(INSTALL_CHANNELS[0].key); - const [menuOpen, setMenuOpen] = useState(false); - const menuRef = useRef(null); - - const activeChannel = INSTALL_CHANNELS.find((ch) => ch.key === activeChannelKey) ?? INSTALL_CHANNELS[0]; - const activeIsSecondary = !activeChannel.primary; + const [activeChannel, setActiveChannel] = useState(0); const showToast = (message: string) => { setToastMessage(message); @@ -201,24 +176,6 @@ const HeroSection: React.FC = () => { return () => clearTimeout(timer); }, [toastVisible]); - useEffect(() => { - if (!menuOpen) return; - const handlePointerDown = (e: MouseEvent | TouchEvent) => { - if (menuRef.current && !menuRef.current.contains(e.target as Node)) setMenuOpen(false); - }; - const handleKeyDown = (e: KeyboardEvent) => { - if (e.key === 'Escape') setMenuOpen(false); - }; - document.addEventListener('mousedown', handlePointerDown); - document.addEventListener('touchstart', handlePointerDown); - document.addEventListener('keydown', handleKeyDown); - return () => { - document.removeEventListener('mousedown', handlePointerDown); - document.removeEventListener('touchstart', handlePointerDown); - document.removeEventListener('keydown', handleKeyDown); - }; - }, [menuOpen]); - useEffect(() => { // Wait until after the first paint before loading the heavy shader chunk. let secondFrame: number | undefined; @@ -252,7 +209,7 @@ const HeroSection: React.FC = () => { minHeight: isMobile ? 600 : isTablet ? 700 : 680, paddingBottom: isMobile ? 60 : 80, position: 'relative', - overflow: 'visible', + overflow: 'hidden', display: 'flex', flexDirection: 'column', alignItems: 'center', @@ -368,140 +325,33 @@ const HeroSection: React.FC = () => { {/* Install channels — tab switcher */} -
-
- {PRIMARY_CHANNELS.map((ch) => { - const isActive = ch.key === activeChannelKey; - return ( - - ); - })} - - {/* Overflow channels live behind a "More" trigger so the row stops - growing each time a new channel is added. */} -
+
+
+ {INSTALL_CHANNELS.map((ch, idx) => ( - - {menuOpen && ( -
- {SECONDARY_CHANNELS.map((ch) => { - const isActive = ch.key === activeChannelKey; - return ( - - ); - })} -
- setMenuOpen(false)} - style={{ - display: 'flex', - alignItems: 'center', - justifyContent: 'space-between', - gap: 8, - padding: '8px 12px', - borderRadius: 6, - color: 'rgba(255,255,255,0.6)', - fontSize: 13, - textDecoration: 'none', - whiteSpace: 'nowrap', - }} - > - {t('hero.allInstallOptions')} - - -
- )} -
+ ))}
{ textOverflow: 'ellipsis', }} > - {activeChannel.cmd} + {INSTALL_CHANNELS[activeChannel].cmd} Copy handleCopy(activeChannel.cmd)} + onClick={() => handleCopy(INSTALL_CHANNELS[activeChannel].cmd)} />
diff --git a/pages/src/content/docs/en/cli-reference.md b/pages/src/content/docs/en/cli-reference.md index 805711b4..5f587c68 100644 --- a/pages/src/content/docs/en/cli-reference.md +++ b/pages/src/content/docs/en/cli-reference.md @@ -27,8 +27,6 @@ Commands: Examples: ocr review --from master --to dev Review diff range ocr review --commit abc123 Review a single commit - ocr review --background "Focus on auth" --background-file ./docs/requirements.md Review with context - ocr review -B ./docs/requirements.md Review with context file ocr config provider Interactive provider setup ocr config model Interactive model selection ocr config set llm.model opus-4-6 Set a config value @@ -92,19 +90,15 @@ staged + unstaged + untracked changes in the current directory's repo. | `--to ` | — | — | Target ref to end the diff at (e.g., `feature-branch`). When set, OCR computes `merge-base(from, to)..to`. | | `--commit ` | `-c` | — | Single commit to review (vs its parent). | | `--preview` | `-p` | `false` | Run the filter pipeline but skip the LLM. Prints the file list and exclusion reasons. Honors `--format json`; `--format sarif` is not supported (a preview has no completed findings to emit). | -| `--no-filter` | — | `false` | Keep all review comments and skip the per-file `REVIEW_FILTER_TASK` LLM post-processing call. | | `--resume ` | — | — | Resume from a previous compatible range or commit review session. | | `--format ` | `-f` | `text` | `text` (human-readable), `json` (machine-readable comment array), or `sarif` (SARIF 2.1.0 report for GitHub Code Scanning). | | `--audience ` | — | `human` | `human` streams progress lines; `agent` quiets stdout and prints only the final summary / JSON. | | `--background ` | `-b` | — | Optional requirement / business context injected into the plan + main prompts. | -| `--background-file ` | `-B` | — | Path to a Markdown file used as review background. Combined with `--background` when both are set. | -| `--exclude ` | — | — | Comma-separated gitignore-style patterns to exclude; merged with the `excludes` section of `rule.json` | | `--concurrency ` | — | `8` | Maximum number of files reviewed in parallel. | | `--timeout ` | — | `10` | Per-file deadline. `0` disables the timeout. | | `--rule ` | — | — | Path to a custom JSON review rule file. Overrides the project-level and global `rule.json`. | | `--max-tools ` | — | template default | Max tool-call rounds per file. `0` uses the template default (`30`); values 1–9 are clamped up to `10`; any value `≥ 10` overrides the template default (even if smaller than `30`). | | `--max-tokens ` | — | config or template default | Per-file prompt token ceiling. Overrides the saved `max_tokens` setting for this run. | -| `--max-tokens-budget ` | — | `0` (unlimited) | Cap total input + output token usage for the review. Dispatch stops once the budget is exceeded and partial results are still published. | | `--provider ` | — | — | Select a configured provider for this run. Names under both `providers` and `custom_providers` are accepted. | | `--model ` | — | — | Override the resolved LLM model for this run (e.g., `claude-opus-4-6`). | | `--max-git-procs ` | — | `16` | Maximum number of concurrent git subprocesses. | @@ -201,9 +195,8 @@ would review the same thing the parent did: - a provider or model change must be asked for explicitly with `--provider` / `--model`. A change that arrived through config or the environment is rejected - the parent must carry a run manifest, which is what its input is verified - against. After file dispatch begins, Ctrl-C cancels the review gracefully and - records one, so completed checkpoints remain resumable. A process killed - before graceful shutdown and sessions older than run manifests do not have one + against. A run killed with Ctrl-C never wrote one, and sessions older than run + manifests never had one - only files the parent's manifest settled are reused. A checkpoint the manifest does not account for, or one that is unreadable, costs that file its checkpoint and nothing more — it is simply reviewed again diff --git a/pages/src/content/docs/en/configuration.md b/pages/src/content/docs/en/configuration.md index f5a61fb9..58f0cbf6 100644 --- a/pages/src/content/docs/en/configuration.md +++ b/pages/src/content/docs/en/configuration.md @@ -54,7 +54,6 @@ environment variable. | `hy-tokenplan` | openai | `https://api.lkeap.cloud.tencent.com/plan/v3` | `TENCENT_HUNYUAN_TOKENPLAN_KEY` | | `iflytek` | openai | `https://spark-api-open.xf-yun.com/v1` | `SPARK_API_KEY` | | `kimi` | openai | `https://api.moonshot.cn/v1` | `MOONSHOT_API_KEY` | -| `kimi-global` | openai | `https://api.moonshot.ai/v1` | `MOONSHOT_GLOBAL_API_KEY` | | `z-ai` | openai | `https://open.bigmodel.cn/api/paas/v4` | `Z_AI_API_KEY` | | `mimo` | openai | `https://api.xiaomimimo.com/v1` | `MIMO_API_KEY` | | `minimax` | openai | `https://api.minimax.io/v1` | `MINIMAX_GLOBAL_API_KEY` | @@ -63,7 +62,6 @@ environment variable. | `siliconflow` | openai | `https://api.siliconflow.com/v1` | `SILICONFLOW_GLOBAL_API_KEY` | | `siliconflow-cn` | openai | `https://api.siliconflow.cn/v1` | `SILICONFLOW_API_KEY` | | `novita` | openai | `https://api.novita.ai/openai` | `NOVITA_API_KEY` | -| `xai` | openai | `https://api.x.ai/v1` | `XAI_API_KEY` | ### Overriding a built-in provider's Base URL diff --git a/pages/src/content/docs/en/installation.md b/pages/src/content/docs/en/installation.md index 38f7bde0..f681003c 100644 --- a/pages/src/content/docs/en/installation.md +++ b/pages/src/content/docs/en/installation.md @@ -76,7 +76,7 @@ A convenience installer that wraps the GitHub Release binary download machines: ```bash -curl -fsSL https://open-codereview.ai/install.sh | sh +curl -fsSL https://raw.githubusercontent.com/alibaba/open-code-review/main/install.sh | sh ``` It honours two environment variables: @@ -91,7 +91,7 @@ The script supports `darwin` and `linux` on `amd64` / `arm64`. On Windows (PowerShell 5.1+), use the PowerShell installer instead: ```powershell -irm https://open-codereview.ai/install.ps1 | iex +irm https://raw.githubusercontent.com/alibaba/open-code-review/main/install.ps1 | iex ``` It honours the same `OCR_INSTALL_DIR` and `OCR_VERSION` variables (set via diff --git a/pages/src/content/docs/ja/cli-reference.md b/pages/src/content/docs/ja/cli-reference.md index 4f093df2..47a13a12 100644 --- a/pages/src/content/docs/ja/cli-reference.md +++ b/pages/src/content/docs/ja/cli-reference.md @@ -26,8 +26,6 @@ Commands: Examples: ocr review --from master --to dev Review diff range ocr review --commit abc123 Review a single commit - ocr review --background "Focus on auth" --background-file ./docs/requirements.md Review with context - ocr review -B ./docs/requirements.md Review with context file ocr config provider Interactive provider setup ocr config model Interactive model selection ocr config set llm.model opus-4-6 Set a config value @@ -88,19 +86,15 @@ ocr r [flags] (alias) | `--to ` | — | — | diff の終了 ref（例: `feature-branch`）。設定すると OCR は `merge-base(from, to)..to` を計算します。 | | `--commit ` | `-c` | — | 単一の commit をレビューします（その親との差分）。 | | `--preview` | `-p` | `false` | フィルタリングのパイプラインを実行しますが LLM はスキップします。ファイル一覧と除外理由を出力します。`--format json` に対応しています。`--format sarif` はサポートされていません（プレビューには出力する完了した指摘がありません）。 | -| `--no-filter` | — | `false` | すべてのレビューコメントを保持し、ファイルごとの `REVIEW_FILTER_TASK` LLM 後処理呼び出しをスキップします。 | | `--resume ` | — | — | 以前の互換性のある範囲または単一 commit レビューセッションから再開します。 | | `--format ` | `-f` | `text` | `text`（人間が読みやすい形式）、`json`（機械可読なコメント配列）または `sarif`（GitHub Code Scanning 用の SARIF 2.1.0 レポート）。 | | `--audience ` | — | `human` | `human` は進捗行をストリーム出力します。`agent` は stdout を静音化し、最終サマリー / JSON のみを出力します。 | | `--background ` | `-b` | — | plan + main prompt に注入する、任意の要件 / 業務コンテキスト。 | -| `--background-file ` | `-B` | — | レビューの背景として使用する Markdown ファイルのパス。`--background` も指定した場合は両方を結合します。 | -| `--exclude ` | — | — | 除外する gitignore 形式のパターン（カンマ区切り）。`rule.json` の excludes とマージされます。 | | `--concurrency ` | — | `8` | 並行してレビューするファイルの最大数。 | | `--timeout ` | — | `10` | ファイルごとの締め切り時間。`0` でタイムアウトを無効化します。 | | `--rule ` | — | — | カスタム JSON レビュールールファイルのパス。プロジェクトレベルおよびグローバルの `rule.json` を上書きします。 | | `--max-tools ` | — | テンプレートのデフォルト | ファイルごとの最大ツール呼び出し回数。`0` はテンプレートのデフォルト（`30`）を使用します。1〜9 は `10` に引き上げられます。`≥ 10` の値はすべてテンプレートのデフォルトを上書きします（`30` より小さくても）。 | | `--max-tokens ` | — | 設定またはテンプレートのデフォルト | ファイルごとのプロンプトトークン上限。この実行で保存済みの `max_tokens` 設定を上書きします。 | -| `--max-tokens-budget ` | — | `0`（無制限） | レビュー全体の入力 + 出力トークン使用量を制限します。予算を超えると処理の割り当てを停止し、部分的な結果は引き続き公開されます。 | | `--provider ` | — | — | 今回の実行で設定済み provider を選択します。`providers` と `custom_providers` の両方の名前を使用できます。 | | `--model ` | — | — | 今回の実行で解決済みの LLM model を上書きします（例: `claude-opus-4-6`）。 | | `--max-git-procs ` | — | `16` | 並行 git サブプロセスの最大数。 | @@ -188,10 +182,8 @@ ocr review --commit abc123 --resume - provider や model の変更は `--provider` / `--model` で明示的に指定する必要が あります。設定ファイルや環境変数経由の変更は拒否されます - 親の実行が run manifest を持っている必要があります。入力はこれと照合して - 検証されます。ファイルの dispatch 開始後は、Ctrl-C によってレビューが正常に - キャンセルされて manifest が書き出されるため、完了済みの checkpoint は再開時に - 再利用できます。正常に終了できなかったプロセスと run manifest より古い - セッションには manifest がありません + 検証されます。Ctrl-C で中断された実行は書き出しておらず、run manifest より + 古いセッションはそもそも持っていません - 再利用されるのは、親の manifest が結果を確定したファイルだけです。manifest が 裏付けないチェックポイントや読み取れないチェックポイントは、そのファイルが もう一度レビューされるだけで、他のファイルには影響しません diff --git a/pages/src/content/docs/ja/configuration.md b/pages/src/content/docs/ja/configuration.md index 8fd5e2a1..1ff441a9 100644 --- a/pages/src/content/docs/ja/configuration.md +++ b/pages/src/content/docs/ja/configuration.md @@ -52,7 +52,6 @@ ocr config set providers.anthropic.api_key sk-ant-xxxxxxxxxx | `hy-tokenplan` | openai | `https://api.lkeap.cloud.tencent.com/plan/v3` | `TENCENT_HUNYUAN_TOKENPLAN_KEY` | | `iflytek` | openai | `https://spark-api-open.xf-yun.com/v1` | `SPARK_API_KEY` | | `kimi` | openai | `https://api.moonshot.cn/v1` | `MOONSHOT_API_KEY` | -| `kimi-global` | openai | `https://api.moonshot.ai/v1` | `MOONSHOT_GLOBAL_API_KEY` | | `z-ai` | openai | `https://open.bigmodel.cn/api/paas/v4` | `Z_AI_API_KEY` | | `mimo` | openai | `https://api.xiaomimimo.com/v1` | `MIMO_API_KEY` | | `minimax` | openai | `https://api.minimax.io/v1` | `MINIMAX_GLOBAL_API_KEY` | @@ -61,7 +60,6 @@ ocr config set providers.anthropic.api_key sk-ant-xxxxxxxxxx | `siliconflow` | openai | `https://api.siliconflow.com/v1` | `SILICONFLOW_GLOBAL_API_KEY` | | `siliconflow-cn` | openai | `https://api.siliconflow.cn/v1` | `SILICONFLOW_API_KEY` | | `novita` | openai | `https://api.novita.ai/openai` | `NOVITA_API_KEY` | -| `xai` | openai | `https://api.x.ai/v1` | `XAI_API_KEY` | ### 組み込み provider の Base URL を上書きする diff --git a/pages/src/content/docs/ja/installation.md b/pages/src/content/docs/ja/installation.md index a0d945cb..04524714 100644 --- a/pages/src/content/docs/ja/installation.md +++ b/pages/src/content/docs/ja/installation.md @@ -74,7 +74,7 @@ GitHub Release バイナリのダウンロード（検証付き）をラップ イメージやヘッドレス環境に適しています。 ```bash -curl -fsSL https://open-codereview.ai/install.sh | sh +curl -fsSL https://raw.githubusercontent.com/alibaba/open-code-review/main/install.sh | sh ``` 2 つの環境変数を認識します。 @@ -89,7 +89,7 @@ curl -fsSL https://open-codereview.ai/install.sh | sh Windows（PowerShell 5.1+）では、代わりに PowerShell インストーラーを使用してください： ```powershell -irm https://open-codereview.ai/install.ps1 | iex +irm https://raw.githubusercontent.com/alibaba/open-code-review/main/install.ps1 | iex ``` 同じ `OCR_INSTALL_DIR` と `OCR_VERSION` を認識します（`$env:OCR_INSTALL_DIR` / diff --git a/pages/src/content/docs/ru/cli-reference.md b/pages/src/content/docs/ru/cli-reference.md index 81f77725..af95256a 100644 --- a/pages/src/content/docs/ru/cli-reference.md +++ b/pages/src/content/docs/ru/cli-reference.md @@ -27,8 +27,6 @@ Commands: Examples: ocr review --from master --to dev Review diff range ocr review --commit abc123 Review a single commit - ocr review --background "Focus on auth" --background-file ./docs/requirements.md Review with context - ocr review -B ./docs/requirements.md Review with context file ocr config provider Interactive provider setup ocr config model Interactive model selection ocr config set llm.model opus-4-6 Set a config value @@ -92,20 +90,15 @@ ocr r [flags] (alias) | `--to ` | — | — | Целевая ссылка, на которой заканчивается diff (например, `feature-branch`). Если задана, OCR вычисляет `merge-base(from, to)..to`. | | `--commit ` | `-c` | — | Один коммит для ревью (сравнивается с родительским). | | `--preview` | `-p` | `false` | Запускает конвейер фильтрации, но пропускает LLM. Выводит список файлов и причины исключения. Учитывает `--format json`; `--format sarif` не поддерживается (в предпросмотре нет завершённых находок для вывода). | -| `--no-filter` | — | `false` | Сохраняет все комментарии ревью и пропускает вызов LLM постобработки `REVIEW_FILTER_TASK` для каждого файла. | | `--resume ` | — | — | Возобновляет предыдущую совместимую сессию ревью диапазона или коммита. | | `--format ` | `-f` | `text` | `text` (для чтения человеком), `json` (машиночитаемый массив комментариев) или `sarif` (отчёт SARIF 2.1.0 для GitHub Code Scanning). | | `--audience ` | — | `human` | `human` выводит ход выполнения; `agent` отключает этот вывод в stdout и печатает только итоговую сводку / JSON. | | `--background ` | `-b` | — | Необязательные требования / бизнес-контекст, добавляемые в промпты планирования и основной задачи. | -| `--background-file ` | `-B` | — | Путь к Markdown-файлу с контекстом ревью. Если также задан `--background`, используются оба источника. | -| `--exclude ` | — | — | Разделённые запятыми шаблоны исключения в стиле gitignore; объединяются с excludes из `rule.json`. | | `--concurrency ` | — | `8` | Максимальное число файлов, проверяемых параллельно. | | `--timeout ` | — | `10` | Срок выполнения для каждого файла. `0` отключает тайм-аут. | | `--rule ` | — | — | Путь к пользовательскому JSON-файлу правил ревью. Переопределяет проектный и глобальный `rule.json`. | | `--max-tools ` | — | значение шаблона | Максимальное число раундов вызова инструментов для каждого файла. `0` использует значение шаблона (`30`); значения 1–9 повышаются до `10`; любое значение `≥ 10` переопределяет значение шаблона (даже если оно меньше `30`). | | `--max-tokens ` | — | значение конфигурации или шаблона | Предел токенов запроса для каждого файла. Переопределяет сохранённое значение `max_tokens` для этого запуска. | -| `--max-tokens-budget ` | — | `0` (без ограничения) | Ограничивает общее число входных + выходных токенов ревью. После превышения бюджета новые задачи не запускаются, а частичные результаты всё равно публикуются. | -| `--provider ` | — | — | Выбирает настроенного провайдера для этого запуска. Поддерживаются имена из `providers` и `custom_providers`. | | `--model ` | — | — | Переопределяет выбранную LLM-модель для этого ревью (например, `claude-opus-4-6`). | | `--max-git-procs ` | — | `16` | Максимальное число одновременно выполняемых подпроцессов Git. | | `--tools ` | — | встроенные | Путь к пользовательскому JSON-файлу конфигурации инструментов. Переопределяет встроенные определения инструментов. | @@ -185,10 +178,8 @@ ocr review --commit abc123 --resume - смену provider или model нужно запросить явно через `--provider` / `--model`; изменение, пришедшее из конфигурации или окружения, отклоняется; - у родительского запуска должен быть run manifest — именно по нему проверяется - вход. После начала dispatch файлов Ctrl-C корректно отменяет ревью и записывает - manifest, поэтому завершённые checkpoint можно использовать при возобновлении. - Процесс, завершённый без корректного закрытия, и сессии старше run manifest не - имеют manifest; + вход. Запуск, прерванный Ctrl-C, его не записал, а сессии старше run manifest + его никогда и не имели; - переиспользуются только файлы, судьбу которых зафиксировал manifest родителя. Контрольная точка, которую manifest не подтверждает или которую не удалось прочитать, стоит этому файлу его контрольной точки и не более — он просто diff --git a/pages/src/content/docs/ru/configuration.md b/pages/src/content/docs/ru/configuration.md index 0b86dacc..39f0925a 100644 --- a/pages/src/content/docs/ru/configuration.md +++ b/pages/src/content/docs/ru/configuration.md @@ -57,7 +57,6 @@ API-ключ. Если `providers..api_key` не задан, OCR испо | `hy-tokenplan` | openai | `https://api.lkeap.cloud.tencent.com/plan/v3` | `TENCENT_HUNYUAN_TOKENPLAN_KEY` | | `iflytek` | openai | `https://spark-api-open.xf-yun.com/v1` | `SPARK_API_KEY` | | `kimi` | openai | `https://api.moonshot.cn/v1` | `MOONSHOT_API_KEY` | -| `kimi-global` | openai | `https://api.moonshot.ai/v1` | `MOONSHOT_GLOBAL_API_KEY` | | `z-ai` | openai | `https://open.bigmodel.cn/api/paas/v4` | `Z_AI_API_KEY` | | `mimo` | openai | `https://api.xiaomimimo.com/v1` | `MIMO_API_KEY` | | `minimax` | openai | `https://api.minimax.io/v1` | `MINIMAX_GLOBAL_API_KEY` | @@ -66,7 +65,6 @@ API-ключ. Если `providers..api_key` не задан, OCR испо | `siliconflow` | openai | `https://api.siliconflow.com/v1` | `SILICONFLOW_GLOBAL_API_KEY` | | `siliconflow-cn` | openai | `https://api.siliconflow.cn/v1` | `SILICONFLOW_API_KEY` | | `novita` | openai | `https://api.novita.ai/openai` | `NOVITA_API_KEY` | -| `xai` | openai | `https://api.x.ai/v1` | `XAI_API_KEY` | ### Переопределение Base URL встроенного провайдера diff --git a/pages/src/content/docs/ru/installation.md b/pages/src/content/docs/ru/installation.md index ec3acbbf..a964ee7f 100644 --- a/pages/src/content/docs/ru/installation.md +++ b/pages/src/content/docs/ru/installation.md @@ -74,7 +74,7 @@ sudo port upgrade open-code-review сумму. Он удобен для базовых образов CI и систем без графического интерфейса: ```bash -curl -fsSL https://open-codereview.ai/install.sh | sh +curl -fsSL https://raw.githubusercontent.com/alibaba/open-code-review/main/install.sh | sh ``` Скрипт учитывает две переменные окружения: @@ -89,7 +89,7 @@ curl -fsSL https://open-codereview.ai/install.sh | sh В Windows с PowerShell 5.1 или новее запустите PowerShell-установщик: ```powershell -irm https://open-codereview.ai/install.ps1 | iex +irm https://raw.githubusercontent.com/alibaba/open-code-review/main/install.ps1 | iex ``` Установщик учитывает те же переменные `OCR_INSTALL_DIR` и `OCR_VERSION` (через diff --git a/pages/src/content/docs/zh/cli-reference.md b/pages/src/content/docs/zh/cli-reference.md index 76c5c022..7f6bc3ff 100644 --- a/pages/src/content/docs/zh/cli-reference.md +++ b/pages/src/content/docs/zh/cli-reference.md @@ -26,8 +26,6 @@ Commands: Examples: ocr review --from master --to dev Review diff range ocr review --commit abc123 Review a single commit - ocr review --background "Focus on auth" --background-file ./docs/requirements.md Review with context - ocr review -B ./docs/requirements.md Review with context file ocr config provider Interactive provider setup ocr config model Interactive model selection ocr config set llm.model opus-4-6 Set a config value @@ -89,19 +87,15 @@ unstaged + untracked 变更。 | `--to ` | — | — | diff 结束 ref（如 `feature-branch`）。设置后 OCR 计算 `merge-base(from, to)..to`。 | | `--commit ` | `-c` | — | 评审单个 commit（相对其父）。 | | `--preview` | `-p` | `false` | 运行过滤流水线但跳过 LLM。打印文件列表与排除原因。支持 `--format json`；不支持 `--format sarif`（预览没有已完成的发现可供输出）。 | -| `--no-filter` | — | `false` | 保留所有评审评论，并跳过每个文件的 `REVIEW_FILTER_TASK` LLM 后处理调用。 | | `--resume ` | — | — | 从之前兼容的区间或单 commit 评审会话恢复。 | | `--format ` | `-f` | `text` | `text`（人类可读）、`json`（机器可读的评论数组）或 `sarif`（用于 GitHub Code Scanning 的 SARIF 2.1.0 报告）。 | | `--audience ` | — | `human` | `human` 流式输出进度行；`agent` 静默 stdout，只打印最终摘要 / JSON。 | | `--background ` | `-b` | — | 注入 plan + main prompt 的可选需求 / 业务上下文。 | -| `--background-file ` | `-B` | — | 用作评审背景的 Markdown 文件路径。与 `--background` 同时设置时会合并两者。 | -| `--exclude ` | — | — | 逗号分隔的 gitignore 风格排除模式；与 `rule.json` 的 excludes 合并。 | | `--concurrency ` | — | `8` | 并行评审的最大文件数。 | | `--timeout ` | — | `10` | 每文件截止时间。`0` 关闭超时。 | | `--rule ` | — | — | 自定义 JSON 评审规则文件路径。覆盖项目级与全局 `rule.json`。 | | `--max-tools ` | — | 模板默认 | 每文件最大工具调用轮数。`0` 用模板默认（`30`）；1–9 会被上调到 `10`；任何 `≥ 10` 的值都覆盖模板默认（即使小于 `30`）。 | | `--max-tokens ` | — | 配置或模板默认 | 每文件提示词 token 上限。覆盖本次运行已保存的 `max_tokens` 设置。 | -| `--max-tokens-budget ` | — | `0`（无限制） | 限制本次评审的输入 + 输出 token 总量。超出预算后停止分发，并仍会发布部分结果。 | | `--provider ` | — | — | 为本次运行选择已配置的 provider。支持 `providers` 和 `custom_providers` 中的名称。 | | `--model ` | — | — | 为本次运行覆盖已解析出的 LLM model（如 `claude-opus-4-6`）。 | | `--max-git-procs ` | — | `16` | 并发 git 子进程的最大数。 | @@ -186,9 +180,8 @@ ocr review --commit abc123 --resume 改变了选中的文件集合，整次恢复会被拒绝，而不是部分复用 - 切换 provider 或 model 必须通过 `--provider` / `--model` 显式声明；经由配置 文件或环境变量发生的变化一律拒绝 -- 父运行必须带有 run manifest，输入正是拿它来校验的。文件派发开始后，Ctrl-C 会 - 优雅取消评审并写出 manifest，因此已完成的 checkpoint 仍可恢复；未能优雅关闭的 - 进程和早于 run manifest 的老 session 则没有 manifest +- 父运行必须带有 run manifest，输入正是拿它来校验的。被 Ctrl-C 终止的运行没写出 + manifest，早于 run manifest 的老 session 则从来就没有 - 只有父 manifest 认领过的文件才会复用。manifest 未认领或已损坏的 checkpoint 只 影响它自己那个文件——该文件重新评审一次，其余不受影响 - `--preview` 和 `--resume` 不能同时使用 diff --git a/pages/src/content/docs/zh/configuration.md b/pages/src/content/docs/zh/configuration.md index e33f2438..f3a33e61 100644 --- a/pages/src/content/docs/zh/configuration.md +++ b/pages/src/content/docs/zh/configuration.md @@ -51,7 +51,6 @@ ocr config set providers.anthropic.api_key sk-ant-xxxxxxxxxx | `hy-tokenplan` | openai | `https://api.lkeap.cloud.tencent.com/plan/v3` | `TENCENT_HUNYUAN_TOKENPLAN_KEY` | | `iflytek` | openai | `https://spark-api-open.xf-yun.com/v1` | `SPARK_API_KEY` | | `kimi` | openai | `https://api.moonshot.cn/v1` | `MOONSHOT_API_KEY` | -| `kimi-global` | openai | `https://api.moonshot.ai/v1` | `MOONSHOT_GLOBAL_API_KEY` | | `z-ai` | openai | `https://open.bigmodel.cn/api/paas/v4` | `Z_AI_API_KEY` | | `mimo` | openai | `https://api.xiaomimimo.com/v1` | `MIMO_API_KEY` | | `minimax` | openai | `https://api.minimax.io/v1` | `MINIMAX_GLOBAL_API_KEY` | @@ -60,7 +59,6 @@ ocr config set providers.anthropic.api_key sk-ant-xxxxxxxxxx | `siliconflow` | openai | `https://api.siliconflow.com/v1` | `SILICONFLOW_GLOBAL_API_KEY` | | `siliconflow-cn` | openai | `https://api.siliconflow.cn/v1` | `SILICONFLOW_API_KEY` | | `novita` | openai | `https://api.novita.ai/openai` | `NOVITA_API_KEY` | -| `xai` | openai | `https://api.x.ai/v1` | `XAI_API_KEY` | ### 覆盖内置 provider 的 Base URL diff --git a/pages/src/content/docs/zh/installation.md b/pages/src/content/docs/zh/installation.md index cfb04ebf..41357e7f 100644 --- a/pages/src/content/docs/zh/installation.md +++ b/pages/src/content/docs/zh/installation.md @@ -72,7 +72,7 @@ sudo port upgrade open-code-review 镜像和无界面环境： ```bash -curl -fsSL https://open-codereview.ai/install.sh | sh +curl -fsSL https://raw.githubusercontent.com/alibaba/open-code-review/main/install.sh | sh ``` 它识别两个环境变量： @@ -87,7 +87,7 @@ curl -fsSL https://open-codereview.ai/install.sh | sh 在 Windows（PowerShell 5.1+）上，请改用 PowerShell 安装脚本： ```powershell -irm https://open-codereview.ai/install.ps1 | iex +irm https://raw.githubusercontent.com/alibaba/open-code-review/main/install.ps1 | iex ``` 它同样识别 `OCR_INSTALL_DIR` 与 `OCR_VERSION`（通过 `$env:OCR_INSTALL_DIR` / diff --git a/pages/src/i18n/en.ts b/pages/src/i18n/en.ts index 07023f10..5e168ccf 100644 --- a/pages/src/i18n/en.ts +++ b/pages/src/i18n/en.ts @@ -20,12 +20,6 @@ export const en = { 'hero.copyFailed': 'Copy failed', 'hero.installNpm': 'npm', 'hero.installBrew': 'Homebrew', - 'hero.installMacPorts': 'MacPorts', - 'hero.installMacOS': 'MacOS', - 'hero.installLinux': 'Linux', - 'hero.installWindows': 'Windows', - 'hero.installMore': 'More', - 'hero.allInstallOptions': 'All install options', // Error boundary 'error.pageLoadFailed': 'Failed to load this page.', @@ -166,8 +160,6 @@ export const en = { 'docs.reviewAgentDesc': 'Output only the final summary without execution process, suitable for automation pipelines.', 'docs.reviewPreviewLabel': 'Dry-Run Preview', 'docs.reviewPreviewDesc': 'Lists the files that would be reviewed along with stats (file count, changed lines) — no LLM calls, no cost. Useful for checking scope before a full review.', - 'docs.reviewToggles': 'Review Stage Toggle: --no-filter', - 'docs.reviewTogglesFilterDesc': 'Keeps all review comments and skips the per-file REVIEW_FILTER_TASK LLM post-processing call.', 'docs.reviewFlags': 'Flag Reference', 'docs.reviewFlagCol1': 'Flag', 'docs.reviewFlagCol2': 'Description', diff --git a/pages/src/i18n/ja.ts b/pages/src/i18n/ja.ts index 0b75c705..2ca9bca5 100644 --- a/pages/src/i18n/ja.ts +++ b/pages/src/i18n/ja.ts @@ -22,12 +22,6 @@ export const ja: TranslationKeys = { 'hero.copyFailed': 'コピー失敗', 'hero.installNpm': 'npm', 'hero.installBrew': 'Homebrew', - 'hero.installMacPorts': 'MacPorts', - 'hero.installMacOS': 'MacOS', - 'hero.installLinux': 'Linux', - 'hero.installWindows': 'Windows', - 'hero.installMore': 'その他', - 'hero.allInstallOptions': 'すべてのインストール方法', // Error boundary 'error.pageLoadFailed': 'ページの読み込みに失敗しました。', @@ -170,8 +164,6 @@ export const ja: TranslationKeys = { 'docs.reviewAgentDesc': '実行プロセスを表示せず最終サマリーのみ出力。自動化パイプラインに適しています。', 'docs.reviewPreviewLabel': 'Dry-Run プレビュー', 'docs.reviewPreviewDesc': 'レビュー対象のファイル一覧と統計情報（ファイル数、変更行数）を表示します。LLM 呼び出しなし、コストなし —— 本番レビュー前にスコープを確認するのに役立ちます。', - 'docs.reviewToggles': 'レビューステージの切り替え：--no-filter', - 'docs.reviewTogglesFilterDesc': 'すべてのレビューコメントを保持し、ファイルごとの REVIEW_FILTER_TASK LLM 後処理呼び出しをスキップします。', 'docs.reviewFlags': 'フラグリファレンス', 'docs.reviewFlagCol1': 'フラグ', 'docs.reviewFlagCol2': '説明', diff --git a/pages/src/i18n/ru.ts b/pages/src/i18n/ru.ts index d25d153e..54bb69de 100644 --- a/pages/src/i18n/ru.ts +++ b/pages/src/i18n/ru.ts @@ -22,12 +22,6 @@ export const ru: TranslationKeys = { 'hero.copyFailed': 'Не удалось скопировать', 'hero.installNpm': 'npm', 'hero.installBrew': 'Homebrew', - 'hero.installMacPorts': 'MacPorts', - 'hero.installMacOS': 'MacOS', - 'hero.installLinux': 'Linux', - 'hero.installWindows': 'Windows', - 'hero.installMore': 'Ещё', - 'hero.allInstallOptions': 'Все способы установки', // Error boundary 'error.pageLoadFailed': 'Не удалось загрузить страницу.', @@ -168,8 +162,6 @@ export const ru: TranslationKeys = { 'docs.reviewAgentDesc': 'Показывает только итоговую сводку без хода выполнения. Такой режим удобен для автоматизации.', 'docs.reviewPreviewLabel': 'Предпросмотр без выполнения', 'docs.reviewPreviewDesc': 'Показывает файлы, которые попадут в ревью, их количество и число изменённых строк. LLM не вызывается, токены не расходуются.', - 'docs.reviewToggles': 'Переключатель этапа ревью: --no-filter', - 'docs.reviewTogglesFilterDesc': 'Сохраняет все комментарии ревью и пропускает вызов LLM постобработки REVIEW_FILTER_TASK для каждого файла.', 'docs.reviewFlags': 'Справочник флагов', 'docs.reviewFlagCol1': 'Флаг', 'docs.reviewFlagCol2': 'Описание', diff --git a/pages/src/i18n/zh.ts b/pages/src/i18n/zh.ts index b46c4be5..878d3559 100644 --- a/pages/src/i18n/zh.ts +++ b/pages/src/i18n/zh.ts @@ -22,12 +22,6 @@ export const zh: TranslationKeys = { 'hero.copyFailed': '复制失败', 'hero.installNpm': 'npm', 'hero.installBrew': 'Homebrew', - 'hero.installMacPorts': 'MacPorts', - 'hero.installMacOS': 'MacOS', - 'hero.installLinux': 'Linux', - 'hero.installWindows': 'Windows', - 'hero.installMore': '更多', - 'hero.allInstallOptions': '全部安装方式', // Error boundary 'error.pageLoadFailed': '页面加载失败。', @@ -170,8 +164,6 @@ export const zh: TranslationKeys = { 'docs.reviewAgentDesc': '仅输出最终摘要，不显示执行过程，适用于自动化流水线。', 'docs.reviewPreviewLabel': 'Dry-Run 预览', 'docs.reviewPreviewDesc': '列出将被审查的文件及统计信息（文件总数、变更行数），但不调用 LLM，不产生费用 —— 适合在正式审查前确认范围。', - 'docs.reviewToggles': '审查阶段开关：--no-filter', - 'docs.reviewTogglesFilterDesc': '保留所有评审评论，并跳过每个文件的 REVIEW_FILTER_TASK LLM 后处理调用。', 'docs.reviewFlags': '参数参考', 'docs.reviewFlagCol1': '参数', 'docs.reviewFlagCol2': '描述', diff --git a/plugins/open-code-review/opencode/open-code-review.ts b/plugins/open-code-review/opencode/open-code-review.ts index 180c5208..7220a092 100644 --- a/plugins/open-code-review/opencode/open-code-review.ts +++ b/plugins/open-code-review/opencode/open-code-review.ts @@ -14,7 +14,6 @@ interface ReviewInput { model?: string concurrency?: number timeoutMinutes?: number - overallTimeoutMinutes?: number maxTools?: number maxGitProcesses?: number preview?: boolean @@ -27,7 +26,7 @@ interface OcrInvocation { interface RunOptions { cwd: string - timeoutMs?: number | null + timeoutMs?: number maxOutputBytes?: number invocation?: OcrInvocation signal?: AbortSignal @@ -118,7 +117,7 @@ function appendChunk( async function runOcr(args: string[], options: RunOptions): Promise { const invocation = options.invocation ?? { command: "ocr", prefixArgs: [] } - const timeoutMs = options.timeoutMs === undefined ? 15 * 60 * 1000 : options.timeoutMs + const timeoutMs = options.timeoutMs ?? 15 * 60 * 1000 const maxOutputBytes = options.maxOutputBytes ?? 10 * 1024 * 1024 return await new Promise((resolve, reject) => { @@ -236,19 +235,17 @@ async function runOcr(args: string[], options: RunOptions): Promise { } options.signal?.addEventListener("abort", abort, { once: true }) - if (timeoutMs !== null) { - timer = setTimeout(() => { - terminateChild() - finish(() => reject(new OcrExecutionError( - `OpenCodeReview timed out after ${Math.round(timeoutMs / 1000)} seconds.`, - { - exitCode: null, - stdout: Buffer.concat(stdoutChunks).toString("utf8"), - stderr: Buffer.concat(stderrChunks).toString("utf8"), - }, - ))) - }, timeoutMs) - } + timer = setTimeout(() => { + terminateChild() + finish(() => reject(new OcrExecutionError( + `OpenCodeReview timed out after ${Math.round(timeoutMs / 1000)} seconds.`, + { + exitCode: null, + stdout: Buffer.concat(stdoutChunks).toString("utf8"), + stderr: Buffer.concat(stderrChunks).toString("utf8"), + }, + ))) + }, timeoutMs) if (options.signal?.aborted) { abort() @@ -287,9 +284,6 @@ const reviewArgs = { model: optionalString("Override the model configured in OpenCodeReview."), concurrency: optionalPositiveInt("Maximum concurrent file reviews."), timeoutMinutes: optionalPositiveInt("Per-file OCR timeout in minutes."), - overallTimeoutMinutes: optionalPositiveInt( - "Optional wall-clock timeout for the complete OCR process in minutes.", - ), maxTools: optionalPositiveInt("Maximum tool-call rounds per file; OCR enforces a minimum of 10."), maxGitProcesses: optionalPositiveInt("Maximum concurrent Git subprocesses."), preview: tool.schema.boolean().optional().describe( @@ -336,15 +330,7 @@ export const OpenCodeReviewPlugin: Plugin = async ({ client, worktree }) => { async execute(args, context) { const input = args as ReviewInput const cwd = context.worktree || context.directory || worktree - const defaultOverallMs = 30 * 60 * 1000 - const options: RunOptions = { - cwd, - signal: context.abort, - timeoutMs: input.overallTimeoutMinutes !== undefined - ? input.overallTimeoutMinutes * 60 * 1000 - : defaultOverallMs, - } - const result = await runOcr(buildReviewArgs(input, cwd), options) + const result = await runOcr(buildReviewArgs(input, cwd), { cwd, signal: context.abort }) return formatReviewResult(result, input.preview === true) }, }), diff --git a/plugins/open-code-review/opencode/test/open-code-review.test.mjs b/plugins/open-code-review/opencode/test/open-code-review.test.mjs index 45087428..27a045a1 100644 --- a/plugins/open-code-review/opencode/test/open-code-review.test.mjs +++ b/plugins/open-code-review/opencode/test/open-code-review.test.mjs @@ -2,7 +2,7 @@ // Copyright 2026 alibaba/open-code-review Contributors import assert from "node:assert/strict" -import { access, chmod, copyFile, link, mkdtemp, readFile, rm, writeFile } from "node:fs/promises" +import { access, chmod, mkdtemp, readFile, rm, writeFile } from "node:fs/promises" import { tmpdir } from "node:os" import { delimiter, join } from "node:path" import test from "node:test" @@ -26,35 +26,15 @@ async function withTemporaryDirectory(callback) { try { return await callback(directory) } finally { - await rm(directory, { - recursive: true, - force: true, - maxRetries: 20, - retryDelay: 50, - }) + await rm(directory, { recursive: true, force: true }) } } async function withFakeOcr(source, callback) { return await withTemporaryDirectory(async (directory) => { - if (process.platform === "win32") { - const executable = join(directory, "ocr.exe") - try { - await link(process.execPath, executable) - } catch { - await copyFile(process.execPath, executable) - } - for (const command of ["review", "version", "llm"]) { - await writeFile( - join(directory, command), - `process.argv.splice(2, 0, ${JSON.stringify(command)})\n${source}\n`, - ) - } - } else { - const executable = join(directory, "ocr") - await writeFile(executable, `#!/usr/bin/env node\n${source}\n`) - await chmod(executable, 0o755) - } + const executable = join(directory, "ocr") + await writeFile(executable, `#!/usr/bin/env node\n${source}\n`) + await chmod(executable, 0o755) const previousPath = process.env.PATH process.env.PATH = `${directory}${delimiter}${previousPath ?? ""}` @@ -108,17 +88,10 @@ async function waitForProcessExit(pid, timeoutMs = 5_000) { async function withShortOverallTimeout(timeoutMs, callback) { const originalSetTimeout = globalThis.setTimeout - const overallTimeouts = [] - globalThis.setTimeout = (handler, delay, ...args) => { - if (delay >= 60 * 1000) { - overallTimeouts.push(delay) - return originalSetTimeout(handler, timeoutMs, ...args) - } - return originalSetTimeout(handler, delay, ...args) - } + globalThis.setTimeout = (handler, delay, ...args) => + originalSetTimeout(handler, delay === 15 * 60 * 1000 ? timeoutMs : delay, ...args) try { - await callback() - return overallTimeouts + return await callback() } finally { globalThis.setTimeout = originalSetTimeout } @@ -175,7 +148,7 @@ test("ocr_review creates agent-friendly workspace arguments", async () => { async (worktree) => { const { hooks } = await loadPlugin(worktree) const output = await hooks.tool.ocr_review.execute( - { background: "Add rate limiting", timeoutMinutes: 30 }, + { background: "Add rate limiting" }, toolContext(worktree), ) assert.deepEqual(JSON.parse(output).argv, [ @@ -188,8 +161,6 @@ test("ocr_review creates agent-friendly workspace arguments", async () => { worktree, "--background", "Add rate limiting", - "--timeout", - "30", ]) }, ) @@ -406,44 +377,17 @@ test("ocr_review kills the whole process group on cancellation", { skip: process ) }) -test("ocr_review defaults to 30-minute overall timeout", async () => { - await withFakeOcr( - "console.log('{\"status\":\"success\",\"findings\":[]}')", - async (worktree) => { - const { hooks } = await loadPlugin(worktree) - const originalSetTimeout = globalThis.setTimeout - const overallTimeouts = [] - globalThis.setTimeout = (handler, delay, ...args) => { - if (delay >= 60 * 1000) { - overallTimeouts.push(delay) - } - return originalSetTimeout(handler, delay, ...args) - } - try { - await hooks.tool.ocr_review.execute({}, toolContext(worktree)) - } finally { - globalThis.setTimeout = originalSetTimeout - } - assert.deepEqual(overallTimeouts, [30 * 60 * 1000]) - }, - ) -}) - -test("ocr_review keeps per-file and overall timeouts independent", async () => { +test("ocr_review terminates after its overall timeout", async () => { await withFakeOcr( "setInterval(() => {}, 1000)", async (worktree) => { const { hooks } = await loadPlugin(worktree) - const overallTimeouts = await withShortOverallTimeout(20, async () => { + await withShortOverallTimeout(20, async () => { await assert.rejects( - hooks.tool.ocr_review.execute( - { timeoutMinutes: 30, overallTimeoutMinutes: 45 }, - toolContext(worktree), - ), - /timed out after 2700 seconds/, + hooks.tool.ocr_review.execute({}, toolContext(worktree)), + /timed out after 900 seconds/, ) }) - assert.deepEqual(overallTimeouts, [45 * 60 * 1000]) }, ) })