V0.3.0 ci ergonomics - #7
Merged
Merged
Conversation
`piace explain` accepts --debug and --debug-dump-dir, the two observation options compare and capture already had, so a rejected inference request can be diagnosed rather than guessed at. The bearer token is a header, so it reaches no dump file. services.inference gains token_limit_param, selecting max_tokens (the default) or max_completion_tokens: OpenAI's GPT-5 family rejects the former outright, and OpenAI-compatible servers other than current OpenAI only understand it. It also gains an optional temperature, sent only when set: PIACE previously hard-coded temperature and seed into every request, Claude 4+ and GPT-5 reject a non-default temperature with a 400, and seed never left a mark anywhere. Pinning them never made a model-generated assessment reproducible: a provider-side model revision still moves the bytes. Dependency-graph edge groups are no longer sent for assessment. An edge change is a consequence of the resource changes around it, carries no before/after pair to reason about, and a run's edges routinely outnumber its resource changes, so sending them spent the group budget and returned a wall of unknown risk indications. The deterministic report still lists every edge group; groups_total now counts what was eligible.
A services file's TLS paths resolved against the process working directory while snapshot paths resolved against the target file and policy_notes_file against the services file: three keys, three bases. On a runner the working directory is wherever the job happens to be standing, so the only reliable TLS form was an absolute path, and the absolute path of a per-job credential directory is not known until the job starts. That is what forced a services file to be a template a job rendered with sed before every run. One rule now covers all of them: a relative path in a config file resolves against the directory of the file that names it. ResolveServices takes that directory, token_file joins policy_notes_file in following it, and every path the resolver returns is absolute, so the transport's open error names one unambiguous location instead of a relative path the reader has to resolve by hand. For the case the template really existed for, each service also accepts ca_bundle_env, client_cert_env and private_key_env naming an environment variable that holds the path. A committed services file is then read in place, unmodified, and the same file works for the binary and the container. The variable carries a path and never key material, must hold an absolute one (it is named in no file, so there is no base it could resolve against), and naming both forms of the same credential is an error rather than a precedence rule nobody remembers, exactly as token_env and token_file already behave. TestAcceptance_TLSPathsResolveAgainstWorkingDirectory asserted the old asymmetry and called it a finding for a future task. This is that task, so it becomes TestAcceptance_TLSPathsResolveAgainstTheServicesFile, joined by the CI shape it unblocks and the both-forms-set rejection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every adopter of `explain --change` writes the same YAML in shell, and
the free-text part of that is the dangerous part. docs/ci.md spends three
paragraphs on it, with a different answer per platform: GitHub
substitutes ${{ }} into script text before a shell sees it, Azure
macro-expands $( ) in task inputs, GitLab expands in the running shell.
Get it wrong and a pull request titled `$(curl ...)` runs on a runner
that holds a credential able to read every catalog in the estate.
change-context takes the variable name instead of the value, so
attacker-controlled text never reaches a command line. There is
deliberately no --title or --description flag, only --title-env /
--title-file and --description-env / --description-file, and the refs
take an -env form too because a branch name may legally contain a
semicolon, a dollar sign and a backtick.
It execs git rather than vendoring an implementation of it: this module
has one dependency and that is worth keeping. It stays optional, so the
rule that held before still holds in the shape that matters: compare and
explain invoke no git and contact nothing but the compiler, PuppetDB and
the inference service, and explain --change still reads a file the caller
produced by any means, so a different VCS or a CI system with no checkout
describes its change exactly as before.
Commit subjects are collected, never bodies, and there is no flag to ask
for them: a body is unbounded text most likely to carry a customer name
or a pasted credential, and the reader refuses a body key outright.
Records come back NUL-separated with \x1f between fields, because a
subject may contain a tab or a newline and a path may contain a newline.
assess.EncodeChangeContext marshals through the same wire type
LoadChangeContext decodes, so the generator cannot drift from the reader,
and the acceptance test asserts exactly that round trip.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The release notes told a reader to verify a detached OpenPGP signature that did not exist yet: this workflow held no signing key, so a human attached SHA256SUMS.asc some time after publication. Meanwhile a checksum manifest published beside its own artifacts attests to integrity and never to origin, because anyone who could replace the binaries could replace the manifest with them. The instruction was unactionable exactly when a consumer would follow it. Keyless cosign closes that at publication time. The certificate is issued against the workflow's own OIDC identity and lives for minutes, so there is no key to store, rotate or lose, and the release notes now carry a verify-blob command with the exact certificate identity for the tag being published. The binaries also get a GitHub build provenance attestation, which answers a different question: which workflow, at which commit, produced these bytes. OpenPGP stays for sites that require it, as an extra rather than as the verification path. The image gains a second home. Docker Hub keeps the name the documentation uses, but an anonymous pull from a shared CI runner IP is precisely what Docker Hub rate-limits, and this workflow already logs in early to dodge that on the BuildKit frontend pull. Mirroring to GHCR costs one login with a token the job already holds and removes a pipeline failure that has nothing to do with this project. The pushed manifest is signed by digest in both registries and attested once. provenance: false stays. It exists to keep BuildKit's own attestation out of the manifest list, where Docker Hub renders it as an unknown/unknown architecture, and nothing is given up now that a real attestation is stored beside the image instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`piace change-context` replaces it, and does the part it did worst: the script emitted YAML with printf and left the caller to append a title and description by hand, which is the step that has to be got right on three CI platforms and is arbitrary code execution on the runner when it is not. Its test went with it. What that test checked, that a generated change context is one `explain --change` accepts, is now checked against the subcommand in cmd/piace/acceptance_changecontext_test.go, and assess.EncodeChangeContext marshals through the reader's own wire type, so the two can no longer drift apart in the first place. examples/change-context.yaml stays: a repository under a different VCS writes the file by hand, and that is the document showing its shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
109 files carried 578 pointers into .kiro/specs/piace: "per design.md section 10", "requirements.md 9.4's ...", "task 7's normalizer". The spec was a build-time artifact of how this repository was written, not a description of how it behaves, and tasks.md was a plan with checkboxes. Keeping them meant two documents to hold in sync; keeping the pointers without them meant 578 references to files nobody could open. Both are gone. Every comment now states the rule it was citing, which is the part a reader needed, and the pointer that made them look elsewhere for it is not there to follow. The same pass removed 49 references to numbered build tasks, replaced with the package that actually does the work. Three of those comments were not merely stale but wrong. internal/capture said production wiring supplies StubCompiler until the compiler adapter lands; the adapter landed, main.go wires it, and the stub has been test-only for some time. Its diagnostic said "not implemented yet". Fixed rather than reworded. Only 141 of the 578 citations were parenthetical enough to remove mechanically. The rest were load-bearing, subjects of their own sentences, and were rewritten by hand. The mechanical pass over the remainder damaged 17 comments by backtracking into a section number, so "See design.md section 2.1 (CLI surface)" left ".1 (CLI surface)" behind; each was found by scanning for the artifact and repaired. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Everything here described a tool that had accumulated: a services file that had to be rendered, a shell script that had to be copied into your repository, three container-based CI examples for a path the same page recommended against, a signature attached by hand some time after publication, and an ADR directory plus a 29 KB implementation plan recording how the code got written rather than what it does. The README is a third shorter. Its `explain` reference moved to docs/change-assessment.md, which is where a reader who has not decided to enable an inference service can skip it, and the sections that restated docs/ci.md are gone. docs/ci.md loses "Why the services file is a template", because it is not one any more, and gains the shape that replaced it: one committed services file, credentials named through `*_env`, nothing rendered and no tracked file a job mutates. CONTEXT.md keeps the glossary and takes the rationale the ADRs held, plus the path rule and the reason `change-context` is allowed to invoke git when nothing else is. It is now the one place the why lives, and the code comments that used to cite `docs/adr/000N` cite it. The CI examples are three, not six. Each says that the download step can be skipped when the binary is already on the runner, which is the honest answer for anyone who bakes it into a runner image or mirrors it internally, and the reason a GitHub Action wrapping five lines of install would not have earned its release process. One services example replaces two, showing both credential forms. The 0.2.0 implementation plan, the ADR directory and the docker CI twins are deleted rather than corrected. Also removes the 180 em dashes from every document, example and workflow comment in the repository. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The examples are the first thing a new user copies, and docs/ci.md now tells every reader to copy examples/ci/services.yaml verbatim. Decoding is strict, so one mistyped key is a hard load error the first time somebody runs it, and nothing in the suite would have noticed: three keys in that file did not exist before this branch. It found one on its first run. examples/ci/services.yaml named a policy-notes.md that does not sit beside it, so the file every CI reader was about to copy failed to load. It now points at the sample one directory up, and says what a real control repository writes instead. The two credential arrangements are exercised as they actually ship: a comparison job loading the merged file with the inference token unset, and an assessment job loading the same file with the mTLS variables unset. That is the whole one-services-file decision, checked rather than reasoned about. Also passes --head-ref-env in all three pipelines. A pull request checkout is detached at a temporary merge commit, so asking git for the branch name yields the literal "HEAD" and an assessment never learns which branch it is reading. And docs/ci.md no longer tells a GitLab job to clean up files it does not create: a file type variable is written outside the checkout by GitLab itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
172 paragraphs across 81 files. Every one was reread rather than search-and-replaced, so the pass also cleared what the earlier sweep had no reason to look at: comments pointing at documents that no longer exist, and two that were simply wrong. acceptance_fixture_test.go still said internal/transport resolves TLS paths against the process working directory rather than the services file, which is exactly what this branch reversed. internal/filecontent's package comment still called internal/diff a "future node differ" and named internal/normalize twice, the second a scar from the earlier task-to-package mapping. Three other doubled package names went with it. The rest is archaeology of the same kind the spec citations were: 70 references to numbered slices and requirements, and the "**Validates: Requirements 5.1**" footers under the property tests. A test's name and its first sentence say what it holds; a number pointing into a deleted document does not. The HTML report's <title> loses its em dash too, which changes every page a comparison writes and so the golden it is checked against. That golden exists to catch a byte the assessment feature adds to a report rendered without one, not to freeze the page, so it is regenerated and the test says which of the two it is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are a few concrete documentation/comment defects introduced in the updated code (typos and an incorrect LoadServicesFile contract statement) that should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR focuses on CI ergonomics and provider compatibility for the v0.3.0 change-assessment workflow: it replaces the shipped git-based script with an in-binary change-context writer, adds services-file support for env-var TLS path references (to avoid templating/copying services files in CI), and updates inference request shaping to work across differing OpenAI-compatible providers.
Changes:
- Replace
scripts/change-context.sh+ its integration test with a Go encoder and CLI-facingchange-contextworkflow. - Extend services configuration to support
*_envTLS path references and tighten path-resolution semantics (relative-to-config-file). - Update inference request/transport debug plumbing (token-limit param selection, optional temperature, observer/body-capture seam) and adjust HTML rendering CSS for assessment risk rows.
File summaries
| File | Description |
|---|---|
| scripts/change-context.sh | Removed legacy git-based change-context generator script. |
| internal/transport/testfixture_test.go | Comment wording cleanup. |
| internal/transport/redact.go | Comment wording cleanup around redaction boundary/contract. |
| internal/transport/errors.go | Comment wording cleanup around error taxonomy/classification. |
| internal/transport/doc.go | Package doc wording cleanup and clarifications. |
| internal/transport/debug.go | Comment wording cleanup for debug redaction boundary. |
| internal/transport/debug_test.go | Comment wording cleanup in tests. |
| internal/transport/authorization_test.go | Update references for authorization-header exception documentation. |
| internal/snapshot/store_test.go | Comment wording cleanup in tests. |
| internal/snapshot/envelope.go | Doc comment cleanup/clarification around envelope semantics. |
| internal/snapshot/envelope_test.go | Comment wording cleanup in tests. |
| internal/snapshot/checksum.go | Doc comment cleanup/clarification for checksum behavior. |
| internal/snapshot/checksum_test.go | Comment wording cleanup in tests. |
| internal/snapshot/canonical.go | Doc comment cleanup/clarification for canonical JSON rules. |
| internal/snapshot/canonical_test.go | Comment wording cleanup in tests. |
| internal/report/text.go | Comment wording cleanup + adjust a truncation line label. |
| internal/report/testdata/sample_report.golden.html | Update golden HTML (title punctuation + assessment row CSS). |
| internal/report/roundtrip_test.go | Remove references to obsolete plan docs in comments. |
| internal/report/options.go | Comment wording cleanup. |
| internal/report/json.go | Comment wording cleanup + keep impact-estimate label stamping rationale. |
| internal/report/html_template.go | Title punctuation change + add CSS + mark assessment rows with .risk class. |
| internal/report/decode.go | Comment wording cleanup around strict decoding and UseNumber. |
| internal/report/assessment_test.go | Rename/adjust test wording reflecting updated golden baseline. |
| internal/puppetdb/types.go | Comment wording updates + update cross-package references. |
| internal/puppetdb/select.go | Comment wording updates about backend selection. |
| internal/puppetdb/identity.go | Comment wording cleanup about identity computation. |
| internal/puppetdb/filesource_test.go | Comment wording cleanup in tests. |
| internal/puppetdb/adapter.go | Comment wording cleanup for adapter behavior and redaction-safe diagnostics. |
| internal/puppetdb/adapter_test.go | Comment wording cleanup in tests. |
| internal/normalize/value.go | Comment wording cleanup for parameter decode/canonicalization rules. |
| internal/normalize/property_test.go | Comment wording cleanup in property tests. |
| internal/normalize/catalog.go | Comment wording cleanup for normalization contract and ordering. |
| internal/model/result_test.go | Comment wording cleanup in tests. |
| internal/model/provenance.go | Comment wording cleanup for provenance structs and constants. |
| internal/model/outcome_test.go | Comment wording cleanup in tests. |
| internal/model/impact.go | Comment wording cleanup/clarification for impact estimate semantics. |
| internal/model/diagnostic.go | Comment wording cleanup/clarification for operations and taxonomy split. |
| internal/model/catalog.go | Comment wording cleanup/clarification for normalized model/value semantics. |
| internal/inference/request.go | Adjust request schema to support max token field variants + optional temperature. |
| internal/inference/client.go | Add options/observer/body-capture debug seam + emit structured events. |
| internal/impact/querier_test.go | Comment wording cleanup in tests. |
| internal/impact/pql.go | Comment wording cleanup for PQL construction and escaping rules. |
| internal/impact/estimate_all.go | Comment wording cleanup/clarify dedupe and sequential issuance. |
| internal/impact/estimate_all_test.go | Comment wording cleanup in tests. |
| internal/filecontent/resolver.go | Comment wording cleanup for body-echo avoidance and safety rationale. |
| internal/filecontent/interfaces.go | Comment wording cleanup for content-retrieval contract and safety constraints. |
| internal/filecontent/evidence.go | Comment wording cleanup for evidence priority ordering and step semantics. |
| internal/filecontent/evidence_test.go | Comment wording cleanup in tests. |
| internal/exitcode/exitcode.go | Comment wording cleanup around precedence/mapping. |
| internal/exitcode/exitcode_test.go | Comment wording cleanup in tests. |
| internal/diff/resources.go | Comment wording cleanup around file content parameter collapsing. |
| internal/diff/redact.go | Comment wording cleanup around redaction invariants and selector semantics. |
| internal/diff/fingerprint.go | Comment wording cleanup around fingerprint purpose and canonicalization. |
| internal/diff/exclusion.go | Comment wording cleanup around exclusion semantics and reporting. |
| internal/diff/edges.go | Comment wording cleanup around normalization assumptions. |
| internal/diff/diff.go | Comment wording cleanup (unreachable fingerprint error path rationale). |
| internal/diff/diff_test.go | Comment wording cleanup in tests. |
| internal/config/target.go | Schema comment cleanup (currently has a broken parenthetical; see review comment). |
| internal/config/target_test.go | Update comment to reflect resolver ownership. |
| internal/config/services.go | Add *_env TLS path references (currently has a typo in comment; see review comment). |
| internal/config/resolve/types.go | Comment wording cleanup for resolved target/endpoint models. |
| internal/config/resolve/target.go | Comment wording cleanup for resolution behavior and invariants. |
| internal/config/resolve/provenance.go | Comment wording cleanup for provenance redaction boundaries. |
| internal/config/resolve/override.go | Comment wording cleanup for override semantics. |
| internal/config/resolve/override_test.go | Comment wording cleanup in tests. |
| internal/config/resolve/load.go | Update services resolution call signature + comment update (comment currently incorrect; see review comment). |
| internal/config/resolve/inference.go | Add token_limit_param + optional temperature + path resolution helper. |
| internal/config/resolve/inference_test.go | Add tests for new inference sampling/config knobs. |
| internal/config/resolve/errors.go | Comment wording cleanup for validation accumulator behavior. |
| internal/config/resolve/doc.go | Comment wording cleanup for package scope boundary. |
| internal/config/resolve/decode.go | Comment wording cleanup for strict YAML decoding rationale. |
| internal/config/inference.go | Add token_limit_param + optional temperature fields to services schema. |
| internal/compiler/request.go | Comment wording cleanup for trusted-facts policy decision. |
| internal/compiler/property_test.go | Comment wording cleanup in property tests. |
| internal/compiler/adapter.go | Comment wording cleanup for adapter policy responsibilities. |
| internal/compiler/adapter_test.go | Comment wording cleanup in tests. |
| internal/compare/workflow.go | Comment wording cleanup for workflow ordering and warning handling. |
| internal/compare/workflow_test.go | Comment wording cleanup in tests. |
| internal/capture/workflow_test.go | Comment wording cleanup in tests. |
| internal/capture/doc.go | Comment wording cleanup/clarify capture destination behavior. |
| internal/capture/compiler.go | Comment wording cleanup for compiler requester interface contract. |
| internal/capture/compiler_stub.go | Update stub diagnostic message + comment wording cleanup. |
| internal/assess/testfixture_test.go | Comment wording cleanup in tests. |
| internal/assess/testdata/request.golden.json | Update golden request JSON to remove temperature/seed + reflect group selection change. |
| internal/assess/pseudonym.go | Comment wording cleanup for pseudonym assignment/reveal behavior. |
| internal/assess/produce.go | Comment wording cleanup (meta fields + partial-input detection). |
| internal/assess/interpret.go | Comment wording cleanup + clarify GroupsTotal semantics. |
| internal/assess/interpret_test.go | Update tests to reflect dropped/filtered groups behavior and message tweaks. |
| internal/assess/changecontextwrite.go | New: encode change context YAML via shared wire shape. |
| internal/assess/changecontextscript_test.go | Removed script-based integration test. |
| internal/assess/changecontext.go | Update change-context docs + add omitempty to wire fields to support encoding. |
| internal/assess/changecontext_test.go | Comment wording cleanup in tests. |
| internal/assess/artifact.go | Comment wording cleanup for assessment artifact determinism and file mode rationale. |
| internal/aggregate/doc.go | Comment wording cleanup/shorten and clarify aggregation/fingerprint contract. |
| internal/aggregate/build.go | Comment wording cleanup around kind ordering. |
| internal/aggregate/build_test.go | Comment wording cleanup in tests. |
| examples/targets-v3-legacy.yaml | Comment punctuation/wording adjustments. |
| examples/targets-snapshot-baseline.yaml | Comment punctuation/wording adjustments. |
| examples/targets-puppetdb-baseline.yaml | Comment punctuation/wording adjustments. |
| examples/services-with-inference.yaml | Removed combined services example (superseded by updated examples). |
| examples/services-explain-only.yaml | Expand explain-only services example to cover multiple providers and new knobs. |
| examples/policy-notes.md | Minor wording/punctuation adjustments. |
| examples/ci/services.yaml.tmpl | Removed templated CI services file example. |
| examples/ci/services.yaml | New: CI services file using *_env TLS path references + inference section. |
| examples/change-context.yaml | Update generator guidance to piace change-context. |
| docs/research/trusted-facts-in-existing-catalog-diff-tools.md | Minor wording/punctuation adjustments. |
| docs/adr/0003-authenticate-the-inference-service-with-a-bearer-token.md | Removed ADR (content consolidated elsewhere). |
| docs/adr/0002-keep-the-change-assessment-out-of-the-result-document.md | Removed ADR (content consolidated elsewhere). |
| docs/adr/0001-request-candidate-catalogs-from-an-existing-compiler.md | Removed ADR (content consolidated elsewhere). |
| cmd/piace/release_test.go | Comment wording cleanup for dependency/CGO checks. |
| cmd/piace/main_test.go | Comment wording cleanup for CLI/config error behavior. |
| cmd/piace/examples_test.go | New: tests that shipped examples load under strict decoding and intended command boundaries. |
| cmd/piace/capture_test.go | Comment wording cleanup for trusted-facts failure attribution. |
| cmd/piace/acceptance_tls_test.go | Comment wording cleanup for transport hostname verification reference. |
| cmd/piace/acceptance_snapshot_test.go | Comment wording cleanup + clarify assertions. |
| cmd/piace/acceptance_service_test.go | Comment wording cleanup for endpoint-contact assertions. |
| cmd/piace/acceptance_fixture_test.go | Comment wording cleanup + clarify fixture assumptions. |
| cmd/piace/acceptance_determinism_test.go | Comment wording cleanup for determinism assertions. |
| cmd/piace/acceptance_debug_test.go | Comment wording cleanup for debug output/dump semantics. |
| cmd/piace/acceptance_compat_test.go | Comment wording cleanup for v3 warning/fallback behavior assertions. |
Review details
- Files reviewed: 183/183 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+39
to
+43
| // from path. Relative TLS paths resolve against path's containing | ||
| // directory. It performs no network or service I/O; only local filesystem | ||
| // access to read path itself, and to read any file an environment variable | ||
| // the file names points at. TLS file existence/readability is validated | ||
| // later, at transport construction (internal/transport). |
Comment on lines
+7
to
+9
| // ServicesFile is the root document of a `--services` YAML file. It | ||
| // keeps endpoint and mTLS settings out of the reviewable target | ||
| // selection file;2 ("Service configuration"). |
Comment on lines
5
to
+9
| // rejection), default resolution, path/glob validation, and cross-field | ||
| // validation are implemented separately (see design.md section 3, | ||
| // "Target configuration resolution"). Keeping the schema and the resolver | ||
| // in separate concerns lets later work build a resolved model on top of | ||
| // this stable wire shape without churning the wire shape itself. | ||
| // | ||
| // Design reference: design.md section 2.2 ("Service configuration") and | ||
| // section 3 ("Target configuration resolution"); requirements.md section 8 | ||
| // ("Target-file shape"). | ||
| // validation are implemented separately (, "Target configuration | ||
| // resolution"). Keeping the schema and the resolver in separate concerns | ||
| // lets later work build a resolved model on top of this stable wire | ||
| // shape without churning the wire shape itself. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.