Repository navigation
Auto-submit external TestFlight beta reviews - #7464
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a marketing-version override for iOS TestFlight uploads, routes external uploads through automatic beta app review submission handling, persists assignment state through the workflow, expands tests for the new control flow, and updates related docs and help text. ChangesBeta App Review Auto-Submission
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow as ios-testflight.yml
participant AssignScript as asc_assign_external_testflight_group.py
participant ASC as App Store Connect API
Workflow->>AssignScript: assign uploaded build
AssignScript->>ASC: read build beta detail
alt READY_FOR_BETA_SUBMISSION
AssignScript->>ASC: create or reuse beta app review submission
AssignScript-->>Workflow: assignment_state = pending or complete
else already active
AssignScript-->>Workflow: assignment_state = complete
else unexpected state
AssignScript-->>Workflow: raise RuntimeError
end
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -321,6 +419,8 @@ def main() -> int: | |||
| f"asc_assign_external_testflight_group: assigned build {args.build_number} to " | |||
| f"{_describe_group(target_group)}" | |||
| ) | |||
| token = _token() | |||
| _ensure_external_review_submission(token, build_id, args.build_number) | |||
| return 0 | |||
There was a problem hiding this comment.
_token() call: in the two early-return paths the token is refreshed once and reused for _ensure_external_review_submission, but in the _assign_build path a second token is minted immediately after the first — before _assign_build makes a single fast HTTP call. _assign_build finishes in under a second, so the first token (valid for 600 s) is still fresh when _ensure_external_review_submission starts. The extra _token() invocation unnecessarily spawns a second openssl dgst subprocess and creates two throw-away JWTs.
| token = _token() | |
| _assign_build(token, target_group["id"], build_id) | |
| print( | |
| f"asc_assign_external_testflight_group: assigned build {args.build_number} to " | |
| f"{_describe_group(target_group)}" | |
| ) | |
| _ensure_external_review_submission(token, build_id, args.build_number) | |
| return 0 |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if external_state in ("READY_FOR_BETA_TESTING", "IN_BETA_REVIEW", "WAITING_FOR_BETA_REVIEW"): | ||
| print( | ||
| "asc_assign_external_testflight_group: build " | ||
| f"{build_number} external state is {external_state}" | ||
| ) | ||
| return | ||
|
|
||
| submission = _beta_review_submission(token, build_id) | ||
| if submission is not None: | ||
| print( | ||
| "asc_assign_external_testflight_group: build " | ||
| f"{build_number} external state is {external_state or 'unknown'} with submission " | ||
| f"{submission['id']} (state={submission['beta_review_state'] or 'unknown'})" | ||
| ) | ||
| return | ||
|
|
||
| raise RuntimeError( | ||
| "build " | ||
| f"{build_number} is in unexpected externalBuildState={external_state or '<empty>'} " | ||
| "with no beta app review submission" | ||
| ) |
There was a problem hiding this comment.
APPROVED / other intermediate states in the whitelist
_ensure_external_review_submission only explicitly passes through READY_FOR_BETA_TESTING, IN_BETA_REVIEW, and WAITING_FOR_BETA_REVIEW. Any other externalBuildState that doesn't carry an existing submission will raise a fatal RuntimeError. Two edge cases worth covering: (1) EXPIRED — a build older than 90 days has no submission, so a delayed or retried CI run would fail with an "unexpected externalBuildState" error. (2) PROCESSING_EXCEPTION — Apple can set this even after processingState=VALID in transient failure scenarios; the current code would permanently fail that CI run rather than allowing a retry. Both are fail-loud by design, but logging a warning and returning instead of raising for EXPIRED and exception states would avoid permanently poisoning those runs.
![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
🤖 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 `@tests/test_ios_testflight_external_distribution.py`:
- Around line 92-135: Add test coverage for the approved idempotent path in the
external review flow by extending the existing tests around
_ensure_external_review_submission, _build_beta_detail, and
_beta_review_submission. Stub external_build_state as BETA_APPROVED (and
optionally IN_BETA_TESTING) with both no existing submission and an existing
submission, and assert the function neither raises nor calls _submit_beta_review
again. Keep the new checks alongside the current submission, dedup, skip, and
failure cases so the intended already-approved behavior is locked in.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 948fb75b-99c7-418a-b528-850d8d1aadba
📒 Files selected for processing (5)
.github/workflows/ios-testflight.ymlios/README.mdios/scripts/asc_assign_external_testflight_group.pyios/scripts/upload-testflight.shtests/test_ios_testflight_external_distribution.py
| except RuntimeError as exc: | ||
| submission = _beta_review_submission(token, build_id) | ||
| if submission is not None: | ||
| review_state = submission["beta_review_state"] | ||
| if review_state in ACTIVE_BETA_REVIEW_STATES: | ||
| print( | ||
| "asc_assign_external_testflight_group: build " | ||
| f"{build_number} external state is READY_FOR_BETA_SUBMISSION with submission " | ||
| f"{submission['id']} (state={review_state or 'unknown'})" | ||
| ) | ||
| return | ||
| raise RuntimeError( | ||
| "build " | ||
| f"{build_number} external state is READY_FOR_BETA_SUBMISSION with beta app review " | ||
| f"submission {submission['id']} in unexpected betaReviewState={review_state or '<empty>'}" | ||
| ) | ||
| sibling_submission = _find_active_review_submission_on_sibling_build(token, build_id) |
There was a problem hiding this comment.
RuntimeError breaks the retry loop on double transient failure
Inside the except RuntimeError handler for a failed _submit_beta_review, the recovery calls to _beta_review_submission (line 438) and _find_active_review_submission_on_sibling_build (line 453) are themselves unprotected. If either raises a transient RuntimeError (e.g., ASC returns 5xx during an outage), it escapes the inner except block entirely — bypassing the deadline check, time.sleep, and retry logic — and propagates up to kill the CI job. The outer try/except at the top of the loop only guards _build_beta_detail and the initial _beta_review_submission, not these recovery calls. Wrapping those two calls in their own try/except RuntimeError (treating a failure the same as "nothing found yet" and continue-ing after the deadline check) would close the gap.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@ios/scripts/asc_assign_external_testflight_group.py`:
- Around line 434-464: The submit-recovery lookups in
asc_assign_external_testflight_group.py can fail transiently and escape the
retry loop; wrap the _beta_review_submission() and
_find_active_review_submission_on_sibling_build() calls inside the existing
try/except retry path in the submission loop so RuntimeError from those helpers
is treated like _submit_beta_review() failures, preserving the sleep, token
refresh, and continue behavior before the deadline.
- Around line 39-47: The state sets for the external TestFlight flow are missing
approved terminal values, so reruns on already-approved builds are not treated
as idempotent success. Update the state handling in
asc_assign_external_testflight_group.py by adding the approved external build
states and beta review state to the existing ACTIVE_EXTERNAL_BUILD_STATES and
ACTIVE_BETA_REVIEW_STATES checks, using the existing assignment/short-circuit
logic in asc_assign_external_testflight_group so those cases return early as
already-complete.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25e03cc3-f995-4726-8464-36f762b3e8c5
📒 Files selected for processing (3)
.github/workflows/ios-testflight.ymlios/scripts/asc_assign_external_testflight_group.pytests/test_ios_testflight_external_distribution.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/ios-testflight.yml:
- Line 531: Move CMUX_TESTFLIGHT_ASSIGN_STATE_OUT_FILE out of the
jobs.assign-external-group env block and into the “Assign uploaded build to the
external beta group” step scope so runner.temp resolves correctly. Update the
relevant step-level env for the assign flow, keeping the state file path tied to
runner.temp where it is available, and leave the rest of the workflow env
unchanged.
In `@ios/scripts/asc_assign_external_testflight_group.py`:
- Around line 458-489: The retry loop in asc_assign_external_testflight_group.py
is swallowing the deliberate RuntimeError raised for an unexpected
betaReviewState inside the submission check, causing terminal states to be
retried as if they were transient lookup failures. Update the try/except around
_beta_review_submission and _find_active_review_submission_on_sibling_build so
that only recovery/lookup errors are handled by the inner except RuntimeError as
recovery_exc, and let the unexpected betaReviewState branch propagate
immediately. Keep the existing current_build_review_pending and
sibling_review_pending paths unchanged, but make sure the RuntimeError from the
unexpected betaReviewState in this block is not converted into last_submit_error
or retried.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d7dcf46-da62-400d-b08c-390aa2f39888
📒 Files selected for processing (3)
.github/workflows/ios-testflight.ymlios/scripts/asc_assign_external_testflight_group.pytests/test_ios_testflight_external_distribution.py
| SHOULD_ASSIGN_ONLY: ${{ needs.decide.outputs.should_assign_only }} | ||
| LAST_UPLOADED_RUN_ID: ${{ needs.decide.outputs.last_uploaded_run_id }} | ||
| BUILD_NUMBER: ${{ needs.upload.outputs.final_build_number }} | ||
| CMUX_TESTFLIGHT_ASSIGN_STATE_OUT_FILE: ${{ runner.temp }}/ios-testflight-assign-state.txt |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section around the reported line and the artifact upload path.
nl -ba .github/workflows/ios-testflight.yml | sed -n '500,590p'Repository: manaflow-ai/cmux
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the workflow file around the reported line and surrounding job structure.
nl -ba .github/workflows/ios-testflight.yml | sed -n '500,590p'Repository: manaflow-ai/cmux
Length of output: 194
🌐 Web query:
GitHub Actions runner context job-level env available runner.temp documentation
💡 Result:
The runner context is not available within the job-level env clause [1]. Because the env block is processed before the job is sent to the runner, you cannot use context expressions like ${{ runner.temp }} at that level [2][3]. Instead, runner-specific information such as RUNNER_TEMP is provided as a default environment variable available directly to the shell [4][5]. You can access it using standard shell syntax (e.g.,
Citations:
- 1: BTCDecoded/blvm-primitives@3168115
- 2: https://github.com/actions/runner/blob/main/docs/adrs/0278-env-context.md
- 3: https://docs.github.com/en/actions/how-tos/write-workflows/choose-what-workflows-do/use-variables
- 4: Clarify Environment Variable Context Differences Between Self-Hosted and GitHub-Hosted Runners github/docs#40410
- 5: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/variables.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/ios-testflight.yml')
lines = p.read_text().splitlines()
for start,end in [(520,580)]:
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: manaflow-ai/cmux
Length of output: 3095
Move CMUX_TESTFLIGHT_ASSIGN_STATE_OUT_FILE to step scope
runner.temp doesn’t resolve in jobs.assign-external-group.env, so this becomes /ios-testflight-assign-state.txt and the assign step can’t write/read the state file. Put it on the Assign uploaded build to the external beta group step, where runner.temp is available.
🧰 Tools
🪛 actionlint (1.7.12)
[error] 531-531: context "runner" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "secrets", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🤖 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 @.github/workflows/ios-testflight.yml at line 531, Move
CMUX_TESTFLIGHT_ASSIGN_STATE_OUT_FILE out of the jobs.assign-external-group env
block and into the “Assign uploaded build to the external beta group” step scope
so runner.temp resolves correctly. Update the relevant step-level env for the
assign flow, keeping the state file path tied to runner.temp where it is
available, and leave the rest of the workflow env unchanged.
Source: Linters/SAST tools
| try: | ||
| submission = _beta_review_submission(token, build_id) | ||
| if submission is not None: | ||
| review_state = submission["beta_review_state"] | ||
| if review_state in ACTIVE_CURRENT_BUILD_BETA_REVIEW_STATES: | ||
| print( | ||
| "asc_assign_external_testflight_group: build " | ||
| f"{build_number} external state is READY_FOR_BETA_SUBMISSION with submission " | ||
| f"{submission['id']} (state={review_state or 'unknown'})" | ||
| ) | ||
| return "current_build_review_pending" | ||
| raise RuntimeError( | ||
| "build " | ||
| f"{build_number} external state is READY_FOR_BETA_SUBMISSION with beta app review " | ||
| f"submission {submission['id']} in unexpected betaReviewState={review_state or '<empty>'}" | ||
| ) | ||
| sibling_submission = _find_active_review_submission_on_sibling_build(token, build_id) | ||
| if sibling_submission is not None: | ||
| _report_pending_sibling_review(build_number, sibling_submission) | ||
| return "sibling_review_pending" | ||
| except RuntimeError as recovery_exc: | ||
| last_submit_error = ( | ||
| f"{str(exc) or 'submit beta app review did not succeed yet'}; " | ||
| f"recovery lookup failed: {recovery_exc}" | ||
| ) | ||
| if time.time() >= deadline: | ||
| raise RuntimeError( | ||
| f"build {build_number} failed to submit beta app review within the timeout window" | ||
| ) | ||
| time.sleep(max(1, poll_seconds)) | ||
| token = _token() | ||
| continue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '400,540p' ios/scripts/asc_assign_external_testflight_group.pyRepository: manaflow-ai/cmux
Length of output: 7005
🏁 Script executed:
rg -n "current_build_review_pending|sibling_review_pending|unexpected betaReviewState|beta_review_state|_find_active_review_submission_on_sibling_build|_report_pending_sibling_review" ios -SRepository: manaflow-ai/cmux
Length of output: 2533
🏁 Script executed:
sed -n '330,390p' ios/scripts/asc_assign_external_testflight_group.pyRepository: manaflow-ai/cmux
Length of output: 2437
🏁 Script executed:
rg -n "asc_assign_external_testflight_group|beta app review|READY_FOR_BETA_SUBMISSION|REJECTED|unexpected betaReviewState|sibling_review_pending|current_build_review_pending" ios -g '*test*' -SRepository: manaflow-ai/cmux
Length of output: 4323
Unexpected betaReviewState errors should not be treated as recovery failures. The inner except RuntimeError as recovery_exc also catches the deliberate raise RuntimeError(... unexpected betaReviewState=...), so a terminal review state gets retried until the deadline and then collapses into a generic timeout. Re-raise that branch or narrow the recovery catch to lookup failures only.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 469-473: Abstract raise to an inner function
(TRY301)
[warning] 469-473: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 484-486: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 484-486: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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 `@ios/scripts/asc_assign_external_testflight_group.py` around lines 458 - 489,
The retry loop in asc_assign_external_testflight_group.py is swallowing the
deliberate RuntimeError raised for an unexpected betaReviewState inside the
submission check, causing terminal states to be retried as if they were
transient lookup failures. Update the try/except around _beta_review_submission
and _find_active_review_submission_on_sibling_build so that only recovery/lookup
errors are handled by the inner except RuntimeError as recovery_exc, and let the
unexpected betaReviewState branch propagate immediately. Keep the existing
current_build_review_pending and sibling_review_pending paths unchanged, but
make sure the RuntimeError from the unexpected betaReviewState in this block is
not converted into last_submit_error or retried.
Source: Linters/SAST tools
This branch was successfully deployed
1 participant
Summary
READY_FOR_BETA_SUBMISSIONTesting
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Auto-submits external TestFlight builds for Beta App Review when Apple marks a new MARKETING_VERSION as READY_FOR_BETA_SUBMISSION, removing a manual step and speeding up external releases. The flow is idempotent, resilient to ASC delays, handles sibling-review races, and continues to auto-assign builds to the external group.
asc_assign_external_testflight_group.pynow polls ASC and creates/reuses the submission forREADY_FOR_BETA_SUBMISSION; it treatsAPPROVED/IN_REVIEW/WAITING_FOR_REVIEWas success, no-ops when the build is already approved/testable, waits if a same-version sibling build is in review, retries transient lookup/submit errors, and fails on unexpected states; writes an assign state to${CMUX_TESTFLIGHT_ASSIGN_STATE_OUT_FILE}.ios-testflight-assignment-state-{complete|pending}artifacts and the schedule uses them to retry assignment-only when sibling reviews block; if artifacts have expired (30d), missing state is treated as complete to avoid permanent red.marketing_version_overridepath to upload latestmainunder an approved MARKETING_VERSION (skips notes; disallowsbuild_number); publishesios-testflight-build-metadata-overrideand excludes override runs from canonical “last uploaded” detection.ios/README.md, workflow summaries, andupload-testflight.shto reflect automatic submission; expands tests to cover polling/retry, sibling-review handling, submission races, and delayed metadata.Written for commit 85a04aa. Summary will update on new commits.
Summary by CodeRabbit