From acb476a1425221d57322432a5a25dfaee80998fa Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 2 Aug 2026 13:23:18 +0000 Subject: [PATCH] Report committed comparison cleanup failures --- docs/cli.md | 5 ++ docs/internal/app-orchestration.md | 5 ++ docs/internal/comparison-publication.md | 10 +++- docs/operations.md | 7 +++ internal/app/comparison.go | 15 +++-- internal/app/comparison_test.go | 27 +++++++++ internal/cli/comparison_test.go | 35 ++++++++++++ internal/comparison/publish.go | 74 +++++++++++++++++-------- internal/comparison/publish_test.go | 71 +++++++++++++++++++----- 9 files changed, 206 insertions(+), 43 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index b126064..35ef091 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -142,6 +142,11 @@ top-level error; it does not expose provider diagnostics. See the [comparison bundle contract](integrations/comparison-bundle.md) for durable artifact fields and failure invariants. +If the bundle is published but cleanup of its replaced prior bundle fails, the +summary still includes the published artifact paths and has status `failed`. +Its JSON error remains generic; the returned command error identifies the +retained backup path for operator recovery. + ## Flag Reference | Flag | Accepted by | Meaning | diff --git a/docs/internal/app-orchestration.md b/docs/internal/app-orchestration.md index 2b51589..1182725 100644 --- a/docs/internal/app-orchestration.md +++ b/docs/internal/app-orchestration.md @@ -36,6 +36,11 @@ internals](prepared-report.md), [comparison execution internals](comparison-execution.md), and [comparison publication internals](comparison-publication.md). +When publication has committed its new bundle, application results contain the +absolute manifest, data-package, and successful report paths even if removal of +the previous sibling backup then fails. That cleanup failure is still returned +as an operational error rather than treating the new bundle as unpublished. + ## Boundaries And Verification The package does not parse flags, load YAML, implement transport, construct provider SDKs, or define report-period policy. Prompt, profile, weather, and Distributor implementations remain behind project-owned contracts. diff --git a/docs/internal/comparison-publication.md b/docs/internal/comparison-publication.md index 241c04b..544d5de 100644 --- a/docs/internal/comparison-publication.md +++ b/docs/internal/comparison-publication.md @@ -11,8 +11,14 @@ 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 plan, writes a private sibling staging directory, and installs it atomically. -Replacement temporarily moves the recognized prior bundle aside and restores it -if installation fails. +Replacement temporarily moves the recognized prior bundle aside, reauthorizes +that moved entry, and restores it if installation fails. + +Publication reports whether the new bundle committed. Once the new bundle is +installed, a failure to remove the prior sibling backup does not roll it back. +The result retains the absolute backup path and returns an inspectable cleanup +error that unwraps the filesystem cause so an operator can recover the prior +bundle if needed. The application preflights before prompt inspection and collection, then preflights again before publication. A cancellation or any failure before the diff --git a/docs/operations.md b/docs/operations.md index 9723e42..aef10d4 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -149,6 +149,13 @@ use the per-report statuses and retain successful output files. For a comparison failure, inspect the published manifest when its path is present: individual profile failures retain their safe result and successful Markdown files, while cancellation and pre-publication errors leave the prior destination unchanged. + +If a replacement commits but cleanup of its prior sibling backup fails, the new +bundle remains valid and its artifact paths appear in the failed command +summary. Preserve the retained backup path reported by the command error until +it has been inspected and cleaned up manually; do not remove the new bundle to +retry that cleanup. + Enable explicit debug capture only when content-rich Promptkit diagnostics are necessary. diff --git a/internal/app/comparison.go b/internal/app/comparison.go index a81e328..0a87638 100644 --- a/internal/app/comparison.go +++ b/internal/app/comparison.go @@ -65,6 +65,8 @@ type ComparisonProfileResult struct { Error *comparison.SafeError } +var publishComparison = comparison.Publish + // CompareDetailed assembles, executes, and atomically publishes a comparison // bundle. Profile failures publish a complete partial bundle; all other // failures leave the destination untouched. @@ -141,13 +143,16 @@ func CompareDetailed(ctx context.Context, req ComparisonRequest) (*ComparisonRes if err != nil { return result, fmt.Errorf("re-preflight comparison destination: %w", err) } - if err := comparison.Publish(ctx, publicationPlan, bundle); err != nil { + publication, err := publishComparison(ctx, publicationPlan, bundle) + if publication.Committed { + result.OutputDirectory = publicationPlan.Target + result.ManifestPath = filepath.Join(publicationPlan.Target, comparison.ManifestFilename) + result.DataPackagePath = filepath.Join(publicationPlan.Target, comparison.DataPackageFilename) + copyComparisonOutcomes(result, executed.Outcomes, true) + } + if err != nil { return result, fmt.Errorf("publish comparison bundle: %w", err) } - result.OutputDirectory = publicationPlan.Target - result.ManifestPath = filepath.Join(publicationPlan.Target, comparison.ManifestFilename) - result.DataPackagePath = filepath.Join(publicationPlan.Target, comparison.DataPackageFilename) - copyComparisonOutcomes(result, executed.Outcomes, true) if result.Failed > 0 { return result, fmt.Errorf("comparison completed with %d failed profiles", result.Failed) diff --git a/internal/app/comparison_test.go b/internal/app/comparison_test.go index 731ce20..67cbbe4 100644 --- a/internal/app/comparison_test.go +++ b/internal/app/comparison_test.go @@ -85,6 +85,33 @@ func TestCompareDetailedPublishesPartialBundleAndReturnsAggregateError(t *testin } } +func TestCompareDetailedRetainsCommittedPathsWhenBackupCleanupFails(t *testing.T) { + bundle := generationBundle(t) + backupPath := filepath.Join(t.TempDir(), ".comparison-daily.backup-retained") + cleanupCause := errors.New("backup cleanup failed") + originalPublish := publishComparison + publishComparison = func(context.Context, comparison.DestinationPlan, comparison.LogicalBundle) (comparison.PublicationResult, error) { + return comparison.PublicationResult{Committed: true, RetainedBackupPath: backupPath}, &comparison.PublicationCleanupError{RetainedBackupPath: backupPath, Err: cleanupCause} + } + t.Cleanup(func() { publishComparison = originalPublish }) + + result, err := CompareDetailed(context.Background(), ComparisonRequest{ + Config: comparisonConfig(), Report: ReportDaily, ProfileIDs: []string{"weather-light", "weather-deep"}, + WorkingDir: t.TempDir(), Date: generationTime("2026-05-29T12:00:00-05:00"), + Clock: timeutil.FixedClock{Time: generationTime("2026-05-29T08:30:00-05:00")}, + Collector: &generationCollector{bundle: &bundle}, Executor: &generationExecutor{}, + }) + var cleanupErr *comparison.PublicationCleanupError + if result == nil || !errors.As(err, &cleanupErr) || !errors.Is(err, cleanupCause) || cleanupErr.RetainedBackupPath != backupPath || !filepath.IsAbs(result.ManifestPath) || !filepath.IsAbs(result.DataPackagePath) { + t.Fatalf("CompareDetailed() result/error = %#v/%v", result, err) + } + for _, profile := range result.Results { + if profile.Status == comparison.StatusSucceeded && !filepath.IsAbs(profile.ReportPath) { + t.Fatalf("published profile result = %#v", profile) + } + } +} + func TestCompareDetailedPreflightsBeforePromptOrCollection(t *testing.T) { invalidDestination := filepath.Join(t.TempDir(), "not-a-directory") if err := os.WriteFile(invalidDestination, []byte("x"), 0o600); err != nil { diff --git a/internal/cli/comparison_test.go b/internal/cli/comparison_test.go index 8c3c3d8..72179d7 100644 --- a/internal/cli/comparison_test.go +++ b/internal/cli/comparison_test.go @@ -212,6 +212,41 @@ func TestCompareCommandWritesStructuredPartialFailure(t *testing.T) { } } +func TestCompareCommandReportsCommittedBundleWhenCleanupFails(t *testing.T) { + workingDir := t.TempDir() + configPath := comparisonConfigPath(t, "weather_api:\n base_url: https://weather.api.example.com/\n") + outputDirectory := filepath.Join(workingDir, "comparison-daily") + result := comparisonResult(outputDirectory, []app.ComparisonProfileResult{ + {Position: 1, ProfileID: "weather-light", ModelName: "light", Status: comparison.StatusSucceeded, ValidationStatus: promptexec.ValidationPassed, ReportPath: filepath.Join(outputDirectory, "01-weather-light.md")}, + {Position: 2, ProfileID: "weather-deep", ModelName: "deep", Status: comparison.StatusSucceeded, ValidationStatus: promptexec.ValidationPassed, ReportPath: filepath.Join(outputDirectory, "02-weather-deep.md")}, + }) + backupPath := filepath.Join(workingDir, ".comparison-daily.backup-retained") + cleanupCause := errors.New("filesystem cleanup detail") + cleanupErr := &comparison.PublicationCleanupError{RetainedBackupPath: backupPath, Err: cleanupCause} + runner := comparisonRunner(t, workingDir) + runner.compareDetailed = func(context.Context, app.ComparisonRequest) (*app.ComparisonResult, error) { + return result, cleanupErr + } + + var stdout, stderr bytes.Buffer + err := runner.Run(context.Background(), []string{"compare", "daily", "--date", "2026-05-29", "--profile", "weather-light", "--profile", "weather-deep", "--config", configPath}, &stdout, &stderr) + if !errors.Is(err, cleanupCause) || stderr.Len() != 0 { + t.Fatalf("Run() error/stderr = %v/%q", err, stderr.String()) + } + var summary comparisonSummary + if err := json.Unmarshal(stdout.Bytes(), &summary); err != nil { + t.Fatalf("decode summary: %v\n%s", err, stdout.String()) + } + if summary.Status != summaryStatusFailed || summary.Error == nil || summary.Error.Message != "comparison did not complete" || summary.ManifestPath != result.ManifestPath || summary.DataPackagePath != result.DataPackagePath || summary.Results[0].ReportPath == "" { + t.Fatalf("summary = %#v", summary) + } + for _, unsafe := range []string{cleanupCause.Error(), backupPath} { + if strings.Contains(stdout.String(), unsafe) { + t.Fatalf("summary contains unsafe recovery detail %q: %s", unsafe, stdout.String()) + } + } +} + func TestCompareCommandWritesSuccessForDefaultAndExplicitDestinations(t *testing.T) { workingDir := t.TempDir() configPath := comparisonConfigPath(t, "weather_api:\n base_url: https://weather.api.example.com/\noutput:\n directory: configured-reports\n") diff --git a/internal/comparison/publish.go b/internal/comparison/publish.go index 694b5c4..14e4e54 100644 --- a/internal/comparison/publish.go +++ b/internal/comparison/publish.go @@ -262,43 +262,72 @@ func unrecognizedBundleError(action string, err error) error { return fmt.Errorf("%w: %s: %v", ErrUnrecognizedBundle, action, err) } +// PublicationResult describes the durable state of a publication attempt. +type PublicationResult struct { + Committed bool + RetainedBackupPath string +} + +// PublicationCleanupError reports that a committed bundle could not remove its +// prior sibling backup. The new bundle remains installed and the backup path +// is retained for operator recovery. +type PublicationCleanupError struct { + RetainedBackupPath string + Err error +} + +func (err *PublicationCleanupError) Error() string { + return fmt.Sprintf("remove comparison backup %q: %v", err.RetainedBackupPath, err.Err) +} + +func (err *PublicationCleanupError) Unwrap() error { + return err.Err +} + // Publish writes a complete logical bundle through a private sibling directory // and atomically installs it at a preflighted destination. -func Publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle) error { - return publish(ctx, plan, bundle, publishOperations{rename: os.Rename}) +func Publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle) (PublicationResult, error) { + return publish(ctx, plan, bundle, publishOperations{rename: os.Rename, removeAll: os.RemoveAll}) } type publishOperations struct { rename func(string, string) error + removeAll func(string) error beforeCommit func() } -func publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle, operations publishOperations) error { +func publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle, operations publishOperations) (PublicationResult, error) { + if operations.rename == nil { + operations.rename = os.Rename + } + if operations.removeAll == nil { + operations.removeAll = os.RemoveAll + } if err := bundle.Validate(); err != nil { - return fmt.Errorf("validate comparison bundle: %w", err) + return PublicationResult{}, fmt.Errorf("validate comparison bundle: %w", err) } manifestData, err := EncodeManifest(bundle.Manifest) if err != nil { - return err + return PublicationResult{}, err } if err := ctx.Err(); err != nil { - return err + return PublicationResult{}, err } plan, err = PlanDestination(plan.WorkingDirectory, plan.Target, plan.Replace) if err != nil { - return err + return PublicationResult{}, err } if err := os.MkdirAll(filepath.Dir(plan.Target), 0o755); err != nil { - return fmt.Errorf("create comparison destination parent %q: %w", filepath.Dir(plan.Target), err) + return PublicationResult{}, fmt.Errorf("create comparison destination parent %q: %w", filepath.Dir(plan.Target), err) } temporaryDirectory, err := os.MkdirTemp(filepath.Dir(plan.Target), "."+filepath.Base(plan.Target)+".staging-") if err != nil { - return fmt.Errorf("create comparison staging directory: %w", err) + return PublicationResult{}, fmt.Errorf("create comparison staging directory: %w", err) } if err := os.Chmod(temporaryDirectory, 0o700); err != nil { os.RemoveAll(temporaryDirectory) - return fmt.Errorf("secure comparison staging directory: %w", err) + return PublicationResult{}, fmt.Errorf("secure comparison staging directory: %w", err) } defer func() { if temporaryDirectory != "" { @@ -307,45 +336,46 @@ func publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle, op }() if err := writeLogicalBundle(ctx, temporaryDirectory, bundle, manifestData); err != nil { - return err + return PublicationResult{}, err } currentPlan, err := PlanDestination(plan.WorkingDirectory, plan.Target, plan.Replace) if err != nil { - return err + return PublicationResult{}, err } if operations.beforeCommit != nil { operations.beforeCommit() } if err := ctx.Err(); err != nil { - return err + return PublicationResult{}, err } if currentPlan.state == destinationAbsent { if err := operations.rename(temporaryDirectory, currentPlan.Target); err != nil { - return fmt.Errorf("publish comparison bundle to %q: %w", currentPlan.Target, err) + return PublicationResult{}, fmt.Errorf("publish comparison bundle to %q: %w", currentPlan.Target, err) } temporaryDirectory = "" - return nil + return PublicationResult{Committed: true}, nil } backupDirectory, err := uniqueSiblingPath(filepath.Dir(currentPlan.Target), "."+filepath.Base(currentPlan.Target)+".backup-") if err != nil { - return err + return PublicationResult{}, err } if err := operations.rename(currentPlan.Target, backupDirectory); err != nil { - return fmt.Errorf("back up comparison destination %q: %w", currentPlan.Target, err) + return PublicationResult{}, fmt.Errorf("back up comparison destination %q: %w", currentPlan.Target, err) } if err := authorizeMovedDestination(currentPlan, backupDirectory); err != nil { - return restoreMovedDestination(operations, backupDirectory, currentPlan.Target, err) + return PublicationResult{}, restoreMovedDestination(operations, backupDirectory, currentPlan.Target, err) } if err := operations.rename(temporaryDirectory, currentPlan.Target); err != nil { - return restoreMovedDestination(operations, backupDirectory, currentPlan.Target, fmt.Errorf("replace comparison destination %q: %w", currentPlan.Target, err)) + return PublicationResult{}, restoreMovedDestination(operations, backupDirectory, currentPlan.Target, fmt.Errorf("replace comparison destination %q: %w", currentPlan.Target, err)) } temporaryDirectory = "" - if err := os.RemoveAll(backupDirectory); err != nil { - return fmt.Errorf("remove comparison backup %q: %w", backupDirectory, err) + if err := operations.removeAll(backupDirectory); err != nil { + cleanupErr := &PublicationCleanupError{RetainedBackupPath: backupDirectory, Err: err} + return PublicationResult{Committed: true, RetainedBackupPath: backupDirectory}, cleanupErr } - return nil + return PublicationResult{Committed: true}, nil } func authorizeMovedDestination(plan DestinationPlan, backupDirectory string) error { diff --git a/internal/comparison/publish_test.go b/internal/comparison/publish_test.go index 60d31fe..5394f37 100644 --- a/internal/comparison/publish_test.go +++ b/internal/comparison/publish_test.go @@ -275,7 +275,7 @@ func TestPublishReauthorizesMovedDestination(t *testing.T) { if err != nil { t.Fatal(err) } - if err := Publish(context.Background(), plan, testBundle()); err != nil { + if _, err := Publish(context.Background(), plan, testBundle()); err != nil { t.Fatal(err) } }, @@ -303,7 +303,7 @@ func TestPublishReauthorizesMovedDestination(t *testing.T) { if err != nil { t.Fatal(err) } - err = publish(context.Background(), plan, testBundle(), publishOperations{ + _, err = publish(context.Background(), plan, testBundle(), publishOperations{ rename: os.Rename, beforeCommit: func() { if err := os.RemoveAll(target); err != nil { @@ -333,7 +333,7 @@ func TestPublishRetainsUnauthorizedMovedDestinationWhenRestoreFails(t *testing.T } var backupPath string calls := 0 - err = publish(context.Background(), plan, testBundle(), publishOperations{ + _, err = publish(context.Background(), plan, testBundle(), publishOperations{ rename: func(oldPath, newPath string) error { calls++ if calls == 1 { @@ -376,7 +376,7 @@ func TestPublishRetainsMovedDestinationWhenTargetReappears(t *testing.T) { t.Fatal(err) } var backupPath string - err = publish(context.Background(), plan, testBundle(), publishOperations{ + _, err = publish(context.Background(), plan, testBundle(), publishOperations{ rename: func(oldPath, newPath string) error { if backupPath != "" { return os.Rename(oldPath, newPath) @@ -413,7 +413,7 @@ func TestPublishWritesAndReplacesBundle(t *testing.T) { if err != nil { t.Fatal(err) } - if err := Publish(context.Background(), plan, testBundle()); err != nil { + if _, err := Publish(context.Background(), plan, testBundle()); err != nil { t.Fatalf("Publish() error = %v", err) } if _, err := RecognizeBundle(target); err != nil { @@ -433,7 +433,7 @@ func TestPublishWritesAndReplacesBundle(t *testing.T) { if err != nil { t.Fatal(err) } - if err := Publish(context.Background(), plan, next); err != nil { + if _, err := Publish(context.Background(), plan, next); err != nil { t.Fatalf("Publish(replace) error = %v", err) } if _, err := os.Stat(filepath.Join(target, "01-weather-light.md")); !errors.Is(err, os.ErrNotExist) { @@ -454,7 +454,7 @@ func TestPublishReplacesEmptyDirectory(t *testing.T) { if err != nil { t.Fatal(err) } - if err := Publish(context.Background(), plan, testBundle()); err != nil { + if _, err := Publish(context.Background(), plan, testBundle()); err != nil { t.Fatalf("Publish() error = %v", err) } if _, err := RecognizeBundle(target); err != nil { @@ -462,6 +462,49 @@ func TestPublishReplacesEmptyDirectory(t *testing.T) { } } +func TestPublishReportsCommittedBundleWhenBackupCleanupFails(t *testing.T) { + workingDirectory := t.TempDir() + target := filepath.Join(workingDirectory, "comparison-daily") + initialPlan, err := PlanDestination(workingDirectory, target, false) + if err != nil { + t.Fatal(err) + } + if _, err := Publish(context.Background(), initialPlan, testBundle()); err != nil { + t.Fatal(err) + } + previous := directorySnapshot(t, target) + + next := testBundle() + next.Reports[0].Markdown = []byte("# Next\n") + plan, err := PlanDestination(workingDirectory, target, true) + if err != nil { + t.Fatal(err) + } + cleanupCause := errors.New("backup removal failed") + var backupPath string + result, err := publish(context.Background(), plan, next, publishOperations{ + rename: os.Rename, + removeAll: func(path string) error { + backupPath = path + return cleanupCause + }, + }) + var cleanupErr *PublicationCleanupError + if !result.Committed || result.RetainedBackupPath != backupPath || !filepath.IsAbs(backupPath) || !errors.As(err, &cleanupErr) || cleanupErr.RetainedBackupPath != backupPath || !errors.Is(err, cleanupCause) { + t.Fatalf("publish() result/error = %#v/%v", result, err) + } + if _, err := RecognizeBundle(target); err != nil { + t.Fatalf("new bundle recognition error = %v", err) + } + if retained := directorySnapshot(t, backupPath); !equalSnapshots(previous, retained) { + t.Fatalf("retained backup = %#v, want %#v", retained, previous) + } + manifestData, err := os.ReadFile(filepath.Join(target, ManifestFilename)) + if err != nil || strings.Contains(string(manifestData), backupPath) { + t.Fatalf("manifest/backup path = %q/%q, error = %v", manifestData, backupPath, err) + } +} + func TestPublishRestoresExistingBundleAfterReplacementFailure(t *testing.T) { workingDirectory := t.TempDir() target := filepath.Join(workingDirectory, "comparison-daily") @@ -470,7 +513,7 @@ func TestPublishRestoresExistingBundleAfterReplacementFailure(t *testing.T) { if err != nil { t.Fatal(err) } - if err := Publish(context.Background(), initialPlan, initial); err != nil { + if _, err := Publish(context.Background(), initialPlan, initial); err != nil { t.Fatal(err) } before := directorySnapshot(t, target) @@ -482,14 +525,14 @@ func TestPublishRestoresExistingBundleAfterReplacementFailure(t *testing.T) { t.Fatal(err) } calls := 0 - err = publish(context.Background(), plan, next, publishOperations{rename: func(oldPath, newPath string) error { + publication, err := publish(context.Background(), plan, next, publishOperations{rename: func(oldPath, newPath string) error { calls++ if calls == 2 { return errors.New("replace failed") } return os.Rename(oldPath, newPath) }}) - if err == nil { + if publication.Committed || err == nil { t.Fatal("publish() succeeded despite replacement failure") } after := directorySnapshot(t, target) @@ -512,7 +555,7 @@ func TestPublishRetainsBackupWhenRestorationFails(t *testing.T) { if err != nil { t.Fatal(err) } - if err := Publish(context.Background(), initialPlan, testBundle()); err != nil { + if _, err := Publish(context.Background(), initialPlan, testBundle()); err != nil { t.Fatal(err) } plan, err := PlanDestination(workingDirectory, target, true) @@ -521,7 +564,7 @@ func TestPublishRetainsBackupWhenRestorationFails(t *testing.T) { } var backupPath string calls := 0 - err = publish(context.Background(), plan, testBundle(), publishOperations{rename: func(oldPath, newPath string) error { + _, err = publish(context.Background(), plan, testBundle(), publishOperations{rename: func(oldPath, newPath string) error { calls++ if calls == 1 { backupPath = newPath @@ -547,7 +590,7 @@ func TestPublishCancellationBeforeCommitLeavesNoDestination(t *testing.T) { t.Fatal(err) } ctx, cancel := context.WithCancel(context.Background()) - if err := publish(ctx, plan, testBundle(), publishOperations{rename: os.Rename, beforeCommit: cancel}); !errors.Is(err, context.Canceled) { + if publication, err := publish(ctx, plan, testBundle(), publishOperations{rename: os.Rename, beforeCommit: cancel}); publication.Committed || !errors.Is(err, context.Canceled) { t.Fatalf("Publish() error = %v, want context cancellation", err) } if _, err := os.Lstat(target); !errors.Is(err, os.ErrNotExist) { @@ -572,7 +615,7 @@ func publishTestBundle(t *testing.T, bundle LogicalBundle) string { if err != nil { t.Fatal(err) } - if err := Publish(context.Background(), plan, bundle); err != nil { + if _, err := Publish(context.Background(), plan, bundle); err != nil { t.Fatal(err) } return target