Repository navigation
Add reviews support: read and translate Store reviews - #180
Open
Alexandre Zollinger Chohfi (azchohfi) wants to merge 27 commits into
Open
Alexandre Zollinger Chohfi (azchohfi) wants to merge 27 commits into
Alexandre Zollinger Chohfi (azchohfi) wants to merge 27 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
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 9, 2026 00:02
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
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 9, 2026 20:51
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
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 9, 2026 21:01
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
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 9, 2026 21:23
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
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 9, 2026 21:35
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
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 9, 2026 21:42
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
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 9, 2026 21:51
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
The table cut review titles and text at a fixed UTF-16 index, so an emoji or a letter with a combining accent that straddled the limit was cut in half, leaving an invalid or mangled character before the ellipsis. Emoji are common in reviews. Truncation now happens on text element boundaries, keeping each character whole or dropping it whole. It moves to TerminalText beside Sanitize so it can be tested directly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
…it reads Every command test mocks IStorePackagedAPI, so the query string construction and the deserialization of the analytics payload had no coverage. The new tests drive the real method against a stub handler. They check every parameter and that omitted ones are not sent, escaping of reserved characters in the product ID and filter, invariant date formatting under a culture whose default calendar is not Gregorian, and reading the payload as the service actually shapes it. InitAsync always fetches a real Entra token, and MSStore.API is strong-named, so the test plants the client through reflection rather than InternalsVisibleTo. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
Alexandre Zollinger Chohfi (azchohfi)
requested review from
Copilot
and removed request for
Copilot
September 28, 2026 22:13
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 28, 2026 22:14
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 28, 2026 22:15
…Error!"
The reviews commands mapped a 403 MSStoreHttpException to "check the
ProductId", but that branch never ran. Checked against the live service, an
unknown product ID and one owned by another account both return 401 with a
body ({"error":"User Unauthorized due to AMS call failure."}). A response with a
body is raised as a plain MSStoreException, not MSStoreHttpException, and the
status was not 403 either, so the most likely user mistake printed a bare
"Error!".
MSStoreException gains an optional StatusCode, set where SubmissionClient
raises it for an error that has a body. Nothing else reads it, so existing
handling is unchanged. The reviews commands now read the status either way and
say the product ID may be wrong or belong to another account for 401 and 403.
Other statuses keep a generic message, and the status is included in the
--verbose log.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
A throttled or unavailable Translator response's Retry-After was honoured whatever its size, so a header asking for an hour, or naming a date a day away, would leave the command waiting that long with nothing on screen. Waits of up to 30 seconds are still honoured. Anything longer fails straight away with the throttling message, and backoff without the header is unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 28, 2026 22:30
Each result item was mapped with item.Translations, so a null item in the response array threw a NullReferenceException, bypassing translation error handling and printing a bare "Error!". An item with no translation quietly left that review untranslated after one was asked for. Both now fail with a TranslationException, as a response with the wrong number of items already does. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 28, 2026 22:40
The X-RequestId comes from a response header. It was sanitized when quoted in the error message but logged raw just above, so with --verbose a control character in it reached the terminal. It is now sanitized once where it is read, and both the log and the message use that value. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 28, 2026 22:50
…y stays `--clear` removed the key first, so if saving settings.json then failed, the key was gone for good while the command reported failure. Removing it cannot be undone, so it now happens only after the settings change is saved, which is the order the reconfigure flow already uses. If the key then cannot be removed, or the credential store cannot be read to confirm it was, the region is restored, so a clear that fails leaves the configuration as it found it. This matches the rollback the store path already has. The partial-failure message remains only for the case where that restore also fails. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
It told the user to "Try a smaller --top value", but `reviews get` translates too and has no --top, and it described only the free tier's limit. It now says to wait and retry, notes that every tier limits characters per hour, and mentions --top only for `reviews list`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 28, 2026 23:00
The services send the next-page link as "@nextlink", which no naming policy produces from "NextLink", so the property was always null. That included the page returned by GetAppReviewsAsync on the public IStorePackagedAPI, which reported that no more pages existed even when they did. It is now mapped explicitly. Nothing in the CLI reads NextLink (the reviews paging hint uses TotalCount and --skip), so no command changes behavior; the returned model is simply accurate now. `apps list` and `reviews list` were re-checked against the live service. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12fc293b-ef9f-4c54-aeec-be90ae079251
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
September 28, 2026 23:10
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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