Skip to content

Expand PSScriptAnalyzer coverage#278

Open
SamErde wants to merge 2 commits intomainfrom
samerde/psscriptanalyzer-coverage
Open

Expand PSScriptAnalyzer coverage#278
SamErde wants to merge 2 commits intomainfrom
samerde/psscriptanalyzer-coverage

Conversation

@SamErde
Copy link
Copy Markdown
Collaborator

@SamErde SamErde commented May 6, 2026

Summary

Expands PSScriptAnalyzer CI coverage from two hand-picked rules to all rules except PSAvoidUsingWriteHost, adds a missing workflow permissions block, and suppresses two confirmed false-positive unused-parameter warnings in the module's public API.


Changes

.github/workflows/powershell.yml

  • Removed includeRule (which ran only PSAvoidGlobalAliases and PSAvoidUsingConvertToSecureStringWithPlainText).
  • Added excludeRule: '"PSAvoidUsingWriteHost"' so all rules run except that one.
  • Write-Host is used intentionally throughout the module for user-facing progress output; suppressing that rule at the workflow level avoids noise without hiding real issues.

.github/workflows/Create External Help.yml

  • Added permissions: contents: read at the workflow level. The workflow previously had no permissions block, meaning it inherited whatever the repository default grants (typically contents: write on public repos with Actions enabled). Explicit contents: read is the minimum this workflow needs (it checks out code but does not push back to the repo).

Private/Invoke-Scans.ps1

  • Added two SuppressMessageAttribute entries for PSReviewUnusedParameter on EnrollmentAgentEKU and PreferredOwner.
  • Both parameters are declared Mandatory and are part of the function's public contract, but are not yet threaded through to any inner Find-* call:
    • EnrollmentAgentEKU — reserved for ESC13 enrollment-agent scan support (the ESC13 case passes $ClientAuthEkus, not the enrollment-agent EKU).
    • PreferredOwner — reserved for planned remediation-ownership integration.
  • Removing these parameters would be a breaking API change; suppression with justification is the correct approach until the parameters are wired up.

Findings triaged but not fixed

Rule Location Decision
PSUseShouldProcessForStateChangingFunctions New-Dictionary False positive — function creates in-memory data structures only, no system state change.
PSUseShouldProcessForStateChangingFunctions Set-RiskRating False positive — sets properties on in-memory PSCustomObjects, not AD/file/system state.
PSUseShouldProcessForStateChangingFunctions Update-ESC*Remediation Out of scope — "do not change remediation execution" constraint.
PSUseShouldProcessForStateChangingFunctions Remove-CommonParameterFromMarkdown Build utility called programmatically; adding ShouldProcess would require updating callers and is outside module scope.
PSReviewUnusedParameter Build-Module.ps1 (PublishToPSGallery, PSGalleryAPIPath) False positive — params are used inside a Build-Module {} scriptblock at lines 149-150; PSSA cannot see usage inside the closure.
PSReviewUnusedParameter Build-Module.ps1 (Mode, Scans, OutputPath) Same as above — used inside PSPublishModule scriptblock.
PSReviewUnusedParameter Find-ESC17.ps1 (Mode) Parameter is referenced only in commented-out WIP code (lines 104-106); intentional placeholder.
PSUseBOMForUnicodeEncodedFile Invoke-Locksmith.ps1 Invoke-Locksmith.ps1 is a generated build artifact compiled from Public/ and Private/ sources. Adding a BOM directly to it would be overwritten by the next build. The fix belongs in the build pipeline (ensure Build-Module.ps1 saves the compiled output with UTF-8 BOM).
PSUseSingularNouns Write-HelpOutDocs.ps1, Write-PlatyPSDocs.ps1 Explicitly out of scope per PR constraints.
PSAvoidTrailingWhitespace Various Style rule — out of scope for this PR.

- powershell.yml: replace includeRule (2 hand-picked rules) with
  excludeRule PSAvoidUsingWriteHost so all other rules run; Write-Host
  is used intentionally throughout for user-facing progress output.

- Create External Help.yml: add workflow-level permissions block
  (contents: read) - workflow had no permissions block, relying on
  permissive repo defaults.

- Private/Invoke-Scans.ps1: suppress PSReviewUnusedParameter for
  EnrollmentAgentEKU and PreferredOwner; both are declared Mandatory as
  part of the public API but are not yet wired through to inner Find-*
  calls (reserved for planned ESC13 and remediation-ownership support).

Findings deferred / not fixed:
- PSUseShouldProcessForStateChangingFunctions: New-Dictionary and
  Set-RiskRating operate on in-memory objects only (false positives);
  Update-ESC* functions are remediation code (out of scope).
- PSReviewUnusedParameter in Build-Module.ps1: params are used inside
  a PSPublishModule scriptblock, causing a PSSA false positive.
- Find-ESC17 Mode param: referenced only in commented-out WIP code.
- PSUseBOMForUnicodeEncodedFile on Invoke-Locksmith.ps1: that file is
  a generated build artifact; the fix belongs in the build pipeline.
- PSAvoidTrailingWhitespace, PSUseSingularNouns: style / out of scope.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SamErde SamErde marked this pull request as ready for review May 6, 2026 09:26
Copilot AI review requested due to automatic review settings May 6, 2026 09:26
@SamErde SamErde marked this pull request as draft May 6, 2026 09:27
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Expands CI linting coverage for the PowerShell codebase by running (nearly) the full PSScriptAnalyzer ruleset in GitHub Actions, tightens workflow token permissions, and adds targeted suppressions for known false-positive unused-parameter reports.

Changes:

  • Updated PSScriptAnalyzer workflow to run all rules except PSAvoidUsingWriteHost.
  • Added explicit contents: read permissions to the “Create External Help” workflow.
  • Suppressed PSReviewUnusedParameter for two intentionally reserved parameters in Invoke-Scans.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
Private/Invoke-Scans.ps1 Adds PSScriptAnalyzer suppressions for two reserved public-contract parameters.
.github/workflows/powershell.yml Expands analyzer coverage by removing includeRule and adding an excludeRule for Write-Host.
.github/workflows/Create External Help.yml Sets explicit minimal GITHUB_TOKEN permissions (contents: read) at the workflow level.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Private/Invoke-Scans.ps1
Comment thread .github/workflows/powershell.yml
… monolith

Copilot review (#278) correctly identified that the SuppressMessageAttribute
entries added to Private/Invoke-Scans.ps1 were not reflected in the generated
Invoke-Locksmith.ps1 monolith. PSScriptAnalyzer scans the whole repository
recursively, so the monolith would still raise the same warnings that the source
suppressions were intended to silence. Applied matching suppressions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SamErde SamErde marked this pull request as ready for review May 6, 2026 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants