unwrap the catalog response and classify failures correctly

- unwrap the {"catalog": ...} envelope, still accepting a bare catalog
- report an environment-less response as a protocol mismatch, not an ENC problem
- make Resource.Line a pointer and Resource.Params() map absent onto empty
- render a missing manifest location as "(source unknown)"
- retry compile failures caused by the compiler's PuppetDB query
- stop retrying manifest errors the compiler returns as HTTP 500
- add a trimmed real-server response fixture and tests for each shape

AGENTS.md

@@ -40,9 +40,20 @@ Makefile and to `scripts/build-rpm.sh`, and add its files to
 
 ## Gotchas
 
+- The compiler wraps its response as `{"catalog": {...}}`. `decodeCatalog`
+  unwraps it and still accepts a bare catalog; decoding straight into
+  `catalog.Catalog` silently yields an empty one.
+- Only `type`, `title`, `tags` and `exported` are present on every resource.
+  `parameters` is absent on class/stage/anchor resources and `file`/`line` on
+  resources with no manifest location, so `Resource.Line` is a pointer and
+  `Resource.Params()` maps absent onto empty.
 - An ENC-supplied environment outranks the requested one. Every compile checks
   the returned catalog's `environment` and hard-fails on a mismatch — never
-  relax that check.
+  relax that check. A returned environment that is empty is a decode failure,
+  not a mismatch, and is reported as such.
+- The compiler returns HTTP 500 for manifest errors too, so retryability is
+  decided from the error text: a failed PuppetDB query (`server_urls`,
+  `/pdb/query/`) is transient, a manifest error never is.
 - Branch names are converted the way g10k does it (`\W` -> `_`, case
   preserved). The rule lives in one function, `catalog.BranchToEnvironment`.
 - Exit codes are a contract: 0 same, 1 compile failed, 2 differences, 3
@@ -62,3 +73,8 @@ Everything is stubbed with `httptest` (a TLS server for puppetserver, a plain
 one for pdbmux) — no live estate access is required. Renderer output is
 covered by golden files under `internal/catalog/testdata`; regenerate with
 `go test ./internal/catalog -update`.
+
+`internal/puppetserver/testdata/catalog_response.json` is a trimmed capture of
+a real OpenVoxServer 8.8 response, kept for its shape: the `catalog` envelope
+and resources that omit `parameters`, `file` and `line`. Sensitive values are
+replaced.

README.md

@@ -36,7 +36,7 @@ catalog-diff web01.example.net --branch benvin/motd-change --testonly
 | `--pdb` | pdbmux base URL (default `https://pdbmux.k8s.syd1.au.unkin.net`) |
 | `--cert` / `--key` / `--cacert` | client mTLS material |
 | `--timeout` | per-compile timeout (default 10m) |
-| `--retry` | retries for transient compile or transport failures (default 2) |
+| `--retry` | retries for transient compile or transport failures (default 2; a failed compiler-side PuppetDB query counts, a manifest error does not) |
 | `--include-classes` | diff `Class` resources (ignored by default; they manage nothing) |
 | `--ignore` | ignore resources matching `Type[Title]` (glob), repeatable |
 | `--ignore-attr` | ignore a parameter on every resource, repeatable |
@@ -101,8 +101,11 @@ or pass `--environment`. `--environment` bypasses the conversion entirely.
 Classification runs through an exec ENC (`encapic` -> `encapi`), and an
 ENC-supplied environment outranks the requested one. Every request sets
 `options.prefer_requested_environment`, and the returned catalog's own
-`environment` is always compared against the one requested. A mismatch is a
-hard error (exit 3) rather than a silent diff of an environment against itself.
+`environment` is always compared against the one requested. A mismatch between
+two named environments is a hard error (exit 3) rather than a silent diff of an
+environment against itself. A response that decodes without an environment name
+at all is reported as a protocol mismatch instead, because that is a client/
+server problem rather than a classification one.
 
 ## What a catalog diff cannot see
 

cmd/catalog-diff/main.go

@@ -325,16 +325,22 @@ func compile(ctx context.Context, server *puppetserver.Client, hostname, environ
 	return nil, &exitError{code: exitTransport, err: lastErr}
 }
 
-// transient reports whether a failure is worth retrying: a transport error, or
-// a server-side 5xx. A manifest that does not compile is not retried.
+// transient reports whether a failure is worth retrying: a transport error, a
+// server-side 5xx, or a compile the compiler's own PuppetDB query broke. A
+// manifest that does not compile, an environment mismatch and an unusable
+// response shape are all settled answers and are not retried.
 func transient(err error) bool {
 	var mismatch *puppetserver.EnvironmentMismatchError
 	if errors.As(err, &mismatch) {
 		return false
 	}
+	var respErr *puppetserver.ResponseError
+	if errors.As(err, &respErr) {
+		return false
+	}
 	var compileErr *puppetserver.CompileError
 	if errors.As(err, &compileErr) {
-		return compileErr.Status >= 500
+		return compileErr.Transient()
 	}
 	return true
 }

cmd/catalog-diff/main_test.go

@@ -345,3 +345,116 @@ func TestRunIgnoreFlags(t *testing.T) {
 		t.Errorf("--ignore not applied:\n%s", out.String())
 	}
 }
+
+// wrappedCatalogFor is the shape OpenVoxServer 8.8 actually returns: the
+// catalog under a single "catalog" key.
+func wrappedCatalogFor(environment, content string) string {
+	return `{"catalog":` + catalogFor(environment, content) + `}`
+}
+
+func TestRunAgainstWrappedResponses(t *testing.T) {
+	opts := testServers(t, func(w http.ResponseWriter, r *http.Request) {
+		if readEnvironment(t, r) == "develop" {
+			_, _ = w.Write([]byte(wrappedCatalogFor("develop", "old")))
+			return
+		}
+		_, _ = w.Write([]byte(wrappedCatalogFor("benvin_test", "new")))
+	})
+	opts.environment = "benvin_test"
+
+	var out bytes.Buffer
+	err := run(context.Background(), &out, "host.example.net", opts)
+	if code := exitCode(t, err); code != exitDifferent {
+		t.Fatalf("exit code %d, want %d (err %v)", code, exitDifferent, err)
+	}
+	if !strings.Contains(out.String(), "File[/etc/motd] content: old -> new") {
+		t.Errorf("wrapped response not diffed:\n%s", out.String())
+	}
+}
+
+func TestRunUnknownResponseShapeDoesNotBlameTheENC(t *testing.T) {
+	var attempts int
+	var mu sync.Mutex
+	opts := testServers(t, func(w http.ResponseWriter, _ *http.Request) {
+		mu.Lock()
+		attempts++
+		mu.Unlock()
+		_, _ = w.Write([]byte(`{"unexpected":{"environment":"benvin_test"}}`))
+	})
+	opts.environment = "benvin_test"
+	opts.testonly = true
+	opts.retry = 3
+
+	err := run(context.Background(), &bytes.Buffer{}, "host.example.net", opts)
+	if code := exitCode(t, err); code != exitTransport {
+		t.Fatalf("exit %d, want %d (err %v)", code, exitTransport, err)
+	}
+	if strings.Contains(err.Error(), "encapi") || strings.Contains(err.Error(), "environment mismatch") {
+		t.Errorf("a decode failure must not be reported as an ENC problem: %v", err)
+	}
+	mu.Lock()
+	defer mu.Unlock()
+	if attempts != 1 {
+		t.Errorf("an unusable response shape must not be retried, saw %d attempts", attempts)
+	}
+}
+
+const pdbFailureBody = "Internal Server Error: org.jruby.exceptions.RuntimeError: (PreformattedError) Evaluation Error: " +
+	"Error while evaluating a Resource Statement, Evaluation Error: Error while evaluating a Function Call, " +
+	"Failed to execute '/pdb/query/v4' on at least 1 of the following 'server_urls': https://puppetdb.example.net " +
+	"(file: /etc/puppetlabs/code/environments/develop/site/profiles/manifests/consul/client.pp, line: 31, column: 29) " +
+	"on node host.example.net"
+
+func TestRunRetriesPuppetDBQueryFailures(t *testing.T) {
+	var attempts int
+	var mu sync.Mutex
+	opts := testServers(t, func(w http.ResponseWriter, r *http.Request) {
+		mu.Lock()
+		attempts++
+		first := attempts == 1
+		mu.Unlock()
+		if first {
+			w.WriteHeader(http.StatusInternalServerError)
+			_, _ = w.Write([]byte(pdbFailureBody))
+			return
+		}
+		_, _ = w.Write([]byte(wrappedCatalogFor(readEnvironment(t, r), "same")))
+	})
+	opts.environment = "benvin_test"
+	opts.testonly = true
+	opts.retry = 2
+
+	if err := run(context.Background(), &bytes.Buffer{}, "host.example.net", opts); err != nil {
+		t.Fatalf("a failed PuppetDB query should have been retried: %v", err)
+	}
+	mu.Lock()
+	defer mu.Unlock()
+	if attempts != 2 {
+		t.Errorf("expected one retry, saw %d compile attempts", attempts)
+	}
+}
+
+func TestRunDoesNotRetryManifestErrorsReturnedAs500(t *testing.T) {
+	var attempts int
+	var mu sync.Mutex
+	opts := testServers(t, func(w http.ResponseWriter, _ *http.Request) {
+		mu.Lock()
+		attempts++
+		mu.Unlock()
+		w.WriteHeader(http.StatusInternalServerError)
+		_, _ = w.Write([]byte("Internal Server Error: org.jruby.exceptions.RuntimeError: (Error) " +
+			"Could not find class roles::infra::incus::node for host.example.net on node host.example.net"))
+	})
+	opts.environment = "benvin_test"
+	opts.testonly = true
+	opts.retry = 3
+
+	if code := exitCode(t, run(context.Background(), &bytes.Buffer{}, "host.example.net", opts)); code != exitCompile {
+		t.Fatalf("exit %d, want %d", code, exitCompile)
+	}
+	mu.Lock()
+	defer mu.Unlock()
+	if attempts != 1 {
+		t.Errorf("a manifest error must not be retried even at HTTP 500, saw %d attempts", attempts)
+	}
+}

internal/catalog/catalog.go

@@ -24,7 +24,10 @@ type Catalog struct {
 	Resources           []Resource      `json:"resources"`
 }
 
-// Resource is a single catalog resource.
+// Resource is a single catalog resource. Only type, title, tags and exported
+// are guaranteed: the compiler omits parameters on class, stage and anchor
+// resources, and omits file and line on resources with no manifest location.
+// Line is a pointer so an omitted line stays distinguishable from line 0.
 type Resource struct {
 	Type                string         `json:"type"`
 	Title               string         `json:"title"`
@@ -32,13 +35,25 @@ type Resource struct {
 	Tags                []string       `json:"tags"`
 	Exported            bool           `json:"exported"`
 	File                string         `json:"file"`
-	Line                int            `json:"line"`
+	Line                *int           `json:"line"`
 	SensitiveParameters []string       `json:"sensitive_parameters"`
 }
 
 // Key is the "Type[Title]" identity used to match resources across catalogs.
 func (r Resource) Key() string { return r.Type + "[" + r.Title + "]" }
 
+// Params returns the resource's parameters, mapping an omitted "parameters"
+// key onto an empty set so absent and empty compare and render identically.
+func (r Resource) Params() map[string]any {
+	if r.Parameters == nil {
+		return map[string]any{}
+	}
+	return r.Parameters
+}
+
+// HasSource reports whether the compiler supplied a manifest location.
+func (r Resource) HasSource() bool { return r.File != "" && r.Line != nil }
+
 // Options controls normalisation. The zero value is the default behaviour:
 // classes ignored, exported resources skipped, ordering-only metaparameters
 // dropped.
@@ -102,7 +117,7 @@ func normalizeTitle(r Resource) string {
 	if r.Type != "File" {
 		return r.Title
 	}
-	if p, ok := r.Parameters["path"].(string); ok && p != "" {
+	if p, ok := r.Params()["path"].(string); ok && p != "" {
 		return p
 	}
 	return r.Title
@@ -119,7 +134,7 @@ func cleanParameters(r Resource, catalogSensitive, ignoreAttrs []string) map[str
 	}
 
 	params := make(map[string]any, len(r.Parameters))
-	for k, v := range r.Parameters {
+	for k, v := range r.Params() {
 		if noiseParameters[k] || ignored[k] || strings.HasPrefix(k, "_") {
 			continue
 		}

internal/catalog/diff.go

@@ -57,9 +57,9 @@ type Change struct {
 	OldValue  any      `json:"old_value"`
 	NewValue  any      `json:"new_value"`
 	OldFile   string   `json:"old_file,omitempty"`
-	OldLine   int      `json:"old_line,omitempty"`
+	OldLine   *int     `json:"old_line,omitempty"`
 	NewFile   string   `json:"new_file,omitempty"`
-	NewLine   int      `json:"new_line,omitempty"`
+	NewLine   *int     `json:"new_line,omitempty"`
 	Severity  string   `json:"severity"`
 	// Datatype marks a change where only the datatype differs (28 vs "28").
 	Datatype bool `json:"datatype,omitempty"`
@@ -99,7 +99,7 @@ func Diff(base, test map[string]Resource, opts DiffOptions) []Change {
 			DiffType: DiffRemoved,
 			Type:     baseRes.Type,
 			Title:    baseRes.Title,
-			OldValue: baseRes.Parameters,
+			OldValue: baseRes.Params(),
 			OldFile:  baseRes.File,
 			OldLine:  baseRes.Line,
 			Severity: Severity(baseRes.Type),
@@ -114,7 +114,7 @@ func Diff(base, test map[string]Resource, opts DiffOptions) []Change {
 			DiffType: DiffAdded,
 			Type:     testRes.Type,
 			Title:    testRes.Title,
-			NewValue: testRes.Parameters,
+			NewValue: testRes.Params(),
 			NewFile:  testRes.File,
 			NewLine:  testRes.Line,
 			Severity: Severity(testRes.Type),
@@ -161,7 +161,7 @@ func diffParameters(baseRes, testRes Resource, opts DiffOptions) []Change {
 		Severity: Severity(testRes.Type),
 	}
 	return walk(proto, []string{"parameters"},
-		anyMap(baseRes.Parameters), anyMap(testRes.Parameters), opts)
+		anyMap(baseRes.Params()), anyMap(testRes.Params()), opts)
 }
 
 func anyMap(m map[string]any) any {

internal/catalog/diff_test.go

@@ -60,6 +60,8 @@ func changeSet(t *testing.T) []Change {
 	)
 }
 
+func intPtr(n int) *int { return &n }
+
 func byKey(changes []Change) map[string][]Change {
 	out := map[string][]Change{}
 	for _, c := range changes {
@@ -82,7 +84,7 @@ func TestNormalizeDropsNoiseParameters(t *testing.T) {
 	if motd.Parameters["content"] != "new" {
 		t.Errorf("real parameters dropped: %+v", motd.Parameters)
 	}
-	if motd.File != "site.pp" || motd.Line != 19 {
+	if motd.File != "site.pp" || motd.Line == nil || *motd.Line != 19 {
 		t.Errorf("file/line context lost: %+v", motd)
 	}
 	if got := motd.Tags; len(got) != 3 || got[0] != "a" || got[2] != "z" {
@@ -228,9 +230,9 @@ func TestDiffSuppressesDatatypeChanges(t *testing.T) {
 }
 
 func TestDiffIgnoresFileLineMoves(t *testing.T) {
-	base := map[string]Resource{"File[/a]": {Type: "File", Title: "/a", File: "a.pp", Line: 1,
+	base := map[string]Resource{"File[/a]": {Type: "File", Title: "/a", File: "a.pp", Line: intPtr(1),
 		Parameters: map[string]any{"ensure": "file"}}}
-	test := map[string]Resource{"File[/a]": {Type: "File", Title: "/a", File: "b.pp", Line: 99,
+	test := map[string]Resource{"File[/a]": {Type: "File", Title: "/a", File: "b.pp", Line: intPtr(99),
 		Parameters: map[string]any{"ensure": "file"}}}
 	if changes := Diff(base, test, DiffOptions{}); len(changes) != 0 {
 		t.Errorf("a resource moving file/line is not a change: %+v", changes)
@@ -306,3 +308,81 @@ func TestDiffNoChanges(t *testing.T) {
 		t.Errorf("identical catalogs produced changes: %+v", changes)
 	}
 }
+
+// Real resources from a live OpenVoxServer 8.8 catalog. Class, stage and
+// anchor resources carry no "parameters" key, and resources with no manifest
+// location carry neither "file" nor "line".
+const optionalFieldCatalog = `{
+  "name":"prodnxsr0009.main.unkin.net","environment":"develop","version":"979c188",
+  "resources":[
+    {"type":"Class","title":"Settings","tags":["class","settings"],"exported":false,"kind":"unknown"},
+    {"type":"Anchor","title":"selinux::end","tags":["anchor","selinux::end"],"file":"init.pp","line":111,"exported":false,"kind":"compilable_type"},
+    {"type":"File","title":"/opt/puppetlabs/puppet/cache/puppet-selinux/modules/frr_local.loaded","tags":["file"],"file":"module.pp","line":177,"exported":false,"kind":"compilable_type"},
+    {"type":"Stage","title":"main","tags":["stage"],"exported":false,"kind":"compilable_type","parameters":{"name":"main"}}
+  ]
+}`
+
+func TestNormalizeAcceptsMissingParameters(t *testing.T) {
+	res := normalized(t, optionalFieldCatalog, Options{IncludeClasses: true})
+
+	for _, key := range []string{"Class[Settings]", "Anchor[selinux::end]"} {
+		r, ok := res[key]
+		if !ok {
+			t.Fatalf("%s dropped: %v", key, res)
+		}
+		if r.Parameters == nil {
+			t.Errorf("%s: absent parameters must normalise to an empty map", key)
+		}
+		if len(r.Parameters) != 0 {
+			t.Errorf("%s: unexpected parameters %v", key, r.Parameters)
+		}
+	}
+}
+
+func TestDiffTreatsMissingParametersAsEmpty(t *testing.T) {
+	absent := normalized(t, `{"resources":[{"type":"Anchor","title":"a","tags":["anchor"],"exported":false}]}`, Options{})
+	empty := normalized(t, `{"resources":[{"type":"Anchor","title":"a","tags":["anchor"],"exported":false,"parameters":{}}]}`, Options{})
+
+	if changes := Diff(absent, empty, DiffOptions{}); len(changes) != 0 {
+		t.Errorf("absent and empty parameters must compare equal: %+v", changes)
+	}
+	if changes := Diff(empty, absent, DiffOptions{}); len(changes) != 0 {
+		t.Errorf("absent and empty parameters must compare equal in both directions: %+v", changes)
+	}
+}
+
+func TestDiffRemovedResourceWithoutParametersCarriesEmptyValue(t *testing.T) {
+	base := normalized(t, optionalFieldCatalog, Options{IncludeClasses: true})
+	changes := byKey(Diff(base, map[string]Resource{}, DiffOptions{}))
+
+	removed := changes["Class[Settings]"]
+	if len(removed) != 1 {
+		t.Fatalf("expected one removal, got %+v", removed)
+	}
+	params, ok := removed[0].OldValue.(map[string]any)
+	if !ok || len(params) != 0 {
+		t.Errorf("a resource without parameters must report an empty set, got %#v", removed[0].OldValue)
+	}
+}
+
+func TestNormalizeKeepsMissingSourceDistinctFromLineZero(t *testing.T) {
+	res := normalized(t, optionalFieldCatalog, Options{IncludeClasses: true})
+
+	settings := res["Class[Settings]"]
+	if settings.HasSource() {
+		t.Errorf("a resource with no file/line must report no source: %+v", settings)
+	}
+	if settings.Line != nil {
+		t.Errorf("an absent line must stay absent, got %d", *settings.Line)
+	}
+
+	anchor := res["Anchor[selinux::end]"]
+	if !anchor.HasSource() || anchor.File != "init.pp" || *anchor.Line != 111 {
+		t.Errorf("manifest location lost: %+v", anchor)
+	}
+
+	zero := Resource{Type: "File", Title: "/a", Line: intPtr(0)}
+	if !zero.HasSource() && zero.File != "" {
+		t.Error("line 0 with a file is a real location")
+	}
+}

internal/catalog/report.go

@@ -173,11 +173,17 @@ func structurePath(structure []string) string {
 	return strings.Join(structure, ".")
 }
 
-func location(file string, line int, opts RenderOptions) string {
-	if !opts.SourceFileLine || file == "" {
+// location renders the manifest source of a change. A resource the compiler
+// gave no file or line for is reported as unknown rather than as an empty
+// path or line 0, which would read as a real location.
+func location(file string, line *int, opts RenderOptions) string {
+	if !opts.SourceFileLine {
 		return ""
 	}
-	return fmt.Sprintf("  (%s:%d)", file, line)
+	if file == "" || line == nil {
+		return "  (source unknown)"
+	}
+	return fmt.Sprintf("  (%s:%d)", file, *line)
 }
 
 func parameterLines(indent string, value any) []string {

internal/catalog/report_test.go

@@ -138,3 +138,37 @@ func TestRenderTextSourceFileLineIsOptional(t *testing.T) {
 		t.Errorf("--display-source-file-line did not print the source:\n%s", located.String())
 	}
 }
+
+func TestRenderTextMarksMissingSourceUnknown(t *testing.T) {
+	changes := []Change{
+		{DiffType: DiffAdded, Type: "Class", Title: "Settings", Severity: SeverityNormal},
+		{DiffType: DiffRemoved, Type: "File", Title: "/etc/motd", OldFile: "site.pp", OldLine: intPtr(9), Severity: SeverityCritical},
+	}
+	var buf bytes.Buffer
+	if err := RenderText(&buf, NewReport("host.example.net", "develop", "benvin_test", changes), RenderOptions{SourceFileLine: true}); err != nil {
+		t.Fatal(err)
+	}
+	out := buf.String()
+	if !strings.Contains(out, "Class[Settings] added  (source unknown)") {
+		t.Errorf("a resource with no manifest location must render as unknown:\n%s", out)
+	}
+	if strings.Contains(out, ":0)") {
+		t.Errorf("an absent line must never render as line 0:\n%s", out)
+	}
+	if !strings.Contains(out, "(site.pp:9)") {
+		t.Errorf("a real location must still render:\n%s", out)
+	}
+}
+
+func TestRenderJSONOmitsAbsentSource(t *testing.T) {
+	changes := []Change{{DiffType: DiffAdded, Type: "Class", Title: "Settings", Severity: SeverityNormal}}
+	var buf bytes.Buffer
+	if err := RenderJSON(&buf, NewReport("host.example.net", "develop", "benvin_test", changes)); err != nil {
+		t.Fatal(err)
+	}
+	for _, key := range []string{"new_file", "new_line", "old_file", "old_line"} {
+		if strings.Contains(buf.String(), key) {
+			t.Errorf("%s must be omitted when the compiler gave no location:\n%s", key, buf.String())
+		}
+	}
+}

internal/catalog/testdata/report.txt

@@ -8,12 +8,12 @@ Changes          : 3 critical, 1 added, 1 removed, 4 changed
 
 CRITICAL
 --------------------------------------------------
-  ! Exec[reload] environment.PATH: /bin -> /usr/bin
+  ! Exec[reload] environment.PATH: /bin -> /usr/bin  (source unknown)
   ~ File[/etc/motd] content: old -> new  (site.pp:19)
-  ~ Package[vim] provider: <absent> -> dnf
+  ~ Package[vim] provider: <absent> -> dnf  (source unknown)
 
 OTHER
 --------------------------------------------------
-  ~ Mysql::Db[app] password: redacted:sha256:f52fbd32b2b3b86f -> redacted:sha256:fb8c2e2b85ca81eb
+  ~ Mysql::Db[app] password: redacted:sha256:f52fbd32b2b3b86f -> redacted:sha256:fb8c2e2b85ca81eb  (source unknown)
   + Notify[added] added  (new.pp:3)
   - Notify[gone] removed  (old.pp:1)

internal/puppetserver/puppetserver.go

@@ -89,10 +89,50 @@ func (e *CompileError) Error() string {
 		e.Environment, e.Status, msg)
 }
 
-// EnvironmentMismatchError means the server compiled a different environment
-// from the one asked for. Classification runs through an exec ENC (encapic ->
-// encapi) whose environment key outranks the requested one, so a mismatch
-// means the diff would have compared an environment against itself.
+// puppetDBMarkers appear in a compile failure caused by the compiler's own
+// PuppetDB query rather than by the manifest. The estate sees these
+// intermittently and the same node compiles on the next attempt.
+var puppetDBMarkers = []string{
+	"server_urls",
+	"/pdb/query/",
+	"/pdb/cmd/",
+}
+
+// manifestMarkers appear in a failure the manifest itself causes. Recompiling
+// the same code cannot change the outcome.
+var manifestMarkers = []string{
+	"evaluation error",
+	"syntax error",
+	"could not find class",
+	"could not find declared class",
+	"could not find data item",
+	"parse error",
+	"compilation-error",
+}
+
+// Transient reports whether the failure is worth retrying. A failed PuppetDB
+// query is checked first because the compiler reports it as an evaluation
+// error, which otherwise reads as a manifest problem. Anything the manifest
+// caused is never retried; every other 5xx is.
+func (e *CompileError) Transient() bool {
+	haystack := strings.ToLower(e.Message + " " + e.Kind)
+	for _, marker := range puppetDBMarkers {
+		if strings.Contains(haystack, marker) {
+			return true
+		}
+	}
+	for _, marker := range manifestMarkers {
+		if strings.Contains(haystack, marker) {
+			return false
+		}
+	}
+	return e.Status >= 500
+}
+
+// EnvironmentMismatchError means the server compiled a different, named
+// environment from the one asked for. It is only raised when both names are
+// non-empty: an unnamed environment is a decode failure, not an ENC decision,
+// and is reported as a ResponseError instead.
 type EnvironmentMismatchError struct {
 	Certname  string
 	Requested string
@@ -102,12 +142,36 @@ type EnvironmentMismatchError struct {
 func (e *EnvironmentMismatchError) Error() string {
 	return fmt.Sprintf(
 		"environment mismatch for %s: requested %q but the server compiled %q. "+
-			"The ENC (encapic -> encapi) supplies an authoritative environment for every node "+
-			"whose encapi status is not \"testing\", and options.prefer_requested_environment did "+
-			"not beat it. Set the node's encapi status to \"testing\" or fix the ENC before diffing",
+			"options.prefer_requested_environment did not override the environment the node "+
+			"resolved to; the usual cause is an ENC-supplied environment (encapic -> encapi) "+
+			"for a node whose encapi status is not \"testing\"",
 		e.Certname, e.Requested, e.Returned)
 }
 
+// ResponseError is a 200 response this client could not turn into a catalog:
+// it did not decode, or it decoded without an environment name. Both mean the
+// response shape is not the one expected, so retrying cannot help.
+type ResponseError struct {
+	Certname string
+	Size     int
+	Err      error
+}
+
+func (e *ResponseError) Error() string {
+	if e.Err != nil {
+		return fmt.Sprintf("catalog response for %s did not decode (%d bytes): %v",
+			e.Certname, e.Size, e.Err)
+	}
+	return fmt.Sprintf(
+		"catalog response for %s decoded without an environment name (%d bytes): "+
+			"the compiler returned a response shape this client does not understand. "+
+			"This is a client/server protocol mismatch; nothing about the node's "+
+			"classification or environment needs changing",
+		e.Certname, e.Size)
+}
+
+func (e *ResponseError) Unwrap() error { return e.Err }
+
 // CompileRequest describes one catalog compilation.
 type CompileRequest struct {
 	Certname     string
@@ -191,9 +255,12 @@ func (c *Client) Compile(ctx context.Context, req CompileRequest) (*catalog.Cata
 		return nil, compileError(req.Environment, resp.StatusCode, raw)
 	}
 
-	var cat catalog.Catalog
-	if err := json.Unmarshal(raw, &cat); err != nil {
-		return nil, fmt.Errorf("parsing catalog for %s: %w", req.Certname, err)
+	cat, err := decodeCatalog(raw)
+	if err != nil {
+		return nil, &ResponseError{Certname: req.Certname, Size: len(raw), Err: err}
+	}
+	if cat.Environment == "" {
+		return nil, &ResponseError{Certname: req.Certname, Size: len(raw)}
 	}
 	if cat.Environment != req.Environment {
 		return nil, &EnvironmentMismatchError{
@@ -202,7 +269,28 @@ func (c *Client) Compile(ctx context.Context, req CompileRequest) (*catalog.Cata
 			Returned:  cat.Environment,
 		}
 	}
-	return &cat, nil
+	return cat, nil
+}
+
+// catalogEnvelope is the wrapper OpenVoxServer / Puppet Server 8 returns from
+// POST /puppet/v4/catalog: the catalog itself sits under a single "catalog"
+// key.
+type catalogEnvelope struct {
+	Catalog *catalog.Catalog `json:"catalog"`
+}
+
+// decodeCatalog accepts both the wrapped response and a bare catalog, so a
+// compiler that does not wrap still works.
+func decodeCatalog(raw []byte) (*catalog.Catalog, error) {
+	var env catalogEnvelope
+	if err := json.Unmarshal(raw, &env); err == nil && env.Catalog != nil {
+		return env.Catalog, nil
+	}
+	var bare catalog.Catalog
+	if err := json.Unmarshal(raw, &bare); err != nil {
+		return nil, err
+	}
+	return &bare, nil
 }
 
 // Health checks the unauthenticated /status/v1/simple endpoint.

internal/puppetserver/puppetserver_test.go

@@ -8,9 +8,13 @@ import (
 	"errors"
 	"net/http"
 	"net/http/httptest"
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 	"time"
+
+	"git.unkin.net/unkin/puppet-catalog-tools/internal/catalog"
 )
 
 const catalogBody = `{
@@ -64,7 +68,7 @@ func TestCompileSuccess(t *testing.T) {
 	if cat.Environment != "benvin_test" || len(cat.Resources) != 2 {
 		t.Fatalf("unexpected catalog: %+v", cat)
 	}
-	if cat.Resources[1].File == "" || cat.Resources[1].Line != 12 {
+	if line := cat.Resources[1].Line; cat.Resources[1].File == "" || line == nil || *line != 12 {
 		t.Errorf("resource context lost: %+v", cat.Resources[1])
 	}
 
@@ -183,10 +187,12 @@ func TestCompileBadCatalogJSON(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	if _, err := tlsClient(t, srv).Compile(context.Background(), CompileRequest{
+	_, err := tlsClient(t, srv).Compile(context.Background(), CompileRequest{
 		Certname: "host.example.net", Environment: "benvin_test",
-	}); err == nil {
-		t.Fatal("expected a parse error")
+	})
+	var re *ResponseError
+	if !errors.As(err, &re) {
+		t.Fatalf("err = %v (%T), want *ResponseError", err, err)
 	}
 }
 
@@ -263,3 +269,156 @@ func TestTransactionUUIDUnique(t *testing.T) {
 		t.Errorf("bad uuids %q %q", a, b)
 	}
 }
+
+// wrappedResponse is a trimmed capture of a real OpenVoxServer 8.8 response:
+// the catalog sits under a single "catalog" key.
+func wrappedResponse(t *testing.T) []byte {
+	t.Helper()
+	raw, err := os.ReadFile(filepath.Join("testdata", "catalog_response.json"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	return raw
+}
+
+func compileFixture(t *testing.T, body []byte, environment string) (*catalog.Catalog, error) {
+	t.Helper()
+	srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = w.Write(body)
+	}))
+	defer srv.Close()
+	return tlsClient(t, srv).Compile(context.Background(), CompileRequest{
+		Certname: "prodnxsr0009.main.unkin.net", Environment: environment,
+	})
+}
+
+func TestCompileUnwrapsServerResponse(t *testing.T) {
+	cat, err := compileFixture(t, wrappedResponse(t), "develop")
+	if err != nil {
+		t.Fatalf("Compile: %v", err)
+	}
+	if cat.Environment != "develop" || cat.Name != "prodnxsr0009.main.unkin.net" {
+		t.Fatalf("envelope not unwrapped: %+v", cat)
+	}
+	if len(cat.Resources) != 9 || len(cat.Classes) == 0 || len(cat.Tags) == 0 {
+		t.Fatalf("catalog body lost: %d resources, %d classes, %d tags",
+			len(cat.Resources), len(cat.Classes), len(cat.Tags))
+	}
+	if string(cat.Version) != `"979c188c3437099863e0b53c57942ef7ed6523a3"` {
+		t.Errorf("version = %s, want the string form the server sends", cat.Version)
+	}
+}
+
+func TestCompileUnwrapsBareCatalog(t *testing.T) {
+	var wrapped struct {
+		Catalog json.RawMessage `json:"catalog"`
+	}
+	if err := json.Unmarshal(wrappedResponse(t), &wrapped); err != nil {
+		t.Fatal(err)
+	}
+	cat, err := compileFixture(t, wrapped.Catalog, "develop")
+	if err != nil {
+		t.Fatalf("an unwrapped catalog must still decode: %v", err)
+	}
+	if cat.Environment != "develop" || len(cat.Resources) != 9 {
+		t.Fatalf("unexpected catalog: %+v", cat)
+	}
+}
+
+func TestCompileDecodesOptionalResourceFields(t *testing.T) {
+	cat, err := compileFixture(t, wrappedResponse(t), "develop")
+	if err != nil {
+		t.Fatal(err)
+	}
+	byKey := map[string]catalog.Resource{}
+	for _, r := range cat.Resources {
+		byKey[r.Key()] = r
+	}
+
+	settings, ok := byKey["Class[Settings]"]
+	if !ok {
+		t.Fatalf("fixture lost Class[Settings]: %v", byKey)
+	}
+	if settings.Parameters != nil || len(settings.Params()) != 0 {
+		t.Errorf("a resource without parameters must read as an empty set: %+v", settings)
+	}
+	if settings.HasSource() || settings.Line != nil {
+		t.Errorf("a resource without file/line must report no source: %+v", settings)
+	}
+
+	anchor := byKey["Anchor[selinux::end]"]
+	if anchor.Parameters != nil {
+		t.Errorf("anchor parameters should be absent: %+v", anchor.Parameters)
+	}
+	if !anchor.HasSource() || *anchor.Line != 111 {
+		t.Errorf("anchor manifest location lost: %+v", anchor)
+	}
+
+	pkg := byKey["Package[openvox-agent]"]
+	if pkg.Params()["ensure"] != "7.37.2" || !pkg.HasSource() {
+		t.Errorf("package decoded wrong: %+v", pkg)
+	}
+
+	key := byKey["File[/etc/dns-updater/key]"]
+	if len(key.SensitiveParameters) != 1 || key.SensitiveParameters[0] != "content" {
+		t.Errorf("per-resource sensitive_parameters lost: %+v", key)
+	}
+
+	member := byKey["Haproxy::Balancermember[prodnxsr0009.main.unkin.net_9443]"]
+	if !member.Exported {
+		t.Errorf("exported flag lost: %+v", member)
+	}
+}
+
+func TestCompileEmptyEnvironmentIsAProtocolError(t *testing.T) {
+	_, err := compileFixture(t, []byte(`{"unexpected":{"environment":"develop"}}`), "develop")
+
+	var re *ResponseError
+	if !errors.As(err, &re) {
+		t.Fatalf("err = %v (%T), want *ResponseError", err, err)
+	}
+	var me *EnvironmentMismatchError
+	if errors.As(err, &me) {
+		t.Fatal("a response with no environment must not be reported as a mismatch")
+	}
+	msg := err.Error()
+	if !strings.Contains(msg, "protocol mismatch") {
+		t.Errorf("message must name the real cause: %s", msg)
+	}
+	for _, forbidden := range []string{"encapi", "ENC", "testing"} {
+		if strings.Contains(msg, forbidden) {
+			t.Errorf("a decode failure must not blame %q: %s", forbidden, msg)
+		}
+	}
+}
+
+func TestCompileErrorTransientClassification(t *testing.T) {
+	const pdbFailure = "Internal Server Error: org.jruby.exceptions.RuntimeError: (PreformattedError) Evaluation Error: " +
+		"Error while evaluating a Resource Statement, Evaluation Error: Error while evaluating a Function Call, " +
+		"Failed to execute '/pdb/query/v4' on at least 1 of the following 'server_urls': https://puppetdb.example.net " +
+		"(file: /etc/puppetlabs/code/environments/develop/site/profiles/manifests/consul/client.pp, line: 31, column: 29) " +
+		"on node host.example.net"
+	const missingClass = "Internal Server Error: org.jruby.exceptions.RuntimeError: (Error) " +
+		"Could not find class roles::infra::incus::node for host.example.net on node host.example.net"
+
+	tests := []struct {
+		name    string
+		err     CompileError
+		wantRun bool
+	}{
+		{name: "puppetdb query failure", err: CompileError{Status: 500, Message: pdbFailure}, wantRun: true},
+		{name: "missing class", err: CompileError{Status: 500, Message: missingClass}},
+		{name: "manifest error at 500", err: CompileError{Status: 500, Message: "Evaluation Error: undefined method `escape' for URI:Module"}},
+		{name: "manifest error at 400", err: CompileError{Status: 400, Kind: "puppet.server/compilation-error", Message: "Syntax error at '}'"}},
+		{name: "bad gateway", err: CompileError{Status: 502, Message: "bad gateway"}, wantRun: true},
+		{name: "service unavailable", err: CompileError{Status: 503, Message: ""}, wantRun: true},
+		{name: "bad request", err: CompileError{Status: 400, Message: "no message returned"}},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := tc.err.Transient(); got != tc.wantRun {
+				t.Errorf("Transient() = %v, want %v", got, tc.wantRun)
+			}
+		})
+	}
+}

internal/puppetserver/testdata/catalog_response.json

@@ -0,0 +1,269 @@
+{
+  "catalog": {
+    "tags": [
+      "settings",
+      "roles::infra::incus::node"
+    ],
+    "name": "prodnxsr0009.main.unkin.net",
+    "version": "979c188c3437099863e0b53c57942ef7ed6523a3",
+    "code_id": null,
+    "catalog_uuid": "8d7f423e-5bf1-4b64-97b4-4a20c67ec59f",
+    "catalog_format": 2,
+    "environment": "develop",
+    "resources": [
+      {
+        "type": "Stage",
+        "title": "main",
+        "tags": [
+          "stage"
+        ],
+        "exported": false,
+        "kind": "compilable_type",
+        "parameters": {
+          "name": "main"
+        }
+      },
+      {
+        "type": "Class",
+        "title": "Settings",
+        "tags": [
+          "class",
+          "settings"
+        ],
+        "exported": false,
+        "kind": "unknown"
+      },
+      {
+        "type": "Class",
+        "title": "Profiles::Puppet::Agent",
+        "tags": [
+          "class",
+          "profiles::puppet::agent",
+          "profiles",
+          "puppet",
+          "agent",
+          "profiles::base",
+          "base",
+          "roles::infra::incus::node",
+          "roles",
+          "infra",
+          "incus",
+          "node"
+        ],
+        "exported": false,
+        "kind": "unknown",
+        "parameters": {
+          "version": "7.37.2",
+          "openvox_enable": true
+        }
+      },
+      {
+        "type": "Anchor",
+        "title": "selinux::end",
+        "tags": [
+          "anchor",
+          "selinux::end",
+          "selinux",
+          "end",
+          "class",
+          "selinux::module",
+          "module",
+          "frr_local",
+          "profiles::selinux::frr",
+          "profiles",
+          "frr",
+          "profiles::base",
+          "base",
+          "roles::infra::incus::node",
+          "roles",
+          "infra",
+          "incus",
+          "node"
+        ],
+        "file": "/etc/puppetlabs/code/environments/develop/external_modules/selinux/manifests/init.pp",
+        "line": 111,
+        "exported": false,
+        "kind": "compilable_type"
+      },
+      {
+        "type": "File",
+        "title": "/opt/puppetlabs/puppet/cache/puppet-selinux/modules/frr_local.loaded",
+        "tags": [
+          "file",
+          "selinux::module",
+          "selinux",
+          "module",
+          "frr_local",
+          "class",
+          "profiles::selinux::frr",
+          "profiles",
+          "frr",
+          "profiles::base",
+          "base",
+          "roles::infra::incus::node",
+          "roles",
+          "infra",
+          "incus",
+          "node"
+        ],
+        "file": "/etc/puppetlabs/code/environments/develop/external_modules/selinux/manifests/module.pp",
+        "line": 177,
+        "exported": false,
+        "kind": "compilable_type"
+      },
+      {
+        "type": "Package",
+        "title": "openvox-agent",
+        "tags": [
+          "package",
+          "openvox-agent",
+          "class",
+          "profiles::puppet::agent",
+          "profiles",
+          "puppet",
+          "agent",
+          "profiles::base",
+          "base",
+          "roles::infra::incus::node",
+          "roles",
+          "infra",
+          "incus",
+          "node"
+        ],
+        "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/puppet/agent.pp",
+        "line": 58,
+        "exported": false,
+        "kind": "compilable_type",
+        "parameters": {
+          "ensure": "7.37.2",
+          "require": "Yumrepo[openvox]"
+        }
+      },
+      {
+        "type": "Service",
+        "title": "puppet",
+        "tags": [
+          "service",
+          "puppet",
+          "class",
+          "profiles::puppet::agent",
+          "profiles",
+          "agent",
+          "profiles::base",
+          "base",
+          "roles::infra::incus::node",
+          "roles",
+          "infra",
+          "incus",
+          "node"
+        ],
+        "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/puppet/agent.pp",
+        "line": 81,
+        "exported": false,
+        "kind": "compilable_type",
+        "parameters": {
+          "ensure": "running",
+          "enable": true,
+          "hasrestart": true,
+          "require": "Package[openvox-agent]"
+        }
+      },
+      {
+        "type": "File",
+        "title": "/etc/dns-updater/key",
+        "tags": [
+          "file",
+          "class",
+          "profiles::dns::updater",
+          "profiles",
+          "dns",
+          "updater",
+          "profiles::dns::base",
+          "base",
+          "profiles::base",
+          "roles::infra::incus::node",
+          "roles",
+          "infra",
+          "incus",
+          "node"
+        ],
+        "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/dns/updater.pp",
+        "line": 97,
+        "exported": false,
+        "kind": "compilable_type",
+        "parameters": {
+          "ensure": "file",
+          "owner": "root",
+          "group": "root",
+          "mode": "0600",
+          "show_diff": false,
+          "content": "REDACTED-IN-FIXTURE"
+        },
+        "sensitive_parameters": [
+          "content"
+        ]
+      },
+      {
+        "type": "Haproxy::Balancermember",
+        "title": "prodnxsr0009.main.unkin.net_9443",
+        "tags": [
+          "be_ceph_dashboard_au-syd1-prod",
+          "haproxy::balancermember",
+          "haproxy",
+          "balancermember",
+          "prodnxsr0009.main.unkin.net_9443",
+          "profiles::haproxy::balancemember",
+          "profiles",
+          "balancemember",
+          "class",
+          "profiles::ceph::dashboard",
+          "ceph",
+          "dashboard",
+          "profiles::base",
+          "base",
+          "roles::infra::incus::node",
+          "roles",
+          "infra",
+          "incus",
+          "node"
+        ],
+        "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/haproxy/balancemember.pp",
+        "line": 12,
+        "exported": true,
+        "kind": "defined_type",
+        "parameters": {
+          "listening_service": "be_ceph_dashboard",
+          "ports": [
+            9443
+          ],
+          "server_names": "prodnxsr0009",
+          "ipaddresses": "198.18.23.9",
+          "options": [
+            "cookie prodnxsr0009",
+            "ssl",
+            "verify none",
+            "check",
+            "inter 2s",
+            "rise 3",
+            "fall 2"
+          ],
+          "tag": "be_ceph_dashboard_au-syd1-prod"
+        }
+      }
+    ],
+    "edges": [
+      {
+        "source": "Stage[main]",
+        "target": "Class[Settings]"
+      },
+      {
+        "source": "Stage[main]",
+        "target": "Class[main]"
+      }
+    ],
+    "classes": [
+      "settings",
+      "profiles::puppet::agent"
+    ]
+  }
+}