From b6d3146aed2a404fc9257e6cfd96727da36d073f Mon Sep 17 00:00:00 2001 From: Trent Blackburn Date: Thu, 20 Aug 2026 00:31:10 -0400 Subject: [PATCH 1/2] fix: Settle the FailBuildOnSeverityLevel contract and retry analyzer crashes Four related problems in Test-PSBuildScriptAnalysis, all of which needed a decision about the public surface rather than a local fix. ParseError is a fourth PSScriptAnalyzer severity, reported for a file that does not parse at all. It was counted by no threshold, so it escaped even the strictest validated value (Information): the record was printed and the build passed. A file the engine cannot read is at least as severe as an analyzer error, so ParseError is now counted with Error and fails every threshold except None. This is a behavioral change that can turn a passing consumer build red, so it has a migration-guide entry. 'Any' was documented in build.properties.ps1 but missing from the ValidateSet, so setting FailBuildOnSeverityLevel = 'Any' failed parameter binding instead of doing what the documentation promised. It is now accepted and wired to the branch that fails on any record. Additive, so no consumer action is required. The comment documenting all of this contradicted itself -- it opened with a list of valid values that did not include 'Any' and then described 'Any' -- and never said what Information does. Rewritten to describe every value. Finally, PSScriptAnalyzer can crash one of its own rules on an internal race that has nothing to do with the code being analyzed (PSScriptAnalyzer#1538, deferred to 2.0). A re-run succeeds, so the analysis is retried up to three times when a rule reports RULE_ERROR. Errors are captured during the retries rather than allowed to surface, because a consumer with ErrorActionPreference = 'Stop' would otherwise terminate on the first crash and never reach the second attempt; whatever the final attempt still reports is re-emitted, so a persistent failure behaves exactly as it did before. Per #147's own measurements a crashed rule does not cost findings, so this removes a flake without changing what a real failure does to anyone. Closes #144 Closes #147 --- CHANGELOG.md | 28 +++ .../Public/Test-PSBuildScriptAnalysis.ps1 | 57 ++++++- PowerShellBuild/build.properties.ps1 | 12 +- PowerShellBuild/en-US/Messages.psd1 | 1 + docs/migration-v0.8-to-v1.0.md | 44 +++++ tests/Test-PSBuildScriptAnalysis.tests.ps1 | 160 +++++++++++++++++- 6 files changed, 294 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd8089f..dd54893 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed +- [**#144**](https://github.com/psake/PowerShellBuild/issues/144) + **Breaking:** `Test-PSBuildScriptAnalysis` now counts PSScriptAnalyzer + `ParseError` records alongside `Error`. A file that does not parse at all + previously satisfied no threshold — not even the strictest validated value, + `Information` — so it was reported and the build passed anyway. It now fails + every threshold except `None`. See the + [v0.8 → v1.0 migration guide](docs/migration-v0.8-to-v1.0.md) — a build that + passed before may now correctly fail. + - [**#120**](https://github.com/psake/PowerShellBuild/issues/120) **Breaking:** the module manifest now requires PowerShell 5.1 or newer (`PowerShellVersion = '5.1'`, previously `'3.0'`) and declares @@ -18,8 +27,27 @@ and this project adheres to [Semantic Versioning](http://semver.org/). [v0.8 → v1.0 migration guide](docs/migration-v0.8-to-v1.0.md) for details. +### Added + +- [**#144**](https://github.com/psake/PowerShellBuild/issues/144) + `Test-PSBuildScriptAnalysis` accepts `Any` as a `SeverityThreshold`, failing + the build on any diagnostic record regardless of severity. `Any` was already + documented in `build.properties.ps1` but was missing from the parameter's + `ValidateSet`, so setting + `$PSBPreference.Test.ScriptAnalysis.FailBuildOnSeverityLevel = 'Any'` failed + parameter binding instead of working as documented. + ### Fixed +- [**#147**](https://github.com/psake/PowerShellBuild/issues/147) + `Test-PSBuildScriptAnalysis` retries the analysis when a PSScriptAnalyzer rule + crashes on an internal race + ([PSScriptAnalyzer#1538](https://github.com/PowerShell/PSScriptAnalyzer/issues/1538)), + which is unrelated to the code being analyzed and succeeds on a re-run. + Consumers who set `$ErrorActionPreference = 'Stop'` — common in a build script — + previously got a randomly red build. A crash that survives every attempt is + still surfaced, so a persistent failure behaves as it did before. + - [**#96**](https://github.com/psake/PowerShellBuild/issues/96) `Test-PSBuildScriptAnalysis` now fails the build when PSScriptAnalyzer reports findings at or above the configured severity threshold. The diff --git a/PowerShellBuild/Public/Test-PSBuildScriptAnalysis.ps1 b/PowerShellBuild/Public/Test-PSBuildScriptAnalysis.ps1 index deff786..62e359a 100644 --- a/PowerShellBuild/Public/Test-PSBuildScriptAnalysis.ps1 +++ b/PowerShellBuild/Public/Test-PSBuildScriptAnalysis.ps1 @@ -8,19 +8,32 @@ function Test-PSBuildScriptAnalysis { Path to PowerShell module directory to run ScriptAnalyzer on. .PARAMETER SeverityThreshold Fail ScriptAnalyzer test if any issues are found with this threshold or higher. + + 'None' reports findings without ever failing the build. 'Information', 'Warning', and + 'Error' fail on a finding at that severity or higher. 'Any' fails on any diagnostic + record at all, regardless of severity. + + PSScriptAnalyzer also emits ParseError records for files that do not parse. Those are + counted alongside Error, so a file that cannot be parsed fails every threshold except + 'None'. .PARAMETER SettingsPath Path to ScriptAnalyzer settings to use. .EXAMPLE PS> Test-PSBuildScriptAnalysis -Path ./Output/MyModule/0.1.0 -SeverityThreshold Error Run ScriptAnalyzer on built module in ./Output/MyModule/0.1.0. Throw error if any errors are found. + .EXAMPLE + PS> Test-PSBuildScriptAnalysis -Path ./Output/MyModule/0.1.0 -SeverityThreshold Any + + Run ScriptAnalyzer on built module in ./Output/MyModule/0.1.0. Throw error if any + diagnostic record is returned, regardless of its severity. #> [CmdletBinding()] param( [parameter(Mandatory)] [string]$Path, - [ValidateSet('None', 'Error', 'Warning', 'Information')] + [ValidateSet('None', 'Error', 'Warning', 'Information', 'Any')] [string]$SeverityThreshold, [string]$SettingsPath @@ -38,13 +51,46 @@ function Test-PSBuildScriptAnalysis { $invokeScriptAnalyzerParameters.Settings = $SettingsPath } - $analysisResult = Invoke-ScriptAnalyzer @invokeScriptAnalyzerParameters -Verbose:$VerbosePreference + # PSScriptAnalyzer runs its script rules in parallel against a process-wide, unsynchronised + # singleton, so a rule can crash on an internal race that has nothing to do with the code + # under analysis (PSScriptAnalyzer#1538, #1351 -- both open, deferred to 2.0). A re-run + # succeeds, so retry rather than letting a random red build reach the consumer. The analyzer + # isolates a failed rule and still returns every other rule's findings, so a crash is not by + # itself a reason to fail. See psake/PowerShellBuild#147. + # + # Errors are captured rather than allowed to surface during the retries, because a consumer + # with $ErrorActionPreference = 'Stop' would otherwise terminate on the first crash and never + # reach the second attempt. Whatever remains after the final attempt is re-emitted below, so + # a persistent failure behaves exactly as it did before this retry existed. + $maximumAttempt = 3 + for ($attempt = 1; $attempt -le $maximumAttempt; $attempt++) { + $analysisErrors = @() + $analysisResult = Invoke-ScriptAnalyzer @invokeScriptAnalyzerParameters ` + -Verbose:$VerbosePreference -ErrorAction SilentlyContinue -ErrorVariable analysisErrors + + # Only the analyzer's own rule crashes are worth retrying. Anything else is a real + # failure that a second attempt will not change. + $ruleErrors = @($analysisErrors).Where({ $_.FullyQualifiedErrorId -like 'RULE_ERROR*' }) + if ($ruleErrors.Count -eq 0 -or $attempt -eq $maximumAttempt) { + break + } + + Write-Warning ($LocalizedData.ScriptAnalyzerRuleErrorRetry -f $attempt, $maximumAttempt, $ruleErrors[0].Exception.Message) + } + + # Surface anything the final attempt still reported, honouring the caller's error preference. + foreach ($analysisError in @($analysisErrors)) { + Write-Error -ErrorRecord $analysisError + } # A single diagnostic record comes back as a scalar rather than a collection, and Windows # PowerShell 5.1 does not expose .Where() or .Count on every scalar type. Wrapping in @() # guarantees collection semantics on both engines. $analysisRecords = @($analysisResult) - $errorCount = ($analysisRecords.Where({ $_.Severity -eq 'Error' })).Count + # ParseError is a fourth PSScriptAnalyzer severity, reported for a file that does not parse + # at all. It is counted with Error: a file the engine cannot read is at least as severe as + # an analyzer error, and leaving it out let it escape every threshold. + $errorCount = ($analysisRecords.Where({ $_.Severity -in @('Error', 'ParseError') })).Count $warningCount = ($analysisRecords.Where({ $_.Severity -eq 'Warning' })).Count $informationCount = ($analysisRecords.Where({ $_.Severity -eq 'Information' })).Count @@ -72,6 +118,11 @@ function Test-PSBuildScriptAnalysis { throw $LocalizedData.ScriptAnalyzerWarnings } } + 'Any' { + if ($analysisRecords.Count -gt 0) { + throw $LocalizedData.ScriptAnalyzerIssues + } + } default { if ($analysisRecords.Count -ne 0) { throw $LocalizedData.ScriptAnalyzerIssues diff --git a/PowerShellBuild/build.properties.ps1 b/PowerShellBuild/build.properties.ps1 index 2f1c0b0..9dc26b5 100644 --- a/PowerShellBuild/build.properties.ps1 +++ b/PowerShellBuild/build.properties.ps1 @@ -66,10 +66,14 @@ $moduleVersion = (Import-PowerShellDataFile -Path $env:BHPSModuleManifest).Modul Enabled = $true # When PSScriptAnalyzer is enabled, control which severity level will generate a build failure. - # Valid values are Error, Warning, Information and None. "None" will report errors but will not - # cause a build failure. "Error" will fail the build only on diagnostic records that are of - # severity error. "Warning" will fail the build on Warning and Error diagnostic records. - # "Any" will fail the build on any diagnostic record, regardless of severity. + # Valid values are None, Information, Warning, Error, and Any. + # "None" reports findings but never fails the build. + # "Information" fails the build on Information, Warning, and Error records. + # "Warning" fails the build on Warning and Error records. + # "Error" fails the build only on Error records. + # "Any" fails the build on any diagnostic record, regardless of severity. + # PSScriptAnalyzer also reports ParseError records for files that do not parse at all. + # Those are counted with Error, so an unparsable file fails every level except "None". FailBuildOnSeverityLevel = 'Error' # Path to the PSScriptAnalyzer settings file. diff --git a/PowerShellBuild/en-US/Messages.psd1 b/PowerShellBuild/en-US/Messages.psd1 index 22f370d..9149e18 100644 --- a/PowerShellBuild/en-US/Messages.psd1 +++ b/PowerShellBuild/en-US/Messages.psd1 @@ -24,6 +24,7 @@ PSScriptAnalyzerResults=PSScriptAnalyzer results: ScriptAnalyzerErrors=One or more ScriptAnalyzer errors were found! ScriptAnalyzerWarnings=One or more ScriptAnalyzer warnings were found! ScriptAnalyzerIssues=One or more ScriptAnalyzer issues were found! +ScriptAnalyzerRuleErrorRetry=A PSScriptAnalyzer rule failed on attempt {0} of {1} and the analysis will be retried. This is an analyzer race, not a problem with the code being analyzed: {2} NoCertificateFound=No valid code signing certificate was found. Verify the configured CertificateSource and that a certificate with a private key is available. CertificateResolvedFromStore=Resolved code signing certificate from store [{0}]: Subject=[{1}] CertificateResolvedFromThumbprint=Resolved code signing certificate by thumbprint [{0}]: Subject=[{1}] diff --git a/docs/migration-v0.8-to-v1.0.md b/docs/migration-v0.8-to-v1.0.md index 04b0b6e..6268436 100644 --- a/docs/migration-v0.8-to-v1.0.md +++ b/docs/migration-v0.8-to-v1.0.md @@ -25,6 +25,9 @@ One line per break; follow the link for details and migration steps. - [Script analysis now actually fails the build](#script-analysis-now-actually-fails-the-build) — the `Analyze` task's severity threshold never fired in 0.8.x; a build that passed before may now correctly fail. +- [Unparsable files now fail the script analysis gate](#unparsable-files-now-fail-the-script-analysis-gate) + — `ParseError` findings are counted with `Error`, so a file that does not + parse fails every threshold except `None`. > More entries will follow as the Phase 2 migrations to > Microsoft.PowerShell.PlatyPS 1.x and psake 5.x land. @@ -156,6 +159,47 @@ now runs as documented instead of throwing. Tracked in issue #96. +### Unparsable files now fail the script analysis gate + +PSScriptAnalyzer's severity enum has four members — `Information`, +`Warning`, `Error`, and `ParseError`. In 0.8.x, +`Test-PSBuildScriptAnalysis` counted only the first three. A +`ParseError` record — a file that does not parse at all — therefore +satisfied no threshold, including the strictest one available +(`Information`): the record was printed in the results table and the +build passed. + +`ParseError` is now counted alongside `Error`, so a file the engine +cannot even read fails every threshold except `None`. A file that does +not parse cannot be meaningfully analyzed, so this closes a gap where +the most severe possible finding was the only one that could never fail +a build. + +**No configuration change is required.** If your build starts failing at +the `Analyze` task after upgrading and the reported record has severity +`ParseError`, the file genuinely does not parse — fix the syntax error. +It was being reported on 0.8.x too; it just never failed anything. + +To check before you upgrade: + + Invoke-ScriptAnalyzer -Path ./Output/MyModule/1.0.0 -Recurse | + Where-Object Severity -eq 'ParseError' + +Any output there is what will start failing your build. + +Alongside this, the severity threshold gains an **`Any`** value, which +fails the build on any diagnostic record regardless of severity: + + $PSBPreference.Test.ScriptAnalysis.FailBuildOnSeverityLevel = 'Any' + +`Any` was documented in `build.properties.ps1` on 0.8.x but was missing +from the parameter's `ValidateSet`, so setting it failed parameter +binding rather than doing what the documentation promised. It now works. +This is additive — existing values behave as before. + +Tracked in issue +[#144](https://github.com/psake/PowerShellBuild/issues/144). + ## Adding an entry (for PR contributors) Every breaking-change PR that lands in v1.0.0 must add an entry here for diff --git a/tests/Test-PSBuildScriptAnalysis.tests.ps1 b/tests/Test-PSBuildScriptAnalysis.tests.ps1 index 600a114..ee121c1 100644 --- a/tests/Test-PSBuildScriptAnalysis.tests.ps1 +++ b/tests/Test-PSBuildScriptAnalysis.tests.ps1 @@ -95,7 +95,7 @@ Describe 'Test-PSBuildScriptAnalysis' { $validateSet = $command.Parameters['SeverityThreshold'].Attributes.Where({ $_.TypeId.Name -eq 'ValidateSetAttribute' })[0] - ($validateSet.ValidValues | Sort-Object) -join ',' | Should -Be 'Error,Information,None,Warning' + ($validateSet.ValidValues | Sort-Object) -join ',' | Should -Be 'Any,Error,Information,None,Warning' } } @@ -249,6 +249,164 @@ Describe 'Test-PSBuildScriptAnalysis' { } } + Context 'ParseError severity' { + + # PSScriptAnalyzer's severity enum has four members; ParseError is reported for a file + # that does not parse at all. It was counted by none of the thresholds, so the strictest + # validated value (Information) still let an unparsable file through: the record was + # printed in the results table and the build passed. It is now counted with Error. + + It 'Fails at the threshold when a ParseError record is returned' -ForEach @( + @{ Threshold = 'Error' } + @{ Threshold = 'Warning' } + @{ Threshold = 'Information' } + @{ Threshold = 'Any' } + ) { + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { + [PSCustomObject]@{ + Severity = 'ParseError' + RuleName = 'FakeParseErrorRule' + ScriptName = 'Unparsable.ps1' + Message = 'A fake ParseError record' + } + } + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = $Threshold + SettingsPath = $script:defaultSettingsPath + } + { Test-PSBuildScriptAnalysis @testParameters } | Should -Throw + } + + It 'Still reports without failing at the None threshold' { + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { + [PSCustomObject]@{ + Severity = 'ParseError' + RuleName = 'FakeParseErrorRule' + ScriptName = 'Unparsable.ps1' + Message = 'A fake ParseError record' + } + } + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = 'None' + SettingsPath = $script:defaultSettingsPath + } + { Test-PSBuildScriptAnalysis @testParameters } | Should -Not -Throw + } + } + + Context 'Any threshold' { + + # 'Any' was documented in build.properties.ps1 but missing from the ValidateSet, so + # setting it failed parameter binding instead of doing what the documentation promised. + + It 'Fails on a finding' -ForEach @( + @{ FindingSeverity = 'Error' } + @{ FindingSeverity = 'Warning' } + @{ FindingSeverity = 'Information' } + ) { + $mockFindings = @( + [PSCustomObject]@{ + Severity = $FindingSeverity + RuleName = "Fake${FindingSeverity}Rule" + ScriptName = 'Fake.ps1' + Message = "A fake $FindingSeverity record" + } + ) + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { + $mockFindings + }.GetNewClosure() + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = 'Any' + SettingsPath = $script:defaultSettingsPath + } + { Test-PSBuildScriptAnalysis @testParameters } | Should -Throw + } + + It 'Passes when there are no findings at all' { + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { } + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = 'Any' + SettingsPath = $script:defaultSettingsPath + } + { Test-PSBuildScriptAnalysis @testParameters } | Should -Not -Throw + } + } + + Context 'Analyzer rule crash retry' { + + # PSScriptAnalyzer can crash a rule on an internal race unrelated to the code being + # analyzed (psake/PowerShellBuild#147). A re-run succeeds, so a RULE_ERROR is retried. + # A consumer with $ErrorActionPreference = 'Stop' would otherwise get a random red build. + + It 'Retries and succeeds when a rule crashes once' { + $script:analyzerAttempt = 0 + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { + $script:analyzerAttempt++ + if ($script:analyzerAttempt -eq 1) { + # Non-terminating, mirroring how the real cmdlet behaves under the + # -ErrorAction SilentlyContinue the function passes. The mock body would + # otherwise inherit $ErrorActionPreference = 'Stop' and throw on attempt one. + $ErrorActionPreference = 'SilentlyContinue' + Write-Error -Message 'Object reference not set to an instance of an object.' -ErrorId 'RULE_ERROR' + } + } + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = 'Error' + SettingsPath = $script:defaultSettingsPath + } + { Test-PSBuildScriptAnalysis @testParameters -WarningAction SilentlyContinue } | + Should -Not -Throw + + Should -Invoke -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -Times 2 -Exactly + } + + It 'Gives up after three attempts and surfaces the error' { + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { + $ErrorActionPreference = 'SilentlyContinue' + Write-Error -Message 'Object reference not set to an instance of an object.' -ErrorId 'RULE_ERROR' + } + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = 'Error' + SettingsPath = $script:defaultSettingsPath + } + # A persistent failure must still reach the caller, so a consumer using + # ErrorActionPreference = 'Stop' behaves exactly as it did before the retry existed. + { Test-PSBuildScriptAnalysis @testParameters -WarningAction SilentlyContinue -ErrorAction Stop } | + Should -Throw + + Should -Invoke -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -Times 3 -Exactly + } + + It 'Does not retry an error that is not a rule crash' { + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { + $ErrorActionPreference = 'SilentlyContinue' + Write-Error -Message 'Some other analyzer failure.' -ErrorId 'SOME_OTHER_ERROR' + } + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = 'Error' + SettingsPath = $script:defaultSettingsPath + } + { Test-PSBuildScriptAnalysis @testParameters -ErrorAction SilentlyContinue } | + Should -Not -Throw + + Should -Invoke -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -Times 1 -Exactly + } + } + Context 'End-to-end analysis' { It 'Fails a script with an Error-severity finding at the Error threshold' { From fb586b3bbf7a79d40153461cb360c78c55d83a2c Mon Sep 17 00:00:00 2001 From: Trent Blackburn Date: Sat, 22 Aug 2026 11:38:21 -0400 Subject: [PATCH 2/2] docs: Drop misleading "only" from the Error threshold comment The "Error" line said the threshold fails the build "only on Error records", which contradicts the ParseError note two lines below it. ParseError records are counted with Error, so they fail at every threshold except "None". Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018TJfFJGtUJY5CFu8MRRMYt --- PowerShellBuild/build.properties.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/PowerShellBuild/build.properties.ps1 b/PowerShellBuild/build.properties.ps1 index 9dc26b5..541cda9 100644 --- a/PowerShellBuild/build.properties.ps1 +++ b/PowerShellBuild/build.properties.ps1 @@ -70,7 +70,7 @@ $moduleVersion = (Import-PowerShellDataFile -Path $env:BHPSModuleManifest).Modul # "None" reports findings but never fails the build. # "Information" fails the build on Information, Warning, and Error records. # "Warning" fails the build on Warning and Error records. - # "Error" fails the build only on Error records. + # "Error" fails the build on Error records. # "Any" fails the build on any diagnostic record, regardless of severity. # PSScriptAnalyzer also reports ParseError records for files that do not parse at all. # Those are counted with Error, so an unparsable file fails every level except "None".