report fact records dropped from the estate table

catalog-impact built its PuppetDB client without a Warn hook, so a host whose
fact records all failed to decode vanished from the estate table with no
warning on any output path.

- wire catalog-impact's Warn hook into NodeWarnings
- default an unset Warn to stderr so a dropped record is never silent
- thread stderr through run() instead of writing os.Stderr directly
- count dropped fact records in the explain resolution-incomplete line

README.md

@@ -242,9 +242,10 @@ A missed host means an unvalidated catalog, so every ambiguity widens:
   `--explain`, in `unresolved_references` in `--json`, on stderr in plain mode,
   and set `summary.resolution_incomplete`.
 
-A fact record that arrives with no certname belongs to no host and cannot be
-folded into the estate table, so it is counted per fact and reported in
-`node_warnings`, in `--explain` and on stderr, and sets
+A fact record that arrives with no certname belongs to no host, and one whose
+value does not decode carries nothing to fold; either way it cannot enter the
+estate table, and a host whose records all fail leaves the table with them. Both
+are reported in `node_warnings`, in `--explain` and on stderr, and set
 `summary.resolution_incomplete` too.
 
 Paths outside the Puppet modulepath — `.woodpecker/`, `doc/`, `spec/`,

cmd/catalog-impact/main.go

@@ -67,7 +67,7 @@ func main() {
 		SilenceUsage:  true,
 		SilenceErrors: true,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return run(cmd.Context(), cmd.OutOrStdout(), opts)
+			return run(cmd.Context(), cmd.OutOrStdout(), cmd.ErrOrStderr(), opts)
 		},
 	}
 
@@ -89,7 +89,7 @@ func main() {
 	os.Exit(exitOK)
 }
 
-func run(ctx context.Context, out io.Writer, opts *options) error {
+func run(ctx context.Context, out, errOut io.Writer, opts *options) error {
 	if opts.jsonOut && opts.explain {
 		return fmt.Errorf("--json and --explain are mutually exclusive")
 	}
@@ -119,12 +119,19 @@ func run(ctx context.Context, out io.Writer, opts *options) error {
 		return fmt.Errorf("indexing %s: %w", opts.repo, err)
 	}
 
-	nodes, nodeWarnings, err := impact.LoadNodes(ctx, puppetdb.New(opts.puppetdb, opts.timeout))
+	// A dropped fact record can take a whole host out of the estate table, so
+	// it rides the same NodeWarnings channel as an unattributable one and
+	// reaches every output path.
+	facts := puppetdb.New(opts.puppetdb, opts.timeout)
+	var dropped []string
+	facts.Warn = func(msg string) { dropped = append(dropped, msg) }
+
+	nodes, nodeWarnings, err := impact.LoadNodes(ctx, facts)
 	if err != nil {
 		return err
 	}
 
-	analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: nodeWarnings}
+	analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: append(dropped, nodeWarnings...)}
 	result := analyzer.Analyze(opts.base, branch, files, opts.limit)
 
 	switch {
@@ -134,26 +141,26 @@ func run(ctx context.Context, out io.Writer, opts *options) error {
 		return impact.RenderExplain(out, result)
 	}
 
-	warn(result)
+	warn(errOut, result)
 	return impact.RenderPlain(out, result)
 }
 
 // warn surfaces the widenings and dead ends on stderr so a piped host list
 // stays clean.
-func warn(r *impact.Result) {
+func warn(w io.Writer, r *impact.Result) {
 	for _, file := range r.UnmappedFiles {
-		fmt.Fprintf(os.Stderr, "warning: %s matches no mapping rule; every host is included\n", file)
+		fmt.Fprintf(w, "warning: %s matches no mapping rule; every host is included\n", file)
 	}
 	for _, line := range impact.UnresolvedLines(r.UnresolvedRefs) {
-		fmt.Fprintf(os.Stderr, "warning: %s did not resolve to a class name; the host list may be incomplete\n", line)
+		fmt.Fprintf(w, "warning: %s did not resolve to a class name; the host list may be incomplete\n", line)
 	}
 	for _, line := range r.NodeWarnings {
-		fmt.Fprintf(os.Stderr, "warning: %s; the estate table may be missing hosts\n", line)
+		fmt.Fprintf(w, "warning: %s; the estate table may be missing hosts\n", line)
 	}
 	for _, class := range r.OrphanClasses {
-		fmt.Fprintf(os.Stderr, "warning: class %s is declared by nothing and listed in no hiera_include; it reaches no hosts\n", class)
+		fmt.Fprintf(w, "warning: class %s is declared by nothing and listed in no hiera_include; it reaches no hosts\n", class)
 	}
 	if r.Truncated {
-		fmt.Fprintf(os.Stderr, "warning: %d hosts affected, output capped at %d by --limit\n", r.Summary.Hosts, len(r.Hosts))
+		fmt.Fprintf(w, "warning: %d hosts affected, output capped at %d by --limit\n", r.Summary.Hosts, len(r.Hosts))
 	}
 }

cmd/catalog-impact/main_test.go

@@ -3,6 +3,8 @@ package main
 import (
 	"bytes"
 	"context"
+	"encoding/json"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -48,7 +50,7 @@ func TestRunWithExplicitFileList(t *testing.T) {
 		puppetdb: fakePuppetDB(t),
 		timeout:  5 * time.Second,
 	}
-	if err := run(context.Background(), &out, opts); err != nil {
+	if err := run(context.Background(), &out, io.Discard, opts); err != nil {
 		t.Fatalf("run: %v", err)
 	}
 	if got, want := out.String(), "web1.example.net\n"; got != want {
@@ -66,7 +68,7 @@ func TestRunExplain(t *testing.T) {
 		explain:  true,
 		timeout:  5 * time.Second,
 	}
-	if err := run(context.Background(), &out, opts); err != nil {
+	if err := run(context.Background(), &out, io.Discard, opts); err != nil {
 		t.Fatalf("run: %v", err)
 	}
 	if !strings.Contains(out.String(), "the whole estate is affected") {
@@ -76,14 +78,100 @@ func TestRunExplain(t *testing.T) {
 
 func TestRunRejectsConflictingOutputFlags(t *testing.T) {
 	opts := &options{jsonOut: true, explain: true}
-	if err := run(context.Background(), &bytes.Buffer{}, opts); err == nil {
+	if err := run(context.Background(), &bytes.Buffer{}, io.Discard, opts); err == nil {
 		t.Fatal("expected an error for --json with --explain")
 	}
 }
 
 func TestRunRejectsNegativeLimit(t *testing.T) {
 	opts := &options{limit: -1}
-	if err := run(context.Background(), &bytes.Buffer{}, opts); err == nil {
+	if err := run(context.Background(), &bytes.Buffer{}, io.Discard, opts); err == nil {
 		t.Fatal("expected an error for a negative --limit")
 	}
 }
+
+// undecodableEstateBody strips the value from every one of web1's records, so
+// each decodes to nothing and the host leaves the estate table with it.
+const undecodableEstateBody = `[
+  {"certname":"web1.example.net","name":"enc_role"},
+  {"certname":"web1.example.net","name":"country"},
+  {"certname":"web1.example.net","name":"os"},
+  {"certname":"k8s1.example.net","name":"enc_role","value":"roles::infra::k8s::node"},
+  {"certname":"k8s1.example.net","name":"country","value":"au"},
+  {"certname":"k8s1.example.net","name":"os","value":{"name":"AlmaLinux","release":{"major":"9","minor":"8"}}}
+]`
+
+func fakePuppetDBBody(t *testing.T, body string) string {
+	t.Helper()
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(body))
+	}))
+	t.Cleanup(srv.Close)
+	return srv.URL
+}
+
+func droppedFactOptions(t *testing.T) *options {
+	t.Helper()
+	return &options{
+		repo:     "../../internal/impact/testdata/repo",
+		base:     "origin/develop",
+		files:    []string{"modules/jellyfin/manifests/install.pp"},
+		puppetdb: fakePuppetDBBody(t, undecodableEstateBody),
+		timeout:  5 * time.Second,
+	}
+}
+
+func TestRunReportsDroppedFactRecordsOnStderr(t *testing.T) {
+	var out, errOut bytes.Buffer
+	if err := run(context.Background(), &out, &errOut, droppedFactOptions(t)); err != nil {
+		t.Fatalf("run: %v", err)
+	}
+	if strings.Contains(out.String(), "web1.example.net") {
+		t.Fatalf("web1 should have dropped out of the estate table, output = %q", out.String())
+	}
+	for _, want := range []string{"undecodable fact(s)", "the estate table may be missing hosts"} {
+		if !strings.Contains(errOut.String(), want) {
+			t.Errorf("stderr = %q, want it to mention %q", errOut.String(), want)
+		}
+	}
+}
+
+func TestRunReportsDroppedFactRecordsInJSON(t *testing.T) {
+	var out bytes.Buffer
+	opts := droppedFactOptions(t)
+	opts.jsonOut = true
+	if err := run(context.Background(), &out, io.Discard, opts); err != nil {
+		t.Fatalf("run: %v", err)
+	}
+
+	var doc struct {
+		NodeWarnings []string `json:"node_warnings"`
+		Summary      struct {
+			ResolutionIncomplete bool `json:"resolution_incomplete"`
+		} `json:"summary"`
+	}
+	if err := json.Unmarshal(out.Bytes(), &doc); err != nil {
+		t.Fatalf("decoding --json output: %v", err)
+	}
+	if len(doc.NodeWarnings) == 0 {
+		t.Error("node_warnings is empty; the dropped records went unreported")
+	}
+	if !doc.Summary.ResolutionIncomplete {
+		t.Error("resolution_incomplete is false after records were dropped")
+	}
+}
+
+func TestRunReportsDroppedFactRecordsInExplain(t *testing.T) {
+	var out bytes.Buffer
+	opts := droppedFactOptions(t)
+	opts.explain = true
+	if err := run(context.Background(), &out, io.Discard, opts); err != nil {
+		t.Fatalf("run: %v", err)
+	}
+	for _, want := range []string{"fact records dropped", "resolution incomplete", "undecodable fact(s)"} {
+		if !strings.Contains(out.String(), want) {
+			t.Errorf("explain output = %q, want it to mention %q", out.String(), want)
+		}
+	}
+}

internal/impact/render.go

@@ -63,8 +63,8 @@ func RenderExplain(w io.Writer, r *Result) error {
 		}
 	}
 	if r.Summary.ResolutionIncomplete {
-		if err := p("resolution incomplete: %d class reference(s) could not be resolved statically",
-			len(r.UnresolvedRefs)); err != nil {
+		if err := p("resolution incomplete: %d class reference(s) unresolved, %d fact record(s) dropped",
+			len(r.UnresolvedRefs), len(r.NodeWarnings)); err != nil {
 			return err
 		}
 	}

internal/puppetdb/puppetdb.go

@@ -12,6 +12,7 @@ import (
 	"io"
 	"net/http"
 	"net/url"
+	"os"
 	"strings"
 	"time"
 )
@@ -24,8 +25,9 @@ type Client struct {
 	BaseURL string
 	HTTP    *http.Client
 
-	// Warn, when set, receives a message for every fact record that could not
-	// be decoded and was therefore dropped.
+	// Warn receives a message for every fact record that could not be decoded
+	// and was therefore dropped. A nil Warn reports to stderr rather than
+	// discarding: a dropped record must never be silent.
 	Warn func(string)
 }
 
@@ -112,9 +114,9 @@ func (c *Client) FactsByName(ctx context.Context, names []string) ([]FactRecord,
 		}
 		out = append(out, FactRecord{Certname: r.Certname, Name: r.Name, Value: v})
 	}
-	if len(skipped) > 0 && c.Warn != nil {
-		c.Warn(fmt.Sprintf("skipped %d undecodable fact(s) in the estate fact table: %s",
-			len(skipped), strings.Join(skipped, ", ")))
+	if len(skipped) > 0 {
+		c.warn("skipped %d undecodable fact(s) in the estate fact table: %s",
+			len(skipped), strings.Join(skipped, ", "))
 	}
 	return out, nil
 }
@@ -244,13 +246,24 @@ func (c *Client) fold(certname string, records []factRecord) (map[string]any, er
 	if len(values) == 0 {
 		return nil, fmt.Errorf("no usable facts for %s: all %d records were undecodable", certname, len(records))
 	}
-	if len(skipped) > 0 && c.Warn != nil {
-		c.Warn(fmt.Sprintf("skipped %d undecodable fact(s) for %s: %s",
-			len(skipped), certname, strings.Join(skipped, ", ")))
+	if len(skipped) > 0 {
+		c.warn("skipped %d undecodable fact(s) for %s: %s",
+			len(skipped), certname, strings.Join(skipped, ", "))
 	}
 	return values, nil
 }
 
+// warn reports a dropped record. An unwired hook falls back to stderr so that
+// forgetting to set one loses visibility, never the record itself.
+func (c *Client) warn(format string, args ...any) {
+	msg := fmt.Sprintf(format, args...)
+	if c.Warn != nil {
+		c.Warn(msg)
+		return
+	}
+	fmt.Fprintln(os.Stderr, "warning:", msg)
+}
+
 func snippet(body []byte) string {
 	s := strings.TrimSpace(string(body))
 	if len(s) > 200 {

internal/puppetdb/puppetdb_test.go

@@ -4,9 +4,12 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"io"
 	"net/http"
 	"net/http/httptest"
+	"os"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 )
@@ -400,3 +403,46 @@ func TestFactsByNameSkipsUndecodableRecords(t *testing.T) {
 		t.Errorf("the skipped fact must be reported, got %v", warnings)
 	}
 }
+
+func TestFactsByNameWithoutWarnHookReportsOnStderr(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`[{"certname":"a.example.net","name":"broken"}]`))
+	}))
+	defer srv.Close()
+
+	stderr := captureStderr(t)
+	if _, err := New(srv.URL, 5*time.Second).FactsByName(context.Background(), []string{"broken"}); err != nil {
+		t.Fatalf("FactsByName: %v", err)
+	}
+	if got := stderr(); !strings.Contains(got, "broken") {
+		t.Errorf("an unwired Warn must still report the drop, stderr = %q", got)
+	}
+}
+
+// captureStderr redirects os.Stderr for the duration of the test and returns a
+// func that restores it and yields what was written.
+func captureStderr(t *testing.T) func() string {
+	t.Helper()
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatalf("pipe: %v", err)
+	}
+	saved := os.Stderr
+	os.Stderr = w
+
+	var once sync.Once
+	var out string
+	read := func() string {
+		once.Do(func() {
+			os.Stderr = saved
+			_ = w.Close()
+			b, _ := io.ReadAll(r)
+			_ = r.Close()
+			out = string(b)
+		})
+		return out
+	}
+	t.Cleanup(func() { read() })
+	return read
+}