From 58c0af1b44d8cb39f9fcad559ba7d009c6645ed1 Mon Sep 17 00:00:00 2001 From: Andreas Jordan Date: Sun, 27 Sep 2026 13:45:34 +0200 Subject: [PATCH 1/5] Tests - Enforce the Stop-Function flow-control invariants over all commands Adds the "command structure" Describe to dbatools.Tests.ps1 (Compliance tag, no SQL Server needed), as agreed in #10655: - rule 1: a Stop-Function that can continue (-Continue, or -SilentlyContinue under EnableException) has a loop or switch in the same function or scriptblock, and -ContinueLabel names one of them - rule 3: a begin block that can set the command's interrupt flag is followed by if (Test-FunctionInterrupt) { return } as the first statement of process - every Stop-Function parameter binds (names, aliases, unique prefixes) - every command file parses The analyzer is tests\dbatools.CommandStructure.ps1; the "command structure analyzer" Describe covers it with positive and negative fixtures. The 23 intentional dynamic-continue sites are listed as exceptions with their reason and exact site count. The invariants and the guidance for rules 2, 4, 5 and 6 are in CLAUDE.md, linked from tests\CLAUDE.md. (do Stop-Function) Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 14 + tests/CLAUDE.md | 12 + tests/dbatools.CommandStructure.ps1 | 375 ++++++++++++++++++++ tests/dbatools.Tests.ps1 | 513 ++++++++++++++++++++++++++++ 4 files changed, 914 insertions(+) create mode 100644 tests/dbatools.CommandStructure.ps1 diff --git a/CLAUDE.md b/CLAUDE.md index e5f96ea3851..7d8e7cc0b31 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. 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: @@ -277,6 +290,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/tests/CLAUDE.md b/tests/CLAUDE.md index 840e4ea05b4..9f4f8661762 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 00000000000..95569631a1b --- /dev/null +++ b/tests/dbatools.CommandStructure.ps1 @@ -0,0 +1,375 @@ +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 switch whose value cannot be read statically (-Continue:$variable, a splat that is not a + hashtable literal in the same function) counts as set. 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 "