From c4d3ed24a92e1b5e93d0b2de8d4978af14f710c5 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 17:19:02 +1000 Subject: [PATCH] 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 | 4 + README.md | 15 ++- cmd/catalog-impact/main.go | 8 +- internal/impact/impact.go | 34 +++++- internal/impact/render.go | 27 +++++ internal/impact/resolve.go | 20 +++- internal/impact/unresolved_test.go | 166 +++++++++++++++++++++++++++++ 7 files changed, 265 insertions(+), 9 deletions(-) create mode 100644 internal/impact/unresolved_test.go diff --git a/AGENTS.md b/AGENTS.md index 0fa72fa..c8f02e7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,6 +71,10 @@ Adding a binary: create `cmd//`. The Makefile, `scripts/build-rpm.sh`, an exact one. `site/roles/manifests/.../.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. diff --git a/README.md b/README.md index ae02b83..2dd05de 100644 --- a/README.md +++ b/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. diff --git a/cmd/catalog-impact/main.go b/cmd/catalog-impact/main.go index 0805844..d51cfa2 100644 --- a/cmd/catalog-impact/main.go +++ b/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) } diff --git a/internal/impact/impact.go b/internal/impact/impact.go index 3f79272..7e243d3 100644 --- a/internal/impact/impact.go +++ b/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 +} diff --git a/internal/impact/render.go b/internal/impact/render.go index eac1a12..3bdc7d5 100644 --- a/internal/impact/render.go +++ b/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 +} diff --git a/internal/impact/resolve.go b/internal/impact/resolve.go index b460659..aa433ca 100644 --- a/internal/impact/resolve.go +++ b/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 } diff --git a/internal/impact/unresolved_test.go b/internal/impact/unresolved_test.go new file mode 100644 index 0000000..d2d898e --- /dev/null +++ b/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) + } +}