Let a manual listings run pick the mode, so the permission probe is reachable - #791
Merged
Conversation
--check-permissions shipped in #784 but nothing could invoke it: the workflow's only dispatch input was a dry_run boolean, so the one thing that answers "can this service account actually edit listings" was reachable only by downloading a private key and running the script by hand. Replace the boolean with a mode choice — dry-run (default), check-permissions, publish. A push to main still always publishes. The probe's exit code is a verdict rather than pass/fail, so the step reads it and annotates: 0 notice, 1 error naming the grant, anything else a warning saying the run reached no verdict and should be repeated. Guard the workflow against drifting from the CLI it calls: build_arg_parser() is now separate from main(), and the tests read the mode list straight out of the YAML and assert every mode maps to a real option string and has a case branch. Renaming a flag without updating the workflow used to surface only as a failed manual run against Play; it now fails the PR. Verified by mutation. The YAML is parsed by hand rather than with PyYAML to keep the suite stdlib-only, which is what lets the validate job install nothing. Also documents the constraint that bit us: workflow_dispatch does not appear until this file reaches the default branch, so the manual run is unavailable until the first promotion to main carries it there. 80 tests, up from 75.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #791 +/- ##
=========================================
Coverage 42.34% 42.34%
Complexity 226 226
=========================================
Files 267 267
Lines 31834 31834
Branches 3650 3650
=========================================
Hits 13479 13479
Misses 18097 18097
Partials 258 258
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
Exit code 1 can produce a false permission verdict, and the drift test does not inspect the workflow’s actual flag mapping.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds selectable manual Play listing modes, including permission checks, while preserving automatic publishing on main.
Changes:
- Replaces the dry-run checkbox with three execution modes.
- Handles permission-probe verdicts and annotations.
- Adds CLI/workflow drift tests and documentation.
File summaries
| File | Description |
|---|---|
docs/store-listings.md |
Documents modes and default-branch availability. |
.github/workflows/play-listings.yml |
Selects and executes the requested mode. |
.github/scripts/test_publish_listings.py |
Adds workflow/CLI consistency tests. |
.github/scripts/publish_listings.py |
Exposes argument-parser construction for tests. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Give the probe verdict codes nothing else can produce. Exit 1 was reachable from a denied patch, bad credentials, unpublishable metadata, and any uncaught exception, so the workflow annotated all four as "you are missing the Play Console grant" — sending someone to fix a permission that was never wrong. Now EXIT_DENIED=3 and EXIT_INCONCLUSIVE=4, with generic failures on EXIT_ERROR=1 and the workflow mapping each separately. 2 is skipped deliberately: argparse exits 2 on a usage error, so the old inconclusive code collided with a mistyped flag. Failures that never reach the grant test are now normalized to inconclusive rather than escaping as tracebacks: opening the edit and reading the listings are both guarded. Outside probe mode they still raise, so a real publish fails loudly. - Make the drift guard read the workflow's own flags. It compared MODE_FLAGS against the parser — the table against itself — so a typo in the YAML (args+=(--check-permissons)) passed every test while the manual run failed. The case arms are now parsed out of the run step and checked against both the table and the parser's real option strings. Verified by mutation: that exact typo now fails two tests. 88 tests, up from 80.
Every error message, comment, and doc line said "Edit store listing, pricing & distribution". That label does not appear anywhere in the current Play Console permission list — someone following a failed probe would go hunting for a checkbox that no longer exists. The permission is now "Manage store presence", under the Store presence heading: "Edit your store listing and run store listing experiments; edit pricing; manage in-app products; edit distribution information and content ratings...". Same grant, current name. Confirmed against the live permission list for this app's Play Console, not from memory.
The prerequisite section asserted PLAY_SERVICE_ACCOUNT_JSON was set up for releases only and implied the first listing publish would 403. Checked against the app's Play Console permissions: "Manage store presence" is already ticked for FT8AF, so that was wrong. Keep the section — the grant is not implied by the release permission, and revoking it would break listing publishes while leaving releases working, which is a confusing failure — but state that it is granted rather than missing, and frame the probe as the check for when something changes rather than a step standing between here and the first publish.
The first real publish failed on the first locale: HTTPError: 404 Client Error: Not Found for url: .../edits/11895923857284451829/listings/ar Only en-US existed on the store, and PATCH is an update — the API has nothing to patch for a language with no listing yet. Choosing PATCH to preserve an existing promo video was right for updates and wrong for the 17 locales this change exists to create. Every test used a fake session that happily accepted a PATCH for an absent locale, so the suite agreed with the bug. upsert_listing() now PATCHes when Play already has the language and PUTs when it does not, with the language field in the PUT body since PUT replaces the whole resource. Nothing is lost by replacing: it only runs for languages that have never had a listing. run_check uses the same path, so the probe no longer 404s against a store whose only listing is the default language. The fake session grew a put() so the distinction is testable, and the two tests that asserted a PATCH for an absent locale — encoding the bug — now assert a create. Added a regression test for the exact store state that broke: en-US live, everything else missing. Nothing was published by the failed run; the edit was abandoned uncommitted. 93 tests, up from 88.
There was a problem hiding this comment.
🟡 Changes recommended
HTTP 401 responses are still incorrectly reported as proof of a missing Play Console grant.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
- Only a 403 is a verdict about the grant. A 401 means the access token was not accepted, so the probe never reached the permission check; report it as INCONCLUSIVE with a credentials hint instead of sending someone to Play Console to fix a grant that was never tested. Test, constants comment, docs table and the workflow's exit-4 message updated. - The probe can PUT when Play has no listings, so the docstring, the log line, the workflow notice and docs no longer claim it always PATCHes; the log names the verb it actually used, and the tests assert it. - Dry-run tests now also assert nothing was PUT, so a regression that created missing locales during --dry-run cannot slip past. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnrELYBhBMQ9RAmQ9cQoyT
There was a problem hiding this comment.
🟡 Changes recommended
The central workflow annotation mapping lacks coverage, and operator-facing descriptions remain inconsistent with the implementation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Balanced
…#791) - Move the probe's exit-code -> GitHub annotation mapping out of the workflow's bash `case` into publish_listings.probe_annotation(), relayed by a new --annotate-verdict RC flag (prints the ::level::message line, exits 0; the step still exits with the probe's own code). It is now unit tested per code — level, message, and that a non-verdict never names the grant — and a drift test asserts the workflow delegates to it and keeps no bash copy. - The CLI help and the workflow's mode-input comment no longer say the probe always PATCHes; both describe the PUT-on-empty-store path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnrELYBhBMQ9RAmQ9cQoyT
There was a problem hiding this comment.
🟡 Changes recommended
The annotation helper can bypass mode validation and report a verdict without running the requested probe.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
…ode (PR #791) The early return for --annotate-verdict ran before the mode-exclusivity check, so `--check-permissions --annotate-verdict 3` skipped the probe and printed a caller-supplied denial with exit 0. The flag is now listed with the modes in that check, so any such combination is an argparse usage error (exit 2) before anything runs. Test covers all three modes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnrELYBhBMQ9RAmQ9cQoyT
…utcomes (PR #791) The docstring called only 3 and 4 verdicts and everything else "never ran", which misclassified a successful probe (0 is the positive verdict) and contradicted 4 meaning no verdict. It now states the three outcomes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnrELYBhBMQ9RAmQ9cQoyT
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.
Follow-up to #784.
Why
--check-permissionsshipped in #784, but nothing could actually invoke it. The workflow's onlyworkflow_dispatchinput was adry_runboolean, so the one command that answers "can this service account edit listings?" was reachable only by downloading a private key and running the script by hand — which is exactly what you'd want CI to do for you, since the secret already lives there.That's the gap this closes.
What
modechoice input replacing the boolean:dry-run(default),check-permissions,publish. A push tomainstill always publishes.The probe writes one listing, it does not always PATCH.
listings.patch404s on a language Play has never had a listing for, so on an app with no listings yet the probe (and a first publish)PUTs instead. The log names whichever call it made.The probe's exit code is a verdict, not pass/fail, so the step reads it instead of letting
set -eflatten the distinction. The codes are chosen not to collide with anything else the script or argparse can produce (2 is skipped on purpose):The code → annotation mapping lives in the script (
probe_annotation, relayed by--annotate-verdict) rather than in a bashcase, so it is unit-tested against theEXIT_*constants and a drift test asserts the workflow delegates to it.Drift guard.
build_arg_parser()is now separate frommain(), and the tests read the mode list straight out of the YAML, asserting every mode maps to a real option string and has acasebranch. Renaming a flag without updating the workflow previously surfaced only as a failed manual run against Play; it now fails the PR. Verified by mutation — renaming--check-permissionsto--verify-permissionsfails with:The YAML is parsed by hand rather than with PyYAML, keeping the suite stdlib-only — which is what lets the
validatejob install nothing and run on forks.Tests
103, up from 75. They cover the modes offered, their mapping to real flags, the
casecoverage, the read-only default, the parser accepting each flag, the 401/403 split, the PUT-on-empty-store path (and that a dry run PUTs nothing either), each exit code's annotation, and the workflow's delegation of that annotation to the script.Also documented
The constraint that prompted this:
workflow_dispatchdoes not appear until the workflow file reaches the default branch.play-listings.ymlis currently only ondev, so there is no "Run workflow" button yet, and there won't be until the first promotion carries it tomain. Worth knowing before anyone goes looking for it.🤖 Generated with Claude Code