PSNativeCommandErrorActionPreference non-zero 0 exit code exception false positive #20034

Activity

1. Thomas Nieto (ThomasNieto)

   added

   Needs-TriageThe issue is new and needs to be triaged by a work group.The issue is new and needs to be triaged by a work group.

   on Jul 28, 2023
2. mklement0 commented on Jul 28, 2023

   

   Michael Klement (mklement0)

   on Jul 28, 2023

   Contributor

   More actions

   As a (largely inconsequential) aside: brew without arguments does result in an error condition: brew considers this invalid syntax, which it signals with exit code 1 and - as a courtesy - outputs its CLI help to stderr. brew -h or brew --help are the expected ways to invoke help (and result in exit code 0).
   In the examples below, I'll use test "$(Get-Item nosuch*)" as a (contrived) example instead; the Unix test utility by design sets its exit code to 1 if the test evaluates to false (which isn't a failure condition).
   A more realistic example is robocopy.exe, which uses several nonzero exit codes for success conditions - see the discussion in #18944.

   An alternative per-call solution - not mutually exclusive with your proposal - is part of the larger Invoke-NativeCommand proposal in #18991 (it also proposes a per-call solution for overriding $PSNativeCommandArgumentPassing); it currently proposes something like
   Invoke-NativeCommand -IgnoreExitCode { test "$(Get-Item nosuch*)" }; if ($LASTEXITCODE) { ... }
   That is, the assumption is that the caller will inspect the exit code later and take appropriate action.
   Conceivably, this could be augmented as follows, in cases where the caller only cares about success vs. failure, not the specific exit code (which with test obviously doesn't make sense):
   Invoke-NativeCommand -SuccessExitCode 0, 1 { test "$(Get-Item nosuch*)" }; ...

   My personal concerns with respect to a potential Register-ProgramExitCode cmdlet are twofold:

   • While there definitely are high-profile CLIs - notably robocopy - that call for a solution that doesn't conflict with $PSNativeCommandUseErrorActionPreference, my sense is that they are a small percentage overall - most utilities play by the rules (0 for success, nonzero for failure), so a registry of such exit codes may not be worth the effort.
     On the flip side, if maintained at an organization level (rather than via a community module, see next point), I can see the appeal of a set-it-and-forget-it solution (but, as with any preference-variable-like solution, it amounts to invisible state, so that looking at a call in isolation you won't know how its exit code will be handled).

   • While a community effort always has its appeal, entrusting something as critical as error handling to a community-maintained module sounds problematic to me; for instance, users can reasonably argue over which of robocopy's nonzero exit codes should be considered a failure, utilities as a whole may be missing, values may be missing, the registered values can go stale over time, ...
3. Dongbo Wang (daxian-dbw)

   added

   WG-Enginecore PowerShell engine, interpreter, and runtimecore PowerShell engine, interpreter, and runtime

   on Jul 28, 2023
4. ThomasNieto commented on Jul 29, 2023

   

   Thomas Nieto (ThomasNieto)

   on Jul 29, 2023

   Last edited by ThomasNieto

   ContributorAuthor

   More actions

   I agree that the example provided was not the best and robocopy would have been a better one.

   Lets first level set on some common points/facts.

   • Exit code 0 as success and any other value as failure is a best practice
   • There is nothing enforcing exit code best practice and developers can and will do their own thing (robocopy, wusa, etc)
   • $PSNativeCommandUseErrorActionPreference set to $true by default is a breaking change due to a new unhandled exception being thrown. Looking at the RFC it should be disabled by default but might just be enabled for preview?

   Now for my views on this feature:

   • Since we know that exit codes values are arbitrary and best practices not enforced, PowerShell cannot assume zero is success and non-zero is a failure.
   • The user should be able to configure this feature on a per executable basis without having to change their execution method i.e. Invoke-NativeCommand
   • If the Register-ProgramExitCode feature was implemented, PowerShell should not ship/enable a default registry of exit codes. That should be up to the user on what they want configured. If a user, community, or official PowerShell module(s) was created with a registry, it shouldn't be a concern since the user would make the active choice to install and import the module with the ability to overwrite any registration. I would equate this to Register-ArgumentCompleter, the command exists for users to add their own completers and community modules like TabExpansionPlusPlus are available. The user is free to install a module with completers and overwrite them as desired.
   • User defined exit codes provide user feedback when running an executable. If the user is able to register a failure exit code with a message or custom error/exception then the PoweShell engine could retrieve the exit code and compare to the registry to get a helpful error message other than non-zero exit code is a failure. Lets say a program test.exe connects to a web service, exit code 5 is invalid credential. The user creates a registration Register-ProgramExitCode -Name test.exe -ExitCode 5 -Description 'Invalid credential'. Now when the user runs the program they get an error with the specific message instead of a generic non-zero exit code exception with a number they have to look up for the meaning.
5. mklement0 commented on Jul 29, 2023

   

   Michael Klement (mklement0)

   on Jul 29, 2023

   Last edited by mklement0

   Contributor

   More actions

   $PSNativeCommandUseErrorActionPreference set to $true by default is a breaking change

   Fortunately that won't be the case: As Steve Lee (@SteveL-MSFT) said here:

   I believe it was mentioned in a separate issue that $PSNativeCommandUseErrorActionPreference is default to $false and only set to $true in 7.4 previews to get feedback.

   In other words: the feature will be off by default in the release version - sensibly so.
   It will be opt-in, just like Bash's set -e is.

   👍React with 👍3Reacted by Thomas Nieto, kasini3000 and Keith Hill
6. mklement0 commented on Jul 29, 2023

   

   Michael Klement (mklement0)

   on Jul 29, 2023

   Contributor

   More actions

   Register-ProgramExitCode feature was implemented, PowerShell should not ship/enable a default registry of exit codes.

   Glad to hear it; that addresses my second concern.

   exit codes values are arbitrary and best practices not enforced

   I think proper framing is important here:

   • The 0-as-success-nonzero-as-failure is only a convention, yes, but a widely observed one with a long history.
   • Crucially, core features of many shells build on this convention:
     • && and || in POSIX-compatible shells, cmd.exe, and nowadays also in PowerShell
     • Conditionals in POSIX-compatible shells
     • set -e in Bash, for instance, and soon $PSNativeCommandUseErrorActionPreference in PowerShell

   So it should be noted that straying from this convention is ill-advised, because it runs afoul of core shell features, especially in the Unix world.
7. mklement0 commented on Jul 29, 2023

   

   Michael Klement (mklement0)

   on Jul 29, 2023

   Last edited by mklement0

   Contributor

   More actions

   P.S.: To be clear:

   • I can see the appeal of what you're proposing to handle those programs that don't play by the rules - although there are only a few, and hopefully their number won't grow.
   • The zero-vs.-nonzero convention hast the inherent limitation that only specific error conditions can be differentiated, not success conditions, which is what robocopy does. And, in the case of error conditions, a corresponding error message is usually emitted by the program itself, to stderr.
8. Steve Lee (SteveL-MSFT)

   added

   WG-NeedsReviewNeeds a review by the labeled Working GroupNeeds a review by the labeled Working Group

   on Oct 2, 2023
9. SteveL-MSFT commented on Oct 9, 2023

   

   Steve Lee (SteveL-MSFT)

   on Oct 9, 2023

   Member

   More actions

   Although PSNativeCommandUseErrorActionPreference is to be stable in 7.4, it does default to $false to preserve backwards compatibility. For robocopy type situations, you would need to write that to disable and re-enable this feature if you intend to use this feature in your script. There is still debate in the WG if having a user settable dictionary is desirable for the longterm.
10. Mathias R. Jessen (IISResetMe)

    assigned

    Mathias R. Jessen (IISResetMe)

    and

    Keith Hill (rkeithhill)

    and unassigned

    Mathias R. Jessen (IISResetMe)

    on Nov 27, 2023
11. rkeithhill commented on Nov 28, 2023

    

    Keith Hill (rkeithhill)

    on Nov 28, 2023

    Collaborator

    More actions

    The @PowerShell/wg-powershell-engine discussed this and concluded that it's not worth adding a mechanism to register successful non-zero exit codes. As Steve Lee (@SteveL-MSFT) points out, this feature is not enabled by default. It is "opt-in". And there is a documented mechanism to handle these exception cases in scripts where you have enabled $PSNativeCommandUseErrorActionPreference. From the about_Preference_Variables docs:

    # Some native commands, like robocopy use non-zero exit codes to represent
    # information other than errors. In these cases, you can temporarily disable
    # the behavior and prevent non-zero exit codes from issuing errors.
    
    $definedPreference = $PSNativeCommandUseErrorActionPreference
    $PSNativeCommandUseErrorActionPreference = $false
    robocopy.exe D:\reports\operational "\\reporting\ops" CY2022Q4.md
    $robocopyExitCode = $LASTEXITCODE
    if ($robocopyExitCode -gt 8) {
        throw "robocopy failed with exit code $robocopyExitCode"
    }
    $PSNativeCommandUseErrorActionPreference = $definedPreference

    Although I suggest we update the docs to take advantage of scriptblock scoping & cleanup e.g.:

    # Enable non-zero exit codes to stop script
    $ErrorActionPreference = 'Stop'
    $PSNativeCommandUseErrorActionPreference = $true
    
    # This should stop the script if a non-zero exit code is returned
    dotnet build
    
    # Handle app that returns non-zero "successful" exit code
    & {
        $PSNativeCommandUseErrorActionPreference = $false
        robocopy.exe D:\reports\operational "\\reporting\ops" CY2022Q4.md
        if ($LASTEXITCODE -gt 8) {
            throw "robocopy failed with exit code $robocopyExitCode"
        }
    }
    
    # This should stop the script if a non-zero exit code is returned
    dotnet test

    👍React with 👍2Reacted by Michael Klement and Mathias R. Jessen
12. Keith Hill (rkeithhill)

    added

    WG-ReviewedA Working Group has reviewed this and made a recommendationA Working Group has reviewed this and made a recommendation

    Resolution-DeclinedThe proposed feature is declined.The proposed feature is declined.

    and removed

    Needs-TriageThe issue is new and needs to be triaged by a work group.The issue is new and needs to be triaged by a work group.

    WG-NeedsReviewNeeds a review by the labeled Working GroupNeeds a review by the labeled Working Group

    on Nov 28, 2023
13. microsoft-github-policy-service commented on Dec 2, 2023

    

    microsoft-github-policy-servicebot

    on Dec 2, 2023 – with Microsoft GitHub Policy Service

    Contributor

    More actions

    This issue has been marked as declined and has not had any activity for 1 day. It has been closed for housekeeping purposes.
14. microsoft-github-policy-service

    closed this as completedon Dec 2, 2023