Resolve the pinned iOS XCFramework the way SPM does - #10
Conversation
The fixture read the AppstackSDK.xcframework directory committed in the distribution repository via git archive <tag>. That directory is vestigial after the move to binaryTarget(url:checksum:): its subtree hash is identical across 4.4.0, 4.4.1, 4.5.0-rc0 and 4.5.0, so it does not track the tag at all. The fixture was therefore compiling the bridge against 4.4.0-era bits while reporting whichever version was pinned, and would keep doing so for every future tag. Concretely, that hid a real API difference: the committed directory does not export setCustomerUserId, while the 4.5.0 release artifact does. The runner now reads the binaryTarget url and checksum from Package.swift at the tag, downloads that artifact, and refuses to continue unless the SHA256 matches the declared checksum. It also rejects a manifest whose URL does not point at the expected release. This needs network access, matching the Android fixture, which already resolves from Maven Central. Verified: the bridge compiles against the real 4.5.0 artifact and all expected C symbols are present; a tampered checksum aborts with exit 8.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe iOS validation flow now reads the tagged ChangesiOS SDK validation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant run-tests.sh
participant Package.swift
participant Release artifact
run-tests.sh->>Package.swift: Read binaryTarget URL and checksum
run-tests.sh->>Release artifact: Download declared archive
run-tests.sh->>Release artifact: Verify SHA-256 checksum
run-tests.sh->>run-tests.sh: Extract verified XCFramework
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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: 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 `@Tests`~/Native/iOS/run-tests.sh:
- Around line 48-51: Update the BINARY_URL validation in run-tests.sh to require
the production GitHub release URL prefix, including the expected SDK version,
rather than only matching the /download/${EXPECTED_SDK_VERSION}/ path. Keep
rejecting non-matching URLs before the artifact download proceeds.
- Around line 53-57: Update the curl invocation in the run-tests download block
to include explicit --connect-timeout and --max-time limits while preserving the
existing failure handling and archive output behavior.
🪄 Autofix
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: 27a7f5ea-ea04-4386-a208-eaa87f07ca47
📒 Files selected for processing (3)
DEVELOPMENT.mdTests~/Native/README.mdTests~/Native/iOS/run-tests.sh
Addresses review on #10. The URL check matched /download/<version>/ anywhere in the string, so a manifest declaring https://evil.example.com/download/4.5.0/... passed it. The checksum does not cover that gap: it comes from the same manifest, so it proves integrity against whatever the manifest claims rather than provenance. The check now anchors the full release origin, which is the same origin Editor/AppstackIOSPostProcessBuild.cs pins, making this a contract assertion rather than only hardening: the fixture now confirms the tag resolves the artifact a Unity build would resolve. The failure message prints both the expected prefix and the declared URL. The download is also bounded with --connect-timeout and --max-time, plus a small --retry, since this PR is what makes the fixture depend on the network: a stall should fail here instead of hanging until the outer CI timeout, and a transient blip should not fail the run. Verified: happy path still passes against the checksum-verified 4.5.0 artifact; a non-matching origin aborts with exit 6 before any download; the lookalike URL that the previous check accepted is now rejected.
|
Both review comments are fixed in a4cfa08. Origin anchoring. The previous check matched Download limits. Added Verification after both changes:
|
1 participant
Follow-up to #9. The iOS contract fixture was validating the bridge against a binary that is not the one we ship. This also corrects an iOS note on #9 that the same problem caused.
The bug
Tests~/Native/iOS/run-tests.shobtained the XCFramework withgit archive <tag> AppstackSDK.xcframeworkfrom anios-appstack-sdkcheckout. That committed directory is vestigial after the distribution repo moved tobinaryTarget(url:checksum:), and it does not track the tag at all:Identical across all four tags. So the fixture compiled the bridge against 4.4.0-era bits while printing the version it was asked to pin, and would have kept doing that for every future tag. What SPM downloads — and what a Unity iOS build actually links — is the release zip named by
binaryTarget(url:), which the fixture never touched.This is not academic: the committed directory does not export
setCustomerUserId, while the real 4.5.0 release artifact does.Correction to the iOS note on #9
#9's description stated that the distributed
4.5.0binary does not exportsetCustomerUserIdand that #7's iOS blocker still stood. That was wrong and has been corrected on #9. It was based on the stale committed directory described above. Verified against the real artifact:binaryTargetURL fromPackage.swiftat tag4.5.0downloads an archive whose SHA256 is4745e8d48767daf034fdaa4e347c0a7f52e5589cba265af7bc3e7da12e0c57e9, matching the checksum the manifest declares, so it is the artifact SPM resolves.setCustomerUserIdis present in its public and private.swiftinterfacefiles and in the framework binary undernm.iOS
4.5.0ships the setter, so #7's blocker is cleared and nothing inios-appstack-sdkneeds re-cutting. The one loose end there is the staleAppstackSDK.xcframework/directory: it is what a reader would naturally inspect, and deleting it — already planned as part of thebinaryTargetmigration — would also shrink clones.The fix
The runner now resolves the binary the way SPM does:
binaryTargeturl and checksum fromPackage.swiftat the tag;It deliberately no longer reads the committed directory, and
DEVELOPMENT.mdplusTests~/Native/README.mdnow say why, so this does not get quietly reintroduced.This requires network access. That matches the Android fixture, which already resolves from Maven Central, so the suite's requirements are unchanged in kind.
The caller-supplied-XCFramework escape hatch from #9 is untouched and still reports those inputs as unverified.
Verification
Verified iOS bridge against AppstackSDK 4.5.0 (checksum-verified release artifact) and all expected C symbols.main.