Don't require --source when partial source failure leaves a single result#6165
Don't require --source when partial source failure leaves a single result#6165Sinan-Karakaya wants to merge 2 commits intomicrosoft:masterfrom
Conversation
Treat source failures as warnings instead of errors when the install command finds exactly one result from working sources, avoiding the need for the user to manually specify --source. Signed-off-by: Sinan Karakaya <me@sinankarakaya.fr>
There was a problem hiding this comment.
Pull request overview
This PR fixes winget install behavior when some configured sources fail during search: if exactly one package match is returned from the working sources, the install should proceed (while still surfacing failed sources as warnings) instead of stopping and requiring --source.
Changes:
- In
HandleSearchResultFailures, treat partial-source failures as warnings (not a hard error) whenShowSearchResultsOnPartialFailureis set and exactly one match exists. - Preserve existing hard-error behavior for zero matches or multiple matches under partial failure.
|
I'm not sure that's the behavior we want. Given multiple sources configured in WinGet and a "search sequence" if two sources would both return "one best match" the user should disambiguate, or source priority should "break the tie". I'd be more in favor of an interactive prompt with the one best match and the source(s) with failures. That also begs the question about what the default behavior should be when interactivity is disabled. |
|
If there is an error searching one or more sources, we don't know if there is really only one result. That is why there is a hard error and the user must explicitly choose the target source. |
Refactors `IsInteractivityAllowed` out of an anonymous namespace to allow broader usage. Updates the partial search failure logic to prompt the user if exactly one match is found and interactivity is allowed, rather than auto-proceeding or failing. Signed-off-by: Sinan KARAKAYA<me@sinankarakaya.fr>
|
Thanks for the feedback, all fair points. I've reworked the approach based on the discussion: Instead of silently auto-proceeding, the updated behavior now shows a For the non-interactive case (COM callers, On the "we don't know if there's really only one result" point: agreed, the prompt doesn't change that. But it shifts responsibility to the user: they're explicitly acknowledging they want to proceed despite the uncertainty, rather than winget guessing on their behalf. If they want certainty, they can still The comment was also updated to reference The source priority / hunt sequence tie-breaking is a separate and more involved problem. I'd be happy to track that as a follow-up if that's useful, but I don't think it blocks this. |
|
@microsoft-github-policy-service agree |
|
Maybe we need to re-think the user settings for interactivity a bit @denelon ? I could see a case where a user would want to specify the default behavior for each interaction For example, disable interactivity and don't accept any source agreements, but still proceed if there were source failures. Proceed through prompt for an install path with a user-defined default but hard stop on configuration review / continue prompt Probably a bit outside the scope of this PR, but just something it brought to mind |
|
I created: to address the tie-breaking scenario so this PR can be reviewed. |
| namespace AppInstaller::CLI::Workflow | ||
| { | ||
| namespace | ||
| bool IsInteractivityAllowed(Execution::Context& context) |
There was a problem hiding this comment.
I don't want this to be publicly visible; callers should be required to go through our explicitly defined prompt flows rather than making arbitrary choices about handling interactivity. That also makes the call sites less cluttered, as they are forced to simply invoke a prompt function with a default rather than have control flow.
I think that the correct approach is going to be refactoring the Reporter prompt methods out into PromptFlow.cpp and have them invoke this function in addition to other checks they do to determine whether to actually prompt or not.
| // When ShowSearchResultsOnPartialFailure is set (e.g. install) and exactly one match | ||
| // was found, prompt the user interactively rather than failing outright. | ||
| // Falls back to the hard error path if interactivity is disabled. | ||
| else if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::ShowSearchResultsOnPartialFailure) && |
There was a problem hiding this comment.
This block duplicates a lot of code from the other blocks. I would rather we just accept the error output and move the prompt invocation into the correct location in the existing else.
If a warning about each source is a critical change to you, the entire function could probably be refactored to better make sematic decisions up front and then execute on those using a more common code path:
- Are we using warnings or errors? Save that choice, select output stream and lambda body to match.
- Enumerate failures, invoke output lambda, extract HRESULT (even if warning).
- If we decided this is a warning, return.
- Continue on with the second half of the existing
elsewith the new prompt.
| </data> | ||
| <data name="SearchFailureSingleResultPrompt" xml:space="preserve"> | ||
| <value>A package was found among the working sources. Would you like to proceed?</value> | ||
| <comment>"working sources" as in "sources that are working correctly"</comment> |
There was a problem hiding this comment.
| <comment>"working sources" as in "sources that are working correctly"</comment> | |
| <comment>"working sources" as in "package sources that are functioning properly"</comment> |
|
|
||
| TEST_CASE("HandleSearchResultFailures_SingleMatchWithPartialFailure", "[HandleSearchResultFailures][workflow]") | ||
| { | ||
| auto makeSearchResult = []() |
There was a problem hiding this comment.
nit: catch2 runs the entire test multiple times, dynamically enabling sections each time to get the complete set. You can just build the result in a local in the test rather than using a lambda. Mostly saves a few lines of code.
Fixes #6164
Right now, when one or more sources fail during an install search, winget shows the results from the sources that did respond and asks you to pick one via
--sourceeven if there's only one result. This happens inHandleSearchResultFailuresvia theShowSearchResultsOnPartialFailureflag, which is set exclusively by the install command.The fix is small: before routing into the "error and stop" path, check whether we're in install mode with exactly one match. If so, fall into the same "warn and continue" path that
TreatSourceFailuresAsWarningalready uses, the failed sources still get surfaced as warnings, but the install proceeds.Existing behavior is preserved for:
--sourceShowSearchResultsOnPartialFailureMicrosoft Reviewers: Open in CodeFlow