Gate the derived Data/ and Reports/ artifacts on pull requests - #130
Merged
juemerson-at-purestorage merged 12 commits intoAug 20, 2026
Merged
Conversation
Allow artifact verification to use a freshly regenerated response-shape map while preserving the committed path as the default, and cover the contract with edition-agnostic AST tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Data/ and Reports/ hold eleven artifacts generated by tools/Build-*.ps1, and nothing regenerated them on a pull request -- update-api-capability-map.yml only rebuilds on schedule, on push to main, and on workflow_dispatch. A PR touching a generator, or adding a Public/ cmdlet the generators read, landed with stale committed artifacts and a green run; the same drift then had to be found by hand twice (dmann000#96, dmann000#97). scripts/Assert-PfbDerivedArtifacts.ps1 regenerates into a scratch directory and compares. Two decisions carry the design: - The spec set is PINNED to Data/PfbCapabilityMap.json's generatedFrom list and staged file-by-file, so a newly published REST version cannot leak in and red an unrelated PR. That separation of PR-caused drift from spec-publication drift is what makes the check safe to be blocking, where the pre-existing one in update-api-capability-map.yml had to stay advisory. - Comparison hashes CRLF-normalized text, not raw bytes. The repo has no .gitattributes, so Windows checkouts hold Reports/*.md as CRLF while the generators emit LF -- byte comparison would pass in CI and fail for every Windows developer. Downstream generators are fed the regenerated upstream artifacts, not the committed ones, or a PR changing Build-PfbCapabilityMap.ps1 would produce a drift report built from the stale map -- a false pass on the exact case the gate exists for. -Artifact runs only the generators needed for the requested subset plus their transitive inputs, which is what makes a local run usable. Data/PfbVersionMap.json is excluded: Update-PfbVersionMap.ps1 needs SSOT_API_KEY, absent on fork PRs, so it would fail for want of a credential rather than for staleness. Tests assert on the script's source rather than running it -- a full regeneration is minutes and needs the gitignored tools/specs/ cache. They are not edition-gated and run on both 5.1 and 7. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five Important findings from review of 149032c. 1. Rename ConvertTo-PfbRepoRelativePath -> ConvertTo-PfbCanonicalArtifactPath. tools/lib/PfbApiDriftTools.ps1:1573 already owns that name with a different arity (-Path AND -RepoRoot) and a RequiredDescribes-listed test. Safe today only because each generator dot-sources the lib into its own scope, and this gate is the only script that invokes them in-process -- one refactor away from whichever loads last silently winning. 2. Rewrite the stale-artifact remediation text. Build-PfbApiDriftReport.ps1, Build-PfbValueEnumMap.ps1 and Build-PfbFieldCmdletMap.ps1 scan the whole spec directory and record what they find (availableSpecVersions, versionDivergenceWarning, processed-version counts), so a bare generator run against a cache newer than the pin re-creates the drift -- and the old message's "not by a newly published REST version" was inverted in exactly that state. Now leads with the reliable fix (-KeepWorkDirectory, copy the scratch output), states the pinned count and range, and names the three directory-scanning generators. The false causal claim is gone. 3. Extract the artifact list from the AST and assert set equality both ways. The substring match was defeated for the artifact at the ROOT of the graph: 'Data/PfbCapabilityMap.json' also appears in the spec-pinning code, so dropping it from $ArtifactPlan left the test green while the gate stopped checking the capability map. The exclusion test now reads the same AST list rather than depending on the description's prose staying unquoted. 4. Guard both load-bearing decisions, which had none. This matters most for the response-shape-map edge: Build-PfbApiDriftReport.ps1:141-156 treats a missing shape map as an optional degradation, so dropping that DependsOn would NOT throw -- it would compare a report with empty response findings, reddening today only because the committed report happens to carry non-empty responseFieldRemovals. A subset-path-only false pass, invisible to a full-set run. Adds structural assertions over $stepArguments (no downstream input resolves against $repoRoot; specs come from the staged dir) and over per-version staging (no wildcard, no recursive copy). 5. Refuse a scratch directory the script does not own. -WorkDirectory tools made $stagedSpecsDir the real tools/specs -- New-Item -Force no-ops, staging wrote into the live cache, and the forced recursive cleanup then deleted it. Now throws if -WorkDirectory resolves under the repo root or already holds a specs/out subdirectory, and creates nothing until both guards pass. The three Minor findings are deferred per the review and untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Residual from the Task 2 re-review. Test-only: the gate script is byte identical, so its runtime behaviour is unchanged. The $stepArguments guard binds the argument EXPRESSIONS each generator is handed. It does not bind $ArtifactPlan's DependsOn column, which decides which steps a subset run actually EXECUTES -- so dropping an edge left every test green. The two fail differently: a wrong expression points a generator at the committed tree, while a missing edge leaves the expression correct but never regenerates the file it names. That gap lands hardest on ApiDriftReport -> ResponseShapeMap, because Build-PfbApiDriftReport.ps1:141-156 treats a missing -ResponseShapeMapPath as an optional degradation rather than an error. Dropping the edge does not throw: the subset run never regenerates the shape map, the report is built with empty response findings, and it reds today only because the committed report happens to carry non-empty responseFieldRemovals. If those ever legitimately empty out it becomes a silent false pass -- on the subset path only, which CI never exercises because CI always runs the full set. The subset path is the LOCAL path, and being trustworthy before a push is why this logic lives in scripts/ at all. Extends the AST extraction to collect Step and DependsOn alongside Artifacts, and pins all seven steps' edges by set equality in both directions, so a dropped edge and a spurious one each fail. Mutation-tested: dropping 'ResponseShapeMap' from ApiDriftReport, dropping PipelineSelectorMap's only edge, and adding a bogus edge to DeadKeyReport are all killed on both editions. Also corrects the comment on the $stepArguments test, which claimed to cover a dropped DependsOn edge. It never did; it now states what it binds and what it does not, and points at the new guard. The two Minor findings from the re-review (finding-5 test matching prose, and the repo-containment check comparing unresolved paths) are deferred per instruction and untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds .github/workflows/verify-derived-artifacts.yml, a thin workflow calling scripts/Assert-PfbDerivedArtifacts.ps1 on PRs that touch Public/, Private/, tools/, Data/ or Reports/. The gate can be blocking because the script pins its spec set to the committed capability map's generatedFrom list, so a newly published REST version cannot red an unrelated PR. Documents the gate and the -Artifact fast path in Reports/README.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The paths filter excluded both files that make up the gate, so a PR editing only the workflow or only Assert-PfbDerivedArtifacts.ps1 never ran it -- the one change most likely to break the gate and the only one no other check covers. Adds exactly those two globs and no more; each glob costs a ~5-minute job and nothing else can change what the generators produce. Also corrects the header's claim about update-api-capability-map.yml: its push trigger is branches [main] AND paths ['Public/**'], so it does not blanket-own main. The conclusion (no push trigger here) is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PureStorageFlashBladePowerShell.psd1 and .psm1 decide which functions are exported, and Build-PfbPipelineSelectorMap.ps1 probes the exported set (tools/lib/PfbPipelineSelectorTools.ps1:123). Neither matched any trigger glob, so a PR adding or removing an export without touching Public/ moved Reports/PfbPipelineSelectorMap.json and merged green -- the exact false pass this gate exists to eliminate. Also rewrites the paths comment, which carried two claims that were not true: nothing outside the list can change generator output (falsified by this very finding), and the gate's own files are covered by no other check (they are, via Tests/AssertDerivedArtifacts.Tests.ps1 on an unfiltered pull_request). The narrower accurate claim is that no other check exercises the gate end to end, and the list is now described as a best effort rather than a closed set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…atch Both guards over scripts/Assert-PfbDerivedArtifacts.ps1 asserted against the script's prose rather than its behaviour, and mutation testing showed both surviving the exact change they were written to block. The spec-pin guard was `$gateSource | Should -Match 'generatedFrom'`. That string also appears in the file's own doc comment, in two throw messages and in a Write-Host line, so replacing the pinned read with a directory scan of tools/specs -- unpinning the gate outright, the one decision that makes it safe to block a build on -- left the test green. It now binds the AST: the assignment to $pinnedVersions must read generatedFrom off the committed capability map, and the staging loop must iterate $pinnedVersions and copy inside it. The scratch-directory guard matched the two throw messages verbatim. Moving both refusals to after the directory is created -- which is what authorises the recursive delete in the finally block, so the guards can no longer prevent anything -- left the messages untouched and the test green. It now binds the if conditions and asserts both guards precede the creation. Each replacement was re-verified by re-running the mutation: the new assertions fail where the old ones passed. Also in this commit: - Keep the CI scratch tree and upload out/ on failure. A red run reported which artifact was stale but deleted the evidence of how it differed, which is the only thing a reader actually needs. specs/ is excluded -- it is the ~50MB pinned spec set and reproducible from the cache. - Justify timeout-minutes from observed CI runs (303-340s cold) rather than a local warm-cache measurement alone. - Correct the trigger comment: Tests/AssertDerivedArtifacts.Tests.ps1 parses the gate's source and never executes it, so "exercises the script" overstated it. - Fix Reports/README.md, which listed five generators under a claim of eleven artifacts. Seven generators produce them, and the two missing ones write the Data/ maps that three of the others read -- so the documented order would have regenerated downstream reports against stale upstream inputs.
update-api-capability-map.yml already declared its permissions; the other three inherited whatever the repository's default_workflow_permissions setting happens to be. That default is a repo setting, not a property of these files -- an admin can flip it to write and every undeclared workflow silently gains write access to the repository with no diff anywhere to show for it. This fork currently defaults to read, but the upstream repo's setting is not readable without admin, so the workflows should say what they need rather than depend on it. All three genuinely need only read: - verify-derived-artifacts.yml regenerates into a scratch tree and compares. - cross-platform-tests.yml checks out, moves the spec cache and artifacts between jobs, and runs Pester. Cache and artifact actions authenticate with ACTIONS_RUNTIME_TOKEN, not GITHUB_TOKEN, so they are unaffected. - publish-to-gallery.yml publishes with PSGALLERY_API_KEY. It creates no release, pushes no tag and opens no PR; scripts/Publish-Gallery.ps1 and scripts/build.ps1 are local file operations plus Publish-Module. publish-to-gallery.yml declares it at workflow level rather than on the publish job, because a caller's permissions cap what a called workflow receives -- job level would leave the `test` job's workflow_call into cross-platform-tests.yml running on the repository default, which is the gap being closed. update-api-capability-map.yml keeps contents: write and pull-requests: write. It opens PRs via peter-evans/create-pull-request and is the only workflow here that writes anything.
`on: push:` and `on: pull_request:` were both unfiltered, so a commit pushed to a branch with an open PR matched both and ran the matrix twice -- 10 jobs for one commit, with Windows pwsh alone near 17 minutes on each side. Filtering `push` to main leaves one run per commit. Branch commits keep their coverage: `pull_request` still fires on every push to a branch with an open PR. The only case that loses a run is a commit pushed before its PR exists, and the run being dropped is the weaker of the two -- a `pull_request` run tests the merge commit rather than the branch tip, so it tests the tree that would actually ship. Fork branches never ran upstream workflows in the first place, so upstream CI was already PR-driven. `workflow_call` is untouched, so publish-to-gallery.yml's release gate still runs the full matrix on a tag. This also matches update-api-capability-map.yml, which already filtered its push trigger to main. Note this is not what a concurrency group would have fixed: concurrency stops an obsolete run when a NEWER commit arrives, whereas this is two runs of the SAME commit. Both are worth having; only this one is in scope here.
Reports/README.md documented the seven report generators and the gate over them, but not the other class of generator in tools/: Update-PfbContextHelp.ps1 and Update-PfbEmptyPipelineGuards.ps1 edit Public/ cmdlet source in place rather than emitting a report. The repo already encodes the distinction in the naming -- Build-Pfb* emits Data/+Reports/, Update-Pfb* mutates tracked source -- but this file never said so, and the obvious next step on reading it was to add the two Update- scripts to $script:ArtifactPlan. That would be wrong twice over. Both are already drift-checked at the same strength as the gate, by a -WhatIf run against the real tree asserting zero would-be changes; and the gate's mechanism does not fit a generator that edits tracked source in place rather than writing fresh output into an empty directory. Say all of that, name the tests that carry the assertion for each, and record what a future source generator needs instead of a plan row. Also drop a stray blank line that was splitting the artifact table, leaving the PfbPipelineSelectorMap row rendering as its own one-row table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
They are merged pull requests, not issues. Checked before citing them in the PR body for this branch; correcting the workflow comment to match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 20, 2026
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
1 participant
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.
Why
Nothing regenerated the eleven derived
Data/andReports/artifacts on a pull request.update-api-capability-map.ymlrebuilds them, but only on a schedule, on push tomain, andon
workflow_dispatch. So a PR that changed a generator — or added aPublic/cmdlet thegenerators read — could land with stale committed artifacts and a fully green CI run. That
drift then had to be found by hand, twice (#96, #97).
This adds one question, asked on the PR that causes it: does every committed artifact still
match what its generator produces?
What
scripts/Assert-PfbDerivedArtifacts.ps1— regenerates all seven generators into athrowaway directory and diffs against the committed copies. The dependency graph lives in
$script:ArtifactPlan, soBuild-PfbApiDriftReport.ps1is always fed a freshly builtcapability map rather than whatever is on disk.
-Artifactnarrows a run to one artifact andthe generators feeding it;
-KeepWorkDirectoryleaves the regenerated output on disk, whichis by definition what the comparison demanded and therefore the reliable fix for a reported
difference. The working tree is never written to, and the script refuses a
-WorkDirectoryinside the repo.
.github/workflows/verify-derived-artifacts.yml— runs the above onpull_request,path-filtered to the inputs that can actually move an artifact (
Public/,Private/,tools/,Data/,Reports/, the manifest and the root module, and the gate's own files).Why this gate can block when the existing regeneration check could not: every generator reads
tools/specs/. A gate run against whatever spec versions happen to be on disk would red anunrelated PR the moment a new REST version is published — which is exactly why the check in
update-api-capability-map.ymlwas deliberately left advisory. This script instead pins thespec set to the committed
Data/PfbCapabilityMap.json's owngeneratedFromlist, whichseparates PR-caused drift from spec-publication drift. That pinning is the whole reason it is
safe to fail a build on.
Tests/AssertDerivedArtifacts.Tests.ps1— the gate's own invariants: that the plancovers all eleven artifacts and no more, that each of the seven generators is named and
wired, that
DependsOnis honoured, and thatData/PfbVersionMap.jsonstays excluded onpurpose (its generator needs
SSOT_API_KEY, which GitHub will not expose to a fork PR, soincluding it would fail the gate for want of a credential rather than for staleness).
tools/Build-PfbPipelineSelectorMap.ps1— adds a-ResponseShapeMapPathoverride so thegate can hand it the freshly regenerated shape map instead of the committed one, with a test
for the parameter contract.
Two unrelated CI fixes found while working here, kept as separate commits:
cross-platform-tests.ymlfilterspushtomain. An unfilteredpushmatched everybranch commit, and a commit on a branch with an open PR also matches
pull_request— soone push ran the whole matrix twice, 10 jobs for a single commit, with Windows pwsh alone
near 17 minutes on each side.
permissions: contents: readon the three workflows that only read. Without a permissionsblock the
GITHUB_TOKENinherits whatever the repo'sdefault_workflow_permissionssettinghappens to be — a setting an admin can flip to write with no change to any file here.
Reports/README.md— documents the regeneration order and why it is not cosmetic, how torun the gate locally, and the distinction the repo already encodes in its naming but had never
written down:
Build-Pfb*emitsData/+Reports/artifacts, whileUpdate-Pfb*(
Update-PfbContextHelp.ps1,Update-PfbEmptyPipelineGuards.ps1) edits trackedPublic/source in place. Those two are deliberately not in
$script:ArtifactPlan: they are alreadydrift-checked at equivalent strength by a real-tree
-WhatIffixed-point assertion, and thegate's regenerate-into-an-empty-directory model does not fit a generator that mutates tracked
source.
Verification
mainatea4d3ea,i.e. after [P0] Empty list results return a coercing total_item_count wrapper; piping it can silently return the full collection #121 landed — worth stating, because [P0] Empty list results return a coercing total_item_count wrapper; piping it can silently return the full collection #121 changed 131
Public/cmdlets andregenerated no artifact. It legitimately owed none: the run confirms its guard insertion is
coverage-neutral, including for
Reports/PfbPipelineSelectorMap.json, whose generator thisPR also modifies.
unrelated spec-version bump is mixed into this diff.
unchanged from before the merge, so no new skips.
5.1 invocation is unreliable when launched from a pwsh 7 parent: 5.1 inherits pwsh 7's
PSModulePath, resolvesMicrosoft.PowerShell.Utilityto pwsh 7's copy, and losesImport-PowerShellDataFile(a manifest-exported function in 5.1, a compiled cmdlet in 7),which
scripts/Assert-PfbTestCoverage.ps1depends on. That is a local-harness artifact withno bearing on CI, where the 5.1 job has a clean module path — but it means the honest answer
is that CI is the first real 5.1 measurement of this branch.
tools/,scripts/,Tests/, workflows and docsonly, so the usual live-FlashBlade verification does not apply.
Known gap
pushis filtered tomainon both workflows, so a branch commit pushed before its PRexists gets no run at all.
pull_requestfires on every subsequent push to a branch with anopen PR, and a
pull_requestrun is the better of the two anyway because it tests the mergecommit rather than the branch tip. Flagging it as a deliberate trade rather than an oversight.
Not included
No version bump and no
CHANGELOG.mdedit — those stay a separate maintainer decision.🤖 Generated with Claude Code