Give New-PfbApiClient a typed body and require legal-hold released

[juemerson-at-purestorage]

Fixes the two confirmed findings of the issue #106 Part 2 audit: cmdlets that build a semantic empty @{} body and, in doing so, send a request the published contract forbids.

Closes #106 — Part 2 only. Part 1 of that issue (New-PfbObjectStoreAccessPolicyRule omitting the required names query parameter) was fixed separately and landed on main in #134; this PR completes the Part 2 audit and fixes what it found, which is the last of #106's two deliverables. The audit record itself is a comment on the issue rather than duplicated here.

New-PfbApiClient — impossible through the public interface

#/components/schemas/ApiClientsPost requires body property public_key on REST 2.0-2.28, and max_role on 2.0-2.18 (optional and deprecated from 2.19). The cmdlet exposed only -Name, -Attributes and -Array, so

New-PfbApiClient -Name 'automation-client'

bound cleanly and sent {}. There was no typed route to either required field; the only way to succeed was to hand-build -Attributes, which #106 treats as an escape hatch rather than the intended interface.

Adds a Typed default parameter set with mandatory -PublicKey and optional -MaxRole, and moves -Attributes into its own mandatory set so a mixed invocation is rejected at bind time instead of letting the raw hashtable silently override a typed value. This is the shape New-PfbObjectStoreAccountExport already uses for the same problem (the #101 fix), down to the comment noting that a reference's resource_type is readOnly and must never be sent. The -Attributes branch now uses .Clone() so the caller's hashtable cannot be mutated.

Deliberately not done: no runtime REST-version check making -MaxRole mandatory below 2.19. The defect being fixed is that no typed path existed, and version-conditional mandatory-ness is not a pattern this module has — inventing one here is a larger decision than this fix. The version boundary is documented in the help instead. The other optional body properties (access_policies, access_token_ttl_in_ms, issuer) stay with #57/#65.

Update-PfbLegalHoldEntity — typed but not enforced

A category #106 did not anticipate, and the reason nothing had flagged it. In every spec from fb2.17.json through fb2.28.json, PATCH /legal-holds/held-entities references #/components/parameters/Legal_holds_release, resolving to released, in: query, required: true. -Released existed but was optional, and the key was sent only when explicitly bound, so

Update-PfbLegalHoldEntity -Name 'fs1' -Recursive $true

omitted it. Two of the cmdlet's own examples documented that shape.

-Released becomes mandatory [bool] and is always sent. A mandatory nullable boolean is a contradiction, and plain [bool] still binds -Released $false, which matters because false is the apply direction. This is the one parameter in the file not guarded by ContainsKey; the comment says why — the convention exists to distinguish "not supplied" from "supplied as false", a distinction that cannot arise for a parameter with no legal omission.

The help needed more than a parameter line. The description claimed the entity is identified by name and changed via -Attributes, and neither is true: a held entity has no name at all (LegalHoldHeldEntity declares only file_system, legal_hold, path, status) and the endpoint accepts no request body. Two of three examples would no longer bind, so they were not merely stale but uncallable. All three are replaced with forms measured on a live array.

Because the drift report scores typed-parameter presence rather than requiredness, this endpoint read as fully covered — that blind spot is #137.

Derived reports

Four artifact pairs move, not two. PfbDeadKeyReport.json and PfbApiDriftReport.json|.md were regenerated up front; CI's "Verify derived artifacts are not stale" job then found PfbFieldCmdletMap.json|Mapping.md and PfbPipelineSelectorMap.json|.md stale as well, since both also walk cmdlet parameter metadata. Those two were regenerated through scripts/Assert-PfbDerivedArtifacts.ps1 -KeepWorkDirectory rather than by running the generators bare, per that script's own warning: Build-PfbFieldCmdletMap.ps1 records what it finds in the whole spec directory, so a bare run against a cache holding anything past the 29 pinned versions writes different output and the gate reports the same artifact stale again.

PfbFieldCmdletMap moves by no-spec-enum-found 1972 → 1974 — the two new parameters, neither of which has a spec enum to match.

PfbPipelineSelectorMap is the one worth reading rather than rubber-stamping, and it cost two test re-baselines. Making -Released mandatory means the probe harness can no longer construct a call for Update-PfbLegalHoldEntity, so its two rows move from Bound/Unbindable to BindError/HarnessRefusal ("would prompt for an unbound mandatory parameter"). BindError is that map's only unmeasured outcome, and two tests pin it at 2 precisely so a rise stops the build — it is the regression shape that produced the original 33-pair blind spot.

Both reds were the tripwires working. Re-baselined 2 → 4 with the movement accounted for in the comments rather than absorbed: the probe supplies a selector and nothing else, so a required parameter it does not know to supply cannot be bound, and the row it replaces measured names=PROBE-name on a key that cannot identify a held entity anyway (#139). The comments also record the part that generalises — any future fix correctly making a required parameter mandatory will convert that cmdlet's pipeline-selector coverage into a triage row, and if this number climbs again for that reason the answer is to teach the probe generator to satisfy mandatory parameters outside the selector under test, not to keep re-baselining.

Neither pin appeared in this branch's scoped runs, which is the module-wide-tripwire gap: a test asserting a whole-repo total belongs to no changed file's touched set. Note they are not symmetric either — the PfbPipelineSelectorRail pin regenerates and is PS7-gated, while the Build-PfbPipelineSelectorMap one reads the committed report and reds on 5.1 too. CI caught both.

The two originally-regenerated reports carry content-identity gates that the source change also reds. Note that the Committed*Report.Tests.ps1 pair is not the gate — those are shape and absolute-path guards that pass regardless; the real gates live in Build-PfbDeadKeyReport.Tests.ps1 and Build-PfbApiDriftReport.Tests.ps1.

Verified as caused-by-this-change rather than assumed: each failure was attributed by stashing the diff, confirming the gate passes on a clean tree, then unstashing.

	before	after
dead keys	83	83
parametersInventoried	2165	2167
skip reason body property	278	280
drift: missing addable body properties	426	424

The dead-key total is a ceiling and holds. The body property skip reason is the one CommittedDeadKeyReport.Tests.ps1 deliberately leaves unceilinged, its comment predicting exactly this case — that ceilinging it would red any PR adding a typed body parameter.

The drift report drops public_key and max_role from the POST /api-clients gap row, and both lose their systemicGaps aggregate rows by falling from two endpoints to one. PATCH /api-clients keeps its own max_role gap row, so the Task 8 canary still fires. All three reports are written LF-only to match their committed form; the generators emit CRLF on Windows, which autocrlf hides from git diff while leaving a mixed-endings file.

Verification

Scoped and dual-edition throughout — the aggregate suite is CI's job, never a task's completion check here.

Scope	pwsh 7	WinPS 5.1
Changed cmdlets + family siblings (8 files)	60 passed / 0 failed	60 passed / 0 failed
Report gates + module-wide tripwires (11 files)	537 / 0	457 / 0, 80 skipped

Container ok on every row, both editions.

Rebased onto main after #135 landed, which replaced the global MaxSkipped ceiling with the per-file ExpectedSkips map. No baseline entry is needed for the new test file: Assert-PfbTestCoverage.ps1 fails an undeclared file that skips anything at all, and both changed test files skip zero on both editions (17 passed / 0 skipped each), so there is nothing to declare. Confirmed by running them alone rather than inferring it from a mixed run — CiCoverageGate.Tests.ps1 exercises the gate against synthetic results, so its own green says nothing about this branch's skip profile.

Both cmdlets were driven test-first. Tests/New-PfbApiClient.Tests.ps1 is new — the cmdlet had no test file — and failed 4 of 7 against the unmodified source before passing 7 of 7. Tests/Update-PfbLegalHoldEntity.Tests.ps1 had every invocation updated to bind, and its "omits recursive and released entirely when not supplied" assertion split: the recursive half kept, the released half replaced by an always-present one. Both mandatory-ness tests assert on command metadata rather than by invoking, because an unbound mandatory parameter prompts rather than throwing and hangs a non-interactive run.

Live verification

This PR touches Public/, so the live-FlashBlade requirement applies in full. No exemption is claimed.

Update-PfbLegalHoldEntity was verified on a physical FlashBlade (Purity//FB 4.8.2, REST 2.26) against this branch's committed code — 7 calls, 7 Pass:

New-PfbFileSystem          Pass   486ms   create
New-PfbLegalHold           Pass   138ms   create
New-PfbLegalHoldEntity     Pass   274ms   create   (apply the hold)
Update-PfbLegalHoldEntity  Pass   178ms   update   <- the call under test
Remove-PfbLegalHold        Pass   145ms   delete
Update-PfbFileSystem       Pass   206ms   update   (destroy)
Remove-PfbFileSystem       Pass   175ms   delete   (eradicate)

The teardown is the proof rather than a formality: while a hold is applied the array refuses to delete it (Can't delete legal hold because it is still in use.) and refuses to destroy the file system (File system with legal holds applied does not support destroy.). Those two steps succeeding is what shows the release actually landed. The array was left clean — zero holds, zero held entities, zero test file systems.

The working release form was measured rather than assumed, and is what the new help examples show:

Update-PfbLegalHoldEntity -Name <hold> -FileSystemNames <fs> -Paths '/' -Recursive $true -Released $true

Two things could not be verified live, stated rather than glossed:

• New-PfbApiClient cannot be live-tested at all. api-clients is a denied family in the live-test harness — minting real credentials on the lab array is out of scope for it — so the call records Skipped-Policy with the family resolved from the cmdlet's own endpoint rather than from a caller-supplied string. That is the rail working, not a coverage gap that was routed around. Its evidence is the dual-edition unit coverage above.
• The pre-fix shape cannot be run live either. -Released is now an unbound mandatory parameter, which prompts rather than throws and would hang a non-interactive run, so there is no request left to send. Mandatory-ness is asserted from command metadata instead. Worth recording that a harness dry run returned WouldRun for that shape: it validates supplied arguments against the module's real parameters but does not check mandatory satisfaction, so a dry run could never have proven this either way.

Not in this PR

• No version bump and no CHANGELOG.md edit — those are maintainer decisions.
• -Name on Update-PfbLegalHoldEntity stays mandatory even though it is an unusable selector. Held entities have no name, so names can never match one; a live array rejects a request selected only by it with Either names or ids query parameter is required despite the key being sent. Demoting a mandatory parameter is a breaking change for positional callers, so it is filed as Update-PfbLegalHoldEntity -Name is a mandatory but unusable selector: a held entity has no name, and the array rejects a request selected only by it #139 rather than folded in here.

Follow-ups filed from the audit

• Drift reporting scores typed-parameter presence, not requiredness, so a required parameter the cmdlet lets you omit reads as covered #137 — drift reporting scores typed-parameter presence, not requiredness, which is why the released defect was invisible to it.
• New-PfbNlmReclamation -Name is a mandatory dead selector: the endpoint declares no names parameter and the operation is array-wide #138 — New-PfbNlmReclamation -Name is a mandatory dead selector: the endpoint declares no names parameter and the operation is array-wide.
• Update-PfbLegalHoldEntity -Name is a mandatory but unusable selector: a held entity has no name, and the array rejects a request selected only by it #139 — Update-PfbLegalHoldEntity -Name is a mandatory but unusable selector.

One correction to #106's own arithmetic

The list of 23 was built by matching one exact literal, $body = if ($Attributes) { $Attributes } else { @{} }. Three cmdlets use a .Clone() variant the pattern does not match (New-PfbLocalDirectoryService, New-PfbLocalGroup, New-PfbFileSystemExport), so the convention is 25 cmdlets, not 23. All three were audited. New-PfbLocalGroup is confirmed broken on the wire for this same failure mode and is #136; the other two came back clean.

The convention has now produced four broken creates (#101, #136, and the two fixed here). For the next sweep of it, match on else { @{} } or on the AST rather than on the whole assignment — .Clone() is a deliberate improvement that avoids mutating the caller's hashtable, so more such variants are likely, and a literal-string sweep under-reports while reading exactly like a complete one.