ci: Include IDB patch hash in release caches#58
Merged
Conversation
Key IDB repository and framework caches by both the pinned IDB commit and the local IDB patch hash so release builds rebuild patched frameworks when AXe patch files change. Also verify FBSimulatorControl release artifacts include the XCTest accessibility client type patch before packaging. Co-Authored-By: OpenAI Codex <codex@openai.com>
WalkthroughThis pull request updates the iOS build release workflow and build script to strengthen cache consistency and binary integrity verification. The workflow now derives IDB cache keys dynamically from the pinned commit in 🚥 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. 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 |
![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
🧹 Nitpick comments (1)
scripts/build.sh (1)
114-130: ⚖️ Poor tradeoffConsider more robust binary verification.
The current verification relies on
strings+grep, which may produce false negatives:
- Compiler optimisations can affect how selectors are stored in the binary.
- The
stringscommand may not reliably extract all selector references depending on binary format.- No validation that the
stringscommand succeeded.For Objective-C selector verification, consider using
nmto check for symbol presence orotool -oVto examine the Objective-C metadata directly, which would be more reliable than string extraction.♻️ Alternative implementation using nm
function verify_fbsimulatorcontrol_has_xctest_client_type_patch() { local binary_path="$1" if [[ ! -f "$binary_path" ]]; then echo "❌ Error: FBSimulatorControl binary not found for accessibility patch verification: $binary_path" exit 1 fi - local client_type_references - client_type_references=$(strings -a "$binary_path" | grep -F "setClientType:" || true) - if [[ -z "$client_type_references" ]]; then + # Check for the selector in Objective-C metadata + if ! otool -oV "$binary_path" 2>/dev/null | grep -q "setClientType:"; then echo "❌ Error: FBSimulatorControl is missing the XCTest accessibility client type patch" echo " Expected to find selector reference 'setClientType:' in: $binary_path" echo " Rebuild IDB frameworks after applying patches/idb/accessibility-client-type-for-xctest.patch." exit 1 fi }🤖 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 `@scripts/build.sh` around lines 114 - 130, The current verify_fbsimulatorcontrol_has_xctest_client_type_patch function uses strings+grep which can miss Objective-C selectors; change the check to use a more reliable binary inspection (e.g., nm to search defined/undefined symbols or otool -oV to inspect ObjC metadata) instead of strings, and also verify the inspection command succeeds before deciding; specifically, replace the strings|grep check for the selector "setClientType:" with an nm (or otool -oV) invocation that looks for that selector/symbol and exit non-zero if the inspection command fails or the symbol is not found, keeping the same error messages and exit behavior in verify_fbsimulatorcontrol_has_xctest_client_type_patch.
🤖 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/release-shared.yml:
- Around line 55-63: The extraction step (id idb_inputs) must validate
PINNED_IDB_COMMIT (from scripts/build.sh via grep) and PATCH_HASH (from
patches/idb) and fail fast with clear messages: after computing
PINNED_IDB_COMMIT ensure it is non-empty and if empty print a descriptive error
and exit 1; for PATCH_HASH first detect whether any files exist under
patches/idb and if none either fail with a clear error or set a deterministic
sentinel (but do not silently use the hash of empty input); ensure grep failures
are surfaced (keep set -euo pipefail and avoid swallowing grep errors) and only
write idb_commit/patch_hash to GITHUB_OUTPUT when they pass validation so the
workflow fails early with actionable logs.
---
Nitpick comments:
In `@scripts/build.sh`:
- Around line 114-130: The current
verify_fbsimulatorcontrol_has_xctest_client_type_patch function uses
strings+grep which can miss Objective-C selectors; change the check to use a
more reliable binary inspection (e.g., nm to search defined/undefined symbols or
otool -oV to inspect ObjC metadata) instead of strings, and also verify the
inspection command succeeds before deciding; specifically, replace the
strings|grep check for the selector "setClientType:" with an nm (or otool -oV)
invocation that looks for that selector/symbol and exit non-zero if the
inspection command fails or the symbol is not found, keeping the same error
messages and exit behavior in
verify_fbsimulatorcontrol_has_xctest_client_type_patch.
🪄 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: 155b91c2-07d6-48ac-a1cc-dc8461b51a71
📒 Files selected for processing (2)
.github/workflows/release-shared.ymlscripts/build.sh
Comment on lines
+55
to
+63
| - name: Resolve IDB cache inputs | ||
| id: idb_inputs | ||
| run: | | ||
| set -euo pipefail | ||
| PINNED_IDB_COMMIT=$(grep '^IDB_GIT_REF=' scripts/build.sh | sed -E 's/^IDB_GIT_REF="\$\{IDB_GIT_REF:-([^}]*)\}"$/\1/') | ||
| PATCH_HASH=$(find patches/idb -name '*.patch' -type f | sort | xargs shasum -a 256 | shasum -a 256 | awk '{print $1}') | ||
| echo "idb_commit=$PINNED_IDB_COMMIT" >> "$GITHUB_OUTPUT" | ||
| echo "patch_hash=$PATCH_HASH" >> "$GITHUB_OUTPUT" | ||
|
|
There was a problem hiding this comment.
Add error handling and validation for cache input extraction.
The extraction logic is fragile and lacks validation:
- The
grepcommand will fail silently if the pattern changes inscripts/build.sh. - No validation that
PINNED_IDB_COMMITis non-empty after extraction. - If no patch files exist,
PATCH_HASHwill be the hash of empty input, which may not be the intended behaviour.
Consider adding validation after extraction to fail fast with a clear error message if the inputs are invalid.
🛡️ Proposed fix to add validation
- name: Resolve IDB cache inputs
id: idb_inputs
run: |
set -euo pipefail
PINNED_IDB_COMMIT=$(grep '^IDB_GIT_REF=' scripts/build.sh | sed -E 's/^IDB_GIT_REF="\$\{IDB_GIT_REF:-([^}]*)\}"$/\1/')
+ if [[ -z "$PINNED_IDB_COMMIT" ]]; then
+ echo "❌ Error: Failed to extract IDB_GIT_REF from scripts/build.sh"
+ exit 1
+ fi
PATCH_HASH=$(find patches/idb -name '*.patch' -type f | sort | xargs shasum -a 256 | shasum -a 256 | awk '{print $1}')
+ if [[ -z "$PATCH_HASH" ]]; then
+ echo "❌ Error: Failed to compute patch hash"
+ exit 1
+ fi
echo "idb_commit=$PINNED_IDB_COMMIT" >> "$GITHUB_OUTPUT"
echo "patch_hash=$PATCH_HASH" >> "$GITHUB_OUTPUT"📝 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.
Suggested change
| - name: Resolve IDB cache inputs | |
| id: idb_inputs | |
| run: | | |
| set -euo pipefail | |
| PINNED_IDB_COMMIT=$(grep '^IDB_GIT_REF=' scripts/build.sh | sed -E 's/^IDB_GIT_REF="\$\{IDB_GIT_REF:-([^}]*)\}"$/\1/') | |
| PATCH_HASH=$(find patches/idb -name '*.patch' -type f | sort | xargs shasum -a 256 | shasum -a 256 | awk '{print $1}') | |
| echo "idb_commit=$PINNED_IDB_COMMIT" >> "$GITHUB_OUTPUT" | |
| echo "patch_hash=$PATCH_HASH" >> "$GITHUB_OUTPUT" | |
| - name: Resolve IDB cache inputs | |
| id: idb_inputs | |
| run: | | |
| set -euo pipefail | |
| PINNED_IDB_COMMIT=$(grep '^IDB_GIT_REF=' scripts/build.sh | sed -E 's/^IDB_GIT_REF="\$\{IDB_GIT_REF:-([^}]*)\}"$/\1/') | |
| if [[ -z "$PINNED_IDB_COMMIT" ]]; then | |
| echo "❌ Error: Failed to extract IDB_GIT_REF from scripts/build.sh" | |
| exit 1 | |
| fi | |
| PATCH_HASH=$(find patches/idb -name '*.patch' -type f | sort | xargs shasum -a 256 | shasum -a 256 | awk '{print $1}') | |
| if [[ -z "$PATCH_HASH" ]]; then | |
| echo "❌ Error: Failed to compute patch hash" | |
| exit 1 | |
| fi | |
| echo "idb_commit=$PINNED_IDB_COMMIT" >> "$GITHUB_OUTPUT" | |
| echo "patch_hash=$PATCH_HASH" >> "$GITHUB_OUTPUT" |
🤖 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/release-shared.yml around lines 55 - 63, The extraction
step (id idb_inputs) must validate PINNED_IDB_COMMIT (from scripts/build.sh via
grep) and PATCH_HASH (from patches/idb) and fail fast with clear messages: after
computing PINNED_IDB_COMMIT ensure it is non-empty and if empty print a
descriptive error and exit 1; for PATCH_HASH first detect whether any files
exist under patches/idb and if none either fail with a clear error or set a
deterministic sentinel (but do not silently use the hash of empty input); ensure
grep failures are surfaced (keep set -euo pipefail and avoid swallowing grep
errors) and only write idb_commit/patch_hash to GITHUB_OUTPUT when they pass
validation so the workflow fails early with actionable logs.
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
1 participant
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.
Ensure AXe release builds invalidate cached IDB and framework artifacts when local IDB patches change.
The release workflow previously keyed IDB/framework caches mostly by the pinned upstream IDB commit. That meant adding or changing a patch under
patches/idb/could still reuse artifacts built from the same upstream commit before the patch existed, allowing stale frameworks into a release.This keys the IDB checkout, runtime Frameworks, and XCFramework caches by both the pinned IDB commit and a hash of
patches/idb/*.patch. It also adds release artifact verification that fails whenFBSimulatorControldoes not contain evidence of the XCTest accessibility client type patch.Validated locally with YAML parsing, shell syntax checking, and
scripts/build.sh verify-xcframeworks.Note
Low Risk
CI cache keying and build-time binary string checks only; no runtime product logic or auth/data paths change.
Overview
Release CI now invalidates IDB-related caches when local patches change, not only when the pinned upstream IDB commit changes. A new workflow step derives the pinned commit from
scripts/build.shand a combined SHA-256 of allpatches/idb/*.patchfiles; thatpatch_hashis appended to restore/save keys for the IDB checkout, runtime Frameworks, and XCFramework caches (replacing the prior dummy/partial keys).scripts/build.shadds a release verification guard onFBSimulatorControl:verify-xcframeworksandverify-archesfail if the framework binary does not contain asetClientType:string, catching releases built without the XCTest accessibility client-type patch.Reviewed by Cursor Bugbot for commit 31bbb59. Bugbot is set up for automated code reviews on this repo. Configure here.