Add reviews support: read and translate Store reviews - #180
Add reviews support: read and translate Store reviews#180Alexandre Zollinger Chohfi (azchohfi) wants to merge 8 commits into
Conversation
Adds `msstore reviews list` and `msstore reviews get`, backed by the Microsoft Store analytics API (/v1.0/my/analytics/reviews). This reuses the existing DevCenter credential path in StorePackagedAPI, so no new authentication is needed. Responding to reviews is not implemented. Microsoft documents the Store reviews API as "currently not in a working state" and points to Partner Center instead, so a reply command would fail on every invocation. The Store returns no translated text and no language metadata for reviews, so `--translate` uses Azure AI Translator with a key the user supplies through MSSTORE_TRANSLATOR_KEY or `msstore settings set-translator-key`. The api-version and wire DTOs sit behind ITranslationService so moving off v3.0 stays contained. Every AppReview field is optional because the analytics API omits fields entirely rather than returning them as null, and `date` is kept as a string because the service returns a US-format value rather than ISO-8601. IEnvironmentInformationService gained GetEnvironmentVariable so the translator service can be tested without mutating process-wide environment state. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
There was a problem hiding this comment.
🟢 Approval recommended
The implementation is cohesive and well-tested, with only a minor user-facing wording issue noted in the reviews list empty-result message.
Pull request overview
Adds a new reviews feature area to msstore-cli, enabling users to list and fetch Microsoft Store analytics reviews for packaged (MSIX) apps, with optional on-demand translation via Azure AI Translator while keeping secrets in the OS credential store.
Changes:
- Introduces
msstore reviews list <productId>(Spectre table) andmsstore reviews get <productId> <reviewId>(JSON to stdout), plus shared--translate [lang]. - Adds an Azure AI Translator client (
ITranslationService) with batching, retry/backoff, and asettings set-translator-keycommand that stores secrets viaICredentialManager. - Extends
IStorePackagedAPI/StorePackagedAPIto call the DevCenter analytics reviews endpoint and models theAppReviewpayload.
File summaries
| File | Description |
|---|---|
| README.md | Documents new reviews commands and translator configuration options. |
| MSStore.CLI/StoreHostBuilderExtensions.cs | Registers new commands/handlers for DI and trimming safety. |
| MSStore.CLI/Services/Translation/TranslationSourceGenerationContext.cs | Adds STJ source-gen context for Translator wire DTOs. |
| MSStore.CLI/Services/Translation/TranslationResult.cs | Adds a small internal result model for translations. |
| MSStore.CLI/Services/Translation/TranslationException.cs | Adds a user-facing exception type for translation failures. |
| MSStore.CLI/Services/Translation/Models/TranslatorLanguagesResponse.cs | DTO for /languages response. |
| MSStore.CLI/Services/Translation/Models/TranslatorLanguage.cs | DTO for individual supported language metadata. |
| MSStore.CLI/Services/Translation/Models/TranslatorErrorResponse.cs | DTO wrapper for Translator error responses. |
| MSStore.CLI/Services/Translation/Models/TranslatorError.cs | DTO for Translator error object (numeric code). |
| MSStore.CLI/Services/Translation/Models/TranslationInfo.cs | DTO for a translation entry in Translator responses. |
| MSStore.CLI/Services/Translation/Models/TranslateResultItem.cs | DTO for one element in the translate response array. |
| MSStore.CLI/Services/Translation/Models/TranslateInput.cs | DTO for one element in the translate request array. |
| MSStore.CLI/Services/Translation/Models/DetectedLanguageInfo.cs | DTO for detected language info (double score). |
| MSStore.CLI/Services/Translation/ITranslationService.cs | Defines translation abstraction used by commands. |
| MSStore.CLI/Services/Translation/AzureAITranslatorService.cs | Implements Translator v3.0 client, batching, retries, and error mapping. |
| MSStore.CLI/Services/IEnvironmentInformationService.cs | Adds GetEnvironmentVariable for testability. |
| MSStore.CLI/Services/EnvironmentInformationService.cs | Implements GetEnvironmentVariable. |
| MSStore.CLI/Services/Configurations.cs | Persists non-secret translator region in settings. |
| MSStore.CLI/Program.cs | Wires translator service + named HttpClient into DI. |
| MSStore.CLI/MicrosoftStoreCLI.cs | Adds reviews as a top-level command. |
| MSStore.CLI/Helpers/ParseResultExtensions.cs | Adds GetTranslateLanguage() helper for --translate. |
| MSStore.CLI/Commands/SettingsCommand.cs | Adds set-translator-key subcommand under settings. |
| MSStore.CLI/Commands/Settings/SetTranslatorKeyCommand.cs | Implements secure storage/clearing of translator key + region. |
| MSStore.CLI/Commands/ReviewsCommand.cs | Adds reviews command and shared options/args (incl. --translate). |
| MSStore.CLI/Commands/Reviews/ReviewTranslator.cs | Translates review title/text in one batched call and populates fields. |
| MSStore.CLI/Commands/Reviews/ListCommand.cs | Implements review listing, filtering, table rendering, and translation. |
| MSStore.CLI/Commands/Reviews/GetCommand.cs | Implements fetching a single review and emitting JSON to stdout. |
| MSStore.CLI.UnitTests/ReviewsCommandUnitTests.cs | Adds command-level tests for list/get and translation behavior. |
| MSStore.CLI.UnitTests/BaseCommandLineTest.cs | Adds fake reviews + translation plumbing to test harness. |
| MSStore.CLI.UnitTests/AzureAITranslatorServiceUnitTests.cs | Adds unit tests covering translator client behavior and error mapping. |
| MSStore.API/SourceGenerationContext.cs | Adds STJ source-gen entries for AppReview and paged response. |
| MSStore.API/Packaged/StorePackagedAPI.cs | Adds GetAppReviewsAsync implementation using analytics endpoint. |
| MSStore.API/Packaged/Models/AppReview.cs | Adds reviews DTO (all-optional fields + CLI-populated translated fields). |
| MSStore.API/Packaged/IStorePackagedAPI.cs | Adds GetAppReviewsAsync to the packaged API interface. |
Review details
- Files reviewed: 34/34 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The message always claimed "for the requested period", even when no date option was passed. With no dates the CLI sends no date parameters and the service returns reviews from every date, so the wording implied a range that was never applied. It now reflects what actually narrowed the query, covering the --rating and --market filters as well as the dates. The date-narrowing check that `reviews get` already used moves to a shared ParseResultExtensions helper so both commands stay consistent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
There was a problem hiding this comment.
🔵 Needs a closer look
Negative paging values (--top/--skip) are not validated and can lead to avoidable service-side failures and confusing runtime behavior.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
MSStore.API/Packaged/StorePackagedAPI.cs:681
GetAppReviewsAsyncvalidatestoponly against the maximum, but negativetop/skipvalues can still be passed through and produce a service-side failure. Add range checks for negative paging values so callers get a clear, deterministic exception.
MSStore.CLI/Commands/Reviews/ListCommand.cs:103--topand--skipaccept numeric values but negative values aren’t rejected, so they can be sent to the analytics API and fail the request. Add explicit validation for negative paging values with a clear error message.
- Files reviewed: 34/34 changed files
- Comments generated: 0 new
- Review effort level: Lite
--top -5 and --skip -3 were passed straight through to the analytics API, which rejected them with an opaque "Error!" and no indication of which argument was wrong. Both are now checked in the command with a clear message, and guarded in GetAppReviewsAsync so any consumer of MSStore.API gets a deterministic ArgumentOutOfRangeException. The argument checks run before AssertClientInitialized so a bad value is always reported as such rather than depending on initialization order. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
There was a problem hiding this comment.
🟢 Approval recommended
The changes align with the PR description, keep stdout/stderr behavior consistent, and include substantial unit tests for both the new commands and the translator client.
Review details
- Files reviewed: 35/35 changed files
- Comments generated: 0 new
- Review effort level: Lite
ReviewsListCommandShouldReportWhenThereAreNoReviews passed locally but failed on all three CI platforms. Spectre only emits colour and style codes when the underlying stream negotiates ANSI support, which differs between a developer machine and CI, so on CI the rendered message was This application has \e[1;4mno\e[0m\e[1m reviews\e[0m. and the assertion on "This application has no reviews." could not match across the markup boundary. The other message variants passed only because their text happens to fall outside the markup. ParseAndInvokeAsync now strips escape sequences so assertions match the visible text regardless of how the console negotiates ANSI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
There was a problem hiding this comment.
🔵 Needs a closer look
There are a couple of user-facing correctness issues to address (notably trimming/persistence of translator key/region and a small README wording mismatch) before this is safe to merge.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
MSStore.CLI/Commands/Settings/SetTranslatorKeyCommand.cs:98
- The key and region are stored exactly as provided. If the user includes leading/trailing whitespace, the stored value will later be sent in request headers verbatim and cause hard-to-diagnose authentication failures. Trim the key/region before persisting them.
README.md:40 - The wording here implies both key and region are stored in the OS secure store, but the implementation stores only the key there and saves the region in settings.json (Configurations.TranslatorRegion). Please adjust the sentence to avoid misleading users about where the region is persisted.
- Files reviewed: 35/35 changed files
- Comments generated: 0 new
- Review effort level: Lite
A key with a trailing newline, which is common when the value is pasted or piped in, is not valid in an HTTP header. The request threw before it was sent, so the user saw a bare "Error!" with no indication of the cause. Keys and regions are now trimmed both when read and before being persisted, and a whitespace-only key is treated as missing. The README also claimed both values go to the OS secure store. Only the key does; the region is not a secret and is saved in settings.json. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
There was a problem hiding this comment.
🔵 Needs a closer look
Translator error logging currently includes full response bodies at Error level, which can leak sensitive data into verbose/CI logs.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
MSStore.CLI/Services/Translation/AzureAITranslatorService.cs:362
- The Error-level logs include the full Translator response body (
content). When users run with --verbose (or in CI with logs collected), this can leak potentially sensitive data into logs. Consider logging only status/request-id at Error and moving the body to Debug (or truncating/redacting it).
- Files reviewed: 36/36 changed files
- Comments generated: 0 new
- Review effort level: Lite
The error log included the full Translator response body. A failed request can echo back the submitted review text, so collected CI logs could end up holding customer review content. Error level now carries only the status, the service error code and the X-RequestId, which is what a support case needs. The body moves to debug level. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
There was a problem hiding this comment.
🔵 Needs a closer look
The new ANSI-stripping regex in BaseCommandLineTest is too narrow and may still leave escape sequences in captured output, risking flaky test assertions across environments.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
MSStore.CLI.UnitTests/BaseCommandLineTest.cs:998
- The ANSI-stripping regex only matches CSI sequences of the form ESC + '[' + digits/';' + letter. Spectre can emit other CSI sequences (e.g., private-mode codes like ESC[?25l/ESC[?25h or sequences with intermediate bytes), which would remain in captured output and can still make string assertions flaky on different terminals/CI hosts.
- Files reviewed: 36/36 changed files
- Comments generated: 0 new
- Review effort level: Lite
The regex only matched CSI sequences made of digits and semicolons, so two kinds of escape Spectre actually emits survived: the cursor hide/show pair a status spinner writes (ESC[?25l and ESC[?25h), whose private parameter byte is not a digit, and the OSC hyperlinks produced by link markup. Both were verified to pass through unchanged. The pattern now follows the ECMA-48 grammar for CSI and OSC, and StripAnsi is covered by its own tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
There was a problem hiding this comment.
🟡 Changes recommended
The Translator JSON source-gen options currently force camelCase property names, which risks sending invalid request payloads to the Translator v3 endpoint.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 1
- Review effort level: Lite
…ming policy The text property was serialized by the shared camelCase policy, so the wire format depended on a setting shared with the response DTOs. The Translator reference is inconsistent about the name: the request body section states it is "Text" and the curl examples and official C# quickstart send that, while the JSON sample in the same section and the published Swagger use "text". The service accepts either. The name is now set explicitly on the property, matching the normative prose, and a test pins the serialized body so a future change to the naming policy cannot silently alter what is sent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, align with the PR description (read-only + external translation), and include thorough command/service/unit test coverage without introducing evident correctness or convention issues.
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 0 new
- Review effort level: Lite
Issue #179 asked for three things: read Store reviews, reply to them, and translate them. Before designing anything I checked whether official APIs actually exist for each, and only one of the three does.
GET /v1.0/my/analytics/reviewsPOST /v1.0/my/reviews/responsesWhat this adds
listsupports--startDate,--endDate,--top,--skip,--ratingand--market. Both support--translate [lang].Reading goes through a new
GetAppReviewsAsynconIStorePackagedAPI. The analytics API lives on the same host and scope thatStorePackagedAPIalready authenticates against, so this reuses the existing DevCenter credential path with no new auth work, and reusesPagedResponse<T>.Reply is deliberately not implemented
The reply API is documented, but Microsoft has flagged it as not working since July 2023, and the banner is still present on a page republished in March 2026. Reports show it returning
400 code:4012and later502. Areviews respondcommand would fail on every invocation, so this ships read-only and the README points at Partner Center instead. There is no replacement API: the Store submission API v2 has no reviews endpoints.Translation is external by necessity
The Store returns no translated text and no language metadata for reviews.
marketis an ISO 3166 country code, not a language, so it cannot stand in for one. Partner Center's translate toggle is UI only.So
--translateuses Azure AI Translator with a key the user supplies, viaMSSTORE_TRANSLATOR_KEY/MSSTORE_TRANSLATOR_REGIONormsstore settings set-translator-key, which stores the key throughICredentialManagerand keeps only the non-secret region insettings.json.It targets Translator v3.0 rather than the newer
2026-06-06GA, because v3.0 works with a plain Translator resource key while the newer version's headline features expect a Microsoft Foundry resource. Theapi-versionconstant and the wire DTOs sit behindITranslationServiceso that swap stays contained.Things worth a careful look
AppReviewfield is optional. The analytics API omits fields entirely rather than returning them as null. A live call against a real app confirmed this:DeviceModel,DeviceRAM,IsTouchEnabledandReviewTitleall came back absent.Dateis a string. The service returns a US-format value like5/17/2014 11:42:41 PM, not ISO-8601, so it is kept as a string to round-trip exactly.startDate/endDatedefault to the current date. In practice, omitting them sends no date parameters and the service returns every review. The option help and the "not found" message reflect the observed behaviour, not the documented one, and the message only suggests widening the range when the caller actually narrowed it.detectedLanguage.scoreis deserialized as a double. The published Translator Swagger types it as an integer, which is wrong.codeis a JSON number, not a string, so it is modelled asint.RetryAfterHttpHandler. That handler retries 429s forever with no attempt cap, and Translator does not document aRetry-Afterheader, so the service does its own capped exponential backoff and only honours the header opportunistically.IEnvironmentInformationServicegainedGetEnvironmentVariableso the translator service can be unit tested without mutating process-wide environment state.Verification
230 tests pass, including 37 new ones covering the commands and the translator client (batching, index alignment, region header handling and error-code mapping).
Also smoke tested end to end against a real Partner Center account: listing and getting real reviews, the
marketandratingfilters, the unauthenticated/languagesvalidation path, and the missing-key and bad-key error messages.Fixes: #179