diff --git a/cmd/opencodereview/agent_cmd.go b/cmd/opencodereview/agent_cmd.go index 738faeeb..1e0d47bd 100644 --- a/cmd/opencodereview/agent_cmd.go +++ b/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, diff --git a/internal/reviewbundle/read_limit.go b/internal/reviewbundle/read_limit.go index d24ded70..1021a05e 100644 --- a/internal/reviewbundle/read_limit.go +++ b/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) diff --git a/internal/reviewbundle/scan.go b/internal/reviewbundle/scan.go index 7ea47711..2774c566 100644 --- a/internal/reviewbundle/scan.go +++ b/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, diff --git a/internal/reviewbundle/scan_test.go b/internal/reviewbundle/scan_test.go index 042689b6..568fb3f0 100644 --- a/internal/reviewbundle/scan_test.go +++ b/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) diff --git a/internal/reviewbundle/target_io.go b/internal/reviewbundle/target_io.go new file mode 100644 index 00000000..83f7533c --- /dev/null +++ b/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 +} diff --git a/internal/reviewbundle/target_io_test.go b/internal/reviewbundle/target_io_test.go new file mode 100644 index 00000000..52a2d803 --- /dev/null +++ b/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) + } +} diff --git a/internal/reviewbundle/validate.go b/internal/reviewbundle/validate.go index 7b08fa56..323e79bf 100644 --- a/internal/reviewbundle/validate.go +++ b/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) } diff --git a/internal/scan/provider.go b/internal/scan/provider.go index 9839644b..efc6922b 100644 --- a/internal/scan/provider.go +++ b/internal/scan/provider.go @@ -132,32 +132,24 @@ func (p *Provider) EnumerateDetailed( skipped = append(skipped, SkippedItem{Path: rel, Reason: "file_size"}) continue } - binary, err := isBinaryFile(full) + binary, content, lineCount, err := readRegularFile(full) if err != nil { - fmt.Fprintf(os.Stderr, "[ocr] WARNING: cannot sniff %s: %v\n", rel, err) + fmt.Fprintf(os.Stderr, "[ocr] WARNING: cannot read %s: %v\n", rel, err) skipped = append(skipped, SkippedItem{Path: rel, Reason: "unreadable"}) continue } if binary { - // Emit placeholder so preview can display [B], but do not - // read the file body — saves memory on large binaries. out = append(out, model.ScanItem{ Path: rel, IsBinary: true, }) continue } - content, err := os.ReadFile(full) - if err != nil { - fmt.Fprintf(os.Stderr, "[ocr] WARNING: cannot read %s: %v\n", rel, err) - skipped = append(skipped, SkippedItem{Path: rel, Reason: "unreadable"}) - continue - } out = append(out, model.ScanItem{ Path: rel, Content: string(content), IsBinary: false, - LineCount: countLines(content), + LineCount: lineCount, }) } return out, skipped, nil @@ -316,6 +308,34 @@ func countLines(content []byte) int { return n } +// readRegularFile opens path once, sniffs for binary content, and reads the body. +func readRegularFile(path string) (binary bool, content []byte, lineCount int, err error) { + file, err := os.Open(path) + if err != nil { + return false, nil, 0, err + } + defer file.Close() + sniff := make([]byte, binarySniffWindow) + read, err := io.ReadFull(file, sniff) + if err != nil && !errors.Is(err, io.EOF) && !errors.Is(err, io.ErrUnexpectedEOF) { + return false, nil, 0, err + } + if bytes.IndexByte(sniff[:read], 0) >= 0 { + return true, nil, 0, nil + } + var body bytes.Buffer + if read > 0 { + if _, err := body.Write(sniff[:read]); err != nil { + return false, nil, 0, err + } + } + if _, err := io.Copy(&body, file); err != nil { + return false, nil, 0, err + } + content = body.Bytes() + return false, content, countLines(content), nil +} + // isBinaryFile reads up to binarySniffWindow bytes from path and reports // whether they contain a NUL byte (git's "binary" heuristic). func isBinaryFile(path string) (bool, error) {