diff --git a/CLAUDE.md b/CLAUDE.md index a13660ac64cd..c3bf5742e628 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -105,6 +105,19 @@ $results = New-Object System.Collections.ArrayList $results ``` +## COMMAND INVARIANTS + +Structural rules for every function in `public/` and `private/functions/`. Rules 1 and 3 are enforced by the "command structure" Describe in `tests/dbatools.Tests.ps1`, which reports file, function and line; the rest is guidance a reviewer checks. + +**Stop-Function flow control, in short:** without EnableException, `-Continue` executes `continue` and every other call sets the caller's interrupt flag and returns. With EnableException, `-SilentlyContinue` executes `continue` and every other call sets the flag and throws. The flag is set in the scope that called Stop-Function. + +1. **(enforced) A Stop-Function that can continue needs a local target.** A call with `-Continue` or `-SilentlyContinue` must sit inside a loop or `switch` of the same function or scriptblock, and `-ContinueLabel` must name a label of one of them. Without a local target the `continue` leaves the command and silently skips an item of the caller's loop; with a label that matches nothing it ends the caller's whole script. A loop around a helper definition or a `ForEach-Object` callback is no target. Helpers that deliberately continue their caller's loop are listed as exceptions in the test, with the reason and the exact number of sites. +2. **Stop, then stop working.** When a Stop-Function call abandons the current operation, prevent further work against the invalid state explicitly, usually with `return` (or `continue` in a loop). Falling through is fine when it is the design and keeps the error and output contract: a failure row emitted after the warning, cleanup followed by flow control, a `$failed` flag that gates the rest. +3. **(enforced) A begin block that can stop guards process.** When `begin` can set the command's interrupt flag, the first statement of `process` is `if (Test-FunctionInterrupt) { return }`. A call counts when it runs in the command's own scope: directly in `begin`, or in a scriptblock that is dot-sourced or handed to `ForEach-Object`, `Where-Object`, `.ForEach()` or `.Where()`. `-SilentlyContinue` alone counts, because it sets the flag without EnableException. `-Continue` counts inside a `try` with a `catch` (or under a `trap`) in `begin`, because under EnableException it sets the flag and throws, and the catch lets `begin` go on. Calls in helper functions do not count - their flag lands in the helper. +4. **Guard the work in `end`, never the cleanup.** An `end` block that does the command's main work starts with the guard; one that disconnects or disposes runs unguarded so it also runs after a stop. +5. **Release the temporary connections you own, preserve the ones the caller owns or shares.** Cleanup must be reachable on error and early-exit paths, preferably in `finally` where the resource lifetime allows it. An unguarded `end` block alone is no cleanup guarantee. +6. **No `continue` semantics in handed-off scriptblocks.** A `-Continue` inside a scriptblock passed to `ForEach-Object` or `Invoke-Command` binds to whatever loop is running when it executes. Intentional dynamic binding needs a comment at the site, an entry in the rule 1 exceptions and a behavioral test; everything else gets restructured. + ## COMMENT PRESERVATION REQUIREMENT **ABSOLUTE MANDATE**: ALL COMMENTS MUST BE PRESERVED EXACTLY as they appear in the original code including: @@ -279,6 +292,7 @@ The dbatools.library version used by CI and local development is pinned in **`.g **dbatools Patterns:** - [ ] SMO used first, T-SQL only when appropriate - [ ] Pipeline output emitted immediately +- [ ] Command invariants kept (Stop-Function flow control, process guard, connection cleanup) - [ ] No `-Detailed`/`-Simple` output mode switches - [ ] Command names use singular nouns diff --git a/private/functions/flowcontrol/Test-ElevationRequirement.ps1 b/private/functions/flowcontrol/Test-ElevationRequirement.ps1 index 1c29aa924c36..4f427c2a899c 100644 --- a/private/functions/flowcontrol/Test-ElevationRequirement.ps1 +++ b/private/functions/flowcontrol/Test-ElevationRequirement.ps1 @@ -72,14 +72,16 @@ function Test-ElevationRequirement { } elseif ($PSCmdlet.ParameterSetName -like "Stop") { if ($testResult) { return $testResult } + # Forwards the switches of the caller. Stop-Function only tests them for truth, so an unbound + # switch or an empty label behaves as if it was not passed. The continue deliberately binds to + # the loop of the command that calls this helper (allowlisted in the command structure test). $splatStopFunction = @{ - Message = "Console not elevated, but elevation is required to perform some actions on localhost for this command." + Message = "Console not elevated, but elevation is required to perform some actions on localhost for this command." + Continue = $Continue + ContinueLabel = $ContinueLabel + SilentlyContinue = $SilentlyContinue } - if (Test-Bound "Continue") { $splatStopFunction["Continue"] = $Continue } - if (Test-Bound "ContinueLabel") { $splatStopFunction["ContinueLabel"] = $ContinueLabel } - if (Test-Bound "SilentlyContinue") { $splatStopFunction["SilentlyContinue"] = $SilentlyContinue } - . Stop-Function @splatStopFunction -FunctionName (Get-PSCallStack)[1].Command return $testResult } diff --git a/tests/CLAUDE.md b/tests/CLAUDE.md index 840e4ea05b43..9f4f8661762a 100644 --- a/tests/CLAUDE.md +++ b/tests/CLAUDE.md @@ -489,6 +489,18 @@ Two things keep that skip honest, and both are worth copying: - **Assert the engine first.** The Azure tests begin with an `It` that fails unless `DatabaseEngineEdition` is `SqlDatabase`. Pointed at anything else the rest would pass while proving nothing, because every other engine simply takes the `USE`. - **Assert the message, not just the failure.** Where the command has to refuse, the test also asserts that the error is *not* the raw "USE statement is not supported" - that is what fails if the code ever reaches `ChangeDatabase` again. +## STRUCTURAL CHECKS OF THE COMMANDS + +The "command structure" Describe in `dbatools.Tests.ps1` enforces rules 1 and 3 of the command invariants in the repository `CLAUDE.md` (COMMAND INVARIANTS), which explains why they exist. It parses `public/` and `private/functions/` without SQL Server, so it runs with the Compliance tag: + +```powershell +Invoke-Pester -Path .\tests\dbatools.Tests.ps1 -TagFilter Compliance +``` + +- The analyzer lives in `dbatools.CommandStructure.ps1`. A change to it needs a positive or negative fixture in the "command structure analyzer" Describe. +- A new rule 1 exception (a helper that deliberately continues its caller's loop) needs the reason in the exception list, a comment at the site and a behavioral test of the calling command. Prefer restructuring the helper. +- The structural checks do not replace behavioral coverage. A fix for a flow-control defect still gets a command-level regression test that fails on the old code, such as a caller loop that counts its iterations. + ## TEST MANAGEMENT GUIDELINES The dbatools test suite must remain manageable in size while ensuring adequate coverage for important functionality. diff --git a/tests/dbatools.CommandStructure.ps1 b/tests/dbatools.CommandStructure.ps1 new file mode 100644 index 000000000000..57910348714b --- /dev/null +++ b/tests/dbatools.CommandStructure.ps1 @@ -0,0 +1,566 @@ +function Get-DbaCommandStructureFinding { + <# + .SYNOPSIS + Static checks for the command invariants in CLAUDE.md that the AST can prove. + + .DESCRIPTION + Used by the "command structure" Describe in dbatools.Tests.ps1, and by its fixtures, which is + why it lives in its own file. It needs no SQL Server and does not load the module. + + Rule 1: a Stop-Function call that can execute continue needs a local target - a loop or a + switch in the same function or scriptblock, carrying the label when -ContinueLabel is used. + Stop-Function continues with -Continue (default mode) and with -SilentlyContinue (under + EnableException), so either switch requires the target. The search for the target stops at a + function or scriptblock boundary: a loop around a helper definition or a callback is not proof, + because the continue then binds to whatever loop is running when it executes. + + Rule 3: when begin can set the command's interrupt flag, process starts with + if (Test-FunctionInterrupt) { return }. Stop-Function sets the flag in its caller's scope + whenever it does not continue, which in the default mode means every call without -Continue, + -SilentlyContinue alone included. A call counts when it runs in the command's own scope: + directly in begin, or in a scriptblock that is dot-sourced or handed to ForEach-Object, + Where-Object, .ForEach() or .Where(). A helper function, & { }, Invoke-Command and .Invoke() + run in a new scope, so the flag they set never reaches Test-FunctionInterrupt. Any other way to + run a scriptblock is reported for review, not assumed safe. A call with -Continue counts too + when a try with a catch, or a trap, in begin encloses it and EnableException is not $false: + Stop-Function then sets the flag and throws, and begin goes on after the catch. The guard counts + only in the exact shape if (Test-FunctionInterrupt) { return }, without arguments or + redirections. + + A switch whose value cannot be read statically (-Continue:$variable, a splat whose value at the + call is not certain) counts as set. A splat is resolved only from a single hashtable literal that + certainly reaches the call and is never changed; see $resolveSplat. Findings are objects with + Rule, File, Function, Line and Message. + + .PARAMETER Ast + The parsed file. + + .PARAMETER File + The name to report the findings under. + + .PARAMETER StopFunctionParameter + The parameters of Stop-Function: every name and alias as a key, the parameter name as its value. + A call parameter is resolved the way PowerShell binds it - name, alias, then unique prefix - and + one that resolves to nothing, or to more than one parameter, is reported, because that call + fails with a parameter binding error. + #> + param( + [Parameter(Mandatory)] + [System.Management.Automation.Language.Ast]$Ast, + [Parameter(Mandatory)] + [string]$File, + [Parameter(Mandatory)] + [hashtable]$StopFunctionParameter + ) + + # A call parameter as the Stop-Function parameter it binds to, or $null. + $resolveParameter = { + param($Name) + if ($StopFunctionParameter.ContainsKey($Name)) { return $StopFunctionParameter[$Name] } + $candidates = @($StopFunctionParameter.Keys | Where-Object { $PSItem.StartsWith($Name, [System.StringComparison]::OrdinalIgnoreCase) } | ForEach-Object { $StopFunctionParameter[$PSItem] } | Select-Object -Unique) + if ($candidates.Count -eq 1) { return $candidates[0] } + return $null + } + + $sameScopeCommands = @("ForEach-Object", "%", "foreach", "Where-Object", "?", "where") + $newScopeCommands = @("Invoke-Command", "icm") + + # The innermost function around a node, or $null for top-level code. + $getFunction = { + param($Node) + $parent = $Node.Parent + while ($parent) { + if ($parent -is [System.Management.Automation.Language.FunctionDefinitionAst]) { return $parent } + $parent = $parent.Parent + } + return $null + } + + $getFunctionName = { + param($Node) + $function = & $getFunction $Node + if ($function) { return $function.Name } + return "