refactor(scripts): align linting and tests with CIHelpers #401
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors linting and security PowerShell scripts to standardize CI integration via the shared CIHelpers module, and updates tests/mocks/docs to match the new CI helper APIs and behaviors.
Changes:
- Expanded
CIHelpers.psm1with new helpers (notably CI env-var support and bulk annotation emission) and added/updated Pester coverage. - Updated linting and security scripts to use CI helper outputs/annotations and introduced skip-main patterns to make scripts more testable.
- Updated Pester tests and mocks to use CI-focused environment helpers instead of GitHub-specific helpers, and refreshed linting documentation.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/tests/security/Update-ActionSHAPinning.Tests.ps1 | Switches tests to dot-source the script with HVE_SKIP_MAIN and uses CI environment mocks. |
| scripts/tests/security/Test-SHAStaleness.Tests.ps1 | Switches tests to dot-source the script with HVE_SKIP_MAIN and uses CI environment mocks. |
| scripts/tests/linting/Validate-MarkdownFrontmatter.Tests.ps1 | Updates tests to CI naming and validates CI annotation integration paths. |
| scripts/tests/linting/Markdown-Link-Check.Tests.ps1 | Updates tests to dot-source with HVE_SKIP_MAIN for function access without executing main. |
| scripts/tests/linting/LintingHelpers.Tests.ps1 | Removes GitHub-helper-focused coverage and narrows tests to core linting helper behaviors. |
| scripts/tests/linting/Link-Lang-Check.Tests.ps1 | Updates tests to dot-source with HVE_SKIP_MAIN for function access without executing main. |
| scripts/tests/linting/Invoke-YamlLint.Tests.ps1 | Updates mocks/assertions from GitHub helpers to CI helpers (Set-CIOutput, Write-CIAnnotation, etc.). |
| scripts/tests/linting/Invoke-PSScriptAnalyzer.Tests.ps1 | Updates mocks/assertions from GitHub helpers to CI helpers and CI integration naming. |
| scripts/tests/linting/Invoke-LinkLanguageCheck.Tests.ps1 | Adds unit coverage for the new core flow and updates CI helper export expectations. |
| scripts/tests/linting/FrontmatterValidation.Tests.ps1 | Updates tests to validate Write-CIAnnotations behavior. |
| scripts/tests/lib/CIHelpers.Tests.ps1 | Adds/updates coverage for new CI helper APIs (including env var helper and bulk annotation helper). |
| scripts/tests/Mocks/GitMocks.psm1 | Renames and generalizes mock helpers from GitHub-specific to CI-oriented environment helpers. |
| scripts/security/Update-ActionSHAPinning.ps1 | Improves response parsing/error handling and adds HVE_SKIP_MAIN to prevent executing main during tests. |
| scripts/security/Test-SHAStaleness.ps1 | Adds HVE_SKIP_MAIN guard around main execution for testability. |
| scripts/linting/Validate-MarkdownFrontmatter.ps1 | Replaces GitHub-only checks with Test-CIEnvironment and CI helper annotation/summary/env helpers. |
| scripts/linting/README.md | Updates linting documentation to describe CI helper integration and exported helper APIs. |
| scripts/linting/Modules/LintingHelpers.psm1 | Removes GitHub-specific helper implementations and aligns module surface toward CI helpers + file/git helpers. |
| scripts/linting/Modules/FrontmatterValidation.psm1 | Removes GitHub-specific annotation helper implementation from the module. |
| scripts/linting/Markdown-Link-Check.ps1 | Adds HVE_SKIP_MAIN and replaces GitHub helper usage with CI helper annotations/summary/env helpers. |
| scripts/linting/Link-Lang-Check.ps1 | Adds HVE_SKIP_MAIN and replaces GitHub-only failure annotation with CI helper annotation. |
| scripts/linting/Invoke-YamlLint.ps1 | Replaces GitHub helper outputs/annotations/summary/env with CI helpers. |
| scripts/linting/Invoke-PSScriptAnalyzer.ps1 | Replaces GitHub helper outputs/annotations/summary/env with CI helpers and maps severities to CI levels. |
| scripts/linting/Invoke-LinkLanguageCheck.ps1 | Refactors into a testable core function and switches annotations/outputs/env/summary to CI helpers. |
| scripts/lib/Modules/CIHelpers.psm1 | Adds CI env var helper and bulk annotation helper; expands exported CI integration surface. |
Comments suppressed due to low confidence (1)
scripts/security/Test-SHAStaleness.ps1:83
- Even when
$env:HVE_SKIP_MAINis set (tests dot-source this script), the script still creates the log directory at import time. This is a side effect outside the guarded main block and can cause tests to write to the repo filesystem unexpectedly. Consider moving the log directory creation behind the skip-main guard (or lazily creating it insideWrite-SecurityLog) so loading functions for tests is side-effect free.
$script:SkipMain = $env:HVE_SKIP_MAIN -eq '1'
# Ensure logging directory exists
$LogDir = Split-Path -Parent $LogPath
if (!(Test-Path $LogDir)) {
New-Item -ItemType Directory -Path $LogDir -Force | Out-Null
}
- replace GitHub Actions helpers across linting scripts - extend CIHelpers for env and annotations with tests - update linting and security test mocks for CI integration - document CIHelpers usage in linting README 🔧 - Generated by Copilot
- Standardize SPDX-License-Identifier headers across 7 files - Fix HVE_SKIP_MAIN handling in integration test contexts - Add ErrorAction Continue to prevent terminating errors in catch blocks - Remove legacy Created date headers from script files 🔧 - Generated by Copilot Co-authored-by: littleKitchen <55852668+littleKitchen@users.noreply.github.com>
WilliamBerryiii
force-pushed
the
refactor/351-linting-cihelpers
branch
from
fbd8cc4 to
d680c7d
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #401 +/- ##
===========================================
+ Coverage 60.96% 74.53% +13.57%
===========================================
Files 19 19
Lines 3233 3295 +62
===========================================
+ Hits 1971 2456 +485
+ Misses 1262 839 -423
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@littleKitchen - This PR is ready for review! It incorporates the learnings from your work on PR #379 and implements the script-as-orchestrator pattern we discussed. Changes in latest commits:
Would appreciate your eyes on it when you have a chance. 🙏 |
- sanitize file paths in step summaries for AzDO injection prevention - move Set-StrictMode inside SkipMain guard to prevent test scope leak - add ExcludePaths param to mock scripts in Link-Lang-Check tests - update README documentation for LintingHelpers module exports 🔒 - Generated by Copilot
| GeneratedAt = Get-Date -Format "yyyy-MM-dd HH:mm:ss UTC" | ||
| Summary = @{ | ||
| TotalWorkflows = @($Results).Count | ||
| WorkflowsChanged = @($Results | Where-Object { $_.PSObject.Properties.Name -contains 'ContentChanged' -and $_.ContentChanged }).Count |
There was a problem hiding this comment.
WorkflowsChanged is computed via Where-Object { $_.PSObject.Properties.Name -contains 'ContentChanged' -and $_.ContentChanged }. Since workflow results are hashtables, the PSObject.Properties check can miss keys and undercount changed workflows. Make this hashtable-safe (for example, branch on -is [hashtable] and use ContainsKey('ContentChanged')/indexing), matching the approach used for the action count sums above.
| WorkflowsChanged = @($Results | Where-Object { $_.PSObject.Properties.Name -contains 'ContentChanged' -and $_.ContentChanged }).Count | |
| WorkflowsChanged = @( | |
| $Results | Where-Object { | |
| if ($_ -is [hashtable]) { | |
| $_.ContainsKey('ContentChanged') -and $null -ne $_['ContentChanged'] -and [bool]$_['ContentChanged'] | |
| } | |
| else { | |
| $_.PSObject.Properties.Name -contains 'ContentChanged' -and $null -ne $_.ContentChanged -and [bool]$_.ContentChanged | |
| } | |
| } | |
| ).Count |
| foreach ($link in $brokenLinks) { | ||
| $summaryContent += "`n| ``$($link.File)`` | ``$($link.Link)`` |" | ||
| $safeFile = if ((Get-CIPlatform) -eq 'azdo') { | ||
| ConvertTo-AzureDevOpsEscaped -Value $link.File | ||
| } else { $link.File } | ||
| $safeLink = if ((Get-CIPlatform) -eq 'azdo') { | ||
| ConvertTo-AzureDevOpsEscaped -Value $link.Link | ||
| } else { $link.Link } | ||
| $summaryContent += "`n| ``$safeFile`` | ``$safeLink`` |" |
There was a problem hiding this comment.
Get-CIPlatform is called twice per broken link when building the step summary table. Since this loop can run for many links, compute the platform once before the loop and reuse it (and only call ConvertTo-AzureDevOpsEscaped when needed) to avoid repeated platform detection work.
| $script:SkipMain = $env:HVE_SKIP_MAIN -eq '1' | ||
|
|
There was a problem hiding this comment.
$script:SkipMain is intended to allow dot-sourcing for tests without side effects, but the script still performs script-level I/O after this point (for example, ensuring the log directory exists) even when HVE_SKIP_MAIN=1. Consider extending the skip-main guard to cover those script-level side effects as well, so importing the functions does not create/modify files during unit tests.
| * **Core Scripts** - Existing validation logic (e.g., `Link-Lang-Check.ps1`, `Validate-MarkdownFrontmatter.ps1`) | ||
| * **Shared Module** (`Modules/LintingHelpers.psm1`) - Common functions for GitHub Actions integration | ||
| * **Shared Module** (`Modules/LintingHelpers.psm1`) - Common functions for file discovery and git operations | ||
| * **CI Helpers** (`scripts/lib/Modules/CIHelpers.psm1`) - CI annotations, outputs, env flags, and step summaries | ||
| * **Configuration Files** - Tool-specific settings (e.g., `PSScriptAnalyzer.psd1`, `markdown-link-check.config.json`) |
There was a problem hiding this comment.
PR description mentions that the Copilot setup workflow was removed, but .github/workflows/copilot-setup-steps.yml is still present in this change set. Either update the PR description to reflect the current state, or remove/disable the workflow if that was the intent.
| Describe 'Write-GitHubAnnotations' -Tag 'Unit' { | ||
| Describe 'Write-CIAnnotations' -Tag 'Unit' { | ||
| BeforeAll { | ||
| Import-Module (Join-Path $PSScriptRoot '../../linting/Modules/FrontmatterValidation.psm1') -Force |
There was a problem hiding this comment.
This Write-CIAnnotations test block calls Write-CIAnnotations, but its BeforeAll only imports FrontmatterValidation.psm1. Unless CIHelpers.psm1 was imported by a previously-run Describe, these tests can fail or become order-dependent (and can break under tag filtering). Import scripts/lib/Modules/CIHelpers.psm1 in this block’s BeforeAll (and optionally remove it in AfterAll) so the tests are self-contained.
| Import-Module (Join-Path $PSScriptRoot '../../linting/Modules/FrontmatterValidation.psm1') -Force | |
| Import-Module (Join-Path $PSScriptRoot '../../linting/Modules/FrontmatterValidation.psm1') -Force | |
| Import-Module (Join-Path $PSScriptRoot '../../lib/Modules/CIHelpers.psm1') -Force |
| if (-not $script:SkipMain) { | ||
| Set-StrictMode -Version Latest | ||
| $ErrorActionPreference = 'Stop' | ||
|
|
||
| try { |
There was a problem hiding this comment.
Inside the main execution block (guarded by $script:SkipMain), the summary still aggregates $results using Measure-Object ... -Sum and PSObject.Properties checks later in the block. Since Update-WorkflowFile returns hashtables, those aggregations are likely to produce incorrect totals. Prefer hashtable-aware aggregation (ContainsKey/indexing) or cast each result to [pscustomobject] before computing totals/counts so CI summaries stay accurate.
| } | ||
| catch { | ||
| Write-SecurityLog "Critical error in SHA pinning process: $($_.Exception.Message)" -Level 'Error' | ||
| if ($env:GITHUB_ACTIONS -eq 'true') { |
There was a problem hiding this comment.
nit: should this one be Write-CIAnnotation now?
| } | ||
| catch { | ||
| Write-Error "Test SHA Staleness failed: $($_.Exception.Message)" | ||
| if ($env:GITHUB_ACTIONS -eq 'true') { |
There was a problem hiding this comment.
nit: Write-CIAnnotation like the others?
Description
Refactored CI helper usage in linting and security scripts, expanded CI helper APIs, and aligned tests and documentation with the new behavior.
Related Issue(s)
Related to #351
Type of Change
Select all that apply:
Code & Documentation:
Infrastructure & Configuration:
AI Artifacts:
prompt-builderagent and addressed all feedback.github/instructions/*.instructions.md).github/prompts/*.prompt.md).github/agents/*.agent.md)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 lint:md-linksnpm run lint:psSecurity Considerations