Skip to content

fix(ios): the iOS app builds, launches and plays -- plus release-time iOS CI - #578

Merged
doublegate merged 11 commits into
mainfrom
test/macOS
Oct 2, 2026
Merged

doublegate merged 11 commits into
mainfrom
test/macOS

Conversation

@doublegate

@doublegate doublegate commented Oct 2, 2026 •

Copy link
Copy Markdown
Owner

Description

The first build of the iOS app on a Mac (Xcode 27.0, iOS 27.0 simulator). In v2.9.7 the app did not compile. Once it did, it crashed at launch, and once that was fixed every game opened to a black screen. This PR fixes all three, plus two build-script defects that turned up along the way. It also gives CI its first build of the Swift app, at release time only, by the maintainer's direction.

The maintainer has played it on the simulator: the picture renders, the controls work, and AccuracyCoin runs to its end.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation update

Component(s) Affected

  • Build system
  • CI/CD
  • Documentation
  • iOS host (ios/), not a template checkbox

Changes Made

Commit What
1b6d2b02 tests/roms/accuracycoin/README.md → RUNTIME.md. It and tests/roms/AccuracyCoin/README.md are one path on a case-insensitive filesystem, so a macOS clone warned "paths have collided" and started with a dirty tree.
196b92c7 The Swift app compiles. The app's enum NesButton collided with the NesButton UniFFI generates from rustynes-mobile (24 errors); renamed NesButtonBit, with the bridge, Kotlin and Android untouched. The FDS BIOS prompt caught MobileError.missingFdsBios; UniFFI keeps Rust's casing, so the case is MissingFdsBios.
c6ca71df ios/project.yml's info: path: block made XcodeGen regenerate the hand-written Info.plist on every build-ios-xcframework.sh run, dropping the document types, UTIs, fonts, orientations and the 120 Hz key. Removed; INFOPLIST_FILE already points at the tracked plist.
a8bd5cf8 The script never set IPHONEOS_DEPLOYMENT_TARGET, so with Xcode 27 every C object (Lua, rcheevos, ring) was built for iOS 27.0 against the app's 17.0 floor: 76 link warnings, and a dyld hazard on iOS 17–26 devices. Now exported as 17.0 (0 warnings).
3510eb2b Launch crash. CloudSaveStateSync.start() checked the iCloud account at launch although sync is opt-in. Without the container entitlement, CKContainer.default() throws an uncatchable CKException, and CKContainer(identifier:) traps too (measured). CloudKit is now untouched while sync is off, and the container is created only when an entitlement probe (public API only) finds it.
bc828a9b Black screen. MetalGameView started its CADisplayLink only after a successful renderer build. At makeUIView the drawable is 0×0, so the build was deferred, and the only retry ran off that same link. Probes showed gfx_init was never called. The link now starts first.
37b1cc5b CI: build the Swift app at release time; make ios.yml run at all. See below.
5d88fcb4 CHANGELOG [Unreleased] entries.
af092df8 Review (Antigravity): log the CloudKit delete and reconcile-fetch failures that were swallowed.
9c3fbd85 Review (Copilot): state accurately when ios.yml runs, and stop presenting the unreleased v2.9.8 as released.
c6d2bce7 Review (CodeRabbit): ios.yml receives only the six signing secrets it reads, declared under workflow_call.secrets and passed by name, not secrets: inherit. Doc comments added to the four touched functions that lacked them (docstring-coverage check).

The CI change, and its limits

  • ios.yml hadn't run for a release since v2.3.8 (gh run list --workflow ios.yml: last tag run 2026-08-20). release-auto.yml pushes release tags with GITHUB_TOKEN, which does not fire on: push: tags. It already calls release.yml directly for that reason, but never called ios.yml. ios.yml now has a workflow_call trigger with a tag input, and release-auto has an ios job that calls it. It passes, by name, only the six App Store Connect / fastlane match secrets the TestFlight steps read, all optional (absent, the upload is skipped).
  • ios.yml now builds the Swift app: xcodebuild build for generic/platform=iOS Simulator, unsigned. A git diff --exit-code step fails the run if the script modified a tracked file (the Info.plist class of bug), and the xcodebuild log is uploaded on failure.
  • Cost, by maintainer direction: macOS jobs never run on PRs, pushes to main or ci.yml's weekly cron (ci.yml is unchanged). ios.yml runs for a release, by hand, and on its pre-existing TestFlight refresh cron, which is dormant until the IOS_SIGNING_READY repo variable is set (corrected after Copilot's review).
  • The trade-off: this check cannot block a release. release-auto creates the tag and GitHub Release before it calls ios.yml, so a broken app still ships and the release run turns red afterwards. For a pre-merge answer, dispatch "iOS" by hand on the release branch.

Testing Performed

  • iPhone 18 Pro simulator, iOS 27.0, Xcode 27.0 on Apple silicon:
    • The app builds with 0 errors.
    • It launches unsigned and ad-hoc signed, each with save-state sync off and forced on. All four run with no crash report; the old code crashed in both unsigned cases.
    • Temporary NSLog probes, not committed, confirmed the entitlement check (unsigned false, ad-hoc true, reaching accountStatus() without trapping) and the black-screen cause and fix (gfx_init never called before, 1206x2622 -> ok and 60 Hz ticks after).
    • nestest.nes, dpcmletterbox.nes and AccuracyCoin.nes render, opened via simctl openurl.
    • The maintainer played it interactively.
  • The new ios.yml steps were run locally as written: the script succeeds, the clean-tree check passes, and the generic-simulator build succeeds with an x86_64 + arm64 app.
  • actionlint passes on ios.yml and release-auto.yml. markdownlint 0.49.1 passes on docs/ios.md and CHANGELOG.md.
  • No Rust code changed, so the Rust test suites were not re-run.

Not verified

  • On a real device: the device branch of the entitlement check (reading embedded.mobileprovision) has not run. App Store and TestFlight builds carry no profile and are treated as entitled.
  • The workflow on GitHub: ios.yml has not run there yet. Its first real run is the next release, or a manual dispatch, and its macos-latest image's Xcode is unknown.
  • The CloudKit sync itself: no iCloud account was signed in, so the account was always unavailable.

Review

All bot findings are adjudicated, with replies on the PR; every inline thread is resolved.

  • Copilot (1 review): 3 findings, fixed in 9c3fbd85.
  • CodeRabbit (full review of 9c3fbd85): no actionable comments, merge risk minimal. Its summary items: least-privilege secrets and docstrings fixed in c6d2bce7; a generation-fencing hardening proposal deferred, since CodeRabbit marks it pre-existing.
  • Antigravity (3 rounds): round 1's blocking finding fixed in af092df8; round 2's blocking claim ("won't compile") refuted, since it builds; round 3 has no blocking issues. Repeated suggestions declined with evidence.
  • Docs7: passed on every head.

Documentation

  • Code comments added/updated
  • User documentation updated (docs/ios.md, CI paragraph)
  • CHANGELOG.md updated

Review closeout

  • Every review thread replied to and resolved
  • Every review body read as well

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Fixed iOS game launch behavior when display sizing is deferred or the app returns to the foreground.
    • Improved CloudKit handling when iCloud access is unavailable, including clearer reporting of sync failures.
    • Corrected handling of missing FDS BIOS files.
  • Build & Reliability

    • iOS builds now run automatically for new releases and verify that build steps leave tracked files unchanged.
    • Improved iOS build compatibility with a default deployment target.

doublegate and others added 8 commits October 2, 2026 00:01
…llide

The repository tracks two directories whose names differ only in case:
tests/roms/accuracycoin/ (the runtime AccuracyCoin.nes, its MIT LICENSE
and a README) and tests/roms/AccuracyCoin/ (SOURCE_CATALOG.tsv, the
sub-test ROMs, the mirror build and a README). On a case-insensitive
filesystem (macOS APFS by default, Windows NTFS) the two are one
directory, so the two README.md paths fold onto the same file. A fresh
`gh repo clone` warns "the following paths have collided", checks out
only one of the pair, and the working tree starts dirty: the uppercase
README.md shows as modified because it holds the lowercase file's bytes.

The two README.md files were the only colliding pair: the LICENSE and
.nes files exist on only one side, so they coexist in the folded
directory. `git ls-files | tr A-Z a-z | sort | uniq -d` is now empty.

The fix renames the lowercase README to RUNTIME.md, which is the
smallest change that removes the collision. Merging the two directories
was rejected because about 20 sites hard-code
tests/roms/accuracycoin/AccuracyCoin.nes (the harness, the netplay
determinism tests, pgo_trainer and the trace tools). The new file states
why it is not called README.md. The one code comment that named the old
path (tests/accuracycoin.rs:64) now points at RUNTIME.md. The cost is
that GitHub no longer renders a README when browsing the lowercase
directory.

On macOS, `git add` with core.ignorecase=true recorded the new file
under the uppercase directory. The index entry was written explicitly
with `git update-index --cacheinfo` so the lowercase path that Linux CI
expects is the one tracked; new files under either directory need the
same check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…face

The first compile of the iOS app on a Mac (Xcode 27.0, iOS 27.0 simulator
SDK) failed with 24 errors from two causes. CI never saw either: the iOS
job builds the Rust xcframework and runs `xcodegen generate`, but never
compiles the Swift target, so both shipped in v2.9.7 green.

1. Two types named `NesButton`. `rustynes-mobile` exports
   `pub enum NesButton` (`#[derive(uniffi::Enum)]`, the press/release
   convenience for `NesController::set_button`), which UniFFI emits into
   ios/Generated/RustyNESCore.swift as `public enum NesButton`. The app
   declares its own `enum NesButton: UInt8` in NesButtons.swift, the
   single-bit mask values the touch overlay and gamepad mapper OR into
   the `set_buttons(port, mask)` wire byte. Both live in one module, so
   every use was "ambiguous for type lookup" and the declaration an
   "invalid redeclaration". The app's enum is renamed `NesButtonBit`
   (it is a bit value, which the old name never said). The bridge's
   type, and so the Kotlin binding and the Android app, are untouched.
   The rename is word-bounded, so `NesButtonMask` and the file name
   `NesButtons.swift` keep their names. The app does not call the
   bridge's `setButton`; it builds the mask itself.

2. `catch MobileError.missingFdsBios` (AppModel.swift, v2.9.7 FDS BIOS
   prompt). UniFFI's Swift generator keeps a Rust error enum's variant
   names verbatim, so the case is `MobileError.MissingFdsBios`, like the
   existing `RomLoad` / `SaveState` / `InvalidPort` cases. As written,
   the pattern did not compile; corrected to the generated name.

Verified: `xcodebuild -scheme RustyNES -destination 'platform=iOS
Simulator,name=iPhone 18 Pro'` reports BUILD SUCCEEDED, and the app
launches and runs on the simulator.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ios/project.yml gave the RustyNES target an `info: path:
RustyNES/Info.plist` block with no `info.properties`. XcodeGen treats
`info.path` as an output: it GENERATES that file from `properties`, so
every run of scripts/build-ios-xcframework.sh (which ends in `xcodegen
generate`) replaced the tracked 65-key plist with a bare template. The
overwrite dropped CFBundleDocumentTypes (the .nes/.fds/.nsf/.rns/.rnm
document types), UTImportedTypeDeclarations, UIAppFonts,
CFBundleDisplayName, CFBundleLocalizations, UILaunchScreen, the
orientation lists and CADisableMinimumFrameDurationOnPhone (the 120 Hz
ProMotion unlock). It showed up as -232 lines of `git diff` after a
local build. In CI the job runs on a throwaway checkout, so any
Xcode build or fastlane archive there ran on the stripped plist and
nobody saw the diff.

The block is removed. `INFOPLIST_FILE: RustyNES/Info.plist` in the
target's settings already points Xcode at the tracked file, which is
all the project needs. A comment at the old site records why the
block must not come back.

Verified: after `xcodegen generate --spec ios/project.yml`,
`git status` shows Info.plist unmodified, and the app builds and
launches.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
scripts/build-ios-xcframework.sh never set IPHONEOS_DEPLOYMENT_TARGET.
rustc and the `cc` crate both fall back to the installed SDK's version
when it is unset, so with Xcode 27 every C object in librustynes_ios.a
was compiled for iOS 27.0: Lua 5.5 (lua-src, via mlua), rcheevos and
ring. The app's deployment target is 17.0 (ios/project.yml), and the
link printed one "object file ... was built for newer 'iOS-simulator'
version (27.0) than being linked (17.0)" warning per object: 76 in
one build. The warning is a real hazard, not noise. Such an object may
reference symbols that only exist on newer iOS, and on an iOS 17-26
device that is a launch-time dyld failure.

The script now exports IPHONEOS_DEPLOYMENT_TARGET=17.0, kept in step
with project.yml by a comment, and a caller can still override it from
the environment.

A clean build picks it up. An incremental one does not fully: ring and
rcheevos rebuild, but lua-src's build script does not declare the
variable as a rerun trigger, so a warm target/ keeps its 27.0 Lua
objects until `cargo clean -p lua-src` for each iOS target. Verified
after that clean: 0 "built for newer" warnings, BUILD SUCCEEDED.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An iOS build without the iCloud container entitlement aborted at launch:

  *** Terminating app due to uncaught exception 'CKException',
  reason: 'containerIdentifier can not be nil'
    ... CloudSaveStateSync.refreshAccount() <- CloudSaveStateSync.start()

AppModel calls `cloudSaveStates.start()` at launch, and start() ran
`CKContainer.default().accountStatus()` unconditionally, even though
save-state sync is opt-in and off by default. `CKContainer.default()`
derives its id from the signed `icloud-container-identifiers`
entitlement and raises an Objective-C exception when there is none,
which Swift's `try?` cannot catch. Any unsigned simulator build hit it,
and so would a sideload whose profile lacks the iCloud capability. The
file header and RustyNES.entitlements both promised it "gracefully
no-op[s]" until provisioned; it did not.

Two changes:

* start() and refreshAccount() return early when sync is disabled, so
  a default install never touches CloudKit.

* The container is created only when the binary carries the
  entitlement: `container` and `database` are now optional, and every
  call site treats nil like a signed-out account, so the slot reports
  unavailable and the local save is never blocked. Creating the
  container explicitly does not avoid the trap:
  `CKContainer(identifier:)` was measured to stop in a `brk` inside its
  initialiser on the iOS 27 simulator. So the entitlement is probed
  first, with public API only (`CloudKitEntitlement.isPresent`):
    - simulator: the main executable's __TEXT,__entitlements section,
      which Xcode links in from the Simulated.xcent and which an
      unsigned build lacks (getsectiondata on _dyld_get_image_header(0));
    - device: the Entitlements dictionary of embedded.mobileprovision,
      read from the CMS envelope by its plist delimiters. App Store and
      TestFlight builds carry no embedded profile and are always signed
      with the capability, so a missing profile counts as entitled.

Measured on the iPhone 18 Pro simulator (iOS 27.0). Each of four builds
was installed fresh and launched with `cloudSaveStates` forced via
`defaults write`:

  unsigned, sync off -> runs, no crash report   (was: CKException)
  unsigned, sync on  -> runs, no crash report   (was: brk in init)
  ad-hoc,   sync off -> runs, no crash report
  ad-hoc,   sync on  -> runs, no crash report

A temporary NSLog probe, removed before this commit, showed the
unsigned build reading isPresent=false and the ad-hoc build
isPresent=true, the latter reaching `accountStatus()` without trapping
(accountAvailable=false: the simulator has no iCloud account).

The device branch, embedded.mobileprovision, is unexercised: no
device was available, and it needs a check on hardware.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ck screen)

Every game opened to a black picture under a working controller
overlay: the ROM loaded, but no frame ever ran or presented.

MetalGameView's Coordinator.attachAndStart() is called from makeUIView,
before SwiftUI lays the MTKView out, so `view.drawableSize` is 0 x 0
there and the renderer build is deferred. The deferred build had two
retry paths, and neither could run:

* `step(_:)`, the CADisplayLink callback, rebuilds when it sees a real
  drawable size. But the display link was created only at the END of a
  successful attachAndStart(), so with the build deferred there was no
  link and `step` never ran.
* `mtkView(_:drawableSizeWillChange:)` was an intentional no-op. Calling
  attachAndStart() from it would not help either, because
  `view.drawableSize` still reads 0 x 0 inside the callback.

The only remaining retry was appWillEnterForeground, so a background
and foreground round trip was the one way a game ever started.

Measured on the iPhone 18 Pro simulator (iOS 27.0) with temporary NSLog
probes (not committed). Before the fix:

  attachAndStart drawableSize=(0.0, 0.0)
  drawableSizeWillChange (1206.0, 2622.0)
  (no step tick, no rustynes_ios_gfx_init call, ever)

After it:

  attachAndStart drawableSize=(0.0, 0.0)   <- makeUIView, deferred
  step #1
  attachAndStart drawableSize=(1206.0, 2622.0)
  gfx_init 1206x2622 -> ok
  step #61, step #121, ...                 <- 60 Hz

The fix starts the display link first in attachAndStart(), before the
size guard. `EmulatorCore.tick()` returns early until the renderer
exists, so ticking before the build is free, and `step`'s existing
resize branch completes the build on the first tick with a real size.
Because the link can now exist before the renderer does, the
foreground handler's not-yet-attached branch also unpauses it.
Otherwise a background trip before the first layout would leave it
paused, since appDidEnterBackground pauses it.

This was a host-side bug. The wgpu/Metal renderer in rustynes-ios
builds and presents on the simulator once it is called. Verified by
opening nestest.nes, dpcmletterbox.nes and AccuracyCoin.nes through
`simctl openurl`: each renders its title screen.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… all

Two gaps, found while building the iOS app on a Mac for the first time:

1. Nothing in CI compiled the Swift app. ios.yml builds the Rust
   xcframework and runs `xcodegen generate`, but never runs xcodebuild on
   the Swift target. So v2.9.7 shipped an app that did not compile: 24
   errors, from a `NesButton` collision with the UniFFI-generated type
   and a mis-cased `MobileError.MissingFdsBios` (fixed in 196b92c).

2. ios.yml had not run for a release since v2.3.8. `gh run list
   --workflow ios.yml` shows its last tag run on 2026-08-20 (v2.3.8),
   and nothing since but a skipped cron. Releases are now cut by
   release-auto.yml, which pushes the tag with GITHUB_TOKEN, and GitHub's
   recursion guard stops such a tag firing `on: push: tags`. release-auto
   already works around this for release.yml by calling it
   (`workflow_call`), but never did so for ios.yml. So v2.3.9 through
   v2.9.7, about forty releases, built no iOS host at all.

Changes:

* ios.yml gains a `workflow_call` trigger with a `tag` input (the same
  shape as release.yml) and checks out `inputs.tag || github.ref`.
  release-auto.yml gains an `ios` job that calls it for every release
  it cuts, with `secrets: inherit` so the existing TestFlight upload
  works once signing is provisioned. The "Detect iOS signing secrets"
  step still skips the upload when the secrets are absent. The job runs
  alongside `build`, so a failure marks the release run red without
  touching the attached binaries.

* After the xcframework step, ios.yml now:
  - fails if the script modified any tracked file (`git diff
    --exit-code`). It rewrote ios/RustyNES/Info.plist on every run until
    c6ca71d, and on a throwaway checkout nothing noticed;
  - runs `xcodebuild build` of the RustyNES scheme for
    `generic/platform=iOS Simulator` with CODE_SIGNING_ALLOWED=NO. No
    device or certificate is involved, so it runs whether or not
    signing is provisioned;
  - uploads the xcodebuild log as an artifact on failure.

* docs/ios.md: the CI paragraph said "gated to tag pushes", which has
  been untrue in practice since v2.3.9. It now describes the
  release-auto call and the Swift build.

Cost, by the maintainer's direction (2026-10-02): macOS jobs run at
release time only, never on PRs, pushes to main or the weekly cron.
ci.yml is untouched. The trade-off is that the check cannot block a
release: release-auto creates the tag and GitHub Release before calling
ios.yml, so a broken app still ships, and the run turns red afterwards.
For a pre-merge answer, dispatch "iOS" by hand on the release branch.

Verified locally (Xcode 27.0, macOS arm64): actionlint passes on both
workflow files. The new steps run as written: the script succeeds,
`git diff --exit-code` over ios/ and scripts/ is clean, and the generic
simulator build reports BUILD SUCCEEDED with an x86_64 + arm64 app. The
workflow itself has not run on GitHub yet; its first real run is the
next release, or a manual dispatch.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…er Unreleased

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@doublegate
doublegate requested a balanced review from Copilot October 2, 2026 04:42
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: doublegate/RustyNES/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c6ce95c5-4f7e-490e-b9b5-f2229fdf44d5

📥 Commits

Reviewing files that changed from the base of the PR and between e3debc9 and 9c3fbd8.

📒 Files selected for processing (16)
  • .github/workflows/ios.yml
  • .github/workflows/release-auto.yml
  • CHANGELOG.md
  • crates/rustynes-test-harness/tests/accuracycoin.rs
  • docs/ios.md
  • ios/RustyNES/AppModel.swift
  • ios/RustyNES/CloudSaveStateSync.swift
  • ios/RustyNES/ControlPadLayout.swift
  • ios/RustyNES/GameControllerManager.swift
  • ios/RustyNES/MetalGameView.swift
  • ios/RustyNES/MultiTouchControlPad.swift
  • ios/RustyNES/NesButtons.swift
  • ios/RustyNES/TouchControlsOverlay.swift
  • ios/project.yml
  • scripts/build-ios-xcframework.sh
  • tests/roms/accuracycoin/RUNTIME.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The changes add an iOS Simulator build to release automation, update iOS build configuration and runtime behavior, and change the AccuracyCoin test harness documentation reference.

Changes

iOS Build and Runtime

Layer / File(s) Summary
iOS build configuration
ios/project.yml, scripts/build-ios-xcframework.sh
XcodeGen no longer generates an Info.plist at the tracked plist path. The XCFramework build script exports IPHONEOS_DEPLOYMENT_TARGET, defaulting it to 17.0 when unset or empty.
iOS controls and renderer behavior
ios/RustyNES/NesButtons.swift, ios/RustyNES/ControlPadLayout.swift, ios/RustyNES/GameControllerManager.swift, ios/RustyNES/MultiTouchControlPad.swift, ios/RustyNES/TouchControlsOverlay.swift, ios/RustyNES/MetalGameView.swift, ios/RustyNES/AppModel.swift, CHANGELOG.md
The button enum is renamed to NesButtonBit across control code. The renderer keeps its display link active when drawable sizing delays attachment and resumes it after foreground return. The BIOS-missing handler matches MobileError.MissingFdsBios. The changelog records these and other Unreleased fixes.
CloudKit entitlement and availability checks
ios/RustyNES/CloudSaveStateSync.swift
CloudKit access now depends on detecting the iCloud-container entitlement. Account checks, uploads, deletes, and reconciliation handle a missing container or database; delete and fetch failures are logged as described in the change.
Release-time iOS build
.github/workflows/ios.yml, .github/workflows/release-auto.yml, docs/ios.md, CHANGELOG.md
The release workflow invokes the iOS workflow with the prepared tag. The iOS workflow checks out that tag, checks for tracked-file changes after the XCFramework build, builds the app unsigned for a generic iOS Simulator, and uploads the xcodebuild log on failure. The documentation describes the workflow and its release trigger behavior.

AccuracyCoin ROM Documentation

Layer / File(s) Summary
ROM documentation reference
crates/rustynes-test-harness/tests/accuracycoin.rs, tests/roms/accuracycoin/RUNTIME.md
The test harness now references RUNTIME.md. The document explains the filename choice for case-insensitive filesystems.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ReleaseWorkflow as release-auto.yml
  participant IOSWorkflow as ios.yml
  participant Checkout as Git checkout
  participant Xcode as xcodebuild
  ReleaseWorkflow->>IOSWorkflow: Call with release tag
  IOSWorkflow->>Checkout: Check out supplied tag
  IOSWorkflow->>Xcode: Build unsigned generic iOS Simulator app
  Xcode-->>IOSWorkflow: Return build status and log
Loading

Merge Risk: ⚪ Minimal · up to 9c3fb

The iOS build, launch and CI fixes show no concrete merge-blocking issue. The real-device iCloud path and the new release workflow have not yet been run, so check them after merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 9c3fb

The changes add useful release-time validation and avoid cloud access when sync is disabled or unavailable. No introduced security vulnerability was established. Remaining uncertainty concerns signed-device entitlement handling and cloud operations during account changes. The new iOS validation runs after release publication rather than blocking it.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The privileged release path can access App Store Connect credentials and signing-repository credentials, while its GitHub token is limited to contents read. The signing configuration names com.doublegate.rustynes, but the supplied evidence does not establish the credentials' actual roles or maximum account/repository scope, so their authority cannot be bounded to that app alone.

Trust Boundaries and Controls

  • observed — Automatic release execution is restricted to successful CI runs originating from pushes to main. Preparation targets that CI head SHA, the iOS caller passes its prepared tag, and checkout disables credential persistence. Unsigned compilation must succeed before the conditional credential-bearing signing steps execute.
  • observed — Cloud save synchronization is off by default, uses the user's private CloudKit database, and now avoids container access at disabled startup. Uploads retain newer-save checks and conditional server-record writes; the entitlement gate does not replace those existing consistency controls.

Resilience and Maintainability Implications

  • observed — Cloud uploads and deletions remain independent asynchronous tasks, with game-SHA checks rather than account or session-generation fencing. These mechanisms predate the PR; the changes add availability guards and deletion/fetch error logging, not cancellation or deletion ordering. Runtime account-switch isolation and deletion recovery remain unverified, rather than established new security findings.

Hardening Proposals

  • proposed — If disabling sync or deleting a slot must invalidate outstanding work, consider account/session/slot generation fencing and persistent deletion intent. Focused account-switch and interrupted-deletion validation would clarify that ownership guarantee; this is a hardening proposal, not a verified PR-introduced vulnerability.
🚥 Pre-merge checks | ✅ 8 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 63.16% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 10 files. (6 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (8 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Docs-As-Spec Sync ✅ Passed PASS — The authoritative PR diff changes no files under crates/rustynes-cpu, crates/rustynes-ppu, crates/rustynes-apu, or crates/rustynes-mappers. The docs-as-spec condition is therefore not a…
Changelog Entry For User-Visible Changes ✅ Passed CHANGELOG.md gains a ## [Unreleased] section with Fixed entries for the iOS compile failure, launch crash, black screen, and iOS build behavior, plus the case-sensitive ROM path fix. It also recor…
No Unwrap/Expect/Panic On Untrusted Input ✅ Passed PASS: The authoritative PR diff adds no .unwrap(), .expect(), or panic!() calls. The only matching occurrence in a changed file is an unchanged .expect(...) in `crates/rustynes-test-harness/te…
Safety Comment On New Unsafe Blocks ✅ Passed The pull request adds no unsafe { ... } block and no unsafe fn. The only changed Rust file updates a documentation path in crates/rustynes-test-harness/tests/accuracycoin.rs; its diff contains n…
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary iOS fixes and the added release-time iOS CI. It is specific, concise, and related to the changeset.
Full details: Docstring Coverage

Explanation

Docstring coverage is 63.16% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 10 files. (6 skipped: 6 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@context7

context7 Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Docs7 for doublegate/rustynes

Result Status Action
Deployment ➖ Not used —
Content review ✅ Passed. No problems found. View findings

Commit c6d2bce

@github-actions

github-actions Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Antigravity review (Gemini via Ultra)

This PR configures CI to compile the iOS host during releases and fixes several iOS runtime issues, including a CloudKit entitlement crash and an unstarted display link.

Blocking issues

None found.

Suggestions

  • tests/roms/accuracycoin/RUNTIME.md (line 9): Renaming README.md fixes the file-level collision, but tracking both accuracycoin/ and AccuracyCoin/ in Git will still cause them to merge into one directory on case-insensitive filesystems (macOS/Windows), causing git status confusion. Consider unifying them into a single directory.

Nitpicks

  • ios/RustyNES/CloudSaveStateSync.swift (profileEntitlements): Extracting the plist by manually searching for <?xml and </plist> byte boundaries inside the CMS envelope is brittle. It works for a best-effort check, but will break if Apple changes the .mobileprovision internal formatting or adds whitespace.
  • .github/workflows/ios.yml (line 182): Piping xcodebuild directly to tee keeps the raw output, which generates massive and unreadable CI logs. Consider piping it through xcbeautify or xcpretty.

Automated first-pass review by agy on a self-hosted runner -- not a human review.

Earlier review rounds (newest first)
Round reviewed at 2026-10-02 05:10 UTC

Antigravity review (Gemini via Ultra)

This PR restores iOS compilation by resolving UniFFI type collisions, introduces a release-time CI check for the Swift app, and prevents launch crashes on unentitled builds by making CloudKit sync safely opt-in.

Blocking issues

  • Build Failure: In ios/RustyNES/CloudSaveStateSync.swift, the optional pattern matching syntax if case .failure(let error) = deleted[id] will fail to compile. Because deleted is a dictionary, deleted[id] returns an Optional<Result<Void, Error>>. You must unwrap the optional using a ?, for example: if case .failure(let error)? = deleted[id].

Suggestions

  • tests/roms/accuracycoin/RUNTIME.md: While renaming this file fixes the git dirty-state on checkout, it leaves two directories (accuracycoin/ and AccuracyCoin/) that still collide and merge on case-insensitive filesystems (macOS/Windows). Consider renaming one of the directories entirely to prevent future file-level collisions.
  • ios/RustyNES/CloudSaveStateSync.swift: Parsing the XML from embedded.mobileprovision using Data.range(of: Data("<?xml".utf8)) assumes the <?xml string doesn't appear earlier in the CMS envelope's binary headers. This is generally safe for Apple's profiles, but could be brittle if the envelope format changes.

Nitpicks

  • In .github/workflows/ios.yml, consider setting CODE_SIGNING_REQUIRED=NO alongside CODE_SIGNING_ALLOWED=NO for the xcodebuild step, as newer Xcode versions sometimes still complain about missing identities without both.

Automated first-pass review by agy on a self-hosted runner -- not a human review.

Round reviewed at 2026-10-02 04:56 UTC

Antigravity review (Gemini via Ultra)

This PR fixes iOS compilation and launch issues, prevents a CloudKit initialization crash on unentitled builds, and adds an unsigned iOS simulator build to the release CI pipeline to catch future regressions.

Blocking issues

  • Silent failure paths: The project style guide explicitly forbids swallowed errors and ignored return values, but the touched lines in ios/RustyNES/CloudSaveStateSync.swift perpetuate two:
    • In delete(sha:slot:): _ = try? await database.modifyRecords(...) completely drops errors and ignores the return value.
    • In reconcile(sha:): the catch { return } block swallows CloudKit fetch errors without logging.

Suggestions

  • Directory casing collision (CHANGELOG.md / accuracycoin/RUNTIME.md): Renaming README.md to RUNTIME.md fixes the immediate file collision, but retaining both accuracycoin/ and AccuracyCoin/ directories still creates a merged, messy directory on case-insensitive filesystems (macOS/Windows). Consider renaming one of the directories completely (e.g., tests/roms/accuracycoin-run/).

Nitpicks

  • ios/RustyNES/CloudSaveStateSync.swift: _dyld_get_image_header(0) assumes the main executable is strictly at index 0. This is practically safe for the simulator, but using dladdr is generally less brittle.

Automated first-pass review by agy on a self-hosted runner -- not a human review.

@doublegate

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Pull request base or head changed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

PR #578 review (Antigravity, blocking): two CloudKit paths in
CloudSaveStateSync dropped their errors with no record, against the
project rule on swallowed errors. The same file already logs every
upload failure with NSLog.

* delete(sha:slot:) ran `_ = try? await database.modifyRecords(...)`.
  It now logs a thrown error and a per-record failure, because
  modifyRecords reports a per-record failure inside its result, not by
  throwing. The exception is `.unknownItem`, which only means the slot
  was never uploaded and is the expected case for a local-only slot.
  The call stays best-effort: the local delete never waits on it.

* reconcile(sha:)'s fetch `catch { return }` now logs before returning.
  The behaviour is unchanged (an offline or transient failure keeps the
  locally derived slot states), but the failure is recorded.

Both lines predate this PR, which only renamed `Self.database` to the
unwrapped `database` on them. Fixed here because the review is right
and the fix is local.

Verified: `xcodebuild build` for generic/platform=iOS Simulator,
unsigned, reports BUILD SUCCEEDED with no warnings in this file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The retained scheduled workflow contradicts the stated release-only, no-cron policy and accompanying documentation.

Review effort: Balanced
Findings: 1 Medium severity · 2 Low severity

Open (3)
What changed in this PR

Fixes iOS compilation, launch, rendering, build tooling, and release-time validation.

Changes:

  • Resolves Swift/UniFFI naming and FDS error mismatches.
  • Fixes CloudKit launch crashes and deferred Metal initialization.
  • Adds iOS app compilation to release automation.
File Description
tests/​roms/​accuracycoin/​RUNTIME.md Avoids case-insensitive path collision.
scripts/​build-ios-xcframework.sh Sets the iOS 17 deployment target.
ios/​RustyNES/​TouchControlsOverlay.swift Updates renamed button type documentation.
ios/​RustyNES/​NesButtons.swift Renames the local button enum.
ios/​RustyNES/​MultiTouchControlPad.swift Uses the renamed enum.
ios/​RustyNES/​MetalGameView.swift Starts rendering retries before attachment.
ios/​RustyNES/​GameControllerManager.swift Uses the renamed enum.
ios/​RustyNES/​ControlPadLayout.swift Updates button layout types.
ios/​RustyNES/​CloudSaveStateSync.swift Safely gates CloudKit initialization.
ios/​RustyNES/​AppModel.swift Corrects the generated FDS error case.
ios/​project.yml Preserves the tracked property list.
docs/​ios.md Documents iOS CI behavior.
crates/​rustynes-test-harness/​tests/​accuracycoin.rs Updates the runtime documentation path.
CHANGELOG.md Records the fixes and CI addition.
.github/​workflows/​release-auto.yml Invokes the reusable iOS workflow.
.github/​workflows/​ios.yml Builds the Swift app and checks cleanliness.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/ios.yml Outdated
Comment thread docs/ios.md Outdated
Comment thread docs/ios.md Outdated
…d v2.9.8 (review)

PR #578 review (Copilot, 3 threads), all correct:

* "Release time only" was false. ios.yml keeps its pre-existing schedule
  trigger, the every-other-month TestFlight refresh (builds expire after
  90 days), and that cron runs the same macOS job, including the new
  Swift build, whenever the IOS_SIGNING_READY repo variable is "true".
  It is dormant today: `gh variable list` shows no variables, and its
  last cron run (2026-09-01) was skipped. The cost constraint behind
  this PR keeps macOS jobs off pull requests, off pushes to main and off
  ci.yml's weekly cron. The refresh cron is TestFlight's renewal
  mechanism and stays. The workflow header comment and docs/ios.md now
  say exactly that: never on a PR; for a release, by hand, and on the
  dormant refresh cron.

* "until the call was added at v2.9.8" and "Until v2.9.8 it rewrote ..."
  presented an unreleased version as released (the current release is
  v2.9.7, and the change sits under CHANGELOG [Unreleased]). The docs now
  say the call is unreleased and first runs on the next release, and the
  workflow comment says "Through v2.9.7". The `e.g. v2.9.8` in the
  workflow_call input description is an example value and stays.

actionlint passes on ios.yml. markdownlint-cli 0.49.1 passes on
docs/ios.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@doublegate

Copy link
Copy Markdown
Owner Author

Re: Antigravity review, round 1 (comment of 04:44:34Z, on 5d88fcb)

Blocking: silent failure paths in CloudSaveStateSync.swift. Fixed in af092df. Both lines predate this PR, which only renamed Self.database to the unwrapped database on them, but the finding is right. The same file already logs every upload failure with NSLog, and these two paths did not.

  • delete(sha:slot:) now logs a thrown error, and also a per-record failure, since modifyRecords reports one inside its result rather than by throwing. The exception is .unknownItem, which only means the slot was never uploaded. The call stays best-effort: the local delete never waits on it.
  • reconcile(sha:)'s fetch catch now logs before returning. Its behaviour is unchanged: an offline or transient failure keeps the locally derived slot states.
  • Verified with xcodebuild build for generic/platform=iOS Simulator, unsigned: BUILD SUCCEEDED, with no warnings in the file.

Suggestion: rename one of the accuracycoin/ / AccuracyCoin/ directories. Declined. About 20 sites hard-code tests/roms/accuracycoin/AccuracyCoin.nes: the harness and its catalog, the netplay determinism and UDP tests, pgo_trainer, the trace tools, NOTICE and docs/ppu-trace-tooling.md. After the README rename, no two tracked paths fold together: git ls-files | tr A-Z a-z | sort | uniq -d is empty. A folded directory on a case-insensitive filesystem is harmless when nothing collides. Renaming a directory that many tools depend on would be churn without a defect to fix.

Nitpick: _dyld_get_image_header(0) vs dladdr. Declined, because dladdr would be wrong here. In a Debug build the app's code lives in RustyNES.debug.dylib, and the __TEXT,__entitlements section is linked only into the main stub executable. The Xcode link line shows -sectcreate __TEXT __entitlements … RustyNES.app-Simulated.xcent on RustyNES, not on the dylib. dladdr on any of our symbols resolves to the debug dylib, which has no such section, so the probe would always report "not entitled". Image index 0 is dyld's main executable. A temporary probe confirmed it reads isPresent=true on an ad-hoc-signed simulator build and false on an unsigned one.

@doublegate

Copy link
Copy Markdown
Owner Author

Re: Docs7 (comment of 04:44:10Z): content review passed with no findings, so there is nothing to act on. Thanks.

@doublegate

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
❌ Action failed

Review failed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@doublegate

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@doublegate

Copy link
Copy Markdown
Owner Author

Re: Antigravity review, round 2 (comment updated 04:56:25Z, on 9c3fbd8)

Blocking: "if case .failure(let error) = deleted[id] will fail to compile." Refuted. It compiles: xcodebuild build -scheme RustyNES -destination 'generic/platform=iOS Simulator' CODE_SIGNING_ALLOWED=NO on af092df reports BUILD SUCCEEDED, with no errors or warnings in CloudSaveStateSync.swift (Xcode 27.0). An enum-case pattern also matches an optional wrapping that case, so .failure(let error) matches Optional<Result<Void, Error>>.some(.failure(_)) and simply does not match nil. The ? form is equivalent, not required. No change.

Suggestion: rename one of the accuracycoin/ / AccuracyCoin/ directories. Declined, as in round 1. No tracked paths collide (git ls-files | tr A-Z a-z | sort | uniq -d is empty), and about 20 sites hard-code tests/roms/accuracycoin/AccuracyCoin.nes.

Suggestion: <?xml search in embedded.mobileprovision. Acknowledged, no change. As the review notes, this is generally safe: the profile's CMS envelope carries the signed plist verbatim, and the search runs only up to the first </plist> after it. A mismatch would fail toward "not entitled", which means no iCloud sync, never a crash. The device branch is called out as unverified in the PR description; the device run sheet is where it gets exercised.

Nitpick: add CODE_SIGNING_REQUIRED=NO. Declined. CODE_SIGNING_ALLOWED=NO is the stronger setting (it disables signing outright), and the exact step as written built unsigned with no identity complaint on Xcode 27.0. If the runner's Xcode disagrees, the step's first run will say so, and the log is uploaded on failure.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

…tions (review)

PR #578 review (CodeRabbit, full review of 9c3fbd8: "No actionable
comments", Merge Risk minimal). Two items in its summary were acted on.

* Least privilege for the called workflow. Its security review noted
  that the release path can reach the App Store Connect and fastlane
  match credentials, and that their scope could not be bounded. The
  part this PR controls was `secrets: inherit` on release-auto's `ios`
  job, which handed ios.yml every repository secret. ios.yml reads
  exactly six:
    ASC_KEY_ID, ASC_ISSUER_ID, ASC_KEY_CONTENT, MATCH_GIT_URL,
    MATCH_PASSWORD, MATCH_GIT_BASIC_AUTHORIZATION
  (`grep -o 'secrets\.[A-Z_]*' ios.yml | sort -u`). ios.yml now declares
  those six under `workflow_call.secrets`, all `required: false`,
  because their absence is the normal unprovisioned state: the "Detect
  iOS signing secrets" step then skips the upload. release-auto now
  passes them by name. Tag pushes, dispatch and the cron read the repo's
  secrets directly, so they are unaffected.

* Docstring coverage (pre-merge check: 63% of the functions this diff
  touches, threshold 80%). Doc comments were added to the four touched
  functions that carried logic without one:
  CloudSaveStateSync.refreshAccount() (when it touches CloudKit, and
  what "unavailable" means to callers); CloudKitEntitlement's
  simulatorEntitlements(), which also records why image 0 and not
  `dladdr` (in Debug the app's code is in RustyNES.debug.dylib, and only
  the stub executable carries __TEXT,__entitlements); plist(_:); and
  MetalGameView.Coordinator.step(_:) (deferred build, 60 Hz lock, and
  the pacing fallback). The remaining undocumented touched function is
  a one-line local helper (`has(_:)`) inside a drawing closure.

Verified: actionlint passes on both workflows, and the generic
iOS-Simulator unsigned build reports BUILD SUCCEEDED.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@doublegate

Copy link
Copy Markdown
Owner Author

Re: CodeRabbit full review (run c6ce95c5, on 9c3fbd8: no actionable comments, Merge Risk minimal, no threads)

Pre-merge check, Docstring Coverage (63% vs 80%). Addressed in c6d2bce. Doc comments were added to the four touched functions that carried logic without one:

  • CloudSaveStateSync.refreshAccount(): when it touches CloudKit, and what "unavailable" means to callers.
  • CloudKitEntitlement.simulatorEntitlements(): this also records why it reads image 0 and not dladdr. In Debug the app's code is in RustyNES.debug.dylib, and only the stub executable carries __TEXT,__entitlements.
  • CloudKitEntitlement.plist(_:).
  • MetalGameView.Coordinator.step(_:): the deferred build, the 60 Hz lock and the pacing fallback.

The remaining undocumented touched function is a one-line local helper (has(_:)) inside a drawing closure.

Security blast radius (credentials whose scope cannot be bounded). The part this PR controls is fixed in c6d2bce. secrets: inherit on release-auto's ios job handed ios.yml every repository secret. ios.yml reads exactly six (ASC_KEY_ID, ASC_ISSUER_ID, ASC_KEY_CONTENT, MATCH_GIT_URL, MATCH_PASSWORD, MATCH_GIT_BASIC_AUTHORIZATION, per grep -o 'secrets\.[A-Z_]*'). It now declares those under workflow_call.secrets, all optional because absence is the normal unprovisioned state, and release-auto passes them by name. The credentials' own account-side scope (App Store Connect key role, match repository permissions) is configured outside this repository and is out of scope here.

Hardening proposal (generation fencing for sync and delete across account switches). Deferred. As the review states, this is pre-existing behaviour and not a PR-introduced vulnerability: the upload and delete tasks and their SHA checks predate this PR, which adds only availability guards and failure logging. It belongs with the CloudKit work that the device run sheet exercises, where an account switch can actually be tested, not in a build-fix PR.

Observed points (release gating, sync off by default, unsigned build before signing steps). These are accurate and need no action. The "runs after release publication" limit is stated in the PR description.

@doublegate

Copy link
Copy Markdown
Owner Author

Re: Antigravity review, round 3 (comment updated 05:10:58Z, on c6d2bce): no blocking issues.

Suggestion: unify accuracycoin/ and AccuracyCoin/. Declined, as in rounds 1 and 2. The new claim, "git status confusion", does not hold after this PR. A fresh macOS checkout of this branch has a clean git status, and git ls-files | tr A-Z a-z | sort | uniq -d is empty. The folded directory is only visible in Finder, and about 20 sites hard-code the lowercase path.

Nitpick: <?xml / </plist> extraction from embedded.mobileprovision. No change, as in round 2. Whitespace or formatting changes between those markers are inside the plist and are handled by PropertyListSerialization. Only a change to the markers themselves would matter, and the failure direction is "not entitled", meaning no iCloud sync, never a crash. The device branch is listed as unverified in the PR description.

Nitpick: pipe xcodebuild through xcbeautify / xcpretty. Declined. It adds an install step and a dependency to a job whose first real run is a release. The raw log is the point of the tee: it is the artifact uploaded on failure, and a formatter would drop exactly the lines needed to diagnose a link or signing failure.

Per this repo's review stopping rule (docs/agents/review-bots.md), this round brings only repeats and nits, so no further push will be made for it.

@doublegate
doublegate marked this pull request as ready for review October 2, 2026 05:13
@doublegate
doublegate merged commit 81c09c2 into main Oct 2, 2026
40 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants