Add a regression gate for dead query keys - #120
Merged
juemerson-at-purestorage merged 10 commits intoAug 17, 2026
Merged
Conversation
Returns the query parameter names an operation declares, resolving $ref'd parameters. Distinguishes $null (endpoint/verb absent from the spec version) from an empty array (present but declaring no query keys), and returns with a unary comma so zero- and one-element results keep their array type. Adds Tests/PfbSpecTools.DeclaredQueryKey.Tests.ps1 with in-memory [PSCustomObject] spec fixtures only, so it runs with no spec cache. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Compare the Public cmdlet inventory with query keys declared by the pinned API spec, commit the dead-key findings, and regenerate them in the capability-map workflow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Allow empty findings, conservatively exclude skipped selectors, and make report sorting reusable across emitted record shapes. Clarify the generator documentation and reject unsupported HTTP methods. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment claimed a group is suppressed when 'no selector was skipped for
any reason'. Two narrower limits, both measured during re-review:
- The rule is operation-scoped, not cmdlet-scoped. An 'endpoint/method
ambiguous' selector has a null Method/Endpoint and so forms its own
null-keyed group rather than suppressing the cmdlet's real one.
- An unresolvable wire name is dropped before the shape test, so the
guarantee covers identifiable selectors only.
Also records the deliberate consequence that a body-surface selector
suppresses its operation's group, which keeps every Update-Pfb* cmdlet
carrying -NewName out of the report. Comment only; the artifact reproduces
byte-identically.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A cmdlet parameter written into a query key the endpoint does not declare is silently discarded by the array: a GET returns the unfiltered collection and a DELETE arrives with no selector at all. Task 3 added the generator and its artifact; this is the gate that turns a newly introduced dead key into a red build instead of a live-array surprise. Two files, for two different failure modes: - Tests/CommittedDeadKeyReport.Tests.ps1 reads only the committed Reports/PfbDeadKeyReport.json and is UNGATED, so it runs on every CI leg including Windows PowerShell 5.1, where the generator's `#Requires -Version 7.0` makes regeneration impossible. Six assertions: anti-vacuity, no new DESTRUCTIVE dead key, no new noSurvivingSelector group, no count growth (dead keys and every skip reason), no absolute path in any string, and ordering under a reimplementation of the generator's ordinal comparer. Every set assertion is MONOTONE -- "nothing new appeared", never "the set is exactly this". An exact pin reds on the very PR that fixes a dead key and forces an artifact regeneration into every later fix. A shrink, all the way to an empty set, must pass. - Tests/Build-PfbDeadKeyReport.Tests.ps1 regenerates and compares, which is the only way to catch a STALE committed artifact. PS7-gated at the Describe, with the generator invoked only from that gated Describe's own BeforeAll: an ungated BeforeAll dragging in a PS7-only script kills the whole file with a container failure. A missing spec cache is a hard failure rather than a skip, because CI always has the cache and the local worktree copy hook fails open. coverage-baseline.psd1 registers the committed guard under both editions and the regeneration gate under pwsh7 only. MaxSkipped is unchanged: the six PS7-gated It blocks fit inside the existing winps51 headroom. Verified on both editions -- pwsh 7: 12 passed / 0 failed / 0 skipped; WinPS 5.1: 6 passed / 0 failed / 6 skipped, container ok. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rage Fix round 1 on the dead-key regression gate. Two blocking defects, both found by mutating the committed artifact. 1. A class shrinking to zero red the gate, in the exact case the design exists to allow. The per-scan non-vacuity floors counted the FILTERED SUBSET rather than the scan input, so fixing all 22 destructive dead keys, or all 18 no-surviving-selector groups, would have red the very PR that did it. The filter is a predicate, not the scan boundary, so the floors now sit on the scan input -- deadKeys, which assertion 1 guarantees non-empty -- and both ordering blocks are guarded rather than floored, since ordering over an empty list is trivially satisfied. Proven by construction: with all destructive entries removed, and again with all 18 groups removed, all seven tests pass. With deadKeys and noSurvivingSelector both emptied, the file reds in exactly three places, all now tagged SHRINK-TO-ZERO RELAX POINT and described in assertion 1's comment; applying that one documented relaxation turns the same input green. 2. A coverage collapse passed green. Ceilings on the dead-key and skip counts cannot see an inventory that simply stopped looking: cutting parametersInventoried 2174 -> 900 and keysEvaluated 1757 -> 700 while dropping a third of the dead keys and a third of the groups cleared every ceiling and every allowlist. Adds floors -- not pins, with the rationale scripts/Assert-PfbSpecCache.ps1 gives for its own -- of 2000 and 1600 against those measured values, plus an internal consistency check on `ok`. deadKey and the skip counts are deliberately left unfloored: those must stay free to reach zero. Proven by construction: the 900/700 collapse now reds, and reds only that test. Also in this round: - Corrects the comment documenting the file's central design decision, which claimed nothing but assertion 1 needed touching in the fix-everything case. It named the wrong number of edit points, so a reader following it during a dead-key-fixing PR would have relaxed one floor, stayed red, and concluded the gate was broken. - `@($null).Count` is 1, so `"deadKeys": null` sailed past the anti-vacuity floor and was caught only downstream. Adds an explicit null check so assertion 1 fails first and loudly, as intended. - Replaces an assertion that could not fail (`Test-Path | Should -BeTrue`, unreachable because the BeforeAll's Get-Content errors into a container failure first) with an explicit throw in BeforeAll carrying an actionable message. - Splits File B into two gated Describes on its dependency boundary: regeneration needs the real spec cache, synthetic classification builds its own fixture and reads no cache. One shared BeforeAll let a broken fixture red the regeneration tests, reporting the real generator as broken. Total It count is unchanged. - Drops `| Out-Null` from the Assert-PfbSpecCache call, which suppressed nothing -- that script reports via Write-Host -- and comments why the output is wanted. coverage-baseline.psd1 tracks the two new Describe names under pwsh7. MaxSkipped unchanged. Verified on both editions -- pwsh 7: 13 passed / 0 failed / 0 skipped; WinPS 5.1: 7 passed / 0 failed / 6 skipped, container ok. The split preserved File B's 5.1 contribution at exactly 6 skips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fix round 2, one narrow change. The two scan-faithfulness assertions in
File A asserted non-vacuity as a property of the COLLECTION -- is it
non-empty -- when what a scan needs to prove is a property of ITSELF:
did it visit every element it was given. Each scan now counts what it
visited and asserts that count equals its input size.
Strictly stronger than the greater-than-zero floor it replaces. `> 0`
proved only that the loop ran at least once, so a `continue` bug that
silently skipped entries passed it. Proven, not asserted: perturbing the
destructive scan to skip every third entry against an UNMODIFIED
artifact reds the equality assertion ("it visited 84 of 126"), and the
same defect judged by the reinstated round-1 floor passes 7 of 7.
It is also trivially satisfied at zero, which dissolves the conflict
round 1 could not. The shrink-to-zero relax points drop from three to
one -- assertion 1, alone, exactly as the plan text said all along --
because a scan that visits 0 of 0 is faithful, while a floor on a
collection that legitimately emptied is not.
Re-proved all four round-1 cases against this change: all 22 destructive
dead keys fixed, 7/7 green; all 18 no-surviving-selector groups fixed,
7/7 green; every dead key fixed, now red in ONE place instead of three,
and green at 7/7 once the single documented edit is applied
mechanically; the 900/700 coverage collapse still reds, and still only
on the inventory-floor test.
Retags assertion 1 as the sole relax point and rewrites its comment to
name the one edit, spell out which two lines it drops and which checks
it keeps, and record that the two scans and both ordering blocks need no
edit because they are already true at zero.
Verified on both editions -- pwsh 7: 13 passed / 0 failed / 0 skipped;
WinPS 5.1: 7 passed / 0 failed / 6 skipped, container ok.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The absolute-path walk's `> 0` floor was one string away from vacuous: replacing its IEnumerable descent branch with a bare `continue` dropped the visited count from 1600 to 1 -- top-level specVersion alone -- and all seven tests still passed. specVersion always being present is exactly what held the floor up while the walk scanned nothing. Floor it on the artifact's real content instead (5 string fields per deadKeys record), which stays shrink-safe at zero dead keys. Strengthen the skip-reason scan to an equality against the property count, so a scan visiting 1 of 4 properties can no longer hide a 14 -> 999 growth in one it never looked at, and rewrite the header to state the actual principle: an equality where the input size is knowable, a content-derived floor where the input is a recursive walk. Replace `ok >= 0` -- non-negative by construction, so unfailable on any generated artifact -- with the reconciliation it was reaching for: ok + deadKey == keysEvaluated, and keysEvaluated + all skip reasons == parametersInventoried. Give each half of the generator gate its own work-root variable instead of a shared $script:workRoot that crossed the seam the split exists to isolate, and make each AfterAll tolerate its own variable being unset when BeforeAll threw first. Fix the coverage-baseline comment that still quoted the pre-split Describe name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two blockers and eight accepted non-blockers from the final review round. Reports/PfbDeadKeyReport.json is unchanged; the generator gets comments only. BLOCKING 1 -- normalise line endings in the committed-artifact comparison (Tests/Build-PfbDeadKeyReport.Tests.ps1). The committed blob is LF and a Windows regeneration is CRLF (Set-Content emits [Environment]::NewLine), and there is no .gitattributes, so the raw byte compare was green only where core.autocrlf=true made the checkout CRLF too. A contributor with autocrlf=false or input got a false "the committed artifact is stale". Both sides are now normalised, as Get-PfbSelectorArtifactHash already prescribes in Tests/PfbPipelineSelectorRail.Tests.ps1. The first-difference locator moves into the normalised text. The working-directory comparison stays a raw byte compare -- both its sides are generator output with no checkout in between. BLOCKING 2 -- anchor the destructive scan's predicate to the severity vocabulary (Tests/CommittedDeadKeyReport.Tests.ps1). `-ne 'DESTRUCTIVE'` matched nothing at all if the vocabulary moved, and both a dropped severity property and a renamed value were measured at a clean green with the file's highest-value assertion covering zero entries -- on the 5.1 leg this ungated file exists to serve. Each entry's severity is now asserted to be one of the three known values inside the existing scan. It is a per-record property assertion, not a floor on a filtered subset, so it costs nothing at zero dead keys. 3. Remove 'body property' from the enforced skip ceilings. Bumping it 278->279 -- any PR adding one body-surface parameter -- red the gate for a safety regression that did not happen; body properties were never evaluable as query keys. Kept as a known reason so it is not reported as a new skip class, and still constrained by the reconciliation invariant. 4. Correct the first reconciliation invariant's comment: it is true by construction for any generated artifact and exists to catch a hand-edit; the second invariant is the one with content against a generator bug. 5. Fix the header's self-contradiction on shrink-to-zero -- assertion 1's relax-point note is the single authority. 6. Summarise the three noSurvivingSelector narrowings in Reports/README.md, with a pointer from tools/README.md and from the generator comment. No new artifact field. 7. Add Build-PfbDeadKeyReport.ps1 and its catching test to CLAUDE.md's regeneration recipe. 8. List Reports/PfbDeadKeyReport.json in the capability-map workflow's PR body. 9. Assert specVersion (present and version-shaped, deliberately not pinned) and deadKeys[].declared on the ungated leg. 10. Comment the unstable sort in Sort-PfbDeadKeyRecords and name the fix. Verified with the scoped set (CommittedDeadKeyReport, Build-PfbDeadKeyReport, PfbSpecTools.DeclaredQueryKey) on both editions: pwsh7 P23 F0 S0, WinPS 5.1 P17 F0 S6, container ok. Every ruling proven by mutation: a one-character wireKey change still reds the byte-identity test (P22 F1); an LF checkout now greens (51108 vs 53263 chars); dropping severity and renaming DESTRUCTIVE each red the destructive scan on 5.1 (P16 F1, was P17 F0); deadKeys:[] reds only assertion 1; body property 278->279 greens File A while wire name unresolved 125->126 and an unreconciled body-property bump each red; the new specVersion and declared assertions were each shown to fire, with a 2.28->2.29 bump confirmed green as the control. Co-Authored-By: Claude <noreply@anthropic.com>
Keep the comments and CI-visible test name aligned with the gate's actual newline and body-surface coverage semantics. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Summary
Public/cmdlets but undeclared for the endpoint and HTTP verbDesign notes
$null) and an endpoint declaring zero query parameters (empty array)noSurvivingSelectoras a deliberately conservative, operation-scoped signal rather than a complete cmdlet refusal listVerification
9EAEF0C3D4EAEA21F5AE3E9B696BE551986882AC674BED83E53B9DA67911FDF9Live testing
Not applicable: this branch changes reporting, tests, and scheduled regeneration only. It changes no cmdlet and issues no FlashBlade API request, so there is no runtime array behavior to exercise.
🤖 Generated with Claude Code