Command invariants - The structural rules behind #10638 and friends, sized by AST sweeps

[andreasjordan]

The same structural defect classes keep resurfacing one command at a time — the Stop-Function -Continue escape (#10638) alone is 91 sites and ~20 pull requests so far. This issue collects the rules in one place, sizes each with a fresh AST sweep over all of public/ and private/functions/ (run 2026-08-30), and proposes making the enforceable ones a structural test so the classes cannot come back.

The invariants

1. Stop-Function -Continue requires an enclosing loop

91 sites found, 73 resolved, 18 remaining — tracked as #10638.

Without a loop in the same function body, the continue unwinds out of the command and silently consumes an iteration of the caller's loop. -SilentlyContinue escapes even under EnableException. The fix wave is under way (#10636–#10653); the rule is proven AST-checkable.

2. Stop-Function in stop mode must be followed by return

48 candidate sites in 26 files (upper bound, untriaged).

Stop-Function's own help says the caller should follow with return — without EnableException the call returns normally and the code runs on with invalid state (seen live at Invoke-DbaDbLogShipping:707, fixed in #10644). The sweep counts sites where a stop-mode call sits mid-block with no return/throw/continue/break after it and is not the last statement of its block. Some will be intentional warn-and-fall-through, so enforcement needs a one-time triage into fixes plus an allowlist baseline.

3. A begin block that can stop needs the interrupt guard in process

146 commands can stop in begin — 133 guarded (91 %), 13 missing.

When a begin-block Stop-Function returns normally (the non-EnableException path), process still runs — against null or half-built state (found live in #10652). The established convention is if (Test-FunctionInterrupt) { return } as the first statement of process, and the codebase already follows it 133 times, always in first position. The holdouts:

• Export-DbaExecutionPlan
• Get-DbaDbStoredProcedure
• Import-DbaCsv
• Import-DbaParquet
• Invoke-DbaDbAzSqlTip
• Measure-DbaDiskSpaceRequirement
• Remove-DbaXESession
• Reset-DbaAdmin
• Start-DbaXESession
• Stop-DbaXESession
• Test-DbaDiskAlignment
• Get-BackupAncientHistory (private)

Invoke-DbaAdvancedRestore also lacks the guard but is triaged fine: its process block only accumulates pipeline input and its end block is guarded. Reset-DbaAdmin is the most uncomfortable entry — a command that resets sysadmin access proceeding past a failed begin validation. Per-command triage still applies before fixing: a begin stop that is only reachable with EnableException forced needs no guard.

4. end blocks: guard the work, never the cleanup

39 end blocks with statements — 21 guarded, 18 unguarded, and the 18 are correct.

This one earned its nuance from the sweep: every unguarded end block turned out to be cleanup or bookkeeping that should run after a stop — the Copy-Dba* DAC disconnects, Import-DbaCsv's connection disposal, elapsed-time messages. A naive "end must be guarded" AST rule would demand 18 wrong changes. The real rule: an end block performing the command's main work (the Restore-DbaDatabase pattern) starts with the guard; an end block doing cleanup runs unguarded. That distinction needs a human, so this stays policy text.

5. Close every connection you open, in a path errors cannot skip

The #10554 ownership pattern: a command that opens a connection (or clones one via context switch) disconnects it, placed so error paths and -WhatIf cannot skip it — finally, not after the catch. AST can find candidates but not ownership, so this also stays policy plus the existing leak-detection tooling.

6. No continue semantics inside handed-off scriptblocks

Stop-Function -Continue inside a scriptblock passed to ForEach-Object or Invoke-Command binds to whatever loop frame is live at run time. Invoke-DbaPfRelog does this intentionally and now documents it at the site (#10643); anywhere else it is a latent escape. Rule: intentional dynamic binding gets a comment, everything else gets restructured.

Enforcement proposal

The precedent exists: the "dbatools test file structure" Describe in tests\dbatools.Tests.ps1 already enforces repo-wide structure on test files. A sibling "dbatools command structure" Describe would parse every command once and assert:

1. Rule 1 — no loop-less -Continue/-SilentlyContinue (enforceable the day the Stop-Function -Continue without an enclosing loop escapes the command and corrupts the caller #10638 fix wave lands; nested helpers via a small allowlist).
2. Rule 3 — begin-can-stop implies the guard heads process (enforceable after the 12 fixes above; 91 % of the codebase already complies).
3. Rule 2 — mid-block stop without return, against a triaged baseline that shrinks over time.

Rules 4–6 become policy text. Where the policy lives — repo CLAUDE.md, the structural test, or both — is the maintainers' call; text plus AST test is the strongest combination, since the test stops regressions and the text explains why to a contributor whose PR just failed it.

Found along the way

Export-DbaExecutionPlan's process block reads Test-Bound -ParamterName Path — a misspelled parameter name, so the export-directory bootstrap does not run as intended. Worth its own small fix whenever that command is next touched.

Suggested order

1. Finish the Stop-Function -Continue without an enclosing loop escapes the command and corrupts the caller #10638 fix wave (18 sites remaining).
2. Fix the 12 missing process guards — small, mechanical, one PR per command per repo policy.
3. Triage the 48 missing-return candidates into fixes and a baseline.
4. Land the "dbatools command structure" Describe enforcing rules 1–3.
5. Add rules 4–6 as policy text wherever the maintainers want them.

Sweeps run against the current development tree; sizes are upper bounds pending per-command triage.

This proposal was created by Claude and reviewed by Andreas Jordan.

[andreasjordan]

Rule 2 triage: all 48 candidate sites are intentional — recommend narrowing the rule

Every one of the 48 "Stop-Function in stop mode without a following return" candidates has been read in context. None is a defect; they fall into five designed patterns:

1. Warn and emit the failure row (29 sites: the Remove-Dba* family with its $output.Status/IsRemoved objects — 20 sites — and Copy-DbaSsisCatalog's migration-status objects — 9 sites). The catch calls Stop-Function to warn, records the failure on the output object, and the next statement emits that object. The fall-through is the feature: without EnableException the user gets a warning plus a failure row; with it, Stop-Function throws first and no row is emitted.
2. Note and proceed (5 sites: Invoke-DbaAdvancedInstall 254/320/379, Invoke-DbaAdvancedUpdate 216/251). Failure notes are appended to the output object; two of the five even have return $output one statement after the flagged line — the sweep only inspected the immediately-next statement.
3. Cleanup, then flow control (5 sites: Save-DbaCommunitySoftware 369/433/441/458/466). A continue is present in every one of these blocks — it just comes after the Remove-Item cleanup lines, so the next-statement heuristic missed it.
4. Failed-flag gating (6 sites: Rename-DbaDatabase 387/442/522/580/693/756). $failed = $true follows each stop, and every later rename region is gated with if (!$failed -and ...). The one site without a break (387) is correct — no loop encloses it.
5. Miscellaneous, verified fine (3 sites): Reset-DbaAdmin:164 is a bool-returning helper with a finally; Invoke-DbatoolsFormatter 52/60 add an install-hint Write-Message after the stop, and that command's process block carries the interrupt guard.

Suggested consequence for this proposal: rule 2 is too strict as stated. The current tree contains zero true positives (the one that motivated the rule, Invoke-DbaDbLogShipping:707, was already fixed in #10644). Instead of enforcing "must be followed by return" with a 48-entry baseline, document patterns 1–4 as accepted in the policy text and drop rule 2 from the AST enforcement list — or narrow it to "no flow control and no output emission anywhere in the remainder of the block", which today flags nothing.

This triage was created by Claude and reviewed by Andreas Jordan.