diff --git a/docs/internal/comparison-publication.md b/docs/internal/comparison-publication.md index b636dc7..516f800 100644 --- a/docs/internal/comparison-publication.md +++ b/docs/internal/comparison-publication.md @@ -15,12 +15,13 @@ filesystem validation then require that exact path and file set. Destination planning is read-only. It requires an exact absolute target that is neither the filesystem root nor the working directory, rejects unsafe symlinks and non-directories, accepts a missing or empty directory, and permits -replacement only for a recognized current bundle. Publication rechecks that -authorization immediately before it writes a private sibling staging directory. -For replacement, it moves the prior bundle to a private sibling backup, -reauthorizes that moved entry, checks for cancellation, and restores it if -cancellation or installing the new bundle prevents replacement. If guarded -restoration fails, the error retains the prior bundle's recovery path. +replacement only for a recognized current bundle. Publication rechecks the +destination namespace and type immediately before it writes a private sibling +staging directory. For replacement, it moves the prior bundle to a private +sibling backup, fully reauthorizes that moved entry, checks for cancellation, +and restores it if cancellation or installing the new bundle prevents +replacement. If guarded restoration fails, the error retains the prior bundle's +recovery path. Planning also validates the final component and the bounded fixed names used for private staging and backup siblings. A destination that cannot form those @@ -37,8 +38,9 @@ complete recognized recovery bundle, partial remnants, an absent sibling, or an uninspectable state. A recovery path is reported only when something remains; only a complete recognized bundle is suitable for rollback recovery. -The application preflights before prompt inspection and collection, then -preflights again before publication. A cancellation or any failure before the +The application preflights before prompt inspection and collection. Publication +performs its transaction-boundary checks and final moved-destination +authorization before installation. A cancellation or any failure before the commit leaves the prior destination untouched. Completed bundles include partial profile results; comparison publication never coordinates Distributor notification. Operator-facing lifecycle and cleanup are in the diff --git a/internal/app/comparison.go b/internal/app/comparison.go index eea496a..3b1f330 100644 --- a/internal/app/comparison.go +++ b/internal/app/comparison.go @@ -110,7 +110,7 @@ func compareDetailed(ctx context.Context, req ComparisonRequest, publish compari return result, err } result.OutputDirectory = outputDirectory - _, err = comparison.PlanDestination(req.WorkingDir, outputDirectory, req.Replace) + publicationPlan, err := comparison.PlanDestination(req.WorkingDir, outputDirectory, req.Replace) if err != nil { return result, fmt.Errorf("preflight comparison destination: %w", err) } @@ -150,10 +150,6 @@ func compareDetailed(ctx context.Context, req ComparisonRequest, publish compari if err := bundle.Validate(); err != nil { return result, fmt.Errorf("build comparison bundle: %w", err) } - publicationPlan, err := comparison.PlanDestination(req.WorkingDir, outputDirectory, req.Replace) - if err != nil { - return result, fmt.Errorf("re-preflight comparison destination: %w", err) - } publication, err := publish(ctx, publicationPlan, bundle) if publication.Committed { result.OutputDirectory = publicationPlan.Target diff --git a/internal/comparison/publish.go b/internal/comparison/publish.go index 37b7719..c587edd 100644 --- a/internal/comparison/publish.go +++ b/internal/comparison/publish.go @@ -73,6 +73,18 @@ var ErrUnrecognizedBundle = errors.New("unrecognized comparison bundle") // PlanDestination performs the read-only comparison destination preflight. func PlanDestination(workingDirectory, target string, replace bool) (DestinationPlan, error) { + return planDestination(workingDirectory, target, replace, RecognizeBundle) +} + +func planDestination(workingDirectory, target string, replace bool, recognize func(string) (Manifest, error)) (DestinationPlan, error) { + plan, err := newDestinationPlan(workingDirectory, target, replace) + if err != nil { + return DestinationPlan{}, err + } + return inspectDestination(plan, recognize) +} + +func newDestinationPlan(workingDirectory, target string, replace bool) (DestinationPlan, error) { workingDirectory, err := absoluteCleanPath(workingDirectory) if err != nil { return DestinationPlan{}, newDestinationError(DestinationInvalidPath, workingDirectory, err) @@ -91,7 +103,19 @@ func PlanDestination(workingDirectory, target string, replace bool) (Destination return DestinationPlan{}, newDestinationError(DestinationInvalidPath, target, err) } - plan := DestinationPlan{WorkingDirectory: workingDirectory, Target: target, Replace: replace} + return DestinationPlan{WorkingDirectory: workingDirectory, Target: target, Replace: replace}, nil +} + +func reauthorizeDestination(plan DestinationPlan) (DestinationPlan, error) { + plan, err := newDestinationPlan(plan.WorkingDirectory, plan.Target, plan.Replace) + if err != nil { + return DestinationPlan{}, err + } + return inspectDestination(plan, nil) +} + +func inspectDestination(plan DestinationPlan, recognize func(string) (Manifest, error)) (DestinationPlan, error) { + target := plan.Target info, err := os.Lstat(target) if err != nil { if !errors.Is(err, os.ErrNotExist) { @@ -117,11 +141,13 @@ func PlanDestination(workingDirectory, target string, replace bool) (Destination plan.state = destinationEmpty return plan, nil } - if !replace { + if !plan.Replace { return DestinationPlan{}, newDestinationError(DestinationNotEmpty, target, nil) } - if _, err := RecognizeBundle(target); err != nil { - return DestinationPlan{}, newDestinationError(DestinationUnrecognized, target, err) + if recognize != nil { + if _, err := recognize(target); err != nil { + return DestinationPlan{}, newDestinationError(DestinationUnrecognized, target, err) + } } plan.state = destinationBundle return plan, nil @@ -188,7 +214,25 @@ func newDestinationError(kind DestinationErrorKind, target string, err error) er // RecognizeBundle verifies that directory contains exactly one valid current // comparison bundle. It never follows bundle entries through symlinks. func RecognizeBundle(directory string) (Manifest, error) { - info, err := os.Lstat(directory) + return recognizeBundle(directory, defaultRecognitionOperations) +} + +type recognitionOperations struct { + lstat func(string) (os.FileInfo, error) + readDir func(string) ([]os.DirEntry, error) + open func(string) (io.ReadCloser, error) +} + +var defaultRecognitionOperations = recognitionOperations{ + lstat: os.Lstat, + readDir: os.ReadDir, + open: func(path string) (io.ReadCloser, error) { + return os.Open(path) + }, +} + +func recognizeBundle(directory string, operations recognitionOperations) (Manifest, error) { + info, err := operations.lstat(directory) if err != nil { return Manifest{}, unrecognizedBundleError("inspect directory", err) } @@ -196,11 +240,11 @@ func RecognizeBundle(directory string) (Manifest, error) { return Manifest{}, unrecognizedBundleError("directory is not a real directory", nil) } - entries, err := os.ReadDir(directory) + entries, err := operations.readDir(directory) if err != nil { return Manifest{}, unrecognizedBundleError("read directory", err) } - manifestData, err := readBundleFile(directory, ManifestFilename) + manifestData, err := readBundleFile(directory, ManifestFilename, operations) if err != nil { return Manifest{}, err } @@ -228,12 +272,15 @@ func RecognizeBundle(directory string) (Manifest, error) { if _, ok := expected[entry.Name()]; !ok { return Manifest{}, unrecognizedBundleError("directory has an undeclared entry", nil) } - if _, err := readBundleFile(directory, entry.Name()); err != nil { + if entry.Name() == ManifestFilename || entry.Name() == DataPackageFilename { + continue + } + if err := inspectBundleFile(directory, entry.Name(), operations); err != nil { return Manifest{}, err } } - dataPackage, err := readBundleFile(directory, DataPackageFilename) + dataPackage, err := readBundleFile(directory, DataPackageFilename, operations) if err != nil { return Manifest{}, err } @@ -243,23 +290,47 @@ func RecognizeBundle(directory string) (Manifest, error) { return manifest, nil } -func readBundleFile(directory, name string) ([]byte, error) { +func readBundleFile(directory, name string, operations recognitionOperations) ([]byte, error) { + file, err := openBundleFile(directory, name, operations) + if err != nil { + return nil, err + } + defer file.Close() + data, err := io.ReadAll(file) + if err != nil { + return nil, unrecognizedBundleError("read bundle entry", err) + } + return data, nil +} + +func inspectBundleFile(directory, name string, operations recognitionOperations) error { + file, err := openBundleFile(directory, name, operations) + if err != nil { + return err + } + if err := file.Close(); err != nil { + return unrecognizedBundleError("close bundle entry", err) + } + return nil +} + +func openBundleFile(directory, name string, operations recognitionOperations) (io.ReadCloser, error) { if !isArtifactBasename(name) { return nil, unrecognizedBundleError("bundle entry name is unsafe", nil) } filePath := filepath.Join(directory, name) - info, err := os.Lstat(filePath) + info, err := operations.lstat(filePath) if err != nil { return nil, unrecognizedBundleError("inspect bundle entry", err) } if info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() { return nil, unrecognizedBundleError("bundle entry is not a regular file", nil) } - data, err := os.ReadFile(filePath) + file, err := operations.open(filePath) if err != nil { return nil, unrecognizedBundleError("read bundle entry", err) } - return data, nil + return file, nil } func decodeManifest(data []byte) (Manifest, error) { @@ -499,6 +570,7 @@ type publishOperations struct { rename func(string, string) error removeAll func(string) error beforeCommit func() + recognize func(string) (Manifest, error) } func publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle, operations publishOperations) (PublicationResult, error) { @@ -508,6 +580,9 @@ func publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle, op if operations.removeAll == nil { operations.removeAll = os.RemoveAll } + if operations.recognize == nil { + operations.recognize = RecognizeBundle + } if err := bundle.Validate(); err != nil { return PublicationResult{}, fmt.Errorf("validate comparison bundle: %w", err) } @@ -518,7 +593,7 @@ func publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle, op if err := ctx.Err(); err != nil { return PublicationResult{}, err } - plan, err = PlanDestination(plan.WorkingDirectory, plan.Target, plan.Replace) + plan, err = reauthorizeDestination(plan) if err != nil { return PublicationResult{}, err } @@ -544,7 +619,7 @@ func publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle, op return PublicationResult{}, err } - currentPlan, err := PlanDestination(plan.WorkingDirectory, plan.Target, plan.Replace) + currentPlan, err := reauthorizeDestination(plan) if err != nil { return PublicationResult{}, err } @@ -569,7 +644,7 @@ func publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle, op if err := operations.rename(currentPlan.Target, backupDirectory); err != nil { return PublicationResult{}, fmt.Errorf("back up comparison destination %q: %w", currentPlan.Target, err) } - if err := authorizeMovedDestination(currentPlan, backupDirectory); err != nil { + if err := authorizeMovedDestination(currentPlan, backupDirectory, operations.recognize); err != nil { return PublicationResult{}, restoreMovedDestination(operations, backupDirectory, currentPlan.Target, err) } if err := ctx.Err(); err != nil { @@ -600,8 +675,8 @@ func inspectBackupRecovery(backupDirectory string) (BackupRecoveryState, string) return BackupRecoveryPartial, backupDirectory } -func authorizeMovedDestination(plan DestinationPlan, backupDirectory string) error { - backupPlan, err := PlanDestination(plan.WorkingDirectory, backupDirectory, plan.Replace) +func authorizeMovedDestination(plan DestinationPlan, backupDirectory string, recognize func(string) (Manifest, error)) error { + backupPlan, err := planDestination(plan.WorkingDirectory, backupDirectory, plan.Replace, recognize) if err != nil { return fmt.Errorf("authorize moved comparison destination %q: %w", backupDirectory, err) } diff --git a/internal/comparison/publish_test.go b/internal/comparison/publish_test.go index 88d8d78..216c789 100644 --- a/internal/comparison/publish_test.go +++ b/internal/comparison/publish_test.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "errors" + "fmt" + "io" "os" "path/filepath" "runtime" @@ -222,6 +224,75 @@ func TestPlanDestinationAcceptsRecognizedReplacement(t *testing.T) { } } +func TestRecognizeBundleReadsOnlyRequiredArtifactContents(t *testing.T) { + for _, reportCount := range []int{2, 12} { + t.Run(fmt.Sprintf("%d reports", reportCount), func(t *testing.T) { + bundle := testBundleWithReports(t, reportCount) + directory := publishTestBundle(t, bundle) + bytesRead := make(map[string]int) + opens := make(map[string]int) + operations := defaultRecognitionOperations + operations.open = func(path string) (io.ReadCloser, error) { + file, err := os.Open(path) + if err != nil { + return nil, err + } + name := filepath.Base(path) + opens[name]++ + return &countingReadCloser{ReadCloser: file, count: func(n int) { + bytesRead[name] += n + }}, nil + } + + if _, err := recognizeBundle(directory, operations); err != nil { + t.Fatalf("recognizeBundle() error = %v", err) + } + for _, name := range []string{ManifestFilename, DataPackageFilename} { + data := readFile(t, filepath.Join(directory, name)) + if opens[name] != 1 || bytesRead[name] != len(data) { + t.Fatalf("%s opens/bytes = %d/%d, want 1/%d", name, opens[name], bytesRead[name], len(data)) + } + } + for _, report := range bundle.Reports { + if opens[report.Path] != 1 || bytesRead[report.Path] != 0 { + t.Fatalf("report %s opens/bytes = %d/%d, want 1/0", report.Path, opens[report.Path], bytesRead[report.Path]) + } + } + }) + } +} + +func TestReplacementUsesEarlyAndFinalRecognition(t *testing.T) { + workingDirectory := t.TempDir() + target := filepath.Join(workingDirectory, "comparison-daily") + bundle := testBundleWithReports(t, 12) + initialPlan, err := PlanDestination(workingDirectory, target, false) + if err != nil { + t.Fatal(err) + } + if _, err := Publish(context.Background(), initialPlan, bundle); err != nil { + t.Fatal(err) + } + + recognitions := 0 + recognize := func(directory string) (Manifest, error) { + recognitions++ + return RecognizeBundle(directory) + } + plan, err := planDestination(workingDirectory, target, true, recognize) + if err != nil { + t.Fatal(err) + } + if _, err := publish(context.Background(), plan, bundle, publishOperations{ + rename: os.Rename, removeAll: os.RemoveAll, recognize: recognize, + }); err != nil { + t.Fatal(err) + } + if recognitions != 2 { + t.Fatalf("full bundle recognitions = %d, want early and final authorization", recognitions) + } +} + func TestPublishReauthorizesMovedDestination(t *testing.T) { tests := []struct { name string @@ -995,6 +1066,41 @@ func testBundle() LogicalBundle { } } +func testBundleWithReports(t *testing.T, count int) LogicalBundle { + t.Helper() + dataPackage := []byte("report: daily\n") + manifest := validManifest() + manifest.Total, manifest.Succeeded, manifest.Failed = count, count, 0 + manifest.Results = make([]Result, count) + reports := make([]BundleReport, count) + for i := range manifest.Results { + position := i + 1 + profileID := fmt.Sprintf("weather-%d", position) + path, err := ReportFilename(position, count, profileID) + if err != nil { + t.Fatal(err) + } + manifest.Results[i] = Result{ + Position: position, ProfileID: profileID, ModelName: "gpt-5-mini", + Status: StatusSucceeded, ValidationStatus: "passed", ReportPath: path, + } + reports[i] = BundleReport{Position: position, Path: path, Markdown: []byte("# Daily\n")} + } + manifest.DataPackage.SHA256 = SHA256(dataPackage) + return LogicalBundle{Manifest: manifest, DataPackage: dataPackage, Reports: reports} +} + +type countingReadCloser struct { + io.ReadCloser + count func(int) +} + +func (reader *countingReadCloser) Read(buffer []byte) (int, error) { + n, err := reader.ReadCloser.Read(buffer) + reader.count(n) + return n, err +} + func assertMode(t *testing.T, path string, want os.FileMode) { t.Helper() info, err := os.Stat(path)