Add git pre-commit hooks for code quality validation#2534
Open
dpaulson45 wants to merge 24 commits intodpaul-Copilotfrom
Open
Add git pre-commit hooks for code quality validation#2534dpaulson45 wants to merge 24 commits intodpaul-Copilotfrom
dpaulson45 wants to merge 24 commits intodpaul-Copilotfrom
Conversation
…nalyzer - pre-commit hook with bash wrapper for Windows compatibility - Test-SensitiveData.ps1: blocks unrecognized email domains and public IPs in test data - Test-HealthCheckerScenarioRunCount.ps1: warns if test files exceed 5 pipeline runs - Test-ScriptAnalyzer.ps1: blocks PSScriptAnalyzer errors on staged files - Install-GitHooks.ps1: one-command setup using git core.hooksPath - Updated CONTRIBUTING.md with Git Hooks setup instructions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an optional repository-managed Git pre-commit hook setup so contributors can run a few local quality checks before committing. It fits into the repo’s existing PowerShell-centric contribution workflow by trying to catch analyzer issues, risky test data, and oversized HealthChecker scenario test files earlier.
Changes:
- Adds a hook installer plus Git hook entrypoints under
.github/GitHooks/. - Introduces three pre-commit validations: sensitive test-data scanning, HealthChecker run-count checking, and ScriptAnalyzer checks.
- Documents the optional hook setup in
CONTRIBUTING.md.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
CONTRIBUTING.md |
Documents optional pre-commit hook setup and what checks it runs. |
.github/Install-GitHooks.ps1 |
Configures core.hooksPath to use the repo-managed hooks directory. |
.github/GitHooks/pre-commit.ps1 |
Main PowerShell pre-commit orchestrator that discovers staged files and runs validators. |
.github/GitHooks/pre-commit |
Bash wrapper that launches the PowerShell pre-commit hook. |
.github/GitHooks/Test-SensitiveData.ps1 |
Scans staged test data files for suspicious email, IP, and credential patterns. |
.github/GitHooks/Test-ScriptAnalyzer.ps1 |
Runs PSScriptAnalyzer against staged PowerShell files during commit. |
.github/GitHooks/Test-HealthCheckerScenarioRunCount.ps1 |
Checks HealthChecker test files for excessive SetDefaultRunOfHealthChecker usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Include renamed files in diff filter (ACMR) - Include .psm1 files in PSScriptAnalyzer check - Require PSScriptAnalyzer >= 1.24 matching CodeFormatter.ps1 - Use repo PSScriptAnalyzerSettings.psd1 and CustomRules.psm1 - Block on all PSScriptAnalyzer violations (warnings and errors) - Remove NotPublished file skip to match repo behavior - Change run count check to advisory (return 0) matching WARN messaging - Add BOM encoding to all hook .ps1 files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Make run count check blocking (return 1) with code-comment bypass hint - Fix credential detection: value-level checks, XML/CLIXML pattern support, word boundary - Expand private IP allowlist: APIPA, CGNAT, TEST-NET, multicast; remove dead IPv6 - Simplify allowedDomainsPattern, remove redundant file read - Add bare domain name scanning with explicit TLD list - Shared reserved TLD skip list for both email and domain checks - Add ExToolsFeedback@microsoft.com to allowed emails - Add fabrikam.com to allowed domains - Context-based exclusions: assemblies, ASP.NET, WMI.NET, msilog paths - Scan all non-script test data files, not just .xml/.config - Remove --no-verify from user-facing output Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add bare domain name scanning with explicit TLD list - Skip domains inside URLs (reference links, not leaked data) - Shared reserved TLD skip list for email and domain checks - Context-based exclusions: assemblies, ASP.NET, WMI.NET, msilog paths - Add Microsft.Net typo to skip pattern (default IIS web.config comment) - Add microsoft.com, outlook.com, live.com to allowed domains - Add ExToolsFeedback@microsoft.com to allowed emails - Add fabrikam.com to allowed domains Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…, ParseError - Add return 0 on success path in Test-HealthCheckerScenarioRunCount.ps1 - Add left anchor to domain regex to prevent suffix bypass (evilcontoso.com) - Use Join-Path for cross-platform path compatibility in pre-commit.ps1 - Add chmod +x for bash wrapper on Linux/macOS in Install-GitHooks.ps1 - Include ParseError in ScriptAnalyzer severity filter Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Capture git config exit code before chmod can overwrite it - Make chmod failure fatal with clear error message - Use PSParser tokenizer for accurate run count (excludes block comments/strings) - Update synopsis and messaging to match blocking behavior (BLOCKED, not WARN) - Skip email domain portions in FQDN check (already caught by email scanner) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Use git -C to target correct repo regardless of CWD in installer - Validate git rev-parse exit code, fail if repo root unavailable - Change Import-Module to -ErrorAction Stop (visible errors vs silent) - Update CONTRIBUTING.md: warns -> blocks for run count check Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
@copilot review |
Addressed: - Test-SensitiveData.ps1: Replace Get-Content -ErrorAction SilentlyContinue with try/catch and -ErrorAction Stop so unreadable/locked files block the commit instead of silently passing (fail closed instead of fail open) - Test-HealthCheckerScenarioRunCount.ps1: Capture PSParser::Tokenize parse errors instead of discarding them via [ref]\, emit warning when parse errors exist so contributors know the run count may be inaccurate - pre-commit.ps1: Add \0 check after git diff --cached to fail early if git is unavailable or not in a repository, instead of silently passing Not addressed (left for author): - Test-ScriptAnalyzer.ps1 settings fallback: Author confirmed 3x this is intentional since PSScriptAnalyzerSettings.psd1 and CustomRules.psm1 are committed files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| $domainsInFile = @{} | ||
|
|
||
| # Patterns defined once outside the per-line loop for performance | ||
| $tldPattern = '(com|net|org|edu|gov|io|info|biz|co|us|uk|de|au|ca|jp|fr|in|eu|cloud|app|dev|lab)' |
Member
Author
There was a problem hiding this comment.
Left for the author — expanding the TLD pattern to cover all public TLDs (e.g., .ru, .cn, .xyz) is an architectural decision. A generic [a-z]{2,63} pattern would significantly broaden detection scope and likely introduce false positives in test data files that contain non-domain dotted strings. The current curated TLD list covers the most common TLDs found in Exchange test data.
Summary of all changes: - Test-SensitiveData.ps1: Use full FQDN for domain reporting instead of extracting last 2 labels (fixes multi-label TLD mis-identification like .co.uk) - Test-ScriptAnalyzer.ps1: Use nested Join-Path with explicit -Path/-ChildPath named parameters for consistency with repo style - Test-HealthCheckerScenarioRunCount.ps1: Wrap Get-Content in try/catch matching Test-SensitiveData.ps1 error handling pattern - pre-commit.ps1: Remove .psd1 from PSScriptAnalyzer filter to match CI behavior - pre-commit.ps1: Add Set-Location to repo root for correct path resolution - CONTRIBUTING.md: Add credential-like values to sensitive data scanning description - CONTRIBUTING.md: Specify -MinimumVersion 1.24 for Install-Module command Original iteration commits: - Iteration 1: FQDN reporting, error handling, Join-Path style, docs updates - Iteration 2: Remove .psd1 from hook filter - Iteration 3: Ensure CWD is repo root for path resolution Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds git pre-commit hooks that validate staged files before commit:
Setup
One-time after cloning:
.github\Install-GitHooks.ps1Uses
git config core.hooksPathso hook updates sync automatically ongit pull.Files
.github/GitHooks/- Hook scripts (bash wrapper + PowerShell).github/Install-GitHooks.ps1- One-command setupCONTRIBUTING.md- Added Git Hooks section