Merge pull request #105 from qiankunli/fix/filereader-rename-fatal-json

fix: file_read rename awareness + fatal-run JSON contract

Author: qiankunli

cmd/ccr/output.go

@@ -176,13 +176,13 @@ func buildDiffLines(comment model.LlmComment) []suggestdiff.DiffLine {
 }
 
 type jsonSummary struct {
-	FilesReviewed    int64  `json:"files_reviewed"`
-	Comments         int64  `json:"comments"`
-	TotalTokens      int64  `json:"total_tokens"`
-	InputTokens      int64  `json:"input_tokens"`
-	OutputTokens     int64  `json:"output_tokens"`
-	CacheReadTokens  int64  `json:"cache_read_tokens,omitempty"`
-	CacheWriteTokens int64  `json:"cache_write_tokens,omitempty"`
+	FilesReviewed    int64 `json:"files_reviewed"`
+	Comments         int64 `json:"comments"`
+	TotalTokens      int64 `json:"total_tokens"`
+	InputTokens      int64 `json:"input_tokens"`
+	OutputTokens     int64 `json:"output_tokens"`
+	CacheReadTokens  int64 `json:"cache_read_tokens,omitempty"`
+	CacheWriteTokens int64 `json:"cache_write_tokens,omitempty"`
 	// ElapsedSec is the review's wall-clock cost in whole seconds — numeric so
 	// consumers (e.g. an MR-comment header) never parse a Go duration string.
 	ElapsedSec int64 `json:"elapsed_sec"`
@@ -276,6 +276,29 @@ func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentW
 	return enc.Encode(out)
 }
 
+// outputJSONFatal emits the failed-run JSON shape. In --format json the stdout
+// contract is "always exactly one JSON object": a fatal run error must not
+// leave stdout empty, or downstream parsers report their own parse failure
+// instead of the actual error. Warnings ride along — on an all-units-failed
+// run they carry the per-unit error reasons.
+func outputJSONFatal(runErr error, warnings []agent.AgentWarning) error {
+	out := jsonOutput{
+		Status:   "failed",
+		Version:  Version,
+		Message:  runErr.Error(),
+		Comments: []model.LlmComment{},
+		ToolCalls: &jsonToolCalls{
+			ByTool: map[string]int64{},
+		},
+	}
+	if len(warnings) > 0 {
+		out.Warnings = warnings
+	}
+	enc := json.NewEncoder(os.Stdout)
+	enc.SetIndent("", "  ")
+	return enc.Encode(out)
+}
+
 func outputJSONNoFiles() error {
 	out := jsonOutput{
 		Status:   "skipped",

cmd/ccr/output_test.go

@@ -1,6 +1,14 @@
 package main
 
-import "testing"
+import (
+	"encoding/json"
+	"errors"
+	"io"
+	"os"
+	"testing"
+
+	"github.com/qiankunli/case-code-review/internal/agent"
+)
 
 func TestSanitizeTerminal(t *testing.T) {
 	tests := []struct {
@@ -33,3 +41,42 @@ func TestSanitizeTerminal(t *testing.T) {
 		})
 	}
 }
+
+func TestOutputJSONFatal_EmitsParseableFailedJSON(t *testing.T) {
+	old := os.Stdout
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	os.Stdout = w
+	emitErr := outputJSONFatal(
+		errors.New("all 3 unit review(s) failed"),
+		[]agent.AgentWarning{{Type: "unit_error", File: "a.go", Message: "context deadline exceeded"}},
+	)
+	w.Close()
+	os.Stdout = old
+	if emitErr != nil {
+		t.Fatal(emitErr)
+	}
+
+	raw, err := io.ReadAll(r)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var out jsonOutput
+	if err := json.Unmarshal(raw, &out); err != nil {
+		t.Fatalf("stdout is not valid JSON: %v\n%s", err, raw)
+	}
+	if out.Status != "failed" {
+		t.Errorf("status = %q, want failed", out.Status)
+	}
+	if out.Message != "all 3 unit review(s) failed" {
+		t.Errorf("message = %q", out.Message)
+	}
+	if len(out.Warnings) != 1 || out.Warnings[0].Type != "unit_error" {
+		t.Errorf("warnings not carried through: %+v", out.Warnings)
+	}
+	if out.Comments == nil {
+		t.Error("comments must be [] not null")
+	}
+}

cmd/ccr/review_cmd.go

@@ -119,6 +119,11 @@ func runReview(args []string) error {
 	comments, err := ag.Run(ctx)
 	if err != nil {
 		telemetry.SetAttr(span, "error", err.Error())
+		// Fatal or not, json mode must put one JSON object on stdout — the
+		// exit code still signals failure for CLI callers.
+		if opts.outputFormat == "json" {
+			_ = outputJSONFatal(err, ag.Warnings())
+		}
 		return fmt.Errorf("review failed: %w", err)
 	}
 

cmd/ccr/scan_cmd.go

@@ -213,6 +213,10 @@ func runScan(args []string) error {
 	comments, err := ag.Run(ctx)
 	if err != nil {
 		telemetry.SetAttr(span, "error", err.Error())
+		// Same stdout contract as review: json mode always emits one JSON object.
+		if opts.outputFormat == "json" {
+			_ = outputJSONFatal(err, ag.Warnings())
+		}
 		return fmt.Errorf("scan failed: %w", err)
 	}
 

internal/agent/agent.go

@@ -447,22 +447,37 @@ func (a *Agent) loadDiffs(ctx context.Context) error {
 }
 
 // injectDiffMap builds a read-only DiffMap from parsed diffs and sets it
-// on the FileReadDiffProvider. Must be called after loadDiffs and before
-// any concurrent access to the registry.
+// on the FileReadDiffProvider, plus the rename/delete path map on the
+// FileReadProvider (so a read of a path this diff moved or removed gets
+// redirected/explained instead of a raw git miss). Must be called after
+// loadDiffs and before any concurrent access to the registry.
 func (a *Agent) injectDiffMap() {
 	m := make(map[string]string, len(a.diffs))
+	renamedTo := make(map[string]string)
+	deleted := make(map[string]bool)
 	for i := range a.diffs {
 		d := &a.diffs[i]
 		if d.NewPath != "/dev/null" {
 			m[d.NewPath] = d.Diff
 		}
+		if d.IsRenamed && d.OldPath != "" && d.NewPath != "" && d.OldPath != d.NewPath {
+			renamedTo[d.OldPath] = d.NewPath
+		}
+		if d.IsDeleted && d.OldPath != "" && d.OldPath != "/dev/null" {
+			deleted[d.OldPath] = true
+		}
 	}
 	dm := tool.NewDiffMap(m)
 	if p, ok := a.args.Tools.Get(tool.FileReadDiff.Name()); ok {
 		if frd, ok := p.(*tool.FileReadDiffProvider); ok {
 			frd.SetDiffMap(dm)
 		}
 	}
+	if p, ok := a.args.Tools.Get(tool.FileRead.Name()); ok {
+		if frp, ok := p.(*tool.FileReadProvider); ok {
+			frp.SetDiffPaths(tool.NewDiffPaths(renamedTo, deleted))
+		}
+	}
 }
 
 // dispatchUnits runs the Plan + Main phases for each review Unit concurrently.

internal/tool/file_read.go

@@ -3,18 +3,46 @@ package tool
 import (
 	"context"
 	"fmt"
+	"maps"
 	"strings"
 )
 
 const fileReadMaxLines = 500
 
+// DiffPaths records what this review's diff did to paths that no longer exist
+// at the review ref (renames and deletions). A file_read miss on such a path
+// is the model following a stale reference from the diff itself (rename
+// headers, leftover imports), so it gets redirected or explained instead of
+// surfacing a raw git error the model can't act on. Frozen after
+// construction; safe for concurrent reads.
+type DiffPaths struct {
+	renamedTo map[string]string // old path -> new path
+	deleted   map[string]bool   // old path -> removed in this diff
+}
+
+// NewDiffPaths creates a frozen DiffPaths from plain maps.
+func NewDiffPaths(renamedTo map[string]string, deleted map[string]bool) DiffPaths {
+	r := make(map[string]string, len(renamedTo))
+	maps.Copy(r, renamedTo)
+	d := make(map[string]bool, len(deleted))
+	maps.Copy(d, deleted)
+	return DiffPaths{renamedTo: r, deleted: d}
+}
+
 // FileReadProvider reads file content at a given path and optional line range.
 type FileReadProvider struct {
 	FileReader *FileReader
+	diffPaths  DiffPaths
 }
 
 func NewFileRead(fr *FileReader) *FileReadProvider { return &FileReadProvider{FileReader: fr} }
 
+// SetDiffPaths installs the rename/delete map for this run. Must be called
+// before concurrent access begins (same contract as FileReadDiffProvider.SetDiffMap).
+func (p *FileReadProvider) SetDiffPaths(dp DiffPaths) {
+	p.diffPaths = dp
+}
+
 func (p *FileReadProvider) Tool() Tool { return FileRead }
 
 func (p *FileReadProvider) Execute(ctx context.Context, args map[string]any) (string, error) {
@@ -44,6 +72,18 @@ func (p *FileReadProvider) Execute(ctx context.Context, args map[string]any) (st
 	}
 
 	lines, totalLines, err := p.FileReader.ReadLines(ctx, filePath, int(startLine), maxLines)
+	var renameNote string
+	if err != nil {
+		// The miss may be the model chasing a path this very diff moved or
+		// removed (rename headers and stale imports keep the old path visible).
+		if to, ok := p.diffPaths.renamedTo[filePath]; ok {
+			renameNote = fmt.Sprintf("NOTE: %q was renamed to %q in this diff; showing the renamed file.\n", filePath, to)
+			filePath = to
+			lines, totalLines, err = p.FileReader.ReadLines(ctx, filePath, int(startLine), maxLines)
+		} else if p.diffPaths.deleted[filePath] {
+			return fmt.Sprintf("File %q was deleted in this diff; it no longer exists at the review ref. Use file_read_diff to see the removed content.", filePath), nil
+		}
+	}
 	if err != nil {
 		return "", fmt.Errorf("file %q not found: %w", filePath, err)
 	}
@@ -62,6 +102,7 @@ func (p *FileReadProvider) Execute(ctx context.Context, args map[string]any) (st
 	displayEnd := int(startLine) - 1 + len(lines)
 
 	var sb strings.Builder
+	sb.WriteString(renameNote)
 	sb.WriteString(fmt.Sprintf("File: %s (Total lines: %d)\n", filePath, totalLines))
 	sb.WriteString(fmt.Sprintf("IS_TRUNCATED: %t\n", truncated))
 	sb.WriteString(fmt.Sprintf("LINE_RANGE: %d-%d\n", int(startLine), displayEnd))

internal/tool/file_read_test.go

@@ -362,3 +362,50 @@ func TestExecute_WithEndLine(t *testing.T) {
 		t.Error("line 5 should not appear")
 	}
 }
+
+func TestExecute_RenamedPath_RedirectsToNewPath(t *testing.T) {
+	dir := t.TempDir()
+	writeTestFile(t, dir, "new.txt", "hello\nworld\n")
+
+	p := NewFileRead(&FileReader{RepoDir: dir, Mode: ModeWorkspace})
+	p.SetDiffPaths(NewDiffPaths(map[string]string{"old.txt": "new.txt"}, nil))
+
+	out, err := p.Execute(context.Background(), map[string]any{"file_path": "old.txt"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !strings.Contains(out, `renamed to "new.txt"`) {
+		t.Errorf("expected rename note, got:\n%s", out)
+	}
+	if !strings.Contains(out, "File: new.txt") {
+		t.Errorf("expected redirected header, got:\n%s", out)
+	}
+	if !strings.Contains(out, "1|hello") {
+		t.Errorf("expected redirected content, got:\n%s", out)
+	}
+}
+
+func TestExecute_DeletedPath_ExplainsDeletion(t *testing.T) {
+	dir := t.TempDir()
+
+	p := NewFileRead(&FileReader{RepoDir: dir, Mode: ModeWorkspace})
+	p.SetDiffPaths(NewDiffPaths(nil, map[string]bool{"gone.txt": true}))
+
+	out, err := p.Execute(context.Background(), map[string]any{"file_path": "gone.txt"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !strings.Contains(out, "was deleted in this diff") {
+		t.Errorf("expected deletion explanation, got:\n%s", out)
+	}
+}
+
+func TestExecute_MissWithoutDiffPaths_StillErrors(t *testing.T) {
+	dir := t.TempDir()
+
+	p := NewFileRead(&FileReader{RepoDir: dir, Mode: ModeWorkspace})
+
+	if _, err := p.Execute(context.Background(), map[string]any{"file_path": "nope.txt"}); err == nil {
+		t.Fatal("expected not-found error for a path absent from the diff maps")
+	}
+}