Remove approval prompt from dotnet tool exec - running dnx is implicit approval#52956
Conversation
dotnet tool exec
|
@copilot the entire "check for approval" concept is unnecessary. The fix here should be to remove the entire "prompt" check - the usage of |
Agreed - I've removed the entire approval check. Running |
dotnet tool execdotnet tool exec - running dnx is implicit approval
dsplaisted
left a comment
There was a problem hiding this comment.
I'm OK with this, but AFAIK npm does prompt you when in interactive mode. I'm not sure everyone was fully agreed to remove the prompt entirely.
| { | ||
| if (_interactive) | ||
| { | ||
| Reporter.Error.WriteLine(CliCommandStrings.ToolDownloadCanceled.Red().Bold()); |
There was a problem hiding this comment.
@copilot Also remove the CliCommandStrings that have been removed from the resources. You'll probably need to do a build to update the xlf files too.
There was a problem hiding this comment.
Removed the unused resource strings (ToolDownloadConfirmationPrompt, ToolDownloadCanceled, ToolDownloadNeedsConfirmation) and updated all xlf files using /t:UpdateXlf. Changes in commit 4744ace.
You're right that npm does prompt in interactive mode. However, @baronfel's guidance was that for If the team prefers npm's behavior (prompt in interactive, auto-accept in non-interactive), I can restore the prompt logic but fix it to properly detect non-interactive scenarios. Let me know which approach you'd prefer. |
There was a problem hiding this comment.
Pull request overview
Removes the interactive approval prompt from dotnet tool exec (dnx), treating invocation as implicit consent to download and run the tool package.
Changes:
- Removed the confirmation/approval flow (and related error paths) from tool execution when the tool package isn’t already cached.
- Deleted now-unused localized resource strings for the tool-download prompt/cancel messaging.
- Updated XLF localization files to remove the deleted tool-download strings.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Cli/dotnet/Commands/Tool/Execute/ToolExecuteCommand.cs | Removes the interactive approval prompt and downloads the tool package immediately when missing. |
| src/Cli/dotnet/Commands/CliCommandStrings.resx | Removes the tool-download confirmation/cancel resource strings no longer referenced. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.cs.xlf | Removes tool-download trans-units; also includes unintended confirmation-prompt translation resets (needs fix). |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.de.xlf | Removes tool-download trans-units; also includes unintended confirmation-prompt translation resets (needs fix). |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.es.xlf | Removes tool-download trans-units; also includes unintended confirmation-prompt translation resets (needs fix). |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.fr.xlf | Removes tool-download trans-units; also includes unintended confirmation-prompt translation resets (needs fix). |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.it.xlf | Removes tool-download trans-units; also includes unintended confirmation-prompt translation resets (needs fix). |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.ja.xlf | Removes tool-download trans-units; also includes unintended confirmation-prompt translation resets (needs fix). |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.ko.xlf | Removes tool-download trans-units; also includes unintended confirmation-prompt translation resets (needs fix). |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.pl.xlf | Removes tool-download trans-units; also includes unintended confirmation-prompt translation resets (needs fix). |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.pt-BR.xlf | Removes tool-download trans-units; also includes unintended confirmation-prompt translation resets (needs fix). |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.ru.xlf | Removes tool-download trans-units; also includes unintended confirmation-prompt translation resets (needs fix). |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.tr.xlf | Removes tool-download trans-units; also includes unintended confirmation-prompt translation resets (needs fix). |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.zh-Hans.xlf | Removes tool-download trans-units; also includes unintended confirmation-prompt translation resets (needs fix). |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.zh-Hant.xlf | Removes tool-download trans-units; also includes unintended confirmation-prompt translation resets (needs fix). |
| <trans-unit id="ConfirmationPromptInvalidChoiceMessage"> | ||
| <source>Please type '{0}' for yes or '{1}' for no.</source> | ||
| <target state="translated">Введите "{0}", чтобы указать "да", и "{1}", чтобы указать "нет".</target> | ||
| <target state="new">Please type '{0}' for yes or '{1}' for no.</target> | ||
| <note /> | ||
| </trans-unit> |
There was a problem hiding this comment.
The localized targets for ConfirmationPromptInvalidChoiceMessage (and the related yes/no key strings immediately below) were reset to English with state="new". These resources are still used by InteractiveConsole.Confirm, so this introduces a localization regression and may also change accepted input keys in localized prompts. Please restore the previous translations/target states for these confirmation prompt entries while keeping only the tool-download trans-units removed.
| <trans-unit id="ConfirmationPromptInvalidChoiceMessage"> | ||
| <source>Please type '{0}' for yes or '{1}' for no.</source> | ||
| <target state="translated">请键入 '{0}' 以表示是或键入 '{1}' 以表示否。</target> | ||
| <target state="new">Please type '{0}' for yes or '{1}' for no.</target> | ||
| <note /> | ||
| </trans-unit> |
There was a problem hiding this comment.
The localized targets for ConfirmationPromptInvalidChoiceMessage (and the related yes/no key strings immediately below) were reset to English with state="new". These resources are still used by InteractiveConsole.Confirm, so this introduces a localization regression and may also change accepted input keys in localized prompts. Please restore the previous translations/target states for these confirmation prompt entries while keeping only the tool-download trans-units removed.
| if (!_toolPackageDownloader.TryGetDownloadedTool(packageId, bestVersion, targetFramework: null, verbosity: _verbosity, out var toolPackage)) | ||
| { | ||
| if (!UserAgreedToRunFromSource(packageId, bestVersion, packageSource)) | ||
| { | ||
| if (_interactive) | ||
| { | ||
| Reporter.Error.WriteLine(CliCommandStrings.ToolDownloadCanceled.Red().Bold()); | ||
| return ERROR_CANCELLED; | ||
| } | ||
| else | ||
| { | ||
| Reporter.Error.WriteLine(CliCommandStrings.ToolDownloadNeedsConfirmation.Red().Bold()); | ||
| return 1; | ||
| } | ||
| } | ||
|
|
||
| // We've already determined which source we will use and displayed that in a confirmation message to the user. | ||
| // We've already determined which source we will use and will use it to download the package. | ||
| // So set the package location here to override the source feeds to just the source we already resolved to. | ||
| // This does mean that we won't work with feeds that have a primary package but where the RID-specific packages are on |
There was a problem hiding this comment.
This change removes the explicit confirmation step before downloading a one-shot tool, but existing dotnet tool exec/dnx end-to-end tests appear to always pass --yes. To validate the new implicit-approval behavior, please add/update at least one test to run dotnet tool exec <package> without --yes (both TTY and non-interactive if feasible) and assert it still downloads/executes successfully.
| <trans-unit id="ConfirmationPromptInvalidChoiceMessage"> | ||
| <source>Please type '{0}' for yes or '{1}' for no.</source> | ||
| <target state="translated">Tapez « {0} » pour Oui ou « {1} » pour non.</target> | ||
| <target state="new">Please type '{0}' for yes or '{1}' for no.</target> | ||
| <note /> | ||
| </trans-unit> |
There was a problem hiding this comment.
The localized targets for ConfirmationPromptInvalidChoiceMessage (and the related yes/no key strings immediately below) were reset to English with state="new". These resources are still used by InteractiveConsole.Confirm, so this introduces a localization regression and may also change accepted input keys in localized prompts. Please restore the previous translations/target states for these confirmation prompt entries while keeping only the tool-download trans-units removed.
| <trans-unit id="ConfirmationPromptInvalidChoiceMessage"> | ||
| <source>Please type '{0}' for yes or '{1}' for no.</source> | ||
| <target state="translated">はいの場合は「{0}」、いいえの場合は「{1}」と入力してください。</target> | ||
| <target state="new">Please type '{0}' for yes or '{1}' for no.</target> | ||
| <note /> | ||
| </trans-unit> |
There was a problem hiding this comment.
The localized targets for ConfirmationPromptInvalidChoiceMessage (and the related yes/no key strings immediately below) were reset to English with state="new". These resources are still used by InteractiveConsole.Confirm, so this introduces a localization regression and may also change accepted input keys in localized prompts. Please restore the previous translations/target states for these confirmation prompt entries while keeping only the tool-download trans-units removed.
| <trans-unit id="ConfirmationPromptInvalidChoiceMessage"> | ||
| <source>Please type '{0}' for yes or '{1}' for no.</source> | ||
| <target state="translated">請輸入 '{0}' 表示是或 '{1}' 表示否。</target> | ||
| <target state="new">Please type '{0}' for yes or '{1}' for no.</target> | ||
| <note /> | ||
| </trans-unit> |
There was a problem hiding this comment.
The localized targets for ConfirmationPromptInvalidChoiceMessage (and the related yes/no key strings immediately below) were reset to English with state="new". These resources are still used by InteractiveConsole.Confirm, so this introduces a localization regression and may also change accepted input keys in localized prompts. Please restore the previous translations/target states for these confirmation prompt entries while keeping only the tool-download trans-units removed.
| <trans-unit id="ConfirmationPromptInvalidChoiceMessage"> | ||
| <source>Please type '{0}' for yes or '{1}' for no.</source> | ||
| <target state="translated">Geben Sie "{0}" für Ja oder "{1}" für Nein ein.</target> | ||
| <target state="new">Please type '{0}' for yes or '{1}' for no.</target> | ||
| <note /> | ||
| </trans-unit> |
There was a problem hiding this comment.
The localized targets for ConfirmationPromptInvalidChoiceMessage (and the related yes/no key strings immediately below) were reset to English with state="new". These resources are still used by InteractiveConsole.Confirm, so this introduces a localization regression and may also change accepted input keys in localized prompts. Please restore the previous translations/target states for these confirmation prompt entries while keeping only the tool-download trans-units removed.
| <trans-unit id="ConfirmationPromptInvalidChoiceMessage"> | ||
| <source>Please type '{0}' for yes or '{1}' for no.</source> | ||
| <target state="translated">Escriba "{0}" para sí o "{1}" para no.</target> | ||
| <target state="new">Please type '{0}' for yes or '{1}' for no.</target> | ||
| <note /> | ||
| </trans-unit> |
There was a problem hiding this comment.
The localized targets for ConfirmationPromptInvalidChoiceMessage (and the related yes/no key strings immediately below) were reset to English with state="new". These resources are still used by InteractiveConsole.Confirm, so this introduces a localization regression and may also change accepted input keys in localized prompts. Please restore the previous translations/target states for these confirmation prompt entries while keeping only the tool-download trans-units removed.
| <trans-unit id="ConfirmationPromptInvalidChoiceMessage"> | ||
| <source>Please type '{0}' for yes or '{1}' for no.</source> | ||
| <target state="translated">Digita '{0}' per sì o '{1}' per no.</target> | ||
| <target state="new">Please type '{0}' for yes or '{1}' for no.</target> | ||
| <note /> | ||
| </trans-unit> |
There was a problem hiding this comment.
The localized targets for ConfirmationPromptInvalidChoiceMessage (and the related yes/no key strings immediately below) were reset to English with state="new". These resources are still used by InteractiveConsole.Confirm, so this introduces a localization regression and may also change accepted input keys in localized prompts. Please restore the previous translations/target states for these confirmation prompt entries while keeping only the tool-download trans-units removed.
| <trans-unit id="ConfirmationPromptInvalidChoiceMessage"> | ||
| <source>Please type '{0}' for yes or '{1}' for no.</source> | ||
| <target state="translated">Wpisz „{0}”, aby dla tak lub „{1}” dla nie.</target> | ||
| <target state="new">Please type '{0}' for yes or '{1}' for no.</target> | ||
| <note /> | ||
| </trans-unit> |
There was a problem hiding this comment.
The localized targets for ConfirmationPromptInvalidChoiceMessage (and the related yes/no key strings immediately below) were reset to English with state="new". These resources are still used by InteractiveConsole.Confirm, so this introduces a localization regression and may also change accepted input keys in localized prompts. Please restore the previous translations/target states for these confirmation prompt entries while keeping only the tool-download trans-units removed.
marcpopMSFT
left a comment
There was a problem hiding this comment.
Tested and confirmed no approval required.
|
Glad to see this fix rolling out. |
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
… logic Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
Co-authored-by: dsplaisted <145043+dsplaisted@users.noreply.github.com>
4744ace to
a73677d
Compare
|
@copilot The help still shows --yes. We don't want to remove the option as we don't want to break folks but we want to remove it from help since it's not needed anymore. I think you want to add a parameter to CreateYesOption that lets us create it hidden for the execute command. |
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
Added Help output change: The |
|
Latest change matches what I requested. I confirmed that the help no longer specifies --yes but the command doesn't break if it's included. |
Remove approval prompt from
dotnet tool exec- running dnx is implicit approvaldotnet tool exec(dnx) previously prompted users to confirm tool downloads, blocking LLMs, scripts, and automation. This PR removes the approval prompt entirely - runningdnxis now treated as implicit approval.Changes
Removed approval check
UserAgreedToRunFromSource()method that prompted for confirmationERROR_CANCELLEDconstant and_interactivefieldToolDownloadConfirmationPrompt,ToolDownloadCanceled,ToolDownloadNeedsConfirmation--yesoption from help output (option still works for backward compatibility)Rationale
When a user runs
dotnet tool exec <package>, they are explicitly requesting to execute that package - this is the approval. The previous confirmation prompt:This aligns with the philosophy of tools like
npxwhere running the command implies consent to download and execute the requested package.Help Output
The
--yesoption is now hidden from help since it's no longer needed, but remains functional for backward compatibility:Before:
After:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.