fix: return text report output failures to command callers

cmd/pgbot/advise.go

@@ -22,7 +22,13 @@ type adviseFlags struct {
 	timeout        time.Duration
 }
 
+type adviseRunner func(context.Context, string, int, float64) (adviseResult, error)
+
 func newAdviseCmd() *cobra.Command {
+	return newAdviseCmdWithRunner(nil)
+}
+
+func newAdviseCmdWithRunner(run adviseRunner) *cobra.Command {
 	var f adviseFlags
 	cmd := &cobra.Command{
 		Use:   "advise <connection-string>",
@@ -36,7 +42,10 @@ func newAdviseCmd() *cobra.Command {
 			"plans your query — it never executes it.",
 		Args: cobra.MaximumNArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return runAdvise(cmd, args, f)
+			if run == nil {
+				return runAdvise(cmd, args, f)
+			}
+			return runAdviseWithRunner(cmd, args, f, run)
 		},
 	}
 	fl := cmd.Flags()
@@ -98,14 +107,18 @@ func adviseRun(ctx context.Context, connString string, top int, minImpr float64)
 }
 
 func runAdvise(cmd *cobra.Command, args []string, f adviseFlags) error {
+	return runAdviseWithRunner(cmd, args, f, adviseRun)
+}
+
+func runAdviseWithRunner(cmd *cobra.Command, args []string, f adviseFlags, run adviseRunner) error {
 	connString := firstNonEmpty(argAt(args, 0), os.Getenv("DATABASE_URL"), os.Getenv("PGBOT_DATABASE_URL"), pgServiceFallback())
 	if connString == "" {
 		return fmt.Errorf("no connection string (pass one or set $DATABASE_URL)")
 	}
 	ctx, cancel := context.WithTimeout(cmd.Context(), f.timeout)
 	defer cancel()
 
-	res, err := adviseRun(ctx, connString, f.top, f.minImprovement)
+	res, err := run(ctx, connString, f.top, f.minImprovement)
 	if err != nil {
 		return err
 	}
@@ -124,12 +137,11 @@ func runAdvise(cmd *cobra.Command, args []string, f adviseFlags) error {
 			"skipped_stale":   stats.SkippedStaleStats,
 		})
 	}
-	render.AdvisorReport(os.Stdout, render.AdvisorInput{
+	return render.AdvisorReport(cmd.OutOrStdout(), render.AdvisorInput{
 		Color: useColor(f.noColor), Database: res.Database, VersionNum: res.VersionNum,
 		Recommendations: recs, Considered: stats.QueriesConsidered, Planned: stats.QueriesPlanned,
 		Candidates: stats.CandidatesTested, SkippedStale: stats.SkippedStaleStats,
 	})
-	return nil
 }
 
 // suggestIndexesTool is the MCP counterpart to `pgbot advise`: validated index

cmd/pgbot/advise_output_error_regression_test.go

@@ -0,0 +1,22 @@
+package main
+
+import (
+	"context"
+	"errors"
+	"testing"
+)
+
+func TestAdviseCommand_ReturnsOutputError(t *testing.T) {
+	cmd := newAdviseCmdWithRunner(func(context.Context, string, int, float64) (adviseResult, error) {
+		return adviseResult{Database: "app", VersionNum: 160000}, nil
+	})
+	cmd.SilenceErrors = true
+	cmd.SilenceUsage = true
+	cmd.SetOut(rejectingCommandWriter{})
+	cmd.SetArgs([]string{"postgres://fixture/app", "--no-color"})
+
+	err := cmd.Execute()
+	if !errors.Is(err, errCommandOutput) {
+		t.Fatalf("command error = %v, want output error (the root maps this execution failure to exit 3)", err)
+	}
+}

cmd/pgbot/diff.go

@@ -3,6 +3,7 @@ package main
 import (
 	"encoding/json"
 	"fmt"
+	"io"
 	"os"
 	"strings"
 	"time"
@@ -33,8 +34,8 @@ func newDiffCmd() *cobra.Command {
 			"reset or pg_stat_statements eviction between the snapshots makes specific deltas\n" +
 			"untrustworthy. It never compares two different databases.",
 		Args: cobra.NoArgs,
-		RunE: func(_ *cobra.Command, _ []string) error {
-			return runDiff(f)
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return runDiff(cmd.OutOrStdout(), f)
 		},
 	}
 	fl := cmd.Flags()
@@ -109,21 +110,20 @@ func resolveDiff(storePath, fpSpec string, since time.Duration) (*diffResult, er
 	}, nil
 }
 
-func runDiff(f diffFlags) error {
+func runDiff(w io.Writer, f diffFlags) error {
 	r, err := resolveDiff(f.storePath, f.fingerprint, f.since)
 	if err != nil {
 		return err
 	}
 	if f.json {
 		return json.NewEncoder(os.Stdout).Encode(diffJSON(r))
 	}
-	render.DiffReport(os.Stdout, render.DiffInput{
+	return render.DiffReport(w, render.DiffInput{
 		Color: useColor(f.noColor), Database: r.Item.Database, Fingerprint: r.Item.Fingerprint,
 		BaselineAt: r.Baseline.CollectedAt, CurrentAt: r.Current.CollectedAt,
 		Requested: r.Requested, Actual: r.Actual,
 		ResetReason: r.ResetReason, PgssEvicted: r.PgssEvicted, Deltas: r.Deltas,
 	})
-	return nil
 }
 
 // diffJSON is the machine-readable shape of a diff, shared by --json and the MCP

cmd/pgbot/diff_output_error_regression_test.go

@@ -0,0 +1,52 @@
+package main
+
+import (
+	"errors"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/pgrundev/pgbot/internal/model"
+	"github.com/pgrundev/pgbot/internal/store"
+)
+
+var errCommandOutput = errors.New("command output rejected")
+
+type rejectingCommandWriter struct{}
+
+func (rejectingCommandWriter) Write([]byte) (int, error) { return 0, errCommandOutput }
+
+func TestDiffCommand_ReturnsOutputError(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "baselines.db")
+	st, err := store.Open(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	now := time.Now().UTC()
+	for i, at := range []time.Time{now.Add(-2 * time.Hour), now.Add(-time.Minute)} {
+		c := &model.Context{
+			SchemaVersion: model.SchemaVersion,
+			Fingerprint:   "diff-output",
+			CollectedAt:   at,
+			Server:        model.ServerInfo{Database: "app"},
+			Health:        &model.Health{Connections: i + 1},
+		}
+		if _, err := st.Save(c); err != nil {
+			st.Close()
+			t.Fatal(err)
+		}
+	}
+	if err := st.Close(); err != nil {
+		t.Fatal(err)
+	}
+
+	cmd := newDiffCmd()
+	cmd.SilenceErrors = true
+	cmd.SilenceUsage = true
+	cmd.SetOut(rejectingCommandWriter{})
+	cmd.SetArgs([]string{"--store", path, "--since", "1h", "--no-color"})
+	err = cmd.Execute()
+	if !errors.Is(err, errCommandOutput) {
+		t.Fatalf("command error = %v, want output error (the root maps this execution failure to exit 3)", err)
+	}
+}

internal/render/advisor.go

@@ -23,7 +23,7 @@ type AdvisorInput struct {
 // AdvisorReport renders validated index recommendations. Every line makes the
 // "hypothetical, planner-confirmed, nothing built" guarantee explicit — a user
 // must never think pgbot changed their database.
-func AdvisorReport(w io.Writer, in AdvisorInput) {
+func AdvisorReport(w io.Writer, in AdvisorInput) error {
 	st := styler{on: in.Color}
 	var b strings.Builder
 
@@ -37,8 +37,7 @@ func AdvisorReport(w io.Writer, in AdvisorInput) {
 			"considered %d slow quer(y/ies), planned %d, tested %d candidate(s) — the planner didn't confirm a cost win for any.",
 			in.Considered, in.Planned, in.Candidates)))
 		fmt.Fprintln(&b, st.dim("That's a good sign: the queries pgbot could plan are already served by existing indexes."))
-		_, _ = io.WriteString(w, b.String())
-		return
+		return writeTextReport(w, b.String())
 	}
 
 	fmt.Fprintln(&b, st.head(fmt.Sprintf("%d validated recommendation(s):", len(in.Recommendations))))
@@ -75,7 +74,7 @@ func AdvisorReport(w io.Writer, in AdvisorInput) {
 		tail += fmt.Sprintf(" %d candidate(s) skipped: table statistics too stale to trust the estimate (ANALYZE first).", in.SkippedStale)
 	}
 	fmt.Fprintln(&b, st.dim(tail))
-	_, _ = io.WriteString(w, b.String())
+	return writeTextReport(w, b.String())
 }
 
 // humanCount abbreviates large call counts (1200000 → 1.2M).

internal/render/diff.go

@@ -26,7 +26,7 @@ type DiffInput struct {
 // DiffReport renders a comparison of two baseline snapshots. It never claims the
 // requested interval when it got a different one, and it says up front when a
 // reset or eviction between the snapshots makes specific deltas untrustworthy.
-func DiffReport(w io.Writer, in DiffInput) {
+func DiffReport(w io.Writer, in DiffInput) error {
 	st := styler{on: in.Color}
 	var b strings.Builder
 
@@ -63,8 +63,7 @@ func DiffReport(w io.Writer, in DiffInput) {
 
 	if in.Deltas == nil || len(in.Deltas.Changes) == 0 {
 		fmt.Fprintln(&b, st.good("✓ nothing material changed between these snapshots"))
-		_, _ = io.WriteString(w, b.String())
-		return
+		return writeTextReport(w, b.String())
 	}
 
 	fmt.Fprintln(&b, st.head(fmt.Sprintf("%d change(s):", len(in.Deltas.Changes))))
@@ -79,7 +78,18 @@ func DiffReport(w io.Writer, in DiffInput) {
 		}
 		fmt.Fprintf(&b, "  %s %s  %s  %s\n", color("·"), d.Subject, change, st.dim(d.Note))
 	}
-	_, _ = io.WriteString(w, b.String())
+	return writeTextReport(w, b.String())
+}
+
+func writeTextReport(w io.Writer, text string) error {
+	n, err := io.WriteString(w, text)
+	if err != nil {
+		return err
+	}
+	if n != len(text) {
+		return io.ErrShortWrite
+	}
+	return nil
 }
 
 // shortFP abbreviates a fingerprint for display.

internal/render/writer_errors_regression_test.go

@@ -0,0 +1,140 @@
+package render
+
+import (
+	"bytes"
+	"errors"
+	"io"
+	"reflect"
+	"strings"
+	"testing"
+
+	"github.com/pgrundev/pgbot/internal/advisor"
+	"github.com/pgrundev/pgbot/internal/model"
+)
+
+var errReportOutput = errors.New("report output rejected")
+
+type boundedReportWriter struct {
+	limit int
+	err   error
+}
+
+func (w boundedReportWriter) Write(p []byte) (int, error) {
+	n := w.limit
+	if n > len(p) {
+		n = len(p)
+	}
+	return n, w.err
+}
+
+func callReport(t *testing.T, report any, w io.Writer, input any) error {
+	t.Helper()
+	out := reflect.ValueOf(report).Call([]reflect.Value{reflect.ValueOf(w), reflect.ValueOf(input)})
+	if len(out) != 1 {
+		t.Fatalf("renderer returned %d values, want one error", len(out))
+	}
+	if out[0].IsNil() {
+		return nil
+	}
+	err, ok := out[0].Interface().(error)
+	if !ok {
+		t.Fatalf("renderer returned %T, want error", out[0].Interface())
+	}
+	return err
+}
+
+func largeDiffInput() DiffInput {
+	changes := make([]model.Delta, 600)
+	for i := range changes {
+		changes[i] = model.Delta{
+			Subject:  strings.Repeat("query-", 24),
+			Severity: model.SeverityWarn,
+			Before:   1,
+			After:    2,
+			Note:     strings.Repeat("changed ", 32),
+		}
+	}
+	return DiffInput{Database: "app", Fingerprint: "abcdef", Deltas: &model.Deltas{Changes: changes}}
+}
+
+func largeAdvisorInput() AdvisorInput {
+	recs := make([]advisor.Recommendation, 300)
+	for i := range recs {
+		recs[i] = advisor.Recommendation{
+			Schema: "public", Table: "orders", IndexDDL: "CREATE INDEX ON public.orders (customer_id)",
+			QueryText: strings.Repeat("SELECT * FROM orders WHERE customer_id = $1 ", 8),
+			Calls:     1000, SharePct: 25, CostBefore: 1000, CostAfter: 100,
+			Caveats: []string{strings.Repeat("writes also maintain this index ", 8)},
+		}
+	}
+	return AdvisorInput{Database: "app", VersionNum: 160000, Recommendations: recs, Planned: 300, Candidates: 300}
+}
+
+func TestTextReports_PropagateWriterErrors(t *testing.T) {
+	tests := []struct {
+		name   string
+		report any
+		input  any
+	}{
+		{name: "diff empty", report: DiffReport, input: DiffInput{Database: "app"}},
+		{name: "diff reset only", report: DiffReport, input: DiffInput{Database: "app", ResetReason: "server restarted"}},
+		{name: "advisor empty", report: AdvisorReport, input: AdvisorInput{Database: "app", VersionNum: 160000}},
+		{name: "diff large", report: DiffReport, input: largeDiffInput()},
+		{name: "advisor large", report: AdvisorReport, input: largeAdvisorInput()},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			err := callReport(t, tc.report, boundedReportWriter{limit: 17, err: errReportOutput}, tc.input)
+			if !errors.Is(err, errReportOutput) {
+				t.Fatalf("error = %v, want errors.Is(..., %v)", err, errReportOutput)
+			}
+		})
+	}
+}
+
+func TestTextReports_RejectSilentShortWrites(t *testing.T) {
+	for _, tc := range []struct {
+		name   string
+		report any
+		input  any
+	}{
+		{name: "diff", report: DiffReport, input: largeDiffInput()},
+		{name: "advisor", report: AdvisorReport, input: largeAdvisorInput()},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			err := callReport(t, tc.report, boundedReportWriter{limit: 17}, tc.input)
+			if !errors.Is(err, io.ErrShortWrite) {
+				t.Fatalf("error = %v, want io.ErrShortWrite", err)
+			}
+		})
+	}
+}
+
+func TestTextReports_PreserveNormalOutput(t *testing.T) {
+	var diffOut bytes.Buffer
+	if err := callReport(t, DiffReport, &diffOut, DiffInput{Database: "app", Deltas: &model.Deltas{}}); err != nil {
+		t.Fatal(err)
+	}
+	if !strings.Contains(diffOut.String(), "nothing material changed") {
+		t.Fatalf("unexpected diff output:\n%s", diffOut.String())
+	}
+
+	var advisorOut bytes.Buffer
+	if err := callReport(t, AdvisorReport, &advisorOut, AdvisorInput{Database: "app", VersionNum: 160000}); err != nil {
+		t.Fatal(err)
+	}
+	if !strings.Contains(advisorOut.String(), "no validated index recommendations") {
+		t.Fatalf("unexpected advisor output:\n%s", advisorOut.String())
+	}
+
+	var largeDiff, largeAdvisor bytes.Buffer
+	if err := callReport(t, DiffReport, &largeDiff, largeDiffInput()); err != nil {
+		t.Fatal(err)
+	}
+	if err := callReport(t, AdvisorReport, &largeAdvisor, largeAdvisorInput()); err != nil {
+		t.Fatal(err)
+	}
+	if largeDiff.Len() < 64<<10 || largeAdvisor.Len() < 64<<10 {
+		t.Fatalf("large reports were unexpectedly small: diff=%d advisor=%d", largeDiff.Len(), largeAdvisor.Len())
+	}
+}