Repository navigation
fix: teach the empty-pipeline guard to tell a selector from a scope key (#126, #128) - #144
Merged
juemerson-at-purestorage merged 34 commits intoAug 23, 2026
Conversation
Define the policy Test-PfbEmptyPipelineRead has been approximating since PR dmann000#125: an empty pipeline must never become a request that returns or mutates objects the caller did not address. Classify the non-selector keys rather than the selectors, so that a key nobody classified degrades to a missed guard rather than a discarded request. Fifteen entries, against a selector allowlist that would need 23 today and grow with every endpoint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
) Two independent reviews. Corrections that were not judgment calls: - Three listed keys (continuation_token, context_names, allow_errors) are invisible to the predicate: context_names is injected into a clone after the guard runs, continuation_token during pagination, and allow_errors is never sent. Live list is 12, not 15. - Scope the completeness rail to the guarded population. Scoped to all of Public/ it reds on the unmodified tree, and greening it would push real selectors (gids, uids, user_sids) onto the non-selector list -- CI pressure in the unsafe direction. Scoped correctly it is clean today, 0 unmatched of 32. - The rail cannot be built on PfbFieldCmdletMap.json, which has no entries for type, protocols or flagged. - Per-cmdlet counts were Public/-wide under a guarded-population header. Guarded counts are 8/8/4/1/1. - The rejected alternative's mechanism must read ValueFromPipelineByPropertyName: 8 of 130 guarded cmdlets have no true ValueFromPipeline parameter. Also record the $null case and the container-scope advantage the alternative has. - Stop claiming the denylist default is safe. Matching prior behaviour is a compatibility property, not a safety one. - Note the pre-existing Test-PfbDeadKeySelectorName, which excludes context_names by explicit comment. - Record the container-scope gap no per-key list can express. - Cut the sibling-product ShouldProcess detail from Prior art. Three questions remain open and are marked inline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ann000#126) 1. Unclassified keys. The runtime default (unclassified reads as a selector, so the request is issued) is a compatibility property, not a safety one, and is no longer defended as safe. Instead a CI completeness gate makes it unreachable: an unclassified key in the guarded population reds the build, so no release can contain one. Records why the generated per-endpoint artifact was declined and what would justify revisiting it. 2. Definition of selector. A selector is a key whose bound the caller can enumerate or author -- identity keys, plus filter as the one caller-authored predicate. Everything else selects among server-defined partitions. Test for a new key: if the server decides how many objects come back, it is scope. This keeps type and protocols as non-selectors, and gives filter a principled reason to differ from a category key rather than an asserted one. 3. Diagnostics. Write-Verbose on every suppression, plus Write-Warning where only non-selector keys were bound. The warning is diagnostics, not the safety mechanism -- the suppression is -- so a preference-controlled stream is adequate and a non-terminating error is not warranted. States plainly that the four genuine lost warnings return at verbose level only. Also records four follow-ups, including the container-scope gap this approach cannot close. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… fixture (dmann000#128) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rd (dmann000#126) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…00#128) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mann000#126) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… of the guard (dmann000#126) Comment-only in Private/; three new pinning tests. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ion Stop rationale (dmann000#126) The old reason named Get-PfbLog and Remove-PfbFileSystemSession as the only unconditional query writes in Public/, which is false -- about fifteen files write unconditionally, several of them selectors. The conclusion holds for a better reason: within the 130 GUARDED files, all 73 selector writes sit behind a non-empty gate, and the guarded set is the only population the predicate can see. Also records that no $WarningPreference-exempt warning exists, so the -WarningAction Stop edge cannot be engineered away. Comment-only. No executable line and no test assertion changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The old justification said the shape test and the runtime list disagree about context_names. They do not -- context_names is absent from the list, so the list reads it as a selector, the same answer the shape test gives, and it is moot anyway because context_names is injected into a clone inside Invoke-PfbApiRequest after this function returns. The real reason is stronger: the two classifiers have OPPOSITE DEFAULTS. Consulting the shape test here would reclassify filter -- the module's one caller-authored predicate, which matches no identity shape -- as scope, and suppress a legitimately filtered call. Comment-only, one file, one block. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…#126) The remediation-sentence comment named four cmdlets carrying a mandatory selector in every parameter set and gave "4 of 130"; the true membership is larger. Rather than replace one hand-maintained list with another -- the exact mechanism that produced this defect twice already -- the comment now states the property without enumerating it and points at the derived assertion in Tests/PfbEmptyPipelineGuardCoverage.Tests.ps1. Both the constants file and the design spec claimed that classifying `flagged` "changes no behaviour until dmann000#142 lands". Dead-key-ness is a SERVER-side fact; this predicate decides on key presence and never reaches the server. Get-PfbAlert writes the key under ContainsKey and Assert-PfbApiCapability treats an undeclared parameter as a silent pass, so `@() | Get-PfbAlert -Flagged $true` returns every alert on main and is suppressed on this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…keys (dmann000#126) The remediation sentence's "call <cmdlet> directly" advice is only safe for a state-changing cmdlet if that cmdlet carries a mandatory selector on every path a caller can take. Remove-PfbLocalGroup is the only non-read verb in the guarded population, and it does -- but that was asserted by a hand-maintained list in a comment, which went stale. A new It derives it: Get and Test are the read verbs, everything else is state-changing so a new verb fails closed, and the in-scope population is asserted non-empty so the offender check cannot pass vacuously. Raise the $script:qualifying floor from 100 to 120 against 130 measured. At 100 a partial detector regression could silently drop 29 cmdlets. It stays a floor, not an equality: an equality passes vacuously under total collapse. Add behavioural cases for `flagged` and `expose_api_token`. The flagged one is the regression test for the false "changes no behaviour until dmann000#142" claim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…00#126) A guard declared inside a nested `function` in an end block was invisible to every rail. Get-PfbGuardRecord's FindAll over the end block is recursive, so the guard counted toward the OUTER cmdlet's GuardCallsInEnd and the cmdlet read as guarded -- while the guard's `return` exited only the nested function and the request still went out. The nesting check tested only for ScriptBlockExpressionAst, and neither a FunctionDefinitionAst nor the ScriptBlockAst forming its body is one, so it answered $false. Add FunctionDefinitionAst to the check and rename it to Test-PfbNestedInInnerScope, with the record properties renamed to Invoke/GuardNestedInInnerScope to match -- the old names become actively false once the predicate matches more than a scriptblock expression. Both call sites stop at a node inside the cmdlet's own definition and the loop tests its condition before its body, so a cmdlet's own FunctionDefinitionAst is never reached and no existing call site changes behaviour. Extend the detector-mutation fixture with a third case. That It exists because this is the file's only one-way detector, so the new branch needs the same protection as the existing one. Zero occurrences in Public/ today; this is a tripwire. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n000#126) Resolve-PfbQueryKeyDisplayName.Tests.ps1: the shipped-cmdlet block reimplements the snake_case -> PascalCase transform and never calls the function. Calling it is not possible there -- its -Caller is a live [PSCmdlet], which the runtime only materialises inside an executing advanced function, so obtaining one for Get-PfbFileSystem would mean invoking Get-PfbFileSystem. Say so, say that the block asserts the TREE's conformance to the transform, and name what covers the function itself. PfbSelectorPolicyCompleteness.Tests.ps1: note that a query write through an ALIAS of the hashtable defeats the gate without producing an unscannable-query-argument record, so the escape hatch is not a complete backstop. Zero occurrences today; a note only, no logic change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
juemerson-at-purestorage
deleted the
docs/issue-126-selector-policy-design
branch
August 25, 2026 20:24
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.
Closes #126. Closes #128.
The defect
Test-PfbEmptyPipelineReadcould not tell a selector (a key naming which objects the callerwants) from a scope or shaping key (one that narrows, sorts or projects whatever is already in
scope). It suppressed a request only when the query was completely empty, so:
issued an unfiltered read and returned file systems the caller never addressed. An empty pipeline
means "no objects were selected";
-Limitcannot select anything, so there was nothing to read.What changed
Test-PfbEmptyPipelineReadnow classifies every key in the outgoing query. If an empty pipelinecarries only non-selector keys the request is suppressed, and the caller gets a warning naming
the parameters that could not select. If any selector is present the request goes out exactly as
before.
Direct calls are never affected —
ExpectingInputgates the whole predicate and is the firstexecutable line.
Get-PfbX -Limit 10typed at a prompt is a deliberate unfiltered read and staysone. The change can only ever remove a request from the wire, never add or alter one.
One behaviour change worth calling out
@() | Get-PfbAlert -Flagged $truereturns every alert onmainand is suppressed here.flaggedis a dead key server-side (#142 — undeclared in all 29 published REST versions), so it wasinitially reasoned about as an inert classification. That was wrong, and it is the one place where
this policy visibly changes a shipped cmdlet's output: dead-key-ness governs what the server does,
while this predicate decides on key presence and never reaches the server.
Get-PfbAlertwritesthe key under
ContainsKey, and an undeclared parameter is a silent pass in the capability check, sonothing stopped the send. Of the twelve classified keys this is the only one whose classification
demonstrably alters current output; it now carries a behavioural test.
Guard dominance (#128)
The existing rails asserted a guard existed in a cmdlet's
endblock, not that it executed onevery path to the request. A guard inside an
if, aforeach, aswitchcase or atrybodysatisfied the old check while leaving paths that reach the request unguarded. The rail now flags
those positions, with one deliberate subtlety: for an
if,Clauses[i].Item1is exempt only ati == 0— ati > 0it is anelseifcondition, which is itself conditionally executed. Theexemption is positional rather than sticky, so a guard at
Clauses[0].Item1nested inside a loopstill flags.
Also closed: a guard inside a nested function in an
endblock was invisible to every rail. Theenclosing search is recursive, so such a guard counted toward the outer cmdlet, while the
nesting check tested only for a scriptblock expression — and a function definition body is a plain
script block, not one. Its
returnwould exit only the nested function, leaving the request to goout with every rail green. Zero occurrences today; this is a tripwire.
Design decisions
A denylist, not an allowlist.
$script:PfbNonSelectorQueryKeyslists the twelve non-selectorkeys; anything unlisted reads as a selector. The failure direction is what decides this: a key
nobody classified yet gets treated as a selector and the request is issued — today's behaviour.
An allowlist would fail toward suppressing a legitimate read, which is new breakage.
filterandcontext_namesare deliberately absent.The list decides, not a spelling test. The predicate never consults the
names/ids/*_namesshape test used by the CI gate. The two have opposite defaults, and
filteris where that bites:it matches no identity shape, so a shape test would call it a scope key and suppress a legitimately
filtered call.
filteris reachable in 124 of the 130 guarded cmdlets.Three classifiers, deliberately divergent. This policy, the CI shape test, and the dead-key
report's
Test-PfbDeadKeySelectorNamedisagree by design, because their failure modes differ — amisclassifying report produces a wrong row, a misclassifying runtime policy discards a request. Each
now carries a comment naming the others and stating why they differ.
Two contested placements left alone.
Get-PfbRemoteArray's guard stays above itscurrent_fleet_onlywrite: now that the key is classified, placement no longer changes thedecision, and moving it would drag a test allowlist change along for no benefit. The one consequence
is documented rather than "fixed" — because the guard runs before the write, an empty-pipe call
reaches it with an empty query and takes the quieter verbose-only path.
New CI rails
knows about, failing closed on any query argument it cannot statically resolve.
is one-way, so if it regressed to always-false two other assertions would pass vacuously.
every guarded cmdlet whose verb is not
Get/Testmust carry a mandatory parameter in everyparameter set. Otherwise the diagnostic's "call it directly instead of piping to it" advice could
mean an unscoped destructive call. Unknown verbs default to state-changing, and the population is
asserted non-empty so the check cannot pass vacuously.
Verification
Tests — scoped, both editions,
Container okthroughout:The derived-artifact gate reports all 11 committed artifacts up to date.
Live FlashBlade — this PR touches
Private/andPublic/, so it is not exempt from liveverification. Run against an internal lab array, Purity//FB 4.8.2, REST 2.26, as the local array
account (nothing here involves
context_names). Every call below is a read; nothing was created,modified or deleted.
main@() | Get-PfbFileSystem -Limit 10@() | Get-PfbFileSystem@() | Get-PfbFileSystem -Filter "name='*'"'fs-share' | Get-PfbFileSystem -Limit 10A: 2 → 0with B, C and D unchanged is the finding. The array holds only two file systems, so theunfiltered read caps at 2 rather than 10 — the assertion is "same as an unfiltered read" versus
"zero", not the literal number. The zero alone would prove nothing, since a broken cmdlet also
returns zero; C and D returning their normal counts in the same session is what rules that out.
Counting used a non-null
namefilter, because@($null).Countis1and this module's empty listresult is a coercing wrapper whose
| Measure-Objectalso reports1.Probe A emitted exactly one warning, naming the offending parameter:
Two harness ledger rows recorded the control in the other direction — a direct call with a
non-selector must still issue:
Get-PfbFileSystem -Limit 2(Pass, 264 ms) andGet-PfbFileSystem -Name fs-share(Pass, 213 ms).The probes were captured at
a472a3f; the four commits since touch onlyTests/, the design spec,and comments. Both changed
Private/files are byte-identical after stripping comments (84 and 16executable lines, unchanged), so nothing since the capture can alter what goes on the wire.
Not included
No version bump and no
CHANGELOG.mdentry — those are the maintainer's call.