-
Notifications
You must be signed in to change notification settings - Fork 2.2k
uvx/uv tool run install confirmation prompt
#16743
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
|
|
||
| /// A builder for an [`RegistryClient`]. | ||
| #[derive(Debug, Clone)] | ||
| #[derive(Clone)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to re-add a Debug implementation. The PreDownloadHook broke the automatic one.
| // Resolve the requirements with the interpreter. | ||
| let resolution = Resolution::from( | ||
| resolve_environment( | ||
| crate::commands::project::resolve_environment_with_hook( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixing
| let mut builder = RegistryClientBuilder::new(client_builder, cache.clone()) | ||
| .index_locations(index_locations.clone()) | ||
| .index_strategy(*index_strategy) | ||
| .markers(interpreter.markers()) | ||
| .platform(interpreter.platform()) | ||
| .build(); | ||
| .platform(interpreter.platform()); | ||
|
|
||
| builder = builder.pre_download_hook_arc(pre_download_hook); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like I should just be able to do this as part of the builder. I keep banging against the compiler doing so though.
Signed-off-by: Mikayla Thompson <[email protected]>
f1fb0ff to
ee2208b
Compare
Signed-off-by: Mikayla Thompson <[email protected]>
bd2907d to
3550e98
Compare
Signed-off-by: Mikayla Thompson <[email protected]>
6e87f75 to
06aaab0
Compare
|
Even though it's opt-in right now (thinking about a future where it's opt-out), do we want a security setting such as |
Summary
This is a proof-of-concept for #16600, providing installation confirmation prompts.
It adds an opt-in confirmation prompt system that asks users to confirm before installing uncached packages when running
uvxoruv tool run. The feature is gated behind thetool-install-confirmationpreview flag.The implementation adds a pre-download hook that gets passed down to the HTTP client to run before downloading any files. Putting it right before the download step ensures that no already-cached packages are prompted. Additionally, this hook can be used by future implementations of the prompt in
uv addor other commands. To get to that step, the biggest change would be toA heuristics mechanism allows certain packages to skip prompts automatically. Right now only the top-packages heuristic is implemented, which skips prompts for a number of popular Python packages determined by analyzing GitHub Code Search results for
uvxusage patterns. Two other heuristics are stubbed out but not implemented yet: previously-installed and a custom allowlist.Configuration is handled through
InstallPromptOptionsin the settings, with anapprove-all-tool-installsflag to skip all prompts (useful for CI/CD) andapprove-all-heuristicsto control which heuristics are enabled. There's also a--approve-all-tool-installsCLI flag that overrides config settings. The prompt logic handles TTY detection for non-interactive environments, to minimize the impact on CI and other non-interactive usecases.Config file (
~/.config/uv/uv.tomloruv.toml):CLI usage:
Catching a typo with top-packages enabled
Test Plan
There are unit tests, but they're somewhat limited by the tty-detecting behavior. It's possibly worth mocking that call to block it, but I haven't made it that far.
A bit on the top-packages.txt file
Zsolt brought up that packages popular for tool use are not necessarily those well-represented in the overall PyPI data. I was curious about the idea of searching GitHub for mentions of
uvxto figure out what packages are being used as tools.The manual search results looked promising, so I embarked on the surprisingly-complicated step of collating this data in a repeatable way.
The GitHub code search API is annoying-at-best:
Fiddling with this resulted in the following non-optimized process:
# run uvx if you need a toolreturnsifas a package name).The results of this look reasonable overall, with some caveats. At this point, I'm putting any package with > 5 mentions in at this point.
uvxcall for the packagewith. Applying a PyPI popularity filter on top of this would make it somewhat more resilient to that type of example.Known edge cases
--with <package>is not caught by the prompt. It's slightly unclear how this fits into the design of only prompting on the top-level package. I think the best approach here is to adjust the prompt to mention both packages. However, this adds some complication where the hook has to somehow collect all of the necessary packages, even though neither the top level (the tool run) or the low level (the http client) know that there will be multiple packages that need to be included (some could already be cached).