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

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

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

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

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

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

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

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

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

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

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

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

Fixes #782

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

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

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

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

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

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

---------

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

898 lines
31 KiB
Go

// SPDX-License-Identifier: Apache-2.0
// Copyright 2026 alibaba/open-code-review Contributors
package main
import (
"context"
"crypto/rand"
"errors"
"fmt"
"io"
"net/url"
"os"
"path/filepath"
"strings"
"sync"
"time"
"github.com/alibaba/open-code-review/internal/agent"
"github.com/alibaba/open-code-review/internal/config/rules"
"github.com/alibaba/open-code-review/internal/config/template"
"github.com/alibaba/open-code-review/internal/config/toolsconfig"
"github.com/alibaba/open-code-review/internal/diff"
"github.com/alibaba/open-code-review/internal/gitcmd"
"github.com/alibaba/open-code-review/internal/llm"
"github.com/alibaba/open-code-review/internal/llmloop"
"github.com/alibaba/open-code-review/internal/model"
"github.com/alibaba/open-code-review/internal/session"
"github.com/alibaba/open-code-review/internal/stdout"
"github.com/alibaba/open-code-review/internal/telemetry"
"github.com/alibaba/open-code-review/internal/tool"
)
// commonContext bundles the state that both `ocr review` and `ocr scan`
// need to load *before* deciding whether to dispatch a preview or a real
// LLM session: a validated template, the resolved repo path, review rules,
// and a shared git subprocess limiter.
type commonContext struct {
Template *template.Template
RepoDir string
Resolver rules.Resolver
FileFilter *rules.FileFilter
GitRunner *gitcmd.Runner
// IsGitRepo reports whether RepoDir is inside a git repository. Always
// true when requireGit was set; may be false when scan accepts non-git
// directories.
IsGitRepo bool
}
// resolveMaxTokens applies the per-run CLI override, then the saved setting,
// and finally the embedded task-template default.
func resolveMaxTokens(templateDefault int, cfg *Config, cliOverride int) (int, error) {
if cliOverride < 0 {
return 0, fmt.Errorf("--max-tokens must be a non-negative integer")
}
if cliOverride > 0 {
return cliOverride, nil
}
if cfg == nil || cfg.MaxTokens == 0 {
return templateDefault, nil
}
if cfg.MaxTokens < 0 {
return 0, fmt.Errorf("invalid max_tokens in app config: must be a positive integer")
}
return cfg.MaxTokens, nil
}
// resolveEffort applies the standard precedence for the review effort preset:
// CLI flag > saved app config > EffortDefault.
func resolveEffort(cfg *Config, cliOverride string) (template.Effort, error) {
if cliOverride != "" {
return template.ParseEffort(cliOverride)
}
if cfg != nil && cfg.Effort != "" {
return template.ParseEffort(cfg.Effort)
}
return template.EffortDefault, nil
}
// previewMaxTokens resolves the per-file prompt ceiling the way a real run
// resolves it, but without building an LLM runtime: resolveMaxTokens needs only
// the app config, which loads independently of endpoint resolution. A preview
// therefore reports the limit its run would actually apply while keeping its
// property of requiring no API key.
func previewMaxTokens(templateDefault, cliOverride int) (int, error) {
cfgPath, err := defaultConfigPath()
if err != nil {
return 0, err
}
appCfg, err := LoadAppConfig(cfgPath)
if err != nil {
return 0, fmt.Errorf("load app config: %w", err)
}
return resolveMaxTokens(templateDefault, appCfg, cliOverride)
}
// loadCommonContext validates the working directory, loads the embedded
// template, raises MaxToolRequestTimes when maxTools exceeds the default,
// resolves the absolute repo path, loads system review rules, and creates
// the global git subprocess limiter. Both review and scan callers go
// through this so the startup sequence stays consistent.
//
// requireGit=true fails fast when the directory is not a git repo (review
// path: diff concept requires git). requireGit=false allows non-git
// directories (scan path: provider falls back to filepath.Walk).
//
// contentRef is the git ref whose file content the rule resolver should
// inspect when disambiguating ambiguous extensions — derive it via
// tool.ParseReviewMode(from, to, commit).RefValue(to, commit). Pass "" to
// read the working tree, which is what scan wants.
func loadCommonContext(repoDirInput, rulePath, contentRef string, maxTools, maxGitProcs int, requireGit bool) (*commonContext, error) {
tpl, err := template.LoadDefault()
if err != nil {
return nil, fmt.Errorf("load default template: %w", err)
}
if maxTools > tpl.MaxToolRequestTimes {
tpl.MaxToolRequestTimes = maxTools
}
if err := tpl.Validate(); err != nil {
return nil, fmt.Errorf("invalid config: %w", err)
}
repoDir, isGit, err := resolveWorkingDir(repoDirInput, requireGit)
if err != nil {
return nil, err
}
// Built before the resolver: the sniffer reads file content at contentRef
// through this limiter.
gitRunner := gitcmd.New(maxGitProcs)
resolver, fileFilter, err := rules.NewResolver(repoDir, rulePath, rules.ResolverOptions{
Ref: contentRef,
Runner: gitRunner,
})
if err != nil {
return nil, fmt.Errorf("load rules: %w", err)
}
return &commonContext{
Template: tpl,
RepoDir: repoDir,
Resolver: resolver,
FileFilter: fileFilter,
GitRunner: gitRunner,
IsGitRepo: isGit,
}, nil
}
// resolveWorkingDir returns (absPath, isGitRepo, err). When requireGit is
// true, returns an error if the directory is not a git repo. When false,
// returns IsGitRepo=false instead of erroring (scan path uses this).
func resolveWorkingDir(input string, requireGit bool) (string, bool, error) {
if input == "" {
wd, err := os.Getwd()
if err != nil {
return "", false, fmt.Errorf("get working directory: %w", err)
}
input = wd
}
absPath, err := filepath.Abs(input)
if err != nil {
return "", false, fmt.Errorf("resolve absolute path: %w", err)
}
if _, statErr := os.Stat(absPath); statErr != nil {
return "", false, fmt.Errorf("stat %s: %w", absPath, statErr)
}
out, err := runGitCmd(absPath, "rev-parse", "--git-dir")
isGit := err == nil && len(out) > 0
if !isGit && requireGit {
return "", false, fmt.Errorf("%s is not a git repository", absPath)
}
// #287: git reports diff and `git show HEAD:<path>` paths relative to the
// repository root, not the current directory. When `ocr review` runs from a
// subdirectory of a monorepo, anchor RepoDir at the git top-level so those
// root-relative paths resolve for both disk reads and git-show reads.
// requireGit is true only for the review path; scan (requireGit=false) keeps
// the CWD so its `git ls-files` walk stays scoped to the subdirectory.
if isGit && requireGit {
// runGitCmdStdout captures stdout only so git stderr notices can't
// pollute the resolved path. --show-toplevel fails (or is empty) when
// there is no work tree — e.g. a bare repo, where --git-dir succeeds so
// isGit is true. Fail loudly there instead of silently reusing the
// subdir, which would reproduce the #287 root-relative-path bug.
top, topErr := runGitCmdStdout(absPath, "rev-parse", "--show-toplevel")
t := strings.TrimSpace(string(top))
if topErr != nil || t == "" {
return "", false, fmt.Errorf("%s is a git repository without a work tree (bare repo?); cannot resolve its top level for review", absPath)
}
absPath = t
}
return absPath, isGit, nil
}
// llmRuntime bundles the LLM-side state both subcommands need once they've
// decided to actually run a session: tool definitions, an app-language
// adjusted template (mutated in place via ApplyLanguage), the LLM client,
// the resolved model name, and a fresh comment collector.
type llmRuntime struct {
Client llm.LLMClient
Model string
Provider string // resolved provider name (non-secret label; empty for non-provider endpoints)
PlanToolDefs []llm.ToolDef
MainToolDefs []llm.ToolDef
Collector *tool.CommentCollector
// RetryCollector observes every LLM HTTP attempt this run makes. It is
// created here rather than on the session or the agent because the client is
// built before either exists, and it is per-run rather than package-level so
// two runs in one process cannot share data. scan gets one too; its requests
// carry no RequestMeta, so every attempt is dropped and the frozen report is
// nil.
RetryCollector *llm.RetryCollector
// RawHolder is the opt-in raw LLM capture sink (OCR_RAW_LOGGING=1),
// created with the client because the middleware mounts at construction;
// the per-session writer is bound later by bindRawWriter. Nil when off.
RawHolder *llm.RawHolder
AppCfg *Config
// RuntimeConfig holds the allowlisted, non-secret runtime settings (protocol,
// sanitized endpoint host, language, timeout) derived from the resolved
// endpoint and app config, for the run manifest's runtime_config_sha256. It
// never carries the token or full URL.
RuntimeConfig agent.RuntimeConfig
}
// newRetryCollector builds the per-run retry collector. It is a variable so a
// test can hand back a collector whose invariants are already violated, which is
// the only way to exercise the Freeze construction-error branch from the
// outside: every production path finalizes every logical request on every exit,
// so a well-behaved run can never produce one.
var newRetryCollector = llm.NewRetryCollector
// loadLLMRuntime loads tool defs from toolConfigPath, reads the app config
// from the user's default config path (applying the configured language to
// tpl — defaulting when the config file is absent), resolves the LLM
// endpoint (honoring resolveOpts), and
// returns the runtime bundle. tpl is mutated in place.
func loadLLMRuntime(tpl *template.Template, toolConfigPath string, resolveOpts llm.ResolveOptions) (*llmRuntime, error) {
toolEntries, err := toolsconfig.Load(toolConfigPath)
if err != nil {
return nil, fmt.Errorf("load tools: %w", err)
}
planToolDefs := agent.BuildToolDefs(toolEntries, true)
mainToolDefs := agent.BuildToolDefs(toolEntries, false)
cfgPath, err := defaultConfigPath()
if err != nil {
return nil, err
}
appCfg, err := LoadAppConfig(cfgPath)
if err != nil {
return nil, fmt.Errorf("load app config: %w", err)
}
// Apply the language directive even when the config file is missing
// (upstream #fix: ApplyLanguage with empty lang falls back to default).
var lang string
if appCfg != nil {
lang = appCfg.Language
}
tpl.ApplyLanguage(lang)
ep, err := llm.ResolveEndpointWithOptions(cfgPath, resolveOpts)
if err != nil {
return nil, fmt.Errorf("resolve LLM endpoint: %w", err)
}
retryCollector := newRetryCollector()
var rawHolder *llm.RawHolder
if llm.RawLoggingEnabled() {
rawHolder = llm.NewRawHolder()
}
return &llmRuntime{
Client: llm.NewLLMClient(ep, retryCollector, rawHolder),
Model: ep.Model,
Provider: ep.Provider,
PlanToolDefs: planToolDefs,
MainToolDefs: mainToolDefs,
Collector: tool.NewCommentCollector(),
RetryCollector: retryCollector,
RawHolder: rawHolder,
AppCfg: appCfg,
RuntimeConfig: agent.RuntimeConfig{
Protocol: ep.Protocol,
EndpointHost: sanitizeEndpointHost(ep.URL),
Language: lang,
Timeout: ep.Timeout,
},
}, nil
}
// bindRawWriter opens the session's raw capture file and attaches it to the
// run's raw holder; defer the returned closer. A nil holder (capture off)
// or an open failure returns a no-op closer: raw capture must never fail a
// review.
func bindRawWriter(holder *llm.RawHolder, repoDir string, sess *session.SessionHistory) func() {
noop := func() {}
if holder == nil {
return noop
}
w, err := session.NewRawFileWriter(repoDir, sess.SessionID)
if err != nil {
fmt.Fprintf(os.Stderr, "[ocr] WARNING: raw logging disabled for this run: %v\n", err)
return noop
}
holder.Set(w)
return func() {
// Detach before closing: LLM calls that run after this closer must
// bypass capture, not write to a closed file. Without this the
// guarantee depends on defer registration order.
holder.Set(nil)
if err := w.Close(); err != nil {
fmt.Fprintf(os.Stderr, "[ocr] WARNING: close raw file: %v\n", err)
}
}
}
// sanitizeEndpointHost extracts the credential-free host[:port] from a full LLM
// endpoint URL, dropping scheme, any embedded userinfo, path, query and fragment
// so no secret material survives into the manifest's runtime_config hash. The
// host is lowercased for a stable identity (DNS is case-insensitive). An empty
// or unparseable URL, or one without a host, yields "".
func sanitizeEndpointHost(rawURL string) string {
if strings.TrimSpace(rawURL) == "" {
return ""
}
u, err := url.Parse(rawURL)
if err != nil || u.Host == "" {
return ""
}
return strings.ToLower(u.Host) // u.Host is host[:port]; userinfo lives in u.User
}
// applyCLIExcludes appends user-supplied --exclude patterns (already split
// into a []string) onto cc.FileFilter.Exclude. Creates the FileFilter if
// none was returned by rule.json layers. Idempotent on empty input.
func applyCLIExcludes(cc *commonContext, patterns []string) {
if len(patterns) == 0 {
return
}
if cc.FileFilter == nil {
cc.FileFilter = &rules.FileFilter{}
}
cc.FileFilter.Exclude = append(cc.FileFilter.Exclude, patterns...)
}
// excludeToolDef returns a copy of defs with any entries whose function name
// matches name removed. Used by `ocr scan` to hide tools that don't make
// sense in full-scan mode (e.g. file_read_diff).
func excludeToolDef(defs []llm.ToolDef, name string) []llm.ToolDef {
out := make([]llm.ToolDef, 0, len(defs))
for _, d := range defs {
if d.Function.Name == name {
continue
}
out = append(out, d)
}
return out
}
// quietHandle wraps the restorer returned by whichever stdout redirection
// newQuietHandle chose, so callers can `defer q.Restore()` for safety while
// emitRunResult restores it early when the agent-text audience needs the trace
// summary on the user's terminal. Restore is idempotent.
type quietHandle struct {
fn func()
}
// isMachineReadable reports whether the output format writes a structured
// document to stdout that must not be interleaved with progress text. Both
// json and sarif move [ocr] progress lines off stdout and suppress the trace
// summary, which is already carried inside the document.
func isMachineReadable(outputFormat string) bool {
switch strings.ToLower(strings.TrimSpace(outputFormat)) {
case "json", "sarif":
return true
default:
return false
}
}
// newQuietHandle routes [ocr] progress lines away from stdout so they cannot
// corrupt a structured output document. What it does depends on why stdout
// needs protecting:
//
// - audience=="agent": the caller wants no progress at all, so progress is
// discarded regardless of format.
// - machine-readable format with a human audience: the human still asked to
// watch the run, so progress is redirected to stderr rather than dropped.
// Every result document (json, sarif, text) is written straight to
// os.Stdout and never through stdout.Writer(), so stdout stays a single
// parseable document while stderr carries the live progress.
// - otherwise: no-op, progress stays on stdout.
func newQuietHandle(outputFormat, audience string) *quietHandle {
h := &quietHandle{}
switch {
case audience == "agent":
h.fn = stdout.Quiet()
case isMachineReadable(outputFormat):
h.fn = stdout.Swap(os.Stderr)
}
return h
}
// Restore re-enables stdout. Safe to call multiple times.
func (h *quietHandle) Restore() {
if h == nil || h.fn == nil {
return
}
h.fn()
h.fn = nil
}
const (
maxOSCSequenceLength = 4096
maxCSISequenceLength = 256
)
// stripAnsiState is the ANSI escape parsing state for stripAnsiWriter.
type stripAnsiState int
const (
ansiNormal stripAnsiState = iota
ansiEsc
ansiCSI
ansiOSC
ansiOSCEsc
)
// stripAnsiWriter removes ANSI escape sequences (colors, cursor moves, OSC
// control strings) from the byte stream it forwards, so terminal-only
// decoration never reaches --output files. It tolerates sequences split
// arbitrarily across Write calls: any in-progress sequence is buffered and
// carried to the next Write. Payload bytes (non-ESC) pass through unchanged,
// so stripping only discards decoration, never result content.
type stripAnsiWriter struct {
dst io.Writer
state stripAnsiState
pending []byte // partial escape sequence carried across Write calls
}
func (w *stripAnsiWriter) Write(p []byte) (int, error) {
// Only new bytes are fed to the state machine. w.pending holds bytes that
// already entered an escape sequence in earlier calls — re-feeding them
// would re-parse the sequence start (e.g. '[' would be mistaken for a CSI
// final byte once the state is already ansiCSI) and leak the sequence into
// the output. The pending buffer is discarded wholesale on completion.
var out []byte
for _, c := range p {
switch w.state {
case ansiNormal:
if c == 0x1b {
w.state = ansiEsc
w.pending = append(w.pending, c)
} else {
out = append(out, c)
}
case ansiEsc:
w.pending = append(w.pending, c)
switch {
case c == '[':
w.state = ansiCSI
case c == ']', c == 'P', c == '^', c == '_':
// OSC, DCS, PM and APC strings all run until ST (or BEL for
// OSC); treat them uniformly through the OSC state.
w.state = ansiOSC
case c >= 0x20 && c <= 0x2f:
// Intermediate byte of a multi-byte escape (e.g. ESC ( B);
// keep collecting so the whole sequence is discarded.
if len(w.pending) >= maxCSISequenceLength {
out = append(out, w.pending...)
w.pending = w.pending[:0]
w.state = ansiNormal
}
default:
// Single-byte escape sequence. Discard.
w.state = ansiNormal
w.pending = w.pending[:0]
}
case ansiCSI:
w.pending = append(w.pending, c)
if c >= 0x40 && c <= 0x7e {
w.state = ansiNormal
w.pending = w.pending[:0]
} else if c < 0x20 || c >= 0x80 || len(w.pending) >= maxCSISequenceLength {
out = append(out, w.pending...)
w.pending = w.pending[:0]
w.state = ansiNormal
}
case ansiOSC:
w.pending = append(w.pending, c)
switch c {
case 0x07: // BEL terminates an OSC string
w.state = ansiNormal
w.pending = w.pending[:0]
case 0x1b: // possible ST terminator (ESC \)
w.state = ansiOSCEsc
default:
if len(w.pending) >= maxOSCSequenceLength {
out = append(out, w.pending...)
w.pending = w.pending[:0]
w.state = ansiNormal
}
}
case ansiOSCEsc:
if c == '\\' { // ST terminates the OSC string
w.state = ansiNormal
w.pending = w.pending[:0]
} else {
// Not a valid ST. The OSC ends here without one (e.g. a bare
// ESC terminator, which some terminals accept). The trailing
// byte is not part of the sequence and must be re-parsed:
// an ESC starts a new escape sequence, anything else is text.
w.state = ansiNormal
w.pending = w.pending[:0]
if c == 0x1b {
w.state = ansiEsc
w.pending = append(w.pending, c)
} else {
out = append(out, c)
}
}
}
}
if len(out) > 0 {
n, err := w.dst.Write(out)
if err != nil {
// The state machine has already consumed p; report the error but
// still return len(p) so a caller that retries on n < len(p) does
// not feed the same bytes through the state machine a second time.
return len(p), err
}
if n != len(out) {
return len(p), io.ErrShortWrite
}
}
return len(p), nil
}
const outputTempPrefix = ".ocr-out-"
// createOutputTemp creates a collision-resistant file in the target directory.
// Unlike os.CreateTemp, the 0666 mode preserves os.Create's umask-sensitive
// permissions for new output files.
func createOutputTemp(dir string) (*os.File, error) {
for range 100 {
path := filepath.Join(dir, outputTempPrefix+rand.Text())
f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o666)
if err == nil {
return f, nil
}
if !os.IsExist(err) {
return nil, err
}
}
return nil, fmt.Errorf("too many temporary output filename collisions in %s", dir)
}
// resolveOutputCommitPath preserves os.Create's behavior for an existing
// symlink: write through the link to its target instead of replacing the link
// itself during the final rename.
func resolveOutputCommitPath(path string) (string, error) {
info, err := os.Lstat(path)
if err != nil {
if os.IsNotExist(err) {
return path, nil
}
return "", fmt.Errorf("inspect output path %s: %w", path, err)
}
if info.Mode()&os.ModeSymlink == 0 {
return path, nil
}
resolved, err := filepath.EvalSymlinks(path)
if err != nil {
return "", fmt.Errorf("resolve output symlink %s: %w", path, err)
}
return resolved, nil
}
func cleanupOutputTemp(file *os.File, path string) error {
var cleanupErr error
if file != nil {
if err := file.Close(); err != nil && !errors.Is(err, os.ErrClosed) {
cleanupErr = errors.Join(cleanupErr, fmt.Errorf("close temporary output file: %w", err))
}
}
if path != "" {
if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
cleanupErr = errors.Join(cleanupErr, fmt.Errorf("remove temporary output file %s: %w", path, err))
}
}
return cleanupErr
}
// lazyFileWriter defers creating a same-directory temporary file until the
// first Write. Close syncs and atomically renames a complete result into place,
// so a run that never writes or encounters a write failure leaves the previous
// target untouched. Replacing the directory entry intentionally gives readers
// a consistent old-or-new snapshot; open descriptors and hard links keep
// referring to the old inode.
type lazyFileWriter struct {
path string
strip bool // strip ANSI when the target format is text
once sync.Once
file *os.File // temporary file committed by Close
tempPath string
commitPath string
stripper *stripAnsiWriter
err error // temporary-file creation or setup error
writeErr error // first error from a Write
closed bool
closeErr error
}
func (w *lazyFileWriter) Write(p []byte) (int, error) {
if w.closed {
return 0, os.ErrClosed
}
w.once.Do(func() {
commitPath, err := resolveOutputCommitPath(w.path)
if err != nil {
w.err = err
return
}
var existingMode os.FileMode
hasExistingMode := false
if info, statErr := os.Stat(commitPath); statErr == nil {
existingMode = info.Mode() & (os.ModePerm | os.ModeSetuid | os.ModeSetgid | os.ModeSticky)
hasExistingMode = true
} else if !os.IsNotExist(statErr) {
w.err = fmt.Errorf("inspect output file %s: %w", w.path, statErr)
return
}
f, err := createOutputTemp(filepath.Dir(commitPath))
if err != nil {
w.err = fmt.Errorf("create temporary output file for %s: %w", w.path, err)
return
}
if hasExistingMode {
if err := f.Chmod(existingMode); err != nil {
cleanupErr := cleanupOutputTemp(f, f.Name())
w.err = errors.Join(fmt.Errorf("preserve output permissions for %s: %w", w.path, err), cleanupErr)
return
}
}
w.file = f
w.tempPath = f.Name()
w.commitPath = commitPath
if w.strip {
w.stripper = &stripAnsiWriter{dst: f}
}
})
if w.err != nil {
return 0, w.err
}
var n int
var err error
if w.stripper != nil {
n, err = w.stripper.Write(p)
} else {
n, err = w.file.Write(p)
}
if err != nil && w.writeErr == nil {
w.writeErr = err
}
return n, err
}
// Err returns the first error encountered while creating or writing the
// underlying file, or nil if none occurred. Text-mode rendering drops the
// per-write errors of fmt.Fprintf, so callers use this to surface write
// failures (e.g. permission denied on the first write) as a command error —
// matching JSON mode, where Encoder.Encode propagates the same failure.
func (w *lazyFileWriter) Err() error {
if w.err != nil {
return w.err
}
return w.writeErr
}
// writeOutError surfaces deferred write errors from writers that record them
// (lazyFileWriter); plain writers such as os.Stdout report nil.
func writeOutError(out io.Writer) error {
if r, ok := out.(interface{ Err() error }); ok {
return r.Err()
}
return nil
}
// Close commits a complete output file atomically. It is a no-op when no Write
// occurred, and it discards the temporary file when a Write already failed.
func (w *lazyFileWriter) Close() error {
if w.closed {
return w.closeErr
}
w.closed = true
if w.file == nil {
return nil
}
if w.writeErr != nil {
w.closeErr = cleanupOutputTemp(w.file, w.tempPath)
w.file = nil
w.tempPath = ""
return w.closeErr
}
if err := w.file.Sync(); err != nil {
cleanupErr := cleanupOutputTemp(w.file, w.tempPath)
w.file = nil
w.tempPath = ""
w.closeErr = errors.Join(fmt.Errorf("sync output file %s: %w", w.path, err), cleanupErr)
return w.closeErr
}
if err := w.file.Close(); err != nil {
cleanupErr := cleanupOutputTemp(w.file, w.tempPath)
w.file = nil
w.tempPath = ""
w.closeErr = errors.Join(fmt.Errorf("close output file %s: %w", w.path, err), cleanupErr)
return w.closeErr
}
w.file = nil
if err := os.Rename(w.tempPath, w.commitPath); err != nil {
cleanupErr := cleanupOutputTemp(nil, w.tempPath)
w.tempPath = ""
w.closeErr = errors.Join(fmt.Errorf("replace output file %s: %w", w.path, err), cleanupErr)
return w.closeErr
}
w.tempPath = ""
fmt.Fprintf(os.Stderr, "[ocr] Results written to %s\n", w.path)
return nil
}
// resolveOutputWriter resolves the --output target into a writer plus a
// cleanup function.
// - "" or "-" → os.Stdout with a no-op cleanup (colors preserved, no hint)
// - otherwise → a lazyFileWriter over a same-directory temporary file,
// deferred until the first Write and atomically committed by Close; text
// format wraps the file in stripAnsiWriter so ANSI colors never reach it.
//
// Fail-fast checks (directory target, missing parent) run here without
// creating or truncating anything; deeper errors (permissions, disk) surface
// on the first Write and fail the command non-zero.
func resolveOutputWriter(path, format string) (io.Writer, func() error, error) {
if path == "" || path == "-" {
return os.Stdout, func() error { return nil }, nil
}
if st, err := os.Stat(path); err == nil && st.IsDir() {
return nil, nil, fmt.Errorf("--output %q is a directory", path)
}
parent := filepath.Dir(path)
if st, err := os.Stat(parent); err != nil || !st.IsDir() {
return nil, nil, fmt.Errorf("--output directory does not exist: %s", parent)
}
w := &lazyFileWriter{path: path, strip: !isMachineReadable(format)}
return w, w.Close, nil
}
// ResultProvider abstracts the metadata both internal/agent.Agent and
// internal/scan.Agent expose post-run, so emitRunResult can finalize
// either without knowing which kind it has.
type ResultProvider interface {
Diffs() []model.Diff
FilesReviewed() int64
TotalInputTokens() int64
TotalOutputTokens() int64
TotalTokensUsed() int64
TotalCacheReadTokens() int64
TotalCacheWriteTokens() int64
Warnings() []agent.AgentWarning
// ProjectSummary is the markdown project-level summary produced by
// scan's PROJECT_SUMMARY_TASK. Empty for review mode and for scans
// that skipped / failed the summary phase.
ProjectSummary() string
ToolCalls() map[string]int64
ToolFailures() []llmloop.ToolFailureDetail
// SessionID returns the persisted session identifier so callers can show it
// in JSON output or failure diagnostics. Returns "" when no session was
// created.
SessionID() string
// BudgetExceeded reports whether the aggregate token budget gate stopped the
// run before all files were reviewed. It is a diagnostic signal only — it
// feeds summary.budget_exceeded and the failure usage record, and never
// decides the run's terminal state. The terminal state comes solely from the
// manifest's coverage: the stop marks the undispatched items
// failed(budget) without recording a run_failure, so it reads as partial
// whenever anything was covered.
BudgetExceeded() bool
// RunManifest returns the frozen v1 coverage result for review runs. Scan
// remains legacy and returns nil.
RunManifest() *session.RunManifest
}
type resumeInfoProvider interface {
ResumeInfo() *agent.ResumeInfo
}
// emitRunResult is the post-LLM-run finalization shared by `ocr review` and
// `ocr scan`: resolves comment line numbers, records telemetry, restores
// stdout early for agent-text audiences so the summary is visible, prints
// the trace summary, and writes the result in the requested format.
//
// q is the silencing handle returned by newQuietHandle; pass nil if no
// silencing was set up (in which case the early restore is a no-op).
//
// retryReport is the frozen LLM retry report, or nil when there is nothing to
// report (a clean run, or a caller that produces no report at all — `ocr scan`
// never freezes one). It is passed as a parameter rather than added to
// ResultProvider because the collector belongs to llmRuntime, not to the
// agent; putting it on the interface would force internal/scan.Agent to
// implement a method that is always nil.
func emitRunResult(
ctx context.Context,
ag ResultProvider,
comments []model.LlmComment,
startTime time.Time,
outputFormat, audience string,
q *quietHandle,
llmIdentity *jsonLLMIdentity,
out io.Writer,
retryReport *llm.RetryReport,
) error {
outputFormat = strings.ToLower(strings.TrimSpace(outputFormat))
comments = diff.ResolveLineNumbers(comments, ag.Diffs())
duration := time.Since(startTime)
telemetry.RecordReviewDuration(ctx, duration)
if len(comments) > 0 {
telemetry.RecordCommentsGenerated(ctx, int64(len(comments)))
}
traceID := telemetry.TraceIDFromContext(ctx)
manifest := ag.RunManifest()
// JSON and SARIF are machine-readable formats written to stdout; they
// share the same suppression of trace summaries and early stdout restore.
machineReadable := isMachineReadable(outputFormat)
if machineReadable && manifest == nil && len(comments) == 0 && ag.FilesReviewed() == 0 {
if outputFormat == "json" {
return outputJSONNoFiles(traceID, llmIdentity, out)
}
return outputSARIF(nil, Version, ag.Warnings(), manifest, out)
}
// Agent-text audiences need stdout back before PrintTraceSummary so the
// summary line lands on their terminal.
if audience == "agent" && !machineReadable {
q.Restore()
}
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(),
})
}
if outputFormat == "json" {
var resumeInfo *agent.ResumeInfo
if p, ok := ag.(resumeInfoProvider); ok {
resumeInfo = p.ResumeInfo()
}
var groups []agent.FileGroupInfo
if p, ok := ag.(interface{ FileGroups() []agent.FileGroupInfo }); ok {
groups = p.FileGroups()
}
return outputJSONWithWarnings(comments, ag.Warnings(), ag.FilesReviewed(),
ag.TotalInputTokens(), ag.TotalOutputTokens(), ag.TotalTokensUsed(),
ag.TotalCacheReadTokens(), ag.TotalCacheWriteTokens(), duration,
ag.ProjectSummary(), ag.ToolCalls(), ag.ToolFailures(), traceID, resumeInfo, ag.SessionID(), manifest, ag.BudgetExceeded(), llmIdentity, out, retryReport, groups)
}
if outputFormat == "sarif" {
return outputSARIF(comments, Version, ag.Warnings(), manifest, out)
}
outputTextWithWarnings(comments, ag.Warnings(), manifest, out)
// Between the comments/warnings block and the project summary: the report is
// run-level diagnostics about how the comments were obtained, so it reads
// after them but must not separate the summary from the end of output.
outputRetryReportText(out, retryReport)
if summary := ag.ProjectSummary(); summary != "" {
fmt.Fprintf(out, "\n\n──────── Project Summary ────────\n\n%s\n", sanitizeTerminal(summary))
}
// Text rendering ignores fmt.Fprintf write errors; surface them here so a
// failed --output write (permission, disk full) fails the command non-zero
// exactly like JSON mode does via Encoder.Encode.
return writeOutError(out)
}