Add a --metadata-dir option to asc publish appstore - #1485
Merged
rudrankriyam merged 4 commits intoMay 8, 2026
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds --metadata-dir to asc publish appstore: loads locale JSONs under the version directory, validates and uploads version localizations, invokes the upload after ensuring/creating the App Store version and before attaching the build, and surfaces an apply_metadata step in dry-run plans. Tests and test-helper changes accompany the feature. ChangesApp Store Publish Metadata Support
Sequence DiagramsequenceDiagram
participant User as User / CLI
participant Publish as Publish Command
participant Metadata as Metadata Loader
participant ASC as App Store Connect
participant Build as Build Manager
User->>Publish: publish appstore --metadata-dir=.../1.2.3
Publish->>ASC: FindOrCreateAppStoreVersion
ASC-->>Publish: VersionID
Publish->>Metadata: applyPublishVersionMetadata(VersionID, Dir)
Metadata->>Metadata: loadPublishVersionMetadataValues(en-US, fr-FR, ...)
Metadata->>ASC: UploadVersionLocalizationsWithWarnings(locales)
ASC-->>Metadata: results
Metadata-->>Publish: results
Publish->>Build: LookupBuild / UploadBuild
Build-->>Publish: BuildID
Publish->>ASC: AttachBuild(VersionID, BuildID)
ASC-->>Publish: success
Publish-->>User: ✓ Published with metadata
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rudrankriyam
marked this pull request as draft
May 4, 2026 19:44
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/cli/publish/publish.go (1)
454-459:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject explicitly empty
--metadata-dirvalues.Right now
--metadata-dir=or a whitespace-only value is accepted, trimmed to"", and then treated as if the flag was never passed. That makes a changed user-facing flag a silent no-op, and the dry-run plan will also omitapply_metadatainstead of failing fast.Suggested fix
buildNumberValue := strings.TrimSpace(*buildNumber) metadataDirValue := strings.TrimSpace(*metadataDir) + if setFlags["metadata-dir"] && metadataDirValue == "" { + return shared.UsageError("--metadata-dir requires a non-empty path") + } localBuildMode := localBuild.localBuildMode()As per coding guidelines, "Never silently ignore accepted flags; unsupported values must return an error" and "For every new or changed flag, add one valid-path test and one invalid-value test that asserts stderr and exit code
2".Also applies to: 560-562
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/publish/publish.go` around lines 454 - 459, Currently an explicitly passed empty or whitespace-only --metadata-dir is trimmed to "" and treated like the flag was not passed; change the logic around metadataDir handling so that if the metadataDir flag was provided but strings.TrimSpace(*metadataDir) yields an empty string you return a user error (exit code 2) instead of silently ignoring it. Locate the metadataDir usage near collectSetFlags(), ipaPath, version, buildNumber and localBuild.localBuildMode() and add a check after metadataDirValue := strings.TrimSpace(*metadataDir) to detect the flag presence (the fs/flag API or the surrounding flag set handling) and error on empty value; apply the same check to the similar block referenced at lines ~560-562 so both code paths fail fast when --metadata-dir is explicitly empty. Ensure tests are added: one valid-path and one invalid-value test asserting stderr and exit code 2 per guidelines.cmd/exit_codes_test.go (1)
468-507:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd test for invalid/missing
--metadata-dirpath.The
--metadata-dirflag has a valid-path test (TestPublishAppStoreMetadataDirAppliesAfterEnsureVersionBeforeAttachinpublish_local_build_test.goline 855), but lacks a test verifying behavior when the directory does not exist or is inaccessible. The error-handling code exists ininternal/cli/metadata/push.go(lines 206–208, 245–247) but is not exercised by tests. Add a test that passes a non-existent path and asserts the expected error message in stderr.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/exit_codes_test.go` around lines 468 - 507, Add a new test modeled on TestPublishAppStoreDryRunInvalidBooleanExitCode that builds the CLI binary and runs the "publish appstore" command with "--metadata-dir" pointing at a non-existent or inaccessible path; assert that the process exits non-zero (compare to ExitUsage) and that stderr contains both "metadata-dir" and the expected error substring produced by internal/cli/metadata/push.go (the error handling around EnsureVersionBeforeAttach / push logic). Use the same pattern of building the binary, setting runCmd.Env via isolatedCLITestEnv, checking errors.As to *exec.ExitError, and validating exitErr.ExitCode() and stderr contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/publish/publish.go`:
- Around line 405-406: The metadata-enabled publish flow can fail when
metadata.ExecutePush needs an explicit App Info ID but none is available; add a
CLI flag (e.g., "app-info-id") alongside metadataDir and submit flags, parse it
into a variable (appInfoID) and thread that value into the code paths that call
metadata.ExecutePush so it can be used when GetAppInfos returns multiple
candidates; update all publish invocations that touch metadata (the places that
call metadata.ExecutePush and related helpers referenced in this file, including
the other similar blocks noted) to pass appInfoID through to the ExecutePush
call and any intermediate helper functions so the metadata command's recovery
path is available during publish.
---
Outside diff comments:
In `@cmd/exit_codes_test.go`:
- Around line 468-507: Add a new test modeled on
TestPublishAppStoreDryRunInvalidBooleanExitCode that builds the CLI binary and
runs the "publish appstore" command with "--metadata-dir" pointing at a
non-existent or inaccessible path; assert that the process exits non-zero
(compare to ExitUsage) and that stderr contains both "metadata-dir" and the
expected error substring produced by internal/cli/metadata/push.go (the error
handling around EnsureVersionBeforeAttach / push logic). Use the same pattern of
building the binary, setting runCmd.Env via isolatedCLITestEnv, checking
errors.As to *exec.ExitError, and validating exitErr.ExitCode() and stderr
contents.
In `@internal/cli/publish/publish.go`:
- Around line 454-459: Currently an explicitly passed empty or whitespace-only
--metadata-dir is trimmed to "" and treated like the flag was not passed; change
the logic around metadataDir handling so that if the metadataDir flag was
provided but strings.TrimSpace(*metadataDir) yields an empty string you return a
user error (exit code 2) instead of silently ignoring it. Locate the metadataDir
usage near collectSetFlags(), ipaPath, version, buildNumber and
localBuild.localBuildMode() and add a check after metadataDirValue :=
strings.TrimSpace(*metadataDir) to detect the flag presence (the fs/flag API or
the surrounding flag set handling) and error on empty value; apply the same
check to the similar block referenced at lines ~560-562 so both code paths fail
fast when --metadata-dir is explicitly empty. Ensure tests are added: one
valid-path and one invalid-value test asserting stderr and exit code 2 per
guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2145feac-8cb6-410f-8fa2-83e353990d38
📒 Files selected for processing (76)
apps/studio/app_helpers.gocmd/age_rating_run_test.gocmd/exit_codes_test.gointernal/asc/beta_license_agreements_test.gointernal/asc/client_app_metadata_test.gointernal/asc/client_core.gointernal/asc/client_http.gointernal/asc/client_http_app_clips_test.gointernal/asc/client_http_test.gointernal/asc/client_http_webhooks_test.gointernal/asc/client_pass_type_ids_test.gointernal/asc/client_publish.gointernal/asc/customer_review_summarizations_test.gointernal/asc/pricing_test.gointernal/asc/sandbox.gointernal/asc/sandbox_test.gointernal/asc/table_render.gointernal/asc/upload_test.gointernal/auth/integration_test.gointernal/cli/analytics/analytics_instances.gointernal/cli/analytics/analytics_reports.gointernal/cli/analytics/analytics_requests.gointernal/cli/androidiosmapping/android_ios_mapping.gointernal/cli/apps/app_registry.gointernal/cli/apps/apps.gointernal/cli/auth/auth.gointernal/cli/builds/build_test_notes.gointernal/cli/builds/builds_commands.gointernal/cli/builds/builds_individual_testers.gointernal/cli/builds/builds_related.gointernal/cli/builds/builds_relationships.gointernal/cli/builds/builds_uploads.gointernal/cli/certificates/certificates.gointernal/cli/encryption/encryption.gointernal/cli/iap/setup.gointernal/cli/metadata/push.gointernal/cli/performance/performance_download.gointernal/cli/performance/performance_metrics.gointernal/cli/pricing/tiers.gointernal/cli/profiles/profiles.gointernal/cli/publish/local_build.gointernal/cli/publish/publish.gointernal/cli/publish/publish_local_build_test.gointernal/cli/reviews/review_attachments.gointernal/cli/reviews/review_items.gointernal/cli/reviews/review_submissions.gointernal/cli/reviews/reviews.gointernal/cli/reviews/reviews_summarizations.gointernal/cli/sandbox/sandbox_helpers.gointernal/cli/shared/build_wait.gointernal/cli/shared/command_builders.gointernal/cli/shared/test_notes.gointernal/cli/shared/tier_resolver.gointernal/cli/signing/signing_fetch.gointernal/cli/status/status.gointernal/cli/submit/submit.gointernal/cli/subscriptions/pricing_equalize.gointernal/cli/subscriptions/setup.gointernal/cli/testflight/beta_groups.gointernal/cli/testflight/beta_groups_relationships.gointernal/cli/testflight/beta_testers.gointernal/cli/testflight/beta_testers_related.gointernal/cli/testflight/beta_testers_relationships.gointernal/cli/testflight/command_wrappers.gointernal/cli/testflight/testflight_review.gointernal/cli/web/web_apps.gointernal/cli/web/web_auth.gointernal/cli/xcode/build_upload_lookup.gointernal/cli/xcode/xcode.gointernal/cli/xcodecloud/xcode_cloud_action_resources_helpers.gointernal/cli/xcodecloud/xcode_cloud_extras.gointernal/cli/xcodecloud/xcode_cloud_list_helpers.gointernal/cli/xcodecloud/xcode_cloud_workflows.gointernal/validation/legal_test.gointernal/web/auth.gointernal/xcode/xcode_test.go
Collaborator
|
I have converted it to draft, and wait for you to do a end-to-end test for it; in the meanwhile, will sort the formatter so this PR is better reviewable! |
justinseanmartin
force-pushed
the
jmartin/publish-with-metadata
branch
from
May 4, 2026 23:29
baca54c to
dda3722
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/cli/publish/publish_local_build_test.go (1)
926-928: ⚡ Quick winCapture and assert
cmd.Execerrors explicitly in both new tests.Both call sites currently ignore execution errors via
capturePublishCommandOutput. Adding explicitrunErrchecks will prevent false positives from partial/leftover output paths.Suggested change pattern
- stdout, _ := capturePublishCommandOutput(t, func() error { - return cmd.Exec(context.Background(), nil) - }) + var runErr error + stdout, _ := capturePublishCommandOutput(t, func() error { + runErr = cmd.Exec(context.Background(), nil) + return runErr + }) + if runErr != nil { + t.Fatalf("Exec() error: %v", runErr) + } @@ - stdout, _ := capturePublishCommandOutput(t, func() error { - return cmd.Exec(context.Background(), nil) - }) + var runErr error + stdout, _ := capturePublishCommandOutput(t, func() error { + runErr = cmd.Exec(context.Background(), nil) + return runErr + }) + if runErr != nil { + t.Fatalf("Exec() error: %v", runErr) + }Also applies to: 963-965
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/publish/publish_local_build_test.go` around lines 926 - 928, The test currently swallows errors from cmd.Exec by calling capturePublishCommandOutput(t, func() error { return cmd.Exec(...) }) and ignoring the returned error; update the two failing test sites (where capturePublishCommandOutput is called around the cmd.Exec invocation) to capture the exec error into a variable (e.g., runErr := ...) and add an explicit assertion that runErr is nil (or fail the test with t.Fatalf/t.Errorf including runErr) immediately after capturing stdout so test failures surface actual Exec errors instead of relying on output content alone.internal/cli/publish/version_metadata_test.go (1)
60-63: ⚡ Quick winAssert the failure reason, not just
err != nil.Right now this can pass on unrelated errors (permissions, path issues, parse issues). Tightening the assertion to the expected missing-files message makes the test regression-resistant.
Suggested test hardening
import ( "os" "path/filepath" + "strings" "testing" ) @@ _, err := loadPublishVersionMetadataValues(dir, "1.2.3") if err == nil { t.Fatal("expected missing version metadata JSON files to fail") } + if !strings.Contains(err.Error(), "no version metadata JSON files found") { + t.Fatalf("expected missing-files error, got %v", err) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/publish/version_metadata_test.go` around lines 60 - 63, Update the test to assert the specific failure reason from loadPublishVersionMetadataValues instead of only checking err != nil: call loadPublishVersionMetadataValues(dir, "1.2.3"), require err != nil, then assert the error text contains the expected missing-files message (e.g. use strings.Contains(err.Error(), "missing version metadata") or compare against a sentinel error if the package exposes one). Modify Test code around loadPublishVersionMetadataValues to import "strings" (or use errors.Is with the package's ErrMissingVersionMetadata) and replace the broad t.Fatal("expected ...") with a targeted assertion that verifies the error message/sentinel to ensure the failure is due to missing JSON files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/cli/publish/publish_local_build_test.go`:
- Around line 926-928: The test currently swallows errors from cmd.Exec by
calling capturePublishCommandOutput(t, func() error { return cmd.Exec(...) })
and ignoring the returned error; update the two failing test sites (where
capturePublishCommandOutput is called around the cmd.Exec invocation) to capture
the exec error into a variable (e.g., runErr := ...) and add an explicit
assertion that runErr is nil (or fail the test with t.Fatalf/t.Errorf including
runErr) immediately after capturing stdout so test failures surface actual Exec
errors instead of relying on output content alone.
In `@internal/cli/publish/version_metadata_test.go`:
- Around line 60-63: Update the test to assert the specific failure reason from
loadPublishVersionMetadataValues instead of only checking err != nil: call
loadPublishVersionMetadataValues(dir, "1.2.3"), require err != nil, then assert
the error text contains the expected missing-files message (e.g. use
strings.Contains(err.Error(), "missing version metadata") or compare against a
sentinel error if the package exposes one). Modify Test code around
loadPublishVersionMetadataValues to import "strings" (or use errors.Is with the
package's ErrMissingVersionMetadata) and replace the broad t.Fatal("expected
...") with a targeted assertion that verifies the error message/sentinel to
ensure the failure is due to missing JSON files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9eb514ad-c05b-484b-8f87-46acf832757a
📒 Files selected for processing (6)
cmd/exit_codes_test.gointernal/cli/publish/local_build.gointernal/cli/publish/publish.gointernal/cli/publish/publish_local_build_test.gointernal/cli/publish/version_metadata.gointernal/cli/publish/version_metadata_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cli/publish/publish.go
justinseanmartin
force-pushed
the
jmartin/publish-with-metadata
branch
from
May 5, 2026 16:05
dda3722 to
0e070b9
Compare
rudrankriyam
added
enhancement
Collaborator
|
@justinseanmartin I made some changes and I think I will merge for the next release soon |
rudrankriyam
marked this pull request as ready for review
May 8, 2026 00:03
Contributor
Author
|
Thanks, appreciate it. I still haven't had time to do the end-to-end verification yet, I'm probably still a week or two away from doing my actual AppStore publish for the next release, but I'll test it out tonight and then abort the submission to at least verify it works as expected. |
rudrankriyam
approved these changes
May 8, 2026
rudrankriyam
left a comment
rudrankriyam
left a comment
Collaborator
There was a problem hiding this comment.
Audited hard. Rebased onto latest main, verified metadata-dir validation and dry-run behavior, and full local guardrails plus GitHub checks are green.
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
2 participants
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.
Summary
This updates
asc publish appstore --metadata-dirto apply App Store version localization metadata during publish, after ensuring the target App Store version exists and before attaching/submitting the build.When
--metadata-diris provided, publish reads files under:metadata/version/<version>/*.jsonand applies any supported version-localization fields present in those files:
descriptionkeywordsmarketingUrlpromotionalTextsupportUrlwhatsNewOmitted fields are left unchanged. This allows App Store Connect to keep any metadata it copied forward when the new version was created, while still letting the publish command patch release-specific metadata such as “What’s New”.
Why This Direction
The publish flow should only touch metadata that belongs to the App Store version being published. Fields like
whatsNew,description,keywords, and support/marketing URLs all live onAppStoreVersionLocalization, so they can be updated directly frommetadata/version/<version>/*.json.We intentionally do not apply
metadata/app-info/*.jsonfrompublish appstore. App info localization is a broader app-level metadata sync concern and may require resolving an App Info ID when an app has multiple app info records. Pulling that into publish made the command more complex and introduced an--app-infoflag that is not needed for the release-metadata workflow.This keeps publish behavior patch-oriented:
This implements the behavior discussed in #1469 while keeping full metadata sync responsibilities with the dedicated metadata commands.
I have yet to do a full end-to-end test of this, as I'm not quite ready to ship another release of my app, but I'd be happy to hold off merging this until I've done an end-to-end test if that's preferable.
Validation
make formatmake check-docsmake lintASC_BYPASS_KEYCHAIN=1 make testSummary by CodeRabbit
New Features
--metadata-dirtopublish appstoreto apply version localization metadata from JSON files; applied after version creation and before build attachment.apply_metadatastep in the publish plan.Bug Fixes
--metadata-dirwith a clear usage error and stderr message.Tests