Skip to content

Conversation

@vaind
Copy link
Contributor

@vaind vaind commented Oct 29, 2025

Summary

  • Replaces custom PowerShell script with microsoft/psscriptanalyzer-action@v1.1
  • Integrates PSScriptAnalyzer results with GitHub Security Code Scanning
  • Simplifies workflow maintenance (reduces from ~30 lines to ~15 lines)

Changes

  • Removed manual Invoke-ScriptAnalyzer PowerShell script from lint job
  • Added Microsoft's official PSScriptAnalyzer action with SARIF output
  • Added CodeQL action to upload SARIF results to GitHub Security tab
  • Added required security-events: write permission

Benefits

  • Less maintenance: Official action handles PSScriptAnalyzer invocation and updates
  • Better integration: SARIF output appears in GitHub Security tab for tracking over time
  • Same behavior: Still fails on errors, passes on warnings/info (configurable)
  • Cleaner code: More declarative, less imperative scripting

Testing

The workflow behavior should remain identical - it will:

  • Run PSScriptAnalyzer with custom settings from PSScriptAnalyzerSettings.psd1
  • Fail the build if errors are found
  • Upload results to GitHub Security for visibility

🤖 Generated with Claude Code

vaind and others added 3 commits October 29, 2025 20:12
Replace custom PowerShell script with microsoft/psscriptanalyzer-action
to simplify the lint job and integrate with GitHub Security features.

Benefits:
- Reduces maintenance burden (30+ lines to ~15 lines)
- SARIF output integrates with Code Scanning alerts in Security tab
- Cleaner workflow with official Microsoft-maintained action
- Maintains same behavior (fails on errors, passes on warnings)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Add path filters to trigger CI when shared workflow files or settings
are modified, ensuring changes to test-powershell-module.yml and
PSScriptAnalyzerSettings.psd1 trigger appropriate workflows.

Each workflow now triggers on changes to:
- Its own workflow file
- The reusable test-powershell-module.yml workflow
- The shared PSScriptAnalyzerSettings.psd1 file

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@github-advanced-security
Copy link
Contributor

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@vaind
Copy link
Contributor Author

vaind commented Nov 13, 2025

Still failing, even with the gh action, so let's try and wait if a future gh runner update fixes this flakiness

image

@vaind
Copy link
Contributor Author

vaind commented Dec 12, 2025

On it! We are reviewing the PR and will provide feedback shortly.
Reference ID: 1

2 similar comments
@vaind
Copy link
Contributor Author

vaind commented Dec 12, 2025

On it! We are reviewing the PR and will provide feedback shortly.
Reference ID: 1

@vaind
Copy link
Contributor Author

vaind commented Dec 12, 2025

On it! We are reviewing the PR and will provide feedback shortly.
Reference ID: 1

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

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

- name: Run PSScriptAnalyzer
uses: microsoft/psscriptanalyzer-action@v1.1
with:
path: ${{ inputs.module-path }}
Copy link
Contributor Author

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

uses: microsoft/psscriptanalyzer-action@v1.1
with:
path: ${{ inputs.module-path }}
recurse: true
Copy link
Contributor Author

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

Comment on lines 54 to +58

- 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
if: always()
with:
Copy link
Contributor Author

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

@vaind
Copy link
Contributor Author

vaind commented Dec 12, 2025

On it! We are reviewing the PR and will provide feedback shortly.
Reference ID: 1

}
- name: Upload SARIF results to GitHub Security
uses: github/codeql-action/upload-sarif@v3
if: always()
Copy link
Contributor Author

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

uses: microsoft/psscriptanalyzer-action@v1.1
with:
path: ${{ inputs.module-path }}
recurse: true
Copy link
Contributor Author

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

Comment on lines +55 to +56
- name: Upload SARIF results to GitHub Security
uses: github/codeql-action/upload-sarif@v3
Copy link
Contributor Author

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

Comment on lines 34 to +36
runs-on: ubuntu-latest
if: false # Flaky with: The term 'Get-Command' is not recognized as a name of a cmdlet,
permissions:
contents: read
Copy link
Contributor Author

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

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