Repository navigation
Conversation
ranm-msft
left a comment
ranm-msft
left a comment
There was a problem hiding this comment.
Holding approval for two agreement-consent paths.
- Fail closed when source agreement state cannot be read.
- Present the complete terms for text-only agreements.
| // Reading the agreements requires contacting the source. Rather than turn a transient | ||
| // failure into a confusing agreement error, use the catalog and let connecting to it | ||
| // report the actual problem. | ||
| selection.Included.Add(catalog); |
There was a problem hiding this comment.
This fails open on agreement-state retrieval: a transient information-endpoint failure adds the catalog, Connect retries the same REST reference, and AcceptSourceAgreements defaults true, so the recovered source can be searched or installed from without presenting its agreements. Please fail or exclude this source until its agreement state can be read successfully.
| if (agreement.Text.Length > MaximumAgreementTextLength) | ||
| { | ||
| // The full text remains available at the URL, which follows when present. | ||
| message.Append(agreement.Text, 0, MaximumAgreementTextLength); |
There was a problem hiding this comment.
This can let the user accept a valid text-only agreement after seeing only its first 2,000 characters; agreement text may be 10,000 characters and the URL is optional, while accepted elicitation proceeds without invoking the full-text fallback. Please only truncate when the complete terms remain accessible, or present the full text before accepting.
ranm-msft
left a comment
ranm-msft
left a comment
There was a problem hiding this comment.
The source-agreement tests need deterministic cleanup before I can sign off.
| [TearDown] | ||
| public void TearDown() | ||
| { | ||
| CleanupTestPackages(); |
There was a problem hiding this comment.
Both x64 and x86 CI time out here after SourceAgreements_Elicitation_Declined leaves TestMCPAgreementsRestSource installed with changed, unaccepted agreements. Because agreementsSource.Remove() follows CleanupTestPackages() in both teardown and setup, the first uninstall timeout prevents source removal and later agreement cases repeat the timeout during setup. Remove the agreement source before running winget uninstall, and ensure source removal still runs if package cleanup throws.
ranm-msft
left a comment
ranm-msft
left a comment
There was a problem hiding this comment.
One package-consent failure path still needs to fail closed.
| { | ||
| // Not every source can supply metadata. If none is available, there is nothing to present, | ||
| // and the install will report if agreements were in fact required. | ||
| result.Clear(); |
There was a problem hiding this comment.
If package metadata cannot be read, this returns an empty list and continues with new InstallOptions(), whose AcceptPackageAgreements value defaults to true. On a source where a later manifest read succeeds, install or upgrade can therefore proceed with the agreements marked accepted without presenting them. Please default this option to false and enable it only after an accepted consent decision, with a failure-then-recovery test.
microsoft-github-policy-service
Bot
added
the
Needs-Author-Feedback
Findings resolved at head; see my follow-up comment.
ranm-msft
left a comment
ranm-msft
left a comment
There was a problem hiding this comment.
Both of my open points are fixed at d559578, so I am withdrawing my changes-requested vote:
- Package consent:
PackageAgreementConsent.GetAgreementsno longer catches the metadata failure, so the install fails closed instead of continuing withAcceptPackageAgreementsdefaulted to true. - Truncated agreement text: text is truncated only when the agreement has a URL that holds the full terms.
The x64 and x86 release builds failed during vcpkg setup ($(VCPKG_INSTALLATION_ROOT)\buildtrees not found, then ...\a\x64 missing). That looks like infrastructure, not this change. I will approve once a rerun is green.
JohnMcPMS
removed
the
Needs-Author-Feedback
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved consent flaws can silently accept unseen dependency terms or reuse stale consent after an explicit refusal.
3 open issues
What changed in this PR
This PR exposes source agreement state through the WinGet COM API and adds source and package consent handling to the MCP server.
Changes:
- Adds COM APIs to query agreement acceptance, disclose implicit fields, and persist source consent.
- Adds MCP elicitation and fallback responses for agreements, including reporting excluded sources.
- Adds COM interoperability and MCP end-to-end tests, with supporting fixtures and configuration.
| File | Description |
|---|---|
| src/WinGetMCPServer/WingetPackageTools.cs | Integrates consent into package search and installation. |
| src/WinGetMCPServer/WinGetMCPServer.csproj | Adds projections needed for localized resources. |
| src/WinGetMCPServer/Response/ToolResponse.cs | Reports excluded and unavailable sources. |
| src/WinGetMCPServer/Response/PackageResponse.cs | Clarifies agreement-related installation errors. |
| src/WinGetMCPServer/Response/ExcludedSources.cs | Tracks sources omitted from requests. |
| src/WinGetMCPServer/Response/AgreementsRequiredResult.cs | Defines agreement response payloads. |
| src/WinGetMCPServer/Response/AgreementResponse.cs | Builds consent-required and refusal responses. |
| src/WinGetMCPServer/Response/AgreementInfo.cs | Represents source and package agreements. |
| src/WinGetMCPServer/Program.cs | Adds client guidance for handling consent. |
| src/WinGetMCPServer/LocalizedStrings.cs | Loads shared WinGet localized strings. |
| src/WinGetMCPServer/Agreements/SourceAgreementConsent.cs | Selects catalogs based on source consent. |
| src/WinGetMCPServer/Agreements/PackageAgreementConsent.cs | Retrieves package terms and prompt text. |
| src/WinGetMCPServer/Agreements/AgreementConsent.cs | Implements elicitation and fallback consent handling. |
| src/Microsoft.Management.Deployment/PackageManager.idl | Defines contract 31 agreement APIs. |
| src/Microsoft.Management.Deployment/PackageCatalogReference.h | Declares source agreement methods. |
| src/Microsoft.Management.Deployment/PackageCatalogReference.cpp | Implements agreement queries and persistence. |
| src/Directory.Packages.props | Adds the MCP Core dependency version. |
| src/AppInstallerCLIE2ETests/Test.runsettings | Adds the MCP server launch parameter. |
| src/AppInstallerCLIE2ETests/README.md | Documents MCP test configuration. |
| src/AppInstallerCLIE2ETests/MCPServerTests.cs | Tests MCP tools and consent flows. |
| src/AppInstallerCLIE2ETests/Interop/SourceAgreementsInterop.cs | Tests COM agreement state and persistence. |
| src/AppInstallerCLIE2ETests/Helpers/TestSetup.cs | Reads the MCP server launch parameter. |
| src/AppInstallerCLIE2ETests/Helpers/AgreementsRestSource.cs | Provides a configurable agreement source fixture. |
| src/AppInstallerCLIE2ETests/Constants.cs | Defines the MCP test parameter name. |
| src/AppInstallerCLIE2ETests/AppInstallerCLIE2ETests.csproj | References MCP Core for client tests. |
Review effort: Balanced
| progress.Report(CreateInstallProgressNotification(PackageInstallProgressState.Finished, 1.0, 1.0)); | ||
| findResult = ReFindForPackage(catalogPackage.DefaultInstallVersion); | ||
| case ConsentDecision.Accepted: | ||
| options.AcceptPackageAgreements = true; |
There was a problem hiding this comment.
I agree that this is a theoretical issue that will be practically non-existent right now. Fixing it requires quite a large change just to get the dependencies out and I don't want to add it to this PR. Will do a follow up.
📖 Description
Expose source agreement state through COM and leverage it in the MCP server to handle source and package agreements.
This leverages elicitation when available, which allows the server to ask the user to make a decision directly. When not available, unaccepted agreements are simply returned as a response so that the agent can present them / submit a new tool call with the agreements field set to
true.🔍 Validation
Add tests for the COM interface additions and MCP server tests.
Microsoft Reviewers: Open in CodeFlow