Add repo scaffold and the catalog-diff CLI #1

Merged

benvin merged 9 commits from benvin/scaffold-catalog-diff into main 2026-09-12 18:18:20 +10:00

Conversation 4 Commits 9 Files Changed 31

+4944 -1

unkin-agent commented 2026-09-12 15:54:43 +10:00

Member

Copy Link

Copy Source

The old catalog_diff.py shells out to puppet catalog compile --server, which compiles locally and silently ignores the server, so branch environments were never really tested. This replaces it with a Go CLI that compiles over the puppetserver API.

• add catalog-diff: compile a host's catalog in a base and a test environment, then diff
• add puppetserver mTLS client for POST /puppet/v4/catalog, hard-failing on an ENC environment override
• add PuppetDB fact client reading facts with one query per node
• add diff normalisation, severity bucketing and text/JSON renderers
• add Makefile, nfpm RPM packaging, woodpecker pipelines and pre-commit config

The old catalog_diff.py shells out to `puppet catalog compile --server`, which compiles locally and silently ignores the server, so branch environments were never really tested. This replaces it with a Go CLI that compiles over the puppetserver API. - add catalog-diff: compile a host's catalog in a base and a test environment, then diff - add puppetserver mTLS client for POST /puppet/v4/catalog, hard-failing on an ENC environment override - add PuppetDB fact client reading facts with one query per node - add diff normalisation, severity bucketing and text/JSON renderers - add Makefile, nfpm RPM packaging, woodpecker pipelines and pre-commit config

unkin-agent added 1 commit 2026-09-12 15:54:44 +10:00

add repo scaffold and the catalog-diff CLI ... 53007a9257

Compile a host's catalog in a base and a test environment over the
puppetserver API, then diff the two.

- add catalog-diff CLI with cobra flags, config precedence and exit codes
- add puppetserver mTLS client for POST /puppet/v4/catalog
- add pdbmux fact client
- add catalog normalisation, diff engine and text/JSON renderers
- add Makefile, nfpm RPM packaging and woodpecker pipelines

unkin-agent added 1 commit 2026-09-12 15:55:36 +10:00

keep stderr quiet when the diff exits with differences dcbd810c34

unkin-agent commented 2026-09-12 16:03:33 +10:00

Author

Member

Copy Link

Copy Source

Reviewed the diff, ran make test (go test -v -race ./..., all green, no data races) and make lint (golangci-lint run ./..., 0 issues).

No correctness bugs found. Specifics checked and confirmed correct:

• Diff determinism is actually tested (TestDiffDeterministicOrdering runs the diff 20x over multi-key maps; Diff's final sort key is (resource key, diff type, structure path), which is provably unique per change, so map-iteration randomness can't leak into output order). deepEqual uses json.Marshal, which sorts map keys, so parameter-map ordering never registers as a change — also covered by TestDiffIgnoresMapOrdering.
• Retry logic: transient() correctly refuses to retry EnvironmentMismatchError and 4xx CompileError, only retries 5xx/transport errors; covered by TestRunRetriesTransientFailures / TestRunDoesNotRetryCompilationErrors. Backoff math is correct (starts at 2s, caps at 30s, jitters).
• Concurrency: the two compiles write to disjoint variables from their own goroutines, share only a read-only facts map — no data race, confirmed by -race. Bounded by --retry/--timeout, no goroutine leak.
• Exit codes: --help, README table, and main.go constants all agree (0/1/2/3, --testonly never returns 2) — verified against tests, not just docs.
• mTLS/TLS client construction, CA/cert/key loading, context propagation via NewRequestWithContext, and response body closing all look correct.
• Security: sensitive catalog parameters are redacted to a sha256 digest before ever reaching stdout/JSON (catalog.redact), so a change is visible without the plaintext leaking. Non-marked catalog content is explicitly documented in the README as remaining in the clear — an honest, deliberate tradeoff rather than an oversight.
• JSON output: Changes is normalized to []Change{} (never nil) in NewReport, and Resource.Parameters is always a non-nil map from cleanParameters — no nil-vs-empty-slice inconsistency found.
• Makefile targets, woodpecker resource requests/serviceAccountName, and RPM+shell-completions packaging all match the node-lookup reference pattern.

Two non-blocking observations (style/question, not bugs):

1. Report.Compiled is a map[string]bool with a false/"failed" rendering path in compiledLine, but every current call site (NewReport) only runs after both compiles already succeeded — so Compiled is always {base: true, test: true} today and the "failed" branch is unreachable. Looks like scaffolding for a future partial-failure report; worth a comment noting that, or trimming until it's used.
2. cleanParameters merges the catalog-level sensitive_parameters list with each resource's own sensitive_parameters and redacts any resource parameter whose name appears in either list — i.e. a catalog-level sensitive name gets redacted on every resource, not just the resource(s) it was declared sensitive on. If the catalog's top-level sensitive_parameters is actually a global list of parameter names (rather than per-resource), this is correct; if it's meant as some other shape, it could over-redact. Worth a quick confirmation against a real OpenVoxServer catalog sample.

On atomicity: this bundles the CLI, two HTTP clients, the diff engine, and the full CI/packaging scaffold into one PR. Normally that would be feature creep, but this is the first PR into a brand-new empty repo — there's no prior scaffold to build on, and splitting it wouldn't leave a working state at any intermediate point (an empty repo with CI, or code with no CI to prove it). Treating this as a justified bootstrap exception rather than a violation.

PR body is compliant with conventions (7 lines, 616 chars, why + present-tense bullets, no narrative/alternatives/AI references).

Reviewed the diff, ran `make test` (`go test -v -race ./...`, all green, no data races) and `make lint` (`golangci-lint run ./...`, 0 issues). No correctness bugs found. Specifics checked and confirmed correct: - Diff determinism is actually tested (`TestDiffDeterministicOrdering` runs the diff 20x over multi-key maps; `Diff`'s final sort key is (resource key, diff type, structure path), which is provably unique per change, so map-iteration randomness can't leak into output order). `deepEqual` uses `json.Marshal`, which sorts map keys, so parameter-map ordering never registers as a change — also covered by `TestDiffIgnoresMapOrdering`. - Retry logic: `transient()` correctly refuses to retry `EnvironmentMismatchError` and 4xx `CompileError`, only retries 5xx/transport errors; covered by `TestRunRetriesTransientFailures` / `TestRunDoesNotRetryCompilationErrors`. Backoff math is correct (starts at 2s, caps at 30s, jitters). - Concurrency: the two compiles write to disjoint variables from their own goroutines, share only a read-only `facts` map — no data race, confirmed by `-race`. Bounded by `--retry`/`--timeout`, no goroutine leak. - Exit codes: `--help`, README table, and `main.go` constants all agree (0/1/2/3, `--testonly` never returns 2) — verified against tests, not just docs. - mTLS/TLS client construction, CA/cert/key loading, context propagation via `NewRequestWithContext`, and response body closing all look correct. - Security: sensitive catalog parameters are redacted to a sha256 digest before ever reaching stdout/JSON (`catalog.redact`), so a change is visible without the plaintext leaking. Non-marked catalog content is explicitly documented in the README as remaining in the clear — an honest, deliberate tradeoff rather than an oversight. - JSON output: `Changes` is normalized to `[]Change{}` (never nil) in `NewReport`, and `Resource.Parameters` is always a non-nil map from `cleanParameters` — no nil-vs-empty-slice inconsistency found. - Makefile targets, woodpecker resource requests/serviceAccountName, and RPM+shell-completions packaging all match the node-lookup reference pattern. Two non-blocking observations (style/question, not bugs): 1. `Report.Compiled` is a `map[string]bool` with a `false`/"failed" rendering path in `compiledLine`, but every current call site (`NewReport`) only runs after both compiles already succeeded — so `Compiled` is always `{base: true, test: true}` today and the "failed" branch is unreachable. Looks like scaffolding for a future partial-failure report; worth a comment noting that, or trimming until it's used. 2. `cleanParameters` merges the catalog-level `sensitive_parameters` list with each resource's own `sensitive_parameters` and redacts any resource parameter whose name appears in either list — i.e. a catalog-level sensitive name gets redacted on every resource, not just the resource(s) it was declared sensitive on. If the catalog's top-level `sensitive_parameters` is actually a global list of parameter names (rather than per-resource), this is correct; if it's meant as some other shape, it could over-redact. Worth a quick confirmation against a real OpenVoxServer catalog sample. On atomicity: this bundles the CLI, two HTTP clients, the diff engine, and the full CI/packaging scaffold into one PR. Normally that would be feature creep, but this is the first PR into a brand-new empty repo — there's no prior scaffold to build on, and splitting it wouldn't leave a working state at any intermediate point (an empty repo with CI, or code with no CI to prove it). Treating this as a justified bootstrap exception rather than a violation. PR body is compliant with conventions (7 lines, 616 chars, why + present-tense bullets, no narrative/alternatives/AI references).

unkin-agent referenced this pull request 2026-09-12 16:15:19 +10:00

add the catalog-impact binary #2

unkin-agent added 1 commit 2026-09-12 16:59:04 +10:00

unwrap the catalog response and classify failures correctly ... a76480d454

- 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

unkin-agent added 1 commit 2026-09-12 17:00:43 +10:00

assert line 0 with a file counts as a real source location 1aea8e8dbf

unkin-agent commented 2026-09-12 17:09:29 +10:00

Author

Member

Copy Link

Copy Source

Reviewed at head 1aea8e8 (benvin/scaffold-catalog-diff). make test, go test -race ./... and make lint all pass clean. Verdict: solid overall — envelope decode, missing-parameters/file/line, retry classification and ENC-blame handling all match the ground-truth response shape and are well covered by tests. Two real bugs found (one crash, one latent decode bug), plus lower-priority items.

Blocking

1. cmd/catalog-diff/main.go:298-326 (compile) — catalog-diff <host> --retry -1 (or any negative --retry) panics with a nil-pointer dereference instead of exiting cleanly. The loop for attempt := 0; attempt <= opts.retry; attempt++ never executes when opts.retry < 0, so lastErr stays nil; that nil gets wrapped in &exitError{err: nil}, and main()'s fmt.Fprintln(os.Stderr, "error:", err) calls exitError.Error() → e.err.Error() on a nil error → SIGSEGV. Reproduced locally (built the binary / drove run() directly with opts.retry = -1, --testonly, a compiling puppetserver stub returning 500). No flag validation currently rejects a negative --retry. Fix: validate --retry >= 0 in flag parsing, or make compile() handle "no attempt ran" explicitly.

2. internal/pdbmux/pdbmux.go:66-84 (Facts fallback) — the /pdb/query/v4/factsets/<certname> fallback decodes into a bare factset struct, i.e. assumes a JSON object. Confirmed real PuppetDB returns an array of factsets at that endpoint; json.Unmarshal([]byte("[{...}]"), &factset{}) fails with json: cannot unmarshal array into Go value of type pdbmux.factset (verified). The bundled test fixture (factsetBody in pdbmux_test.go) is itself a bare object, so the test only proves the code matches its own wrong assumption — it never exercises the real shape. This currently "works" only because pdbmux (the internal proxy) happens to return an object here, diverging from the real PuppetDB API it's meant to be compatible with. If pdbmux is ever pointed at real PuppetDB for this route, or its behavior changes, the fallback breaks for every node that needs it. Fix: accept both shapes (try array first, or a json.RawMessage-sniffing decode), and add a fixture using the real array shape.

Worth fixing, not blocking

3. internal/pdbmux/pdbmux.go:109-121 (fold) — a fact record with no "value" key decodes Value as a nil json.RawMessage, and json.Unmarshal(nil, &v) returns "unexpected end of JSON input" (verified, does not panic). That aborts Facts() entirely for the whole host on one malformed record, rather than skipping just that fact. Real PuppetDB fact data can be sparse/odd; worth treating a missing value as nil instead of a hard error.

4. internal/puppetserver/testdata/catalog_response.json / AGENTS.md note it's "a trimmed capture of a real OpenVoxServer 8.8 response" — it carries a real production hostname (prodnxsr0009.main.unkin.net), real class/role names (profiles::ceph::dashboard, roles::infra::incus::node), and a real internal path layout. The one genuine secret (content) is manually replaced with REDACTED-IN-FIXTURE, and the IP used (198.18.23.9) is in the IANA benchmark-reserved range, so nothing sensitive leaks — but this is a public repo, and topology/naming don't need to come from live infra to prove the envelope/missing-field shape. Consider a synthetic fixture instead.

Not blocking, would have done differently

5. cmd/catalog-diff/main.go:238-252 (run) — the base and test compiles run concurrently but neither is cancelled when the other fails; a fast manifest error in the test environment still waits out the full base-environment compile (up to --timeout, default 10m) before reporting. An errgroup-with-cancel would fail faster.

6. Same function, compile() — if ctx is cancelled during the backoff sleep, sleepCtx returns the context error but compile() discards it and returns the previous compile error via lastErr, so a caller-imposed deadline surfaces as a stale compile failure rather than a timeout/cancellation. Minor, but slightly misattributes the actual cause.

Confirmed correct / no action needed

• Envelope decode (catalog wrapper), missing parameters (→ empty map, distinguishable via Resource.Parameters == nil), missing file/line (pointer Line, HasSource()), and decode failures reported as protocol errors (not ENC/classification blame) — all match the real response shape and are exercised by targeted tests including the real captured fixture.
• Retry classification correctly orders PuppetDB-query markers before manifest markers so a PuppetDB failure wrapped in "Evaluation Error" text isn't misread as a manifest bug, and both directions are tested against the real error strings given (Failed to execute '/pdb/query/v4'..., undefined method 'escape'..., Could not find class...).
• Secret redaction (redact() → sha256 digest) happens at Normalize time, so it's baked into every Change.OldValue/NewValue before either the text or JSON renderer sees it — confirmed via the golden report.json/report.txt fixtures.
• Diff output is deterministic despite Go's random map iteration: Diff() sorts the final change list by key/type/structure, and deepEqual/unionKeys normalize via JSON re-encoding / explicit key sorting. Covered by TestDiffDeterministicOrdering.
• Exit codes (0/1/2/3, --testonly never 2) match --help/README and are individually tested.
• Makefile, nfpm packaging (with shell completions), and Woodpecker backend_options.kubernetes.resources all match the node-lookup house conventions closely (including serviceAccountName: default, which node-lookup also uses).
• PR body is compliant: why-paragraph + present-tense bullets, 7 lines, 616 chars.
• Bundling CLI + HTTP clients + diff engine + full CI/packaging scaffold in this first PR is a justified bootstrap, not feature creep — it mirrors the reference repo's layout exactly and the tool is not meaningfully shippable/testable without the packaging half.

Reviewed at head `1aea8e8` (`benvin/scaffold-catalog-diff`). `make test`, `go test -race ./...` and `make lint` all pass clean. Verdict: solid overall — envelope decode, missing-`parameters`/`file`/`line`, retry classification and ENC-blame handling all match the ground-truth response shape and are well covered by tests. Two real bugs found (one crash, one latent decode bug), plus lower-priority items. **Blocking** 1. `cmd/catalog-diff/main.go:298-326` (`compile`) — `catalog-diff <host> --retry -1` (or any negative `--retry`) panics with a nil-pointer dereference instead of exiting cleanly. The loop `for attempt := 0; attempt <= opts.retry; attempt++` never executes when `opts.retry < 0`, so `lastErr` stays `nil`; that `nil` gets wrapped in `&exitError{err: nil}`, and `main()`'s `fmt.Fprintln(os.Stderr, "error:", err)` calls `exitError.Error()` → `e.err.Error()` on a nil `error` → SIGSEGV. Reproduced locally (built the binary / drove `run()` directly with `opts.retry = -1`, `--testonly`, a compiling puppetserver stub returning 500). No flag validation currently rejects a negative `--retry`. Fix: validate `--retry >= 0` in flag parsing, or make `compile()` handle "no attempt ran" explicitly. 2. `internal/pdbmux/pdbmux.go:66-84` (`Facts` fallback) — the `/pdb/query/v4/factsets/<certname>` fallback decodes into a bare `factset` struct, i.e. assumes a JSON object. Confirmed real PuppetDB returns an **array** of factsets at that endpoint; `json.Unmarshal([]byte("[{...}]"), &factset{})` fails with `json: cannot unmarshal array into Go value of type pdbmux.factset` (verified). The bundled test fixture (`factsetBody` in `pdbmux_test.go`) is itself a bare object, so the test only proves the code matches its own wrong assumption — it never exercises the real shape. This currently "works" only because pdbmux (the internal proxy) happens to return an object here, diverging from the real PuppetDB API it's meant to be compatible with. If pdbmux is ever pointed at real PuppetDB for this route, or its behavior changes, the fallback breaks for every node that needs it. Fix: accept both shapes (try array first, or a `json.RawMessage`-sniffing decode), and add a fixture using the real array shape. **Worth fixing, not blocking** 3. `internal/pdbmux/pdbmux.go:109-121` (`fold`) — a fact record with no `"value"` key decodes `Value` as a nil `json.RawMessage`, and `json.Unmarshal(nil, &v)` returns `"unexpected end of JSON input"` (verified, does not panic). That aborts `Facts()` entirely for the whole host on one malformed record, rather than skipping just that fact. Real PuppetDB fact data can be sparse/odd; worth treating a missing `value` as `nil` instead of a hard error. 4. `internal/puppetserver/testdata/catalog_response.json` / `AGENTS.md` note it's "a trimmed capture of a real OpenVoxServer 8.8 response" — it carries a real production hostname (`prodnxsr0009.main.unkin.net`), real class/role names (`profiles::ceph::dashboard`, `roles::infra::incus::node`), and a real internal path layout. The one genuine secret (`content`) is manually replaced with `REDACTED-IN-FIXTURE`, and the IP used (`198.18.23.9`) is in the IANA benchmark-reserved range, so nothing sensitive leaks — but this is a public repo, and topology/naming don't need to come from live infra to prove the envelope/missing-field shape. Consider a synthetic fixture instead. **Not blocking, would have done differently** 5. `cmd/catalog-diff/main.go:238-252` (`run`) — the base and test compiles run concurrently but neither is cancelled when the other fails; a fast manifest error in the test environment still waits out the full base-environment compile (up to `--timeout`, default 10m) before reporting. An `errgroup`-with-cancel would fail faster. 6. Same function, `compile()` — if `ctx` is cancelled during the backoff sleep, `sleepCtx` returns the context error but `compile()` discards it and returns the *previous* compile error via `lastErr`, so a caller-imposed deadline surfaces as a stale compile failure rather than a timeout/cancellation. Minor, but slightly misattributes the actual cause. **Confirmed correct / no action needed** - Envelope decode (`catalog` wrapper), missing `parameters` (→ empty map, distinguishable via `Resource.Parameters == nil`), missing `file`/`line` (pointer `Line`, `HasSource()`), and decode failures reported as protocol errors (not ENC/classification blame) — all match the real response shape and are exercised by targeted tests including the real captured fixture. - Retry classification correctly orders PuppetDB-query markers before manifest markers so a PuppetDB failure wrapped in "Evaluation Error" text isn't misread as a manifest bug, and both directions are tested against the real error strings given (`Failed to execute '/pdb/query/v4'...`, `undefined method 'escape'...`, `Could not find class...`). - Secret redaction (`redact()` → sha256 digest) happens at `Normalize` time, so it's baked into every `Change.OldValue/NewValue` before either the text or JSON renderer sees it — confirmed via the golden `report.json`/`report.txt` fixtures. - Diff output is deterministic despite Go's random map iteration: `Diff()` sorts the final change list by key/type/structure, and `deepEqual`/`unionKeys` normalize via JSON re-encoding / explicit key sorting. Covered by `TestDiffDeterministicOrdering`. - Exit codes (0/1/2/3, `--testonly` never 2) match `--help`/README and are individually tested. - Makefile, nfpm packaging (with shell completions), and Woodpecker `backend_options.kubernetes.resources` all match the `node-lookup` house conventions closely (including `serviceAccountName: default`, which node-lookup also uses). - PR body is compliant: why-paragraph + present-tense bullets, 7 lines, 616 chars. - Bundling CLI + HTTP clients + diff engine + full CI/packaging scaffold in this first PR is a justified bootstrap, not feature creep — it mirrors the reference repo's layout exactly and the tool is not meaningfully shippable/testable without the packaging half.

unkin-agent added 3 commits 2026-09-12 17:19:33 +10:00

validate flag ranges and make exitError printable when empty ... 6e4ee54203

A negative --retry skipped the compile loop, leaving a nil error inside
exitError that panicked when printed.

- reject a negative or oversized --retry and a non-positive --timeout as
  usage errors (exit 3)
- print an exit code when exitError carries no inner error
- cap the backoff shift so a large --retry cannot overflow
- cancel the sibling compile on the first failure, and do not report that
  cancellation as the cause
- report a context cancellation during the retry backoff instead of the
  stale compile error

accept both factset shapes and skip undecodable facts ... 7f2c6f16c8

Real PuppetDB answers /factsets/<certname> with an array; the fallback
decoded a bare object, so it only worked against pdbmux's own divergent
response.

- decode an array of factsets, picking the requested certname
- keep accepting a single bare factset object
- drop and report a fact record that will not decode instead of failing
  the whole fetch
- fail only when nothing in the response decodes

make the catalog fixture synthetic ... 496f258cb0

The capture carried a production certname, real role and class names and
internal paths into a public repo.

- replace hostnames, class names and paths with fake equivalents
- keep the shape under test: catalog envelope, string version, null
  code_id, resources with and without parameters and file/line, the
  exported and sensitive_parameters cases
- document the range of --retry and --timeout

unkin-agent commented 2026-09-12 17:30:51 +10:00

Author

Member

Copy Link

Copy Source

Targeted review of the three most recent commits only (6e4ee54, 7f2c6f1, 496f258). go test -race (x3), make lint, and make pre-commit all pass clean.

Blocking

decodeFactsets() (internal/pdbmux/pdbmux.go) silently accepts a single non-matching factset.
When the array response contains exactly one entry and its certname doesn't match the requested one, the code falls through the len(sets) == 1 shortcut and returns that entry's facts with no error and no Warn call:

for _, fs := range sets {
    if fs.Certname == certname {
        return fs.Facts.Data, nil
    }
}
if len(sets) == 1 {
    return sets[0].Facts.Data, nil   // returned even when fs.Certname != certname
}

Reproduced directly: a factset array [{"certname":"totally-different-host.example.net", ...}] requested for host.example.net returns that other host's facts with err == nil. If PDB or pdbmux ever answers with a single factset for the wrong node (alias, caller bug, backend quirk), catalog-diff will silently compile against the wrong host's facts and produce a misleading diff with no signal anything went wrong. There's no test covering "array with exactly one non-matching entry" — only the 0-match, 2+-match, and bare-object (no certname field at all) cases are covered.

Suggest either: only take the single-entry shortcut when Certname == "" (matching the actual pdbmux quirk this was written for), or at minimum route it through Warn so the mismatch is visible.

Non-blocking

• decodeFactsets() peeks the first non-whitespace byte to choose array vs. object, but doesn't strip a UTF-8 BOM. A BOM-prefixed array (EF BB BF + [...]) is misdispatched to the bare-object branch and fails with invalid character '\xef'... instead of parsing as the array it is. Low real-world likelihood and it fails loudly rather than silently, and it's not a regression (the old single-shape decoder had the same gap) — not worth blocking on, but a one-line bytes.TrimPrefix would close it if anyone cares.
• The "no factset matched" error is wrapped as "parsing factset for %s: %w" even though it isn't a parse failure — reads a little oddly, cosmetic only.

Verified clean (no issues found)

• Cancellation/abandoned(): worked through all the combinations (parent done, parent live + sibling failed, near-simultaneous) — the design only cancels the shared compileCtx via abandon() on sibling failure, while the parent ctx is only ever done from an external cause, so ctx.Err() == nil && errors.Is(err, context.Canceled) cleanly separates "fallout from the sibling" from "the user/parent cancelled." No case found where a genuine failure gets misreported as abandonment or vice versa. defer abandon() covers every return path including panics; wg.Wait() happens-before the read of both errors, so no data race — confirmed with -race x3, clean.
• backoff(): checked 0, 1, 31, 32, 63, 64, and 1<<30 — the shift is only computed for 1 <= attempt < 32, so it never overflows (max shift is 2s<<30 ≈ 24.8 days, still fits int64, and gets clamped to the 30s ceiling before use) and is skipped entirely (falls back to the 30s ceiling) for anything at or above 32, however large. d is always positive before the jitter call, so rand.Int63n(d/2) never sees a non-positive argument and the total is always > 0. Confirmed via the existing TestBackoffStaysWithinBounds.
• Flag validation: --retry bounds and --timeout positivity are checked in validate(), called before resolveEnvironment or anything else touches the values; all reject with exit 3 as documented. No other numeric/duration flag has the same class of gap.
• Fixture scrub: grepped the whole tree for the retired real certname/UUID/version hash/role and class names/paths — none remain. The one real-looking IP (198.18.23.9) was replaced with 192.0.2.9 (TEST-NET-1, documentation-reserved). The only remaining *.unkin.net hosts are the tool's actual intentional config defaults (puppet.k8s.syd1.au.unkin.net, pdbmux.k8s.syd1.au.unkin.net) and CI/module-path references — pre-existing, not fixture leakage, untouched by these commits. Wire-format shape is preserved: envelope, null code_id, string version, resources with/without parameters/file+line, exported: true case, and per-resource sensitive_parameters are all still exercised.
• fold()'s Warn hook is nil-checked before use.

Judgment calls

• fold() erroring when every record is undecodable: agree with the choice. Silently returning an empty-but-"successful" fact map would let a completely broken fetch masquerade as a real (if factless) host, compiling a catalog that's quietly wrong rather than loudly failing. Loud failure is the safer default here.
• maxRetry = 20 cap: keep it. The overflow bug is already independently fixed in the backoff() rewrite (verified safe for any int, including very large values, since the shift is gated on attempt < 32), so the cap is no longer load-bearing for correctness — but it's a free, sensible operator guard against a typo like --retry 999999 turning a single host lookup into an effectively-unbounded hang. No reason to drop it.

Targeted review of the three most recent commits only (`6e4ee54`, `7f2c6f1`, `496f258`). `go test -race` (x3), `make lint`, and `make pre-commit` all pass clean. ## Blocking **`decodeFactsets()` (internal/pdbmux/pdbmux.go) silently accepts a single non-matching factset.** When the array response contains exactly one entry and its `certname` doesn't match the requested one, the code falls through the `len(sets) == 1` shortcut and returns that entry's facts with no error and no `Warn` call: ```go for _, fs := range sets { if fs.Certname == certname { return fs.Facts.Data, nil } } if len(sets) == 1 { return sets[0].Facts.Data, nil // returned even when fs.Certname != certname } ``` Reproduced directly: a factset array `[{"certname":"totally-different-host.example.net", ...}]` requested for `host.example.net` returns that other host's facts with `err == nil`. If PDB or pdbmux ever answers with a single factset for the wrong node (alias, caller bug, backend quirk), catalog-diff will silently compile against the wrong host's facts and produce a misleading diff with no signal anything went wrong. There's no test covering "array with exactly one non-matching entry" — only the 0-match, 2+-match, and bare-object (no certname field at all) cases are covered. Suggest either: only take the single-entry shortcut when `Certname == ""` (matching the actual pdbmux quirk this was written for), or at minimum route it through `Warn` so the mismatch is visible. ## Non-blocking - `decodeFactsets()` peeks the first non-whitespace byte to choose array vs. object, but doesn't strip a UTF-8 BOM. A BOM-prefixed array (`EF BB BF` + `[...]`) is misdispatched to the bare-object branch and fails with `invalid character '\xef'...` instead of parsing as the array it is. Low real-world likelihood and it fails loudly rather than silently, and it's not a regression (the old single-shape decoder had the same gap) — not worth blocking on, but a one-line `bytes.TrimPrefix` would close it if anyone cares. - The "no factset matched" error is wrapped as `"parsing factset for %s: %w"` even though it isn't a parse failure — reads a little oddly, cosmetic only. ## Verified clean (no issues found) - **Cancellation/`abandoned()`**: worked through all the combinations (parent done, parent live + sibling failed, near-simultaneous) — the design only cancels the shared `compileCtx` via `abandon()` on sibling failure, while the parent `ctx` is only ever done from an external cause, so `ctx.Err() == nil && errors.Is(err, context.Canceled)` cleanly separates "fallout from the sibling" from "the user/parent cancelled." No case found where a genuine failure gets misreported as abandonment or vice versa. `defer abandon()` covers every return path including panics; `wg.Wait()` happens-before the read of both errors, so no data race — confirmed with `-race` x3, clean. - **`backoff()`**: checked 0, 1, 31, 32, 63, 64, and `1<<30` — the shift is only computed for `1 <= attempt < 32`, so it never overflows (max shift is `2s<<30 ≈ 24.8 days`, still fits int64, and gets clamped to the 30s ceiling before use) and is skipped entirely (falls back to the 30s ceiling) for anything at or above 32, however large. `d` is always positive before the jitter call, so `rand.Int63n(d/2)` never sees a non-positive argument and the total is always > 0. Confirmed via the existing `TestBackoffStaysWithinBounds`. - **Flag validation**: `--retry` bounds and `--timeout` positivity are checked in `validate()`, called before `resolveEnvironment` or anything else touches the values; all reject with exit 3 as documented. No other numeric/duration flag has the same class of gap. - **Fixture scrub**: grepped the whole tree for the retired real certname/UUID/version hash/role and class names/paths — none remain. The one real-looking IP (`198.18.23.9`) was replaced with `192.0.2.9` (TEST-NET-1, documentation-reserved). The only remaining `*.unkin.net` hosts are the tool's actual intentional config defaults (`puppet.k8s.syd1.au.unkin.net`, `pdbmux.k8s.syd1.au.unkin.net`) and CI/module-path references — pre-existing, not fixture leakage, untouched by these commits. Wire-format shape is preserved: envelope, null `code_id`, string `version`, resources with/without `parameters`/`file`+`line`, `exported: true` case, and per-resource `sensitive_parameters` are all still exercised. - `fold()`'s `Warn` hook is nil-checked before use. ## Judgment calls - **`fold()` erroring when every record is undecodable**: agree with the choice. Silently returning an empty-but-"successful" fact map would let a completely broken fetch masquerade as a real (if factless) host, compiling a catalog that's quietly wrong rather than loudly failing. Loud failure is the safer default here. - **`maxRetry = 20` cap**: keep it. The overflow bug is already independently fixed in the `backoff()` rewrite (verified safe for any `int`, including very large values, since the shift is gated on `attempt < 32`), so the cap is no longer load-bearing for correctness — but it's a free, sensible operator guard against a typo like `--retry 999999` turning a single host lookup into an effectively-unbounded hang. No reason to drop it.

unkin-agent added 1 commit 2026-09-12 17:35:15 +10:00

stop serving another node's factset ... 5cbe4c8713

Also strip a leading BOM and stop reporting a certname mismatch as a parse error.

unkin-agent added 1 commit 2026-09-12 17:49:58 +10:00

read facts from PuppetDB's query API ... 3f14554fae

- rename internal/pdbmux to internal/puppetdb
- fetch facts with one GET /pdb/query/v4/facts AST query per node
- drop the per-node fallback route and its two response shapes
- rename --pdb to --puppetdb, CATALOG_DIFF_PDB to CATALOG_DIFF_PUPPETDB and
  pdb_url to puppetdb_url
- describe the fact source generically in the README and AGENTS.md

unkin-agent commented 2026-09-12 18:17:26 +10:00

Author

Member

Copy Link

Copy Source

Reviewed commit 3f14554 (rename internal/pdbmux→internal/puppetdb, switch to GET /pdb/query/v4/facts with an AST query).

Verdict: clean, no blocking issues.

• AST query ["=","certname",<host>] is built via json.Marshal then percent-encoded via url.Values.Encode(). Verified with the added test that a hostile certname (we"ird &host) survives both encodings intact — no injection/desync risk for weird certnames.
• 200 [] as the missing-node signal is handled correctly (decodeFacts empty → len(records)==0 → ErrNodeNotFound), kept distinct from other non-200 statuses. Can't be confused with a node that genuinely has facts, since PuppetDB only omits rows for a certname it has none for.
• Checked every deleted test against the surviving suite: each has a like-for-like replacement covering the same behavior (case-insensitive certname match, certname mismatch reporting, no-certname records kept, BOM tolerance, undecodable-record skip+warn). The deletions (bare-object factset shape, /nodes/<certname>/facts fallback) are exactly the dead paths the commit message says were dropped — nothing lost as collateral.
• The certname-mismatch guard (decodeFacts's "no facts for %s: puppetdb returned %s") is still reachable and exercised (TestFactsRejectsRecordsForAnotherNode, TestFactsRejectsAMixedResponseNamingEveryNode) — a reasonable defensive check to keep even though the query should filter server-side.
• Proxy-neutrality: grepped the tree — the only remaining pdbmux.*unkin.net occurrences are the default URL value (config.go) and its README/AGENTS.md mirrors, exactly as permitted. No comment, error string or test name still describes it as a proxy.
• No real estate hostnames/roles/IPs introduced by this commit.
• Minor/non-blocking: the explicit status == http.StatusNotFound branch in Facts() is probably unreachable against a real PuppetDB, which answers 200 [] rather than 404 for an unknown certname — harmless defensive code, not worth blocking on.

go test -race ./..., make lint, make pre-commit all green on this branch.

Reviewed commit `3f14554` (rename `internal/pdbmux`→`internal/puppetdb`, switch to `GET /pdb/query/v4/facts` with an AST query). **Verdict: clean, no blocking issues.** - AST query `["=","certname",<host>]` is built via `json.Marshal` then percent-encoded via `url.Values.Encode()`. Verified with the added test that a hostile certname (`we"ird &host`) survives both encodings intact — no injection/desync risk for weird certnames. - `200 []` as the missing-node signal is handled correctly (`decodeFacts` empty → `len(records)==0` → `ErrNodeNotFound`), kept distinct from other non-200 statuses. Can't be confused with a node that genuinely has facts, since PuppetDB only omits rows for a certname it has none for. - Checked every deleted test against the surviving suite: each has a like-for-like replacement covering the same behavior (case-insensitive certname match, certname mismatch reporting, no-certname records kept, BOM tolerance, undecodable-record skip+warn). The deletions (bare-object factset shape, `/nodes/<certname>/facts` fallback) are exactly the dead paths the commit message says were dropped — nothing lost as collateral. - The certname-mismatch guard (`decodeFacts`'s "no facts for %s: puppetdb returned %s") is still reachable and exercised (`TestFactsRejectsRecordsForAnotherNode`, `TestFactsRejectsAMixedResponseNamingEveryNode`) — a reasonable defensive check to keep even though the query should filter server-side. - Proxy-neutrality: grepped the tree — the only remaining `pdbmux.*unkin.net` occurrences are the default URL value (`config.go`) and its README/AGENTS.md mirrors, exactly as permitted. No comment, error string or test name still describes it as a proxy. - No real estate hostnames/roles/IPs introduced by this commit. - Minor/non-blocking: the explicit `status == http.StatusNotFound` branch in `Facts()` is probably unreachable against a real PuppetDB, which answers `200 []` rather than 404 for an unknown certname — harmless defensive code, not worth blocking on. `go test -race ./...`, `make lint`, `make pre-commit` all green on this branch.

benvin merged commit bd1ed2efb2 into main 2026-09-12 18:18:20 +10:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Something is not working

duplicate

This issue or pull request already exists

enhancement

New feature

help wanted

Need some help

invalid

Something is wrong

question

More information is needed

wontfix

This won't be fixed

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

benvin (BenVincent) unkin-agent (Unkin Agent) unkinben (Ben Vincent)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/puppet-catalog-tools#1