fix: Settle the severity threshold contract and retry analyzer crashes

CHANGELOG.md

@@ -9,6 +9,15 @@
 
 ### 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 @@
   [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
@@ -34,7 +62,7 @@
   that passed before may now correctly fail.
 - [**#96**](https://github.com/psake/PowerShellBuild/issues/96)
   `Test-PSBuildScriptAnalysis` no longer fails with a path-resolution error
   when `SettingsPath` is not supplied. An unsupplied path was forwarded to
   PSScriptAnalyzer as `-Settings ''`, which resolved against the current
   directory and threw before any analysis ran, so the function's own
   documented example could not run as written.

PowerShellBuild/Public/Test-PSBuildScriptAnalysis.ps1

@@ -8,19 +8,32 @@
         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
@@ -32,19 +45,52 @@
         Path    = $Path
         Recurse = $true
     }
     # An unsupplied SettingsPath must not be forwarded. PSScriptAnalyzer resolves an empty
     # -Settings value against the current directory and fails before any analysis runs.
     if (-not [string]::IsNullOrWhiteSpace($SettingsPath)) {
         $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 @@
                 throw $LocalizedData.ScriptAnalyzerWarnings
             }
         }
+        'Any' {
+            if ($analysisRecords.Count -gt 0) {
+                throw $LocalizedData.ScriptAnalyzerIssues
+            }
+        }
         default {
             if ($analysisRecords.Count -ne 0) {
                 throw $LocalizedData.ScriptAnalyzerIssues

PowerShellBuild/build.properties.ps1

@@ -66,10 +66,14 @@
             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.
@@ -126,7 +130,7 @@
         # Value passed to New-MarkdownHelp and Update-MarkdownHelp.
         AlphabeticParamsOrder = $false
 
         # Exclude the parameters marked with `DontShow` in the parameter attribute from the help content.
         # Value passed to New-MarkdownHelp and Update-MarkdownHelp.
         ExcludeDontShow       = $false
 
@@ -169,10 +173,10 @@
 
         # Name of the environment variable that holds the Base64-encoded PFX certificate.
         # Used by the EnvVar source and as the presence-detection key for Auto.
         CertificateEnvVar         = 'SIGNCERTIFICATE'
 
         # Name of the environment variable that holds the PFX password (EnvVar source).
         CertificatePasswordEnvVar = 'CERTIFICATEPASSWORD'
 
         # File system path to a PFX/P12 certificate file (PfxFile source).
         PfxFilePath               = $null

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}]

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

tests/Test-PSBuildScriptAnalysis.tests.ps1

@@ -19,8 +19,8 @@
         Import-Module -Name ([IO.Path]::Combine($script:moduleRoot, 'Output', 'PowerShellBuild')) -Force
 
         $script:cleanPath = Join-Path -Path $TestDrive -ChildPath 'clean'
         $script:errorFindingPath = Join-Path -Path $TestDrive -ChildPath 'errorfinding'
         $script:warningFindingPath = Join-Path -Path $TestDrive -ChildPath 'warningfinding'
         foreach ($directory in @($script:cleanPath, $script:errorFindingPath, $script:warningFindingPath)) {
             New-Item -Path $directory -ItemType Directory -Force > $null
         }
@@ -95,7 +95,7 @@
             $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 @@
         }
     }
 
+    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> 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 <FindingSeverity> 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' {
@@ -310,7 +468,7 @@
             { Test-PSBuildScriptAnalysis @testParameters } | Should -Not -Throw
         }
 
         # Regression: an unsupplied SettingsPath was passed to the analyzer as -Settings '',
         # which PSScriptAnalyzer resolved against the current directory and rejected, so the
         # documented example (no -SettingsPath) failed with a path error instead of analyzing.
         It 'Analyzes with the default rule set when no settings path is supplied' {