diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7d53381..6fbb682 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,24 +2,32 @@ name: CI # Pull requests run the test job. A merge to main runs the same test job # and then, only if it passed, builds the release artifacts and uploads -# them. Both live in one workflow so `needs:` can gate the build on the -# tests — a cross-workflow dependency would need `workflow_run`, which -# reports its status against the wrong commit and is easy to misread. +# them. A `v*` tag runs both and then publishes a GitHub Release from the +# artifacts the build job already verified. +# +# All three live in one workflow so `needs:` can gate each stage on the +# one before it — a cross-workflow dependency would need `workflow_run`, +# which reports its status against the wrong commit and is easy to +# misread. That gating is the point on a tag: a release is published only +# from a commit whose tests passed on every matrix leg. on: pull_request: push: branches: [main] + tags: ['v*'] -# Least privilege. Nothing here writes to the repository; add -# `contents: write` only if a job is later taught to publish a Release. +# Least privilege by default. Only the release job writes to the +# repository, and it raises its own permission locally rather than +# granting it to every job here. permissions: contents: read -# A new push to the same branch supersedes the previous run. Runs on main -# are never cancelled, so every merged commit keeps its artifacts. +# A new push to the same branch supersedes the previous run. Only pull +# requests are cancelled: a merged commit keeps its artifacts, and +# cancelling a tag run would leave a published tag with no release. concurrency: group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} jobs: test: @@ -80,6 +88,12 @@ jobs: name: build release artifacts needs: test runs-on: ubuntu-latest + # Consumed by the release job, so it publishes the version this job + # actually stamped rather than re-deriving it from the ref and + # risking a disagreement between the two. + outputs: + version: ${{ steps.version.outputs.version }} + prerelease: ${{ steps.version.outputs.prerelease }} steps: - uses: actions/checkout@v4 @@ -88,12 +102,39 @@ jobs: go-version-file: go.mod cache-dependency-path: go.sum + # A tag builds the version it names; anything else builds a + # commit-stamped development version that can never be mistaken for + # a release. docs/release.md's artifact names carry no `v`, so the + # prefix is stripped here rather than taught to the build script, + # which stays a plain `VERSION` interface usable by hand. + - name: Resolve the version to stamp + id: version + run: | + if [ "$GITHUB_REF_TYPE" = "tag" ]; then + version="${GITHUB_REF_NAME#v}" + # A malformed tag has to fail here. Downstream it becomes an + # artifact filename and the -X main.toolVersion stamp, where + # `piace-1.0-linux-amd64` would be published looking every bit + # as deliberate as a correct one. + if ! printf '%s' "$version" | grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z.]+)?$'; then + echo "::error::tag $GITHUB_REF_NAME is not vMAJOR.MINOR.PATCH[-prerelease]" + exit 1 + fi + case "$version" in *-*) prerelease=true ;; *) prerelease=false ;; esac + else + version="main-${GITHUB_SHA::7}" + prerelease=false + fi + echo "version=$version" >> "$GITHUB_OUTPUT" + echo "prerelease=$prerelease" >> "$GITHUB_OUTPUT" + echo "building version $version" + # Every artifact is CGO-free (requirements.md 12.1), so one Linux # runner cross-compiles the whole supported matrix. Running this on # pull requests too means a broken release script is found in review # rather than at release time. - name: Build artifacts and checksum manifest - run: bash scripts/build-release.sh "main-${GITHUB_SHA::7}" + run: bash scripts/build-release.sh "${{ steps.version.outputs.version }}" # docs/release.md tells a consumer to verify the manifest before # installing. Running that same check here proves the manifest CI @@ -114,16 +155,131 @@ jobs: || { echo "::error::$binary is not statically linked"; exit 1; } done - # Uploaded only for main: a pull request has already had the build + # Ties the artifact back to the tag. -X main.toolVersion is what + # `piace version` prints and what every result document records as + # its invocation metadata, so a mis-stamped binary would misreport + # itself in every report it ever produced — and the checksum + # manifest would happily certify it. + - name: Confirm the binary reports the version it was stamped with + env: + VERSION: ${{ steps.version.outputs.version }} + run: | + reported="$(./dist/piace-${VERSION}-linux-amd64 version)" + echo "$reported" + if [ "$reported" != "piace ${VERSION}" ]; then + echo "::error::binary reports '$reported', expected 'piace ${VERSION}'" + exit 1 + fi + + # Uploaded for pushes only: a pull request has already had the build # validated above, and does not need artifacts accumulating against - # the repository's storage. + # the repository's storage. On a tag this is also the handoff to the + # release job, which publishes these exact bytes rather than + # building its own. - name: Upload artifacts - if: github.event_name == 'push' && github.ref == 'refs/heads/main' + if: github.event_name == 'push' uses: actions/upload-artifact@v4 with: - name: piace-main-${{ github.sha }} + name: piace-${{ steps.version.outputs.version }} path: | dist/piace-* dist/SHA256SUMS retention-days: 30 if-no-files-found: error + + release: + name: publish release + needs: build + # github.ref_type is 'tag' only under the tags: ['v*'] trigger above, + # so a push to main builds and uploads as before and stops there. + if: github.ref_type == 'tag' + runs-on: ubuntu-latest + # The only job in this workflow that writes to the repository, and the + # only one holding the permission to. Granting it at the workflow + # level would hand it to the test job as well, which runs the code + # under review. + permissions: + contents: write + steps: + # The published bytes are the ones the build job already checked: + # manifest verified, static linking confirmed, version stamp + # asserted. Rebuilding here would publish artifacts that nothing had + # checked, and a compiler or toolchain difference between the two + # jobs would be invisible. + - name: Download the verified artifacts + uses: actions/download-artifact@v4 + with: + name: piace-${{ needs.build.outputs.version }} + path: dist + + # Re-verified after the round trip through artifact upload and + # download, so a truncated or corrupted transfer fails here rather + # than reaching a consumer who trusts the manifest. + - name: Re-verify the checksum manifest + working-directory: dist + run: sha256sum --check SHA256SUMS + + # The notes state plainly what this release does and does not prove. + # A checksum manifest published beside its own artifacts attests to + # integrity, never to origin: anyone who could replace the binaries + # could replace SHA256SUMS with them. Only the detached signature + # closes that gap, and this workflow holds no signing key, so the + # release says so instead of leaving a consumer to follow a + # verification step that cannot yet succeed. + - name: Compose the release notes + env: + VERSION: ${{ needs.build.outputs.version }} + run: | + { + echo "Statically linked, CGO-free binaries for linux/amd64, linux/arm64," + echo "darwin/amd64 and darwin/arm64." + echo + echo "## Verifying this download" + echo + echo '```sh' + echo "sha256sum --check --ignore-missing SHA256SUMS # shasum -a 256 on macOS" + echo "./piace-${VERSION}-- version" + echo '```' + echo + echo "\`SHA256SUMS.asc\` — the detached OpenPGP signature over the manifest — is" + echo "signed and attached separately after publication; this workflow holds no" + echo "signing key. Until it appears, the checksums above show only that a download" + echo "is intact, not where it came from. See \`docs/release.md\` for the full" + echo "procedure and the signing key fingerprint." + echo + echo "## SHA256SUMS" + echo + echo '```' + cat dist/SHA256SUMS + echo '```' + } > release-notes.md + cat release-notes.md + + # gh resolves the repository from GH_REPO, so this job needs no + # checkout: the only inputs are the downloaded artifacts and the + # notes composed above. The tag already exists — pushing it is what + # triggered the run — so gh attaches the release to it rather than + # creating one. + # + # Re-running this job after a release already exists fails, and is + # meant to. The alternative — falling back to `gh release upload + # --clobber` — would quietly overwrite the assets of a release + # people may already have downloaded, to rescue a case (a partial + # publish) that is rarer than the case it endangers. Delete the + # incomplete release and re-run if that happens. + - name: Publish the release + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + VERSION: ${{ needs.build.outputs.version }} + PRERELEASE: ${{ needs.build.outputs.prerelease }} + run: | + flags=() + if [ "$PRERELEASE" = "true" ]; then + flags+=(--prerelease) + fi + gh release create "$GITHUB_REF_NAME" \ + --title "piace $VERSION" \ + --notes-file release-notes.md \ + "${flags[@]}" \ + dist/piace-* dist/SHA256SUMS diff --git a/.kiro/specs/piace/design.md b/.kiro/specs/piace/design.md index 61f36c0..0aa9e4a 100644 --- a/.kiro/specs/piace/design.md +++ b/.kiro/specs/piace/design.md @@ -380,6 +380,38 @@ inlined CSS/JavaScript only; it makes target failure, v3 warning, exclusions, and final outcome visible without network access. HTML, text, and JSON derive from the same redacted projection, preventing format drift or secret exposure. +### 9.1 Display policy + +The three formats show the same document at three levels of detail, decided +through shared helpers so they cannot drift apart in what a value says. JSON +encodes the complete document and takes no options. HTML is complete too and +uses disclosure rather than omission: resource changes, edge changes, aggregate +groups, and each estimate's PQL, request options and full certname list are on +the page inside closed `
`. Every list of rows is closed and every +summary carries its count, so the page a reader lands on is an index of the run +— outcome, reasons, tally, and one line per target with a counted chip per +section — and one click reaches any of it. What stays outside every disclosure +is anything requirement 8.5 requires visibly marked (retrieval and compilation +failures, the v3 warning) and requirement 9.3's estimate label and note. Only +the text report omits, because a CI log is a linear read with nothing to +expand — it drops edge changes and each estimate's query mechanics, and caps an +estimate's certname sample unless `--impact-nodes` is passed. Requirements 5.3, +6.5, 7.4, 8.2, and 9.4 are discharged by JSON, and visibly by HTML as well. + +Two invariants keep this safe rather than lossy. Every section header counts +what it actually displays, not what the document holds. And a target whose only +differences are edges — `has_difference` true, no resource change — is never +rendered as unchanged: HTML shows the edges, and the text report prints an +explicit note, so no report reads as "nothing changed" on a run that exits +non-zero (requirements 10.2, 10.5). + +Requirement 8.3's "no HTTP server, a CDN, network access, or sibling assets" +also rules out webfonts and image files, so the HTML report uses system font +stacks with declared fallbacks and draws its disclosure markers in CSS. It +commits to one light palette rather than following the reader's system theme: +a review artifact is shared, printed, and pasted into tickets, and a single +appearance is a single thing to verify. + ## 10. Error taxonomy and outcomes PIACE records all target-local problems with an operation, safe reason, and diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..83c7460 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,118 @@ +# Changelog + +All notable changes to PIACE are recorded here. The format follows +[Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and the project +follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [Unreleased] + +## [0.1.0] - 2026-08-28 + +First release: the whole tool, so this entry describes what it does rather +than what changed. + +### Comparison + +- `piace compare` compares each target's **baseline catalog** (PuppetDB's + latest, or a local snapshot) against a **candidate catalog** compiled by an + existing Puppet Server or OpenVox compiler, and reports a per-node diff, a + cross-node aggregate view, and an optional PuppetDB-backed impact estimate. +- Deterministic semantic normalization: exact `Type[title]` identities with no + case folding, a canonical value domain with exact-decimal numbers, and + resources and edges sorted before comparison and serialization. +- Generated catalog noise is excluded before comparison — tags, source + file/line, `exported`, `aliases`, and the `alias` parameter the PuppetDB + terminus injects into a stored catalog. +- Four change kinds: resource added, resource removed, parameter changed, and + dependency-graph edge added/removed. A target whose only differences are + edges is still reported as changed. +- Managed `File` content evidence without content disclosure: inline `content` + digests, an authoritative compiled checksum, compiler retrieval of a + `source` reference, and the explicit `reference_changed` / + `content_indeterminate` states when bytes cannot be compared. No managed + file bytes reach any output, log, PQL query, or aggregate state. +- Configurable per-target exclusions (`Type[title]`, case-sensitive glob) and + redaction selectors, applied after equality so redaction cannot alter a + comparison. +- Impact estimates report only that a node's latest *stored* catalog contains + the exact `Type[title]`, bounded by a configured timeout and result limit, + and labelled as such in every format. + +### Catalog APIs + +- `catalog_api: v4` is the supported path. Every request carries + `persistence: {facts: false, catalog: false}`, so a comparison leaves the + target's stored factset and catalog untouched, and sends the target's own + trusted facts. +- `catalog_api: v3` is a degraded path, with a non-suppressible trusted-fact + warning in every output format, and an opt-in, never implicit, v4→v3 + fallback. + +### Snapshots + +- `piace capture facts` and `piace capture catalog` write PIACE envelopes — + format version, target identity, source, capture timestamp, SHA-256 payload + checksum, and a catalog's requested environment, compiler API version and + input factset identity — atomically, at `0600`, never overwriting without + `--replace`. Every field is validated on reuse. + +### Reports + +- Three formats from one redacted result document, so they cannot disagree: + **text** for a CI log (the only format that omits anything), **JSON** as the + complete versioned record, and **HTML** as a complete, self-contained page + with no JavaScript, webfonts, or images. +- The HTML report is an index of the run: every list of rows sits in a closed + disclosure whose heading counts what it holds. Failures, the v3 warning and + the outcome badges never collapse. + +### Outcomes + +- Exit codes `0` (clean / differences allowed), `10` + (policy-disallowed difference), `20` (compilation failure) and `30` + (operational error), with documented precedence. A run is never `clean` + while any target has an unreported retrieval, compilation, normalization or + content-verification failure. + +### Transport and secrecy + +- Independent hardened mTLS clients per service, with bounded timeouts, + response size limits, and no redirect following. +- Private keys, certificate material and authorization headers never reach a + diagnostic, report or log. +- `--debug` prints request metadata only (safe for CI); `--debug-dump-dir` + writes verbatim, unredacted bodies to `0600` files in a `0700` directory, + never to stdout or stderr. + +### Build and release + +- Dependency-free and CGO-free; Go 1.22+ is the only build requirement. +- CI runs `gofmt`, `go vet`, `go build` and `go test -race` on Linux and + macOS, cross-compiles the full platform matrix on every pull request, and + publishes a GitHub Release with `SHA256SUMS` from a `v*` tag. The detached + signature over the manifest is attached by hand afterwards — CI holds no + signing key, and the release notes say so. + +### Known limitations + +- **`catalog_api: v3` with `baseline.source: puppetdb` is not rejected.** + requirements.md 1.8 requires a v3 target to use a file baseline, but config + validation does not yet enforce it. The configuration loads and the run + overwrites the baseline it just read; the symptom on the next run is a + baseline-environment mismatch naming the candidate environment. Set + `baseline.source: file` yourself. See the README's + "If you must use v3, compare against a captured file". +- **Two PuppetDB impact-endpoint behaviours are unconfirmed against a + deployment**: that design §8's PQL text is accepted at the root + `/pdb/query/v4`, and that `limit`/`order_by` are honoured there. If + `order_by` is not honoured, a *truncated* impact sample is not reproducible. +- **The Puppet `Sensitive` wire shape** (`{"__ptype":"Sensitive","__pvalue":…}`) + is derived from Puppet's Ruby serializer source rather than a captured + response. A compiler emitting a different encoding would pass the acceptance + suite with the value unredacted. + +The last two are recorded as skipped tests carrying their confirmation +procedures in `cmd/piace/acceptance_assumptions_test.go`. + +[Unreleased]: https://github.com/example42/piace/compare/v0.1.0...HEAD +[0.1.0]: https://github.com/example42/piace/releases/tag/v0.1.0 diff --git a/README.md b/README.md index ee511eb..ec71f6c 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,18 @@ differences, a cross-node aggregate view, and an optional PuppetDB-backed estimate of a changed resource's wider stored-catalog footprint. PIACE is a client of PuppetDB and a compiler. It does not compile Puppet code -locally, embed a Puppet runtime, run agents, or write anything to PuppetDB. +locally, embed a Puppet runtime, run agents, or issue any write or command +request to PuppetDB — every PuppetDB request it makes is a read. + +That is not the same as "nothing changes server-side", and the difference is +the choice of catalog API. On the supported `catalog_api: v4` path nothing +changes: each candidate request carries `persistence: {facts: false, catalog: +false}` and the compiler stores neither the facts PIACE submitted nor the +catalog it compiled. On `catalog_api: v3` the *compiler* stores both, because +that endpoint has no persistence control — PIACE still writes nothing itself, +but the target's stored factset and catalog are rewritten as a side effect of +asking for a candidate. Read [Use `catalog_api: v4`](#use-catalog_api-v4) +before selecting v3. Terminology used throughout the code and reports is fixed in [CONTEXT.md](CONTEXT.md). @@ -47,7 +58,7 @@ Release artifacts and their checksum/signature workflow: ### Continuous integration [`.github/workflows/ci.yml`](.github/workflows/ci.yml) runs on every pull -request and on every push to `main`: +request, on every push to `main`, and on every `v*` tag: - **test** — `gofmt`, `go vet`, `go build`, and `go test -race -count=1` on Linux against the Go version `go.mod` declares and against current stable, @@ -56,16 +67,27 @@ request and on every push to `main`: `sha256sum`/`shasum` branch are where the two diverge. - **build** — gated on `test`. Cross-compiles the full supported platform matrix, verifies the generated `SHA256SUMS` manifest the same way - [docs/release.md](docs/release.md) tells a consumer to, and confirms the - Linux binaries are statically linked. It runs on pull requests too, so a - broken release script surfaces in review rather than at release time; - artifacts are uploaded only for `main`. + [docs/release.md](docs/release.md) tells a consumer to, confirms the Linux + binaries are statically linked, and checks each binary reports the version + it was stamped with. It runs on pull requests too, so a broken release + script surfaces in review rather than at release time; artifacts are + uploaded for pushes only. +- **release** — gated on `build`, and only on a tag. Publishes a GitHub + Release from the artifacts `build` produced, rather than rebuilding, so + what a consumer downloads is what CI checked. It is the only job granted + `contents: write`. + +Cutting a release is `git push origin v1.0.0`; a malformed tag fails before +anything is built. The detached signature is not automated — CI holds no +signing key — so it is attached by hand afterwards, and the release notes +say so rather than leaving a consumer following a verification step that +cannot yet succeed. See [docs/release.md](docs/release.md). ## Usage ``` piace compare --targets TARGETS.yaml --services SERVICES.yaml \ - [--text-out PATH] [--json-out PATH] [--html-out PATH] + [--text-out PATH] [--json-out PATH] [--html-out PATH] [--impact-nodes] piace capture facts --targets TARGETS.yaml --services SERVICES.yaml [--replace] @@ -77,6 +99,43 @@ Omitting `--text-out` writes the text report to stdout; JSON and HTML are produced only when explicitly requested. All three render from one redacted result document, so they cannot disagree. +They do not all show the same amount of it. JSON and HTML are complete; only +the text report omits anything. + +The HTML report keeps everything and collapses it. What you land on is an index +of the run: the outcome, the reasons, the tally, and one line per target with a +counted chip per section. Every list of rows — resource changes, dependency-graph +edges, aggregate groups, the exclusion detail, the provenance block, an estimate's +PQL, request options and full node list — is a closed section whose heading counts +what it holds, and one click opens any of it. A real run of four targets is under +two screens closed where it used to be seventy. Nothing is capped — a closed +section already keeps a thousand certnames out of the way without dropping a +name — and the page embeds the canonical JSON at the bottom as well. + +What never collapses is a failure: retrieval and compilation failures, the v3 +trusted-fact warning, the run diagnostics and every outcome badge stay in the +scanning path, because a mark you have to go looking for is not a visible one. +Printing expands the collapsed sections too, so a filed or pasted copy is the +same document as the one on screen; the canonical JSON is the one exception, +since it is that document a second time and half a megabyte of it on paper +serves nobody. + +It is one self-contained file with a light background, no webfonts, no images +and no JavaScript — expand and collapse is `
`: `file://` is all it +needs. + +The text report is the one that summarizes, because a CI log is a linear read +with nothing to expand. It omits edge changes — a consequence of the resource +changes, and routinely more numerous than them — and each estimate's PQL and +request options, and it names an estimate's first few certnames and counts the +rest. `--impact-nodes` names all of them, up to the configured `result_limit`; +it does not affect the HTML report, which never capped them. + +A target whose *only* differences are edges is still reported as changed: HTML +shows the edges, and the text report prints a count in place of the list. +Shortening a reading path must never make a run that exits non-zero read as if +nothing changed. + `capture catalog --environment ENV` requests the catalog for `ENV` — typically the production/default environment, captured after merge, so development-branch runs baseline against a frozen catalog rather than a later one from another @@ -123,7 +182,9 @@ version: 1 defaults: candidate: environment: feature-123 - catalog_api: v4 # v3 | v4 — v4 unless the compiler lacks it + catalog_api: v4 # v3 | v4 — v4 unless the compiler lacks it; + # v3 rewrites PuppetDB state, and needs + # baseline.source: file (see below) allow_v3_fallback: false # valid only with v4; opt-in, never implicit facts: source: puppetdb # puppetdb | file @@ -276,7 +337,8 @@ So with `catalog_api: v3`: — for the next target in the same run, and for every later run. The symptom is a baseline-environment mismatch that names the candidate environment. A v3 target needs `baseline.source: file`, captured while the baseline - environment's catalog was the stored one. + environment's catalog was the stored one — see + [If you must use v3, compare against a captured file](#if-you-must-use-v3-compare-against-a-captured-file). - **A file baseline does not make v3 harmless.** It stops PIACE from destroying its own input. It does not stop the compiler from writing the candidate facts and catalog into PuppetDB, where anything reading PuppetDB state — reporting, @@ -287,6 +349,57 @@ Puppet Server and OpenVox behave identically here: both serve v3 and v4, and both honour the v4 `persistence` field. `catalog_api` is the only thing that decides. +### If you must use v3, compare against a captured file + +The only workable shape for a v3 target is a **baseline that no longer comes +from PuppetDB**: a snapshot captured earlier, from disk, that the candidate +compilation cannot reach in to overwrite. Comparing against a snapshot is not a +workaround here — with v3 it is the only arrangement in which the baseline +survives the run that reads it. + +```yaml +defaults: + candidate: + environment: feature-123 + catalog_api: v3 + baseline: + source: file # required, not optional, with v3 + environment: production + file: snapshots/catalogs/{certname}.json +``` + +The snapshot is produced by `piace capture catalog`, which writes to the same +`baseline.file` path `compare` later reads: + +```sh +# once, from the baseline environment, while it is the deployed one +piace capture catalog --targets targets.yaml --services services.yaml \ + --environment production + +# then, per change, as often as you like +piace compare --targets targets.yaml --services services.yaml +``` + +Three things to keep straight: + +- **Capture from the baseline environment, and capture it fresh.** The snapshot + is the thing every later comparison is measured against; a stale one silently + reports drift that was already merged. Re-capture after each promotion to the + baseline environment — requirements.md 11.7 describes exactly this loop, CI + refreshing catalog snapshots from the default environment after a merge. +- **`capture catalog` compiles too, through the target's own `catalog_api`.** A + v3 capture therefore stores what it compiled — but it compiled the *baseline* + environment, which is what an agent run would have stored anyway, so the + damage a v3 compare does is absent here. Capturing with `catalog_api: v4` + avoids even that. +- **PIACE does not currently refuse `catalog_api: v3` with + `baseline.source: puppetdb`.** requirements.md 1.8 says it should; config + validation does not yet enforce it. The configuration loads, the first + comparison looks normal, and the run corrupts the baseline it just read — the + symptom on the next run is an operational error naming a baseline-environment + mismatch against the candidate environment. Set `baseline.source: file` + yourself; nothing will do it for you. + ## Two things the reports say, and mean literally **The v3 warning.** With `catalog_api: v3` — or any permitted v4→v3 fallback — @@ -300,8 +413,14 @@ compiler lookup is available. **The impact estimate.** It reports only that a node's latest *stored* catalog contains the exact `Type[title]`. It is not proof those nodes would change, and PIACE never compiles them. Queries are bounded by `timeout` and `result_limit`; -an over-limit result is marked truncated with a sorted certname sample. Because -an enabled estimate is requested analysis, a failed one is an operational error. +an over-limit result is marked truncated and reported as *more than* the limit, +never as an exact population, with a sorted certname sample. Because an enabled +estimate is requested analysis, a failed one is an operational error. + +The compact per-estimate line depends on the section header for its meaning: +`Service[nginx]: 9 nodes: …` is not a claim about those nodes, because the +fixed note above it says once, for the whole section, what a listed certname +does and does not mean. ## Output and secrecy @@ -359,6 +478,7 @@ still rests on. ## Further reading +- [CHANGELOG.md](CHANGELOG.md) — what each release contains - [CONTEXT.md](CONTEXT.md) — domain language - [.kiro/specs/piace/](.kiro/specs/piace/) — requirements, design, tasks - [docs/adr/0001-request-candidate-catalogs-from-an-existing-compiler.md](docs/adr/0001-request-candidate-catalogs-from-an-existing-compiler.md) diff --git a/cmd/piace/acceptance_impact_test.go b/cmd/piace/acceptance_impact_test.go index 5f59e62..852681e 100644 --- a/cmd/piace/acceptance_impact_test.go +++ b/cmd/piace/acceptance_impact_test.go @@ -59,10 +59,21 @@ func TestAcceptance_ImpactEstimateBoundsAndLabelling(t *testing.T) { } } - // requirements.md 9.4: the exact generated PQL is reported. - wantPQL := `resources[certname] { type = "Service" and title = "nginx" }` - if !strings.Contains(got.stdout, wantPQL) { - t.Errorf("the report does not carry the exact generated PQL:\n%s", got.stdout) + // requirements.md 9.4: the exact generated PQL is reported. It is + // discharged by the JSON report and by the canonical JSON the HTML + // artifact embeds; the text report and the HTML reading path omit it + // as repeated bulk (see internal/report's doc.go). Asserting it here + // against both artifacts is what keeps that trade honest — the + // obligation moved, it did not lapse. + wantPQL := `resources[certname] { type = \"Service\" and title = \"nginx\" }` + if !strings.Contains(got.json, wantPQL) { + t.Errorf("the JSON report does not carry the exact generated PQL:\n%s", got.json) + } + if !strings.Contains(got.html, template.HTMLEscapeString(wantPQL)) { + t.Errorf("the HTML artifact does not embed the exact generated PQL") + } + if strings.Contains(got.stdout, "resources[certname]") { + t.Errorf("the text report still prints the PQL:\n%s", got.stdout) } // requirements.md 9.6: truncation is marked and the sample is the @@ -180,3 +191,80 @@ func TestAcceptance_ImpactQueryWireShape(t *testing.T) { t.Errorf("order_by = %q", q["order_by"]) } } + +// TestAcceptance_ImpactNodesControlsTheCertnameSample covers the +// `--impact-nodes` option end to end. +// +// The default is capped because a bounded estimate may hold as many +// certnames as its configured `result_limit` — a thousand in a realistic +// deployment — and a section of several hundred estimates, each naming a +// thousand nodes, is not a CI log anyone reads. What the cap must never +// do is understate the estimate, so the count stays exact in both forms +// and only the names are elided. +func TestAcceptance_ImpactNodesControlsTheCertnameSample(t *testing.T) { + // A result limit above the returned count keeps the estimate + // untruncated, so this exercises the display cap rather than the + // query bound — two different elisions that must not be confused. + defaults := strings.Replace(impactDefaults, " result_limit: 2", " result_limit: 50", 1) + + nodes := []string{ + "db-01.example.test", "db-02.example.test", "db-03.example.test", + "db-04.example.test", "db-05.example.test", "db-06.example.test", + "db-07.example.test", + } + + // Both runs share ONE harness: an artifact records the service + // authorities it talked to, and a second harness listens on different + // ports, so two harnesses could never produce identical HTML however + // inert the flag was. + h := newHarness(t) + h.seedTarget("web-01.example.test", baseResources(), changedResources(), baseEdges()) + h.pdb.impactCertnames = nodes + h.writeConfigs(t, targetsYAML(defaults, target("web-01.example.test"))) + + run := func(t *testing.T, extra ...string) artifacts { + t.Helper() + got := h.compare(t, extra...) + if got.code != exitcode.Success { + t.Fatalf("exit = %d, want 0\nstderr:\n%s", got.code, got.stderr) + } + return got + } + + byDefault := run(t) + withFlag := run(t, "--impact-nodes") + + if !strings.Contains(byDefault.stdout, "7 nodes") { + t.Errorf("the capped line does not state the full node count:\n%s", byDefault.stdout) + } + if !strings.Contains(byDefault.stdout, "(+2 more)") { + t.Errorf("the capped line does not count the elided certnames:\n%s", byDefault.stdout) + } + if strings.Contains(byDefault.stdout, "db-07.example.test") { + t.Errorf("a certname past the cap was named:\n%s", byDefault.stdout) + } + // The elision is display-only: the artifact still holds every name. + if !strings.Contains(byDefault.json, "db-07.example.test") { + t.Error("the JSON report lost a certname the text report elided") + } + + if !strings.Contains(withFlag.stdout, "db-07.example.test") { + t.Errorf("--impact-nodes did not name every certname:\n%s", withFlag.stdout) + } + if strings.Contains(withFlag.stdout, "more)") { + t.Errorf("--impact-nodes still elided part of the sample:\n%s", withFlag.stdout) + } + + // The flag is a text-report control. The HTML artifact names every + // certname either way -- it collapses the list rather than capping it + // -- so the two runs must produce the same page, byte for byte. + // Asserting only that both contain db-07 would keep passing if display + // options were ever threaded back into report.HTML and capped there; + // asserting equality is what actually pins the documented contract. + if !strings.Contains(byDefault.html, "db-07.example.test") { + t.Error("the HTML report capped the certname list; it should disclose all of them") + } + if byDefault.html != withFlag.html { + t.Error("--impact-nodes changed the HTML artifact; it is a text-report control") + } +} diff --git a/cmd/piace/acceptance_test.go b/cmd/piace/acceptance_test.go index 035a68d..331f4e8 100644 --- a/cmd/piace/acceptance_test.go +++ b/cmd/piace/acceptance_test.go @@ -317,8 +317,17 @@ func TestAcceptance_ExclusionsSuppressDifferencesAndAreReported(t *testing.T) { if !strings.Contains(got.stdout, "excluded: Notify[noi*]") { t.Errorf("text report does not report the applied exclusion rule:\n%s", got.stdout) } - if !strings.Contains(got.stdout, "1 edge(s) suppressed") { - t.Errorf("the edge attached to an excluded resource was not suppressed and counted:\n%s", got.stdout) + // requirements.md 6.4/6.5: the edge attached to the excluded resource + // is suppressed and counted. The count is asserted against the JSON + // report because the text and HTML formats omit edge information + // entirely (see internal/report's doc.go); 6.5 asks for the counts in + // "machine-readable and human-readable output", and the human-readable + // half is the rule identity and its resource/parameter counts above. + if !strings.Contains(got.json, `"suppressed_edges":1`) { + t.Errorf("the edge attached to an excluded resource was not suppressed and counted:\n%s", got.json) + } + if strings.Contains(got.stdout, "edge(s) suppressed") { + t.Errorf("the text report still prints the suppressed-edge count:\n%s", got.stdout) } if !strings.Contains(got.html, "Excluded differences") { t.Error("the HTML report does not visibly mark excluded differences (requirements.md 8.5)") diff --git a/cmd/piace/main.go b/cmd/piace/main.go index a5bbddb..d6d7a88 100644 --- a/cmd/piace/main.go +++ b/cmd/piace/main.go @@ -72,11 +72,20 @@ func run(args []string, stdout, stderr *os.File) exitcode.Code { func usage() string { return `piace compare --targets TARGETS.yaml --services SERVICES.yaml \ - [--text-out PATH] [--json-out PATH] [--html-out PATH] + [--text-out PATH] [--json-out PATH] [--html-out PATH] [--impact-nodes] piace capture facts --targets TARGETS.yaml --services SERVICES.yaml piace capture catalog --targets TARGETS.yaml --services SERVICES.yaml \ --environment ENVIRONMENT +The text report summarizes for a CI log: it omits dependency-graph edge +changes and each impact estimate's PQL and request options, and names only +the first few certnames per estimate. The JSON and HTML reports are +complete -- HTML keeps everything, with the bulk behind expandable +sections. + --impact-nodes list every certname an impact estimate returned + instead of a capped sample; affects the text + report only (compare only) + Every subcommand also accepts: --debug print one line per service request to stderr (method, URL, status, duration, body sizes, response top-level @@ -93,7 +102,12 @@ type compareFlags struct { textOut string jsonOut string htmlOut string - debug debugFlags + // impactNodes is display policy for the text report only; it never + // reaches resolve.Config, because what PIACE queries and what PIACE + // prints are separate concerns and an estimate's certname sample is + // already bounded by the target's configured result_limit. + impactNodes bool + debug debugFlags } func runCompare(args []string, stdout, stderr *os.File) exitcode.Code { @@ -105,6 +119,7 @@ func runCompare(args []string, stdout, stderr *os.File) exitcode.Code { fs.StringVar(&f.textOut, "text-out", "", "path to write the text report (default: stdout)") fs.StringVar(&f.jsonOut, "json-out", "", "path to write the versioned JSON report") fs.StringVar(&f.htmlOut, "html-out", "", "path to write the static HTML report") + fs.BoolVar(&f.impactNodes, "impact-nodes", false, "list every certname an impact estimate returned instead of a capped sample (text report only)") f.debug.register(fs) if err := fs.Parse(args); err != nil { return exitcode.OperationalError @@ -206,6 +221,14 @@ func newCompareWorkflow(cfg resolve.Config, debugOpts []transport.Option) (*comp // contains no credentials, private material, managed content bytes, or // unredacted sensitive values by construction. func writeReports(f compareFlags, result model.Result, stdout *os.File) error { + // Display policy applies to the text report alone. report.JSON takes + // no options by design — it is the complete machine-readable record, + // and a flag that changed what it contained would make one run's + // artifact incomparable with another's — and report.HTML takes none + // because it shows everything too, using disclosure rather than + // omission to stay readable. + opts := report.Options{ImpactNodes: f.impactNodes} + if f.jsonOut != "" { data, err := report.JSON(result) if err != nil { @@ -226,7 +249,7 @@ func writeReports(f compareFlags, result model.Result, stdout *os.File) error { } } - text, err := report.Text(result) + text, err := report.Text(result, opts) if err != nil { return err } diff --git a/docs/release.md b/docs/release.md index 29fc90d..c98a771 100644 --- a/docs/release.md +++ b/docs/release.md @@ -27,7 +27,51 @@ release metadata (design.md section 11): they are published with the release and edited deliberately in `scripts/build-release.sh`, never discovered or downloaded at run time. -## Generating the artifacts +## Cutting a release + +Pushing a `v*` tag runs the whole thing: + +```sh +git tag v1.0.0 +git push origin v1.0.0 +``` + +A tagged run uses the workflow **as it exists at the tagged commit**, so +tag a commit that already carries `.github/workflows/ci.yml`. Tagging a +branch the workflow has not reached yet does nothing at all — no run, no +error, nothing in the Actions log — which is a confusing way to spend a +version number. + +`.github/workflows/ci.yml` then runs the test matrix, builds every +platform, verifies the manifest, confirms the Linux binaries are +statically linked and that each reports the version it was stamped with, +and publishes a GitHub Release with the binaries and `SHA256SUMS` +attached. The release job publishes the artifacts the build job produced +rather than rebuilding, so what a consumer downloads is what CI checked. +A tag that is not `vMAJOR.MINOR.PATCH[-prerelease]` fails before anything +is built; a tag whose version carries a `-suffix` is published as a +prerelease. + +**The signature is not part of that.** CI holds no signing key, so a +freshly published release contains two of the three files above. Sign the +manifest and attach it as the last step: + +```sh +gh release download v1.0.0 --pattern SHA256SUMS +gpg --armor --detach-sign --local-user SHA256SUMS +gh release upload v1.0.0 SHA256SUMS.asc +``` + +Until that lands, the published checksums show only that a download is +intact, not where it came from — a manifest published beside its own +artifacts attests to integrity, never to origin. The release notes say so +in as many words, so a consumer is not left following a verification step +that cannot yet succeed. + +## Generating the artifacts by hand + +CI runs exactly this, and it stays usable directly for an air-gapped or +out-of-band build: ```sh scripts/build-release.sh 1.0.0 diff --git a/internal/filecontent/doc.go b/internal/filecontent/doc.go index bdb032a..7e17e69 100644 --- a/internal/filecontent/doc.go +++ b/internal/filecontent/doc.go @@ -57,6 +57,11 @@ // rule distinguishing the two, and evidence.go's ResolveFileContentEvidence // doc comment for the precise decision tree. // +// Resolution stops between step 2 and step 3 for a directory or +// recursive File resource, whose `source` names a directory tree the +// compiler's file_content endpoint cannot serve at all; see "Sources +// that are not byte-comparable" below. +// // Every returned model.FileContentEvidence carries only an algorithm // name, digest hex strings, the evidence-source enum, and the // comparison state — never managed file bytes. Hashing always happens @@ -108,6 +113,51 @@ // diagnostic guarantees the retrieval failure is never silently dropped // even if a caller ignored the State value. // +// # Sources that are not byte-comparable +// +// A File resource with `ensure => directory`, or with any `recurse` +// value other than an explicit false, copies a whole directory tree: +// its `source` (e.g. `puppet:///modules/tp/run_info/`) names a +// directory, not a file. The compiler's file_content endpoint serves one +// file's bytes and nothing else -- a directory reference is rejected +// outright (verified against a deployed OpenVox compiler on 2026-08-28: +// GET /puppet/v3/file_content/modules/tp/run_info/?environment= +// returns HTTP 500 with an empty body, for both the trailing-slash and +// the bare form). Attempting step 3 for such a resource therefore cannot +// succeed, and reporting its inevitable failure as content_indeterminate +// would state that content evidence was *lost* when in truth +// byte-level content evidence was never the right evidence for this +// resource. +// +// ResolveFileContentEvidence therefore tests isDirectoryOrRecursive on +// both sides after step 2 and before step 3, and resolves the resource +// without any retrieval: +// +// - The `source` references differ (or one side has one and the other +// does not): FileContentReferenceChanged, with a *warning*-severity +// verify_content diagnostic. This is design.md section 7.2's +// "Source/reference changes are always reported without rendering +// their bytes" applied to the one case where rendering them is not +// merely undesirable but impossible. The severity is what +// distinguishes it from step 4: nothing failed, so +// model.OutcomeForDiagnostic must not turn the comparison into an +// operational error, while the diagnostic still records why no +// digest accompanies the change. +// - The references do not visibly differ, so some other +// content-bearing parameter did: FileContentIndeterminate with an +// error-severity diagnostic, exactly as step 4 would. There is +// neither a reference-level fact to report nor bytes to compare, and +// model.TargetResult.ClassifyOutcome's indeterminate check keeps that +// from collapsing into a clean run. +// +// This widens the reference_changed state beyond the "no ContentResolver +// was supplied" rule stated below: reference_changed now also covers a +// reference that changed and *cannot* be byte-compared by any resolver, +// not only one that changed with no resolver available to try. Both +// readings share the same meaning -- "the reference changed and no +// byte-level comparison stands behind that statement" -- and neither +// ever claims a verified content change. +// // # Identifying "a recognized compatible checksum" (step 2) // // Puppet's `File` resource type documents two related but distinct @@ -189,9 +239,12 @@ // application/octet-stream and HTTP 200, per Puppet's documented v3 // file_content endpoint (puppetlabs/puppet, api/docs/http_file_content.md): // "The file_content endpoint returns the contents of the specified -// file." A 404 response ("Not Found: Could not find file_content -// ") is documented for a missing file but was not exercised -// against a live compiler; nothing depends on the body text, because +// file." A missing file returns HTTP 404 with the documented body +// ({"message":"Not Found: Could not find file_content ", +// "issue_kind":"RESOURCE_NOT_FOUND"}), and a reference naming a +// directory returns HTTP 500 with an empty body -- both verified +// against a deployed OpenVox compiler on 2026-08-28. Nothing depends +// on either body, because // this package treats any non-2xx response as a retrieval failure // (step 4b), never // inspecting the response body for meaning, matching this package's diff --git a/internal/filecontent/evidence.go b/internal/filecontent/evidence.go index 8773a50..13a304c 100644 --- a/internal/filecontent/evidence.go +++ b/internal/filecontent/evidence.go @@ -31,6 +31,32 @@ const ( checksumValueParameter = "checksum_value" ) +// ensureParameter and recurseParameter are the two Puppet File parameters +// that decide whether a `source` reference names a single file at all. +// They are not content-bearing themselves -- the differ never collapses +// them into a content change -- but they gate step 3, because the +// compiler's file_content endpoint serves a file's bytes and nothing +// else; see doc.go's "Sources that are not byte-comparable" section. +const ( + ensureParameter = "ensure" + recurseParameter = "recurse" +) + +// directoryEnsureValue is the `ensure` value naming a directory, and +// recursiveRecurseValues is the set of `recurse` string values that turn +// a File resource into a recursive directory copy. Puppet documents +// `recurse`'s allowed values as true, false, remote, and inf ("inf" and a +// numeric depth being the deprecated spellings of unlimited/limited +// recursion); every spelling other than an explicit false means the +// `source` names a directory tree. +const directoryEnsureValue = "directory" + +var recursiveRecurseValues = map[string]bool{ + "true": true, + "remote": true, + "inf": true, +} + // recognizedChecksumAlgorithms is the exact set of `checksum` algorithm // names Puppet's `checksum_value` parameter documentation restricts // itself to ("Only md5, sha256, sha224, sha384 and sha512 are supported @@ -91,6 +117,17 @@ func ResolveFileContentEvidence( return evidence, nil } + // A directory or recursive File resource's `source` names a + // directory tree, not a file. Step 3 cannot compare it: the + // compiler's file_content endpoint serves a single file's bytes and + // rejects a directory reference outright. Resolve it here, before + // any retrieval is attempted, rather than issuing a request whose + // failure would be reported as if content evidence had been lost. + if isDirectoryOrRecursive(before) || isDirectoryOrRecursive(after) { + evidence, diag := resolveNonByteComparable(certname, identity, before, after) + return evidence, diag + } + // Step 3: retrieve whichever side needs it (a `source` reference, // when that side has no literal content), hash locally, and compare. rc := RetrievalContext{Certname: certname, Identity: identity, Environment: environment} @@ -118,7 +155,7 @@ func ResolveFileContentEvidence( // Step 4: comparable bytes could not be established for both sides. state, reason := classifyUnresolvedState(retriever, beforeRes, afterRes) - diag := verifyContentDiagnostic(certname, identity, reason) + diag := verifyContentDiagnostic(model.SeverityError, certname, identity, reason) return model.FileContentEvidence{State: state}, &diag } @@ -178,6 +215,64 @@ func getChecksumAlgorithm(params map[string]model.Value) string { return defaultChecksumAlgorithm } +// isDirectoryOrRecursive reports whether params describes a File +// resource whose `source` (if any) names a directory tree rather than a +// single file: `ensure => directory`, or any `recurse` value other than +// an explicit false. Both spellings matter independently -- a recursive +// File may leave `ensure` unset, and PIACE sees each side separately, so +// one side alone is enough to make byte comparison inapplicable. +// +// `recurse` arrives from a catalog as a JSON boolean, so the bool case is +// the common one; the string and numeric cases cover Puppet's documented +// "remote"/"inf" spellings and the deprecated numeric recursion depth. +func isDirectoryOrRecursive(params map[string]model.Value) bool { + if ensure, ok := getStringParam(params, ensureParameter); ok && + lowerASCII(ensure) == directoryEnsureValue { + return true + } + if params == nil { + return false + } + switch v := params[recurseParameter].(type) { + case bool: + return v + case string: + return recursiveRecurseValues[lowerASCII(v)] + case model.Number: + // A numeric recursion depth of 0 disables recursion, exactly as + // `recurse => false` does; any other depth enables it. + return string(v) != "0" + default: + return false + } +} + +// resolveNonByteComparable resolves a File resource whose content-bearing +// parameters changed but whose `source` names a directory tree, so no +// byte-level comparison is possible or meaningful. No retrieval is +// attempted; see doc.go's "Sources that are not byte-comparable" section +// for why this is reported as a reference change at warning severity +// rather than as a failed content verification. +func resolveNonByteComparable(certname string, identity model.ResourceIdentity, before, after map[string]model.Value) (model.FileContentEvidence, *model.Diagnostic) { + beforeRef, beforeHas := getReferenceParam(before, sourceParameter) + afterRef, afterHas := getReferenceParam(after, sourceParameter) + + if (beforeHas || afterHas) && (beforeRef != afterRef || beforeHas != afterHas) { + diag := verifyContentDiagnostic(model.SeverityWarning, certname, identity, + "content source reference changed on a directory or recursive File; byte-level content comparison does not apply to a directory source") + return model.FileContentEvidence{State: model.FileContentReferenceChanged}, &diag + } + + // The reference did not visibly change, so some other content-bearing + // parameter did (a checksum or checksum_value the compiler inlined on + // only one side, say). There is no reference-level fact to report and + // no bytes to compare, which is exactly content_indeterminate: this + // case keeps error severity so it cannot collapse into a clean run. + diag := verifyContentDiagnostic(model.SeverityError, certname, identity, + "content evidence changed on a directory or recursive File; byte-level content comparison does not apply to a directory source") + return model.FileContentEvidence{State: model.FileContentIndeterminate}, &diag +} + // sideResolution is the outcome of resolving one side (before or after) // of a File resource's content-bearing parameters toward a comparable // digest, per design.md section 7.2 step 3. @@ -235,10 +330,13 @@ func classifyUnresolvedState(retriever ContentRetriever, before, after sideResol // verifyContentDiagnostic builds a model.OperationVerifyContent // diagnostic identifying only the target, resource identity, and a safe // reason string -- never a parameter value, source reference, or file -// content. -func verifyContentDiagnostic(certname string, identity model.ResourceIdentity, reason string) model.Diagnostic { +// content. severity is the caller's: a retrieval that was attempted and +// failed is an error, while a comparison this package declines to attempt +// because bytes are not the right evidence for the resource at all is a +// warning (see doc.go). +func verifyContentDiagnostic(severity model.DiagnosticSeverity, certname string, identity model.ResourceIdentity, reason string) model.Diagnostic { return model.Diagnostic{ - Severity: model.SeverityError, + Severity: severity, Operation: model.OperationVerifyContent, Certname: certname, Message: identity.String() + ": " + reason, diff --git a/internal/filecontent/evidence_test.go b/internal/filecontent/evidence_test.go index 2084c3e..36dab92 100644 --- a/internal/filecontent/evidence_test.go +++ b/internal/filecontent/evidence_test.go @@ -371,6 +371,17 @@ func TestResolveFileContentEvidence_NeverLeaksContentAcrossAllStates(t *testing. before: fileParams(map[string]model.Value{"source": "puppet:///modules/example/" + sampleContentBytes}), after: fileParams(map[string]model.Value{"source": "puppet:///modules/example/other"}), }, + { + name: "directory_reference_changed", + before: fileParams(map[string]model.Value{ + "ensure": "directory", "recurse": true, + "source": "puppet:///modules/example/" + sampleContentBytes + "/", + }), + after: fileParams(map[string]model.Value{ + "ensure": "directory", "recurse": true, + "source": "puppet:///modules/example/other/", + }), + }, { name: "retrieval_failure", before: fileParams(map[string]model.Value{"source": "puppet:///modules/example/data.txt"}), @@ -389,3 +400,148 @@ func TestResolveFileContentEvidence_NeverLeaksContentAcrossAllStates(t *testing. }) } } + +// countingRetriever fails the test if it is ever asked for a digest. A +// directory or recursive File's source names a directory tree the +// compiler's file_content endpoint cannot serve, so resolution must stop +// before any retrieval is attempted. +type countingRetriever struct { + t *testing.T + calls int +} + +func (c *countingRetriever) Digest(_ context.Context, reference string, _ RetrievalContext) (DigestEvidence, error) { + c.t.Helper() + c.calls++ + c.t.Fatalf("retriever called for a directory/recursive File source %q", reference) + return DigestEvidence{}, nil +} + +func TestResolveFileContentEvidence_DirectoryRecursiveSource_ReferenceChangedWithoutRetrieval(t *testing.T) { + cases := []struct { + name string + beforeOverride map[string]model.Value + afterOverride map[string]model.Value + }{ + { + // The exact shape observed in a live comparison: a tp module + // directory copy whose source lost its trailing slash between + // the baseline and candidate environments. + name: "ensure_directory_bool_recurse", + beforeOverride: map[string]model.Value{ + "ensure": "directory", "recurse": true, + "source": "puppet:///modules/tp/run_info/", + }, + afterOverride: map[string]model.Value{ + "ensure": "directory", "recurse": true, + "source": "puppet:///modules/tp/run_info", + }, + }, + { + name: "recurse_remote_without_ensure", + beforeOverride: map[string]model.Value{ + "recurse": "remote", "source": "puppet:///modules/example/tree/", + }, + afterOverride: map[string]model.Value{ + "recurse": "remote", "source": "puppet:///modules/example/other/", + }, + }, + { + // Only one side is a directory: byte comparison still does not + // apply, because the two sides are not comparable as files. + name: "one_side_directory", + beforeOverride: map[string]model.Value{ + "ensure": "directory", "recurse": true, + "source": "puppet:///modules/example/tree/", + }, + afterOverride: map[string]model.Value{ + "source": "puppet:///modules/example/tree/file.txt", + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + retriever := &countingRetriever{t: t} + evidence, diag := ResolveFileContentEvidence(context.Background(), "web-01", "upstream", + model.ResourceIdentity{Type: "File", Title: "info scripts"}, + fileParams(tc.beforeOverride), fileParams(tc.afterOverride), retriever) + + if retriever.calls != 0 { + t.Fatalf("retriever calls = %d, want 0", retriever.calls) + } + if evidence.State != model.FileContentReferenceChanged { + t.Errorf("State = %q, want %q", evidence.State, model.FileContentReferenceChanged) + } + if evidence.Algorithm != "" || evidence.BeforeDigest != "" || evidence.AfterDigest != "" { + t.Errorf("digest fields must stay empty, got %+v", evidence) + } + if diag == nil { + t.Fatal("diagnostic = nil, want a verify_content diagnostic") + } + if diag.Severity != model.SeverityWarning { + t.Errorf("Severity = %q, want %q", diag.Severity, model.SeverityWarning) + } + if diag.Operation != model.OperationVerifyContent { + t.Errorf("Operation = %q, want %q", diag.Operation, model.OperationVerifyContent) + } + // A warning must not turn the target into an operational error. + if _, contributes := model.OutcomeForDiagnostic(*diag); contributes { + t.Error("a not-applicable content comparison must not contribute to the outcome") + } + }) + } +} + +func TestResolveFileContentEvidence_DirectoryUnchangedSource_Indeterminate(t *testing.T) { + retriever := &countingRetriever{t: t} + before := fileParams(map[string]model.Value{ + "ensure": "directory", "recurse": true, + "source": "puppet:///modules/tp/run_info/", "checksum_value": "abc", + }) + after := fileParams(map[string]model.Value{ + "ensure": "directory", "recurse": true, + "source": "puppet:///modules/tp/run_info/", + }) + + evidence, diag := ResolveFileContentEvidence(context.Background(), "web-01", "upstream", + model.ResourceIdentity{Type: "File", Title: "info scripts"}, before, after, retriever) + + if retriever.calls != 0 { + t.Fatalf("retriever calls = %d, want 0", retriever.calls) + } + if evidence.State != model.FileContentIndeterminate { + t.Errorf("State = %q, want %q", evidence.State, model.FileContentIndeterminate) + } + if diag == nil || diag.Severity != model.SeverityError { + t.Fatalf("want an error-severity diagnostic, got %+v", diag) + } +} + +func TestIsDirectoryOrRecursive(t *testing.T) { + cases := []struct { + name string + params map[string]model.Value + want bool + }{ + {"plain file", map[string]model.Value{"source": "puppet:///modules/example/a.txt"}, false}, + {"ensure file", map[string]model.Value{"ensure": "file"}, false}, + {"ensure Directory mixed case", map[string]model.Value{"ensure": "Directory"}, true}, + {"recurse bool true", map[string]model.Value{"recurse": true}, true}, + {"recurse bool false", map[string]model.Value{"recurse": false}, false}, + {"recurse string true", map[string]model.Value{"recurse": "true"}, true}, + {"recurse string false", map[string]model.Value{"recurse": "false"}, false}, + {"recurse remote", map[string]model.Value{"recurse": "remote"}, true}, + {"recurse inf", map[string]model.Value{"recurse": "inf"}, true}, + {"recurse depth 0", map[string]model.Value{"recurse": model.Number("0")}, false}, + {"recurse depth 2", map[string]model.Value{"recurse": model.Number("2")}, true}, + {"nil params", nil, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isDirectoryOrRecursive(tc.params); got != tc.want { + t.Errorf("isDirectoryOrRecursive = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/internal/normalize/catalog_test.go b/internal/normalize/catalog_test.go index 1121d95..ccdee8d 100644 --- a/internal/normalize/catalog_test.go +++ b/internal/normalize/catalog_test.go @@ -157,6 +157,59 @@ func TestCatalog_DropsTagsFileLineAndOtherMetadata(t *testing.T) { } } +// TestCatalog_DropsAliasParameter verifies the `alias` metaparameter the +// PuppetDB terminus injects into a stored catalog is dropped from both +// wire shapes, so a PuppetDB baseline and a compiled candidate do not +// differ by it alone (requirements.md 5.9; see doc.go). +func TestCatalog_DropsAliasParameter(t *testing.T) { + resources := `[{"type":"File","title":"info scripts","parameters":{"path":"/etc/tp/run_info","alias":["/etc/tp/run_info"]}}]` + + for _, tc := range []struct { + name string + raw puppetdb.Catalog + }{ + {"puppetdb shape", pdbShapedCatalog("web-01.example.test", "production", resources, `[]`)}, + {"compiler shape", compilerShapedCatalog("web-01.example.test", "production", resources, `[]`)}, + } { + t.Run(tc.name, func(t *testing.T) { + got, diag := Catalog(tc.raw) + if diag != nil { + t.Fatalf("unexpected diagnostic: %+v", diag) + } + params := got.Resources[0].Parameters + if _, ok := params["alias"]; ok { + t.Errorf("alias parameter was not dropped: %+v", params) + } + if params["path"] != "/etc/tp/run_info" { + t.Errorf("Parameters[path] = %v, want the declared value preserved", params["path"]) + } + if len(params) != 1 { + t.Errorf("Parameters = %+v, want exactly {path: ...}", params) + } + }) + } +} + +// TestCatalog_ResourceWithOnlyAliasParameter verifies a resource whose +// only parameter is the dropped `alias` (Stage[main] and Class[main] are +// exactly this in a stored catalog) normalizes to an empty parameter map, +// not to a diagnostic or a resource carrying a leftover key. +func TestCatalog_ResourceWithOnlyAliasParameter(t *testing.T) { + raw := pdbShapedCatalog("web-01.example.test", "production", + `[{"type":"Stage","title":"main","parameters":{"alias":["main"]}}]`, `[]`) + + got, diag := Catalog(raw) + if diag != nil { + t.Fatalf("unexpected diagnostic: %+v", diag) + } + if len(got.Resources) != 1 { + t.Fatalf("Resources = %+v", got.Resources) + } + if len(got.Resources[0].Parameters) != 0 { + t.Errorf("Parameters = %+v, want empty", got.Resources[0].Parameters) + } +} + // TestCatalog_RejectsMalformedResourcesShape verifies an unrecognized // "resources" shape (neither object nor array) produces a reported // model.OperationNormalize diagnostic and a zero NormalizedCatalog, diff --git a/internal/normalize/doc.go b/internal/normalize/doc.go index dabf7bc..d1aa2dd 100644 --- a/internal/normalize/doc.go +++ b/internal/normalize/doc.go @@ -93,6 +93,36 @@ // must keep its own reference to the raw puppetdb.Catalog rather than // recovering it from a NormalizedCatalog. // +// One *parameter* is dropped for the same reason, and it is the only one: +// Puppet's `alias` metaparameter. The PuppetDB terminus injects it into a +// stored catalog's `parameters` object for every resource whose namevar +// differs from its title, recording the catalog-internal alias index as +// if it were a declared attribute; a compiler's own catalog response +// carries no such parameter. Measured against a deployed OpenVox +// installation on 2026-08-28 for one node: the PuppetDB-stored catalog +// carried `alias` on 9 of 53 resources (`Stage[main]`, `Class[main]`, +// `File[info scripts]`, ...), while the same node's freshly compiled +// catalog carried it on 0 of 40 through both the v3 and the v4 endpoint — +// and `alias` was the *only* parameter present on one side and absent on +// the other. Comparing a PuppetDB baseline against a compiled candidate +// therefore reported a spurious `alias: [...] -> null` parameter change +// for roughly a quarter of the shared resources: exactly the "generated +// noise" requirements.md 5.9 excludes ("catalog metadata unrelated to +// managed file content"). +// +// Dropping it cannot hide a real difference. `alias` only registers +// additional keys in the compiler's own resource index so that +// `File['/etc/tp/run_info']` resolves to `File['info scripts']` during +// compilation and relationship resolution; it is never enforced on a +// node, and a change to it cannot alter anything an agent does to a +// system. The drop is symmetric — applied to whichever wire shape is +// being normalized, not conditionally to the PuppetDB one — because a +// file baseline captured from PuppetDB carries `alias` too, and a +// shape-conditional filter would let the same asymmetry back in through a +// snapshot. See value.go's generatedMetadataParameters, which is that +// list and is deliberately not generalized beyond the one parameter +// actually measured to cause this. +// // # Canonical parameter values and Property 1 // // Each parameter value is converted into the model.Value domain (nil, diff --git a/internal/normalize/value.go b/internal/normalize/value.go index 459f67f..e0b597c 100644 --- a/internal/normalize/value.go +++ b/internal/normalize/value.go @@ -9,9 +9,27 @@ import ( "github.com/example42/piace/internal/snapshot" ) +// generatedMetadataParameters names the resource parameters this package +// drops as generated catalog metadata rather than managed configuration, +// per requirements.md 5.9 ("THE CLI SHALL exclude generated/noise-oriented +// fields from the semantic diff: tags, source file/line information, and +// catalog metadata unrelated to managed file content"). See doc.go's +// "What is dropped, and why that is safe" section for the full rationale +// and the measurements behind it. +var generatedMetadataParameters = map[string]bool{ + "alias": true, +} + +// isGeneratedMetadataParameter reports whether name is a parameter +// decodeParameters drops on both sides of a comparison. +func isGeneratedMetadataParameter(name string) bool { + return generatedMetadataParameters[name] +} + // decodeParameters decodes a resource's raw "parameters" JSON object into // the model.Value domain, canonicalizing every numeric value with -// snapshot.CanonicalNumberString along the way. A missing/empty +// snapshot.CanonicalNumberString along the way, and dropping every +// parameter isGeneratedMetadataParameter names. A missing/empty // "parameters" field decodes to an empty (nil) parameter map rather than // an error: PuppetDB's documented catalog wire format v8 states "Puppet // will only provide Booleans, strings, arrays, and hashes... Attributes @@ -41,6 +59,9 @@ func decodeParameters(raw json.RawMessage) (map[string]model.Value, error) { } out := make(map[string]model.Value, len(decoded)) for k, v := range decoded { + if isGeneratedMetadataParameter(k) { + continue + } cv, err := canonicalizeRaw(v) if err != nil { return nil, fmt.Errorf("parameter %q: %w", k, err) diff --git a/internal/report/doc.go b/internal/report/doc.go index d05b1f9..d0ee72d 100644 --- a/internal/report/doc.go +++ b/internal/report/doc.go @@ -21,17 +21,67 @@ // drift even when both are safe, so both formats call formatValue, and // both label the same sections with the same constants. // -// # Determinism -// -// design.md Property 1 requires byte-identical output for identical -// inputs. model.Result carries three map fields (ConfigProvenance's -// Candidate/Facts/Baseline/ImpactEstimate projections). Go's -// encoding/json sorts map keys, so the JSON path is safe automatically — -// but a `range` over a map in a text or HTML renderer is not, so every -// map here is iterated through sortedKeys. Everything else in a Result is -// already ordered by the package that produced it (targets by certname, -// aggregate groups by kind and identity, estimates by identity, certname -// samples locally sorted). +// # Three formats, three amounts of detail +// +// The three formats show the same document at three levels of detail. +// This is display policy, not a second projection: nothing here filters +// or recomputes what a comparison found, and every format decides through +// the same shared helpers (formatValue, changeSummary/changeParts, +// estimateCount, targetCountList), so they cannot drift apart in what a +// value says. +// +// - JSON is the complete record and takes no options at all. +// - HTML is complete too, and uses disclosure rather than omission: +// resource changes, edge changes, aggregate groups, an estimate's PQL, +// request options and full certname list are all on the page, inside +// closed
. Every list of rows is closed and every summary +// carries the count of what it holds, so the page a reader lands on is +// an index of the run — the outcome, the reasons, the tally, and one +// line per target with a counted chip per section — and one click +// reaches any of it. Nothing is capped, because a closed disclosure +// already keeps a thousand certnames out of the reading path without +// dropping a name. What stays outside every disclosure is anything +// requirements.md 8.5 requires visibly marked (see below) and the +// estimate label and note requirement 9.3 requires. +// - Text is the only format that omits, because a CI log is a linear +// read with no way to skip a section and no way to expand one. It +// drops edge changes (a run's edge differences routinely outnumber +// its resource differences, being a consequence of them), an +// estimate's PQL and request options (identical in shape on every +// line of a section that can run to hundreds of entries), and an +// estimate's certnames past Options.inlineCertnameCap unless +// Options.ImpactNodes is set. The count is never elided, only names. +// +// So requirements.md 5.3 and 7.4 (edges identified and retained through +// aggregation as a distinct kind), 6.5 (suppressed-difference counts), +// 8.2 ("complete node diffs"), and 9.4 ("the exact generated PQL query") +// are discharged by the JSON report and, for everything but the JSON +// envelope itself, visibly by the HTML report as well. The HTML artifact +// also embeds the canonical JSON in its closing disclosure, so the page +// is a complete record twice over. +// +// One consequence has to be handled explicitly rather than by omission. +// model.NodeDiff.HasDifference is true for a target whose only +// differences are edges, and that target still drives the run's outcome +// and exit code. HTML renders those edges, so nothing is needed there; +// the text report prints a note instead of an empty change list, because +// a report that showed nothing would read as "no changes" on a run that +// exits non-zero, contradicting its own stated outcome +// (requirements.md 10.2) and brushing 10.5. For the same reason every +// section header in both formats counts what it actually displays rather +// than what the document holds. +// +// # A light page, and nothing to fetch +// +// requirements.md 8.3's "no HTTP server, a CDN, network access, or +// sibling assets" is stronger than it first reads: it also rules out a +// webfont and an image file. The HTML report is therefore built from +// system font stacks with declared fallbacks, and its only piece of +// iconography — the disclosure triangle — is drawn with CSS borders +// rather than set in a glyph a reader's machine may not have. The page +// commits to a single light palette rather than following the reader's +// system theme: a review artifact gets shared, printed, and pasted into +// tickets, and one appearance is one thing to check. // // # requirement 9.3's label // @@ -46,6 +96,13 @@ // will change: the estimate says only that a node's latest stored catalog // contains the resource. // +// The compact per-estimate line depends on that section header for its +// meaning. "Class[Foo]: 9 nodes: ..." is not a claim about those nodes on +// its own, because ImpactEstimateNote stands immediately above it and +// says, once for the whole section, what a listed certname does and does +// not mean. Any format that ever prints an estimate line without that +// header would be stating something requirement 9.3 forbids. +// // # HTML safety // // requirements.md 8.3 requires an artifact that opens over `file://` diff --git a/internal/report/html.go b/internal/report/html.go index 6862fc0..96e7f93 100644 --- a/internal/report/html.go +++ b/internal/report/html.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "html/template" + "strings" "github.com/example42/piace/internal/exitcode" "github.com/example42/piace/internal/model" @@ -16,10 +17,27 @@ import ( // // Self-containment is structural, not a review promise: the template is a // package constant with one inlined
-

PIACE report

-

{{.Outcome}} - exit code {{.ExitCode}} · piace {{.ToolVersion}} · {{.TimestampUTC}}

-{{if .Compiler}} -

compiler: {{.Compiler}} · puppetdb: {{.PuppetDB}}

-{{end}} -{{if .Reasons}} -
    {{range .Reasons}}
  • — {{.}}
    • {{end}}
-{{end}} +
+
+

PIACE report

+ {{.Outcome}} +
+

exit {{.ExitCode}} · piace {{.ToolVersion}} · {{.TimestampUTC}}{{if .Compiler}} · compiler {{.Compiler}} · puppetdb {{.PuppetDB}}{{end}}

+ {{if .Tally}} +
+ {{range .Tally}}
{{.Count}}{{.Label}}
{{end}} +
+ {{end}} + {{if .Reasons}} +
    {{range .Reasons}}
  • {{.}}
    • {{end}}
+ {{end}} +
-

Targets ({{.TotalTargets}})

+

Targets {{.TotalTargets}}

{{range .Targets}} -
-

{{.Certname}} {{.Outcome}}

+
+
+

{{.Certname}}

+ {{.Outcome}} +
{{if .V3Warning}} @@ -80,74 +496,143 @@ summary { cursor: pointer; font-size: .9rem; } {{end}} {{if not .Compared}} -

No node diff was produced for this target.

+

No node diff was produced for this target.

{{else if not .HasDifference}} -

No non-excluded differences.

- {{else}} -
    {{range .Changes}}
  • {{.}}
    • {{end}}
+

No non-excluded differences.

{{end}} - {{if .Exclusions}} - - {{end}} +
+ {{if .Changes}} +
+ Resource changes {{len .Changes}} +
+
    + {{range .Changes}} +
  • + {{.Sign}} + {{.Identity}}{{if .Parameter}}{{.Parameter}}{{end}}{{if .HasValues}}{{.Before}}→{{.After}}{{end}}{{if .Note}}{{.Note}}{{end}} +
    • + {{end}} +
+
+
+ {{end}} + + {{if .EdgeChanges}} +
+ Dependency-graph edges {{len .EdgeChanges}} +
+
    + {{range .EdgeChanges}} +
  • + {{.Sign}} + {{.Source}}→{{.Target}} +
    • + {{end}} +
+
+
+ {{end}} + + {{if .Exclusions}} +
+ Excluded differences {{len .Exclusions}} +
+
    {{range .Exclusions}}
  • {{.}}
    • {{end}}
+
+
+ {{end}} -
- Provenance and resolved configuration - {{if .Baseline}}

baseline

{{range .Baseline}}
{{.Key}}
{{.Value}}
{{end}}
{{end}} - {{if .Facts}}

facts

{{range .Facts}}
{{.Key}}
{{.Value}}
{{end}}
{{end}} - {{if .Candidate}}

candidate

{{range .Candidate}}
{{.Key}}
{{.Value}}
{{end}}
{{end}} - {{if .Config}}

configuration

{{range .Config}}
{{.Key}}
{{.Value}}
{{end}}
{{end}} - {{if .Exclude}}

exclusion rules

    {{range .Exclude}}
  • {{.}}
    • {{end}}
{{end}} - {{if .Redact}}

redaction selectors

    {{range .Redact}}
  • {{.}}
    • {{end}}
{{end}} -
+
+ Provenance and resolved configuration +
+ {{if .Baseline}}

baseline

{{range .Baseline}}
{{.Key}}
{{.Value}}
{{end}}
{{end}} + {{if .Facts}}

facts

{{range .Facts}}
{{.Key}}
{{.Value}}
{{end}}
{{end}} + {{if .Candidate}}

candidate

{{range .Candidate}}
{{.Key}}
{{.Value}}
{{end}}
{{end}} + {{if .Config}}

configuration

{{range .Config}}
{{.Key}}
{{.Value}}
{{end}}
{{end}} + {{if .Exclude}}

exclusion rules

    {{range .Exclude}}
  • {{.}}
    • {{end}}
{{end}} + {{if .Redact}}

redaction selectors

    {{range .Redact}}
  • {{.}}
    • {{end}}
{{end}} +
+
+
{{end}} -

Aggregate diff ({{.TotalGroups}})

-{{if not .Aggregate}}

No grouped changes.

{{end}} -
    -{{range .Aggregate}} -
  • - {{.Label}} - {{if .HasValues}}
    {{.Before}} → {{.After}}
    {{end}} -
    {{len .Certnames}} target(s): {{range $i, $c := .Certnames}}{{if $i}}, {{end}}{{$c}}{{end}}
    -
    • +

    Aggregate diff {{.TotalGroups}}

    +{{if or .Aggregate .EdgeAggregate}} +
    +
    + {{if .Aggregate}} +
    + Grouped resource changes {{.TotalGroups}} +
    +
      + {{range .Aggregate}} +
    • + {{.Sign}} + {{.Identity}}{{if .Parameter}}{{.Parameter}}{{end}}{{if .HasValues}}{{.Before}}→{{.After}}{{end}}{{.Targets}} +
      • + {{end}} +
    +
    +
    + {{end}} + {{if .EdgeAggregate}} +
    + Dependency-graph edge groups {{.TotalEdgeGroups}} +
    +
      + {{range .EdgeAggregate}} +
    • + {{.Sign}} + {{.Source}}→{{.Target}}{{.Targets}} +
      • + {{end}} +
    +
    +
    + {{end}} +
    +
    +{{else}} +

    No grouped resource changes.

    {{end}} -
{{if .Estimates}} -

{{.EstimateLabel}} ({{.TotalEstimates}})

+

{{.EstimateLabel}} {{.TotalEstimates}}

-
    -{{range .Estimates}} -
  • - {{.Identity}} - {{.Status}} -
    {{.Summary}}
    -
    pql: {{.PQL}}
    -
    request: {{.Request}}
    - {{if .Certnames}} -
    nodes whose latest stored catalog contains this resource: - {{range $i, $c := .Certnames}}{{if $i}}, {{end}}{{$c}}{{end}}
    - {{end}} -
    • -{{end}} -
+
+
+
+ Queried resources {{.TotalEstimates}}{{if .TotalEstimateFailures}}{{.TotalEstimateFailures}} failed{{end}} +
+ {{range .Estimates}} +
+ {{.Identity}}{{if .Failed}}{{.Status}}{{end}}{{.Count}} +
+ {{if .Certnames}}

{{.NodeLabel}} whose latest stored catalog contains this resource

{{.Certnames}}

{{end}} +

pql

{{.PQL}}

+

request

{{.Request}}

+
+
+ {{end}} +
+
+
+
{{end}} {{if .Diagnostics}} -

Run diagnostics

+

Run diagnostics {{len .Diagnostics}}

{{range .Diagnostics}} {{end}} {{end}}

Result document

-
- Canonical JSON (schema-versioned, identical to the --json-out artifact) - 
{{.CanonicalJSON}}
+
+ Canonical JSON — schema-versioned, identical to the --json-out artifact + 
{{.CanonicalJSON}}
diff --git a/internal/report/html_test.go b/internal/report/html_test.go index 503012f..2473b22 100644 --- a/internal/report/html_test.go +++ b/internal/report/html_test.go @@ -90,23 +90,185 @@ func TestHTML_VisiblyMarksRequiredStates(t *testing.T) { } } -// TestHTML_EdgeGroupRendersWithoutAResourceIdentity guards the shape -// split in model.AggregateChangeKey: an edge group has a nil Identity and -// no before/after values, and must render rather than panic. -func TestHTML_EdgeGroupRendersWithoutAResourceIdentity(t *testing.T) { +// TestAggregateKeyLabel_HandlesAnEdgeKeysNilIdentity guards the shape +// split in model.AggregateChangeKey: an edge key sets Edge and leaves +// Identity nil, so a caller that dereferences Identity unconditionally +// panics. +// +// The HTML and text renderers no longer reach this branch — they filter +// edge groups out first — but the hazard is structural, not situational, +// so it is tested directly at the helper rather than through a format +// that happens to exercise it today. +func TestAggregateKeyLabel_HandlesAnEdgeKeysNilIdentity(t *testing.T) { + key := model.AggregateChangeKey{Kind: model.ChangeEdgeRemoved, Edge: &model.Edge{Source: "Class[a]", Target: "Class[b]"}} + if got := aggregateKeyLabel(key); got != "edge_removed Class[a] -> Class[b]" { + t.Errorf("aggregateKeyLabel = %q", got) + } +} + +// TestHTML_KeepsEverythingBehindDisclosure is this format's half of the +// display contract. Text drops edge changes, an estimate's PQL and +// request options, and the certnames past a cap; HTML keeps all of it and +// uses
instead — so every string the text test asserts is +// ABSENT must be present here, in the page itself rather than only in the +// canonical JSON embedded at the bottom. +func TestHTML_KeepsEverythingBehindDisclosure(t *testing.T) { + data, err := HTML(sampleResult()) + if err != nil { + t.Fatalf("HTML: %v", err) + } + out := string(data) + visible := out[:strings.Index(out, "

Result document

")] + + for _, want := range []string{ + "Class[a]", // a per-target edge change + "Class[b]", // ...and its other endpoint + "1 edge(s) suppressed", // requirements.md 6.5, in full + `resources[certname] { type = "Service"`, // requirements.md 9.4 + "/pdb/query/v4", // requirements.md 9.7 request scope + "order_by", // ...and its options + "db-01.example.test, db-02.example.test", // the uncapped certname sample + } { + if !strings.Contains(visible, want) { + t.Errorf("HTML report does not show %q above the result document", want) + } + } + + // Present is not enough: the bulk has to be collapsed, or the page is + // just the old wall of text with nicer colors. + for _, summary := range []string{ + "Resource changes", // per-target resource changes + "Dependency-graph edges", // per-target edges + "Grouped resource changes", // aggregate resource groups + "Dependency-graph edge groups", // aggregate edge groups + "Queried resources", // the estimate list + "Excluded differences", // requirements.md 8.5 + } { + if !strings.Contains(visible, summary) { + t.Errorf("HTML report has no disclosure headed %q", summary) + } + } + // ...and no list of rows is in the scanning path: a section left open + // buries every section after it, which on a real run means four + // figures of rows above the outcome a reader came for. + if strings.Contains(visible, "
is not visible. Everything else on a +// target may collapse; these may not. +// +// TestHTML_VisiblyMarksRequiredStates cannot catch this — it substring +// searches, and collapsed content still matches. +func TestHTML_KeepsFailuresOutOfDisclosure(t *testing.T) { + data, err := HTML(sampleResult()) + if err != nil { + t.Fatalf("HTML: %v", err) + } + out := string(data) + + for _, mark := range []string{ + "Trusted-fact compatibility", // the v3 warning banner + "load_baseline failed", // a per-target error banner + } { + at := strings.Index(out, mark) + if at < 0 { + t.Errorf("HTML report does not show %q at all", mark) + continue + } + // The mark belongs to a target card, so the enclosing card is the + // last one opened before it; any
between that card's + // start and the mark would be hiding it. + card := strings.LastIndex(out[:at], `
`) + if card < 0 { + t.Errorf("%q is not inside a target card", mark) + continue + } + if strings.Contains(out[card:at], "1 failed`) { + t.Error("the closed estimate list does not mark that one estimate failed") + } + // The failed entry keeps its place in the list rather than being + // hoisted out of it, so its reason is still one click away. + if !strings.Contains(out, "puppetdb returned status 503") { + t.Error("the failed estimate's reason is not on the page") + } +} + +// TestHTML_CountsAgreeWithWhatIsRendered guards the header/body agreement +// per section. sampleResult holds two aggregate groups, one of each +// shape, which the page splits into separate counted sections. +func TestHTML_CountsAgreeWithWhatIsRendered(t *testing.T) { + data, err := HTML(sampleResult()) + if err != nil { + t.Fatalf("HTML: %v", err) + } + visible := string(data)[:strings.Index(string(data), "

Result document

")] + + for _, want := range []string{ + `

Aggregate diff 1

`, + `Grouped resource changes 1`, + `Dependency-graph edge groups 1`, + `Resource changes 4`, + `Dependency-graph edges 1`, + } { + if !strings.Contains(visible, want) { + t.Errorf("HTML report is missing the counted heading %q", want) + } + } +} + +// TestHTML_EdgeOnlyTargetShowsItsEdges is the report/exit-code contract +// for this format. A target whose only differences are edges still drove +// the run's outcome; because HTML keeps edge changes, it discharges that +// by rendering them rather than by the note the text report needs. +func TestHTML_EdgeOnlyTargetShowsItsEdges(t *testing.T) { r := model.NewResult("test", "2026-08-25T12:00:00Z") - r.Aggregate = model.AggregateDiff{Groups: []model.AggregateGroup{{ - Key: model.AggregateChangeKey{Kind: model.ChangeEdgeRemoved, Edge: &model.Edge{Source: "Class[a]", Target: "Class[b]"}}, - Certnames: []string{"web-01.example.test"}, - }}} + r.Targets = []model.TargetResult{{ + Certname: "web-01.example.test", + Config: &model.ConfigProvenance{}, + NodeDiff: &model.NodeDiff{ + Certname: "web-01.example.test", + HasDifference: true, + EdgeChanges: []model.EdgeChange{ + {Kind: model.ChangeEdgeAdded, Edge: model.Edge{Source: "Class[a]", Target: "Class[b]"}}, + }, + }, + }} r.Reduce() data, err := HTML(r) if err != nil { t.Fatalf("HTML: %v", err) } - if !strings.Contains(string(data), "edge_removed Class[a] -> Class[b]") { - t.Errorf("edge group did not render: %s", string(data)) + visible := string(data)[:strings.Index(string(data), "

Result document

")] + + if strings.Contains(visible, "No non-excluded differences") { + t.Errorf("an edge-only difference was reported as no changes\n---\n%s", visible) + } + if !strings.Contains(visible, `Dependency-graph edges 1`) { + t.Errorf("the target's edge changes are not shown\n---\n%s", visible) } } diff --git a/internal/report/options.go b/internal/report/options.go new file mode 100644 index 0000000..a759ba4 --- /dev/null +++ b/internal/report/options.go @@ -0,0 +1,26 @@ +package report + +// Options is the display policy the text and HTML renderers apply. It +// carries presentation choices only — never anything that could change +// what the comparison found — so the JSON report, which is the complete +// machine-readable record (requirements.md 8.2), ignores it entirely and +// takes no Options at all. +// +// Two formats therefore show less than the document contains, by design: +// a CI log and a review page are read top to bottom by a human, and a +// per-target list of several hundred dependency-graph edges or a repeated +// PQL string per estimate buries the resource changes a reviewer is +// actually looking for. Nothing shown is ever recomputed or re-derived +// for a format (see doc.go); the renderers only choose what to print. +type Options struct { + // ImpactNodes prints every certname an impact estimate returned + // instead of the capped inline sample. A bounded estimate may hold up + // to its configured result_limit certnames — a thousand by default — + // so the full list is opt-in per estimate section, not the default. + ImpactNodes bool +} + +// inlineCertnameCap is how many certnames an impact estimate prints +// inline when Options.ImpactNodes is not set. The remainder is reported +// as a "+N more" tail, so the count is never hidden — only the names are. +const inlineCertnameCap = 5 diff --git a/internal/report/render.go b/internal/report/render.go index 2b22ea4..8881c74 100644 --- a/internal/report/render.go +++ b/internal/report/render.go @@ -116,21 +116,31 @@ func fileContentSummary(e model.FileContentEvidence) string { return summary } -// edgeSummary renders one edge-level change as a single line. Direction -// is significant (design.md section 7.1), so the arrow is never -// normalized away. -func edgeSummary(c model.EdgeChange) string { - sign := "+" - if c.Kind == model.ChangeEdgeRemoved { - sign = "-" +// certnameList renders a certname sample as one comma-separated line. +// Without Options.ImpactNodes it prints at most inlineCertnameCap names +// and reports the rest as a "+N more" tail: the count stays exact, only +// the names are elided, so a compact line never understates how many +// nodes an estimate returned. +func certnameList(names []string, showAll bool) string { + if len(names) == 0 { + return "" } - return fmt.Sprintf("%sedge %s -> %s", sign, c.Edge.Source, c.Edge.Target) + if showAll || len(names) <= inlineCertnameCap { + return strings.Join(names, ", ") + } + return fmt.Sprintf("%s (+%d more)", strings.Join(names[:inlineCertnameCap], ", "), len(names)-inlineCertnameCap) } // aggregateKeyLabel renders an aggregate group's key. It handles both // shapes of model.AggregateChangeKey — exactly one of Identity and Edge // is set, selected by Kind — so an edge group never dereferences a nil // Identity. +// +// The text report filters edge groups out before reaching here, so its +// Edge branch is exercised only by HTML (which renders them) and by +// direct callers. It is written defensively regardless: Identity is a +// pointer that is nil for every edge-kind key, so any caller that reaches +// this function with one would otherwise panic. func aggregateKeyLabel(key model.AggregateChangeKey) string { if key.Edge != nil { return fmt.Sprintf("%s %s -> %s", key.Kind, key.Edge.Source, key.Edge.Target) @@ -144,31 +154,130 @@ func aggregateKeyLabel(key model.AggregateChangeKey) string { return fmt.Sprintf("%s %s", key.Kind, key.Identity) } +// aggregateGroupLabel renders an aggregate group's change as one phrase: +// its key, plus the before/after pair when the group carries one. +// +// The value pair is joined without a colon after the parameter name — +// "ensure \"running\" -> \"stopped\"", not "ensure: \"running\" -> ..." — +// so that the only colon on the finished line is the structural one +// separating the change from its targets. +func aggregateGroupLabel(g model.AggregateGroup) string { + label := aggregateKeyLabel(g.Key) + if g.Before != nil || g.After != nil { + label += fmt.Sprintf(" %s -> %s", formatValue(g.Before), formatValue(g.After)) + } + return label +} + +// targetCountList renders an aggregate group's certnames as a count and +// the names, matching the shape an impact estimate uses for its own +// nodes so the two sections read alike. +// +// The count is not bracketed. "Class[nginx] [4]" puts a bracketed number +// immediately after a bracketed resource title, which reads as a second +// resource reference on a line whose whole purpose is to be scanned +// quickly. +// +// Nor is the list ever capped: these are the operator's own configured +// targets, a set they wrote and whose size they already know — unlike an +// impact estimate's certnames, which come from the estate. +func targetCountList(certnames []string) string { + noun := "targets" + if len(certnames) == 1 { + noun = "target" + } + return fmt.Sprintf("%d %s: %s", len(certnames), noun, strings.Join(certnames, ", ")) +} + +// displayedGroups filters an aggregate diff down to the groups the text +// report shows. Edge groups are dropped: a CI log is a linear read with +// no way to skip a section, and a run's edge groups routinely outnumber +// its resource groups. +// +// HTML does not use this — it renders edge groups behind their own +// disclosure — and the underlying model.AggregateDiff is untouched: +// requirements.md 7.4 requires edge changes to survive aggregation as a +// distinct kind, and they do. +func displayedGroups(groups []model.AggregateGroup) []model.AggregateGroup { + displayed := make([]model.AggregateGroup, 0, len(groups)) + for _, g := range groups { + if g.Key.Kind == model.ChangeEdgeAdded || g.Key.Kind == model.ChangeEdgeRemoved { + continue + } + displayed = append(displayed, g) + } + return displayed +} + // exclusionSummary renders one applied exclusion rule and its suppressed -// counts, per requirements.md 6.5. +// counts for the text report, per requirements.md 6.5. The +// suppressed-edge count is omitted for the same reason edge changes +// themselves are omitted from that format (see doc.go). func exclusionSummary(e model.ExclusionOutcome) string { + return fmt.Sprintf("%s[%s]: %d resource(s), %d parameter(s) suppressed", + e.Rule.Type, e.Rule.Title, e.SuppressedResources, e.SuppressedParameters) +} + +// exclusionSummaryFull is exclusionSummary with the suppressed-edge count +// restored, for the HTML report and the JSON document — the two formats +// that keep everything. +func exclusionSummaryFull(e model.ExclusionOutcome) string { return fmt.Sprintf("%s[%s]: %d resource(s), %d parameter(s), %d edge(s) suppressed", e.Rule.Type, e.Rule.Title, e.SuppressedResources, e.SuppressedParameters, e.SuppressedEdges) } -// estimateSummary renders one estimate's status line. It describes what -// the bounded query returned, never a total and never a prediction; see -// model.ImpactEstimate.ResultCount. -func estimateSummary(e model.ImpactEstimate) string { - switch e.Status { - case model.ImpactStatusCompleted: - summary := fmt.Sprintf("completed: %d node(s) returned (result_limit %d)", e.ResultCount, e.ResultLimit) - if e.Truncated { - summary += fmt.Sprintf("; truncated, showing the first %d certnames in sorted order", len(e.Certnames)) - } +// estimateSummary renders one estimate as a single compact line: the +// count the bounded query returned, followed by the certname sample. It +// describes what the query returned, never a total and never a +// prediction; see model.ImpactEstimate.ResultCount. +// +// Only the happy path collapses. A truncated estimate still says so and +// still names its result_limit (requirements.md 9.6), and a timeout or +// failure still reports its status and reason rather than a node count +// (requirements.md 9.7) — compacting either into "N nodes" would state +// something the run does not know. +// +// The PQL and the request options are deliberately absent: they are the +// same string on every line of a several-hundred-estimate section, and +// requirements.md 9.4's "report the exact generated PQL query" is +// discharged by the JSON report, which records both verbatim. +func estimateSummary(e model.ImpactEstimate, showAllNodes bool) string { + summary := estimateCount(e) + if e.Status != model.ImpactStatusCompleted { return summary - default: + } + if list := certnameList(e.Certnames, showAllNodes); list != "" { + summary += ": " + list + } + return summary +} + +// estimateCount states what the bounded query returned, without naming +// any node. It is the whole of an estimate's line in the text report +// (before the sample) and the always-visible half of one in HTML, where +// the certnames sit behind a disclosure. +// +// Only the happy path collapses to a bare count. A truncated estimate +// still says so and still names its result_limit (requirements.md 9.6), +// and a timeout or failure still reports its status and reason rather +// than a node count (requirements.md 9.7) — compacting either into +// "N nodes" would state something the run does not know. +func estimateCount(e model.ImpactEstimate) string { + if e.Status != model.ImpactStatusCompleted { reason := e.FailureReason if reason == "" { reason = "no reason reported" } return fmt.Sprintf("%s: %s", e.Status, reason) } + if e.Truncated { + // ResultCount is ResultLimit+1 here — all the bounded query asked + // for — so the only true statement about the population is that + // it exceeds the limit. "more than N" says exactly that; the + // exact count would be a fabrication. + return fmt.Sprintf("more than %d nodes (truncated at result_limit %d)", e.ResultLimit, e.ResultLimit) + } + return fmt.Sprintf("%d %s", e.ResultCount, plural(e.ResultCount, "node", "nodes")) } // sortedKeys returns m's keys in lexicographic order. Every map in a diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 2446bb6..5b0ba86 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -94,7 +94,7 @@ func TestJSON_CanonicalizesNumberSpelling(t *testing.T) { // final outcome first, then per-target status, node changes, // warnings/errors, aggregate summary, impact summary. func TestText_SectionOrder(t *testing.T) { - data, err := Text(sampleResult()) + data, err := Text(sampleResult(), Options{}) if err != nil { t.Fatalf("Text: %v", err) } @@ -117,26 +117,27 @@ func TestText_SectionOrder(t *testing.T) { } // TestText_RequiredContent checks the elements requirements.md 2.5, 6.5, -// 9.3, 9.4, and 10.2 require to be visible in the CI log. +// 9.3, 9.6, and 10.2 require to be visible in the CI log. Requirement +// 9.4's PQL is deliberately not among them: it is discharged by the JSON +// report — see TestJSON_KeepsWhatTextAndHTMLOmit. func TestText_RequiredContent(t *testing.T) { - data, err := Text(sampleResult()) + data, err := Text(sampleResult(), Options{}) if err != nil { t.Fatalf("Text: %v", err) } out := string(data) for _, want := range []string{ - "reason:", // 10.2 - model.V3TrustedFactWarning, // 2.5 - "Package[*]: 2 resource(s)", // 6.5 - ImpactEstimateLabel, // 9.3 - ImpactEstimateNote, // 9.3 - `resources[certname] { type = "Service"`, // 9.4 - "truncated", // 9.6 - "ERROR [load_baseline]", // 8.2/10.5 + "reason:", // 10.2 + model.V3TrustedFactWarning, // 2.5 + "Package[*]: 2 resource(s)", // 6.5 + ImpactEstimateLabel, // 9.3 + ImpactEstimateNote, // 9.3 + "truncated", // 9.6 + "result_limit 2", // 9.6: the bound is named, not just the state + "ERROR [load_baseline]", // 8.2/10.5 `~ Service[nginx] ensure: "stopped" -> "running"`, "~ File[/etc/motd] content: changed (via inline_content) sha256 aaaa -> bbbb", - "+edge Class[a] -> Class[b]", } { if !strings.Contains(out, want) { t.Errorf("text report is missing %q\n---\n%s", want, out) @@ -144,11 +145,197 @@ func TestText_RequiredContent(t *testing.T) { } } +// TestText_OmitsEdgesAndQueryMechanics locks the display policy the two +// human-facing formats apply (see doc.go). These strings are not merely +// absent by accident: each one was previously rendered, and each is the +// bulk that buried the resource changes a reviewer reads a CI log for. +func TestText_OmitsEdgesAndQueryMechanics(t *testing.T) { + data, err := Text(sampleResult(), Options{}) + if err != nil { + t.Fatalf("Text: %v", err) + } + out := string(data) + + for _, forbidden := range []string{ + "+edge Class[a] -> Class[b]", // per-target edge change + "edge_added Class[a]", // aggregate edge group + "edge(s) suppressed", // exclusion edge count + "resources[certname]", // an estimate's PQL + "/pdb/query/v4", // an estimate's request path + "order_by", // an estimate's request options + } { + if strings.Contains(out, forbidden) { + t.Errorf("text report still shows %q\n---\n%s", forbidden, out) + } + } +} + +// TestText_CountsOnlyWhatItPrints guards the header/body agreement that +// keeps a section from promising lines it does not show. sampleResult has +// four resource changes and one edge change on its first target, and two +// aggregate groups of which one is an edge group. +func TestText_CountsOnlyWhatItPrints(t *testing.T) { + data, err := Text(sampleResult(), Options{}) + if err != nil { + t.Fatalf("Text: %v", err) + } + out := string(data) + + if !strings.Contains(out, "changes (4):") { + t.Errorf("per-target change count does not match the displayed lines\n---\n%s", out) + } + if !strings.Contains(out, "aggregate diff (1):") { + t.Errorf("aggregate count still includes the filtered edge group\n---\n%s", out) + } +} + +// TestText_AggregateLineIsUnambiguous locks the shape of an aggregate +// line. The count is not bracketed, because a bracketed number directly +// after a bracketed resource title reads as a second resource reference, +// and there is exactly one structural colon separating the change from +// its targets — so the parameter name is not followed by one. +func TestText_AggregateLineIsUnambiguous(t *testing.T) { + data, err := Text(sampleResult(), Options{}) + if err != nil { + t.Fatalf("Text: %v", err) + } + out := string(data) + + const want = ` parameter_changed Service[nginx] ensure "stopped" -> "running": 1 target: web-01.example.test` + "\n" + if !strings.Contains(out, want) { + t.Errorf("aggregate line shape changed:\nwant %q\n---\n%s", want, out) + } +} + +// TestText_EdgeOnlyTargetIsNotReportedAsUnchanged is the report/exit-code +// contract. A target whose only differences are edges still has +// HasDifference true and still drives the run's outcome, so hiding its +// edge list must not turn its section into a blank that reads as "nothing +// changed" (requirements.md 10.2/10.5). +func TestText_EdgeOnlyTargetIsNotReportedAsUnchanged(t *testing.T) { + r := model.NewResult("test", "2026-08-25T12:00:00Z") + r.Targets = []model.TargetResult{{ + Certname: "web-01.example.test", + Config: &model.ConfigProvenance{}, + NodeDiff: &model.NodeDiff{ + Certname: "web-01.example.test", + HasDifference: true, + EdgeChanges: []model.EdgeChange{ + {Kind: model.ChangeEdgeAdded, Edge: model.Edge{Source: "Class[a]", Target: "Class[b]"}}, + }, + }, + }} + r.Reduce() + + data, err := Text(r, Options{}) + if err != nil { + t.Fatalf("Text: %v", err) + } + out := string(data) + + if strings.Contains(out, "changes: none") { + t.Errorf("an edge-only difference was reported as no changes\n---\n%s", out) + } + if !strings.Contains(out, "1 dependency-edge difference(s) only") { + t.Errorf("the edge-only note is missing\n---\n%s", out) + } + if strings.Contains(out, "Class[a]") { + t.Errorf("the note leaked the edge it stands in for\n---\n%s", out) + } +} + +// TestJSON_KeepsWhatTextAndHTMLOmit is where requirements.md 5.3, 6.5, +// 7.4, 8.2, and 9.4 are actually discharged. Trimming the two +// human-facing formats is only defensible while the machine-readable +// record stays complete, so this test fails the moment display policy +// leaks into JSON. +func TestJSON_KeepsWhatTextAndHTMLOmit(t *testing.T) { + data, err := JSON(sampleResult()) + if err != nil { + t.Fatalf("JSON: %v", err) + } + var decoded model.Result + if err := json.Unmarshal(data, &decoded); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + + if len(decoded.Targets[0].NodeDiff.EdgeChanges) != 1 { // 5.3 + t.Errorf("edge changes did not survive into the JSON report") + } + if decoded.Targets[0].NodeDiff.Exclusions[0].SuppressedEdges != 1 { // 6.5 + t.Errorf("the suppressed-edge count did not survive into the JSON report") + } + var edgeGroups int + for _, g := range decoded.Aggregate.Groups { // 7.4 + if g.Key.Kind == model.ChangeEdgeAdded || g.Key.Kind == model.ChangeEdgeRemoved { + edgeGroups++ + } + } + if edgeGroups != 1 { + t.Errorf("aggregate edge groups = %d, want 1", edgeGroups) + } + if decoded.ImpactEstimates[0].PQL == "" { // 9.4 + t.Errorf("the generated PQL did not survive into the JSON report") + } + if decoded.ImpactEstimates[0].Request.Path == "" || decoded.ImpactEstimates[0].Request.OrderBy == "" { // 9.7 + t.Errorf("the request options did not survive into the JSON report") + } +} + +// TestText_ImpactNodesControlsTheCertnameSample covers the --impact-nodes +// option: without it a long sample is capped and the remainder counted, +// with it every certname is named. The count itself is never elided +// either way, since that is what the estimate actually measured. +func TestText_ImpactNodesControlsTheCertnameSample(t *testing.T) { + names := make([]string, 0, inlineCertnameCap+3) + for i := 0; i < inlineCertnameCap+3; i++ { + names = append(names, string(rune('a'+i))+".example.test") + } + + r := model.NewResult("test", "2026-08-25T12:00:00Z") + r.ImpactEstimates = []model.ImpactEstimate{{ + Identity: model.ResourceIdentity{Type: "File", Title: "/etc/nginx/conf.d"}, + PQL: `resources[certname] { type = "File" and title = "/etc/nginx/conf.d" }`, + Request: model.ImpactRequest{Path: "/pdb/query/v4", Limit: 1001}, + ResultLimit: 1000, Timeout: "10s", Status: model.ImpactStatusCompleted, + Certnames: names, ResultCount: len(names), + }} + r.Reduce() + + capped, err := Text(r, Options{}) + if err != nil { + t.Fatalf("Text: %v", err) + } + full, err := Text(r, Options{ImpactNodes: true}) + if err != nil { + t.Fatalf("Text: %v", err) + } + + if !strings.Contains(string(capped), "(+3 more)") { + t.Errorf("the capped sample does not report the elided remainder\n---\n%s", capped) + } + last := names[len(names)-1] + if strings.Contains(string(capped), last) { + t.Errorf("the capped sample named %q past the cap", last) + } + if !strings.Contains(string(full), last) { + t.Errorf("--impact-nodes did not name every certname\n---\n%s", full) + } + if strings.Contains(string(full), "more)") { + t.Errorf("--impact-nodes still elided part of the sample\n---\n%s", full) + } + for _, want := range []string{"8 nodes"} { + if !strings.Contains(string(capped), want) || !strings.Contains(string(full), want) { + t.Errorf("the node count %q is not stated in both forms", want) + } + } +} + // TestText_NeverClaimsEstimatedNodesWillChange guards requirements.md // 9.3's prohibition and CONTEXT.md's _Avoid_ wording for impact // estimates. func TestText_NeverClaimsEstimatedNodesWillChange(t *testing.T) { - data, err := Text(sampleResult()) + data, err := Text(sampleResult(), Options{}) if err != nil { t.Fatalf("Text: %v", err) } @@ -166,12 +353,12 @@ func TestText_NeverClaimsEstimatedNodesWillChange(t *testing.T) { // TestText_IsByteIdenticalForIdenticalInput is design.md's Property 1 // applied to the text report, which iterates provenance maps. func TestText_IsByteIdenticalForIdenticalInput(t *testing.T) { - first, err := Text(sampleResult()) + first, err := Text(sampleResult(), Options{}) if err != nil { t.Fatalf("Text: %v", err) } for i := 0; i < 20; i++ { - next, err := Text(sampleResult()) + next, err := Text(sampleResult(), Options{}) if err != nil { t.Fatalf("Text: %v", err) } diff --git a/internal/report/text.go b/internal/report/text.go index 6a1e890..70c958a 100644 --- a/internal/report/text.go +++ b/internal/report/text.go @@ -16,7 +16,10 @@ import ( // The outcome and its reasons come first because a CI log is read from // the top and often truncated; requirements.md 10.2 requires both to be // present. -func Text(r model.Result) ([]byte, error) { +// +// opts selects display policy only — what this format prints, never what +// it says about the run. See Options. +func Text(r model.Result, opts Options) ([]byte, error) { var b bytes.Buffer fmt.Fprintf(&b, "PIACE %s (%s)\n", r.Invocation.ToolVersion, r.Invocation.TimestampUTC) @@ -30,7 +33,7 @@ func Text(r model.Result) ([]byte, error) { writeTextTargets(&b, r.Targets) writeTextAggregate(&b, r.Aggregate) - writeTextImpact(&b, r.ImpactEstimates) + writeTextImpact(&b, r.ImpactEstimates, opts) writeTextRunDiagnostics(&b, r.Diagnostics) return b.Bytes(), nil @@ -110,54 +113,62 @@ func candidateProvenanceLine(p model.CandidateProvenance) string { return strings.Join(parts, " ") } +// writeTextNodeDiff prints one target's displayed changes. The count in +// the header counts what is actually printed, not what the node diff +// holds, so the header can never promise lines that follow it. +// +// A target whose only differences are dependency-graph edges still has +// HasDifference true and still drives the run's outcome and exit code, so +// it gets an explicit note rather than an empty list: a report that +// printed nothing here would read as "no changes" on a run that exits +// non-zero, which is exactly the stdout/exit-code contradiction +// writeReports in cmd/piace is ordered to prevent. func writeTextNodeDiff(b *bytes.Buffer, nd model.NodeDiff) { - if !nd.HasDifference { + switch { + case !nd.HasDifference: fmt.Fprintf(b, " changes: none\n") - } else { - fmt.Fprintf(b, " changes (%d resource, %d edge):\n", len(nd.ResourceChanges), len(nd.EdgeChanges)) + case len(nd.ResourceChanges) == 0: + fmt.Fprintf(b, " changes: %d dependency-edge difference(s) only, not shown in the text report\n", len(nd.EdgeChanges)) + default: + fmt.Fprintf(b, " changes (%d):\n", len(nd.ResourceChanges)) for _, c := range nd.ResourceChanges { fmt.Fprintf(b, " %s\n", changeSummary(c)) } - for _, c := range nd.EdgeChanges { - fmt.Fprintf(b, " %s\n", edgeSummary(c)) - } } for _, e := range nd.Exclusions { fmt.Fprintf(b, " excluded: %s\n", exclusionSummary(e)) } } +// writeTextAggregate prints one line per displayed aggregate group. The +// header counts displayed groups rather than len(agg.Groups), which still +// includes the edge groups this format filters out. +// +// Certnames are never capped here: they are the operator's own configured +// targets, a set they wrote themselves and whose size they already know — +// unlike an impact estimate's certnames, which come from the estate. func writeTextAggregate(b *bytes.Buffer, agg model.AggregateDiff) { - fmt.Fprintf(b, "\naggregate diff (%d group(s)):\n", len(agg.Groups)) - for _, g := range agg.Groups { - fmt.Fprintf(b, " %s\n", aggregateKeyLabel(g.Key)) - if g.Key.Edge == nil && (g.Before != nil || g.After != nil) { - fmt.Fprintf(b, " %s -> %s\n", formatValue(g.Before), formatValue(g.After)) - } - fmt.Fprintf(b, " %d target(s): %s\n", len(g.Certnames), strings.Join(g.Certnames, ", ")) + groups := displayedGroups(agg.Groups) + fmt.Fprintf(b, "\naggregate diff (%d):\n", len(groups)) + for _, g := range groups { + fmt.Fprintf(b, " %s: %s\n", aggregateGroupLabel(g), targetCountList(g.Certnames)) } } -// writeTextImpact renders the impact section. The section header carries -// requirement 9.3's label and note; individual entries never phrase a -// returned certname as a node that will change. -func writeTextImpact(b *bytes.Buffer, estimates []model.ImpactEstimate) { +// writeTextImpact renders the impact section, one line per estimate. The +// section header carries requirement 9.3's label and note, which is what +// keeps a bare "N nodes" line from reading as a prediction: the note +// above it states, once for the whole section, that a listed certname +// means only that the node's latest stored catalog contains the resource. +// No individual entry ever phrases a certname as a node that will change. +func writeTextImpact(b *bytes.Buffer, estimates []model.ImpactEstimate, opts Options) { if len(estimates) == 0 { return } fmt.Fprintf(b, "\n%s (%d):\n", ImpactEstimateLabel, len(estimates)) fmt.Fprintf(b, " %s\n", ImpactEstimateNote) for _, e := range estimates { - fmt.Fprintf(b, " %s: %s\n", e.Identity, estimateSummary(e)) - fmt.Fprintf(b, " pql: %s\n", e.PQL) - fmt.Fprintf(b, " request: path=%s limit=%d timeout=%s", e.Request.Path, e.Request.Limit, e.Timeout) - if e.Request.OrderBy != "" { - fmt.Fprintf(b, " order_by=%s", e.Request.OrderBy) - } - fmt.Fprintln(b) - if len(e.Certnames) > 0 { - fmt.Fprintf(b, " nodes with this resource in their latest stored catalog: %s\n", strings.Join(e.Certnames, ", ")) - } + fmt.Fprintf(b, " %s: %s\n", e.Identity, estimateSummary(e, opts.ImpactNodes)) } }