Skip to content

Don't require --source when partial source failure leaves a single result #6165

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.

Sign up for GitHub

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

Open
Sinan-Karakaya wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Don't require --source when partial source failure leaves a single result #6165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
981e1a9
fix: Auto-proceed on single result from working sources
Sinan-Karakaya Apr 21, 2026
3014777
fix: Add prompt to proceed on partial search failure with single result
Sinan-Karakaya Apr 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Resources.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,7 @@ namespace AppInstaller::CLI::Resource
WINGET_DEFINE_RESOURCE_STRINGID(SearchFailureError);
WINGET_DEFINE_RESOURCE_STRINGID(SearchFailureErrorListMatches);
WINGET_DEFINE_RESOURCE_STRINGID(SearchFailureErrorNoMatches);
WINGET_DEFINE_RESOURCE_STRINGID(SearchFailureSingleResultPrompt);
WINGET_DEFINE_RESOURCE_STRINGID(SearchFailureWarning);
WINGET_DEFINE_RESOURCE_STRINGID(SearchId);
WINGET_DEFINE_RESOURCE_STRINGID(SearchMatch);
Expand Down
47 changes: 24 additions & 23 deletions src/AppInstallerCLICore/Workflows/PromptFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,35 +11,36 @@ using namespace AppInstaller::Utility::literals;

namespace AppInstaller::CLI::Workflow
{
namespace
bool IsInteractivityAllowed(Execution::Context& context)
Copy link
Copy Markdown
Member

@JohnMcPMS JohnMcPMS Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
bool IsInteractivityAllowed(Execution::Context& context)
// Interactivity can be disabled for several reasons:
// * We are running in a non-interactive context (e.g., COM call)
// * It is disabled in the settings
// * It was disabled from the command line

if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::DisableInteractivity))
{
// Interactivity can be disabled for several reasons:
// * We are running in a non-interactive context (e.g., COM call)
// * It is disabled in the settings
// * It was disabled from the command line
AICLI_LOG(CLI, Verbose, << "Skipping prompt. Interactivity is disabled due to non-interactive context.");
return false;
}

if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::DisableInteractivity))
{
AICLI_LOG(CLI, Verbose, << "Skipping prompt. Interactivity is disabled due to non-interactive context.");
return false;
}
if (context.Args.Contains(Execution::Args::Type::DisableInteractivity))
{
AICLI_LOG(CLI, Verbose, << "Skipping prompt. Interactivity is disabled by command line argument.");
return false;
}

if (context.Args.Contains(Execution::Args::Type::DisableInteractivity))
{
AICLI_LOG(CLI, Verbose, << "Skipping prompt. Interactivity is disabled by command line argument.");
return false;
}
if (Settings::User().Get<Settings::Setting::InteractivityDisable>())
{
AICLI_LOG(CLI, Verbose, << "Skipping prompt. Interactivity is disabled in settings.");
return false;
}

if (Settings::User().Get<Settings::Setting::InteractivityDisable>())
{
AICLI_LOG(CLI, Verbose, << "Skipping prompt. Interactivity is disabled in settings.");
return false;
}
return true;
}

return true;
}
namespace
{

bool HandleSourceAgreementsForOneSource(Execution::Context& context, const Repository::Source& source)
{
Expand Down
4 changes: 4 additions & 0 deletions src/AppInstallerCLICore/Workflows/PromptFlow.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@

namespace AppInstaller::CLI::Workflow
{
// Returns true if interactivity is currently allowed in the given context.
bool IsInteractivityAllowed(Execution::Context& context);


// Handles all opened source(s) agreements if needed.
// Required Args: The source to be checked for agreements
// Inputs: None
Expand Down
32 changes: 32 additions & 0 deletions src/AppInstallerCLICore/Workflows/WorkflowBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,38 @@ namespace AppInstaller::CLI::Workflow
warn << Resource::String::SearchFailureWarning(Utility::LocIndView{ failure.SourceName }) << std::endl;
}
}
// 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) &&
Copy link
Copy Markdown
Member

@JohnMcPMS JohnMcPMS Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Are we using warnings or errors? Save that choice, select output stream and lambda body to match.
  2. Enumerate failures, invoke output lambda, extract HRESULT (even if warning).
  3. If we decided this is a warning, return.
  4. Continue on with the second half of the existing else with the new prompt.

searchResult.Matches.size() == 1 &&
IsInteractivityAllowed(context))
{
{
auto warn = context.Reporter.Warn();
for (const auto& failure : searchResult.Failures)
{
warn << Resource::String::SearchFailureWarning(Utility::LocIndView{ failure.SourceName }) << std::endl;
}
}

if (context.Reporter.PromptForBoolResponse(Resource::String::SearchFailureSingleResultPrompt, Reporter::Level::Warning, false))
{
return;
}

// User declined; terminate with the source failure HRESULT without re-showing errors
HRESULT overallHR = S_OK;
for (const auto& failure : searchResult.Failures)
{
HRESULT failureHR = HandleException(nullptr, failure.Exception);
if (overallHR == S_OK)
{
overallHR = failureHR;
}
}
context.SetTerminationHR(overallHR);
}
else
{
HRESULT overallHR = S_OK;
Expand Down
4 changes: 4 additions & 0 deletions src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1332,6 +1332,10 @@ Do you agree to the terms?</value>
Please specify one of them using the --source option to proceed.</value>
<comment>{Locked="--source"} "working sources" as in "sources that are working correctly"</comment>
</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>
Copy link
Copy Markdown
Member

@JohnMcPMS JohnMcPMS Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<comment>"working sources" as in "sources that are working correctly"</comment>
<comment>"working sources" as in "package sources that are functioning properly"</comment>

</data>
<data name="SearchFailureErrorNoMatches" xml:space="preserve">
<value>No packages were found among the working sources.</value>
<comment>"working sources" as in "sources that are working correctly"</comment>
Expand Down
62 changes: 62 additions & 0 deletions src/AppInstallerCLITests/WorkFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <AppInstallerDownloader.h>
#include <winget/Settings.h>
#include <Workflows/DownloadFlow.h>
#include <Workflows/WorkflowBase.h>
#include <Commands/InstallCommand.h>
#include <Commands/SettingsCommand.h>
#include <Commands/ValidateCommand.h>
Expand Down Expand Up @@ -187,3 +188,64 @@ TEST_CASE("Export_Settings", "[Settings][workflow]")
REQUIRE(userSettingsFileValue.find("settings.json") != std::string::npos);
}
}

TEST_CASE("HandleSearchResultFailures_SingleMatchWithPartialFailure", "[HandleSearchResultFailures][workflow]")
{
auto makeSearchResult = []()
Copy link
Copy Markdown
Member

@JohnMcPMS JohnMcPMS Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
SearchResult result;
result.Matches.push_back(ResultMatch{
TestCompositePackage::Make(std::vector<Manifest>{ Manifest{} }),
PackageMatchFilter{ PackageMatchField::Id, MatchType::Exact }
});
result.Failures.push_back({ "FailedSource", std::make_exception_ptr(std::runtime_error("source error")) });
return result;
};

SECTION("Interactive_Accept")
{
std::istringstream input{ "y\n" };
std::ostringstream output;
TestContext context{ output, input };
auto previousThreadGlobals = context.SetForCurrentThread();
context.SetFlags(ContextFlag::ShowSearchResultsOnPartialFailure);
context.Add<Data::SearchResult>(makeSearchResult());

context << HandleSearchResultFailures;

INFO(output.str());
REQUIRE_FALSE(context.IsTerminated());
REQUIRE(output.str().find("FailedSource") != std::string::npos);
}

SECTION("Interactive_Decline")
{
std::istringstream input{ "n\n" };
std::ostringstream output;
TestContext context{ output, input };
auto previousThreadGlobals = context.SetForCurrentThread();
context.SetFlags(ContextFlag::ShowSearchResultsOnPartialFailure);
context.Add<Data::SearchResult>(makeSearchResult());

context << HandleSearchResultFailures;

INFO(output.str());
REQUIRE(context.IsTerminated());
}

SECTION("NonInteractive_HardError")
{
std::ostringstream output;
TestContext context{ output, std::cin };
auto previousThreadGlobals = context.SetForCurrentThread();
context.SetFlags(ContextFlag::ShowSearchResultsOnPartialFailure);
context.Args.AddArg(Args::Type::DisableInteractivity);
context.Add<Data::SearchResult>(makeSearchResult());

context << HandleSearchResultFailures;

INFO(output.str());
REQUIRE(context.IsTerminated());
REQUIRE(output.str().find("FailedSource") != std::string::npos);
}
}
Loading