report unresolved class references instead of resolving to silence

- walk the classes holding an unresolved reference when tracing any class
  it could have named, so a dropped edge cannot lose hosts
- list unresolved references in --explain, in unresolved_references in
  --json and on stderr, and flag summary.resolution_incomplete
- name an ordering reference as such in the --explain chain

AGENTS.md

@@ -71,6 +71,10 @@ Adding a binary: create `cmd/<name>/`. The Makefile, `scripts/build-rpm.sh`,
   an exact one. `site/roles/manifests/.../<t3>.pp` is exact.
 - The bias is to over-include: an unmapped path widens to the whole estate and
   says so on stderr. Never turn a widening into a silent empty result.
+- The same applies in the parser. A class reference it cannot reduce to literal
+  names becomes an `UnresolvedRef` against its file, and every class that
+  reference could have named is treated as reaching the classes the file
+  declares. Never drop a reference the parser failed on.
 - The repo index comes from the `--repo` checkout, not from `--branch`. CI runs
   on the branch checkout, so the two agree there.
 

README.md

@@ -203,7 +203,10 @@ Finding what applies a class means searching two places, not one:
 
 1. `.pp` files under `site/` and `modules/` for `include`, `contain`,
    `inherits`, `class { '...': }`, capitalised resource references,
-   `create_resources()`, `$class::var` reads and defined-type usage.
+   `create_resources()`, `$class::var` reads and defined-type usage. Comments
+   are stripped with quote tracking and heredoc bodies are skipped, so a `#` in
+   a string never truncates a line and documentation in a heredoc never becomes
+   an edge.
 2. `hieradata/**/*.{yaml,eyaml}` for the class as a list item under
    `hiera_include:`. `profiles::base` declares
    `($hiera_include - $hiera_exclude).include` with a `unique` merge across the
@@ -226,7 +229,15 @@ A missed host means an unvalidated catalog, so every ambiguity widens:
 
 - a path matching no rule is reported on stderr and mapped to every host;
 - an asset no manifest references widens to its whole module;
-- an unrecognised hieradata path in a `hiera_include` hit widens to every host.
+- an unrecognised hieradata path in a `hiera_include` hit widens to every host;
+- a class reference that is not a literal name is reported as unresolved and
+  widened. A selector yields every literal name in its arms, and a bare
+  variable is followed through the assignments in its own file. What is left —
+  `include "profile::role_${role}"`, `include lookup('x')` — is recorded
+  against its file, and every class the reference could have named is treated
+  as reaching the classes that file declares. The references appear in
+  `--explain`, in `unresolved_references` in `--json`, on stderr in plain mode,
+  and set `summary.resolution_incomplete`.
 
 Paths outside the Puppet modulepath — `.woodpecker/`, `doc/`, `spec/`,
 `vendor/`, `Gemfile`, `*.md` — cannot reach a catalog and are skipped.

cmd/catalog-impact/main.go

@@ -34,8 +34,9 @@ traced backwards — through include, contain and class {} in manifests, and
 through hiera_include in hieradata — until only role classes and hierarchy
 positions remain.
 
-The bias is to over-include: an unrecognised path widens to the whole estate
-rather than silently returning nothing.`
+The bias is to over-include: an unrecognised path widens to the whole estate,
+and a class reference that is not a literal name — a selector, an interpolated
+name, a variable — is reported rather than silently dropped.`
 
 type options struct {
 	repo    string
@@ -143,6 +144,9 @@ func warn(r *impact.Result) {
 	for _, file := range r.UnmappedFiles {
 		fmt.Fprintf(os.Stderr, "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)
+	}
 	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)
 	}

internal/impact/impact.go

@@ -23,6 +23,9 @@ type Summary struct {
 	Hosts             int  `json:"hosts"`
 	Files             int  `json:"files"`
 	AllHostsTriggered bool `json:"all_hosts_triggered"`
+	// ResolutionIncomplete is set when a class reference could not be resolved
+	// statically, so the answer is a best effort rather than exact.
+	ResolutionIncomplete bool `json:"resolution_incomplete"`
 }
 
 // Result is the whole analysis, and the --json document.
@@ -32,7 +35,10 @@ type Result struct {
 	ChangedFiles  []string `json:"changed_files"`
 	Hosts         []Host   `json:"hosts"`
 	UnmappedFiles []string `json:"unmapped_files"`
-	Summary       Summary  `json:"summary"`
+	// UnresolvedRefs are class references the parser could not reduce to
+	// names, either in a changed file or on the path from one to a host.
+	UnresolvedRefs []UnresolvedRef `json:"unresolved_references"`
+	Summary        Summary         `json:"summary"`
 
 	// IgnoredFiles are changed files outside the modulepath — CI, docs,
 	// specs — which cannot reach a catalog.
@@ -72,6 +78,7 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu
 
 	reasons := map[string][]Reason{}
 	orphans := map[string]bool{}
+	var unresolved []UnresolvedRef
 	// hitFiles records which changed files put at least one host in the result.
 	hitFiles := map[string]bool{}
 	result := &Result{
@@ -102,8 +109,11 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu
 			}
 		}
 
+		unresolved = append(unresolved, a.Index.UnresolvedIn(file)...)
+
 		for _, ref := range m.classes {
 			res := resolver.resolve(ref.name)
+			unresolved = append(unresolved, res.unresolved...)
 			if res.orphan {
 				orphans[ref.name] = true
 				continue
@@ -132,6 +142,8 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu
 	}
 	sort.Strings(result.OrphanClasses)
 
+	result.UnresolvedRefs = sortUnresolved(dedupeUnresolved(unresolved))
+
 	certnames := make([]string, 0, len(reasons))
 	for certname := range reasons {
 		certnames = append(certnames, certname)
@@ -139,9 +151,10 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu
 	sort.Strings(certnames)
 
 	result.Summary = Summary{
-		Hosts:             len(certnames),
-		Files:             len(result.ChangedFiles),
-		AllHostsTriggered: len(certnames) > 0 && len(certnames) == len(a.Nodes),
+		Hosts:                len(certnames),
+		Files:                len(result.ChangedFiles),
+		AllHostsTriggered:    len(certnames) > 0 && len(certnames) == len(a.Nodes),
+		ResolutionIncomplete: len(result.UnresolvedRefs) > 0,
 	}
 
 	if limit > 0 && len(certnames) > limit {
@@ -176,3 +189,16 @@ func dedupeReasons(in []Reason) []Reason {
 	}
 	return out
 }
+
+func sortUnresolved(in []UnresolvedRef) []UnresolvedRef {
+	sort.Slice(in, func(i, j int) bool {
+		if in[i].File != in[j].File {
+			return in[i].File < in[j].File
+		}
+		if in[i].Form != in[j].Form {
+			return in[i].Form < in[j].Form
+		}
+		return in[i].Expr < in[j].Expr
+	})
+	return in
+}

internal/impact/render.go

@@ -29,6 +29,9 @@ func RenderJSON(w io.Writer, r *Result) error {
 	if r.UnmappedFiles == nil {
 		r.UnmappedFiles = []string{}
 	}
+	if r.UnresolvedRefs == nil {
+		r.UnresolvedRefs = []UnresolvedRef{}
+	}
 	return enc.Encode(r)
 }
 
@@ -56,6 +59,12 @@ func RenderExplain(w io.Writer, r *Result) error {
 			return err
 		}
 	}
+	if r.Summary.ResolutionIncomplete {
+		if err := p("resolution incomplete: %d class reference(s) could not be resolved statically",
+			len(r.UnresolvedRefs)); err != nil {
+			return err
+		}
+	}
 
 	if err := section(w, "unmapped files (widened to all hosts)", r.UnmappedFiles); err != nil {
 		return err
@@ -66,6 +75,10 @@ func RenderExplain(w io.Writer, r *Result) error {
 	if err := section(w, "changed files that reach no host (the role they belong to has no members)", r.EmptyFiles); err != nil {
 		return err
 	}
+	if err := section(w, "unresolved class references (the host list may be incomplete)",
+		UnresolvedLines(r.UnresolvedRefs)); err != nil {
+		return err
+	}
 	if err := section(w, "orphan classes (nothing declares them and no hiera_include lists them)", r.OrphanClasses); err != nil {
 		return err
 	}
@@ -103,3 +116,17 @@ func section(w io.Writer, title string, items []string) error {
 	}
 	return nil
 }
+
+// UnresolvedLines renders unresolved references one per line for the report
+// and for the stderr warnings.
+func UnresolvedLines(refs []UnresolvedRef) []string {
+	out := make([]string, 0, len(refs))
+	for _, u := range refs {
+		line := fmt.Sprintf("%s: %s %s", u.File, u.Form, u.Expr)
+		if u.Prefix != "" {
+			line += fmt.Sprintf(" (names a class under %s)", u.Prefix)
+		}
+		out = append(out, line)
+	}
+	return out
+}

internal/impact/resolve.go

@@ -22,6 +22,8 @@ type classResolver struct {
 type resolution struct {
 	hosts  map[string][]hit
 	orphan bool
+	// unresolved are the references whose incompleteness widened this walk.
+	unresolved []UnresolvedRef
 }
 
 func newClassResolver(idx *Index, nodes []Node) *classResolver {
@@ -71,12 +73,28 @@ func (r *classResolver) resolve(class string) *resolution {
 				continue
 			}
 			seen[parent] = true
-			via[parent] = via[current] + " <- " + parent
+			via[parent] = via[current] + " <- " + parent + r.idx.EdgeNote(current, parent)
 			queue = append(queue, parent)
 		}
+
+		// A reference the parser could not name might have been this class, so
+		// the classes holding it are walked too rather than dropped.
+		for _, u := range r.idx.UnresolvedIncluders(current) {
+			for _, parent := range u.Classes {
+				if seen[parent] {
+					continue
+				}
+				seen[parent] = true
+				via[parent] = via[current] + " <- " + parent +
+					fmtDetail(" (unresolved %s %s in %s)", u.Ref.Form, u.Ref.Expr, u.Ref.File)
+				queue = append(queue, parent)
+			}
+			res.unresolved = append(res.unresolved, u.Ref)
+		}
 	}
 
 	res.orphan = !reached
+	res.unresolved = dedupeUnresolved(res.unresolved)
 	return res
 }
 

internal/impact/unresolved_test.go

@@ -0,0 +1,166 @@
+package impact
+
+import (
+	"bytes"
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"reflect"
+	"strings"
+	"testing"
+)
+
+// tempRepo writes a throwaway control repo from a path -> content map.
+func tempRepo(t *testing.T, files map[string]string) string {
+	t.Helper()
+	root := t.TempDir()
+	for rel, content := range files {
+		full := filepath.Join(root, filepath.FromSlash(rel))
+		if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(full, []byte(content), 0o600); err != nil {
+			t.Fatal(err)
+		}
+	}
+	return root
+}
+
+func buildIndex(t *testing.T, files map[string]string) *Index {
+	t.Helper()
+	idx, err := BuildIndex(tempRepo(t, files))
+	if err != nil {
+		t.Fatalf("BuildIndex: %v", err)
+	}
+	return idx
+}
+
+func TestIndexRecordsUnresolvedReferenceAgainstItsFile(t *testing.T) {
+	idx := testIndex(t)
+	got := idx.UnresolvedIn("site/profiles/manifests/dynamic.pp")
+	want := []UnresolvedRef{{
+		File:   "site/profiles/manifests/dynamic.pp",
+		Form:   "include",
+		Expr:   `"profiles::dyn_${facts['role']}"`,
+		Prefix: "profiles::dyn_",
+	}}
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("UnresolvedIn = %#v, want %#v", got, want)
+	}
+}
+
+func TestResolveWidensThroughAnUnresolvedPrefix(t *testing.T) {
+	// profiles::dynamic includes "profiles::dyn_${...}". Any class under that
+	// prefix must reach whatever profiles::dynamic reaches.
+	if got, want := resolvedHosts(t, "profiles::dyn_web"), []string{"k8s1", "k8s2", "web1"}; !reflect.DeepEqual(got, want) {
+		t.Errorf("profiles::dyn_web = %v, want %v", got, want)
+	}
+	// A class outside the prefix is not dragged in by it.
+	res := newClassResolver(testIndex(t), fixtureNodes()).resolve("profiles::unrelated_thing")
+	if !res.orphan {
+		t.Errorf("an unrelated class was widened to %v", res.hosts)
+	}
+}
+
+func TestResolveWidensThroughAnUnresolvedVariable(t *testing.T) {
+	// With no literal prefix to match on, every class is a candidate, so the
+	// holding class's hosts are included rather than dropped.
+	idx := buildIndex(t, map[string]string{
+		"site/roles/manifests/base.pp":          "class roles::base {\n  include profiles::base\n}\n",
+		"site/profiles/manifests/base.pp":       "class profiles::base {\n  include profiles::wild\n}\n",
+		"site/profiles/manifests/wild.pp":       "class profiles::wild {\n  $c = lookup('profile')\n  include $c\n}\n",
+		"site/profiles/manifests/standalone.pp": "class profiles::standalone {\n}\n",
+	})
+	res := newClassResolver(idx, fixtureNodes()).resolve("profiles::standalone")
+	if got, want := uniqueSorted(hostNames(res)), []string{"bare1"}; !reflect.DeepEqual(got, want) {
+		t.Errorf("hosts = %v, want %v", got, want)
+	}
+	if len(res.unresolved) != 1 || res.unresolved[0].File != "site/profiles/manifests/wild.pp" {
+		t.Errorf("resolution did not report the unresolved reference: %#v", res.unresolved)
+	}
+}
+
+func hostNames(res *resolution) []string {
+	out := make([]string, 0, len(res.hosts))
+	for certname := range res.hosts {
+		out = append(out, certname)
+	}
+	return out
+}
+
+func TestAnalyzeReportsUnresolvedReferencesInAChangedFile(t *testing.T) {
+	r := analyze(t, "site/profiles/manifests/dynamic.pp")
+	if !r.Summary.ResolutionIncomplete {
+		t.Error("summary did not flag the incomplete resolution")
+	}
+	if len(r.UnresolvedRefs) != 1 || r.UnresolvedRefs[0].Prefix != "profiles::dyn_" {
+		t.Fatalf("UnresolvedRefs = %#v", r.UnresolvedRefs)
+	}
+
+	var explain bytes.Buffer
+	if err := RenderExplain(&explain, r); err != nil {
+		t.Fatalf("RenderExplain: %v", err)
+	}
+	for _, want := range []string{"resolution incomplete", "unresolved class references",
+		"site/profiles/manifests/dynamic.pp", "profiles::dyn_"} {
+		if !strings.Contains(explain.String(), want) {
+			t.Errorf("explain output missing %q:\n%s", want, explain.String())
+		}
+	}
+
+	var out bytes.Buffer
+	if err := RenderJSON(&out, r); err != nil {
+		t.Fatalf("RenderJSON: %v", err)
+	}
+	var doc struct {
+		UnresolvedRefs []UnresolvedRef `json:"unresolved_references"`
+		Summary        struct {
+			ResolutionIncomplete bool `json:"resolution_incomplete"`
+		} `json:"summary"`
+	}
+	if err := json.Unmarshal(out.Bytes(), &doc); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+	if !doc.Summary.ResolutionIncomplete || len(doc.UnresolvedRefs) != 1 {
+		t.Errorf("json = %s", out.String())
+	}
+}
+
+func TestAnalyzeReportsNoUnresolvedReferencesForACleanFile(t *testing.T) {
+	r := analyze(t, "site/profiles/manifests/leaf.pp")
+	if r.Summary.ResolutionIncomplete || len(r.UnresolvedRefs) != 0 {
+		t.Errorf("clean file reported %#v", r.UnresolvedRefs)
+	}
+}
+
+func TestExplainCallsAnOrderingReferenceWhatItIs(t *testing.T) {
+	idx := buildIndex(t, map[string]string{
+		"site/roles/manifests/base.pp":       "class roles::base {\n  include profiles::base\n}\n",
+		"site/profiles/manifests/base.pp":    "class profiles::base {\n  service { 's': require => Class['profiles::ordered'] }\n}\n",
+		"site/profiles/manifests/ordered.pp": "class profiles::ordered {\n}\n",
+	})
+	res := newClassResolver(idx, fixtureNodes()).resolve("profiles::ordered")
+	hits := res.hosts["bare1"]
+	if len(hits) == 0 {
+		t.Fatalf("profiles::ordered reached no host: %#v", res.hosts)
+	}
+	if !strings.Contains(hits[0].detail, "not a declaration") {
+		t.Errorf("detail = %q, want it to mark the ordering reference", hits[0].detail)
+	}
+}
+
+func TestExplainLeavesADeclarationUnannotated(t *testing.T) {
+	idx := buildIndex(t, map[string]string{
+		"site/roles/manifests/base.pp":       "class roles::base {\n  include profiles::base\n}\n",
+		"site/profiles/manifests/base.pp":    "class profiles::base {\n  include profiles::ordered\n}\n",
+		"site/profiles/manifests/ordered.pp": "class profiles::ordered {\n}\n",
+	})
+	res := newClassResolver(idx, fixtureNodes()).resolve("profiles::ordered")
+	hits := res.hosts["bare1"]
+	if len(hits) == 0 {
+		t.Fatalf("profiles::ordered reached no host: %#v", res.hosts)
+	}
+	if strings.Contains(hits[0].detail, "not a declaration") {
+		t.Errorf("detail = %q, an include is a declaration", hits[0].detail)
+	}
+}