diff --git a/builtins_reporting_test.go b/builtins_reporting_test.go index e06bc8d..93c1df5 100644 --- a/builtins_reporting_test.go +++ b/builtins_reporting_test.go @@ -73,7 +73,7 @@ func TestHelpAndVersionDestinationFailuresUseReporterObservations(t *testing.T) if !errors.Is(outcome.Presentation.Output.WriteError, writeErr) || outcome.Presentation.Output.State != DeliveryIncomplete { t.Errorf("Execute(%q) presentation = %#v", args, outcome.Presentation) } - if !strings.Contains(diagnostics.String(), "ERROR: Command failed.") { + if !strings.Contains(diagnostics.String(), "deliver") || !strings.Contains(diagnostics.String(), "output") { t.Errorf("Execute(%q) diagnostics = %q", args, diagnostics.String()) } } diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 5f50e22..c6ae737 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Invocation reporting audit remediation -Status: remediation in progress; 2 of 7 findings complete. +Status: remediation in progress; 3 of 7 findings complete. ## Review scope @@ -185,7 +185,12 @@ detail-rendering path as needed to establish that the shared fix applies there. ## 3. Explain failures introduced by CommandKit presentation -Priority: P2. Status: open. +Priority: P2. Status: complete. + +- [x] Completed: presentation failures that turn success into failure now retain + a core-owned rendering/delivery explanation. Supplied explanations and causes, + generic handler fallback, quiet rendering, domain status, and failed-sink + behavior remain covered. Validation: `GOTOOLCHAIN=local go test ./...` at root. ### Finding and evidence diff --git a/execute_external_test.go b/execute_external_test.go index 42a2d0f..092c58a 100644 --- a/execute_external_test.go +++ b/execute_external_test.go @@ -518,7 +518,7 @@ func TestHelpEntryFormsPresentReportAndDiagnosticFailures(t *testing.T) { for _, args := range [][]string{nil, {"notes"}, {"--help"}, {"-h"}, {"notes", "--help"}, {"help"}, {"help", "notes"}} { var diagnostics bytes.Buffer result := app.Execute(context.Background(), args, commandkit.Streams{Out: failingWriter{reportErr}, Err: &diagnostics}) - if result.Kind != commandkit.OutcomeOperational || result.Presentation.Output.State != commandkit.DeliveryIncomplete || !errors.Is(result.Presentation.Output.WriteError, reportErr) || result.Presentation.Diagnostics.State != commandkit.DeliveryComplete || !strings.Contains(diagnostics.String(), "ERROR: Command failed.") { + if result.Kind != commandkit.OutcomeOperational || result.Presentation.Output.State != commandkit.DeliveryIncomplete || !errors.Is(result.Presentation.Output.WriteError, reportErr) || result.Presentation.Diagnostics.State != commandkit.DeliveryComplete || (!strings.Contains(diagnostics.String(), "deliver") || !strings.Contains(diagnostics.String(), "output")) { t.Fatalf("%v: result=%#v diagnostics=%q", args, result, diagnostics.String()) } result = app.Execute(context.Background(), args, commandkit.Streams{Out: failingWriter{reportErr}, Err: failingWriter{diagnosticErr}}) diff --git a/execute_reporting_test.go b/execute_reporting_test.go index 25b030c..5a0c99d 100644 --- a/execute_reporting_test.go +++ b/execute_reporting_test.go @@ -117,7 +117,7 @@ func TestExecuteFinalizesEarlyAndIgnoredWriterFailures(t *testing.T) { if outcome.Kind != OutcomeOperational || outcome.Status != 1 || !errors.Is(outcome.Presentation.Output.WriteError, writeErr) { t.Fatalf("ignored output failure outcome = %#v", outcome) } - if !strings.Contains(diagnostics.String(), "ERROR: Command failed.") { + if !strings.Contains(diagnostics.String(), "deliver") || !strings.Contains(diagnostics.String(), "output") { t.Fatalf("fallback diagnostic = %q", diagnostics.String()) } diff --git a/reporter.go b/reporter.go index fa89794..13f903e 100644 --- a/reporter.go +++ b/reporter.go @@ -281,6 +281,9 @@ func (r *Reporter) finalize(outcome Outcome, completion func(*Reporter)) Outcome state.mu.Lock() if presentationFailedLocked(state) { + if outcome.Status == 0 && !hasExplanation(outcome.Explanation) { + outcome.Explanation.Message = presentationFailureMessage(state) + } outcome = applyPresentationFailure(outcome) } if outcome.Status != 0 { @@ -328,18 +331,38 @@ func applyPresentationFailure(outcome Outcome) Outcome { } func ensureFallbackExplanation(outcome Outcome) Outcome { - if outcome.Explanation.Message != "" { + if hasExplanation(outcome.Explanation) { return outcome } - for _, detail := range outcome.Explanation.Details { - if detail != nil { - return outcome - } - } outcome.Explanation = Explanation{Message: "Command failed."} return outcome } +func hasExplanation(explanation Explanation) bool { + if explanation.Message != "" { + return true + } + for _, detail := range explanation.Details { + if detail != nil { + return true + } + } + return false +} + +func presentationFailureMessage(state *reportingState) string { + switch { + case state.output.renderError != nil: + return "Unable to render command output." + case state.output.writeError != nil: + return "Unable to deliver command output." + case state.diagnostics.renderError != nil: + return "Unable to render invocation diagnostics." + default: + return "Unable to deliver invocation diagnostics." + } +} + func renderTerminalLocked(state *reportingState, explanation Explanation) error { destination := state.destination(destinationDiagnostics) destination.submitted = true diff --git a/reporter_error_test.go b/reporter_error_test.go index 90d8835..570765d 100644 --- a/reporter_error_test.go +++ b/reporter_error_test.go @@ -1,12 +1,67 @@ package commandkit import ( + "bytes" "errors" "fmt" "io" + "strings" "testing" ) +func TestReporterExplainsItsOwnFailuresWithoutExposingCauses(t *testing.T) { + for _, kind := range []string{"output", "diagnostics"} { + for _, quiet := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/quiet=%t", kind, quiet), func(t *testing.T) { + var diagnostics bytes.Buffer + reporter := newReporter(Streams{Err: &diagnostics}) + if quiet { + reporter.setVisibility(modeQuiet, false, false) + } + failure := errors.New("private rendering detail") + render := func(io.Writer) error { return failure } + if kind == "output" { + _ = reporter.RenderOutput(render) + } else { + _ = reporter.Warn("", render) + } + cause := errors.New("private application cause") + got := reporter.finalize(Domain("example", 0).WithCause(cause), nil) + if got.Kind != OutcomeDomain || got.Status != 1 || got.Cause != cause { + t.Fatalf("outcome=%#v", got) + } + if !strings.Contains(got.Explanation.Message, "render") || !strings.Contains(got.Explanation.Message, kind) || !strings.Contains(diagnostics.String(), got.Explanation.Message) { + t.Fatalf("explanation=%#v diagnostics=%q", got.Explanation, diagnostics.String()) + } + if strings.Contains(diagnostics.String(), "private") { + t.Fatalf("private error text was exposed: %q", diagnostics.String()) + } + }) + } + } +} + +func TestReporterPreservesExplicitSuccessExplanationOnPresentationFailure(t *testing.T) { + for _, message := range []string{"supplied context", ""} { + t.Run(message, func(t *testing.T) { + var diagnostics bytes.Buffer + reporter := newReporter(Streams{Err: &diagnostics}) + _ = reporter.RenderOutput(func(io.Writer) error { return errors.New("render failed") }) + calls := 0 + got := reporter.finalize(Success().WithExplanation(message, func(io.Writer) error { + calls++ + return nil + }), nil) + if got.Status != 1 || got.Explanation.Message != message || calls != 1 { + t.Fatalf("outcome=%#v calls=%d", got, calls) + } + if message == "" && diagnostics.Len() != 0 { + t.Fatalf("explicit zero-byte explanation replaced: %q", diagnostics.String()) + } + }) + } +} + func TestReporterPreservesJoinedRendererEvidence(t *testing.T) { for _, kind := range []string{"output", "detail"} { for _, test := range []struct { diff --git a/reporter_test.go b/reporter_test.go index 0249d43..f532029 100644 --- a/reporter_test.go +++ b/reporter_test.go @@ -592,7 +592,7 @@ func TestReporterPresentationFailureChangesOutcomeAndPreservesDomain(t *testing. if outcome.Kind != OutcomeOperational || outcome.Status != 1 || !errors.Is(outcome.Cause, cause) { t.Fatalf("success presentation failure outcome = %#v", outcome) } - if !errors.Is(outcome.Presentation.Output.WriteError, outputErr) || diagnostics.String() != "ERROR: Command failed.\n" { + if !errors.Is(outcome.Presentation.Output.WriteError, outputErr) || (!strings.Contains(diagnostics.String(), "deliver") || !strings.Contains(diagnostics.String(), "output")) { t.Fatalf("presentation=%#v diagnostics=%q", outcome.Presentation, diagnostics.String()) } @@ -644,7 +644,7 @@ func TestReporterUsesHealthyDiagnosticsAfterOutputRenderFailure(t *testing.T) { if outcome.Kind != OutcomeOperational || outcome.Status != 1 { t.Fatalf("outcome = %#v", outcome) } - if !errors.Is(outcome.Presentation.Output.RenderError, renderErr) || diagnostics.String() != "ERROR: Command failed.\n" { + if !errors.Is(outcome.Presentation.Output.RenderError, renderErr) || (!strings.Contains(diagnostics.String(), "render") || !strings.Contains(diagnostics.String(), "output")) { t.Fatalf("presentation=%#v diagnostics=%q", outcome.Presentation, diagnostics.String()) } }