From 0bafbcb21fae61b89f76eddd0bc4fc4e5d7b9f79 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 25 Aug 2026 23:46:21 +0000 Subject: [PATCH] Harden PromptKit upgrade integration --- docs/cli.md | 14 +++- docs/integrations/comparison-bundle.md | 7 +- docs/roadmap/promptkit-v0.8.0.md | 2 +- internal/adapters/promptkit/adapter_test.go | 50 ++++++++++++ internal/app/comparison_execution.go | 9 ++- internal/app/comparison_execution_test.go | 84 ++++++++++++++++++++- internal/app/comparison_test.go | 34 +++++++++ internal/app/generation_test.go | 61 +++++++++++++++ internal/comparison/comparison.go | 2 +- internal/comparison/comparison_test.go | 14 ++++ internal/comparison/publish.go | 9 ++- internal/comparison/publish_test.go | 42 +++++++++++ 12 files changed, 314 insertions(+), 14 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index 6045196..6516fc7 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -106,7 +106,10 @@ period, prompt version, timezone, and status. Successful output has an absolute When available, the summary also includes the effective `profileId`, `backendId`, `modelName`, `sourceWarnings`, `validationStatus`, requested -`llmDebugPath`, and compact Distributor `notification` result. It does not +`repairAttempts`, `llmDebugPath`, and compact Distributor `notification` +result. `repairAttempts` is `0` when the initial output passed validation, +positive when PromptKit made corrective generation calls, and omitted when +validation did not complete. The summary does not include historical or transient artifact paths such as metadata, prompt input, raw generated text, render context, or notification receipts. @@ -115,7 +118,8 @@ raw generated text, render context, or notification receipts. A run summary contains `command`, `batch`, `status`, `startedAt`, `finishedAt`, `total`, `succeeded`, `failed`, and a `reports` array. Each report item includes its identity, status, effective profile and model details when available, -source warnings, validation status, and absolute `outputPath` after publication. +source warnings, validation status, repair-attempt count when validation +completed, and absolute `outputPath` after publication. The top-level summary may also contain a batch `notification` object and `error`. Batch status is `failed` if any report or the batch notification fails. The `total`, `succeeded`, and `failed` counters describe report items only, so @@ -143,7 +147,8 @@ A comparison summary contains these fields in this order: `command`, successful `results[].reportPath` are absolute. `results` preserves the supplied profile order and each item contains `position`, `profileId`, optional `backendId`, `modelName`, `status`, optional `validationStatus`, optional -`reportPath`, optional `llmDebugPath`, and optional safe `error`. +`repairAttempts`, optional `reportPath`, optional `llmDebugPath`, and optional +safe `error`. The repair-attempt semantics match the generate summary. The comparison status is `succeeded` only when every selected profile succeeds and the bundle is published. Individual profile failures still publish a @@ -157,7 +162,8 @@ published Promptkit category; destination failures use `destination_`; and committed cleanup failures use `publication_cleanup` with a message that states whether a complete prior bundle, partial remnants, or no prior bundle remains, or that recovery state could not be inspected. It does not expose -provider diagnostics, filesystem causes, or recovery paths. See the +provider diagnostics, filesystem causes, or recovery paths. A provider HTTP +failure may include its numeric status in the safe message. See the [comparison bundle contract](integrations/comparison-bundle.md) for durable artifact fields and failure invariants. diff --git a/docs/integrations/comparison-bundle.md b/docs/integrations/comparison-bundle.md index 0f5ed29..98285c7 100644 --- a/docs/integrations/comparison-bundle.md +++ b/docs/integrations/comparison-bundle.md @@ -71,10 +71,11 @@ a `reportPath` exactly equal to the canonical `NN-profile-slug.md` filename for its position, total, and logical profile ID, and no `error`. A failed result has `status: "failed"`, no `reportPath`, and an `error` object with nonblank `category` and `message`. Its validation status is absent, `failed`, or -`skipped`. Error messages are valid UTF-8 and no longer than 1,024 bytes. +`skipped`; it may also be `passed` when a WeatherReporter step after PromptKit +validation failed. Error messages are valid UTF-8 and no longer than 1,024 bytes. `backendId` and `validationStatus` are omitted when unavailable. A failed -result with a completed validation (`failed` or `skipped`) must retain its -non-negative `repairAttempts`; early operational failures omit both fields. +result with any completed validation status must retain its non-negative +`repairAttempts`; early operational failures omit both fields. Every successful Markdown file is declared by exactly one successful result. The directory contains no extra entries. Consumers can therefore verify the diff --git a/docs/roadmap/promptkit-v0.8.0.md b/docs/roadmap/promptkit-v0.8.0.md index 32de678..a13178c 100644 --- a/docs/roadmap/promptkit-v0.8.0.md +++ b/docs/roadmap/promptkit-v0.8.0.md @@ -1,6 +1,6 @@ # PromptKit v0.8.0 Upgrade Roadmap -Status: Accepted feature direction; implementation has not started. +Status: Implemented. ## Purpose diff --git a/internal/adapters/promptkit/adapter_test.go b/internal/adapters/promptkit/adapter_test.go index 4d5a860..b583a92 100644 --- a/internal/adapters/promptkit/adapter_test.go +++ b/internal/adapters/promptkit/adapter_test.go @@ -239,6 +239,56 @@ func TestMaintainedWeatherLightLocalProfileExampleInspectsOffline(t *testing.T) assertProfile(t, adapter, "weather-light", "", "weather-local") } +func TestMaintainedWeatherLightLocalProfileExampleExecutesThroughProductionClient(t *testing.T) { + t.Setenv("WEATHERREPORTER_TEST_MISSING_KEY", "") + for _, test := range []struct { + name string + credentialSource string + }{ + {name: "without credential source"}, + {name: "with blank optional credential source", credentialSource: "\napi_key_env: WEATHERREPORTER_TEST_MISSING_KEY\n"}, + } { + t.Run(test.name, func(t *testing.T) { + var authorization string + server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { + authorization = request.Header.Get("Authorization") + writer.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprintf(writer, `{"choices":[{"message":{"content":%q}}],"usage":{"prompt_tokens":12,"completion_tokens":8,"total_tokens":20}}`, validResponse().Content) + })) + defer server.Close() + + example, err := os.ReadFile(filepath.Join("..", "..", "..", "examples", "weather-light-local-profile.yml")) + if err != nil { + t.Fatal(err) + } + profile := strings.Replace(string(example), "http://127.0.0.1:11434/v1", server.URL+"/v1", 1) + test.credentialSource + adapter, err := New(Config{ProfileFile: writeProfileFile(t, profile)}) + if err != nil { + t.Fatalf("New() error = %v", err) + } + request := testExecuteRequest() + request.ProfileID = "weather-light" + var preparation promptexec.Preparation + result, err := adapter.Execute(context.Background(), request, func(value promptexec.Preparation, _ *promptexec.PreparationDebug) error { + preparation = value + return nil + }) + if err != nil { + t.Fatalf("Execute() error = %v", err) + } + if authorization != "" { + t.Fatalf("Authorization header = %q, want absent", authorization) + } + if preparation.ProfileID != "weather-light" || preparation.BackendID != "" || preparation.ModelName != "weather-local" || preparation.Output.RepairAttempts != 1 { + t.Fatalf("preparation = %#v", preparation) + } + if result == nil || result.ProfileID != "weather-light" || result.BackendID != "" || result.ModelName != "weather-local" || result.Validation.Status != promptexec.ValidationPassed || result.Validation.RepairAttempts != 0 { + t.Fatalf("execution = %#v", result) + } + }) + } +} + func TestProfileResolutionFallsThroughOnlyWhenTheConfiguredIDIsAbsent(t *testing.T) { absentAdapter, err := New(Config{ProfileDirectory: testProfileDirectory(t, map[string]string{"profile.yml": `id: other-profile backend: openrouter diff --git a/internal/app/comparison_execution.go b/internal/app/comparison_execution.go index e0d92c7..5fb3954 100644 --- a/internal/app/comparison_execution.go +++ b/internal/app/comparison_execution.go @@ -173,9 +173,14 @@ func comparisonExecutionMessage(err error) string { if errors.Is(err, context.DeadlineExceeded) { return "profile execution deadline exceeded" } + operation := "profile execution" var execution *profileExecutionError if errors.As(err, &execution) { - return comparison.TruncateErrorMessage(execution.operation + " failed") + operation = execution.operation } - return "profile execution failed" + var generation *promptexec.GenerationError + if errors.As(err, &generation) && generation.StatusCode() > 0 { + return comparison.TruncateErrorMessage(fmt.Sprintf("%s failed (HTTP %d)", operation, generation.StatusCode())) + } + return comparison.TruncateErrorMessage(operation + " failed") } diff --git a/internal/app/comparison_execution_test.go b/internal/app/comparison_execution_test.go index 05b71e8..45e2918 100644 --- a/internal/app/comparison_execution_test.go +++ b/internal/app/comparison_execution_test.go @@ -4,9 +4,11 @@ import ( "context" "errors" "fmt" + "net/http" "os" "path/filepath" "reflect" + "strings" "sync" "testing" "time" @@ -72,6 +74,66 @@ func TestExecuteComparisonProfilesContinuesAfterProfileFailure(t *testing.T) { } } +func TestExecuteComparisonProfilesPreservesIndependentRepairOutcomes(t *testing.T) { + prepared, prompt := preparedDailyProfile(t) + profiles := comparisonProfiles(4) + executor := newBarrierExecutor(profiles) + executor.setValidation(profiles[0].ProfileID, promptexec.ValidationPassed, 0) + executor.setValidation(profiles[1].ProfileID, promptexec.ValidationPassed, 1) + executor.setValidation(profiles[2].ProfileID, promptexec.ValidationFailed, 1) + executor.setError(profiles[3].ProfileID, errors.New("provider failure")) + results := startComparisonExecution(t, context.Background(), comparisonExecutionRequest{ + Prepared: prepared, Inspection: comparisonInspection(prompt, profiles), ComparisonID: "comparison_daily", Executor: executor, + }, executor) + waitForProfileStarts(t, executor, profiles, results) + executor.releaseAll() + result := <-results + wantStatuses := []string{comparison.StatusSucceeded, comparison.StatusSucceeded, comparison.StatusFailed, comparison.StatusFailed} + wantValidations := []promptexec.ValidationStatus{promptexec.ValidationPassed, promptexec.ValidationPassed, promptexec.ValidationFailed, ""} + wantRepairs := []*int{intPointer(0), intPointer(1), intPointer(1), nil} + for index, outcome := range result.Outcomes { + if outcome.Status != wantStatuses[index] || outcome.ValidationStatus != wantValidations[index] || !reflect.DeepEqual(outcome.RepairAttempts, wantRepairs[index]) { + t.Fatalf("outcome[%d] = %#v, want status/validation/repairs %q/%q/%#v", index, outcome, wantStatuses[index], wantValidations[index], wantRepairs[index]) + } + } +} + +func TestExecuteComparisonProfilesCapturesConcurrentProviderFailures(t *testing.T) { + prepared, prompt := preparedDailyProfile(t) + profiles := comparisonProfiles(2) + debugWriter, err := promptdebug.NewPromptDebugWriter(t.TempDir()) + if errors.Is(err, promptdebug.ErrSecureCaptureUnsupported) { + t.Skipf("secure prompt debug capture is unavailable: %v", err) + } + if err != nil { + t.Fatalf("NewPromptDebugWriter() error = %v", err) + } + markers := []string{"first-provider-private-marker", "second-provider-private-marker"} + statuses := []int{http.StatusTooManyRequests, http.StatusServiceUnavailable} + executor := newBarrierExecutor(profiles) + for index, profile := range profiles { + executor.setError(profile.ProfileID, promptexec.NewGenerationError(statuses[index], "provider_code", "provider_type", markers[index], nil)) + } + results := startComparisonExecution(t, context.Background(), comparisonExecutionRequest{ + Prepared: prepared, Inspection: comparisonInspection(prompt, profiles), ComparisonID: "comparison_daily", DebugWriter: debugWriter, Executor: executor, + }, executor) + waitForProfileStarts(t, executor, profiles, results) + executor.releaseAll() + result := <-results + for index, outcome := range result.Outcomes { + if outcome.Status != comparison.StatusFailed || outcome.Error == nil || outcome.Error.Category != string(promptexec.Generation) || outcome.Error.Message != fmt.Sprintf("execute prompt failed (HTTP %d)", statuses[index]) || strings.Contains(outcome.Error.Message, markers[index]) || outcome.LLMDebugPath == "" { + t.Fatalf("outcome[%d] = %#v", index, outcome) + } + failure, readErr := os.ReadFile(filepath.Join(outcome.LLMDebugPath, "failure.json")) + if readErr != nil { + t.Fatal(readErr) + } + if !strings.Contains(string(failure), markers[index]) || strings.Contains(string(failure), markers[1-index]) { + t.Fatalf("failure[%d] = %s", index, failure) + } + } +} + func TestExecuteComparisonProfilesPropagatesCancellationAndJoins(t *testing.T) { prepared, prompt := preparedDailyProfile(t) profiles := comparisonProfiles(4) @@ -139,6 +201,8 @@ type barrierExecutor struct { releases map[string]chan struct{} requests map[string]promptexec.ExecuteRequest errors map[string]error + validations map[string]promptexec.ValidationStatus + repairAttempts map[string]int profiles map[string]ComparisonProfileInspection inFlight int maximum int @@ -153,7 +217,7 @@ func newBarrierExecutor(profiles []ComparisonProfileInspection) *barrierExecutor } return &barrierExecutor{ started: make(chan string, len(profiles)), callbackFailures: make(chan error, len(profiles)), releases: releases, - requests: make(map[string]promptexec.ExecuteRequest, len(profiles)), errors: map[string]error{}, profiles: identities, + requests: make(map[string]promptexec.ExecuteRequest, len(profiles)), errors: map[string]error{}, validations: map[string]promptexec.ValidationStatus{}, repairAttempts: map[string]int{}, profiles: identities, } } @@ -195,15 +259,20 @@ func (e *barrierExecutor) Execute(ctx context.Context, req promptexec.ExecuteReq e.mu.Lock() e.inFlight-- err := e.errors[req.ProfileID] + validationStatus := e.validations[req.ProfileID] + repairAttempts := e.repairAttempts[req.ProfileID] e.mu.Unlock() if err != nil { return nil, err } + if validationStatus == "" { + validationStatus = promptexec.ValidationPassed + } return &promptexec.Execution{ PromptID: req.PromptID, PromptVersion: req.PromptVersion, PromptHash: generationPromptHash, ProfileID: req.ProfileID, BackendID: profile.BackendID, ModelName: profile.ModelName, StartedAt: stamp, EndedAt: stamp, RawOutput: comparisonRawOutput(), - Validation: promptexec.NewValidation(promptexec.ValidationPassed, "json_schema", generationDefinitionForPrompt(req.PromptID).GeneratedTextSchemaID+".generated_text.schema.json", 0, nil), + Validation: promptexec.NewValidation(validationStatus, "json_schema", generationDefinitionForPrompt(req.PromptID).GeneratedTextSchemaID+".generated_text.schema.json", repairAttempts, nil), }, nil } @@ -220,6 +289,13 @@ func (e *barrierExecutor) setError(profileID string, err error) { e.errors[profileID] = err } +func (e *barrierExecutor) setValidation(profileID string, status promptexec.ValidationStatus, repairAttempts int) { + e.mu.Lock() + defer e.mu.Unlock() + e.validations[profileID] = status + e.repairAttempts[profileID] = repairAttempts +} + func (e *barrierExecutor) release(profileID string) { close(e.releases[profileID]) } @@ -318,4 +394,8 @@ func bytesEqual(left, right []byte) bool { return reflect.DeepEqual(left, right) } +func intPointer(value int) *int { + return &value +} + var _ promptexec.Executor = (*barrierExecutor)(nil) diff --git a/internal/app/comparison_test.go b/internal/app/comparison_test.go index 3116b42..59ffd84 100644 --- a/internal/app/comparison_test.go +++ b/internal/app/comparison_test.go @@ -87,6 +87,40 @@ func TestCompareDetailedPublishesPartialBundleAndReturnsAggregateError(t *testin } } +func TestCompareDetailedPublishesPostValidationProfileFailure(t *testing.T) { + bundle := generationBundle(t) + executor := &generationExecutor{complete: func(execution *promptexec.Execution) { + if execution.ProfileID == "weather-deep" { + execution.RawOutput = []byte(`{"summary":42}`) + } + }} + 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: executor, + }) + if err == nil || result == nil || result.Succeeded != 1 || result.Failed != 1 || result.ManifestPath == "" { + t.Fatalf("CompareDetailed() result/error = %#v/%v", result, err) + } + failure := result.Results[1] + if failure.Status != comparison.StatusFailed || failure.ValidationStatus != promptexec.ValidationPassed || failure.RepairAttempts == nil || *failure.RepairAttempts != 0 { + t.Fatalf("post-validation failure = %#v", failure) + } + data, readErr := os.ReadFile(result.ManifestPath) + if readErr != nil { + t.Fatal(readErr) + } + var manifest comparison.Manifest + if decodeErr := json.Unmarshal(data, &manifest); decodeErr != nil { + t.Fatal(decodeErr) + } + manifestFailure := manifest.Results[1] + if manifestFailure.ValidationStatus != "passed" || manifestFailure.RepairAttempts == nil || *manifestFailure.RepairAttempts != 0 { + t.Fatalf("published post-validation failure = %#v", manifestFailure) + } +} + func TestCompareDetailedRetainsCommittedPathsWhenBackupCleanupFails(t *testing.T) { for _, test := range []struct { name string diff --git a/internal/app/generation_test.go b/internal/app/generation_test.go index 3d68cc2..98322df 100644 --- a/internal/app/generation_test.go +++ b/internal/app/generation_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "net/http" "os" "path/filepath" "strings" @@ -549,6 +550,66 @@ func TestGenerateDetailedWritesRequestedPromptDebugArtifacts(t *testing.T) { } } +func TestGenerateDetailedCapturesProviderFailureOnlyInDebugArtifacts(t *testing.T) { + bundle := generationBundle(t) + debugRoot := t.TempDir() + const marker = "provider-private-generation-marker" + result, err := GenerateDetailed(context.Background(), GenerateRequest{ + Config: generationConfig(), Report: ReportDaily, + Date: generationTime("2026-05-29T12:00:00-05:00"), Now: generationTime("2026-05-29T08:30:00-05:00"), + WorkingDir: t.TempDir(), LLMDebugDir: debugRoot, Collector: &generationCollector{bundle: &bundle}, + Executor: &generationExecutor{executeErr: promptexec.NewGenerationError(http.StatusTooManyRequests, "rate_limit", "provider_error", marker, nil)}, + }) + if errors.Is(err, promptdebug.ErrSecureCaptureUnsupported) { + t.Skipf("secure prompt debug capture is unavailable: %v", err) + } + if err == nil || result == nil || result.LLMDebugPath == "" || promptexec.CategoryOf(err) != promptexec.Generation || !strings.Contains(err.Error(), "HTTP 429") || strings.Contains(err.Error(), marker) || result.OutputPath != "" { + t.Fatalf("GenerateDetailed() result/error = %#v/%v", result, err) + } + for _, name := range []string{"preparation.json", "failure.json"} { + if _, statErr := os.Stat(filepath.Join(result.LLMDebugPath, name)); statErr != nil { + t.Fatalf("debug artifact %q: %v", name, statErr) + } + } + data, readErr := os.ReadFile(filepath.Join(result.LLMDebugPath, "failure.json")) + if readErr != nil || !strings.Contains(string(data), marker) { + t.Fatalf("failure artifact = %q, error = %v", data, readErr) + } +} + +func TestGenerateDetailedPreservesProviderFailureWhenFailureDebugWriteFails(t *testing.T) { + bundle := generationBundle(t) + debugRoot := t.TempDir() + const marker = "provider-private-write-failure-marker" + var setupErr error + executor := &generationExecutor{ + executeErr: promptexec.NewGenerationError(http.StatusServiceUnavailable, "unavailable", "provider_error", marker, nil), + beforeExecute: func(promptexec.ExecuteRequest) { + setupErr = filepath.Walk(debugRoot, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if info.Name() == "preparation.json" { + return os.Mkdir(filepath.Join(filepath.Dir(path), "failure.json"), 0o700) + } + return nil + }) + }, + } + result, err := GenerateDetailed(context.Background(), GenerateRequest{ + Config: generationConfig(), Report: ReportDaily, + Date: generationTime("2026-05-29T12:00:00-05:00"), Now: generationTime("2026-05-29T08:30:00-05:00"), + WorkingDir: t.TempDir(), LLMDebugDir: debugRoot, Collector: &generationCollector{bundle: &bundle}, Executor: executor, + }) + if errors.Is(err, promptdebug.ErrSecureCaptureUnsupported) { + t.Skipf("secure prompt debug capture is unavailable: %v", err) + } + var generationError *promptexec.GenerationError + if setupErr != nil || err == nil || result == nil || result.OutputPath != "" || promptexec.CategoryOf(err) != promptexec.Generation || !errors.As(err, &generationError) || generationError.StatusCode() != http.StatusServiceUnavailable || !strings.Contains(err.Error(), "HTTP 503") || strings.Contains(err.Error(), marker) { + t.Fatalf("GenerateDetailed() setup/result/error = %v/%#v/%v", setupErr, result, err) + } +} + func generationConfig() config.Config { cfg := config.Defaults() cfg.WeatherAPI.Timezone, cfg.Location.ID = "America/Chicago", "home" diff --git a/internal/comparison/comparison.go b/internal/comparison/comparison.go index cd281ad..5941761 100644 --- a/internal/comparison/comparison.go +++ b/internal/comparison/comparison.go @@ -331,7 +331,7 @@ func (safeError SafeError) validate() error { } func isValidationStatus(status string) bool { - return status == "" || status == "failed" || status == "skipped" + return status == "" || status == "passed" || status == "failed" || status == "skipped" } func isSHA256(value string) bool { diff --git a/internal/comparison/comparison_test.go b/internal/comparison/comparison_test.go index f57d9a8..2df3a62 100644 --- a/internal/comparison/comparison_test.go +++ b/internal/comparison/comparison_test.go @@ -202,6 +202,9 @@ func TestManifestValidateRejectsInvariants(t *testing.T) { {name: "successful result without passed validation", mutate: func(manifest *Manifest) { manifest.Results[0].ValidationStatus = "failed" }}, {name: "failed result with report", mutate: func(manifest *Manifest) { manifest.Results[1].ReportPath = "02-weather-deep.md" }}, {name: "failed result without error", mutate: func(manifest *Manifest) { manifest.Results[1].Error = nil }}, + {name: "failed passed result without repair provenance", mutate: func(manifest *Manifest) { + manifest.Results[1].ValidationStatus = "passed" + }}, {name: "traversal report path", mutate: func(manifest *Manifest) { manifest.Results[0].ReportPath = "../report.md" }}, {name: "duplicate report path", mutate: func(manifest *Manifest) { manifest.Results[1] = Result{Position: 2, ProfileID: "weather-deep", ModelName: "gpt-5", Status: StatusSucceeded, ValidationStatus: "passed", ReportPath: manifest.Results[0].ReportPath} @@ -223,6 +226,17 @@ func TestManifestValidateRejectsInvariants(t *testing.T) { } } +func TestManifestValidateAcceptsPostValidationFailure(t *testing.T) { + t.Parallel() + + manifest := validManifest() + manifest.Results[1].ValidationStatus = "passed" + manifest.Results[1].RepairAttempts = intPtr(0) + if err := manifest.Validate(); err != nil { + t.Fatalf("Manifest.Validate() rejected post-validation failure: %v", err) + } +} + func TestLogicalBundleValidate(t *testing.T) { t.Parallel() diff --git a/internal/comparison/publish.go b/internal/comparison/publish.go index 15ac811..4807b2a 100644 --- a/internal/comparison/publish.go +++ b/internal/comparison/publish.go @@ -519,8 +519,15 @@ func validateSafeErrorJSON(decoder *json.Decoder) error { } func validateRepairAttemptsJSON(decoder *json.Decoder) error { + var raw json.RawMessage + if err := decoder.Decode(&raw); err != nil { + return err + } + if bytes.Equal(bytes.TrimSpace(raw), []byte("null")) { + return fmt.Errorf("repairAttempts must be an integer") + } var value int - if err := decoder.Decode(&value); err != nil { + if err := json.Unmarshal(raw, &value); err != nil { return err } if value < 0 { diff --git a/internal/comparison/publish_test.go b/internal/comparison/publish_test.go index f4c26bc..effdaf7 100644 --- a/internal/comparison/publish_test.go +++ b/internal/comparison/publish_test.go @@ -1,6 +1,7 @@ package comparison import ( + "bytes" "context" "encoding/json" "errors" @@ -146,6 +147,23 @@ func TestRecognizeBundleRejectsUnsafeAndMalformedContents(t *testing.T) { t.Helper() appendManifestField(t, directory, `"SchemaVersion": "weatherreporter.comparison.v1"`) }}, + {name: "null repair attempts", mutate: func(t *testing.T, directory string) { + t.Helper() + path := filepath.Join(directory, ManifestFilename) + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + before := []byte(" \"status\": \"failed\",\n \"error\": {") + after := []byte(" \"status\": \"failed\",\n \"repairAttempts\": null,\n \"error\": {") + data = bytes.Replace(data, before, after, 1) + if bytes.Equal(data, readFile(t, path)) { + t.Fatal("failed to add null repairAttempts") + } + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatal(err) + } + }}, {name: "traversal report path", mutate: func(t *testing.T, directory string) { t.Helper() path := filepath.Join(directory, ManifestFilename) @@ -202,6 +220,30 @@ func TestRecognizeBundleRejectsUnsafeAndMalformedContents(t *testing.T) { } } +func TestValidateRepairAttemptsJSONRejectsNonIntegerValues(t *testing.T) { + t.Parallel() + + for _, value := range []string{"null", "-1", "1.5", `"1"`, "9223372036854775808", "{"} { + value := value + t.Run(value, func(t *testing.T) { + t.Parallel() + if err := validateRepairAttemptsJSON(json.NewDecoder(strings.NewReader(value))); err == nil { + t.Fatalf("validateRepairAttemptsJSON(%q) error = nil", value) + } + }) + } +} + +func TestValidateRepairAttemptsJSONAcceptsNonnegativeIntegers(t *testing.T) { + t.Parallel() + + for _, value := range []string{"0", "1"} { + if err := validateRepairAttemptsJSON(json.NewDecoder(strings.NewReader(value))); err != nil { + t.Fatalf("validateRepairAttemptsJSON(%q) error = %v", value, err) + } + } +} + func appendManifestField(t *testing.T, directory, field string) { t.Helper() path := filepath.Join(directory, ManifestFilename)