ci: resolve build number from TestFlight API instead of run number#103
ci: resolve build number from TestFlight API instead of run number#103evgeniyChepelev wants to merge 12 commits into
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughrelease.yml now computes and exports a TestFlight/App Store Connect build-number; testflight.yml adds dispatch inputs and overrides, queries ASC for latest pre-release build when needed, and resolves the gate job output build-number by precedence (override → ASC lookup → github.run_number). Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant JWT as JWT Generator
participant ASC as App Store Connect API
participant Workflow as CI Workflow
GHA->>JWT: generate ES256 JWT (App Store Connect key)
JWT-->>GHA: return signed JWT
GHA->>ASC: GET /builds?filter[app]=<id>&filter[preReleaseVersion]=<version> (Auth: JWT)
ASC-->>GHA: 200 OK + builds list
GHA->>Workflow: compute max build.attributes.version
Workflow-->>Workflow: increment build-number
Workflow-->>GHA: export resolved build-number (override → tfbuild → run_number)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
|
/testflight |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 45-52: The curl call that sets RESPONSE currently masks failures
and allows shell globbing; update the call that produces RESPONSE (the block
using curl and assigning RESPONSE) to add --globoff to disable URL globbing,
remove the "|| echo '{}'" fallback so failures propagate, then parse LATEST from
RESPONSE as before but validate that LATEST is a numeric value (the variable
LATEST extracted from jq) and if not numeric or empty, print a clear error and
exit non-zero instead of defaulting to 0; finally compute NEXT and write
build-number only after validation succeeds (symbols to edit: RESPONSE
assignment, LATEST, NEXT).
- Around line 37-42: The JWT signature generation uses openssl dgst -sign which
outputs a DER-encoded ECDSA signature but App Store Connect ES256 requires raw
IEEE-P1363 r||s; update the SIGNATURE generation to produce a raw r||s 64-byte
signature (concatenate r and s, each fixed-width 32 bytes) before
base64url-encoding. Replace the current SIGNATURE creation that references
HEADER, PAYLOAD, and KEY_FILE with a step that either: (a) calls a small
Node/crypto script to sign the string "%s.%s" (HEADER and PAYLOAD) with the
private key using ECDSA and obtains the raw r||s signature, or (b)
post-processes the DER signature from openssl to extract r and s and left-pad
each to 32 bytes and concat; then base64url-encode that raw signature and set
JWT="${HEADER}.${PAYLOAD}.${SIGNATURE}" so the final TOKEN uses the correct
ES256 format.
In @.github/workflows/testflight.yml:
- Around line 176-183: The curl invocation that sets RESPONSE should be changed
to include --globoff and to stop masking failures by removing the "|| echo '{}'"
fallback so API/network/auth errors cause the job to fail; update the command
that builds RESPONSE (the curl line referenced by RESPONSE) to add --globoff and
drop the fallback, then ensure the subsequent logic that parses LATEST and
computes NEXT (variables LATEST and NEXT) relies on a valid RESPONSE (or add an
explicit check that RESPONSE is non-empty or exit with an error) so you don't
silently default the build number to 1.
- Around line 168-173: The SIGNATURE generation currently uses openssl dgst
-sign which yields a DER-encoded ECDSA signature; replace that step so the
signature is produced in raw IEEE P1363 format (concatenated r||s, 64 bytes for
P-256) and then base64url-encode it before assembling JWT (HEADER, PAYLOAD,
SIGNATURE, JWT). Implement this by replacing the openssl dgst invocation that
sets SIGNATURE with a step that produces the raw r||s signature (for example,
use a Node.js one-liner calling crypto.sign(..., { dsaEncoding: 'ieee-p1363' })
and base64url-encode the result) and apply the same change in both
testflight.yml and release.yml.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c82c78f-a30e-476f-a007-bba93a62c193
📒 Files selected for processing (2)
.github/workflows/release.yml.github/workflows/testflight.yml
| RESPONSE=$(curl -sf \ | ||
| -H "Authorization: Bearer $JWT" \ | ||
| "https://api.appstoreconnect.apple.com/v1/builds?filter[app]=${APP_ID}&filter[preReleaseVersion.version]=${VERSION}&sort=-version&limit=1&fields[builds]=version" \ | ||
| || echo '{}') | ||
|
|
||
| LATEST=$(echo "$RESPONSE" | jq -r '.data[0].attributes.version // "0"') | ||
| NEXT=$((LATEST + 1)) | ||
| echo "build-number=$NEXT" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set +e
echo "Without --globoff:"
curl -sS -o /dev/null 'https://example.invalid/v1/builds?filter[app]=123' 2>&1
echo "exit=$?"
echo
echo "With --globoff:"
curl --globoff -sS -o /dev/null --connect-timeout 1 'https://example.invalid/v1/builds?filter[app]=123' 2>&1
echo "exit=$?"Repository: netbirdio/ios-client
Length of output: 296
🏁 Script executed:
cat -n .github/workflows/release.yml | sed -n '40,60p'Repository: netbirdio/ios-client
Length of output: 1264
Do not mask TestFlight lookup failures as build number 1.
The URL contains filter[app]-style brackets, which curl treats as URL globbing unless --globoff is used. Failures are currently swallowed with || echo '{}', silently converting auth/query/network failures into LATEST=0 and emitting build-number=1. Add --globoff to disable globbing, remove the failure-masking pattern, and validate the response is numeric:
🛠️ Proposed fix to fail closed and disable curl globbing
- RESPONSE=$(curl -sf \
+ RESPONSE=$(curl --globoff -fsS \
-H "Authorization: Bearer $JWT" \
"https://api.appstoreconnect.apple.com/v1/builds?filter[app]=${APP_ID}&filter[preReleaseVersion.version]=${VERSION}&sort=-version&limit=1&fields[builds]=version" \
- || echo '{}')
+ )
- LATEST=$(echo "$RESPONSE" | jq -r '.data[0].attributes.version // "0"')
+ LATEST=$(jq -r '.data[0].attributes.version // "0"' <<< "$RESPONSE")
+ if ! [[ "$LATEST" =~ ^[0-9]+$ ]]; then
+ echo "::error::Latest TestFlight build number is not numeric: '$LATEST'"
+ exit 1
+ fi
NEXT=$((LATEST + 1))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 45 - 52, The curl call that sets
RESPONSE currently masks failures and allows shell globbing; update the call
that produces RESPONSE (the block using curl and assigning RESPONSE) to add
--globoff to disable URL globbing, remove the "|| echo '{}'" fallback so
failures propagate, then parse LATEST from RESPONSE as before but validate that
LATEST is a numeric value (the variable LATEST extracted from jq) and if not
numeric or empty, print a clear error and exit non-zero instead of defaulting to
0; finally compute NEXT and write build-number only after validation succeeds
(symbols to edit: RESPONSE assignment, LATEST, NEXT).
| RESPONSE=$(curl -sf \ | ||
| -H "Authorization: Bearer $JWT" \ | ||
| "https://api.appstoreconnect.apple.com/v1/builds?filter[app]=${APP_ID}&filter[preReleaseVersion.version]=${VERSION}&sort=-version&limit=1&fields[builds]=version" \ | ||
| || echo '{}') | ||
|
|
||
| LATEST=$(echo "$RESPONSE" | jq -r '.data[0].attributes.version // "0"') | ||
| NEXT=$((LATEST + 1)) | ||
| echo "build-number=$NEXT" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set +e
echo "Without --globoff:"
curl -sS -o /dev/null 'https://example.invalid/v1/builds?filter[app]=123' 2>&1
echo "exit=$?"
echo
echo "With --globoff:"
curl --globoff -sS -o /dev/null --connect-timeout 1 'https://example.invalid/v1/builds?filter[app]=123' 2>&1
echo "exit=$?"Repository: netbirdio/ios-client
Length of output: 296
Add --globoff to the curl command and fail if the API request fails.
The URL contains square brackets (filter[app], filter[preReleaseVersion.version], fields[builds]) which curl interprets as glob patterns by default, causing it to exit with code 3 before reaching the API. The || echo '{}' fallback then masks this (and any legitimate auth/network/API failures) by silently returning an empty object, which causes the build number to default to 1. This risks overwriting existing TestFlight builds.
Proposed fix
- RESPONSE=$(curl -sf \
+ RESPONSE=$(curl --globoff -fsS \
-H "Authorization: Bearer $JWT" \
"https://api.appstoreconnect.apple.com/v1/builds?filter[app]=${APP_ID}&filter[preReleaseVersion.version]=${VERSION}&sort=-version&limit=1&fields[builds]=version" \
- || echo '{}')
+ )
- LATEST=$(echo "$RESPONSE" | jq -r '.data[0].attributes.version // "0"')
+ LATEST=$(jq -r '.data[0].attributes.version // "0"' <<< "$RESPONSE")
+ if ! [[ "$LATEST" =~ ^[0-9]+$ ]]; then
+ echo "::error::Latest TestFlight build number is not numeric: '$LATEST'"
+ exit 1
+ fi
NEXT=$((LATEST + 1))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RESPONSE=$(curl -sf \ | |
| -H "Authorization: Bearer $JWT" \ | |
| "https://api.appstoreconnect.apple.com/v1/builds?filter[app]=${APP_ID}&filter[preReleaseVersion.version]=${VERSION}&sort=-version&limit=1&fields[builds]=version" \ | |
| || echo '{}') | |
| LATEST=$(echo "$RESPONSE" | jq -r '.data[0].attributes.version // "0"') | |
| NEXT=$((LATEST + 1)) | |
| echo "build-number=$NEXT" >> "$GITHUB_OUTPUT" | |
| RESPONSE=$(curl --globoff -fsS \ | |
| -H "Authorization: Bearer $JWT" \ | |
| "https://api.appstoreconnect.apple.com/v1/builds?filter[app]=${APP_ID}&filter[preReleaseVersion.version]=${VERSION}&sort=-version&limit=1&fields[builds]=version" \ | |
| ) | |
| LATEST=$(jq -r '.data[0].attributes.version // "0"' <<< "$RESPONSE") | |
| if ! [[ "$LATEST" =~ ^[0-9]+$ ]]; then | |
| echo "::error::Latest TestFlight build number is not numeric: '$LATEST'" | |
| exit 1 | |
| fi | |
| NEXT=$((LATEST + 1)) | |
| echo "build-number=$NEXT" >> "$GITHUB_OUTPUT" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/testflight.yml around lines 176 - 183, The curl invocation
that sets RESPONSE should be changed to include --globoff and to stop masking
failures by removing the "|| echo '{}'" fallback so API/network/auth errors
cause the job to fail; update the command that builds RESPONSE (the curl line
referenced by RESPONSE) to add --globoff and drop the fallback, then ensure the
subsequent logic that parses LATEST and computes NEXT (variables LATEST and
NEXT) relies on a valid RESPONSE (or add an explicit check that RESPONSE is
non-empty or exit with an error) so you don't silently default the build number
to 1.
|
TestFlight builds uploaded |
|
/testflight |
1 similar comment
|
/testflight |
|
/testflight |
|
/testflight |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.github/workflows/testflight.yml (1)
213-215:⚠️ Potential issue | 🔴 CriticalAdd
--globoffhere too — same URL-globbing issue asrelease.yml.This curl uses the same bracket-laden URL (
filter[app],filter[preReleaseVersion.version],fields[builds]) and will be rejected by curl's URL globbing. Same fix as therelease.ymlcounterpart.🛠️ Proposed fix
- HTTP_STATUS=$(curl -s -o /tmp/tf_response.json -w "%{http_code}" \ + HTTP_STATUS=$(curl --globoff -sS -o /tmp/tf_response.json -w "%{http_code}" \ -H "Authorization: Bearer $JWT" \ "https://api.appstoreconnect.apple.com/v1/builds?filter[app]=${APP_ID}&filter[preReleaseVersion.version]=${VERSION}&sort=-version&limit=25&fields[builds]=version")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/testflight.yml around lines 213 - 215, The curl invocation that sets HTTP_STATUS uses a bracketed App Store Connect URL (the filter[app], filter[preReleaseVersion.version], fields[builds] query params) which will be rejected by curl's URL globbing; update the curl command that assigns HTTP_STATUS to include the --globoff option so curl treats the URL literally (i.e., add --globoff to the existing curl invocation that writes to /tmp/tf_response.json and sets HTTP_STATUS)..github/workflows/release.yml (1)
49-51:⚠️ Potential issue | 🔴 CriticalAdd
--globoffto the curl call — URL brackets still break curl's URL globbing.The URL contains
filter[app],filter[preReleaseVersion.version], andfields[builds], all of which curl interprets as glob ranges by default and will reject withcurl: (3) bad range specificationbefore any request is made. Withset -euo pipefail,HTTP_STATUS=$(curl …)then propagates the failure, but the error is opaque ("bad range" rather than a TestFlight problem). This was flagged previously on the DER-masking pair and only the|| echo '{}'half was fixed.🛠️ Proposed fix
- HTTP_STATUS=$(curl -s -o /tmp/tf_response.json -w "%{http_code}" \ + HTTP_STATUS=$(curl --globoff -sS -o /tmp/tf_response.json -w "%{http_code}" \ -H "Authorization: Bearer $JWT" \ "https://api.appstoreconnect.apple.com/v1/builds?filter[app]=${APP_ID}&filter[preReleaseVersion.version]=${VERSION}&sort=-version&limit=25&fields[builds]=version")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 49 - 51, The curl invocation that sets HTTP_STATUS is failing due to URL globbing on bracketed query parameters; update the curl command used in the HTTP_STATUS assignment to include the --globoff flag so curl treats the URL literally (i.e., modify the curl call that writes to /tmp/tf_response.json and sets HTTP_STATUS to add --globoff while preserving the existing -s -o -w, Authorization header and URL).
🧹 Nitpick comments (3)
.github/workflows/release.yml (2)
35-47: Minor: usetrapto guarantee the private-key tempfile is removed.If any step between
mktempandrm -f "$KEY_FILE"fails (e.g., theopenssl | python3pipeline underset -euo pipefail), the decoded.p8key stays on disk for the rest of the job. Runner is ephemeral, but subsequent steps (uploading logs, artifacts) could inadvertently capture it. Atrapon EXIT makes cleanup unconditional.🛠️ Proposed fix
KEY_FILE=$(mktemp) + trap 'rm -f "$KEY_FILE"' EXIT echo "$API_KEY_BASE64" | base64 --decode > "$KEY_FILE" @@ JWT="${HEADER}.${PAYLOAD}.${SIGNATURE}" - rm -f "$KEY_FILE"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 35 - 47, Add an unconditional cleanup trap immediately after creating the temporary key file (after KEY_FILE=$(mktemp)) so the tempfile is removed on EXIT even if a later command fails; reference the KEY_FILE variable and ensure the trap runs rm -f "$KEY_FILE" (or equivalent) on EXIT and remains in effect for the entire block that uses HEADER, PAYLOAD, SIGNATURE and JWT so the file is always removed before the job ends.
25-62: Consider extracting the JWT + TestFlight lookup into a composite action.The entire ES256 JWT mint + Builds API query +
jq maxlogic is duplicated verbatim in.github/workflows/testflight.yml(lines 170–226). Any future fix (e.g., the--globoffissue, rotating to a different App Store Connect endpoint, or replacing the inline Python DER parser) has to be applied twice and kept in sync. Moving it to.github/actions/resolve-testflight-build-number/action.yml(composite action) takingapp-id/versionas inputs and emittingbuild-numberwould eliminate the drift risk.Not a blocker for this PR — fine to defer if you'd rather land the feature first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 25 - 62, Extract the ES256 JWT minting + TestFlight builds lookup (the "Resolve build number from TestFlight" step that generates JWT, SIGNATURE, calls the App Store Connect builds API, computes LATEST via jq and sets "build-number") into a reusable composite action (e.g., resolve-testflight-build-number) that accepts inputs app-id and version and exposes output build-number; move all JWT/key handling and curl/jq logic into the composite action, accept/send required secrets as action inputs, and replace the duplicated inline blocks in both workflows with a single step that calls the new composite action so future fixes (DER parser, --globoff, endpoints) are changed once..github/workflows/testflight.yml (1)
238-243: Redundant but harmless fallback chain.
tfbuildalready short-circuits whenBUILD_OVERRIDEis set (lines 182–186), sosteps.tfbuild.outputs.build-numberwill already equal the override when one is supplied — the first term of this OR chain never wins over the second in practice. And iftfbuildruns and fails to produce an output, the step itself exits non-zero, so thegithub.run_numbertier only fires whenshould-build == 'false'(build job is skipped anyway).Not a bug; just flagging so the precedence isn't mistaken for actual defense-in-depth. If you want a genuine safety net for "tfbuild silently produced nothing", make it explicit with a warning log.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/testflight.yml around lines 238 - 243, The fallback chain for buildNumber is redundant because steps.tfbuild already respects BUILD_OVERRIDE; update the logic around the buildNumber assignment (the buildNumber variable and the core.setOutput/core.info usage) to either remove the unnecessary first term ('${{ steps.pre.outputs.build-number-override }}') or, if you want a real safety net, explicitly read steps.tfbuild.outputs.build-number and if it is empty log a warning via core.warning before falling back to '${{ github.run_number }}' and then call core.setOutput('build-number', buildNumber) and core.info; reference the buildNumber variable, steps.pre.outputs.build-number-override, steps.tfbuild.outputs.build-number, github.run_number, core.setOutput, and core.info when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/release.yml:
- Around line 49-51: The curl invocation that sets HTTP_STATUS is failing due to
URL globbing on bracketed query parameters; update the curl command used in the
HTTP_STATUS assignment to include the --globoff flag so curl treats the URL
literally (i.e., modify the curl call that writes to /tmp/tf_response.json and
sets HTTP_STATUS to add --globoff while preserving the existing -s -o -w,
Authorization header and URL).
In @.github/workflows/testflight.yml:
- Around line 213-215: The curl invocation that sets HTTP_STATUS uses a
bracketed App Store Connect URL (the filter[app],
filter[preReleaseVersion.version], fields[builds] query params) which will be
rejected by curl's URL globbing; update the curl command that assigns
HTTP_STATUS to include the --globoff option so curl treats the URL literally
(i.e., add --globoff to the existing curl invocation that writes to
/tmp/tf_response.json and sets HTTP_STATUS).
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 35-47: Add an unconditional cleanup trap immediately after
creating the temporary key file (after KEY_FILE=$(mktemp)) so the tempfile is
removed on EXIT even if a later command fails; reference the KEY_FILE variable
and ensure the trap runs rm -f "$KEY_FILE" (or equivalent) on EXIT and remains
in effect for the entire block that uses HEADER, PAYLOAD, SIGNATURE and JWT so
the file is always removed before the job ends.
- Around line 25-62: Extract the ES256 JWT minting + TestFlight builds lookup
(the "Resolve build number from TestFlight" step that generates JWT, SIGNATURE,
calls the App Store Connect builds API, computes LATEST via jq and sets
"build-number") into a reusable composite action (e.g.,
resolve-testflight-build-number) that accepts inputs app-id and version and
exposes output build-number; move all JWT/key handling and curl/jq logic into
the composite action, accept/send required secrets as action inputs, and replace
the duplicated inline blocks in both workflows with a single step that calls the
new composite action so future fixes (DER parser, --globoff, endpoints) are
changed once.
In @.github/workflows/testflight.yml:
- Around line 238-243: The fallback chain for buildNumber is redundant because
steps.tfbuild already respects BUILD_OVERRIDE; update the logic around the
buildNumber assignment (the buildNumber variable and the
core.setOutput/core.info usage) to either remove the unnecessary first term
('${{ steps.pre.outputs.build-number-override }}') or, if you want a real safety
net, explicitly read steps.tfbuild.outputs.build-number and if it is empty log a
warning via core.warning before falling back to '${{ github.run_number }}' and
then call core.setOutput('build-number', buildNumber) and core.info; reference
the buildNumber variable, steps.pre.outputs.build-number-override,
steps.tfbuild.outputs.build-number, github.run_number, core.setOutput, and
core.info when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6dd6a281-5461-42ee-81d1-cd5d9f9fd0ef
📒 Files selected for processing (2)
.github/workflows/release.yml.github/workflows/testflight.yml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/testflight.yml (1)
214-222:⚠️ Potential issue | 🔴 CriticalAdd
--globoffand treat curl transport failures as fatal.The URL contains
filter[app],filter[preReleaseVersion.version],fields[builds]— curl interprets[...]as URL-globbing and exits with code 3 before sending the request. The current invocation has no--globoffand captures only%{http_code}(which will be empty on a transport failure); withset -e, a failed command substitution assignment does not abort the script, soHTTP_STATUSbecomes empty and your!= "200"branch does fire — but the error message will be misleading ("HTTP " with no status) and any other transport/TLS/DNS error is similarly swallowed into the same generic branch. Add--globoffand--fail-with-body(or check the curl exit code explicitly) so real failures surface clearly.🔒️ Proposed fix
- HTTP_STATUS=$(curl -s -o /tmp/tf_response.json -w "%{http_code}" \ + HTTP_STATUS=$(curl --globoff -sS -o /tmp/tf_response.json -w "%{http_code}" \ -H "Authorization: Bearer $JWT" \ - "https://api.appstoreconnect.apple.com/v1/builds?filter[app]=${APP_ID}&filter[preReleaseVersion.version]=${VERSION}&sort=-version&limit=25&fields[builds]=version") + "https://api.appstoreconnect.apple.com/v1/builds?filter[app]=${APP_ID}&filter[preReleaseVersion.version]=${VERSION}&sort=-version&limit=25&fields[builds]=version") \ + || { echo "::error::curl failed talking to App Store Connect"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/testflight.yml around lines 214 - 222, The curl invocation that sets HTTP_STATUS/RESPONSE should be made robust by disabling URL globbing and surfacing transport failures: add --globoff and --fail-with-body to the curl options (or explicitly check curl's exit code after the command) when calling curl that assigns HTTP_STATUS and writes /tmp/tf_response.json; then if curl exits non‑zero log a clear error (including curl's exit code and any response body) and exit 1 instead of relying only on HTTP_STATUS != "200". Ensure you update the curl command that produces HTTP_STATUS and the subsequent branch that checks HTTP_STATUS/RESPONSE to handle and report transport failures distinctly.
🧹 Nitpick comments (2)
.github/workflows/testflight.yml (2)
224-225:tonumber+ bash arithmetic assumes strictly integer build numbers.If any returned
attributes.versionis non-integer (e.g."5.1", accidentally pushed via Xcode),jq | tonumberyields a float,maxreturns a float, andNEXT=$((LATEST + 1))then fails with"invalid arithmetic operator". iOS TestFlight typically uses monotonic integers so this is unlikely in practice, but a defensive guard (orflooring in jq) would make the failure mode explicit:- LATEST=$(echo "$RESPONSE" | jq -r '[.data[].attributes.version | tonumber] | max // 0') + LATEST=$(jq -r '[.data[].attributes.version | tonumber | floor] | max // 0' <<<"$RESPONSE") + if ! [[ "$LATEST" =~ ^[0-9]+$ ]]; then + echo "::error::Unexpected non-integer TestFlight build number: '$LATEST'" + exit 1 + fi NEXT=$((LATEST + 1))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/testflight.yml around lines 224 - 225, The current pipeline computes LATEST via jq using "tonumber" which can produce floats and break NEXT arithmetic; update the jq expression that sets LATEST so it coerces/guards to integers (e.g., use tonumber? and floor or fallback to 0) before taking max, so LATEST is always an integer and NEXT=$((LATEST + 1)) will not fail; modify the command that assigns LATEST (the expression producing LATEST from RESPONSE) to apply floor and safe tonumber handling.
200-212: Clean up the decoded key file on any failure path.Between writing
KEY_FILE(line 201) andrm -f(line 212), several commands can fail (openssl,python3, base64 pipes). Withset -e, a mid-pipeline failure leaves the decoded private key on the runner's disk. Use atrapto guarantee cleanup.🛡️ Proposed hardening
KEY_FILE=$(mktemp) + trap 'rm -f "$KEY_FILE"' EXIT echo "$API_KEY_BASE64" | base64 --decode > "$KEY_FILE" @@ JWT="${HEADER}.${PAYLOAD}.${SIGNATURE}" - rm -f "$KEY_FILE"Also optional: the inline Python DER→IEEE-P1363 converter is correct for P-256 (single-byte DER length always < 128) but quite dense. If you expect to touch this again, consider extracting it into a small
scripts/der_to_p1363.pywith a brief comment on the length-byte assumption — it'll make future review of the JWT pipeline a lot easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/testflight.yml around lines 200 - 212, Add a guaranteed cleanup trap so the temporary decoded key (KEY_FILE) is removed on any failure or exit: after creating KEY_FILE with mktemp, install a trap on EXIT/ERR that rm -f "$KEY_FILE" (or unset the trap at the end) so failures in the openssl / python3 pipeline that generate SIGNATURE (and before the final rm -f) cannot leave the private key on disk; ensure the trap is set before running the HEADER/PAYLOAD/SIGNATURE pipeline and optionally clear the trap only after JWT is built and the explicit rm -f "$KEY_FILE" has run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/testflight.yml:
- Line 5: Remove the feature branch "ci/unified-build-number-from-testflight"
from the push trigger in the TestFlight workflow so it doesn't run on pushes to
the PR source after merge; edit the branches array in
.github/workflows/testflight.yml (the line containing branches: [main,
ci/unified-build-number-from-testflight]) and delete the feature branch entry so
only the intended branch(es) (e.g., main) remain.
---
Duplicate comments:
In @.github/workflows/testflight.yml:
- Around line 214-222: The curl invocation that sets HTTP_STATUS/RESPONSE should
be made robust by disabling URL globbing and surfacing transport failures: add
--globoff and --fail-with-body to the curl options (or explicitly check curl's
exit code after the command) when calling curl that assigns HTTP_STATUS and
writes /tmp/tf_response.json; then if curl exits non‑zero log a clear error
(including curl's exit code and any response body) and exit 1 instead of relying
only on HTTP_STATUS != "200". Ensure you update the curl command that produces
HTTP_STATUS and the subsequent branch that checks HTTP_STATUS/RESPONSE to handle
and report transport failures distinctly.
---
Nitpick comments:
In @.github/workflows/testflight.yml:
- Around line 224-225: The current pipeline computes LATEST via jq using
"tonumber" which can produce floats and break NEXT arithmetic; update the jq
expression that sets LATEST so it coerces/guards to integers (e.g., use
tonumber? and floor or fallback to 0) before taking max, so LATEST is always an
integer and NEXT=$((LATEST + 1)) will not fail; modify the command that assigns
LATEST (the expression producing LATEST from RESPONSE) to apply floor and safe
tonumber handling.
- Around line 200-212: Add a guaranteed cleanup trap so the temporary decoded
key (KEY_FILE) is removed on any failure or exit: after creating KEY_FILE with
mktemp, install a trap on EXIT/ERR that rm -f "$KEY_FILE" (or unset the trap at
the end) so failures in the openssl / python3 pipeline that generate SIGNATURE
(and before the final rm -f) cannot leave the private key on disk; ensure the
trap is set before running the HEADER/PAYLOAD/SIGNATURE pipeline and optionally
clear the trap only after JWT is built and the explicit rm -f "$KEY_FILE" has
run.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5695f66f-0db4-4bce-863e-cfd3e5653a90
📒 Files selected for processing (1)
.github/workflows/testflight.yml
| on: | ||
| push: | ||
| branches: [main] | ||
| branches: [main, ci/unified-build-number-from-testflight] |
There was a problem hiding this comment.
Remove the feature branch from the push trigger before merging.
ci/unified-build-number-from-testflight is the PR's source branch and shouldn't run TestFlight on every push to it once merged (it also bypasses the PR gate). Drop it before merge.
♻️ Proposed change
- branches: [main, ci/unified-build-number-from-testflight]
+ branches: [main]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| branches: [main, ci/unified-build-number-from-testflight] | |
| branches: [main] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/testflight.yml at line 5, Remove the feature branch
"ci/unified-build-number-from-testflight" from the push trigger in the
TestFlight workflow so it doesn't run on pushes to the PR source after merge;
edit the branches array in .github/workflows/testflight.yml (the line containing
branches: [main, ci/unified-build-number-from-testflight]) and delete the
feature branch entry so only the intended branch(es) (e.g., main) remain.
1 participant
Description
/testflight
Summary by CodeRabbit