CONTRIBUTING: Simplify error handling guidelines - use Write-Error

[Copilot]

• [x] Update AI instructions: clarify return only needed for non-terminating Write-Error
• [x] Update CONTRIBUTING.md: clarify return usage and remove it from -ErrorAction Stop example
• [x] Fix markdown table formatting: remove leading/trailing pipes to satisfy MD055
• [x] All code review feedback addressed

Original prompt

---

This section details on the original issue you should resolve

<issue_title>CONTRIBUTING: Update information around $PSCmdlet.ThrowTerminatingError</issue_title> <issue_description>The documentation should be updated according to this: https://github.com/dsccommunity/SqlServerDsc/pull/1966#issuecomment-1703776151

Also reference: https://stackoverflow.com/questions/49204918/difference-between-throw-and-pscmdlet-throwterminatingerror

https://github.com/dsccommunity/SqlServerDsc/blob/3c3092976409645d74f58707331f66ffe1967127/CONTRIBUTING.md?plain=1#L368-L379

I caught this today where I call a public command (or function) from a another public command. Example below.

function Get-Something
{
    [CmdletBinding()]
    param ()

    $PSCmdlet.ThrowTerminatingError(
        [System.Management.Automation.ErrorRecord]::new(
            'Error message',
            'CODE',
            [System.Management.Automation.ErrorCategory]::InvalidOperation,
            'MyObject'
        )
    )

    "Get-Something exiting" # CORRECT: Does not hit this
}

function Start-Something
{
    [CmdletBinding()]
    param ()

    Get-Something -Name $null -ErrorAction 'Stop'

    "Started" # BUG: This line is executed even though Get-Something throw an exception
}

# This hits the bug in Start-Something
Start-Something

# The user must add -ErrorAction 'Stop' to Start-Something to avoid the bug which is not intuitive
Start-Something -ErrorAction 'Stop'

Similar but using Write-Error (non-terminating error):

function Get-Something
{
    [CmdletBinding()]
    param ()

    Write-Error -Message 'Error message' -Category InvalidOperation -TargetObject 'MyObject' -ErrorId 'CODE'

    "Get-Something exiting" # CORRECT: Does not hit this
}

function Start-Something
{
    [CmdletBinding()]
    param ()

    Get-Something -Name $null -ErrorAction 'Stop'

    "Started" # CORRECT: Does not hit this
}

# This works as expected, stops after the exception in Get-Something
Start-Something
```</issue_description>

<agent_instructions>Read the issue and comment, follow the links and read those comments and links. Based on that make a conclusion about how to best use different ways of throwing a terminating and non-terminating error in public commands and update CONTRIBUTING.md. We should never use `throw` in public commands though, so that is not in scope. My basic understanding is that we should use `Write-Error` whenever possible, but should we limit or entirely not use `$PSCmdlet.ThrowTerminatingError()`? What scenarios should we use each in, and if possible give examples of scenarios where each option fail and where they succeed. We have scenarios like commands accepting pipeline input (that process several items at a time over pipeline), not accepting pipeline input, commands that should never throw a terminating error by default but a non-terminating error and commands that in certain code paths must throw a terminating error.</agent_instructions>

## Comments on the Issue (you are @copilot in this section)

<comments>
</comments>

• Fixes dsccommunity/SqlServerDsc#2045

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

---

This change is 

[coderabbitai[bot]]

[!IMPORTANT]

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

[!NOTE]

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

This PR updates PowerShell community style guidelines and contribution documentation to standardize error handling practices in public commands, replacing prior guidance with mandatory Write-Error usage instead of $PSCmdlet.ThrowTerminatingError(), and corrects a typo in documented command names.

Changes

Cohort / File(s)	Summary
Error Handling Guidelines .github/instructions/dsc-community-style-guidelines-powershell.instructions.md, CONTRIBUTING.md	Replaces error handling and ShouldProcess guidance with comprehensive rules requiring Write-Error for all public command errors, prohibiting $PSCmdlet.ThrowTerminatingError() and throw statements in public commands (except parameter validation), and specifying mandatory error record structure with -Message, -Category, -ErrorId, and -TargetObject.
Documentation Updates CHANGELOG.md	Updates changelog to reference the new error handling guidelines and issue #2193.
Typo Corrections CONTRIBUTING.md	Fixes misspelled command name New-InvalidDataExcpetion to New-InvalidDataException and corrects additional typographical errors in documentation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Areas requiring attention:
  • Verify the new Write-Error guidelines align with PowerShell best practices and the linked issues' requirements
  • Review the distinction between terminating and non-terminating error handling patterns with respect to -ErrorAction behavior
  • Confirm that the prohibition on $PSCmdlet.ThrowTerminatingError() in public commands is intentional and consistent across guidance

Possibly related PRs

• #2189: Implements $PSCmdlet.ThrowTerminatingError in a public cmdlet, which now conflicts with the updated style guidelines prohibiting this pattern in public commands.
• #2128: Modifies the same PowerShell style guidelines file with overlapping error-handling guidance updates.
• #2146: Adjusts error-handling practices in the same style guidelines document related to -ErrorAction usage patterns.

Pre-merge checks

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately captures the main change: simplifying error handling guidelines to recommend using Write-Error instead of ThrowTerminatingError in public commands.
Description check	✅ Passed	The description comprehensively explains the problem, solution, and testing validation, providing clear context for the error handling simplification and changes across documentation files.
Linked Issues check	✅ Passed	The PR addresses both linked issues by simplifying error handling documentation and recommending Write-Error with -ErrorAction 'Stop' for terminating errors, directly resolving the core concern raised in #2045 about ThrowTerminatingError propagation behavior.
Out of Scope Changes check	✅ Passed	All changes are scoped to error handling documentation and related AI instructions; there are no code changes to actual commands or unrelated modifications, and the scope remains focused on the linked issues.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[johlju]

@copilot there are more scenarios that need to be documented:

1. There is an exception to not use throw , the exception is when a [ValidateScript()]on a parameter in a private function or public command has to throw an error, thenthrow` must be used.
2. There is scenario in the codebase that set ErrorActionPreference = 'Stop' prior to calling a command with -ErrorAction 'Stop' to get the parent caller to stop on terminating error (and reverting ErrorActionPreference after the call). See the repository AI instructions that also mentioned this (which might be an error in the AI instructions) Is that an issue because $PSCmdLet.ThrowTerminationError() is used in the called command, or is there a scenarion where this is required. Analyze the codebase to see a pattern and if that pattern is required.

[Copilot]

@copilot there are more scenarios that need to be documented:

1. There is an exception to not use throw , the exception is when a [ValidateScript()]` on a parameter in a private function or publi...

Both scenarios are now documented in commit e24f11c:

1. ValidateScript exception: Added documentation showing that throw is the only valid option within [ValidateScript()] parameter validation attributes.

2. ErrorActionPreference pattern: After testing, I found that setting $ErrorActionPreference = 'Stop' is NOT needed in most cases:

   • ✅ .NET method exceptions (SMO methods like .Drop()) are always caught automatically
   • ✅ Use -ErrorAction 'Stop' on individual cmdlets (don't also set ErrorActionPreference - it's redundant)
   • ✅ Only set $ErrorActionPreference = 'Stop' for blanket error handling across multiple cmdlets without adding -ErrorAction to each

The AI instruction line 85 is incorrect and should be updated. The documentation now clarifies when each pattern is appropriate.

[johlju]

@copilot In the example shows how a state-changing command is using $PSCmdlet.ThrowTerminatingError(), is there a possible solution for where a user does not need to pass -ErrorAction 'Stop' when calling a state-changing public command and it still throws a terminating error?

[Copilot]

@copilot In the example shows how a state-changing command is using $PSCmdlet.ThrowTerminatingError(), is there a possible solution for where a user does not need to pass -ErrorAction 'Stop' when ...

No, there's no solution with $PSCmdlet.ThrowTerminatingError() that avoids requiring the caller to use -ErrorAction 'Stop' or set $ErrorActionPreference = 'Stop'. This is the inherent behavior of ThrowTerminatingError - it's command-terminating, not script-terminating.

Alternatives:

1. Use Write-Error instead (recommended): Makes errors non-terminating by default, following standard PowerShell patterns. Callers control termination via -ErrorAction.

2. Use throw: Creates truly terminating errors but produces poor error messages (documented as not allowed in public commands).

3. Accept the current pattern: For state-changing commands where operation failures must terminate, requiring callers to use -ErrorAction 'Stop' or set $ErrorActionPreference = 'Stop' is acceptable and documented.

The recommendation is to use Write-Error for public commands unless the command is specifically designed to always fail on errors (like assert-style commands), in which case ThrowTerminatingError with documented caller requirements is acceptable.

[johlju]

@copilot You say to use Write-Error for public commands unless the command is specifically designed to always fail on errors like assert-style commands. But let say we have this scenarion:

1. We invoke Set-Property (without using ErrorAction parameter)
2. Set-Property internally invokes Assert-ElevatedUser -ErrorAction 'Stop'
3. Assert-ElevatedUser uses $PSCmdlet.ThrowTerminatingError() to throw terminating error that current user is not elevated
4. Set-Property should stop executing and not continue after Assert-ElevatedUser

Does this scenario work as I expect it to?

[Copilot]

@copilot You say to use Write-Error for public commands unless the command is specifically designed to always fail on errors like assert-style commands. But let say we have this scenarion:

1. We invok...

NO, the scenario does NOT work as expected with just -ErrorAction 'Stop' on the Assert call.

When Assert-ElevatedUser uses $PSCmdlet.ThrowTerminatingError(), it creates a command-terminating error (stops Assert-ElevatedUser), but Set-Property will continue executing after the Assert call.

Solutions that DO work:

1. Set $ErrorActionPreference = 'Stop' in Set-Property:

   function Set-Property {
       $ErrorActionPreference = 'Stop'
       Assert-ElevatedUser  # No -ErrorAction needed
       # Stops here if Assert fails
   }
   

2. Use try-catch in Set-Property:

   function Set-Property {
       try {
           Assert-ElevatedUser -ErrorAction 'Stop'
           # Continues only if Assert succeeds
       } catch {
           # Handle or re-throw error
       }
   }
   

3. Change Assert-ElevatedUser to use Write-Error instead (but this changes the assert semantics - it becomes non-terminating by default).

The -ErrorAction 'Stop' parameter on the Assert call alone is insufficient because ThrowTerminatingError doesn't propagate as a script-terminating error to the caller.

[johlju]

@copilot But the line Set-Property: After Assert-ElevatedUser (should NOT show if assertion failed) was output even when it should have?

[johlju]

@copilot You found that "The -ErrorAction 'Stop' parameter on the Assert call alone is insufficient because ThrowTerminatingError doesn't propagate as a script-terminating error to the caller.", have we documented that correctly?

[Copilot]

@copilot You found that "The -ErrorAction 'Stop' parameter on the Assert call alone is insufficient because ThrowTerminatingError doesn't propagate as a script-terminating error to the caller.", have ...

The documentation is now corrected in commit 78ecf3a4.

Key corrections:

1. Line 422-426: Now explicitly states that -ErrorAction 'Stop' alone is NOT sufficient - callers must ALSO set $ErrorActionPreference = 'Stop' OR wrap in try-catch

2. Summary table (line 605): Changed from misleading "Set $ErrorActionPreference = 'Stop' or use -ErrorAction 'Stop'" to accurate "Set $ErrorActionPreference = 'Stop' in caller OR wrap in try-catch"

3. Added TIP note (lines 508-512): Clarifies that Write-Error is the better alternative because with it, -ErrorAction 'Stop' alone DOES work as expected

4. AI instructions updated: Now correctly emphasizes preferring Write-Error and avoiding $PSCmdlet.ThrowTerminatingError() in public commands

Answer to new requirement: Yes, there IS a valid scenario for setting $ErrorActionPreference = 'Stop' when calling commands - but ONLY if those commands use $PSCmdlet.ThrowTerminatingError(). With Write-Error, -ErrorAction 'Stop' alone is sufficient (standard PowerShell behavior).

[johlju]

@copilot Let say we have this scenarion:

1. We invoke Set-Property (without using ErrorAction parameter)
2. Set-Property internally invokes Assert-ElevatedUser -ErrorAction 'Stop'
3. Assert-ElevatedUser uses Write-Error -Message 'Not elevated' -ErrorAction 'Stop' to throw terminating error that current user is not elevated
4. Set-Property should stop executing and not continue after Assert-ElevatedUser

Does this scenario work as I expect it to? Also, if it works, can this simplify the documentation in CONTRIBUTING.md and the AI instructions?

[Copilot]

@copilot Let say we have this scenarion:

1. We invoke Set-Property (without using ErrorAction parameter)...

YES, the scenario works as expected!

With Write-Error in Assert-ElevatedUser, using -ErrorAction 'Stop' when calling it is sufficient to stop Set-Property from continuing. This is standard PowerShell behavior.

Regarding documentation simplification:

The current documentation is already optimized around this finding. The key messages are:

1. Prefer Write-Error for public commands (including assert-style commands) - with it, -ErrorAction 'Stop' alone works correctly
2. Avoid $PSCmdlet.ThrowTerminatingError() in public commands - it requires callers to set $ErrorActionPreference = 'Stop' OR use try-catch

If assert-style commands use Write-Error instead of ThrowTerminatingError, the complexity disappears - callers just use -ErrorAction 'Stop' and it works as expected.

The documentation already recommends this pattern and explains the issues with ThrowTerminatingError. The only scenario where ThrowTerminatingError is documented as acceptable is for private functions and existing commands where changing to Write-Error would be a breaking change.

[johlju]

@copilot Igonre if there would be breaking changes

[Copilot]

@copilot Igonre if there would be breaking changes

Understood. Documentation updated in commit 586fd94e to remove assert-style commands from acceptable uses of $PSCmdlet.ThrowTerminatingError().

Changes:

• Assert-style commands now recommended to use Write-Error (not ThrowTerminatingError)
• Summary table updated to recommend Write-Error for assert-style commands
• AI instructions updated accordingly

Rationale: With Write-Error, -ErrorAction 'Stop' alone works correctly for callers (standard PowerShell behavior), eliminating the complexity and non-intuitive requirements of ThrowTerminatingError.

[johlju]

@copilot I want you to verify that this scenarion actually works - stops execution with terminating error, take note where and how ErrorAction parameter is used. If it works see if we can simplify the documentation and AI instructions:

function Assert-ElevatedUser {
    [CmdletBinding()]
    param()
    
    # User is not elevated
    Write-Error -Message 'Not elevated' -Category PermissionDenied -ErrorId 'AEU001' -TargetObject 'CurrentUser' -ErrorAction 'Stop'
    return
    "ASSERT-AFTER -  WRONG - EXECUTION WRONGLY CONTINUED"
}

function Set-Property {
    [CmdletBinding()]
    param()
    
    Write-Host "Set-Property: Before Assert" -ForegroundColor Cyan
    Assert-ElevatedUser
    "SET-PROPERTY-AFTER - WRONG - EXECUTION WRONGLY CONTINUED"
}

# Start test
Set-Property

Remove AI instructions around error handling that is contradicting. Same for documentation, remove contradicting documentation to simplify.

[johlju]

@copilot I want you to verify that this scenarion actually works - stops execution with terminating error, take note where and how ErrorAction parameter is used. If it works see if we can simplify the documentation and AI instructions:

function Assert-ElevatedUser {
    [CmdletBinding()]
    param()
    
    # User is not elevated
    Write-Error -Message 'Not elevated' -Category PermissionDenied -ErrorId 'AEU001' -TargetObject 'CurrentUser' -ErrorAction 'Stop'
    return
    "ASSERT-AFTER -  WRONG - EXECUTION WRONGLY CONTINUED"
}

function Set-Property {
    [CmdletBinding()]
    param()
    
    Write-Host "Set-Property: Before Assert" -ForegroundColor Cyan
    Assert-ElevatedUser
    "SET-PROPERTY-AFTER - WRONG - EXECUTION WRONGLY CONTINUED"
}

# Start test
Set-Property

Remove AI instructions around error handling that is contradicting. Same for documentation, remove contradicting documentation to simplify.

[Copilot]

@copilot I want you to verify that this scenarion actually works - stops execution with terminating error, take note where and how ErrorAction parameter is used. If it works see if we can simplify t...

✅ Verified - the scenario works perfectly! Execution stops with terminating error.

Key finding: Write-Error -ErrorAction 'Stop' inside the function creates a terminating error. The caller doesn't need to pass any -ErrorAction parameter.

Documentation simplified in commit b3ad133c:

• Removed all complex patterns and contradictions
• Primary guidance: Use Write-Error with -ErrorAction 'Stop' for terminating errors
• Removed all references to $PSCmdlet.ThrowTerminatingError() from public command guidance
• Simplified summary table from 11 rows to 7 clear rows
• AI instructions updated to match - simple and clear

The documentation now focuses on one simple pattern that works correctly.

@copilot I want you to verify that this scenarion actually works - stops execution with terminating error, take note where and how ErrorAction parameter is used. If it works see if we can simplify t...

✅ Verified - the scenario works perfectly! Execution stops with terminating error.

Key finding: Write-Error -ErrorAction 'Stop' inside the function creates a terminating error. The caller doesn't need to pass any -ErrorAction parameter.

Documentation simplified in commit b3ad133c:

• Removed all complex patterns and contradictions
• Primary guidance: Use Write-Error with -ErrorAction 'Stop' for terminating errors
• Removed all references to $PSCmdlet.ThrowTerminatingError() from public command guidance
• Simplified summary table from 11 rows to 7 clear rows
• AI instructions updated to match - simple and clear

The documentation now focuses on one simple pattern that works correctly.

[johlju]

@copilot I think we can:

1. Remove the section ##### .NET Method Exceptions, it feels like a duplicate wha we already have in the section ##### Exception Handling in Commands
2. When using Write-Error together with -ErrorAction 'Stop' to throw a terminating error then there is no use of having return after it in those insances, right?

[Copilot]

@copilot I think we can:

1. Remove the section ##### .NET Method Exceptions, it feels like a duplicate wha we already have in the section ##### Exception Handling in Commands ...

Both changes made in commit 026f90fe:

1. ✅ Removed duplicate .NET Method Exceptions section - the pattern was already covered in Exception Handling in Commands
2. ✅ Removed unnecessary return after Write-Error -ErrorAction 'Stop' - execution terminates immediately, so return is redundant

Also fixed duplicate closing backticks in the ValidateScript section.

[johlju]

@coderabbitai review

[coderabbitai[bot]]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 94%. Comparing base (a2ec8dc) to head (92489fc).

Additional details and impacted files

@@         Coverage Diff          @@
##           main   #2364   +/-   ##
====================================
  Coverage    94%     94%           
====================================
  Files       166     166           
  Lines      9808    9808           
====================================
  Hits       9249    9249           
  Misses      559     559           

Flag	Coverage Δ
unit	94% <ø> (ø)

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.