feat(ce-review): add Swift/iOS stack-specific reviewer persona#565
feat(ce-review): add Swift/iOS stack-specific reviewer persona#565jcjvm wants to merge 1 commit intoEveryInc:mainfrom
Conversation
Adds a conditional reviewer for Swift and iOS codebases, covering SwiftUI view body complexity, state property wrapper misuse, memory retain cycles, async/await concurrency hazards, accessibility gaps, and magic numbers in financial logic. Follows the same pattern as kieran-typescript, kieran-python, and kieran-rails reviewers. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af3ac4da25
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| --- | ||
| name: swift-ios-reviewer | ||
| description: Conditional code-review persona, selected when the diff touches Swift files (.swift), Xcode project files (.pbxproj, .xcodeproj), storyboards, XIBs, or iOS-specific configuration (Info.plist, entitlements). Reviews Swift and iOS code for SwiftUI correctness, state management, memory safety, concurrency, accessibility, and financial logic integrity. |
There was a problem hiding this comment.
plugins/compound-engineering/AGENTS.md requires that every new agent be added to plugins/compound-engineering/README.md with updated counts, but this commit adds swift-ios-reviewer without any README update. I verified the docs still have no swift-ios entry in the Agents section, so the published plugin inventory is now out of sync with the actual shipped personas and users will miss this reviewer when discovering capabilities.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,99 @@ | |||
| --- | |||
| name: swift-ios-reviewer | |||
| description: Conditional code-review persona, selected when the diff touches Swift files (.swift), Xcode project files (.pbxproj, .xcodeproj), storyboards, XIBs, or iOS-specific configuration (Info.plist, entitlements). Reviews Swift and iOS code for SwiftUI correctness, state management, memory safety, concurrency, accessibility, and financial logic integrity. | |||
There was a problem hiding this comment.
Info.plist is in the trigger list but never appears in the hunt list below, so it fires the reviewer without giving it anything to look for.
The trigger list is also missing a few files that are core iOS review surface today:
PrivacyInfo.xcprivacy(Apple required since May 2024 for apps and SDKs using required-reason APIs like UserDefaults, file timestamps, system boot time, disk space, active keyboard)*.entitlements(keychain access groups, app groups, iCloud containers, push capability, associated domains)*.xcdatamodeld(schema changes that cause migration crashes on upgrade)Package.swift,Package.resolved(SPM dependency additions and version bumps carry the same supply-chain implications as package.json)
| View bodies that do too much work, triggering layout passes or recomputation on every state change. | ||
|
|
||
| - **Expensive computation inside `body`** -- sorting, filtering, date formatting, number formatting, or network-derived transforms that rerun on every view update. These should be computed properties, `.task` modifiers, or cached in the view model. | ||
| - **Deep view hierarchies without extraction** -- a single `body` property exceeding ~60 lines or nesting 4+ levels of containers. Large bodies are hard to diff-review and hide state dependencies. |
There was a problem hiding this comment.
The 60-line and 4+ level thresholds will produce false positives on real SwiftUI code that is fine. Peer personas deliberately avoid numeric heuristics. kieran-typescript just says "existing-file complexity that would be easier as a new module" and trusts the model to calibrate.
The genuinely iOS-specific failure mode here is that large bodies hide their dependency graph from SwiftUI's change tracker, which causes over-rendering and redundant layout passes. Naming that mode is sharper than a line count, and it survives refactors where the body gets shorter but still has the same problem.
| ### 4. Concurrency issues | ||
|
|
||
| Swift concurrency bugs around `async/await`, actors, `@MainActor`, and `Sendable` conformance. | ||
|
|
||
| - **Missing `@MainActor` on UI-mutating code** -- view models or functions that update `@Published` properties from a non-main-actor context. In Swift 6 strict concurrency this is a compile error; in Swift 5 it's a silent data race. | ||
| - **`Sendable` violations** -- passing non-`Sendable` types across actor boundaries (task groups, `Task { }` from main actor, actor method calls). Check whether the project uses strict concurrency checking (`-strict-concurrency=complete`). | ||
| - **Blocking the main actor** -- synchronous file I/O, `Thread.sleep`, `DispatchSemaphore.wait()`, or CPU-intensive computation on `@MainActor`-isolated code paths. These freeze the UI. | ||
| - **Unstructured `Task { }` without cancellation** -- fire-and-forget tasks spawned in `viewDidLoad`, `onAppear`, or init without storing the `Task` handle. If the view is dismissed, the task continues running and may mutate deallocated state. | ||
| - **Actor reentrancy surprises** -- `await` calls inside actor methods where mutable state may have changed between suspension and resumption. The classic pattern: read state, await something, use state assuming it hasn't changed. |
There was a problem hiding this comment.
Missing a major iOS concurrency bug class here: Core Data and SwiftData context threading.
NSManagedObject accessed off its context's queue, missing perform or performAndWait wrappers, main-context fetches from background threads, passing managed objects across contexts instead of object IDs. These are consistently one of the top crash classes in iOS apps that use Core Data, and nothing else in the persona catalog will catch them. Worth a dedicated bullet.
| ### 6. Magic numbers and hardcoded values in financial or business logic | ||
|
|
||
| Literal numeric values in calculations, thresholds, or business rules that should be named constants, configuration, or server-driven. | ||
|
|
||
| - **Hardcoded rates, fees, or percentages** -- tax rates, interest rates, transaction fees, or conversion factors embedded as raw numbers (`amount * 0.029 + 0.30`). These change with jurisdiction, plan tier, or regulation and must be named constants or fetched from configuration. | ||
| - **Hardcoded currency formatting** -- assuming 2 decimal places, USD symbol, or specific locale. Use `NumberFormatter` with `currencyCode` or `Decimal` with explicit rounding rules. | ||
| - **Floating-point arithmetic for money** -- using `Double` or `Float` for monetary calculations instead of `Decimal` or integer cents. Floating-point rounding errors accumulate and produce incorrect totals. | ||
| - **Hardcoded thresholds** -- magic numbers for limits, caps, minimums, or tier boundaries (e.g., `if amount > 10000`) without a named constant explaining the business rule. |
There was a problem hiding this comment.
Most of this section is not Swift-specific. Hardcoded rates, magic thresholds, and currency format assumptions are correctness and domain concerns that correctness-reviewer should own (or a future domain reviewer). Putting them here means every Swift diff gets reviewed against a finance-app lens that most iOS apps do not match.
The genuinely iOS-specific content is two things:
DecimalvsDoublefor money, which is a Swift type-choice callNumberFormatterwith an explicitlocaleandcurrencyCoderather than format-string assumptions
Consider collapsing the section to just those and letting the generic reviewers own magic numbers and thresholds. As written this section reads like it was imported from a different brief than the rest of the persona.
| - **UIKit vs SwiftUI choice** -- do not second-guess the framework choice. Review the code in whichever framework was chosen. | ||
| - **Minor naming disagreements** -- unless a name is actively misleading about state ownership or lifecycle behavior. | ||
| - **Test-only code** -- force unwraps, hardcoded values, and simplified patterns in test files are acceptable. Don't apply production standards to test helpers. | ||
| - **Generated code** -- `.pbxproj` changes, auto-generated asset catalogs, and Core Data model files. Review only human-authored Swift. |
There was a problem hiding this comment.
Two carve-outs to narrow here.
.xcdatamodeld should not be excluded. Adding a non-optional attribute without a default value, removing an entity, or changing a relationship's delete rule all cause migration crashes on upgrade. These are high-severity reviewable changes, not noise.
.pbxproj is too blunt. UUIDs and file-reference churn are noise, agreed. But these changes live inside pbxproj and matter:
- Target membership changes (a file silently leaving the app target, or a test file accidentally added to it)
- Build setting changes (optimization level,
ENABLE_BITCODE,OTHER_SWIFT_FLAGSdisabling strict concurrency,SWIFT_VERSIONbumps) - Embedded framework and linker flag changes
- Code signing identity and provisioning profile changes
Suggest narrowing the exclusion to "pure file-reference and UUID churn in .pbxproj, auto-generated asset catalogs" and keeping semantic build-setting, target membership, signing, and Core Data model changes in scope.
2 participants
Summary
swift-ios-reviewer.mdas a new stack-specific conditional persona in the ce-review pipeline.swiftfiles, SwiftUI views, UIKit controllers, Xcode project files (.pbxproj), storyboards, XIBs,Info.plist, or iOS entitlementskieran-typescript,kieran-python, andkieran-railsreviewersWhat the reviewer hunts for
body, deep hierarchies without extraction,@Statemutations during evaluation@ObservedObjectfor owned objects,@StateObjectfor injected deps,@Statefor reference types, missing@Published[weak self]in escaping closures, strong capture in Combinesink/assign, closure-based delegation cycles@MainActoron UI mutations,Sendableviolations, blocking main actor, unstructuredTask {}without cancellation, actor reentrancyDoublefor money instead ofDecimal, magic number thresholds without named constantsChanges
agents/review/swift-ios-reviewer.mdskills/ce-review/SKILL.mdskills/ce-review/references/persona-catalog.mdMotivation
The existing ce-review pipeline has stack-specific reviewers for Rails, Python, TypeScript, and frontend Stimulus/Turbo code, but no coverage for Swift/iOS. iOS projects have a distinct class of bugs around SwiftUI state management, ARC retain cycles, and Swift concurrency that generic reviewers don't catch. This fills that gap using the established conditional persona pattern.
🤖 Generated with Claude Code