diff --git a/internal/app/comparison.go b/internal/app/comparison.go index 0a87638..605e925 100644 --- a/internal/app/comparison.go +++ b/internal/app/comparison.go @@ -90,6 +90,11 @@ func CompareDetailed(ctx context.Context, req ComparisonRequest) (*ComparisonRes return nil, fmt.Errorf("build comparison identity: %w", err) } result := initialComparisonResult(req, resolved, comparisonID, now.UTC()) + defer func() { + if result.FinishedAt.IsZero() { + finalizeComparisonResult(result, clock) + } + }() outputName, err := resolved.OutputName() if err != nil { @@ -99,11 +104,11 @@ func CompareDetailed(ctx context.Context, req ComparisonRequest) (*ComparisonRes if err != nil { return result, err } - initialPlan, err := comparison.PlanDestination(req.WorkingDir, outputDirectory, req.Replace) + result.OutputDirectory = outputDirectory + _, err = comparison.PlanDestination(req.WorkingDir, outputDirectory, req.Replace) if err != nil { return result, fmt.Errorf("preflight comparison destination: %w", err) } - result.OutputDirectory = initialPlan.Target debugWriter, err := promptdebug.NewPromptDebugWriter(req.LLMDebugDir) if err != nil { @@ -129,7 +134,7 @@ func CompareDetailed(ctx context.Context, req ComparisonRequest) (*ComparisonRes executed := executeComparisonProfiles(ctx, comparisonExecutionRequest{ Prepared: prepared, Inspection: inspection, ComparisonID: comparisonID, DebugWriter: debugWriter, Executor: req.Executor, }) - result.FinishedAt = clock.Now().UTC() + finalizeComparisonResult(result, clock) copyComparisonOutcomes(result, executed.Outcomes, false) if executed.Canceled { return result, fmt.Errorf("comparison execution: %w", ctx.Err()) @@ -160,6 +165,17 @@ func CompareDetailed(ctx context.Context, req ComparisonRequest) (*ComparisonRes return result, nil } +func finalizeComparisonResult(result *ComparisonResult, clock timeutil.Clock) { + finishedAt := clock.Now().UTC() + if finishedAt.IsZero() { + finishedAt = time.Unix(0, 1).UTC() + } + if finishedAt.Before(result.StartedAt) { + finishedAt = result.StartedAt + } + result.FinishedAt = finishedAt +} + func initialComparisonResult(req ComparisonRequest, resolved report.Resolved, comparisonID string, startedAt time.Time) *ComparisonResult { metadata := resolved.Metadata() return &ComparisonResult{ diff --git a/internal/app/comparison_test.go b/internal/app/comparison_test.go index 67cbbe4..09e8ba5 100644 --- a/internal/app/comparison_test.go +++ b/internal/app/comparison_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" "testing" + "time" "gitea.maximumdirect.net/eric/weatherreporter/internal/comparison" "gitea.maximumdirect.net/eric/weatherreporter/internal/config" @@ -130,6 +131,78 @@ func TestCompareDetailedPreflightsBeforePromptOrCollection(t *testing.T) { } } +func TestCompareDetailedFinalizesUnpublishedFailures(t *testing.T) { + for _, test := range []struct { + name string + prepare func(t *testing.T, outputDirectory string) + debugDir string + executor *generationExecutor + collector *generationCollector + wantPrompt bool + }{ + { + name: "destination preflight", + prepare: func(t *testing.T, outputDirectory string) { + t.Helper() + if err := os.WriteFile(outputDirectory, []byte("not a directory"), 0o600); err != nil { + t.Fatal(err) + } + }, + executor: &generationExecutor{}, + }, + { + name: "debug initialization", + debugDir: "relative-debug-directory", + executor: &generationExecutor{}, + collector: &generationCollector{}, + }, + { + name: "prompt preflight", + executor: &generationExecutor{inspectErr: promptexec.NewError(promptexec.PromptLoad, "unsafe prompt detail", errors.New("unsafe cause"))}, + collector: &generationCollector{}, + wantPrompt: false, + }, + { + name: "collection", + executor: &generationExecutor{}, + collector: &generationCollector{err: errors.New("collection failed")}, + wantPrompt: true, + }, + { + name: "preparation", + executor: &generationExecutor{}, + collector: &generationCollector{bundle: &weatherdata.Bundle{}}, + wantPrompt: true, + }, + } { + t.Run(test.name, func(t *testing.T) { + workingDirectory := t.TempDir() + outputDirectory := filepath.Join(workingDirectory, "comparison-output") + if test.prepare != nil { + test.prepare(t, outputDirectory) + } + collector := test.collector + if collector == nil { + bundle := generationBundle(t) + collector = &generationCollector{bundle: &bundle} + } + result, err := CompareDetailed(context.Background(), ComparisonRequest{ + Config: comparisonConfig(), Report: ReportDaily, ProfileIDs: []string{"weather-light", "weather-deep"}, + WorkingDir: workingDirectory, OutputDir: outputDirectory, LLMDebugDir: test.debugDir, + Date: generationTime("2026-05-29T12:00:00-05:00"), Clock: timeutil.FixedClock{Time: generationTime("2026-05-29T08:30:00-05:00")}, + Collector: collector, Executor: test.executor, + }) + if err == nil { + t.Fatal("CompareDetailed() error = nil") + } + assertUnpublishedComparisonResult(t, result, outputDirectory) + if (result.PromptID != "") != test.wantPrompt || (result.PromptHash != "") != test.wantPrompt { + t.Fatalf("prompt identity = %q/%q, want resolved=%t", result.PromptID, result.PromptHash, test.wantPrompt) + } + }) + } +} + func TestCompareDetailedLeavesDestinationWhenCollectionOrPreparationFails(t *testing.T) { collectionErr := errors.New("weather collection failed") for _, test := range []struct { @@ -233,3 +306,15 @@ func comparisonConfig() config.Config { cfg.WeatherAPI.Timezone, cfg.Location.ID = "America/Chicago", "home" return cfg } + +func assertUnpublishedComparisonResult(t *testing.T, result *ComparisonResult, outputDirectory string) { + t.Helper() + if result == nil || result.OutputDirectory != outputDirectory || !filepath.IsAbs(result.OutputDirectory) || result.FinishedAt.IsZero() || result.FinishedAt.Location() != time.UTC || result.FinishedAt.Before(result.StartedAt) || result.ManifestPath != "" || result.DataPackagePath != "" { + t.Fatalf("unpublished comparison result = %#v", result) + } + for _, profile := range result.Results { + if profile.ReportPath != "" { + t.Fatalf("unpublished profile result = %#v", profile) + } + } +} diff --git a/internal/cli/comparison_test.go b/internal/cli/comparison_test.go index 72179d7..3e23fca 100644 --- a/internal/cli/comparison_test.go +++ b/internal/cli/comparison_test.go @@ -237,7 +237,7 @@ func TestCompareCommandReportsCommittedBundleWhenCleanupFails(t *testing.T) { 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 == "" { + if summary.Status != summaryStatusFailed || summary.Error == nil || summary.Error.Category != "publication_cleanup" || summary.Error.Message != "comparison published but cleanup 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} { diff --git a/internal/cli/result.go b/internal/cli/result.go index 2f2ee3f..aeaeec9 100644 --- a/internal/cli/result.go +++ b/internal/cli/result.go @@ -10,6 +10,7 @@ import ( "gitea.maximumdirect.net/eric/weatherreporter/internal/app" "gitea.maximumdirect.net/eric/weatherreporter/internal/comparison" + "gitea.maximumdirect.net/eric/weatherreporter/internal/promptexec" "gitea.maximumdirect.net/eric/weatherreporter/internal/report" "gitea.maximumdirect.net/eric/weatherreporter/internal/timeutil" "gitea.maximumdirect.net/eric/weatherreporter/internal/weatherdata" @@ -216,29 +217,50 @@ func comparisonSummaryStatus(result *app.ComparisonResult, err error) string { } func safeComparisonSummaryError(err error) *comparison.SafeError { - message := "comparison did not complete" - if aggregate, ok := comparisonAggregateErrorMessage(err.Error()); ok { - message = aggregate - } else if errors.Is(err, context.DeadlineExceeded) { - message = "comparison deadline exceeded" - } else if errors.Is(err, context.Canceled) { - message = "comparison canceled" + if aggregate, ok := comparisonAggregateErrorMessage(err); ok { + safe := comparison.NewSafeError("application", aggregate) + return &safe } - safe := comparison.NewSafeError("application", message) + var cleanupErr *comparison.PublicationCleanupError + if errors.As(err, &cleanupErr) { + safe := comparison.NewSafeError("publication_cleanup", "comparison published but cleanup did not complete") + return &safe + } + var destinationErr *comparison.DestinationError + if errors.As(err, &destinationErr) { + safe := comparison.NewSafeError("destination_"+string(destinationErr.Kind), "comparison destination preflight failed") + return &safe + } + if category := promptexec.CategoryOf(err); category != "" { + safe := comparison.NewSafeError(string(category), "comparison prompt operation failed") + return &safe + } + if errors.Is(err, context.DeadlineExceeded) { + safe := comparison.NewSafeError("deadline_exceeded", "comparison deadline exceeded") + return &safe + } + if errors.Is(err, context.Canceled) { + safe := comparison.NewSafeError("canceled", "comparison canceled") + return &safe + } + safe := comparison.NewSafeError("application", "comparison did not complete") return &safe } -func comparisonAggregateErrorMessage(value string) (string, bool) { +func comparisonAggregateErrorMessage(err error) (string, bool) { const prefix = "comparison completed with " const suffix = " failed profiles" - if !strings.HasPrefix(value, prefix) || !strings.HasSuffix(value, suffix) { - return "", false + for candidate := err; candidate != nil; candidate = errors.Unwrap(candidate) { + value := candidate.Error() + if !strings.HasPrefix(value, prefix) || !strings.HasSuffix(value, suffix) { + continue + } + count, parseErr := strconv.Atoi(strings.TrimSuffix(strings.TrimPrefix(value, prefix), suffix)) + if parseErr == nil && count > 0 { + return fmt.Sprintf("comparison completed with %d failed profiles", count), true + } } - count, err := strconv.Atoi(strings.TrimSuffix(strings.TrimPrefix(value, prefix), suffix)) - if err != nil || count < 1 { - return "", false - } - return fmt.Sprintf("comparison completed with %d failed profiles", count), true + return "", false } func batchSummaryStatus(result *app.BatchResult) string { diff --git a/internal/cli/result_test.go b/internal/cli/result_test.go index 15b0f96..c2b7928 100644 --- a/internal/cli/result_test.go +++ b/internal/cli/result_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "strings" "testing" "time" @@ -135,3 +136,76 @@ func TestComparisonSummaryClassifiesCompleteAndAllFailedResults(t *testing.T) { t.Fatalf("all-failed summary = %#v", summary) } } + +func TestSafeComparisonSummaryErrorClassifiesWrappedFailures(t *testing.T) { + unsafeDetail := "unsafe filesystem and provider detail" + unsafeCause := errors.New(unsafeDetail) + for _, test := range []struct { + name string + err error + category string + message string + }{ + { + name: "aggregate profile failure", + err: fmt.Errorf("outer wrapper: %w", errors.New("comparison completed with 2 failed profiles")), + category: "application", + message: "comparison completed with 2 failed profiles", + }, + { + name: "canceled", + err: fmt.Errorf("outer wrapper: %w", context.Canceled), + category: "canceled", + message: "comparison canceled", + }, + { + name: "deadline exceeded", + err: fmt.Errorf("outer wrapper: %w", context.DeadlineExceeded), + category: "deadline_exceeded", + message: "comparison deadline exceeded", + }, + { + name: "prompt operation", + err: fmt.Errorf("outer wrapper: %w", promptexec.NewError(promptexec.Generation, "unsafe prompt detail", fmt.Errorf("%w: %s", context.Canceled, unsafeDetail))), + category: string(promptexec.Generation), + message: "comparison prompt operation failed", + }, + { + name: "destination preflight", + err: fmt.Errorf("outer wrapper: %w", &comparison.DestinationError{ + Kind: comparison.DestinationNotEmpty, Target: "/tmp/" + unsafeDetail, Err: fmt.Errorf("%w: %s", context.Canceled, unsafeDetail), + }), + category: "destination_not_empty", + message: "comparison destination preflight failed", + }, + { + name: "publication cleanup", + err: fmt.Errorf("outer wrapper: %w", &comparison.PublicationCleanupError{ + RetainedBackupPath: "/tmp/" + unsafeDetail, Err: fmt.Errorf("%w: %s", context.Canceled, unsafeDetail), + }), + category: "publication_cleanup", + message: "comparison published but cleanup did not complete", + }, + { + name: "unknown", + err: fmt.Errorf("outer wrapper: %w", unsafeCause), + category: "application", + message: "comparison did not complete", + }, + } { + t.Run(test.name, func(t *testing.T) { + result := &app.ComparisonResult{ComparisonID: "comparison_test", ReportID: report.Daily} + summary := newComparisonSummary(result, test.err) + if summary.Error == nil || summary.Error.Category != test.category || summary.Error.Message != test.message { + t.Fatalf("summary error = %#v, want %q/%q", summary.Error, test.category, test.message) + } + data, err := json.Marshal(summary) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(data), unsafeDetail) { + t.Fatalf("summary includes unsafe detail: %s", data) + } + }) + } +}