Skip to content

ci: replace manual PSScriptAnalyzer with official GitHub Action #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
vaind wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

ci: replace manual PSScriptAnalyzer with official GitHub Action #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96eebce
ci: replace manual PSScriptAnalyzer with official GitHub Action
vaind Oct 29, 2025
71b9426
ci: add path filters for workflow and settings files
vaind Oct 29, 2025
5b16a41
Merge branch 'main' into ci/use-psscriptanalyzer-action
vaind Nov 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/app-runner.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@ on:
branches: ['main']
paths:
- 'app-runner/**'
- '.github/workflows/app-runner.yml'
- '.github/workflows/test-powershell-module.yml'
- 'PSScriptAnalyzerSettings.psd1'
pull_request:
branches: ['main']
paths:
- 'app-runner/**'
- '.github/workflows/app-runner.yml'
- '.github/workflows/test-powershell-module.yml'
- 'PSScriptAnalyzerSettings.psd1'

jobs:
test:
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/sentry-api-client.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@ on:
branches: ['main']
paths:
- 'sentry-api-client/**'
- '.github/workflows/sentry-api-client.yml'
- '.github/workflows/test-powershell-module.yml'
- 'PSScriptAnalyzerSettings.psd1'
pull_request:
branches: ['main']
paths:
- 'sentry-api-client/**'
- '.github/workflows/sentry-api-client.yml'
- '.github/workflows/test-powershell-module.yml'
- 'PSScriptAnalyzerSettings.psd1'

jobs:
test:
Expand Down
38 changes: 16 additions & 22 deletions .github/workflows/test-powershell-module.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ on:
jobs:
lint:
runs-on: ubuntu-latest
if: false # Flaky with: The term 'Get-Command' is not recognized as a name of a cmdlet,
permissions:
contents: read
Comment on lines 34 to +36
Copy link
Contributor Author

@vaind vaind Dec 12, 2025

Choose a reason for hiding this comment

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

The lint job's permissions block grants security-events: write to enable GitHub Security integration for SARIF uploads. However, the parent workflow test-powershell-module.yml has permissions: contents: read at the top level. Verify that the inherited permissions in child jobs called by app-runner.yml and sentry-api-client.yml are correctly merged. The child job's permissions override the parent's, but this should be explicitly documented or confirmed to avoid unexpected permission issues.
Severity: MEDIUM

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: .github/workflows/test-powershell-module.yml#L34-L36

Potential issue: The lint job's `permissions` block grants `security-events: write` to
enable GitHub Security integration for SARIF uploads. However, the parent workflow
`test-powershell-module.yml` has `permissions: contents: read` at the top level. Verify
that the inherited permissions in child jobs called by `app-runner.yml` and
`sentry-api-client.yml` are correctly merged. The child job's permissions override the
parent's, but this should be explicitly documented or confirmed to avoid unexpected
permission issues.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 1

security-events: write
Comment on lines +35 to +37
Copy link
Contributor Author

@vaind vaind Dec 12, 2025

Choose a reason for hiding this comment

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

The permissions block was added at lines 35-37, but it appears the if: false condition removal and permissions addition may have unintended consequences. Removing the if: false condition will now enable the lint job that was previously disabled. Ensure that the new GitHub Action-based approach resolves all the issues that originally caused this job to be disabled (flaky behavior with 'Get-Command' not recognized). Consider adding explicit documentation about why this job was previously disabled and confirming all blocking issues are resolved.
Severity: MEDIUM

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: .github/workflows/test-powershell-module.yml#L35-L37

Potential issue: The `permissions` block was added at lines 35-37, but it appears the
`if: false` condition removal and permissions addition may have unintended consequences.
Removing the `if: false` condition will now enable the lint job that was previously
disabled. Ensure that the new GitHub Action-based approach resolves all the issues that
originally caused this job to be disabled (flaky behavior with 'Get-Command' not
recognized). Consider adding explicit documentation about why this job was previously
disabled and confirming all blocking issues are resolved.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 1

defaults:
run:
working-directory: ${{ inputs.module-path }}
Expand All @@ -42,28 +44,20 @@ jobs:
- name: Checkout repository
uses: actions/checkout@v4

- run: $PSVersionTable

- name: Show PSScriptAnalyzer versions
run: |
Get-Module PSScriptAnalyzer | Format-Table -AutoSize
Get-Command Invoke-ScriptAnalyzer | Format-Table -AutoSize
- name: Run PSScriptAnalyzer
uses: microsoft/psscriptanalyzer-action@v1.1
with:
path: ${{ inputs.module-path }}
Copy link
Contributor Author

@vaind vaind Dec 12, 2025

Choose a reason for hiding this comment

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

The SARIF output file path is hardcoded as results.sarif. This could cause issues if multiple jobs in the same workflow run try to generate SARIF files, as they would overwrite each other. Consider making the SARIF file path unique or context-specific (e.g., using matrix variables if applicable) to prevent file conflicts. Additionally, ensure the file is generated in a location accessible to the upload step (currently appears to be relative to the workflow root).
Severity: MEDIUM

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: .github/workflows/test-powershell-module.yml#L50

Potential issue: The SARIF output file path is hardcoded as `results.sarif`. This could
cause issues if multiple jobs in the same workflow run try to generate SARIF files, as
they would overwrite each other. Consider making the SARIF file path unique or
context-specific (e.g., using matrix variables if applicable) to prevent file conflicts.
Additionally, ensure the file is generated in a location accessible to the upload step
(currently appears to be relative to the workflow root).

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 1

recurse: true
Copy link
Contributor Author

@vaind vaind Dec 12, 2025

Choose a reason for hiding this comment

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

The settings input parameter passes ${{ inputs.settings-path }} directly. However, the original PowerShell script constructed the path as Join-Path ".." "${{ inputs.settings-path }}" to navigate up from the module directory. The new action appears to run from the repository root (based on the path parameter), so you may need to verify that the settings file path resolution is correct. If the action runs from the module directory context, the path handling might differ from the original implementation.
Severity: HIGH

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: .github/workflows/test-powershell-module.yml#L51

Potential issue: The `settings` input parameter passes `${{ inputs.settings-path }}`
directly. However, the original PowerShell script constructed the path as `Join-Path
".." "${{ inputs.settings-path }}"` to navigate up from the module directory. The new
action appears to run from the repository root (based on the path parameter), so you may
need to verify that the settings file path resolution is correct. If the action runs
from the module directory context, the path handling might differ from the original
implementation.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 1

Copy link
Contributor Author

@vaind vaind Dec 12, 2025

Choose a reason for hiding this comment

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

The settings path is now passed directly to the microsoft/psscriptanalyzer-action as ${{ inputs.settings-path }}, which should be relative to the repository root. However, verify that this change is intentional compared to the previous implementation that used Join-Path ".." "${{ inputs.settings-path }}". The new approach assumes the settings file is at the repository root, which is correct given the default value and how app-runner.yml and sentry-api-client.yml define it. Ensure all callers of this workflow are passing the correct repository-root-relative path.
Severity: MEDIUM

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: .github/workflows/test-powershell-module.yml#L51

Potential issue: The settings path is now passed directly to the
`microsoft/psscriptanalyzer-action` as `${{ inputs.settings-path }}`, which should be
relative to the repository root. However, verify that this change is intentional
compared to the previous implementation that used `Join-Path ".." "${{
inputs.settings-path }}"`. The new approach assumes the settings file is at the
repository root, which is correct given the default value and how `app-runner.yml` and
`sentry-api-client.yml` define it. Ensure all callers of this workflow are passing the
correct repository-root-relative path.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 1

output: results.sarif
settings: ${{ inputs.settings-path }}

- name: Run PowerShell Script Analyzer
run: |
Write-Host "Running PSScriptAnalyzer on ${{ inputs.module-name }} module..."
$settingsPath = Join-Path ".." "${{ inputs.settings-path }}"
$analysisResults = Invoke-ScriptAnalyzer -Path . -Recurse -ReportSummary -Settings $settingsPath
if ($analysisResults) {
Write-Host "PSScriptAnalyzer found issues:" -ForegroundColor Yellow
$analysisResults | Format-Table -AutoSize
if ($analysisResults | Where-Object { $_.Severity -eq 'Error' }) {
Write-Error "PSScriptAnalyzer found errors. Please fix them before merging."
exit 1
}
} else {
Write-Host "PSScriptAnalyzer passed with no issues" -ForegroundColor Green
}
- name: Upload SARIF results to GitHub Security
uses: github/codeql-action/upload-sarif@v3
Comment on lines +55 to +56
Copy link
Contributor Author

@vaind vaind Dec 12, 2025

Choose a reason for hiding this comment

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

The if: always() condition on the SARIF upload step may mask issues. If the PSScriptAnalyzer action fails to generate a valid SARIF file or encounters an error, the upload will fail without a clear indication of the root cause. Consider adding error handling to verify the SARIF file exists before attempting the upload, or use if: success() || failure() to distinguish between different failure scenarios.
Severity: MEDIUM

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: .github/workflows/test-powershell-module.yml#L55-L56

Potential issue: The `if: always()` condition on the SARIF upload step may mask issues.
If the PSScriptAnalyzer action fails to generate a valid SARIF file or encounters an
error, the upload will fail without a clear indication of the root cause. Consider
adding error handling to verify the SARIF file exists before attempting the upload, or
use `if: success() || failure()` to distinguish between different failure scenarios.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 1

if: always()
Copy link
Contributor Author

@vaind vaind Dec 12, 2025

Choose a reason for hiding this comment

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

The sarif_file path must account for the working directory context. Since the lint job has defaults.run.working-directory set to ${{ inputs.module-path }}, the SARIF file is generated in that subdirectory. However, the github/codeql-action/upload-sarif action operates in the repository root context, so the path should be relative to the repository root. Change this to ${{ inputs.module-path }}/results.sarif to ensure the file is correctly located and uploaded.
Severity: HIGH

💡 Suggested Fix

Suggested change
if: always()
sarif_file: ${{ inputs.module-path }}/results.sarif

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 1

with:
Comment on lines 54 to +58
Copy link
Contributor Author

@vaind vaind Dec 12, 2025

Choose a reason for hiding this comment

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

The SARIF upload step uses if: always() which is correct for ensuring results are uploaded even if the analysis fails. However, verify that the microsoft/psscriptanalyzer-action properly sets a failure exit code when errors are detected (similar to the original script's exit 1 behavior). If the action doesn't fail the job on errors, the workflow status might incorrectly show as successful despite finding issues. The original code explicitly checked for 'Error' severity and exited with code 1.
Severity: HIGH

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: .github/workflows/test-powershell-module.yml#L54-L58

Potential issue: The SARIF upload step uses `if: always()` which is correct for ensuring
results are uploaded even if the analysis fails. However, verify that the
`microsoft/psscriptanalyzer-action` properly sets a failure exit code when errors are
detected (similar to the original script's `exit 1` behavior). If the action doesn't
fail the job on errors, the workflow status might incorrectly show as successful despite
finding issues. The original code explicitly checked for 'Error' severity and exited
with code 1.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 1

sarif_file: results.sarif
category: psscriptanalyzer

unit-tests:
runs-on: windows-latest
Expand Down
Loading