From 9dab286f5b403d8a736ada89ce6c6069f588ec3d Mon Sep 17 00:00:00 2001 From: Alessandro Franceschi Date: Wed, 26 Aug 2026 09:12:12 +0200 Subject: [PATCH 1/5] feat: make the reports readable, each at its own level of detail The three formats now show the same document at three levels of detail, decided through shared helpers so they cannot drift apart in what a value says. JSON is unchanged: it stays the complete machine-readable record and takes no display options. HTML keeps everything too and uses disclosure instead of omission. Edge changes, aggregate edge groups, an estimate's PQL, request options and full certname list are all on the page inside closed
; a target's resource changes open by default. The page is redesigned around a light palette on a warm paper ground, a serif/sans/mono split so identities and values are distinguishable without color, a colored +/-/~ gutter, sticky section headers, and a masthead tally. Requirement 8.3 rules out webfonts and image files as well as CDNs, so the type is system font stacks with declared fallbacks and the disclosure marker is drawn in CSS. Text is the only format that omits, because a CI log is a linear read with nothing to expand. It drops edge changes -- a consequence of the resource changes, and routinely more numerous -- along with each estimate's query mechanics, and caps an estimate's certname sample. The new --impact-nodes flag lifts that cap; it is a text-report control and leaves the HTML artifact byte-identical. On a real four-target run this takes the text report from 3623 to 1297 lines and the HTML reading path down by 59%. Requirements 5.3, 6.5, 7.4, 8.2 and 9.4 are discharged by the JSON report, and visibly by HTML as well; acceptance tests assert them there rather than in stdout. Two invariants keep the trimming honest: every section header counts what it actually displays, and a target whose only differences are edges is never rendered as unchanged -- HTML shows the edges, text prints a note -- so no report reads as "nothing changed" on a run that exits non-zero. Co-Authored-By: Claude Opus 5 --- .kiro/specs/piace/design.md | 27 ++ README.md | 35 +- cmd/piace/acceptance_impact_test.go | 96 ++++- cmd/piace/acceptance_test.go | 13 +- cmd/piace/main.go | 29 +- internal/report/doc.go | 74 +++- internal/report/html.go | 340 ++++++++++++---- internal/report/html_template.go | 598 ++++++++++++++++++++++++---- internal/report/html_test.go | 117 +++++- internal/report/options.go | 26 ++ internal/report/render.go | 149 ++++++- internal/report/report_test.go | 217 +++++++++- internal/report/text.go | 69 ++-- 13 files changed, 1535 insertions(+), 255 deletions(-) create mode 100644 internal/report/options.go diff --git a/.kiro/specs/piace/design.md b/.kiro/specs/piace/design.md index 61f36c0..26e6af1 100644 --- a/.kiro/specs/piace/design.md +++ b/.kiro/specs/piace/design.md @@ -380,6 +380,33 @@ 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: edge changes, aggregate edge groups, and +each estimate's PQL, request options and full certname list are on the page +inside closed `
`; a target's resource changes open by default. 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/README.md b/README.md index ee511eb..8952d12 100644 --- a/README.md +++ b/README.md @@ -65,7 +65,7 @@ request and on every push to `main`: ``` 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 +77,29 @@ 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. Resource changes are open; +dependency-graph edges, an estimate's PQL, request options and full node list, +the exclusion detail and the provenance block all sit in closed sections you +expand. 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. It is one self-contained file with a light background, no +webfonts and no images: `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 @@ -300,8 +323,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 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/internal/report/doc.go b/internal/report/doc.go index d05b1f9..1c87154 100644 --- a/internal/report/doc.go +++ b/internal/report/doc.go @@ -21,17 +21,62 @@ // 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: +// edge changes, aggregate edge groups, an estimate's PQL, request +// options and full certname list are all on the page, inside closed +//
. A target's resource changes open by default; everything +// else starts closed. Nothing is capped, because a closed disclosure +// already keeps a thousand certnames out of the reading path without +// dropping a name. +// - 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 +91,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..65c5f70 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,22 @@ 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,65 +444,123 @@ 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 .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}} +
+ {{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 .Aggregate}} +
    +
      + {{range .Aggregate}} +
    • + {{.Sign}} + {{.Identity}}{{if .Parameter}}{{.Parameter}}{{end}}{{if .HasValues}}{{.Before}}→{{.After}}{{end}}{{.Targets}} +
      • + {{end}} +
    +
    +{{else}} +

    No grouped resource changes.

    +{{end}} +{{if .EdgeAggregate}} +
    + Dependency-graph edge groups {{.TotalEdgeGroups}} +
    +
      + {{range .EdgeAggregate}} +
    • + {{.Sign}} + {{.Source}}→{{.Target}}{{.Targets}} +
      • + {{end}} +
    +
    +
    {{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}} -
+
+ {{range .Estimates}} +
+
+ {{.Identity}} + {{if .Failed}}{{.Status}}{{end}} + {{.Count}} +
+
+ Nodes and query +
+ {{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}} @@ -146,8 +568,8 @@ summary { cursor: pointer; font-size: .9rem; }

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..0e4f80e 100644 --- a/internal/report/html_test.go +++ b/internal/report/html_test.go @@ -90,23 +90,120 @@ 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{ + "Dependency-graph edges", // per-target edges + "Dependency-graph edge groups", // aggregate edge groups + "Nodes and query", // an estimate's certnames, PQL, request + "Excluded differences", // requirements.md 8.5 + } { + if !strings.Contains(visible, summary) { + t.Errorf("HTML report has no disclosure headed %q", summary) + } + } + // ...and the resource changes, the thing a reader came for, must not be. + if !strings.Contains(visible, "
") { + t.Error("the resource-change list is not open by default") + } +} + +// 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

`, + `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)) } } From b2c4374673222c21bf64b322840735d95114738f Mon Sep 17 00:00:00 2001 From: Alessandro Franceschi Date: Fri, 28 Aug 2026 14:34:22 +0200 Subject: [PATCH 2/5] feat(report): make the HTML report an index of the run Every list of rows -- resource changes, edge changes, aggregate groups, exclusion detail, provenance, and an estimate's PQL, request options and node list -- now sits in a closed
whose summary carries its count, so the page a reader lands on is the outcome, the reasons, the tally, and one line per target with a counted chip per section. What stays outside every disclosure is what requirements.md 8.5 requires visibly marked: retrieval and compilation failures, the v3 warning, the run diagnostics and the outcome badges. A mark a reader has to go looking for is not a visible one. A failed impact estimate would be invisible inside the closed estimate list, so its count rides on that summary rather than lifting the entry out of its place. Nothing is capped and nothing is dropped: a closed disclosure already keeps a thousand certnames out of the reading path. Co-Authored-By: Claude Opus 5 --- .kiro/specs/piace/design.md | 11 +- internal/report/doc.go | 15 +- internal/report/html.go | 33 ++- internal/report/html_template.go | 369 ++++++++++++++++++------------- internal/report/html_test.go | 73 +++++- 5 files changed, 328 insertions(+), 173 deletions(-) diff --git a/.kiro/specs/piace/design.md b/.kiro/specs/piace/design.md index 26e6af1..0aa9e4a 100644 --- a/.kiro/specs/piace/design.md +++ b/.kiro/specs/piace/design.md @@ -385,9 +385,14 @@ from the same redacted projection, preventing format drift or secret exposure. 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: edge changes, aggregate edge groups, and -each estimate's PQL, request options and full certname list are on the page -inside closed `
`; a target's resource changes open by default. Only +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, diff --git a/internal/report/doc.go b/internal/report/doc.go index 1c87154..d0ee72d 100644 --- a/internal/report/doc.go +++ b/internal/report/doc.go @@ -32,12 +32,17 @@ // // - JSON is the complete record and takes no options at all. // - HTML is complete too, and uses disclosure rather than omission: -// edge changes, aggregate edge groups, an estimate's PQL, request -// options and full certname list are all on the page, inside closed -//
. A target's resource changes open by default; everything -// else starts closed. Nothing is capped, because a closed disclosure +// 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. +// 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 diff --git a/internal/report/html.go b/internal/report/html.go index 65c5f70..96e7f93 100644 --- a/internal/report/html.go +++ b/internal/report/html.go @@ -30,9 +30,14 @@ import ( // Unlike the text report, HTML takes no display Options: it shows // everything the result document holds — edge changes, an estimate's PQL, // request options, and full certname list — and uses disclosure rather -// than omission to keep the page readable. A reader who wants the detail -// opens it; a reader scanning for the outcome never has to page past it. -// See doc.go. +// than omission to keep the page readable. Every list of rows is closed +// and its summary carries its count, so a reader scanning for the outcome +// reads an index of the run rather than paging through it, and a reader +// who wants a section opens it. What never goes behind a disclosure is a +// failure: the per-target error banners, the v3 warning, the run +// diagnostics and the outcome badges stay in the scanning path, because +// requirements.md 8.5's "visibly mark" is not satisfied by a mark a +// reader has to go looking for. See doc.go. // // Everything variable is interpolated through html/template, whose // contextual escaping is what makes an attacker-shaped resource title or @@ -83,6 +88,12 @@ type htmlView struct { TotalGroups int TotalEdgeGroups int TotalEstimates int + // TotalEstimateFailures rides on the estimate list's closed summary. + // The list is a disclosure like every other list of rows on the page, + // and a failed estimate inside a closed one would be invisible; the + // count keeps it in the scanning path without lifting the failed + // entries out of their place in the list. + TotalEstimateFailures int } // htmlTally is one figure in the masthead's at-a-glance row. It is @@ -109,10 +120,12 @@ type htmlTarget struct { Warnings []htmlDiagnostic Compared bool // Changes and EdgeChanges are the target's differences, split because - // the page discloses them separately: resource changes open, edges - // closed. HasDifference stays authoritative for whether the target - // changed at all, so a target whose only differences are edges is - // never rendered as unchanged. + // the page discloses them separately: a run's edge differences + // routinely outnumber its resource differences, being a consequence of + // them, so they get their own chip rather than padding the list a + // reader opens first. HasDifference stays authoritative for whether + // the target changed at all, so a target whose only differences are + // edges is never rendered as unchanged. HasDifference bool Changes []htmlChange EdgeChanges []htmlEdge @@ -227,7 +240,11 @@ func buildHTMLView(r model.Result, canonicalJSON string) htmlView { view.TotalEdgeGroups = len(view.EdgeAggregate) for _, e := range r.ImpactEstimates { - view.Estimates = append(view.Estimates, buildHTMLEstimate(e)) + estimate := buildHTMLEstimate(e) + if estimate.Failed { + view.TotalEstimateFailures++ + } + view.Estimates = append(view.Estimates, estimate) } view.TotalEstimates = len(view.Estimates) diff --git a/internal/report/html_template.go b/internal/report/html_template.go index 9b7b943..62ca586 100644 --- a/internal/report/html_template.go +++ b/internal/report/html_template.go @@ -14,13 +14,36 @@ package report // disclosure triangle — is drawn with CSS borders rather than set in a // glyph that may be missing on a reader's machine. // +// # The scanning path is one screen; everything else is one click +// // Disclosure sections use
, which needs no JavaScript. They are // what lets this format keep everything the result document holds while -// staying readable: bulk a CI log has to omit (edge changes, an -// estimate's PQL and full certname list) is present here, one click away -// rather than in the scanning path. A target's resource changes open by -// default because they are what the reader came for; everything else -// starts closed. +// staying readable, and the page applies them by one rule: a list of rows +// is always closed, and its states how many rows it holds, so +// the collapsed page is a complete index of the run rather than a +// truncation of it. A real run reaches four figures of rows (773 resource +// changes, 838 edge changes, 549 aggregate groups, 220 estimates is an +// ordinary shape); opened by default, any one of those sections buries +// every section after it. +// +// The one thing that never goes behind a disclosure is a failure. The +// per-target error banners, the v3 trusted-fact warning, the run +// diagnostics and every outcome badge sit outside
at the level +// they belong to, because requirements.md 8.5 requires catalog retrieval +// failure and compilation failure to be visibly marked and a mark a +// reader has to go looking for is not one. Same for requirement 9.3's +// label and note above the estimates. +// +// A target's disclosures are laid out as a wrapping row of chips that +// expand to full width when opened (.disclosures), which is what keeps a +// four-target run to roughly a screen: closed, each target is its +// certname, its outcome, and a chip per section with a count on it. +// +// Because the bulk is now closed, the print rules force disclosure +// content visible: a shared, printed or ticket-pasted artifact that +// dropped every row it had collapsed would be a different document from +// the one on screen. Engines hide
content two ways, so both +// are overridden. // // The template is layout only. Every decision about what a section shows // — how a change splits into sign, identity and values; which aggregate @@ -67,12 +90,12 @@ const htmlSource = ` body { margin: 0; - padding: 2rem 1.25rem 5rem; + padding: 1.75rem 1.25rem 4rem; background: var(--paper); color: var(--ink); font-family: var(--sans); font-size: 15.5px; - line-height: 1.55; + line-height: 1.5; -webkit-text-size-adjust: 100%; } @@ -85,7 +108,7 @@ main { max-width: 76rem; margin: 0 auto; } border: 1px solid var(--line); border-left: 4px solid var(--rule, var(--muted)); border-radius: var(--radius); - padding: 1.35rem 1.5rem; + padding: 1.1rem 1.35rem 1.2rem; } .masthead.clean { --rule: var(--clean-fg); } .masthead.allowed { --rule: var(--allowed-fg); } @@ -95,7 +118,7 @@ main { max-width: 76rem; margin: 0 auto; } .masthead h1 { font-family: var(--serif); - font-size: 1.45rem; + font-size: 1.35rem; font-weight: 600; letter-spacing: -0.01em; margin: 0; @@ -104,13 +127,13 @@ main { max-width: 76rem; margin: 0 auto; } display: flex; flex-wrap: wrap; align-items: center; - gap: .6rem .9rem; + gap: .5rem .85rem; } .stamp { font-family: var(--mono); - font-size: .78rem; + font-size: .75rem; color: var(--faint); - margin: .5rem 0 0; + margin: .35rem 0 0; overflow-wrap: anywhere; } .stamp span { white-space: nowrap; } @@ -118,30 +141,30 @@ main { max-width: 76rem; margin: 0 auto; } .tally { display: flex; flex-wrap: wrap; - gap: .35rem 2.25rem; - margin: 1.15rem 0 0; - padding: 1rem 0 0; + gap: .3rem 2rem; + margin: .9rem 0 0; + padding: .85rem 0 0; border-top: 1px solid var(--line-soft); } -.tally-item { display: flex; align-items: baseline; gap: .4rem; } +.tally-item { display: flex; align-items: baseline; gap: .35rem; } .tally-n { font-family: var(--serif); - font-size: 1.35rem; + font-size: 1.25rem; font-weight: 600; letter-spacing: -0.015em; font-variant-numeric: tabular-nums; } -.tally-l { font-size: .8rem; color: var(--muted); } +.tally-l { font-size: .78rem; color: var(--muted); } -.reasons { list-style: none; margin: 1.1rem 0 0; padding: 0; } +.reasons { list-style: none; margin: .85rem 0 0; padding: 0; } .reasons li { position: relative; padding-left: 1.1rem; - font-size: .9rem; + font-size: .85rem; color: var(--muted); overflow-wrap: anywhere; } -.reasons li + li { margin-top: .3rem; } +.reasons li + li { margin-top: .2rem; } .reasons li::before { content: ""; position: absolute; @@ -154,9 +177,9 @@ main { max-width: 76rem; margin: 0 auto; } .badge { display: inline-block; - padding: .12rem .55rem .18rem; + padding: .1rem .5rem .16rem; border-radius: 999px; - font-size: .74rem; + font-size: .72rem; font-weight: 600; letter-spacing: .01em; white-space: nowrap; @@ -178,11 +201,11 @@ h2 { justify-content: space-between; gap: 1rem; font-family: var(--serif); - font-size: 1.05rem; + font-size: 1rem; font-weight: 600; letter-spacing: -0.005em; - margin: 2.75rem 0 .85rem; - padding: .55rem 0 .4rem; + margin: 1.9rem 0 .6rem; + padding: .5rem 0 .35rem; background: var(--paper); border-bottom: 1px solid var(--line); } @@ -194,7 +217,7 @@ h2 .count { font-variant-numeric: tabular-nums; } -.empty { color: var(--faint); font-size: .875rem; margin: .4rem 0; } +.empty { color: var(--faint); font-size: .85rem; margin: .35rem 0; } /* ---- cards --------------------------------------------------------- */ @@ -202,19 +225,18 @@ h2 .count { background: var(--surface); border: 1px solid var(--line); border-radius: var(--radius); - padding: 1.1rem 1.25rem; - margin: 0 0 .9rem; + padding: .85rem 1rem .9rem; + margin: 0 0 .55rem; } .card-head { display: flex; flex-wrap: wrap; align-items: center; - gap: .5rem .7rem; - margin-bottom: .2rem; + gap: .4rem .65rem; } .card-head h3 { font-family: var(--mono); - font-size: .95rem; + font-size: .9rem; font-weight: 600; margin: 0; overflow-wrap: anywhere; @@ -224,9 +246,9 @@ h2 .count { .banner { border-radius: 6px; - padding: .6rem .8rem; - margin: .7rem 0; - font-size: .875rem; + padding: .5rem .7rem; + margin: .55rem 0 0; + font-size: .84rem; overflow-wrap: anywhere; } .banner strong { font-weight: 600; } @@ -240,18 +262,18 @@ h2 .count { /* ---- disclosure ---------------------------------------------------- */ -details { margin: .55rem 0 0; } +details { margin: .5rem 0 0; } summary { display: flex; align-items: center; - gap: .5rem; + gap: .45rem; list-style: none; cursor: pointer; - padding: .35rem .6rem; + padding: .28rem .55rem; border-radius: 6px; background: var(--sunk); border: 1px solid var(--line-soft); - font-size: .82rem; + font-size: .78rem; font-weight: 600; color: var(--muted); user-select: none; @@ -274,18 +296,31 @@ summary .count { color: var(--faint); font-variant-numeric: tabular-nums; } -.disclosed { padding: .7rem .1rem .2rem .75rem; } +.disclosed { padding: .6rem .1rem .1rem .7rem; } + +/* A target's (or the aggregate section's) disclosures sit side by side + as chips and take a full row of their own once opened, so the closed + card is one line of section headings rather than four stacked bars. */ +.disclosures { + display: flex; + flex-wrap: wrap; + align-items: flex-start; + gap: .4rem; + margin-top: .6rem; +} +.disclosures > details { margin: 0; min-width: 0; flex: 0 1 auto; } +.disclosures > details[open] { flex: 1 0 100%; } /* ---- change rows --------------------------------------------------- */ .rows { list-style: none; margin: 0; padding: 0; } .rows > li { display: grid; - grid-template-columns: .95rem minmax(0, 1fr); - gap: 0 .6rem; - padding: .17rem 0; - font-size: .8125rem; - line-height: 1.5; + grid-template-columns: .9rem minmax(0, 1fr); + gap: 0 .5rem; + padding: .12rem 0; + font-size: .8rem; + line-height: 1.45; border-top: 1px solid transparent; } .rows > li + li { border-top-color: var(--line-soft); } @@ -303,48 +338,54 @@ summary .count { .ident { font-family: var(--mono); overflow-wrap: anywhere; } .param { font-family: var(--mono); color: var(--muted); } .param::before { content: " · "; color: var(--faint); } +/* The before/after pair rides on the identity's own line and wraps only + when it has to: one row per change instead of two. */ .vals { - display: block; font-family: var(--mono); color: var(--muted); overflow-wrap: anywhere; - margin-top: .05rem; } +.vals::before { content: " · "; color: var(--faint); } .vals .was { color: var(--remove); } .vals .now { color: var(--add); } .note { - display: block; font-family: var(--mono); color: var(--muted); overflow-wrap: anywhere; - margin-top: .05rem; } +.note::before { content: " · "; color: var(--faint); } .on { display: block; color: var(--faint); - font-size: .78rem; + font-size: .76rem; overflow-wrap: anywhere; - margin-top: .05rem; } .arrow { color: var(--faint); padding: 0 .25rem; } /* ---- estimates ----------------------------------------------------- */ -.est { padding: .55rem 0; } -.est + .est { border-top: 1px solid var(--line-soft); } -.est-head { - display: flex; +/* One estimate is one clickable line: the resource, its status, and its + node count are the summary; the certnames, PQL and request options are + behind it. */ +.est { margin: 0; } +.est > summary { + background: none; + border: none; + border-radius: 0; + border-top: 1px solid var(--line-soft); + padding: .22rem .1rem; + font-weight: 400; flex-wrap: wrap; - align-items: baseline; - gap: .35rem .7rem; + gap: .3rem .6rem; } -.est-head .ident { font-size: .8125rem; } +.est:first-child > summary { border-top: none; } +.est > summary .ident { font-size: .8rem; color: var(--ink); } .est-count { - font-size: .8rem; + font-size: .78rem; color: var(--muted); font-variant-numeric: tabular-nums; } -.est details { margin-top: .3rem; } +.est .disclosed { padding: .35rem .1rem .55rem 1.05rem; } /* ---- key/value grids ----------------------------------------------- */ @@ -352,29 +393,29 @@ summary .count { display: grid; grid-template-columns: max-content minmax(0, 1fr); gap: .1rem .9rem; - font-size: .8125rem; - margin: .2rem 0 .8rem; + font-size: .8rem; + margin: .15rem 0 .7rem; } .kv dt { color: var(--muted); } .kv dd { margin: 0; font-family: var(--mono); overflow-wrap: anywhere; } .sub { - font-size: .74rem; + font-size: .72rem; font-weight: 600; letter-spacing: .04em; text-transform: uppercase; color: var(--faint); - margin: .9rem 0 .15rem; + margin: .75rem 0 .1rem; } -.sub:first-child { margin-top: .2rem; } +.sub:first-child { margin-top: .15rem; } .plain { list-style: none; - margin: .2rem 0 .8rem; + margin: .15rem 0 .7rem; padding: 0; font-family: var(--mono); - font-size: .8125rem; + font-size: .8rem; } .plain li { overflow-wrap: anywhere; } -.prose { font-size: .8125rem; overflow-wrap: anywhere; margin: .1rem 0 .6rem; } +.prose { font-size: .8rem; overflow-wrap: anywhere; margin: .1rem 0 .5rem; } .prose.mono { font-family: var(--mono); } pre { @@ -394,6 +435,7 @@ pre { body { padding: 1.25rem .75rem 3rem; } .kv { grid-template-columns: minmax(0, 1fr); gap: 0; } .kv dt { margin-top: .35rem; } + .disclosures > details { flex: 1 0 100%; } } @media print { @@ -401,6 +443,16 @@ pre { h2 { position: static; } .card, .est { break-inside: avoid; } pre { max-height: none; } + /* Everything the screen keeps one click away still prints: a closed + disclosure must not silently drop rows from a shared or filed copy. + Engines hide the content two ways, so both are overridden. The + canonical JSON is the exception — it is the same document a second + time, machine-readable, and half a megabyte of it on paper serves + nobody; a reader who opens it before printing still gets it. */ + details:not(.record) > *:not(summary) { display: block !important; } + details:not(.record)::details-content { content-visibility: visible !important; } + details:not(.record) > summary::before { transform: rotate(90deg); } + .disclosures > details { flex: 1 0 100%; } } @@ -449,113 +501,124 @@ pre {

No non-excluded differences.

{{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 .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 .EdgeChanges}} +
+ Dependency-graph edges {{len .EdgeChanges}} +
+
    + {{range .EdgeChanges}} +
  • + {{.Sign}} + {{.Source}}→{{.Target}} +
    • + {{end}} +
+
+
+ {{end}} - {{if .Exclusions}} -
- Excluded differences {{len .Exclusions}} -
-
    {{range .Exclusions}}
  • {{.}}
    • {{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 .Aggregate}} +{{if or .Aggregate .EdgeAggregate}}
-
    - {{range .Aggregate}} -
  • - {{.Sign}} - {{.Identity}}{{if .Parameter}}{{.Parameter}}{{end}}{{if .HasValues}}{{.Before}}→{{.After}}{{end}}{{.Targets}} -
    • +
    + {{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 .EdgeAggregate}} -
- Dependency-graph edge groups {{.TotalEdgeGroups}} -
-
    - {{range .EdgeAggregate}} -
  • - {{.Sign}} - {{.Source}}→{{.Target}}{{.Targets}} -
    • - {{end}} -
-
-
-{{end}} {{if .Estimates}}

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

- {{range .Estimates}} -
-
- {{.Identity}} - {{if .Failed}}{{.Status}}{{end}} - {{.Count}} -
+
- Nodes and query + Queried resources {{.TotalEstimates}}{{if .TotalEstimateFailures}}{{.TotalEstimateFailures}} failed{{end}}
- {{if .Certnames}}

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

{{.Certnames}}

{{end}} -

pql

{{.PQL}}

-

request

{{.Request}}

+ {{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}}
{{end}} @@ -567,7 +630,7 @@ pre { {{end}}

Result document

-
+
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 0e4f80e..2473b22 100644 --- a/internal/report/html_test.go +++ b/internal/report/html_test.go @@ -137,18 +137,82 @@ func TestHTML_KeepsEverythingBehindDisclosure(t *testing.T) { // 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 - "Nodes and query", // an estimate's certnames, PQL, request + "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 the resource changes, the thing a reader came for, must not be. - if !strings.Contains(visible, "
") { - t.Error("the resource-change list is not open by default") + // ...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") } } @@ -164,6 +228,7 @@ func TestHTML_CountsAgreeWithWhatIsRendered(t *testing.T) { for _, want := range []string{ `

Aggregate diff 1

`, + `Grouped resource changes 1`, `Dependency-graph edge groups 1`, `Resource changes 4`, `Dependency-graph edges 1`, From 144463f2351b042882b6d98a8b38c23fec85ad32 Mon Sep 17 00:00:00 2001 From: Alessandro Franceschi Date: Fri, 28 Aug 2026 14:34:31 +0200 Subject: [PATCH 3/5] ci: publish a GitHub Release from a v* tag A `v*` tag now runs the test matrix, builds every platform, and publishes a Release from the artifacts the build job already verified -- manifest checked, static linking confirmed, version stamp asserted -- rather than rebuilding bytes nothing has checked. A malformed tag fails before anything is built; a `-suffix` version publishes as a prerelease. `contents: write` is raised locally in the release job rather than at the workflow level, where it would also be handed to the test job that runs the code under review. Tag runs are never cancelled by concurrency: a published tag with no release is worse than a superseded run. The detached signature stays manual -- CI holds no signing key -- so the generated notes say plainly that the checksums show a download is intact, never where it came from, and docs/release.md carries the signing step. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 182 ++++++++++++++++++++++++++++++++++++--- docs/release.md | 46 +++++++++- 2 files changed, 214 insertions(+), 14 deletions(-) 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/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 From 5aa8547db7e0c7ad7ad413eb81e0ed8449cb3e2b Mon Sep 17 00:00:00 2001 From: Alessandro Franceschi Date: Fri, 28 Aug 2026 14:34:45 +0200 Subject: [PATCH 4/5] fix: stop reporting directory sources and injected aliases as changes Two sources of false or misleading output, both found against a deployed OpenVox installation. A File resource with `ensure => directory` or `recurse => true` names a directory tree, which the compiler's file_content endpoint cannot serve: it answers HTTP 500 with an empty body, for both the trailing-slash and the bare form. PIACE attempted retrieval anyway whenever such a resource's `source` changed, then reported the inevitable failure as content_indeterminate -- an error-severity verify_content diagnostic turning the whole target into an operational error, exit 30 -- as if content evidence had been lost, when byte-level evidence was never the right evidence for the resource. Resolution now stops between step 2 and step 3 for those resources: a changed reference is reported as reference_changed with a *warning*, so the fact is still recorded and no digest is claimed, but nothing failed and the outcome does not say it did. An unchanged reference with some other content-bearing parameter changed stays content_indeterminate. The predicate keys off `ensure` and `recurse`, never the trailing slash, which was present on only one side of the case that surfaced this. Separately, the PuppetDB terminus injects an `alias` parameter into a stored catalog for every resource whose namevar differs from its title. A compiler's own catalog carries none, through either v3 or v4, so comparing a PuppetDB baseline against a compiled candidate reported a spurious `alias: [...] -> null` for 9 of 53 resources on one measured node -- and `alias` was the only parameter present on one side and absent on the other. It is generated catalog metadata under requirements.md 5.9, and dropping it cannot hide a real difference: it only registers extra keys in the compiler's resource index and is never enforced on a node. The drop is symmetric, because a file baseline captured from PuppetDB carries it too. Co-Authored-By: Claude Opus 5 --- internal/filecontent/doc.go | 59 +++++++++- internal/filecontent/evidence.go | 106 ++++++++++++++++- internal/filecontent/evidence_test.go | 156 ++++++++++++++++++++++++++ internal/normalize/catalog_test.go | 53 +++++++++ internal/normalize/doc.go | 30 +++++ internal/normalize/value.go | 23 +++- 6 files changed, 419 insertions(+), 8 deletions(-) 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) From bc8056ae0e6aecd32b1403c3d8c62f7c6e1f41cf Mon Sep 17 00:00:00 2001 From: Alessandro Franceschi Date: Fri, 28 Aug 2026 14:34:55 +0200 Subject: [PATCH 5/5] docs: add a changelog for v0.1.0 and correct the PuppetDB write claim The README said PIACE does not "write anything to PuppetDB". That is true of PIACE itself -- every PuppetDB request it makes is a read -- but not of a run: on `catalog_api: v3` the compiler stores the facts submitted and the catalog it compiled, because that endpoint has no persistence control. The v4 path changes nothing, carrying `persistence: {facts: false, catalog: false}`. The section documenting this was already accurate; only the blanket sentence in the intro contradicted it. Adds a subsection spelling out the one workable v3 arrangement: a baseline captured to a file, which the candidate compilation cannot reach in to overwrite, produced by `piace capture catalog` at the same path `compare` later reads. It also records that config validation does not yet enforce requirements.md 1.8 -- `catalog_api: v3` with `baseline.source: puppetdb` loads, runs, and corrupts the baseline it just read. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 118 +++++++++++++++++++++++++++++++++++++++++++++++++ README.md | 121 ++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 224 insertions(+), 15 deletions(-) create mode 100644 CHANGELOG.md 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 8952d12..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,10 +67,21 @@ 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 @@ -80,13 +102,27 @@ 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. Resource changes are open; -dependency-graph edges, an estimate's PQL, request options and full node list, -the exclusion detail and the provenance block all sit in closed sections you -expand. 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. It is one self-contained file with a light background, no -webfonts and no images: `file://` is all it needs. +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 @@ -146,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 @@ -299,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, @@ -310,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 — @@ -388,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)