From 473fb027a17fe7010ed154483ea43184ad0a1242 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 26 Oct 2026 14:35:44 +0000 Subject: [PATCH] Avoid retaining hidden simple deferred messages --- docs/roadmap/remediation.md | 10 ++++-- reporter.go | 47 ++++++++++++++++--------- reporter_deferred_test.go | 69 +++++++++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 18 deletions(-) create mode 100644 reporter_deferred_test.go diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index bca51a0..e0f9da9 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Invocation reporting audit remediation -Status: remediation in progress; 5 of 7 findings complete. +Status: remediation in progress; 6 of 7 findings complete. ## Review scope @@ -381,7 +381,13 @@ owners. ## 6. Avoid retaining deferred messages that need no presentation work -Priority: P3. Status: for consideration. +Priority: P3. Status: complete. + +- [x] Completed: renderer-free hidden deferred messages record suppression + without queue retention. Finalization detaches the queue without an array + copy and delivers captured callback lists without recopying them. Hidden rich + callbacks, FIFO order, no-content handling, capture, and closure remain covered. + Validation: root module suite and focused reporter race checks pass. ### Finding and complexity diff --git a/reporter.go b/reporter.go index 13f903e..5c133cf 100644 --- a/reporter.go +++ b/reporter.go @@ -249,6 +249,16 @@ func (r DeferredReporter) enqueue(category messageCategory, message string, deta if state.closed { return ErrReporterClosed } + if !hasDetails(details) { + if message == "" { + return nil + } + if !state.visible(category) { + state.diagnostics.submitted = true + state.diagnostics.suppressed = true + return nil + } + } state.deferred = append(state.deferred, deferredMessage{ category: category, message: message, @@ -268,12 +278,16 @@ func (r *Reporter) finalize(outcome Outcome, completion func(*Reporter)) Outcome state.mu.Unlock() return normalizeOutcome(outcome) } - deferred := append([]deferredMessage(nil), state.deferred...) + deferred := state.deferred state.deferred = nil state.mu.Unlock() - for _, message := range deferred { - _ = r.submitMessage(message.category, message.message, message.details) + for index := range deferred { + message := deferred[index] + deferred[index] = deferredMessage{} + state.mu.Lock() + _ = state.submitMessageLocked(message.category, message.message, message.details) + state.mu.Unlock() } if completion != nil { completion(r) @@ -339,10 +353,11 @@ func ensureFallbackExplanation(outcome Outcome) Outcome { } func hasExplanation(explanation Explanation) bool { - if explanation.Message != "" { - return true - } - for _, detail := range explanation.Details { + return explanation.Message != "" || hasDetails(explanation.Details) +} + +func hasDetails(details []Detail) bool { + for _, detail := range details { if detail != nil { return true } @@ -405,16 +420,16 @@ func (r *Reporter) submitMessage(category messageCategory, message string, detai if state.closed { return ErrReporterClosed } + return state.submitMessageLocked(category, message, append([]Detail(nil), details...)) +} - details = append([]Detail(nil), details...) - hasDetails := false - for _, detail := range details { - if detail != nil { - hasDetails = true - break - } +// Queued detail lists have already been captured; delivery need not copy them. +func (state *reportingState) submitMessageLocked(category messageCategory, message string, details []Detail) error { + if state.closed { + return ErrReporterClosed } - if message == "" && !hasDetails { + rich := hasDetails(details) + if message == "" && !rich { return nil } @@ -423,7 +438,7 @@ func (r *Reporter) submitMessage(category messageCategory, message string, detai destination.submitted = true if !visible { destination.suppressed = true - if !hasDetails { + if !rich { return nil } } diff --git a/reporter_deferred_test.go b/reporter_deferred_test.go new file mode 100644 index 0000000..c797e0f --- /dev/null +++ b/reporter_deferred_test.go @@ -0,0 +1,69 @@ +package commandkit + +import ( + "bytes" + "errors" + "fmt" + "io" + "strings" + "testing" +) + +func TestHiddenSimpleDeferredMessagesDoNotAccumulate(t *testing.T) { + for _, mode := range []reportingMode{modeOrdinary, modeQuiet} { + t.Run(fmt.Sprint(mode), func(t *testing.T) { + reporter := newReporter(Streams{}) + reporter.setVisibility(mode, false, false) + for index := 0; index < 256; index++ { + if err := reporter.AtEnd().Debug(fmt.Sprintf("hidden %d", index), nil); err != nil { + t.Fatal(err) + } + if err := reporter.AtEnd().Warn("hidden warning"); err != nil { + t.Fatal(err) + } + } + // This assertion owns the storage optimization, not a public queue API. + if len(reporter.state.deferred) != 0 { + t.Fatalf("retained %d renderer-free hidden messages", len(reporter.state.deferred)) + } + got := reporter.finalize(Success(), nil) + if got.Status != 0 || got.Presentation.Diagnostics.State != DeliverySuppressed { + t.Fatalf("outcome=%#v", got) + } + if err := reporter.AtEnd().Debug("late"); !errors.Is(err, ErrReporterClosed) { + t.Fatalf("closed hidden submission=%v", err) + } + }) + } + noContent := newReporter(Streams{}) + _ = noContent.AtEnd().Warn("", nil) + if got := noContent.finalize(Success(), nil); got.Presentation.Diagnostics.State != DeliveryNone { + t.Fatalf("empty deferred submission became content: %#v", got) + } +} + +func TestHiddenDeferredDetailsRemainDeferredAndCaptured(t *testing.T) { + var diagnostics bytes.Buffer + reporter := newReporter(Streams{Err: &diagnostics}) + reporter.setVisibility(modeQuiet, false, false) + failure := errors.New("hidden rendering failed") + var order []string + details := []Detail{func(w io.Writer) error { + order = append(order, "first") + _, _ = io.WriteString(w, "hidden detail") + return failure + }} + _ = reporter.AtEnd().Warn("hidden", details...) + details[0] = func(io.Writer) error { t.Error("caller changed captured callback"); return nil } + _ = reporter.AtEnd().Info("", func(io.Writer) error { order = append(order, "second"); return nil }) + if len(order) != 0 { + t.Fatal("details ran at submission") + } + got := reporter.finalize(Success(), nil) + if strings.Join(order, ",") != "first,second" || !errors.Is(got.Presentation.Diagnostics.RenderError, failure) || got.Status != 1 { + t.Fatalf("order=%v outcome=%#v", order, got) + } + if strings.Contains(diagnostics.String(), "hidden") || len(reporter.state.deferred) != 0 { + t.Fatalf("hidden content leaked or queue retained: %q", diagnostics.String()) + } +}