From faa9c8d0c4fc310af7cbab46ea8dcc902e9d8937 Mon Sep 17 00:00:00 2001 From: Divyam Talwar Date: Tue, 22 Sep 2026 06:15:55 +0530 Subject: [PATCH] fix: return text report output failures to command callers The diff and advisor text renderers ignored their final write result, so a broken destination could produce a successful command with missing output. Return the original error, reject silent short writes, and propagate the result through the actual Cobra handlers. Tests exercise both command paths, real SQLite-backed diff history, large and empty reports, reset-only output and successful output preservation. The advisor collection seam supplies only the backend result. The original diff command and a swallowed-error advisor control fail; focused tests pass 20 race repetitions and full render/CLI race suites pass. No SQL, dependencies or JSON contract changes. The separate reset branch in PR #64 must propagate this same writer error when the changes are combined. --- cmd/pgbot/advise.go | 20 ++- .../advise_output_error_regression_test.go | 22 +++ cmd/pgbot/diff.go | 10 +- .../diff_output_error_regression_test.go | 52 +++++++ internal/render/advisor.go | 7 +- internal/render/diff.go | 18 ++- .../render/writer_errors_regression_test.go | 140 ++++++++++++++++++ 7 files changed, 252 insertions(+), 17 deletions(-) create mode 100644 cmd/pgbot/advise_output_error_regression_test.go create mode 100644 cmd/pgbot/diff_output_error_regression_test.go create mode 100644 internal/render/writer_errors_regression_test.go diff --git a/cmd/pgbot/advise.go b/cmd/pgbot/advise.go index 8c9ff21..d9c239c 100644 --- a/cmd/pgbot/advise.go +++ b/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 ", @@ -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,6 +107,10 @@ 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)") @@ -105,7 +118,7 @@ func runAdvise(cmd *cobra.Command, args []string, f adviseFlags) error { 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 diff --git a/cmd/pgbot/advise_output_error_regression_test.go b/cmd/pgbot/advise_output_error_regression_test.go new file mode 100644 index 0000000..6089b45 --- /dev/null +++ b/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) + } +} diff --git a/cmd/pgbot/diff.go b/cmd/pgbot/diff.go index 4a54076..9c6a893 100644 --- a/cmd/pgbot/diff.go +++ b/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,7 +110,7 @@ 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 @@ -117,13 +118,12 @@ func runDiff(f diffFlags) error { 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 diff --git a/cmd/pgbot/diff_output_error_regression_test.go b/cmd/pgbot/diff_output_error_regression_test.go new file mode 100644 index 0000000..beecbd8 --- /dev/null +++ b/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) + } +} diff --git a/internal/render/advisor.go b/internal/render/advisor.go index 0b61c71..00f2cee 100644 --- a/internal/render/advisor.go +++ b/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). diff --git a/internal/render/diff.go b/internal/render/diff.go index 29c3b8e..0145130 100644 --- a/internal/render/diff.go +++ b/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. diff --git a/internal/render/writer_errors_regression_test.go b/internal/render/writer_errors_regression_test.go new file mode 100644 index 0000000..fadd471 --- /dev/null +++ b/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()) + } +}