Repository navigation
test(#21): strengthen contract tests (conventions + export integrity, AST-driven) - #65
Merged
Merged
Conversation
…one-function-per-file, SEB-prefix, approved verbs, no aliases/Write-Host, export integrity) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughContract tests now share a memoized AST helper, and the parse, remote-script, API, export, and convention suites use it while adding export-wiring, naming, verb, alias, and allow-list checks. ChangesContract test hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Strengthens the Contract test layer by expanding export-integrity checks and adding a new AST-driven conventions contract, so convention/export drift fails CI consistently across all modules/functions.
Changes:
- Extended
Exports.Tests.ps1to enforce reverse-export, root-vs-submodule export-set equality, and$Modules↔Modules/directory integrity. - Added
Conventions.Tests.ps1to enforce CLAUDE.md conventions (help, attributes, naming, verbs, no aliases, noWrite-Host) with documented allow-lists for pre-existing debt.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Tests/Contract/Exports.Tests.ps1 | Adds reverse export and module wiring consistency checks (root exports, submodule exports, and $Modules directory list). |
| Tests/Contract/Conventions.Tests.ps1 | Introduces a centralized, AST-based conventions contract with allow-lists for known legacy exceptions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+8
to
+11
| # This file also enforces the REVERSE direction and the wider export wiring (issue #21): | ||
| # - reverse-export: every root FunctionsToExport entry has a backing Public/*.ps1 file; | ||
| # - the root manifest's export set equals the union of every sub-module manifest's exports; | ||
| # - every module directory on disk is listed in the root SEBackup.psm1 $Modules array. |
Comment on lines
+17
to
+40
| function script:Get-ContractFunctionInfo { | ||
| # Returns one hashtable per function definition found in $Path, with the metadata each | ||
| # convention test needs. TopLevel=$true means the function is at file scope (not nested | ||
| # inside another function) -- used by the one-function-per-file rule. | ||
| param([string]$Path) | ||
| $tokens = $null; $errors = $null | ||
| $ast = [System.Management.Automation.Language.Parser]::ParseFile($Path, [ref]$tokens, [ref]$errors) | ||
| $topLevel = @($ast.FindAll({ param($n) $n -is [System.Management.Automation.Language.FunctionDefinitionAst] }, $false)) | ||
| $all = @($ast.FindAll({ param($n) $n -is [System.Management.Automation.Language.FunctionDefinitionAst] }, $true)) | ||
| foreach ($fn in $all) { | ||
| $attrs = @() | ||
| if ($fn.Body.ParamBlock) { $attrs = @($fn.Body.ParamBlock.Attributes.TypeName.FullName) } | ||
| $help = $fn.GetHelpContent() | ||
| @{ | ||
| Name = $fn.Name | ||
| IsTopLevel = ($topLevel -contains $fn) | ||
| HasCmdletBind = ($attrs -contains 'CmdletBinding') | ||
| HasOutputType = ($attrs -contains 'OutputType') | ||
| HasSynopsis = ($help -and -not [string]::IsNullOrWhiteSpace($help.Synopsis)) | ||
| HasDescription = ($help -and -not [string]::IsNullOrWhiteSpace($help.Description)) | ||
| HasExample = ($help -and $help.Examples -and $help.Examples.Count -gt 0) | ||
| } | ||
| } | ||
| } |
…sentinels; stale-allow-list guard; top-level naming; memoized AST (8-lens) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines
+20
to
+42
| # Initialise the cache once. Guard with Get-Variable so re-dot-sourcing (each container dot-sources | ||
| # this file again) does NOT wipe the accumulated cache. | ||
| if (-not (Get-Variable -Name __ContractAstCache -Scope Script -ErrorAction SilentlyContinue)) { | ||
| $script:__ContractAstCache = @{} | ||
| } | ||
|
|
||
| function Get-ContractAst { | ||
| # Parse $Path once and return its ScriptBlockAst; subsequent calls for the same path return | ||
| # the cached AST. Tokens/parse-errors are cached alongside so callers that need errors (the | ||
| # parse-integrity contract) can read them without re-parsing. | ||
| param([Parameter(Mandatory)][string]$Path) | ||
| $key = [System.IO.Path]::GetFullPath($Path) | ||
| if (-not $script:__ContractAstCache.ContainsKey($key)) { | ||
| $tokens = $null; $errors = $null | ||
| $ast = [System.Management.Automation.Language.Parser]::ParseFile($key, [ref]$tokens, [ref]$errors) | ||
| $script:__ContractAstCache[$key] = [pscustomobject]@{ | ||
| Ast = $ast | ||
| Tokens = $tokens | ||
| Errors = @($errors) | ||
| } | ||
| } | ||
| return $script:__ContractAstCache[$key] | ||
| } |
Comment on lines
21
to
24
| BeforeAll { | ||
| . "$PSScriptRoot/_ContractAst.ps1" # memoized Get-ContractAst (shared parse cache) | ||
| $repoRoot = (Resolve-Path "$PSScriptRoot/../..").Path | ||
| Import-Module "$repoRoot/SEBackup.psd1" -Force -DisableNameChecking 3>$null |
|
|
||
| $tokens = $null; $errors = $null | ||
| $ast = [System.Management.Automation.Language.Parser]::ParseFile($Path, [ref]$tokens, [ref]$errors) | ||
| $ast = (Get-ContractAst -Path $Path).Ast |
Comment on lines
+3
to
+6
| # Convention contract: a single layer that guards the CLAUDE.md authoring rules across the | ||
| # WHOLE codebase, so convention drift is caught once rather than per function. Each rule below | ||
| # corresponds to a section of CLAUDE.md (naming, comment-based help, [CmdletBinding]/[OutputType], | ||
| # one-function-per-file, approved verbs, no aliases, no Write-Host in module code). |
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.
Implements #21 — strengthen the Contract test layer so convention drift fails CI everywhere (one AST/introspection check per rule, data-driven over all modules+functions; each failure names the offending file/function).
New contracts
Tests/Contract/Conventions.Tests.ps1(new): (1) comment-based help (.SYNOPSIS/.DESCRIPTION/.EXAMPLE) on every public function; (2)[CmdletBinding()]+[OutputType()]; (3) one top-level function per file (name == BaseName); (4) SEB-prefix both ways (exported must, private must not); (5) approved verbs (Get-Verb); (6) no aliases inModules/; (7) noWrite-HostinModules/.Exports.Tests.ps1(extended): reverse-export (every export has aPublic/*.ps1), root manifest == union of sub-module exports (both ways), every module dir listed in$Modules.Pre-existing debt — allow-listed (green now, new drift fails)
Like
build.ps1's PSSA baseline. Rules 1, CmdletBinding, exported-prefix, verbs, aliases, and all export-integrity are clean with no allow-list. Three rules had real existing violations, documented as Skip-allow-lists:[OutputType()]missing on 23 public functions (Add-SEBMetric, Invoke-SEBRemoteCommand, Write-SEBLog, …).-SEBprefix on 25 private functions (Test-SEBPreFlight, Protect-SEBSecret, New-SEBDiscordEmbed, …) — contradicts CLAUDE.md.Test-SEBConfig.ps1(3 file-scope validators);Write-Host:Write-SEBLog.ps1(the logger's own console sink).These two debts (OutputType, private-prefix) are worth a follow-up cleanup; allow-listing avoids touching ~48 files in a test PR.
Validation
Contract tests 1486 passed / 49 skipped (the documented exceptions); whole suite 2189 passed;
build.ps1BUILD OK. All 9 rules spot-checked: each turned RED on a planted violation, then reverted.Closes #21.
🤖 Generated with Claude Code
Summary by CodeRabbit