diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 9c98ffc..5f50e22 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Invocation reporting audit remediation -Status: remediation in progress; 1 of 7 findings complete. +Status: remediation in progress; 2 of 7 findings complete. ## Review scope @@ -132,7 +132,12 @@ correctly detects missing artifacts or invalid selections. ## 2. Retain independent rendering errors joined with write errors -Priority: P2. Status: open. +Priority: P2. Status: complete. + +- [x] Completed: joined callback errors retain independent rendering evidence, + while plain and wrapped sink errors remain write failures. Output and detail + regressions pass, including joined destination errors. + Validation: `GOTOOLCHAIN=local go test ./...` in the repository root. ### Finding and evidence diff --git a/reporter.go b/reporter.go index 5c680d4..fa89794 100644 --- a/reporter.go +++ b/reporter.go @@ -3,6 +3,7 @@ package commandkit import ( "errors" "io" + "reflect" "strings" "sync" @@ -475,13 +476,46 @@ func finishRenderLocked(state *reportingState, destination reportingDestination, } func recordRendererErrorLocked(state *reportingState, destination reportingDestination, renderErr, writeErr error) error { - if renderErr == nil || (writeErr != nil && errors.Is(renderErr, writeErr)) { + if renderErr == nil || (writeErr != nil && propagatesOnlyWriteError(renderErr, writeErr)) { return nil } state.recordRenderErrorLocked(destination, renderErr) return renderErr } +// A joined callback error may contain both a writer failure and independent +// rendering evidence. Walk each branch once rather than discarding the entire +// error merely because errors.Is finds the writer failure somewhere in it. +func propagatesOnlyWriteError(err, writeErr error) bool { + for err != nil { + if reflect.TypeOf(err).Comparable() && err == writeErr { + return true + } + switch wrapped := err.(type) { + case interface{ Unwrap() []error }: + found := false + for _, child := range wrapped.Unwrap() { + if child != nil { + found = true + if !propagatesOnlyWriteError(child, writeErr) { + return false + } + } + } + if found { + return true + } + case interface{ Unwrap() error }: + if child := wrapped.Unwrap(); child != nil { + err = child + continue + } + } + return errors.Is(err, writeErr) + } + return false +} + func combineOperationErrors(renderErr, writeErr error) error { switch { case renderErr == nil: diff --git a/reporter_error_test.go b/reporter_error_test.go new file mode 100644 index 0000000..90d8835 --- /dev/null +++ b/reporter_error_test.go @@ -0,0 +1,60 @@ +package commandkit + +import ( + "errors" + "fmt" + "io" + "testing" +) + +func TestReporterPreservesJoinedRendererEvidence(t *testing.T) { + for _, kind := range []string{"output", "detail"} { + for _, test := range []struct { + name string + combine func(error, error) error + rendering bool + }{ + {"joined", func(render, write error) error { return errors.Join(render, write) }, true}, + {"wrapped join", func(render, write error) error { + return fmt.Errorf("rendering context: %w", errors.Join(write, render)) + }, true}, + {"plain write", func(_, write error) error { return write }, false}, + {"wrapped write", func(_, write error) error { return fmt.Errorf("writing context: %w", write) }, false}, + {"joined write only", func(_, write error) error { return errors.Join(write, fmt.Errorf("same writer: %w", write)) }, false}, + } { + t.Run(kind+"/"+test.name, func(t *testing.T) { + renderErr := errors.New("independent rendering failure") + // The destination error can itself be a joined value. Returning that + // complete error still propagates a single destination failure. + writeErr := errors.Join(errors.New("sink failure"), errors.New("sink detail")) + destination := &reporterTestWriter{err: writeErr} + reporter := newReporter(Streams{Out: destination, Err: destination}) + reporter.setVisibility(modeVerbose, false, false) + render := func(w io.Writer) error { + _, err := io.WriteString(w, "report") + return test.combine(renderErr, err) + } + var err error + if kind == "output" { + err = reporter.RenderOutput(render) + } else { + err = reporter.Warn("", render) + } + if !errors.Is(err, writeErr) || errors.Is(err, renderErr) != test.rendering { + t.Fatalf("returned error lost evidence: %v", err) + } + got := reporter.finalize(Success(), nil) + result := got.Presentation.Output + if kind == "detail" { + result = got.Presentation.Diagnostics + } + if result.State != DeliveryIncomplete || !errors.Is(result.WriteError, writeErr) || errors.Is(result.RenderError, renderErr) != test.rendering { + t.Fatalf("presentation=%#v", result) + } + if !test.rendering && result.RenderError != nil { + t.Fatalf("write propagation became rendering evidence: %v", result.RenderError) + } + }) + } + } +}