ci: run the ~23% of the suite CI was silently skipping, and gate against it recurring (#63) by juemerson-at-purestorage · Pull Request #107 · dmann000/fb-powershell

juemerson-at-purestorage · 2026-08-10T17:15:41Z

Fixes #63.

The problem

tools/specs/ is a ~50MB cache of raw OpenAPI specs. It is gitignored, because it is a build input rather than source, so on a bare CI runner it does not exist. Every tooling test gates itself on the cache being present and skips gracefully when it is not — so the jobs reported success while roughly 23% of the suite never ran, including the absolute-path regression guards added in PR #62.

The second-order trap is worse than the headline number. A Describe whose guard evaluates false at BeforeAll time contributes neither a skip nor a pass: it vanishes from the result tree entirely. A healthy-looking skip count therefore proves nothing, and a green summary is indistinguishable from a silently-absent assertion.

The fix, on four axes

1. The guarded function is now directly unit-testable. ConvertTo-PfbRepoRelativePath moved out of tools/Build-PfbApiDriftReport.ps1 (a param()-headed script that cannot be dot-sourced without running the whole build) into tools/lib/PfbApiDriftTools.ps1, which the builder and Tests/PfbApiDriftTools.Tests.ps1 already dot-source. Six new ungated tests cover it: root stripping, case-insensitivity, POSIX separators, the never-emit-an-absolute-path invariant, the outside-root fallback, and null/empty passthrough.

This axis matters because retargeting the guards at the committed report — the issue's own first recommendation — does not satisfy acceptance criterion 3 on its own. Breaking the function does not alter a file that is already committed, so the regression would still pass.

2. The committed artifacts are guarded with no spec cache required. New self-contained Tests/CommittedDriftReport.Tests.ps1 reads Reports/PfbApiDriftReport.json with plain ConvertFrom-Json and asserts, on all four legs: the artifacts exist and are non-vacuous; no absolute path appears in unresolvedParameters, in enriched missingBodyProperties[].target.file, or in the markdown; and the hand-authored annotations plus summary fields survived generation.

A new file rather than a block appended to Build-PfbApiDriftReport.Tests.ps1: that file's top-level BeforeAll invokes a #Requires -Version 7.0 script and uses ConvertFrom-Json -Depth, and every Describe in it is PS7-gated. One ungated block there would drag the BeforeAll onto 5.1 and take all 65 tests down with it — the exact container-failure shape documented in the run-pester-tests notes.

3. A prepare-specs job materialises the cache once per run. It restores, fetches only newly-published versions, hard-fails via scripts/Assert-PfbSpecCache.ps1 if the result is below a floor, saves, and publishes an artifact the test legs download. The cache key prefix is deliberately shared with update-api-capability-map.yml so a warm cache written by either workflow serves both. An artifact rather than four per-leg restores: it fetches from the spec index at most once per run and is immune to cache key/branch scoping differences across the matrix.

The floor is 20 rather than the current 29, so a newly-published REST version cannot red the build on arrival.

4. A two-part coverage gate makes the next hole loud. scripts/Invoke-PfbCiPester.ps1 replaces the Pester block that was copy-pasted in three places across two workflows, and calls scripts/Assert-PfbTestCoverage.ps1:

RequiredDescribes — a named allowlist per edition. Catches a Describe that disappears from the result tree, which a skip ceiling structurally cannot see.
MaxSkipped — a ceiling with headroom, counting NotRun as well as Skipped, since Pester reports a discovery-time skip as the former.

The gate takes a result object rather than running Pester itself, so Tests/CiCoverageGate.Tests.ps1 exercises it fully with a hand-built stand-in — no suite run, no cache dependency. Nine ungated tests, the headline one asserting that a run reporting zero skips still fails when a required Describe is absent. It also pins the real baseline: both editions defined, and every allowlist entry actually present in some test file, so a rename or typo fails here rather than reddening CI forever for the wrong reason.

Baselines are per-edition by necessity. Every tooling Describe carries -Skip:($PSVersionTable.PSVersion.Major -lt 7), so the two legs differ by ~170 skips by design; one shared ceiling would be either a permanent false red on 5.1 or useless on 7.

The gate runs before the failure exit, so a run with both a real failure and a coverage regression reports both.

update-api-capability-map.yml additionally captures committed artifact hashes before regeneration and reports the diff. It reports only — it does not fail — since a legitimate spec update is expected to change those files.

Where the new scripts live, and one `.gitignore` fix

The three new scripts go in scripts/, alongside build.ps1 and Publish-Gallery.ps1, because that is what they are. The repo already has two distinct directories with a real boundary:

	`tools/`	`scripts/`
Purpose	the API capability-map generator toolchain	repo lifecycle / CI plumbing
Output	committed artifacts — `Data/.json`, `Reports/`	no artifact; it performs an action
Invoked by	`update-api-capability-map.yml`, maintainer on demand	`publish-to-gallery.yml`, CI

Invoke-PfbCiPester.ps1 is the direct sibling of Publish-Gallery.ps1 — a workflow step extracted into a script. Assert-PfbSpecCache.ps1 and Assert-PfbTestCoverage.ps1 assert and gate; they generate nothing. None of the three belongs in the generator toolchain.

Separately, .gitignore carried a blanket tools/ rule, added as a drive-by in 06ccd6e (a commit about a WinPS 5.1 crash from PKCS#8 fixtures) with the comment "Not yet decided whether this should be tracked -- excluded for now". That decision has effectively been made — 14 files under tools/ are tracked, including a maintained tools/README.md. The rule only affected new files, and silently: git does not descend into an ignored directory, so a newly created tools/*.ps1 produces no untracked marker at all.

It was also shadowing the rule that does the real work. Because the last matching pattern wins:

before:  git check-ignore -v tools/specs  →  .gitignore:46:tools/
after:   git check-ignore -v tools/specs  →  .gitignore:39:tools/specs/

The purpose-built, carefully-commented tools/specs/ exclusion was dormant. Removing the blanket rule reactivates it, and is a verified no-op for anything tracked or ignored today: 29 untracked paths under tools/, all 29 under tools/specs/, 0 elsewhere; tracked count unchanged at 14; only .gitignore itself appears in git status. No test reads .gitignore, and the capability-map workflow's change check is path-scoped to -- Data Reports.

Happy to split this into its own PR if you would rather keep it separate — it is one commit (dec8fe5) and reverts cleanly.

Results

Runs 31359783827 and 31360895252, both 5/5 green.

Leg	Before	After
ubuntu-latest, pwsh	503 passed / 0 failed / 154 skipped	1842 / 0 / 2
windows-latest, WinPS 5.1	—	1675 / 0 / 169

All nine required Describe blocks report OK. The six that were previously invisible contribute 10, 14, 1, 1, 4 and 3 tests. prepare-specs completes in 13s on a warm cache, confirming the shared key prefix works.

The 169 skips on 5.1 were enumerated across 28 blocks and characterised: 165 are tooling/drift tests, PS7-gated because every script under tools/ carries #Requires -Version 7.0. The remaining 4 are shipped module code, all edition-complementary — encrypted-PKCS#8 JWT minting (2, needs a PS6+ API, with its 5.1 error path explicitly tested), the paired variant of the never-log-the-private-key-password assertion (1; the general form of that assertion is ungated and does run on 5.1), and Set-PfbTlsProtocol's "no-op on PowerShell 7+" case (1). pwsh 7's only two skips are exactly the 5.1-only tests, which is the symmetry proving nothing is uncovered on both legs.

Notes for review

MaxSkipped is a ceiling, not a pin. Raising it deliberately in a reviewed diff, with the reason in the commit message, is the intended workflow when a change legitimately adds skips.
One verification gap, stated plainly: the gate's logic is proven by unit test (a zero-skip run with a required Describe absent throws), but the wiring — a gate throw becoming a non-zero exit and reddening the job — was never exercised against CI with a deliberate break. Cheapest way to close it is a scratch commit dropping the Download API spec cache step from test-pwsh only, confirming that job goes red, then discarding the commit.
No version or CHANGELOG changes, per the maintainer-owned release process.
Out of scope, noted for visibility: the scheduled update-api-capability-map run has failed at its Open pull request step on every run since 2026-07-24. Unrelated to this change, and excluded per the discussion on the issue.

🤖 Generated with Claude Code

…it-test it (dmann000#63) The guards that assert the drift report emits no absolute path all run against a FRESHLY REGENERATED report, so they depend on tools/specs/ -- a gitignored build cache that never exists on a CI runner. They reported "skipped", the job reported success, and the regression they exist to catch was unprotected. Re-pointing those guards at the committed report (next commit) removes the cache dependency, but cannot catch a regression in this function: breaking it does not alter a file that is already committed. Only a direct unit test does. The function lived inside tools/Build-PfbApiDriftReport.ps1, a param()-headed script that cannot be dot-sourced without running the whole report build, so it was untestable in isolation. Moved to tools/lib/PfbApiDriftTools.ps1, which the builder already dot-sources and the tests already load. -RepoRoot is now an explicit parameter rather than a closed-over script variable, since a lib function must not depend on its caller's scope. The new Describe is deliberately ungated: it needs neither the spec cache nor PowerShell 7, so it runs on all four CI legs. Verified by mutation: replacing the Substring with a passthrough reds 4 tests on pwsh 7 and 4 on Windows PowerShell 5.1. Refs dmann000#63

…ann000#63) Adds seven assertions over Reports/PfbApiDriftReport.json|.md as committed, rather than over a report the test regenerates. Regenerating needs tools/specs/ -- a ~50MB gitignored build cache absent on every CI runner -- so the existing versions of these checks reported "skipped" while the job reported "success". The standing requirement is about what is committed to the repository, so reading the committed file is both cheaper and more on-point. These need no cache, no PowerShell 7 and no fixture tree, so they run on all four CI legs. The regenerating originals are deliberately kept: they answer "does the generator still wire this through", which the committed-file versions cannot. Placed in a NEW file rather than as a Describe inside Build-PfbApiDriftReport.Tests.ps1. That file's top-level BeforeAll invokes tools/Build-PfbApiDriftReport.ps1 (`#Requires -Version 7.0`) and calls ConvertFrom-Json -Depth, which Windows PowerShell 5.1 does not have. Every Describe there is PS7-gated, so on 5.1 the BeforeAll never runs -- adding one ungated block would have dragged it in and failed all 65 tests in the file with a container error. A guard that must run everywhere needs a home that can. Verified: 7 passed / 0 skipped on pwsh 7 AND on Windows PowerShell 5.1, with Build-PfbApiDriftReport.Tests.ps1 still reporting 65 skipped / Container ok on 5.1. Injecting one absolute path into the committed report reds exactly the one covering test on both editions. Refs dmann000#63

Adds a prepare-specs job that restores the shared actions/cache, tops it up via Update-PfbApiSpecs.ps1 (which skips versions already on disk, so a warm cache means only newly-published versions are fetched), asserts the result is non-empty, saves the cache, and publishes the directory as a one-day artifact. Both test jobs now depend on it and unpack the artifact before running. The cache key prefix is shared with update-api-capability-map.yml so a warm cache written by either workflow serves both. An artifact rather than a per-leg cache restore: it fetches from the published spec index at most once per run instead of up to four times, and it is immune to cache key/branch scoping differences across the matrix. The non-empty assertion is the load-bearing part. Without it a restore-miss combined with a fetch failure would hand the test legs an empty directory and still report success -- reproducing the exact defect being fixed. Assert-PfbSpecCache.ps1 is in scripts/, not tools/: .gitignore:46 ignores tools/ wholesale, so a new file added there would be silently untracked. The existing tools/*.ps1 are tracked only because they predate that rule. Verified: the assertion passes against a populated tools/specs (29 files) and throws against an empty directory. Workflow parses and both test jobs resolve needs: prepare-specs. Refs dmann000#63

…eiling (dmann000#63) Restoring tools/specs fixes today's coverage hole. This makes the next one a red build instead of a silent one. Two independent assertions, because either alone has a gap: Skip ceiling catches a Describe that starts reporting skips. Required-Describe catches a Describe that vanishes from the result tree allowlist entirely. These blocks skip GRACEFULLY: when their guard evaluates false at BeforeAll time, or the file is filtered out of a run, they contribute NEITHER a skip NOR a pass. That second case is the reason a ceiling alone is not enough, and it is not hypothetical -- verified here: a scoped run reports "Skipped + not-run: 0", which reads as perfectly healthy, while seven required Describes report 0 (MISSING). A green summary and a silently-absent assertion are otherwise indistinguishable, which is issue dmann000#63's own failure shape one level down. Ceilings are per-edition because the two legs differ by ~200 skips by design: every tooling Describe carries -Skip:(PSVersion -lt 7), so the 5.1 leg skips all of them. One shared ceiling would be a permanent false red on 5.1 or useless on 7. Also extracts the Pester invocation into scripts/Invoke-PfbCiPester.ps1. The same block was copy-pasted in three places across two workflows and all three needed this change. A script rather than a composite action: lintable, diffable, testable, no ${{ }} escaping, and runnable outside Actions. The workflow diff for this change is a net deletion. Run.Exit flips to $false because it terminates the process on failure, which would skip the gate; the explicit exit preserves the same red build, and the gate runs first so a run that both fails tests and lost coverage reports both. Note on the ceilings: 15 / 260 are provisional headroom values. The full suite cannot be run locally (it exceeds the 600s tool-call cap), so they must be tightened against the first CI run that has the spec cache restored. Verified: result-object shape (.Tests / .Path[0] / .Result / NotRunCount) confirmed against Pester 6.0.0 and 6.0.1 on both editions; the gate throws on a missing Describe, throws on an exceeded ceiling, and passes when satisfied; the baseline parses under both editions. Refs dmann000#63

…ility-map workflow (dmann000#63) No CI job has ever regenerated Data/PfbCapabilityMap.json or Data/PfbResponseShapeMap.json and compared the result against what is committed, so a generator change committed without regenerating passes silently. That comparison had to be done by hand for dmann000#96 and dmann000#97. Hashes the two maps before the build steps overwrite them, then names the outcome in the run summary. Deliberately reports rather than fails: a legitimate diff is this workflow's whole purpose, since a newly published REST version changes both maps, so a hard failure would red the scheduled run every time one lands. The value is that drift is now stated instead of inferred from the PR body. Also retires the third copy of the Pester invocation. It could not be replaced by a workflow_call to cross-platform-tests.yml -- a reusable workflow runs on a fresh runner with a fresh checkout, so it would test the committed Data/ and Reports/ rather than the ones this job just regenerated, which is the entire reason the step was inline. A script runs in this job's workspace. Step ordering verified unchanged: the response-shape map is still built after the spec fetch and before the drift report, per the load-bearing note already in this file. Refs dmann000#63

…nd spot (dmann000#63) The gate is what turns the next silent coverage hole into a red build, so a vacuous gate would be worse than none: it would look like protection while providing none -- issue dmann000#63's own failure shape, one level up. These prove it fails when it should. The headline case asserts a run with SkippedCount 0 and a required Describe entirely absent from the result tree still fails. That combination reads as perfectly healthy to a skip-ceiling-only gate, and it is the exact shape the issue's follow-up comment describes: a block that skips gracefully contributes neither a skip nor a pass. Also pins the real Tests/coverage-baseline.psd1: both editions define a ceiling and a non-empty allowlist, and every allowlist entry names a Describe that some test file actually defines -- a typo or a rename would otherwise red the build forever for the wrong reason. Takes a hand-built result object rather than running Pester, so these need no spec cache and cannot invoke the aggregate suite. Verified: 9 passed / 0 skipped on pwsh 7 and on Windows PowerShell 5.1. Refs dmann000#63

…t actions (dmann000#63) Replaces the provisional headroom guesses with figures measured on run 31359783827, the first run with the spec cache actually restored: ubuntu-latest, pwsh 1842 passed / 0 failed / 2 skipped -> ceiling 8 windows-latest, WinPS 5.1 1675 passed / 0 failed / 169 skipped -> ceiling 185 The pwsh figure is the acceptance criterion: 2 skipped, down from 154 on the measurement in the issue. The 5.1 gap is the PS7-gated tooling Describes, skipping exactly as designed. All nine required Describes reported OK on that run, including the six that had been silently skipping in CI: the real-artifact drift-report block (10 tests), the Task 8 canaries (14), the field-cmdlet map (1), the committed capability map (1), the committed value-enum map (4), and the Task 6 real-data invariants (3). Also bumps upload-artifact/download-artifact v4 -> v5, clearing the Node 20 deprecation warnings the new steps introduced. Consistent with this repo already tracking current majors (checkout@v7, cache@v6). Refs dmann000#63

…000#63) `tools/` was added to .gitignore as a drive-by in 06ccd6e (a commit about a Windows PowerShell 5.1 crash from PKCS#8 test fixtures) with the comment "Not yet decided whether this should be tracked -- excluded for now". The repo has since decided: 14 files under tools/ are tracked, including a maintained tools/README.md documenting the toolchain. The rule only affected NEW files, and silently -- git does not descend into an ignored directory, so a new tools/*.ps1 produced no untracked marker at all. Nothing told you it had been swallowed. That trap is what steered the CI scripts in this PR to scripts/, and it would catch the next person the same way. It was also shadowing the rule that does the real work. Because the last matching pattern wins, `git check-ignore -v tools/specs` reported `.gitignore:46:tools/` -- so the purpose-built, carefully-commented `tools/specs/` rule was dormant. Removing the blanket rule reactivates it: before: .gitignore:46:tools/ tools/specs after: .gitignore:39:tools/specs/ tools/specs Verified a no-op for what is tracked or ignored today: 29 untracked paths under tools/, all 29 under tools/specs/, 0 elsewhere; tracked count unchanged at 14; only .gitignore appears in git status. No test reads .gitignore, and the capability-map workflow's change check is path-scoped to `-- Data Reports`.

…ot against its prose The two annotation guards pinned a field name ('context_names') and a note's wording ('not yet implemented'). Both move for legitimate reasons, and pinning them points the guard the wrong way: - The notes are hand-written prose describing decisions still being made. A test asserting 'not yet implemented' fails on the very PR that implements the thing and rewords the note, so the guard obstructs the change it should be indifferent to. - Which fields are annotated moves too. A field leaves systemicGaps once the module starts sending it -- continuation_token is already gone for exactly that reason, via Get-PfbCentralInjectionSites. Naming one field makes an unrelated, correct improvement look like a regression. What issue dmann000#63 was actually about is the wiring: a note in the source file reaching the committed artifact unmodified. That is invariant under both kinds of movement, so it is what these now assert -- every matchType=field and matchType=endpoint annotation is looked up in docs/drift-annotations.json and required to appear verbatim on the matching committed row. Each carries a non-vacuity floor, since an annotation set that matched nothing would otherwise pass by checking nothing. Endpoint matching uses .Contains on lowered strings rather than -like: a wildcard pattern treats ` as an escape character and [ ] as a character class. Verified red before green -- rewording the source note without regenerating the report, deleting the field annotations, and stripping annotations off the committed rows each fail the intended assertion. Passes on both pwsh 7 and Windows PowerShell 5.1 (7 tests, 0 skipped). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…landed on main The 5.1 leg went red on the skip ceiling with 0 failed tests: 1818 passed, 190 skipped, ceiling 185. Nothing in this branch caused it. PR dmann000#98 (Fusion context Phase 0) merged to main between this branch's previous CI run and its latest one. A pull_request run tests the MERGE commit, so the ceiling -- measured against a main that predated dmann000#98 -- went stale without any change here. dmann000#98 added 21 PS7-gated It blocks (8 in Build-PfbCapabilityMap.ContextScopeDrift.Tests.ps1, 9 in Build-PfbCapabilityMap.Tests.ps1, 4 in PfbSpecTools.ContextScope.Tests.ps1), and the observed 5.1 skip count moved 169 -> 190. Exactly +21. They run on 7 -- the pwsh7 leg is unchanged at 2 skipped over 2006 passed, against its ceiling of 8 -- so this is the PS7 gate doing its job, not lost coverage. All three required Describes ran on 5.1 with 0 skips. Raised to 206, keeping the same +16 headroom over measured that 185 had over 169, with the measurement, the run id and the attribution recorded inline as this file asks for. Worth noting the gate behaved correctly here: it is designed to red on a stale ceiling, and a merge to main is one of the ways a ceiling goes stale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

PR #107 added Tests/coverage-baseline.psd1 after this lane branched. The three TotalOnly guard Describes are ungated and read only committed capability data, so they must contribute executed tests on both editions; list them in both RequiredDescribes allowlists so their disappearance reds the build instead of passing silently. No ceiling change: the guards add no skipped tests. Co-Authored-By: Claude <noreply@anthropic.com>

Rebasing this lane onto a main carrying PR #98 (regenerated capability map) and PR #107 changes the drift denominator, so the measured figures quoted as the justification for the 0.55 -> 0.60 top-10 aggregation ceiling were stale. Record the re-measured values -- main alone 54.81% (570/1040), this lane on top of it 55.79% (564/1011) -- alongside the original pre-#98 pair. The conclusion is unchanged: 0.55 is genuinely exceeded and 0.60 still holds. Co-Authored-By: Claude <noreply@anthropic.com>

juemerson-at-purestorage and others added 10 commits August 9, 2026 19:37

juemerson-at-purestorage merged commit 1116ba8 into dmann000:main Aug 13, 2026
5 checks passed

juemerson-at-purestorage mentioned this pull request Aug 13, 2026

Drift report AST scanners miscount coverage two ways (private helpers credited; variable-keyed injection not) #113

Open

Navigation Menu

ci: run the ~23% of the suite CI was silently skipping, and gate against it recurring (#63) - #107

ci: run the ~23% of the suite CI was silently skipping, and gate against it recurring (#63)#107
juemerson-at-purestorage merged 10 commits into
dmann000:mainfrom
juemerson-at-purestorage:fix/issue-63-ci-silent-skip

juemerson-at-purestorage commented Aug 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

juemerson-at-purestorage commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The problem

The fix, on four axes

Where the new scripts live, and one .gitignore fix

Results

Notes for review

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

juemerson-at-purestorage commented Aug 10, 2026 •

edited

Loading

Where the new scripts live, and one `.gitignore` fix