perf: optimize scan prepare memory and staleness checks

cmd/opencodereview/agent_cmd.go

@@ -336,7 +336,7 @@ func executeAgentScanPrepare(
 	writer io.Writer,
 ) error {
 	started := time.Now()
-	manifest, encoded, err := reviewbundle.PrepareScan(ctx, reviewbundle.ScanOptions{
+	scanOptions := reviewbundle.ScanOptions{
 		RepoDir:          repoDir,
 		Paths:            splitPaths(options.paths),
 		Resolver:         resolver,
@@ -347,7 +347,20 @@ func executeAgentScanPrepare(
 		MaxBundleSize:    int64(options.maxBundleBytes),
 		BatchStrategy:    options.batchStrategy,
 		BatchSize:        options.batchSize,
-	})
+	}
+	var outputFile *os.File
+	if options.outputPath != "" && !options.preview {
+		file, err := os.OpenFile(options.outputPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600)
+		if err != nil {
+			return fmt.Errorf("open scan manifest output %s: %w", options.outputPath, err)
+		}
+		outputFile = file
+		scanOptions.EncodedWriter = file
+	}
+	manifest, encoded, err := reviewbundle.PrepareScan(ctx, scanOptions)
+	if closeErr := closeAgentOutputFile(outputFile); closeErr != nil && err == nil {
+		err = closeErr
+	}
 	if err != nil {
 		return fmt.Errorf("prepare agent scan manifest: %w", err)
 	}
@@ -382,12 +395,25 @@ func executeAgentScanPrepare(
 		return nil
 	}
 	if options.outputPath != "" {
+		if outputFile != nil {
+			return nil
+		}
 		return writePrivateFile(options.outputPath, encoded)
 	}
+	if len(encoded) == 0 {
+		return fmt.Errorf("scan manifest encoding is empty")
+	}
 	_, err = writer.Write(append(encoded, '\n'))
 	return err
 }
 
+func closeAgentOutputFile(file *os.File) error {
+	if file == nil {
+		return nil
+	}
+	return file.Close()
+}
+
 func recordAgentEvent(
 	repoDir string,
 	sessionID string,

internal/reviewbundle/read_limit.go

@@ -33,6 +33,32 @@ func validateProtocolDocumentSize(encoded []byte) error {
 	return nil
 }
 
+type protocolDocumentWriter struct {
+	w        io.Writer
+	n        int64
+	limitErr error
+}
+
+func (writer *protocolDocumentWriter) Write(data []byte) (int, error) {
+	writer.n += int64(len(data))
+	if writer.n > MaxProtocolDocumentBytes {
+		writer.limitErr = &ProtocolError{
+			Code: "manifest_too_large",
+			Message: fmt.Sprintf(
+				"document exceeds %d byte protocol limit (%d bytes); reduce scope or bundle count",
+				MaxProtocolDocumentBytes,
+				writer.n,
+			),
+		}
+		return 0, writer.limitErr
+	}
+	return writer.w.Write(data)
+}
+
+func (writer *protocolDocumentWriter) limitError() error {
+	return writer.limitErr
+}
+
 func readLimited(reader io.Reader) ([]byte, error) {
 	limited := io.LimitReader(reader, MaxProtocolDocumentBytes+1)
 	data, err := io.ReadAll(limited)

internal/reviewbundle/scan.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"sort"
 
 	"github.com/open-code-review/open-code-review/internal/config/rules"
@@ -27,6 +28,7 @@ type ScanOptions struct {
 	MaxBundleSize    int64
 	BatchStrategy    string
 	BatchSize        int
+	EncodedWriter    io.Writer
 }
 
 // ScanManifest links all deterministic full-file review bundles.
@@ -133,12 +135,19 @@ func PrepareScan(ctx context.Context, options ScanOptions) (*ScanManifest, []byt
 		); err != nil {
 			return nil, nil, err
 		}
+		clearScanItemsContent(batch)
 	}
 	manifestID, err := computeManifestID(manifest)
 	if err != nil {
 		return nil, nil, err
 	}
 	manifest.ManifestID = manifestID
+	if options.EncodedWriter != nil {
+		if err := encodeScanManifest(manifest, options.EncodedWriter); err != nil {
+			return nil, nil, err
+		}
+		return manifest, nil, nil
+	}
 	encoded, err := json.Marshal(manifest)
 	if err != nil {
 		return nil, nil, fmt.Errorf("marshal scan manifest: %w", err)
@@ -149,6 +158,21 @@ func PrepareScan(ctx context.Context, options ScanOptions) (*ScanManifest, []byt
 	return manifest, encoded, nil
 }
 
+func clearScanItemsContent(items []model.ScanItem) {
+	for index := range items {
+		items[index].Content = ""
+	}
+}
+
+func encodeScanManifest(manifest *ScanManifest, writer io.Writer) error {
+	limitWriter := &protocolDocumentWriter{w: writer}
+	encoder := json.NewEncoder(limitWriter)
+	if err := encoder.Encode(manifest); err != nil {
+		return fmt.Errorf("marshal scan manifest: %w", err)
+	}
+	return limitWriter.limitError()
+}
+
 func filterAndBudgetScanItems(
 	ctx context.Context,
 	manifest *ScanManifest,

internal/reviewbundle/scan_test.go

@@ -41,6 +41,28 @@ func TestPrepareScanBuildsDeterministicGroupedManifest(t *testing.T) {
 		string(firstJSON) != string(secondJSON) {
 		t.Fatal("scan manifest is not deterministic")
 	}
+	var streamed strings.Builder
+	streamedManifest, streamedJSON, err := PrepareScan(context.Background(), ScanOptions{
+		RepoDir:       repository,
+		Paths:         options.Paths,
+		Resolver:      options.Resolver,
+		FileFilter:    options.FileFilter,
+		GitRunner:     options.GitRunner,
+		BatchStrategy: options.BatchStrategy,
+		BatchSize:     options.BatchSize,
+		MaxBundleSize: options.MaxBundleSize,
+		EncodedWriter: &streamed,
+	})
+	if err != nil {
+		t.Fatalf("PrepareScan(stream) error = %v", err)
+	}
+	if streamedJSON != nil {
+		t.Fatalf("streamed encoded = %v, want nil", streamedJSON)
+	}
+	if streamedManifest.ManifestID != first.ManifestID ||
+		strings.TrimSpace(streamed.String()) != strings.TrimSpace(string(firstJSON)) {
+		t.Fatal("streamed scan manifest differs from buffered encoding")
+	}
 	if first.Summary.TotalFiles != 4 || first.Summary.ReviewableFiles != 3 ||
 		first.Summary.ExcludedFiles != 1 {
 		t.Fatalf("summary = %+v", first.Summary)

internal/reviewbundle/target_io.go

@@ -0,0 +1,71 @@
+package reviewbundle
+
+import (
+	"crypto/sha256"
+	"encoding/binary"
+	"encoding/hex"
+	"fmt"
+	"hash"
+	"io"
+	"os"
+	"path/filepath"
+	"strings"
+)
+
+func resolveScanTargetPath(repoDir, path string) (string, error) {
+	root, err := filepath.EvalSymlinks(repoDir)
+	if err != nil {
+		return "", err
+	}
+	full := filepath.Join(root, filepath.FromSlash(path))
+	resolved, err := filepath.EvalSymlinks(full)
+	if err != nil {
+		return "", err
+	}
+	relative, err := filepath.Rel(root, resolved)
+	if err != nil || relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) {
+		return "", fmt.Errorf("resolved path escapes repository")
+	}
+	return resolved, nil
+}
+
+func hashScanTargetFileAtPath(repoDir, path string) (string, error) {
+	resolved, err := resolveScanTargetPath(repoDir, path)
+	if err != nil {
+		return "", err
+	}
+	info, err := os.Stat(resolved)
+	if err != nil {
+		return "", err
+	}
+	file, err := os.Open(resolved)
+	if err != nil {
+		return "", err
+	}
+	defer file.Close()
+	return hashStreamedFileContent(file, info.Size())
+}
+
+func hashStreamedFileContent(file *os.File, size int64) (string, error) {
+	hasher := sha256.New()
+	if err := writeLengthPrefixedStream(hasher, file, size); err != nil {
+		return "", err
+	}
+	return "sha256:" + hex.EncodeToString(hasher.Sum(nil)), nil
+}
+
+func writeLengthPrefixedStream(hasher hash.Hash, reader io.Reader, size int64) error {
+	var length [8]byte
+	binary.BigEndian.PutUint64(length[:], uint64(size))
+	if _, err := hasher.Write(length[:]); err != nil {
+		return err
+	}
+	written, err := io.Copy(hasher, reader)
+	if err != nil {
+		return err
+	}
+	if written != size {
+		return fmt.Errorf("file size changed while hashing")
+	}
+	return nil
+}

internal/reviewbundle/target_io_test.go

@@ -0,0 +1,64 @@
+package reviewbundle
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestHashScanTargetFileMatchesBundleContentSHA256(t *testing.T) {
+	repository := t.TempDir()
+	content := []byte("package sample\n\nfunc ScanHashTarget() {}\n")
+	path := "scan.go"
+	if err := os.WriteFile(filepath.Join(repository, path), content, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	digest, err := hashScanTargetFileAtPath(repository, path)
+	if err != nil {
+		t.Fatalf("hashScanTargetFileAtPath() error = %v", err)
+	}
+	if digest != hashFields(content) {
+		t.Fatalf("hash = %q, want %q", digest, hashFields(content))
+	}
+}
+
+func TestValidateCommentsDetectsStaleScanBundle(t *testing.T) {
+	repository := t.TempDir()
+	path := "scan.go"
+	content := []byte("package sample\n\nfunc Stale() {}\n")
+	if err := os.WriteFile(filepath.Join(repository, path), content, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	bundle := &Bundle{
+		SchemaVersion: BundleSchemaVersion,
+		BundleID:      "sha256:scan",
+		Target:        Target{Mode: TargetScan},
+		Summary:       Summary{ReviewableFiles: 1},
+		Files: []File{{
+			Path:          path,
+			Reviewable:    true,
+			ContentSHA256: hashFields(content),
+		}},
+		Contract: DefaultContract(),
+	}
+	comments := &Comments{
+		SchemaVersion: CommentsSchemaVersion,
+		BundleID:      bundle.BundleID,
+		Comments:      []ReviewComment{},
+		Summary:       CommentsSummary{IssuesFound: 0, FilesReviewed: 0},
+	}
+	result := ValidateComments(t.Context(), bundle, comments, repository, nil)
+	if !result.Valid {
+		t.Fatalf("ValidateComments() valid = false, errors = %+v", result.Errors)
+	}
+	if err := os.WriteFile(filepath.Join(repository, path), []byte("package sample\n\nfunc Changed() {}\n"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	result = ValidateComments(t.Context(), bundle, comments, repository, nil)
+	if result.Valid {
+		t.Fatal("ValidateComments() valid = true, want stale scan bundle")
+	}
+	if len(result.Errors) == 0 || result.Errors[0].Code != "stale_bundle" {
+		t.Fatalf("errors = %+v, want stale_bundle", result.Errors)
+	}
+}

internal/reviewbundle/validate.go

@@ -119,8 +119,8 @@ func validateFreshTarget(
 ) {
 	if bundle.Target.Mode == TargetScan {
 		for _, file := range bundle.Files {
-			content, err := readTargetFile(ctx, bundle, repoDir, file.Path, runner)
-			if err != nil || hashFields(content) != file.ContentSHA256 {
+			digest, err := hashScanTargetFileAtPath(repoDir, file.Path)
+			if err != nil || digest != file.ContentSHA256 {
 				addValidationError(
 					result,
 					"stale_bundle",
@@ -316,19 +316,10 @@ func readTargetFile(
 			bundle.Target.HeadSHA+":"+path,
 		)
 	}
-	root, err := filepath.EvalSymlinks(repoDir)
+	resolved, err := resolveScanTargetPath(repoDir, path)
 	if err != nil {
 		return nil, err
 	}
-	full := filepath.Join(root, filepath.FromSlash(path))
-	resolved, err := filepath.EvalSymlinks(full)
-	if err != nil {
-		return nil, err
-	}
-	relative, err := filepath.Rel(root, resolved)
-	if err != nil || relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) {
-		return nil, fmt.Errorf("resolved path escapes repository")
-	}
 	return os.ReadFile(resolved)
 }