chore: PSScriptAnalyzer baseline, CI gate, and the cleanup that makes it enforceable - #155
Merged
juemerson-at-purestorage merged 9 commits intoAug 28, 2026
Conversation
A BOM-less file containing non-ASCII is decoded as UTF-8 by PowerShell 7
and as Windows-1252 by Windows PowerShell 5.1, so comment-based help
rendered mojibake on 5.1 only:
5.1 systems ae" detach via Remove-PfbDataEvictionPolicyFileSystem first.
7 systems -- detach via Remove-PfbDataEvictionPolicyFileSystem first.
Replaces 81 em dashes with "--" and 3 ellipses with "..." across 24
tracked files, leaving the tree pure ASCII. PSUseBOMForUnicodeEncodedFile
goes from 24 findings to 0.
Tests/PfbSpecTools.Tests.ps1 is the one behavioural change. Its e-acute is
the assertion rather than prose, so deleting the character would remove
the coverage the test exists to provide. It becomes an escape instead:
$spec.info.description | Should -Match "caf$([char]0x00E9)"
Note the quote style flips from single to double -- a single-quoted string
does not interpolate, so left as '...' the test would assert on the
literal escape text and fail.
No line endings or BOM-presence were changed; the repo is mixed-EOL by
file and each was rewritten with the encoding it was read with.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two rules account for every remaining Error-severity finding in the repo,
and both are correct-by-design in the sites that raise them. Annotate the
sites rather than change the code, so the analyzer baseline can reach zero
Errors without loosening a rule repo-wide.
T9 -- PSAvoidUsingConvertToSecureStringWithPlainText (21 findings, 5 files)
Tests/Connect-PfbArray.Context.Tests.ps1 8 -> 0
Tests/Connect-PfbArray.NativeLoginVersionGate.Tests.ps1 3 -> 0
Tests/Connect-PfbArray.OAuth2TokenRefresh.Tests.ps1 2 -> 0
Tests/Get-PfbApiTokenViaSsh.Tests.ps1 6 -> 0
Tests/New-PfbJwtToken.Tests.ps1 2 -> 0
Every site is a test building a literal credential for a mocked call.
There is no secret to protect: the plaintext is a fixture, and the rule
is warning about a production practice that does not apply. File-wide
suppression, because the sites are the whole point of these files.
T5 -- PSUseApprovedVerbs (3 findings, 2 files)
tools/Build-PfbDeadKeyReport.ps1 Sort-PfbDeadKeyRecords
tools/lib/PfbPipelineSelectorTools.ps1 Sort-PfbSelectorRecords,
Sort-PfbSelectorFindings
Sort is not an approved verb, but these are internal report helpers that
are never exported and never surface to a user running Get-Command. The
count is pinned at 3 by the maintainer: it must neither grow (a new
unapproved verb slipping in) nor shrink silently. Function-scoped
suppression, so a fourth would still be reported.
Every attribute is written with BOTH constructor arguments. PSScriptAnalyzer
reads the AST and accepts a one-argument form, but SuppressMessageAttribute
has no one-argument constructor, so executing such a file throws
"Cannot find an overload for .ctor and the argument count: 1" -- a defect
the analyzer cannot see and only a load or a Pester run catches.
Verification
Analyzer PSAvoidUsingConvertToSecureStringWithPlainText 21 -> 0
PSUseApprovedVerbs 3 -> 0
Error severity 0
total findings 596 -> 572
Per file loads clean; Pester discovery non-zero and passing
(39, 16, 18, 5, 3) -- a zero would have read as a pass
Full suite PS7 total=3632 passed=3630 failed=0 skipped=2
Wire out of scope for the live-test mandate; classified, not judged
Additions only -- 21 insertions, no line modified, no behaviour touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…IndexVersions
$Matches is the automatic variable PowerShell repopulates on every -match, and
Get-PfbSwaggerIndexVersions was assigning over it:
$matches = [regex]::Matches($IndexHtml, 'redoc/fb(\d+\.\d+)-api-reference\.html')
$versions = $matches | ForEach-Object { $_.Groups[1].Value } | Select-Object -Unique
Not a bug today -- the value is consumed on the very next line, and nothing between
the two lines runs a -match. It is a tripwire rather than a defect: inserting any
-match between them would silently clobber the collection, and the symptom would be
a wrong REST version list rather than an error. That is the expensive failure shape
for this function, because its output drives which spec files get fetched.
Renamed to $versionMatches. Two lines, one function, no call-site change -- the
variable never left this scope.
The other two $Matches references in the file are untouched and are genuine reads of
the automatic variable after a -match:
PfbSpecTools.ps1:140 return "/$($Matches[1])"
PfbSpecTools.ps1:329 $ownerForThis = $Matches[1]
Verification
Analyzer PSAvoidAssignmentToAutomaticVariable 1 -> 0
control: PSAvoidUsingEmptyCatchBlock still 11, so the sweep was live
and the zero is measured rather than vacuous
Parse 0 errors
Function Get-PfbSwaggerIndexVersions returns 2.0, 2.9, 2.28 from sample HTML --
ordering confirmed numeric, so 2.9 still precedes 2.28
empty-input control returns nothing, so the pass is not vacuous
Pester Tests\PfbSpecTools.Tests.ps1 total=55 passed=55 failed=0 skipped=0
This clears the last non-zero rule in the CI gate set planned for the analyzer
workflow; the other four guards were already at zero.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the analyzer configuration the CI job in the next commit reads. It is an
ALLOWLIST -- IncludeRules, not ExcludeRules -- so every rule that runs is there
because someone chose it, and every rule that does not run is absent for a reason
written down beside it.
Why an allowlist
Exclusions hide their own scope. The draft this replaces excluded rules for
Tests/ and would have silently disabled them everywhere the sweep path changed.
An allowlist makes both the inclusions and the omissions visible in one place,
which matters here because several omissions are load-bearing:
PSUseCompatibleCommands configured but NOT listed. Repo-wide it is 21889 --
Tests/ alone is 21870, because the rule compares
against profiles of BUILT-IN commands and reports
every Pester assertion. Only Public/ is genuinely
clean, so CI gates it there rather than repo-wide.
PSProvideCommentHelp configured but NOT listed, and needs
ExportedOnly = $false. At the default $true it
reports 0 across all 544 cmdlets WITHOUT EVALUATING
ANY OF THEM, because one function per dot-sourced
file means no analysed file holds its own export
statement.
Both are requested explicitly by the CI step and scoped to Public/. Note the
asymmetry that makes this work: a caller's -IncludeRule is UNIONED with this
file's IncludeRules, so the CI step must filter its results by RuleName -- an
unfiltered Public/ scan returns 22 records, none of them the gated rule.
ExcludeRules is the asymmetric one and VETOES a caller's -IncludeRule, so
excluding a rule here rather than merely not listing it would break the gate
outright.
T10 -- the ten default-disabled rules stay off
Recorded in the file rather than omitted, so their absence reads as a decision.
Force-enable sweep at this commit totals 13826 latent findings. The one to be
careful about is PSUseConstrainedLanguageMode (4825): it is not a style rule and
those are not formatting debt -- it reports constructs that would fail under
Constrained Language Mode, which this module does not claim to support. Enabling
it "to tidy up" would be a functional change.
Also not adopted: PSUseCompatibleTypes, at 30 rather than 0. All 30 are false
positives -- the rule reads neither $PSVersionTable guards nor #Requires, so a
type used only inside a correctly-guarded branch is still reported. It must not
be adopted later on a mistaken "it's already clean" reading.
Every count in the file is measured, not recalled
The generator (pssa-tasks\New-PfbPssaSettings.ps1, not part of this repo) now
measures each figure at generation time and substitutes it, rather than carrying
literals. This matters because the previous literals, one day old, had all
drifted:
PSUseSingularNouns 43 -> 44 (29 internal / 15 exported)
PSUseCompatibleCommands 20966 -> 21889
T10 latent total 13188 -> 13826
PSUseConstrainedLanguageMode 4569 -> 4825
union probe records 30 -> 22
The file records the date and commit it was measured at, since a count is only
meaningful against a tree.
Verification
Windows pwsh 7 -- total 276, Error 0, all three repo-wide guards 0,
GATE Public/ PSProvideCommentHelp 0 (CONTROL Private/ 12),
GATE Public/ PSUseCompatibleCommands 0 (CONTROL Private/ 2)
Linux Ubuntu 26.04, pwsh 7.6.3 Core -- byte-identical results on every one
of those figures, and both Windows compatibility profiles resolve on
a Linux install. Without that the gate would have passed vacuously
forever on the CI runner; the Private/ = 2 control is what proves the
rule actually evaluated.
Both a live control ran BEFORE every assertion, because each assertion is
"this count is zero" and an analyzer that returns nothing satisfies
all of them.
Parse Import-PowerShellDataFile succeeds on both platforms, 23 rules listed
Wire no executable change to module source; classified, not judged
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… + T7d)
Adds an `analyze` job to the Tests workflow and docs/script-analysis.md explaining
the configuration, what CI enforces, and what it deliberately does not.
Why a CI job and not a hook
The PostToolUse parse-check hook is per-edit and only ever sees the file just
written. This rule set is repo-wide and low-frequency: what it catches is
"someone added a 5.1-incompatible construct anywhere", which no per-file check
can see.
Why one Linux leg rather than the existing 4-leg matrix
The analyzer parses; it does not execute the module, so its findings do not vary
by host OS or edition. Verified rather than assumed -- the identical sweep on
Windows pwsh 7 and on Ubuntu 26.04 / pwsh 7.6.3 returned the same 276 total, the
same zero on every guard, and the same Private/ controls at 12 and 2. Four legs
would quadruple the cost for four identical results.
The compatibility rules look like the exception and are not: they target 5.1 and
7.0 by configuration, from static profiles shipped with the analyzer on every
platform, so the job reports on 5.1 from a Linux runner with no 5.1 present.
That the two Windows profiles resolve on a Linux install was checked, not
assumed -- had they not, PSUseCompatibleCommands would evaluate nothing and hand
the Public/ gate a permanent free pass.
The job takes no `needs:`. Nothing here reads tools/specs/, so it starts
immediately and finishes while the test matrix is still running.
What it gates -- six conditions, each at a measured zero
Error severity repo-wide 0
PSUseCompatibleSyntax repo-wide 0
PSAvoidAssignmentToAutomaticVariable repo-wide 0
PSUseBOMForUnicodeEncodedFile repo-wide 0
PSUseApprovedVerbs repo-wide 0
PSProvideCommentHelp Public/ 0 control Private/ 12
PSUseCompatibleCommands Public/ 0 control Private/ 2
Every one is a REGRESSION guard: it is not cleaning anything up, it is refusing
to let the first one in. Warnings are reported and not gated -- 276 today, and a
threshold would either be met trivially or block every PR.
Controls, because every assertion here is "this count is zero"
An analyzer that returns nothing satisfies all of them, and that is not
hypothetical: Invoke-ScriptAnalyzer honours -WhatIf and returns an empty set, and
a mis-specified rule name or an unparseable settings file produces the same
silence. So:
* A live probe runs BEFORE any assertion and fails the job if the analyzer
finds nothing in a snippet that definitely has a finding.
* The two Public/-scoped gates each require their rule to be NONZERO in
Private/. For PSProvideCommentHelp that is the likely failure mode, not a
theoretical one -- its default ExportedOnly = $true silences it entirely in
this codebase, so if that config is ever dropped the control fails instead of
the gate passing forever.
* The RuleName filter on the scoped gates is mandatory, not defensive: a
caller's -IncludeRule is UNIONED with the settings file's IncludeRules, so
an unfiltered Public/ scan returns 22 records, none of them the gated rule.
Verified both ways
Passes the exact `run:` text, extracted from the parsed YAML rather than
retyped, executed on Ubuntu 26.04 / pwsh 7.6.3:
control: analyzer live (1 finding on the probe)
total 276: 0 Error, 276 Warning
gate PSProvideCommentHelp (Public/): 0, control Private/: 12
gate PSUseCompatibleCommands (Public/): 0, control Private/: 2
PSScriptAnalyzer gate passed. EXIT=0
Fails a temporary file tripping two guards from different families -- an
assignment to $matches and an unapproved verb -- run through the same
script:
FAIL: PSAvoidAssignmentToAutomaticVariable regressed: 1, expected 0
FAIL: PSUseApprovedVerbs regressed: 1, expected 0
PSScriptAnalyzer gate failed: 2 condition(s). EXIT=1
A gate that has only ever been seen to pass is indistinguishable from
one that cannot fail. The file was deleted after the run.
YAML parses; jobs are prepare-specs, test-pwsh, analyze,
test-windows-powershell-5-1
Not gated, and why -- recorded in the job itself so a future reader does not have
to rediscover it: PSUseDeclaredVarsMoreThanAssignments (127, pending T4),
PSUseCompatibleCommands repo-wide (21,889, of which Tests/ is 21,870),
PSAvoidGlobalVars (61), PSUseSingularNouns (44), and PSAvoidUsingEmptyCatchBlock
(11, deferred to dmann000#117).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… + T4b)
Removes 127 assignments that nothing reads, taking
PSUseDeclaredVarsMoreThanAssignments from 127 to 0 and the repo-wide analyzer
total from 276 to 149.
Why now, when this was already decided against
tools/Update-PfbTestModuleImport.ps1 created this condition on purpose. Its
165-file pass replaced `Import-Module $manifest -Force` with a dot-source of
Tests/PfbTestModule.ps1 and deliberately left the surrounding assignments alone,
its header saying deleting them "would be a much larger, riskier diff for no
gain". That is this task, proposed and rejected in writing by the author of the
tool that produced the boilerplate.
Both halves of that rationale have since expired, which is why this is not a
re-litigation:
no gain While 127 known-dead assignments stand, the rule reports nothing
but them, so a genuinely dead variable in a NEW test file arrives as
finding 128 of 127 and is invisible. A PSGallery-preset rule is also
permanently unenforceable in CI. Neither consideration existed when
that paragraph was written -- the analyzer was not a project yet.
riskier The risk was assumed, not measured. Measured: 122 files flagged for
$manifest, and exactly one of them needs $moduleRoot kept.
That tool's header is amended in this commit rather than left to contradict the
tree, because re-closing this task on a stale paragraph is a failure that has
already happened once.
Why a script and not an edit session
Three files defeat three different mechanical strategies, and each would have
been a silent corruption:
ArrayConnection.ShouldProcessTarget.Tests.ps1 assigns at line 164, not the
template position, and passes it to Get-PfbTargetRecorder -Manifest.
Defeats "delete line 5".
RemovedCmdlets.Tests.ps1 byte-identical to the 122 in lines 1-5, then uses
$manifest at lines 10 and 38. Defeats template matching -- NOTHING in the
first five lines distinguishes it.
Update-PfbTestModuleImport.Tests.ps1 has no $manifest variable at all; every
occurrence is backtick-escaped fixture text inside a string literal.
Defeats a regex sweep, and did false-positive a grep during the
investigation.
So the file set came from Invoke-ScriptAnalyzer, the statements to delete came
from the AST, and every deletion was re-verified by re-parsing. The analyzer
excluded all three by itself. A filename exception list would have been a fact
about today's tree; an AST reference check stays true when a test is added.
Rails, all refusals rather than best-effort: a file is touched only if the
analyzer flagged that exact variable in it; a variable is removed only if the
AST shows no other reference; the right-hand side must be a bare
Join-Path/Split-Path, so nothing with a possible side effect is removed; the
file must still parse afterwards or the edit is rolled back. One file was
refused and handled by hand (below) -- a refusal is the rail working.
It is a TWO-pass operation, and one pass looks like a failure
While the $manifest line stands it REFERENCES $moduleRoot, so $moduleRoot is not
"assigned but never used" and the analyzer does not flag it. Deleting $manifest
orphans it, and only pass 2 sees it. Measured here: pass 1 edited 126 files and
moved the count 127 -> 119, because it was trading findings rather than removing
them. Pass 2 took 119 -> 2. Each pass re-runs every rail against the tree as the
previous pass actually left it, not against a stale in-memory plan.
The last two, by hand
Connect-PfbArray.CapabilityMapStaleness.Tests.ps1 refused, because it holds
two assignments to $moduleRoot and the script requires exactly one. The
line 4 one is dead; the line 86 one is inside an It block and is assigned
immediately before its use at line 96. Line 4 deleted, line 86 untouched.
Update-PfbEmptyPipelineGuards.Tests.ps1:285 refused correctly: the RHS is
New-GuardFixture, not Join-Path, and that call CREATES the on-disk fixture
the test then depends on. Only the assignment is dead, so it became
`$null = New-GuardFixture ...` -- the call stays.
Both were required for the follow-up commit: a gate written as "must be 0" would
fail on a residual of 2.
Verification
Rule PSUseDeclaredVarsMoreThanAssignments in Tests/: 127 -> 0, behind a
live control (a synthetic dead-variable snippet must return a
finding first). Invoke-ScriptAnalyzer declares SupportsShouldProcess,
so an inherited $WhatIfPreference silently empties its results -- a
zero from it means nothing without that control.
Parse all 209 files under Tests/ parse, 0 errors
Tests all 127 changed files run under BOTH editions, in three scoped
batches: pwsh 7 2043 passed / 0 failed, WinPS 5.1 2041 passed /
0 failed / 4 skipped. These are setup blocks, so an actual run is
the only proof that matters.
Exceptions the three files the script must not touch, plus the two hand-fixed
ones, re-run together: 88 passed / 0 failed on pwsh 7.
Analyzer repo-wide total 276 -> 149, Error still 0
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tightening the previous commit was for. The rule reached a repo-wide zero when
the 127 dead $manifest assignments were deleted, so it becomes the sixth
regression guard rather than a number reported and ignored.
Why it was not gated before, and why that was not caution
At 127 findings no threshold would have worked. Gating on zero was unpassable, and
gating on "no more than 127" is not a gate -- it accepts the 128th finding as long
as one of the known ones is fixed in the same PR. The rule was inert as a signal
regardless of what CI did with it. Deleting the boilerplate is what made a gate
expressible at all, which is the argument that justified the deletion.
It is also the one guard that cannot pass vacuously: the job's liveness probe
already analyses a synthetic dead-variable snippet with this exact rule before any
assertion runs. If the rule ever goes inert, the control throws first and the job
fails loudly instead of reporting a clean zero.
Verified both ways, on the CI platform
The `run:` text was extracted from the PARSED YAML rather than retyped, and
executed on Ubuntu 26.04 / pwsh 7.6.3:
Passes control: analyzer live (1 finding on the probe)
total 149: 0 Error, 149 Warning
gate PSProvideCommentHelp (Public/): 0, control Private/: 12
gate PSUseCompatibleCommands (Public/): 0, control Private/: 2
PSScriptAnalyzer gate passed.
Fails a temporary tools/ file holding one variable assigned and never read,
run through the same extracted script:
total 150: 0 Error, 150 Warning
::error file=...zz-gate-negative-control.ps1,line=8::
PSUseDeclaredVarsMoreThanAssignments: The variable
'deadOnPurpose' is assigned but never used.
FAIL: PSUseDeclaredVarsMoreThanAssignments regressed: 1 finding(s),
expected 0
PSScriptAnalyzer gate failed: 1 condition(s).
The file was deleted after the run; the tree was confirmed clean before
committing. A guard that has only ever been seen to pass is
indistinguishable from one that cannot fail, and this guard is new, so it
gets its own negative control rather than inheriting the last one's.
Also refreshed here
* The guard table in docs/script-analysis.md gained the new rule and
PSUseApprovedVerbs, which was already gated but undocumented.
* The "Pending" section became a record of WHY the rule is gateable, since the
thing it was pending on is done.
* Warning-count figures: 276 -> 149. The workflow comment now carries the whole
sequence -- 302, then 276, then 149 -- and says to re-measure rather than
trust the line, because each of those went stale within days.
* The -EnableExit example keeps 276 and now says so explicitly: it is this
repo's own pre-cleanup figure, and 276 mod 256 = 20 is what makes it a useful
illustration.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… at three
Three internal helpers in tools/ use the unapproved verb Sort-, and an earlier
commit suppressed rather than renamed them. This is the test that keeps that
decision honest.
Equalities in both directions, because either half alone cannot fail
* "no more than three" accepts a NEW unapproved verb as long as whoever adds it
also suppresses it.
* "at least three" cannot notice that one of them was renamed away, leaving a
suppression that now proves nothing.
So the set of unapproved-verb functions across Public/, Private/, tools/ and
scripts/ is asserted EQUAL to the three names, the count is pinned at 3, and each
of the three is asserted to carry exactly one matching suppression. A legitimate
change to any of that is expected to edit this test, which is the point.
tools/ and scripts/ are in scope deliberately: all three offenders live in tools/,
so a population check that swept only the shipped folders would be trivially
correct and permanently green.
The attribute shape is asserted separately, and it throws later than recorded
SuppressMessageAttribute has no one-argument constructor. PSScriptAnalyzer accepts
('PSUseApprovedVerbs') from the AST and reports the finding as suppressed, because
an analyzer only ever walks the AST and never constructs the attribute. So the
second argument is pinned to the empty string.
Measured while writing this, and it corrects what the working notes said: the
one-argument form throws on INVOCATION, not on definition or file load.
Dot-sourcing a file containing it succeeds silently; `Cannot find an overload for
".ctor" and the argument count: "1"` arrives the first time the function is
called. That matters -- a broken suppression in tools/ would survive module load,
survive any test that merely imports the file, and fail in the middle of a
build-tool run.
The probes were written the wrong way round first for exactly that reason: they
dot-sourced both forms and asserted the broken one threw, and it did not. Fixing
them to CALL the function is what produced the finding. A probe that only defined
the function would have passed on both forms and proven nothing.
No PSScriptAnalyzer dependency
Everything here is AST plus Get-Verb, so it runs on both editions with no module
to install and cannot report a vacuous zero from an inert analyzer. The live
finding count is already gated by the analyze job in CI; what this file pins is the
population and the attribute shape.
Verification
Passes 14 tests, 0 failed, under BOTH editions (pwsh 7 and Windows
PowerShell 5.1)
Fails a temporary tools/ file defining `Sort-PfbZzNegativeControl` failed
exactly one test -- "has exactly the three known unapproved-verb
functions, and no others" -- and nothing else. Deleted after the run.
Control the population test asserts >100 functions were found first: every
other assertion is over that collection, and an empty one would satisfy
them by accident if a directory moved or a file failed to parse.
Discovery the -ForEach list lives in BeforeDiscovery, not BeforeAll. Pester
evaluates -ForEach during discovery, before any BeforeAll runs, so from
BeforeAll it is $null and the file dies with "Value can not be null or
empty array" -- which reports as 3 passed rather than as 9 tests
silently never generated. Hit while writing this; recorded because the
symptom looks like a pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…CII pass One line. The em dash in the report's own header prose lives in a string literal in tools/Build-PfbValueEnumMap.ps1, which the non-ASCII pass (4949739) converted to `--`. The committed report still carried the old character, so a fresh regeneration differed and scripts/Assert-PfbDerivedArtifacts.ps1 reported it STALE. Regenerated through the gate with -UpdateCommitted rather than by running the generator bare: a bare run only reproduces the gate's output when tools/specs/ holds exactly the 29 pinned versions, and anything newer in the cache makes the same artifact report stale again. Worth noting for the branch as a whole: it is wire-EXEMPT and this file still moved. The two gates answer different questions -- one asks whether an executable line changed, the other whether a committed artifact still matches its source -- and passing one says nothing about the other. All 11 checked artifacts up to date afterwards. Co-Authored-By: Claude Opus 5 (1M context) <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.
What this is
PSScriptAnalyzer, configured and wired into CI, plus the source changes needed to
make the configuration honest. Nothing here changes what the module sends or how it
parses a response.
The headline is not the finding count. It is that four rules which previously
reported nothing actionable now report only real defects, and CI refuses to let the
first new one in.
ErrorseverityPSUseDeclaredVarsMoreThanAssignmentsPSUseBOMForUnicodeEncodedFilePSUseApprovedVerbsPSAvoidAssignmentToAutomaticVariableLive-testing exemption
EXEMPT. Classified mechanically by
Test-PfbWireExemption.ps1, not by readingthe diff. Ten in-scope files changed; every one classified
inert.The only executable changes in the branch are in
Tests/andtools/, neither ofwhich can alter a request or a response.
Worth flagging, because the two gates get conflated: this branch is wire-exempt and
still moved a committed artifact.
Reports/PfbValueEnumReconciliation.mdembeds aline of prose that lives in a string literal in
tools/Build-PfbValueEnumMap.ps1,and the ASCII pass changed an em dash in it, so a fresh regeneration differed.
scripts/Assert-PfbDerivedArtifacts.ps1caught it;e9253a1regenerates it throughthat gate. All 11 checked artifacts are up to date at HEAD. The wire test asks
whether an executable line changed; the artifact gate asks whether a committed file
still matches its source. Passing one says nothing about the other.
The commits
4949739e94012d03ea602$matches→$versionMatchesinGet-PfbSwaggerIndexVersions3a6076cPSScriptAnalyzerSettings.psd149aac99d8585a9$manifest/$moduleRootboilerplate fromTests/f0fe427PSUseDeclaredVarsMoreThanAssignmentsnow that it is at zeroc0e0a9fPSUseApprovedVerbspopulation and suppression shape at threee9253a1Reports/PfbValueEnumReconciliation.mdafter the ASCII passEach is independently reviewable and the commit messages carry the measurements
rather than repeating them here.
The configuration is an allowlist
IncludeRules, notExcludeRules, so every rule that runs is there because someonechose it and every rule that does not is absent with a reason written beside it. Two
omissions are load-bearing:
PSUseCompatibleCommands— configured but not listed. Repo-wide it reports21,889 findings, of which
Tests/is 21,870: the rule compares against profiles ofbuilt-in commands, so it reports every Pester assertion. Only
Public/isgenuinely clean, so CI gates it there.
PSProvideCommentHelp— configured but not listed, and needsExportedOnly = $false. At the default$trueit reports 0 across all 544 cmdletswithout evaluating any of them, because one function per dot-sourced file means
no analysed file holds its own export statement.
Note the asymmetry, because it is not obvious and it bites: a caller's
-IncludeRuleis UNION'd with the settings file'sIncludeRulesrather thanreplacing it, so the CI step must filter results by
RuleName— unfiltered, aPublic/scan returns 22 records, none of them the gated rule.ExcludeRulesis theasymmetric one and vetoes a caller's
-IncludeRule, so excluding a rule ratherthan merely not listing it would break the gate outright.
What CI gates
Seven conditions, each at a measured zero. Every one is a regression guard: it is
not cleaning anything up, it is refusing to let the first one in.
ErrorseverityPSUseCompatibleSyntaxPSAvoidAssignmentToAutomaticVariablePSUseBOMForUnicodeEncodedFilePSUseApprovedVerbsPSUseDeclaredVarsMoreThanAssignmentsPSProvideCommentHelpPublic/— control:Private/= 12PSUseCompatibleCommandsPublic/— control:Private/= 2Warnings are reported and not gated. 149 stand today; a threshold would either be
met trivially or block every PR. Do not read a green build as evidence the warning
count is holding.
One Linux leg, not the existing four-leg matrix: the analyzer parses, it does not
execute the module, so findings do not vary by host or edition. That was verified
rather than assumed — the identical sweep on Windows pwsh 7 and Ubuntu 26.04 / pwsh
7.6.3 returned the same total, the same zero on every guard, and the same
Private/controls. The compatibility rules look like the exception and are not: they target
5.1 from static profiles that ship with the analyzer on every platform, and that
those two Windows profiles resolve on a Linux install was checked, because had they
not,
PSUseCompatibleCommandswould evaluate nothing and hand thePublic/gate apermanent free pass.
Every zero here has a control
This is the part worth reviewing, because the whole PR is a set of assertions of the
form "this count is zero", and an analyzer that returns nothing satisfies all of
them.
That is not hypothetical.
Invoke-ScriptAnalyzerdeclaresSupportsShouldProcess,so a
$WhatIfPreferenceset by a calling script propagates into it: it analysesnothing and returns an empty result, with no error. That produced a real
findings (before) 0 … nothing to doagainst a tree holding 127 of them. Everyanalyzer call in this branch passes
-WhatIf:$false.So: a live probe runs before any assertion and fails the job if the analyzer
finds nothing in a snippet that definitely has a finding; each
Public/-scoped gaterequires its rule to be nonzero in
Private/; and each gate was proven able tofail on a deliberately-broken control file before its pass was believed, not just
observed to pass.
The one deliberately large diff
d8585a9touches 126 files inTests/, deleting a two-lineBeforeAlltemplatethat nothing reads. It deserves the scrutiny.
tools/Update-PfbTestModuleImport.ps1created this condition on purpose — its165-file pass replaced the import statement and left the surrounding assignments
alone, its header saying deleting them "would be a much larger, riskier diff for no
gain". Both halves of that have since expired, and its header is amended in the same
commit rather than left to contradict the tree:
them, so a genuinely dead variable in a new test file arrives as finding 128 of
127 and is invisible. No gate is expressible at any threshold. That is the whole
reason the deletion is worth doing.
exactly one of them needs
$moduleRootkept.It was scripted rather than hand-edited because three files defeat three different
mechanical strategies, and each would have been a silent corruption: one assigns at
line 164 rather than the template position; one is byte-identical in lines 1–5 and
then uses
$manifestat lines 10 and 38; one has no$manifestvariable at all,only backtick-escaped fixture text inside a string literal. The file set therefore
came from the analyzer's own output and the statements from the AST — never a regex,
never a line number — and every deletion was re-verified by re-parsing.
Two files needed hands.
Update-PfbEmptyPipelineGuards.Tests.ps1:285was known: theright-hand side is a call that creates the fixture the test depends on, so the
assignment went and the call stayed. The other was not:
Connect-PfbArray.CapabilityMapStaleness.Tests.ps1holds two assignments to$moduleRoot— line 4 dead, line 86 assigned immediately before its use at line 96 —so the script refused it rather than guessing. Both refusals are the safety rails
working as designed.
Testing
Windows PowerShell 5.1 — 2041 passed, 0 failed, 4 skipped. These are setup blocks,
so an actual run is the only proof that matters.
together: 88 passed, 0 failed.
Tests/PfbApprovedVerbSuppressions.Tests.ps1: 14 passed, 0 failed on botheditions, and negative-controlled — a temporary unapproved-verb function failed
exactly one test and nothing else.
run:text, extracted from the parsed YAML rather thanretyped, executed on Ubuntu 26.04 / pwsh 7.6.3: passes at 149/0 Error, and fails
with
EXIT=1naming the rule when handed a deliberately-broken file.Not addressed here
PSAvoidUsingEmptyCatchBlock) — deferred to 13 Get-Pfb* cmdlets swallow API errors and return $null, making a failed call indistinguishable from an empty result #117, whichalready adjudicated all three shipped sites. Reported, never gated, so CI is green
with them outstanding. Worth knowing: a comment does not clear this rule
(
catch { # x }is still flagged), so every working fix adds an executable line toshipped code and would require a live FlashBlade run.
PSAvoidGlobalVars(61) — a deliberate Pester cross-mock-scope pattern.PSUseSingularNouns(44) — kept in the settings file, deliberately not gated.(13,826 latent findings) are recorded in the settings file rather than omitted, so
their absence reads as a choice. The one to be careful about is
PSUseConstrainedLanguageMode(4,825): it reports constructs that would fail underConstrained Language Mode, which this module does not claim to support, so enabling
it "to tidy up" would be a functional change.