feat(scripts): add Write-CIStepSummary markdown table to Test-SHAStaleness github output#660
Conversation
…eness github output - build markdown summary table with dependency name, SHA age, threshold, and stale/current status in github output branch of Write-OutputResult - invoke Write-CIStepSummary after per-dependency Write-CIAnnotation loop - add 12 Pester tests asserting Write-CIAnnotation call counts and Write-CIStepSummary content
There was a problem hiding this comment.
Pull request overview
This pull request adds GitHub Actions step summary support to the Test-SHAStaleness.ps1 script by implementing Write-CIStepSummary with a markdown table showing dependency staleness results. The change addresses issue #633 where SHA staleness checks produced CI annotations but left the job summary tab empty.
Changes:
- Add markdown table generation in the GitHub output format branch of
Write-OutputResultshowing dependency name, SHA age, threshold, and status - Call
Write-CIStepSummaryafter per-dependency annotations to populate the GitHub Actions job summary - Add 12 Pester tests across two contexts to verify annotation counts and summary content
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| scripts/security/Test-SHAStaleness.ps1 | Adds Write-CIStepSummary call with markdown table for GitHub output format; adds UTF-8 BOM to first line |
| scripts/tests/security/Test-SHAStaleness.Tests.ps1 | Adds two new test contexts with 12 test cases mocking Write-CIAnnotation and Write-CIStepSummary |
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/env pwsh | |||
| #!/usr/bin/env pwsh | |||
There was a problem hiding this comment.
A UTF-8 BOM (Byte Order Mark) character has been added to the first line of the file. This is inconsistent with other security scripts like Test-ActionVersionConsistency.ps1 and Update-ActionSHAPinning.ps1 which do not have a BOM. While PowerShell can handle BOM characters, they are generally unnecessary for UTF-8 files and can cause issues with some tools.
Remove the BOM and save the file as UTF-8 without BOM to maintain consistency with the rest of the codebase.
| #!/usr/bin/env pwsh | |
| #!/usr/bin/env pwsh |
| $staleCount = @($Dependencies | Where-Object { $_.DaysOld -gt $MaxAge }).Count | ||
| $cleanCount = $totalCount - $staleCount |
There was a problem hiding this comment.
The calculation of $staleCount and $cleanCount is incorrect because the $Dependencies array passed to this function only contains dependencies that are already stale (where DaysOld > $MaxAge). Dependencies are filtered before being added to $script:StaleDependencies at line 611. This means $staleCount will always equal $totalCount and $cleanCount will always be 0.
Since the Dependencies array only contains stale items, simplify to:
$staleCount = $totalCount$cleanCount = 0(or remove this variable entirely)
| $staleCount = @($Dependencies | Where-Object { $_.DaysOld -gt $MaxAge }).Count | |
| $cleanCount = $totalCount - $staleCount | |
| $staleCount = $totalCount | |
| $cleanCount = 0 |
| $status = if ($Dep.DaysOld -gt $MaxAge) { '⚠️ Stale' } else { '✅ Current' } | ||
| "| $($Dep.Name) | $($Dep.DaysOld) | $MaxAge | $status |" |
There was a problem hiding this comment.
The conditional check if ($Dep.DaysOld -gt $MaxAge) will always be true because the $Dependencies array only contains dependencies that have already been filtered where DaysOld > $MaxAge (see line 611 where items are added to $script:StaleDependencies). The '✅ Current' status will never be displayed.
Since all dependencies in this array are stale, remove the conditional and always use '
| $script:githubDeps = @( | ||
| @{ Type = 'GitHubAction'; Name = 'actions/checkout'; DaysOld = 45; Severity = 'Low'; File = 'ci.yml'; Message = 'GitHub Action is 45 days old' } | ||
| @{ Type = 'GitHubAction'; Name = 'actions/setup-node'; DaysOld = 90; Severity = 'High'; File = 'build.yml'; Message = 'GitHub Action is 90 days old' } | ||
| ) |
There was a problem hiding this comment.
Both test dependencies have DaysOld values (45 and 90) that exceed the default $MaxAge of 30 days, making them both stale. However, the implementation assumes the $Dependencies array can contain both stale and current items (see lines 710 and 724), which is incorrect. Since the actual $script:StaleDependencies array only contains already-filtered stale items, these test cases validate incorrect logic.
Update the test data to reflect the actual behavior: all items in the array are stale by definition, so remove any assertions that check for mixed stale/current status or clean counts.
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/env pwsh | |||
There was a problem hiding this comment.
Please note you seem to be saving the updated ps1 file with a wrong encoding.
Here's how to fix it in VS code:
Open the file in VS Code, click the encoding in the status bar (bottom right) for that file, select "Save with Encoding", then choose "UTF-8" (not "UTF-8 with BOM").
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #660 +/- ##
==========================================
+ Coverage 86.01% 86.06% +0.05%
==========================================
Files 24 24
Lines 4727 4737 +10
==========================================
+ Hits 4066 4077 +11
+ Misses 661 660 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ged table summary to use "Found" instead of "Scanned" 3) set $status to "Stale" by default
…inputs - remove mixed stale/current assumptions in summary and tests - update Pester assertions to match stale-only dependency behavior
katriendg
left a comment
There was a problem hiding this comment.
Thank you @AhmedMustafa249 for your contribution!
We have a few open comments to address before we can merge this one. Should be minor hopefully.
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/env pwsh | |||
| #!/usr/bin/env pwsh | |||
There was a problem hiding this comment.
could you please resave the file as UTF-* without BOM? Thanks
| } | ||
|
|
||
| It 'Passes markdown containing the summary table header' { | ||
| Mock Write-CIAnnotation { } |
There was a problem hiding this comment.
nit: indentation missing?
| } | ||
|
|
||
| It 'Calls Write-CIAnnotation for each dependency with Warning level' { | ||
| Mock Write-CIAnnotation { } |
There was a problem hiding this comment.
Could we simplify the two repeated calls into the Context section with BeforeEach?
|
|
||
| Context 'GitHub output format with no stale dependencies' { | ||
| It 'Calls Write-CIAnnotation with Notice level for no stale deps' { | ||
| Mock Write-CIAnnotation { } |
There was a problem hiding this comment.
Also here the duplicate calls, can we add the BeforeEach approach to optimize the calls? You can see a few of these approaches already done in other test files.
|
|
||
| # Build step summary markdown table | ||
| $totalCount = @($Dependencies).Count | ||
| $staleCount = $totalCount |
There was a problem hiding this comment.
if $staleCount is always assigned totalCount I believe we can remove staleCount. Or are we missing a value?
Hey @katriendg, thank you for all the comments!🙏 I'll get to resolving everything so we can get this merged asap 🫡 |
…redundant $staleCount variable - Consolidate repeated Mock/Write-OutputResult calls into BeforeEach blocks in the GitHub output format test contexts - Remove the $staleCount variable that was always identical to $totalCount in Write-OutputResult. - Resave scripts/security/Test-SHAStaleness.ps1 as UTF-8
| # 🔒 SHA Staleness Analysis | ||
|
|
||
| **Found:** $totalCount | **Stale:** $staleCount | ||
| **Found:** $totalCount | **Stale:** $totalCount |
There was a problem hiding this comment.
For the Staleness analysis summary here, as per the issue 'Expected Behaviour section' it must show the total deps count and the stale count -- but both come from the same variable, we could omit this for **Stale dependencies found:** which would relay the same message
Pull Request
Description
Related Issue(s)
Fixes #633
Type of Change
Select all that apply:
Code & Documentation:
Infrastructure & Configuration:
Other:
.ps1,.sh,.py)Testing
Checklist
Required Checks
Required Automated Checks
The following validation commands must pass before merging:
npm run lint:mdnpm run spell-checknpm run lint:frontmatternpm run validate:skillsnpm run lint:md-linksnpm run lint:psSecurity Considerations
Additional Notes
Minor findings:
Mixed stale/current test gap: Both test dependencies use DaysOld > MaxAge (45 and 90 vs threshold 30). No test exercises the ✅ Current row status alongside a stale row in the same table. A mixed-scenario test (e.g., DaysOld = 10 + DaysOld = 45) would validate the stale/clean counting logic and both status indicator branches.
Message content not asserted: Tests verify -Level parameter filters but not -Message content (e.g., severity tag [Low], dependency details). Adding -Message ParameterFilter assertions would strengthen coverage.
Potential Follow-up work would include: