fix: Make the script analysis severity threshold fail the build#143
Open
tablackburn wants to merge 3 commits into
Open
fix: Make the script analysis severity threshold fail the build#143tablackburn wants to merge 3 commits into
tablackburn wants to merge 3 commits into
Conversation
Test-PSBuildScriptAnalysis counted findings by severity using $_Severity, an undefined variable, rather than $_.Severity. Every count was therefore zero and the Error, Warning, and Information thresholds could never throw. The Analyze task printed its findings and passed regardless, so for every consumer using the default FailBuildOnSeverityLevel of Error, the gate only looked like it was working. Also stop forwarding an unsupplied SettingsPath to the analyzer. It was passed through as -Settings '', which PSScriptAnalyzer resolved against the current directory and rejected before running any analysis, so the function's own documented example could not run as written. Add tests covering both fixes (#96). Severity gating runs against a mocked Invoke-ScriptAnalyzer so every finding/threshold combination is exercised deterministically: rule severities can change between PSScriptAnalyzer releases, and no rule in the default set reliably emits an Information record. A smaller end-to-end layer runs the real analyzer over scripts generated into $TestDrive to prove the wiring and settings-file handling. Closes #96 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BxrhSpD47TSrS9hEJpDMaH
There was a problem hiding this comment.
Pull request overview
This PR strengthens the Test-PSBuildScriptAnalysis build gate by adding comprehensive Pester coverage and correcting logic that previously prevented configured severity thresholds from failing the build, plus improving settings-file forwarding behavior.
Changes:
- Add a new Pester test suite covering command surface, severity gating (mocked analyzer), end-to-end analyzer runs, and settings-file behavior.
- Fix severity counting in
Test-PSBuildScriptAnalysis(use$_.Severityand clearer variable names) and avoid passing an empty-Settingsvalue to PSScriptAnalyzer. - Document the behavior change in the v1.0 migration guide and changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/Test-PSBuildScriptAnalysis.tests.ps1 |
Adds layered unit + end-to-end tests to ensure severity gating and settings-path handling behave correctly. |
PowerShellBuild/Public/Test-PSBuildScriptAnalysis.ps1 |
Fixes the severity threshold enforcement logic and conditionally forwards analyzer settings. |
docs/migration-v0.8-to-v1.0.md |
Adds a migration note that the previously inert script-analysis gate can now fail builds. |
CHANGELOG.md |
Adds two “Fixed” entries describing the severity gate fix and settings-path forwarding fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The Windows PowerShell 5.1 CI job failed six of the new severity-gating tests with "[System.Management.Automation.PSCustomObject] does not contain a method named 'Where'". PowerShell unrolls a single-element array, so a mock returning one record handed the function a scalar PSCustomObject. Windows PowerShell 5.1 does not expose .Where() on that type while PowerShell 7 does. Every end-to-end test using real DiagnosticRecord objects passed on 5.1, including the single-record case, so real analyzer output was never affected. The failure still exposed a latent fragility worth fixing: both the .Where() calls and the $analysisResult.Count check in the no-threshold branch assumed collection semantics that 5.1 does not guarantee for every scalar type. Wrapping the result in @() yields an [object[]] on both engines, which always supports .Where() and .Count, and makes the function independent of how many records the analyzer returns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BxrhSpD47TSrS9hEJpDMaH
The guide stated it covered breaking changes only, while already carrying an entry for a bug fix that can turn a passing build red on upgrade. State the scope the guide actually serves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BxrhSpD47TSrS9hEJpDMaH
This was referenced Jul 25, 2026
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.
Description
Adds tests for
Test-PSBuildScriptAnalysis(#96) and fixes two latent bugs the tests exposed.Bug 1 — the severity gate never fired. Findings were counted with
$_Severity, an undefined variable, instead of$_.Severity. All three counts were therefore always0, so theError,Warning, andInformationbranches of the threshold switch could never throw. TheAnalyzetask printed its findings and passed regardless. SinceFailBuildOnSeverityLeveldefaults toError, every consumer running that task had a gate that only looked like it was working.Bug 2 — an unsupplied
SettingsPathbroke the call. It was forwarded to the analyzer as-Settings '', which PSScriptAnalyzer resolves against the current directory and rejects before running any analysis. The function's own documented example (Test-PSBuildScriptAnalysis -Path ./Output/MyModule/0.1.0 -SeverityThreshold Error) therefore could not run as written.-Settingsis now only passed when a settings path is supplied.While touching those lines,
$errors/$warnings/$infoswere renamed to$errorCount/$warningCount/$informationCountperinstructions/shorthand.instructions.md.Tests (
tests/Test-PSBuildScriptAnalysis.tests.ps1, 31 tests) are layered:Invoke-ScriptAnalyzerso every finding/threshold combination is exercised deterministically. Real analysis cannot cover the full matrix: rule severities can change between PSScriptAnalyzer releases, and no rule in the default rule set reliably emits anInformationrecord.$TestDrive(anError-severity fixture viaPSAvoidUsingConvertToSecureStringWithPlainText, aWarning-severity fixture viaPSAvoidUsingWriteHost, and a clean fixture), proving the function is genuinely wired to PSScriptAnalyzer and honors a settings file that excludes a rule.Fixture scripts contain deliberate analyzer violations, are generated at runtime, and are never checked in, so the repository's own script analysis can never discover them (the #97 convention).
Related Issue
Closes #96. Part of Phase 4 (test infrastructure) of #120.
Motivation and Context
This is the highest-value item remaining in the unblocked test backlog: it is not just missing coverage, it is a build gate that has been silently inert for every consumer. The three PlatyPS test issues (#99/#100/#101) are deliberately deferred to Phase 2 so they can be written against the new
Microsoft.PowerShell.PlatyPSAPI rather than rewritten later.Worth noting: this repository's own
Analyzetask in the rootpsakeFile.ps1callsInvoke-ScriptAnalyzerdirectly and gets$_.Severityright, which is why the typo in the shipped function went unnoticed and why this fix has no effect on this repository's own build gate.How Has This Been Tested?
instructions/testing.instructions.md: the tests were written and run against the unfixed module first, producing 9 failures for exactly the two diagnosed causes — 8 × "Expected an exception to be thrown, but no exception was thrown" (dead severity gate) and 1 × "Cannot find the path" (empty-Settings). The fix turns all 31 green../build.ps1 -Task Teston PowerShell 7.6 / Linux: 421 passed, 0 failed, 15 skipped (up from 391 before this branch).no findings + no threshold suppliedcase is covered end-to-end rather than with a mock, because the real analyzer returns$nullinstead of an empty collection — a state a mocked@()cannot reproduce. CI's Windows PowerShell 5.1 job exercises that path on the Desktop engine.Checklist
Breaking Changes
No API change, but a real behavioral one: a consumer build that passed on 0.8.x can now correctly fail. The findings were always being reported; they were simply never enforced. A migration-guide entry documents detection and the three options (fix the findings, exclude rules via the settings file, or set
FailBuildOnSeverityLevel = 'None').This is a judgment call worth a reviewer's opinion: it is classified as a bug fix, but it is listed in
docs/migration-v0.8-to-v1.0.mdbecause it can turn a green build red on upgrade with no other change. Happy to drop it to a changelog-only entry if the guide should stay strictly for intentional API breaks.Out of scope
build.properties.ps1documents an'Any'value forFailBuildOnSeverityLevel, but the parameter'sValidateSetaccepts onlyNone/Error/Warning/Information, so passing'Any'fails parameter binding. Resolving that means either widening the public parameter contract or correcting the documentation — a design decision rather than a bug fix, so it is left for a separate issue.🤖 Generated with Claude Code
https://claude.ai/code/session_01BxrhSpD47TSrS9hEJpDMaH
Generated by Claude Code