fix(ci): make iOS post-login E2E hermetic - #486
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (36)
📝 WalkthroughWalkthroughThis PR makes iOS UI tests hermetic and fail-closed, adds deterministic fixtures and strict result/artifact checks, introduces token refresh and throwing session persistence, standardizes REST timestamps as RFC3339, and adds explicit E2E backend port-conflict handling. ChangesiOS authentication and persistence
Hermetic iOS UI-test gate
E2E backend port handling
Backend timestamp contracts
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
efcf451 to
979e637
Compare
|
The first real hosted Simulator run reached the exact-SHA backend setup and failed before Xcode because the source-built PostgreSQL 18.4 installation omitted the bundled Fixed in
Verification: exact PostgreSQL 18.4 source build + live create/drop smoke passed locally; 32 iOS CI contract/result tests, 19 fail-closed checks, actionlint, production hardening (250 checks + 97 Node + 18/16 Python), Swift build/behavior, i18n/parity, and independent code review all pass. New exact-head CI is the remaining proof. |
|
Follow-up database prerequisite closure at
Exact-head hosted validation is running at https://github.com/jason931225/maintenance/actions/runs/29900569211. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ios-ui-tests.yml:
- Around line 41-43: Remove the “Cache Rust dependencies + build artifacts” step
using Swatinem/rust-cache because its CARGO_HOME and CARGO_TARGET_DIR contents
are deleted by the final rm -rf "$D"; alternatively, relocate those Cargo
directories outside "$D" before retaining the cache step.
In `@docs/CI-GATES.md`:
- Around line 680-684: Update the CI-GATES documentation paragraph describing
OTP and session-material handling to distinguish database storage from temporary
files: state that the database fixture stores only the SHA-256 OTP digest, while
plaintext OTP and access/refresh tokens are briefly written to mode-0700
runner-local files, masked and scanned, then deleted.
In `@ios/Sources/MaintenanceFieldApp/FieldViews.swift`:
- Around line 565-568: Update the accessibility identifiers for the
search-result and open-thread rows in the messenger view so they cannot collide
when the same message appears in both sections. Add a distinct dynamic
identifier to FieldAccessibilityID, mirror it in FieldUITestCase.AID, and use
the section-specific identifiers at the ForEach blocks around
MessengerMessageRow.
In `@ios/Sources/MaintenanceFieldCoreBehaviorTests/main.swift`:
- Around line 2205-2211: Update the jwt(expiration:) helper to be throwing by
removing try! and propagating JSONSerialization errors; then mark both jwt call
sites around the existing throws contexts to use try. Preserve the current JWT
header, payload, and signature formatting.
In `@ios/UITests/MessengerUITests.swift`:
- Around line 63-66: Add the offline-fallback string under the
`messenger_send_pending` key in the shared `KO` localization helper, then update
the assertion in `MessengerUITests` to retrieve that key instead of hardcoding
the Korean text. Preserve the existing negative assertion and failure message.
In `@ios/UITests/Support/FieldUITestCase.swift`:
- Around line 157-196: Remove the redundant UITestFixture.workOrderID wrapper
and update its call sites to invoke requiredID directly with the same key and
environment parameters. Keep requiredID as the shared validation path for all
fixture identifiers.
In `@scripts/boot-backend-port-conflict.test.mjs`:
- Around line 1-97: Add coverage for the default reclaim behavior by creating a
stale loopback listener without SO_REUSEADDR, invoking boot() with
E2E_PORT_CONFLICT_MODE set to reclaim and the marker app configured, then assert
the boot succeeds, the stale listener exits, the port becomes available, and the
marker confirms backend startup.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: bc60f034-d163-47ce-bb7e-0be3b3197e71
📒 Files selected for processing (38)
.github/workflows/ios-ui-tests.ymlHANDOFF.mddocs/CI-GATES.mddocs/parity-checklist.mddocs/program/console-program-ledger.mde2e/harness/boot-backend.she2e/harness/seed-mech.sqle2e/harness/seed-mobile-ci.sqlios/Config/App.xcconfigios/Config/README.mdios/Sources/MaintenanceFieldApp/AppContainer.swiftios/Sources/MaintenanceFieldApp/FieldAccessibilityID.swiftios/Sources/MaintenanceFieldApp/FieldViewModel.swiftios/Sources/MaintenanceFieldApp/FieldViews.swiftios/Sources/MaintenanceFieldApp/Resources/ko.lproj/Localizable.stringsios/Sources/MaintenanceFieldCore/APIGateway.swiftios/Sources/MaintenanceFieldCore/AuthRepository.swiftios/Sources/MaintenanceFieldCore/LoginStateMachine.swiftios/Sources/MaintenanceFieldCore/PersistenceStores.swiftios/Sources/MaintenanceFieldCoreBehaviorTests/main.swiftios/UITests/AccessibilityAuditUITests.swiftios/UITests/CameraCaptureUITests.swiftios/UITests/FieldAccessibilityIDParityTests.swiftios/UITests/FieldCriticalPathUITests.swiftios/UITests/LoginValidationUITests.swiftios/UITests/MessengerUITests.swiftios/UITests/PreflightUITests.swiftios/UITests/Support/FieldUITestCase.swiftios/UITests/Support/RealBackendSession.swiftios/UITests/Support/RealSessionSeed.swiftios/project.ymlpackage.jsonscripts/boot-backend-port-conflict.test.mjsscripts/check-i18n.mjsscripts/check-ios-ui-test-fail-closed.mjsscripts/check-ios-ui-test-fail-closed.test.mjsscripts/verify-xcresult-test-results.mjsscripts/verify-xcresult-test-results.test.mjs
|
Implemented and locally verified the final iOS CI fixes at exact head Root cause of the remaining simulator failure: the candidate backend bound loopback correctly, but the WebAuthn relying-party origin/ID used the IP literal Hardening included:
Fresh evidence: 39/39 focused Node tests, 21/21 iOS contract checks, actionlint, YAML parse, bash syntax for all workflow run blocks, iOS string/i18n gates, Swift build and core behavior tests, 250 production-hardening checks, 97 Node production-hardening tests, 34 Python tests, package-lock integrity, and two independent APPROVE reviews. |
|
CI follow-up: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ios/Sources/MaintenanceFieldCore/PersistenceStores.swift (1)
162-181: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPropagate rollback failures from
restore(...). If one delete succeeds and a later restore write fails, the caller only sees the original delete error while the store can be left partially deleted. Surface the rollback failure too, and add coverage for delete failure followed by restore-write failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/Sources/MaintenanceFieldCore/PersistenceStores.swift` around lines 162 - 181, Update the private static restore(...) method to propagate write failures instead of suppressing them with try?, and make its callers preserve/report both the original delete error and any rollback failure. Add coverage for a delete failure followed by a restore-write failure, asserting that the surfaced error includes the rollback failure.ios/UITests/FieldAccessibilityIDParityTests.swift (1)
3-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClass name no longer matches its test content.
FieldAccessibilityIDParityTestsnow only tests hermetic-runner environment parsing (RealBackendSession,UITestFixture) — AID string parity moved to the host-sidecheck-ios-ui-test-fail-closed.mjsgate per the updated doc comment inFieldUITestCase.swift. Keeping the old name here is misleading for anyone searching for AID-parity coverage.Consider renaming the class/file (e.g.
HermeticRunnerConfigurationTests) to reflect its actual purpose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/UITests/FieldAccessibilityIDParityTests.swift` around lines 3 - 43, Rename the test class FieldAccessibilityIDParityTests and its file to reflect their current purpose, such as HermeticRunnerConfigurationTests, while preserving testHermeticRunnerConfigurationRequiresMintedPairAndNamedFixtures and its existing assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ios/Sources/MaintenanceFieldCore/PersistenceStores.swift`:
- Around line 284-289: Update the deferred cleanup in
KeychainAccessGroup.resolveShared to capture the status returned by
SecItemDelete. Treat errSecItemNotFound as expected, but log or assert when
deletion returns any other failure status so probe-item cleanup problems are
visible.
---
Outside diff comments:
In `@ios/Sources/MaintenanceFieldCore/PersistenceStores.swift`:
- Around line 162-181: Update the private static restore(...) method to
propagate write failures instead of suppressing them with try?, and make its
callers preserve/report both the original delete error and any rollback failure.
Add coverage for a delete failure followed by a restore-write failure, asserting
that the surfaced error includes the rollback failure.
In `@ios/UITests/FieldAccessibilityIDParityTests.swift`:
- Around line 3-43: Rename the test class FieldAccessibilityIDParityTests and
its file to reflect their current purpose, such as
HermeticRunnerConfigurationTests, while preserving
testHermeticRunnerConfigurationRequiresMintedPairAndNamedFixtures and its
existing assertions unchanged.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: ddcd4a36-9234-463e-b950-eb1ca46798df
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
.github/workflows/ios-ui-tests.ymldocs/CI-GATES.mdios/Config/App.xcconfigios/Config/MaintenanceFieldApp.entitlementsios/Config/MaintenanceFieldUITestSeeder.entitlementsios/Config/MaintenanceFieldUITests.entitlementsios/Config/README.mdios/Sources/MaintenanceFieldApp/FieldAccessibilityID.swiftios/Sources/MaintenanceFieldApp/FieldViews.swiftios/Sources/MaintenanceFieldCore/PersistenceStores.swiftios/Sources/MaintenanceFieldCoreBehaviorTests/main.swiftios/Sources/MaintenanceFieldUITestSeeder/UITestSeederApp.swiftios/UITests/FieldAccessibilityIDParityTests.swiftios/UITests/FieldCriticalPathUITests.swiftios/UITests/LoginValidationUITests.swiftios/UITests/MessengerUITests.swiftios/UITests/PreflightUITests.swiftios/UITests/Support/FieldUITestCase.swiftios/UITests/Support/RealSessionSeed.swiftios/project.ymlpackage.jsonscripts/boot-backend-port-conflict.test.mjsscripts/boot-ios-ui-backend.mjsscripts/boot-ios-ui-backend.test.mjsscripts/check-ios-ui-test-fail-closed.mjsscripts/check-ios-ui-test-fail-closed.test.mjs
💤 Files with no reviewable changes (1)
- ios/Config/MaintenanceFieldUITests.entitlements
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ios/Sources/MaintenanceFieldCore/APIGateway.swift (1)
182-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject an empty rotated access token.
Only
refreshTokenis validated here. A response withaccessToken == ""is saved and promoted, causing retries to sendBearerinstead of invalidating the session. Treat either empty token asinvalidSession, and add the corresponding behavior case.Proposed fix
case let .ok(response): let tokens = try response.body.json - guard let refreshToken = tokens.refreshToken, !refreshToken.isEmpty else { + guard !tokens.accessToken.isEmpty, + let refreshToken = tokens.refreshToken, + !refreshToken.isEmpty + else { throw SessionRefreshError.invalidSession }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/Sources/MaintenanceFieldCore/APIGateway.swift` around lines 182 - 187, Update the .ok response handling in APIGateway to validate that both tokens.accessToken and tokens.refreshToken are non-empty before constructing AuthTokens; throw SessionRefreshError.invalidSession when either is missing or empty, and add the corresponding behavior test case for an empty access token.ios/UITests/Support/FieldUITestCase.swift (1)
223-256: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the explicit
deinitrequired by the project's SwiftLint config.
FieldUITestCase(Line 232) trips therequired_deinitopt-in rule ("Classes should have an explicit deinit method"), which the project has evidently enabled. Aside from the lint gate, the rest of the setup/teardown refactor (unconditional seed insetUpWithError, gated clear intearDownWithError, appearance reset) looks correct.🛠️ Proposed fix
`@MainActor` class FieldUITestCase: XCTestCase { var app: XCUIApplication! private(set) var seededSession = false + + deinit {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/UITests/Support/FieldUITestCase.swift` around lines 223 - 256, Add an explicit deinit to FieldUITestCase to satisfy the enabled SwiftLint required_deinit rule, leaving the existing setup and teardown behavior unchanged.Source: Linters/SAST tools
ios/UITests/LoginValidationUITests.swift (1)
9-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicates
FieldUITestCase.launchSignedOutApp/launchAppwiring.This setup manually re-implements exactly what
FieldUITestCase.launchSignedOutApp()(clear +LaunchLocale.arguments+MAINTENANCE_API_BASE_URL+ launch) already does.LoginValidationUITestscan't simply subclassFieldUITestCasebecause itssetUpWithErrorunconditionally seeds a session, but the launch-environment wiring itself (env key, locale args) is now duplicated in three places across this cohort (here, and twice inPreflightUITests.swift), so a future change to the env var name or locale args risks silently diverging in one of them.♻️ Suggested extraction
+extension XCUIApplication { + /// Shared launch wiring for the runner's isolated local backend. + static func mntFreshInstance(_ presentation: Presentation = .standard) throws -> XCUIApplication { + let app = XCUIApplication() + app.launchArguments += LaunchLocale.arguments + app.launchArguments += presentation.launchArguments + app.launchEnvironment["MAINTENANCE_API_BASE_URL"] = try RealBackendSession.baseURL() + return app + } +} + override func setUpWithError() throws { try super.setUpWithError() continueAfterFailure = false try RealSessionSeed.clear() - app = XCUIApplication() - app.launchArguments += LaunchLocale.arguments - app.launchEnvironment["MAINTENANCE_API_BASE_URL"] = try RealBackendSession.baseURL() - app.launch() + app = try XCUIApplication.mntFreshInstance() + app.launch() }
FieldUITestCase.launchAppwould then delegate to the same helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/UITests/LoginValidationUITests.swift` around lines 9 - 17, Extract the shared app-launch wiring for locale arguments, MAINTENANCE_API_BASE_URL, and launch into a reusable helper near FieldUITestCase, and have FieldUITestCase.launchApp and launchSignedOutApp delegate to it. Update LoginValidationUITests.setUpWithError and the duplicated PreflightUITests setup paths to use that helper while preserving their distinct session-seeding behavior.
🤖 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 `@backend/crates/workorder/rest/src/lib.rs`:
- Around line 5009-5113: Extract shared fixture builders for the duplicated
EquipmentSummary, NamedEntity, and AssignmentSummary literals used by
work_order_list_item_serializes_timestamps_as_rfc3339_strings and this timestamp
test. Update both tests to call the helpers, preserving their current fixture
values and allowing future nested-field changes to be maintained in one place.
---
Outside diff comments:
In `@ios/Sources/MaintenanceFieldCore/APIGateway.swift`:
- Around line 182-187: Update the .ok response handling in APIGateway to
validate that both tokens.accessToken and tokens.refreshToken are non-empty
before constructing AuthTokens; throw SessionRefreshError.invalidSession when
either is missing or empty, and add the corresponding behavior test case for an
empty access token.
In `@ios/UITests/LoginValidationUITests.swift`:
- Around line 9-17: Extract the shared app-launch wiring for locale arguments,
MAINTENANCE_API_BASE_URL, and launch into a reusable helper near
FieldUITestCase, and have FieldUITestCase.launchApp and launchSignedOutApp
delegate to it. Update LoginValidationUITests.setUpWithError and the duplicated
PreflightUITests setup paths to use that helper while preserving their distinct
session-seeding behavior.
In `@ios/UITests/Support/FieldUITestCase.swift`:
- Around line 223-256: Add an explicit deinit to FieldUITestCase to satisfy the
enabled SwiftLint required_deinit rule, leaving the existing setup and teardown
behavior unchanged.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 319c9853-a4ec-4bea-8b70-2561a93b498b
⛔ Files ignored due to path filters (1)
backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/workflows/ios-ui-tests.ymlbackend/crates/compliance/rest/Cargo.tomlbackend/crates/compliance/rest/src/lib.rsbackend/crates/messenger/application/Cargo.tomlbackend/crates/messenger/application/src/lib.rsbackend/crates/workorder/rest/src/lib.rsios/Sources/MaintenanceFieldApp/FieldAccessibilityID.swiftios/Sources/MaintenanceFieldApp/FieldViews.swiftios/Sources/MaintenanceFieldCore/APIGateway.swiftios/Sources/MaintenanceFieldCoreBehaviorTests/main.swiftios/UITests/FieldCriticalPathUITests.swiftios/UITests/LoginValidationUITests.swiftios/UITests/PreflightUITests.swiftios/UITests/Support/FieldUITestCase.swiftios/UITests/Support/RealSessionSeed.swiftscripts/check-ios-ui-test-fail-closed.mjsscripts/check-ios-ui-test-fail-closed.test.mjs
💤 Files with no reviewable changes (2)
- ios/Sources/MaintenanceFieldApp/FieldViews.swift
- ios/Sources/MaintenanceFieldApp/FieldAccessibilityID.swift
🤖 I have created a release *beep* *boop* --- ## [0.2.1](v0.2.0...v0.2.1) (2026-07-23) ### Bug Fixes * **ci:** make iOS post-login E2E hermetic ([#486](#486)) ([3740621](3740621)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
Summary
macos-15.xcresultevidence against the source-discovered XCTest set, scan retained artifacts for raw session material, then prove identity-aware teardownTrust and portability boundary
Public/untrusted pull-request code runs only on GitHub-hosted macOS. It is never routed to a reusable self-hosted runner. The workflow's backend, PostgreSQL, session, paths, and cleanup contracts are portable seams; a future self-hosted/Oyatie Cloud lane must supply a separately governed ephemeral/JIT runner with teardown attestation rather than weakening this gate. Apple distribution signing/TestFlight remains a separate release boundary.
Fail-closed evidence
16F6, iOS 18.5 runtime, XcodeGen 2.46.0, PostgreSQL 18.4XCTSkip, optional fork suite, or conditional accessibility pathLocal verification
swift build --package-path iosswift test --package-path iosswift run --package-path ios MaintenanceFieldCoreBehaviorTestsnpm run check:swiftnpm run test:api-client-contract:swiftnpm run check:api-drift:swiftnpm run check:ios-ui-test-fail-closedgit diff --checkIndependent comprehensive review: APPROVE. Live Xcode/Simulator CI for this exact head remains the mandatory merge gate.
Closes #434
Summary by CodeRabbit