diff --git a/.claude/skills/mobile-sdk-ios-pr-review/SKILL.md b/.claude/skills/mobile-sdk-ios-pr-review/SKILL.md new file mode 120000 index 0000000000..a4fab9de83 --- /dev/null +++ b/.claude/skills/mobile-sdk-ios-pr-review/SKILL.md @@ -0,0 +1 @@ +../../../.prizm/presubmits/skills/mobile-sdk-ios-pr-review/SKILL.md \ No newline at end of file diff --git a/.prizm/presubmits/skills/mobile-sdk-ios-pr-review/SKILL.md b/.prizm/presubmits/skills/mobile-sdk-ios-pr-review/SKILL.md new file mode 100644 index 0000000000..b839972835 --- /dev/null +++ b/.prizm/presubmits/skills/mobile-sdk-ios-pr-review/SKILL.md @@ -0,0 +1,736 @@ +--- +name: mobile-sdk-ios-pr-review +description: Reviews PRs to the Salesforce Mobile SDK for iOS for public-API breakage, OAuth/credential safety, SQLCipher correctness, multi-user account regressions, missing localization, and iOS platform pitfalls. Tuned for a public open-source SDK where every change reaches external developers. + +prizm: + version: 1.0 + selectors: + paths: + include: + - "libs/**/*.swift" + - "libs/**/*.h" + - "libs/**/*.m" + - "libs/**/*.xib" + - "libs/**/*.storyboard" + - "libs/**/Configuration/*.xcconfig" + - "native/**/*.swift" + - "native/**/*.h" + - "native/**/*.m" + - "configuration/*.xcconfig" + - "*.podspec" + - "shared/resources/**/*.strings" + exclude: + - "**/build/**" + - "**/DerivedData/**" + - "**/Pods/**" + - "**/node_modules/**" + - "external/**" + metadata: + description: "Mobile SDK for iOS PR reviewer covering public API, OAuth, SQLCipher, multi-user, localization, and iOS/Swift/Obj-C platform concerns" + author: "Salesforce Mobile SDK" + tags: + - mobile-sdk + - ios + - swift + - objective-c + - public-api + - oauth + - sqlcipher + - multi-user + presubmit: + level: 'warn' + complexity: 'high' + docs_url: 'https://developer.salesforce.com/docs/platform/mobile-sdk/guide' + failure_help: | + The PR appears to break a public-SDK contract, leak credentials/PII, + misuse SQLCipher/Keychain, or skip localization/deprecation discipline. + The Mobile SDK ships to thousands of external apps — every public-API + change requires a deprecation cycle, every credential path needs review, + and every user-facing string needs localization. See: + - CLAUDE.md (Code Review Checklist, Escalation rules) + - iOS deprecation page: https://developer.salesforce.com/docs/platform/mobile-sdk/guide/ios-current-deprecations.html + - Mobile SDK Development Guide: https://developer.salesforce.com/docs/platform/mobile-sdk/guide + Address the rationale and re-request review, or escalate to a Mobile SDK + maintainer if the change is intentional and a deprecation/migration plan + is documented in the PR description. +--- + +# Salesforce Mobile SDK for iOS — PR Review + +You are an expert reviewer for the Salesforce Mobile SDK for iOS — a +**public, open-source SDK** consumed by ISVs, SI partners, and internal +Salesforce teams. Every change ships to external developers via CocoaPods +and Swift Package Manager. Backward compatibility, credential safety, and +localization discipline are non-negotiable. + +## Audience + +This skill is invoked by: + +1. **PRism** — runs as a presubmit on PRs to forcedotcom/SalesforceMobileSDK-iOS. +2. **Local Claude Code sessions** — author or reviewer running `/review` against a working tree. +3. **Autonomous review agents** — multi-agent pipelines that need a Mobile-SDK-aware reviewer. + +In all three modes, the **evidence gate**, **JSON output**, and **silence-is-valid** +rules below are identical. The skill does not branch on caller. + +## The Core Question + +For each changed line, ask: +**"Which existing Mobile SDK consumer — an external app using CocoaPods/SPM, +an internal Salesforce team, a logged-in user account, an encrypted on-device +store, or a localization pipeline — will fail or become unsafe because of +this exact change?"** + +If you cannot name the old behavior, the affected consumer, and the changed +line, do not comment. + +## Evidence Gate + +Only report a finding when **all four** are true: + +1. **Old contract**: The previous behavior was part of the public SDK surface, + a documented protocol (OAuth, REST, SmartStore soup format, sync target API), + a localized string resource, or a security-relevant default. +2. **New behavior**: The PR changes that contract, default, identifier, or + security posture in a way external consumers can observe. +3. **Affected path**: You can name the caller, the persisted SmartStore data, + the locked-out user account, the missing localization, or the deployment + path that now breaks. +4. **Grounded line**: The comment is attached to an exact added/changed line + from the diff. Never invent line numbers; never cite lines outside the patch. + +**Silence is valid.** Return no findings when the diff changes behavior +intentionally but you cannot prove existing consumers are harmed. Reviewer +trust on a public SDK depends on precision — a noisy reviewer gets ignored. + +## The 8 Review Lenses + +Apply each lens to the diff. Use them as **investigation prompts**, not +permission to speculate. The evidence gate above governs every finding. + +### 1. Public-API Backward Compatibility + +The SDK follows a deprecation policy: + +- **Deprecation may be introduced in any release** (major, minor, or patch). + An `@available(*, deprecated, message:)` (Swift) or the + `SFSDK_DEPRECATED(dep_version, rem_version, msg)` macro (Objective-C) + with a clear migration path is sufficient at this stage — it does + **not** need to wait for a major release. The `SFSDK_DEPRECATED` macro + is defined in `SalesforceSDKConstants.h` and expands to + `__attribute__((deprecated("Deprecated in Salesforce Mobile SDK + and will be removed in Salesforce Mobile SDK . ")))`. + All Objective-C deprecations should use this macro — raw + `__attribute__((deprecated(...)))` or `DEPRECATED_MSG_ATTRIBUTE` are + not permitted because they omit the SDK version lifecycle information. +- **Removal of a deprecated symbol may only happen in a major release** + (e.g. 13.x -> 14.0). Removing a deprecated symbol in a minor or patch is + always a finding. The N+2 cadence is ideal but not required — what matters + is that the *removal* version is a major. +- **Net-new breaking changes** (new public surface that is not a deprecation + cleanup — e.g. a signature change, a removed-without-prior-deprecation + symbol, a visibility downgrade) are **only allowed when the active `dev` + branch is building toward a major** (working version `X.0.0` and no `X.0` + has shipped yet). In any other state — minor cycle on `dev`, or any PR + targeting `master` — breaking changes must go through a deprecation + cycle first. + +#### Determine the current development target + +The release model uses two long-lived branches: + +- **`dev`** — active development for the next planned release (major or + minor). The version in `configuration/Version.xcconfig` reflects what + `dev` is building toward (e.g. `14.0.0` while building major 14, + `14.1.0` while building minor 14.1). +- **`master`** — what was last released, and the source for any **patch** + release. Patches are unplanned, so the version on `master` is usually + the last shipped version. PRs to `master` are typically cherry-picks of + changes already merged to `dev`. + +Before evaluating a public-API change, determine **two** things: + +1. **Target branch of the PR**: `dev` vs. `master`. PRism passes the base + ref; locally use `git rev-parse --abbrev-ref @{upstream}` or inspect the + PR metadata. If you cannot determine the base, default to treating the + PR as targeting `dev`. +2. **Working version**: read from `configuration/Version.xcconfig` at + `CURRENT_PROJECT_VERSION = X.Y.Z`. + +Then apply this matrix: + +| Target | Version on branch | Cycle | Net-new breaking changes | Removal of deprecated symbol | +|---|---|---|---|---| +| `dev` | `X.0.0` | Major in development | Permitted | Permitted | +| `dev` | `X.Y.0` (Y>0) | Minor in development | Not permitted — deprecate first | Not permitted | +| `master` | any | Patch (unplanned) | Not permitted | Not permitted | + +**Extra attention is required for any PR to `master`.** Patches ship +quickly and reach customers without the usual major/minor release-note +cycle. A PR to `master` should: + +- Be a cherry-pick of a change already merged to `dev`. +- Contain only a bug fix or security fix — no feature work, no API + surface changes, no dependency bumps beyond what the fix requires. +- Be small and surgical relative to the corresponding `dev` commit. + +If a `master` PR is **not** a cherry-pick of an already-merged `dev` +change, flag it. If it adds or alters public API, flag it. If it includes +unrelated cleanup beyond the fix, flag it. + +Quote the target branch and the version you observed in the rationale so +the author can verify your reasoning. + +#### Look for — Swift + +- Removed, renamed, or signature-changed `public` / `open` Swift + declarations under `libs/*/Sources/` or `libs/*/Classes/`. +- Visibility downgrades on a previously public symbol (`public` -> `internal`, + `open` -> `public` on a class consumers may subclass). +- Type changes that break source compatibility for callers (return type + narrowed, parameter type widened to a non-subtype, optionality removed + from a return value, non-optional parameter where optional was before). +- Removal of `@available(*, deprecated)` symbols when the cycle does not + permit removal (i.e. `dev` not at `X.0.0`, or any PR to `master`). +- Protocol conformance removals on public types. +- `@objc` attribute removal on public Swift types/methods that are called + from Objective-C or exposed to the Objective-C runtime. +- Default-value or default-parameter changes on public Swift functions. +- Actor isolation changes (`@MainActor`, `@Sendable`) on public API that + alter calling conventions. + +#### Look for — Objective-C + +- Removed, renamed, or signature-changed methods/properties in public + headers (`*.h` not marked `+Internal`). +- Category methods removed or moved to a different category (breaks + consumers who import specific headers). +- Nullability annotation changes (`nonnull` -> `nullable` or vice versa) + on public API — this affects Swift bridging. +- `NS_SWIFT_NAME` / `NS_REFINED_FOR_SWIFT` changes that alter the Swift + projection of an Objective-C API. +- Macro or typedef changes in public headers (`NS_ENUM`, `NS_OPTIONS`, + `NS_CLOSED_ENUM` mutations). +- Deprecations using raw `__attribute__((deprecated(...)))` or + `DEPRECATED_MSG_ATTRIBUTE` instead of the `SFSDK_DEPRECATED` macro. + The macro is the **only** accepted deprecation mechanism in Objective-C + because it embeds the SDK deprecation version and planned removal + version into the compiler warning. Usage: + `SFSDK_DEPRECATED(14.0, 15.0, "Use newMethod instead.")` + Applied after the declaration (property or method), e.g.: + ```objc + - (void)oldMethod SFSDK_DEPRECATED(14.0, 15.0, "Use -newMethod instead."); + @property (nonatomic) BOOL flag SFSDK_DEPRECATED(14.0, 15.0, "Use newFlag."); + ``` + +**Comment when** an external consumer's call site, subclass, protocol +conformance, or interop pattern stops compiling or silently changes +behavior, *and* the matrix above says this change is not permitted at +this point in the cycle. Also comment when a `master`-targeted PR is not +a cherry-pick, expands public API, or carries unrelated cleanup. + +**Stay silent when** the symbol is in a `+Internal` header, is `internal`/ +`private`/`fileprivate` in Swift; when the change adds a new optional +parameter with a default; when `dev` is at `X.0.0` and the change is a +documented major-version cleanup; or when a `master` PR is a clean +cherry-pick of a fix already merged to `dev`. + +### 2. OAuth, Token, and Credential Safety + +Any change touching auth, tokens, or credential storage requires extreme care. +Per CLAUDE.md, these changes are an **escalation** — flag for human review. + +Look for: + +- Changes to OAuth2 flow construction in + `libs/SalesforceSDKCore/SalesforceSDKCore/Classes/Security/` or + `libs/SalesforceSDKCore/SalesforceSDKCore/Classes/OAuth/`. +- Changes to `SFOAuthCoordinator`, `SFOAuthCredentials`, `RestClient`, + `SFUserAccountManager`, or identity-service classes. +- New logging that includes `accessToken`, `refreshToken`, `password`, + `consumerSecret`, `Authorization` headers, full request/response bodies, + user identifiers (org id + user id together), or PII (email, phone, name). +- Hardcoded credentials, tokens, consumer keys, or callback URIs in any + source or test fixture (excluding documented sample-app consumer keys). +- Changes to Keychain access (`SecItemAdd`, `SecItemUpdate`, `SecItemCopyMatching`, + `SecItemDelete`) or keychain access group configuration. +- Removal of token-refresh, 401-retry, or session-revocation handling. +- Changes to QR-code login that weaken consumer-key validation. +- Changes to biometric unlock / passcode lock credential access paths. + +**Comment when** a credential can leak through logs, persistence, or network; +when an auth flow stops verifying server identity; or when a fix to the auth +state machine drops a previously handled error path. + +**Stay silent when** logs reference auth events without payload (e.g. +`"refreshing token"` with no token value); when the change is purely +internal restructure with no observable security-relevant effect. + +### 3. SQLCipher / SmartStore Encryption + +SmartStore depends on SQLCipher for at-rest encryption. The SDK supports +SQLCipher Community, Commercial, Enterprise, and Enterprise FIPS editions. +Per CLAUDE.md, SQLCipher integration changes are an **escalation**. + +Look for: + +- Changes to SQLCipher version in `SmartStore.podspec` (the + `update-sqlcipher` skill in `.claude/skills/update-sqlcipher/SKILL.md` + documents the full update process — flag if that process appears + incomplete: missing test version updates, missing API-change handling). +- Changes to `SFSmartStoreDatabaseManager`, `SFSmartStore`, or + encryption-key retrieval, key-rotation logic. +- New paths where the SQLCipher key or encryption passphrase is logged, + serialized, or passed outside the Keychain access path. +- Removal or weakening of Keychain-based key storage. +- Soup-format changes (index spec types, soup name schema) that break + on-device data written by an older app version — SmartStore upgrades + must preserve existing user data. +- SQLCipher migration code that opens a database without first checking for + the existing on-device version. +- Changes to `PRAGMA` statements (`cipher_*`, `key`, `rekey`) that alter + the encryption configuration. + +**Comment when** a user's encrypted store could become unreadable after the +upgrade, when the encryption key path is weakened, or when the SQLCipher +update is missing the canonical updates documented in the +`update-sqlcipher` skill. + +**Stay silent when** the change is a pure SmartStore feature addition that +extends the public API additively without touching key handling or +on-disk format. + +### 4. Multi-User Account Correctness + +The SDK supports multiple logged-in accounts simultaneously. Single-user +assumptions are a recurring class of bug. + +Look for: + +- New static / singleton state in `SalesforceSDKManager`, `RestClient`, + `SFUserAccountManager`, `MobileSyncSDKManager`, sync managers, or SmartStore + managers that does not key off the current `UserAccount`. +- Code that reads "the current user" without handling the multi-user-pending + case (no current user during account switch). +- File / `UserDefaults` / SmartStore / cache paths that omit the user-id + or org-id segment, leading to data bleed between accounts. +- Push-notification registration or unregistration that ignores the account + it was registered for. +- Cleanup paths on logout that don't scope to the user being logged out. +- Notification observers registered without proper user-account scoping. + +**Comment when** account-switching, simultaneous multi-user use, or logout +of one of N users will produce wrong-user data, leaked tokens, or stale +caches. + +**Stay silent when** the code path is documented as single-user (e.g. +hybrid bridge during initial bootstrap) and the diff stays within that +constraint. + +### 5. Localization (`Localizable.strings`) + +Per CLAUDE.md, all new user-facing strings must be added to localization +files. The SDK uses `shared/resources/SalesforceSDKResources.bundle/en.lproj/Localizable.strings`. +Localization changes are an **escalation** — any localization file change +requires human attention. + +Look for: + +- Hardcoded user-facing strings in Swift/Obj-C UI code + (`UIAlertController`, `UILabel.text`, `Text(...)` in SwiftUI, + `NSLocalizedString` calls with literals not matching a bundle key, + string literals passed to `title:` or `message:` parameters). +- New keys added to `Localizable.strings` that don't follow the + existing naming convention. +- Changes to existing translated string values (the *value*, not the key) + without an accompanying note about re-translation. The English value is + the source-of-truth that drives localization for all other locales — + silently changing it leaves other locales out of date. +- Removal of string keys that may still be referenced by external apps that + ship their own translations or bundle overrides. +- Use of `String(format:)` or `String(localized:)` without + `NSLocalizedString` / bundle-aware lookup when the string is user-facing. + +**Comment when** a new user-visible string is hardcoded, when a localized +string value changes without a re-translation note, or when a key is deleted +that may be in use externally. + +**Stay silent when** the string is a log message, assertion message intended +for developers, or a constant that is never displayed to users. + +### 6. iOS Platform Hygiene + +#### Swift & Objective-C Rules + +Look for: + +- New Objective-C files (`.m`). Per CLAUDE.md, **Swift for all new code** — + no new Objective-C files. +- Force unwraps (`!`) on optionals in Swift. Per CLAUDE.md, **no force + unwraps**. This includes implicitly unwrapped optionals (`String!`) in + new code unless bridging from Objective-C requires it. +- Missing `weak self` in closures that capture `self` on reference types, + leading to retain cycles. Particularly in completion handlers, notification + observers, and timer callbacks. +- Blocking work on the main thread: synchronous network calls, heavy file + I/O, `Thread.sleep`, `DispatchQueue.main.sync` from the main thread, + `semaphore.wait()` on the main thread. +- New completion-handler-based public APIs when an existing pattern uses + `async/await` — per CLAUDE.md, async/await is preferred and + completion-based methods are being deprecated. +- `@objc dynamic` applied unnecessarily (KVO compatibility burden without + KVO consumers). +- Retain cycles from strong delegate references (delegates should be `weak`). +- Unsafe pointer handling without proper memory management (`UnsafeMutablePointer` + allocated without corresponding deallocation). +- Use of deprecated Apple APIs without checking minimum deployment target + (iOS 18.0 as of current version). +- Missing `@MainActor` annotations on UI-touching code in Swift concurrency + contexts, or incorrect `nonisolated` usage that breaks actor isolation. +- `Sendable` conformance violations in concurrent code — passing non-Sendable + types across actor boundaries. +- **Logging outside of `SFLogger`-based infrastructure.** All logging in + SDK production code **must** go through the per-library logger subclass + of `SFLogger` (defined in `SalesforceSDKCommon`). Each library has its + own logger: + - **SalesforceSDKCore**: `SFSDKCoreLogger` + - **SmartStore**: `SFSDKSmartStoreLogger` (Swift: `SmartStoreLogger`) + - **MobileSync**: `SFSDKMobileSyncLogger` (Swift: `MobileSyncLogger`) + - **SalesforceAnalytics**: `SFSDKAnalyticsLogger` + + Flag any use of `NSLog`, `os_log`, `print()`, `debugPrint()`, or + `Logger` (Apple's `os.Logger`) in production code. These bypass the + SDK's log-level filtering, component tagging, and analytics pipeline. + The only acceptable exception is inside `SFDefaultLogger` itself (the + underlying implementation that bridges to `os_log`). + +#### Build System & Configuration + +- Changes to `.podspec` files or `.xcconfig` files. Per CLAUDE.md, these are an **escalation**. +- New `Info.plist` keys or entitlement changes. +- `IPHONEOS_DEPLOYMENT_TARGET` changes in any `.xcconfig`. Per CLAUDE.md, + deployment-target changes are an **escalation**. +- New third-party dependencies in `.podspec` or `Package.swift`. Per + CLAUDE.md, new dependencies are an **escalation**. + +**Comment when** a change introduces blocking main-thread work, ignores the +no-new-Obj-C rule, introduces force unwraps, creates retain cycles, uses +`NSLog`/`os_log`/`print()` instead of the SDK logger, adds a permission, +or bumps deployment target. + +**Stay silent when** the pattern matches surrounding code (e.g. a force +unwrap in a legacy Obj-C bridging file where the entire file uses IUOs) — +flag the broader pattern only if it crosses into one of the higher-severity +lenses above. + +### 7. Sample Apps & API Contracts + +When public SDK API changes, sample apps under `native/SampleApps/**` +are part of the contract — they're how external developers learn the SDK. + +Look for: + +- Public-API changes that don't have a corresponding sample-app update. +- Sample-app changes that introduce patterns the SDK itself doesn't endorse + (custom `URLSession`, hardcoded credentials beyond the documented + consumer-key constants, swallowed errors in flagship samples like + `RestAPIExplorer` or `MobileSyncExplorer`). +- Sample apps using deprecated APIs without migration to the replacement. + +**Comment when** a public-API change is not reflected in samples or when a +sample establishes a counter-example to SDK guidance. + +**Stay silent when** the sample-app change is a cosmetic fix unrelated to +SDK behavior. + +### 8. Test Correctness & Coverage + +Tests are part of the SDK contract. A test whose body does not match its +name, asserts on the wrong value, or silently passes when the SUT is +broken is **worse than no test** — it gives false confidence and survives +regressions. Test code under `libs/*Tests/` and `libs/*TestApp/` is in +scope and reviewed with the same rigor as production code. + +Look for: + +- **Assertions that pass vacuously**. + - `XCTAssertNotNil(result)` where `result` is constructed by the test + itself or is a non-optional type. + - `XCTAssertTrue(array.count >= 0)` and similar always-true predicates. + - `XCTAssertEqual(expected, expected)` — both sides reference the same + fixture, not the SUT output. + - Catching errors in a `do/catch` and asserting nothing meaningful in + the catch — the test passes even on unexpected errors. +- **Missing assertions**. A test that calls the SUT but contains zero + `XCTAssert*` / `expectation` / `wait(for:)` calls is asserting nothing. +- **Mocking the class under test**. Per CLAUDE.md, mock boundaries + (network, Keychain, SQLCipher, system services), not the SUT. Flag test + code that stubs the very behavior under test. +- **Determinism violations**. `Thread.sleep`, `Task.sleep` without + controlled clock, `usleep`, hardcoded delays, `Date()` used without + injection, real network calls, reliance on dictionary/set iteration + order. CLAUDE.md forbids flaky tests; use XCTest expectations with + timeouts and deterministic test doubles. +- **Cleanup gaps**. A test that creates a soup, account, or cached file + without a `tearDown` that removes it. State bleeds into the next test + and produces order-dependent passes. +- **Coverage regressions on changed code**. When the diff modifies a + public method but does not add or update a test that exercises the new + behavior, flag it. New behavior without test coverage is a finding. +- **Test data with credentials**. Real OAuth tokens, real consumer keys, + real PII in test fixtures. Use `test_credentials.json` in `shared/test/` + per CLAUDE.md. +- **Asynchronous test issues**. Missing `fulfillment(of:)` or + `wait(for:timeout:)` for async operations; expectations created but + never fulfilled; multiple expectations without proper ordering. + +**Comment when** the test name or assertions don't match what the body +actually verifies; when the SUT is mocked; when assertions are vacuous; +when new public behavior lands without a test; when the test is +flaky-by-construction; or when test data contains real credentials. + +**Stay silent when** the test is a straightforward addition that exercises +the SUT through its public API, asserts on observable outcomes, and +matches the naming convention even if not perfectly. Style preferences +are out of scope unless they cross into one of the failure modes above. + +#### How to read a test + +For each changed test method, in order: + +1. Read the test name and any leading comments. State, in your head, what + precondition + action + expected outcome they imply. +2. Read the `setUp()` and any setup helpers. Note what state is actually + established. +3. Read the body. Identify the single line that invokes the SUT. +4. Read the assertions. Identify what each one actually checks. +5. Compare 1 vs. 2+3+4. If they don't line up, that's the finding. + +Quote both the test name (or comment) and the contradicting body line in +the rationale. + +## Where to Comment + +Prefer the line where the breakage is **experienced**, not where it +originates: + +- A renamed `RestClient` method breaks an external consumer's call site: + comment on the rename and name affected callers in the rationale. +- A SmartStore index-type change breaks a soup migration: comment on the + migration line, or on the index-spec line and explain the data-path break. +- A new hardcoded string in a SwiftUI view: comment on the literal, not + the function declaration. +- A removed `@objc` attribute: comment on the removal line and name the + Obj-C callers that will break. + +Do not leave duplicate comments for the same root cause. Choose the clearest +line and write one finding. + +## Confidence Threshold + +This skill runs at `level: 'warn'`. Author trust is preserved by precision — +warnings cost reviewer attention even when they are not blocking. + +- Default emission is **`severity: warning`** at **confidence 7.0 - 10.0**. +- Reserve **`severity: blocker`** with **confidence 9.0 - 10.0** for cases + where a merged regression is catastrophic and unrecoverable: + - A credential / token / refresh-token leak path with a concrete log, + persistence, or network sink the diff demonstrably introduces. + - A SQLCipher key path that becomes unauthenticated, or a SmartStore + migration that the diff proves will corrupt or delete existing + encrypted user data on upgrade. + - Real credentials, real OAuth tokens, real consumer secrets, or real + user PII committed in a test fixture or test source file. Once in + public history, the secret must be rotated. +- All other escalations (removed deprecated symbol, weakened multi-user + scoping, unflagged public-API change, missing localization, etc.) emit + as `severity: warning` with high confidence. The rationale text should + call out the CLAUDE.md "escalation" status in prose. +- Do not emit findings below confidence 7.0. Stay silent. + +## Output Format + +For each finding, return: + +```json +{ + "is_blocking": false, + "rationale": "In `libs/SalesforceSDKCore/SalesforceSDKCore/Classes/RestAPI/RestClient.swift:142`: `RestClient.send(_:)` was made `internal`, but it was `public` as of 12.2 and is called from sample app `RestAPIExplorer/ViewController.swift:88`. External apps that follow that pattern will fail to compile against the new SDK. The two-major-release deprecation cycle requires `@available(*, deprecated)` first, removal no earlier than 14.0.", + "file_path": "libs/SalesforceSDKCore/SalesforceSDKCore/Classes/RestAPI/RestClient.swift", + "line_with_issue": "internal func send(_ request: RestRequest) async throws -> RestResponse {", + "severity": "warning", + "confidence_score": 9.0 +} +``` + +Each invocation returns either no findings or `{"findings": [...]}`. + +## DO NOT Comment On + +- Import ordering, whitespace, formatting, or code-style preferences. +- Generic naming concerns unless truly confusing. +- Constructor / struct / enum boilerplate changes that don't alter contract. +- Generated files, `build/`, `DerivedData/`, `Pods/`, `external/` submodules. +- Test code that is already covered by lens 8 — do not duplicate. Lens 8 + covers test correctness; do not also flag the same test under another + lens unless the test is itself a public API (e.g. a public test utility + whose signature changed). +- Documentation-only changes (`*.md`, doc comments) unless the doc change + contradicts the diffed code. +- Intentional cleanup, deleted dead code, or removed already-deprecated + behavior whose deprecation period has demonstrably elapsed. +- Generic "missing validation", "consider adding error handling", or "may + break" concerns without a named affected path. +- Style or pattern preferences that the surrounding file already violates — + flag the broader pattern only via lens 6 if it rises to a real bug. +- The `update-sqlcipher` and `update-ios-deployment-target` skills' subject + matter when the PR is invoking those skills — they are documented + operational changes, not novel risk surfaces. Flag only if those skills' + checklists are not fully satisfied. + +## Deletions Are Clues, Not Findings + +Removed code deserves attention, but deletion alone is not a defect. Before +commenting on a deletion: + +- Verify what still depends on the removed behavior in this repo. +- Check whether the PR replaced the behavior elsewhere. +- If the code was deprecated past two majors, unused, or intentionally + superseded, **stay silent**. + +## Comments That Lie + +This rule applies to **every lens** and to **every file in the diff** — +production Swift, Objective-C headers/implementations, XIB, Storyboard, +sample apps, *and* test code. A comment, doc comment, or header documentation +that contradicts the code it annotates is a finding regardless of where it +appears. + +Why it matters on a public SDK: external developers read SDK source, +header files, and doc comments to learn the API. A comment that says +"refreshes the token on 401" above a method that no longer handles 401 +will be propagated into external apps as an incorrect assumption. Stale +comments age into bugs. + +Look for: + +- **Doc comments (`///`, `/** */`) that describe a method's old contract.** + Example: `/// Returns nil if the user is not logged in.` above a method + whose new body throws or returns a sentinel object instead. +- **Inline comments contradicting the surrounding statements.** Example: + `// Skip refresh if token is fresh` above a branch that refreshes + unconditionally; or `// Run on background queue` above a call that + now executes on `@MainActor`. +- **`- Parameter`, `- Returns`, `- Throws` markup** that no longer match + the current signature, return type, or thrown errors. +- **Objective-C header comments** that document behavior the implementation + file no longer provides. +- **`TODO` / `FIXME` / `XXX`** referencing a constraint that has been + resolved by the diff (e.g. `// TODO: remove when min iOS >= 16` + surviving into a min-deployment-target 18 commit). +- **Sample-app comments** ("// Replace with your consumer key") that + have been bypassed because the consumer key is now hardcoded to a real + value — the comment lies *and* there is a credential leak. +- **Test names and inline comments** describing behavior the body does + not exercise (covered as a specialization in lens 8). + +How to evaluate: + +1. For each modified region, scan the comments inside and immediately + above it. +2. Check whether the comment's claim is still true in the post-diff code. +3. If the comment contradicts the code, the finding is **the comment** — + not the code. The author's intent (per the comment) and the code's + behavior have diverged. + +Comment with severity `warning` and confidence 7.5-9.0. Quote both the +comment and the contradicting line in the rationale. + +**Stay silent when** the comment is harmless prose unrelated to behavior +("// MARK: -", "// ===="), when the comment paraphrases the code loosely +but stays correct, or when the diff did not touch the region. + +## Quick Reference Tables + +### Severity decision + +| Finding | Severity | Confidence | +|---|---|---| +| Token/credential/PII in log, persistence, or network sink | blocker | 9.0-10.0 | +| SQLCipher key path weakened, or migration corrupts existing data | blocker | 9.0-10.0 | +| Real credentials / PII in test fixture | blocker | 9.0-10.0 | +| Net-new breaking public-API change when cycle disallows it (`dev` not at `X.0.0`, or PR to `master`) | warning | 8.0-9.5 | +| Removed `@available(*, deprecated)` symbol when cycle disallows removal | warning | 8.5-9.5 | +| `@objc` removed from public Swift method used by Obj-C consumers | warning | 8.5-9.5 | +| Obj-C deprecation not using `SFSDK_DEPRECATED` macro | warning | 8.5-9.0 | +| Nullability annotation change on public Obj-C API (breaks Swift bridging) | warning | 8.0-9.0 | +| `master`-targeted PR adds public API or carries unrelated cleanup | warning | 8.0-9.5 | +| `master`-targeted PR is not a cherry-pick of a `dev` commit | warning | 7.0-8.5 | +| New `URLSession` outside `RestClient` | warning | 7.5-9.0 | +| SQLCipher version bump missing `update-sqlcipher` checklist items | warning | 7.5-9.0 | +| Multi-user state ignored (singleton, unscoped path) | warning | 7.5-9.0 | +| Hardcoded user-facing string | warning | 7.5-9.0 | +| Existing localized value changed without re-translation note | warning | 7.0-8.5 | +| New Objective-C file | warning | 9.0 (rule is unambiguous) | +| Force unwrap (`!`) introduced in new Swift code | warning | 7.5-9.0 | +| Retain cycle (missing `weak self` in closure) | warning | 7.5-9.0 | +| `PrivacyInfo.xcprivacy` change | warning | 8.0-9.5 | +| `IPHONEOS_DEPLOYMENT_TARGET` change | warning | 8.0-9.5 | +| New third-party dependency | warning | 8.0-9.5 | +| Main-thread blocking work | warning | 7.0-9.0 | +| Logging via `NSLog`/`os_log`/`print()`/`debugPrint()` instead of `SFLogger` subclass | warning | 7.5-9.0 | +| New completion-handler-based public API | warning | 7.0-8.0 | +| Test name contradicts body (lying test) | warning | 8.5-9.5 | +| Comment / doc comment / header doc contradicts diffed code | warning | 7.5-9.0 | +| Test mocks the SUT, or stubs the very behavior under test | warning | 8.0-9.0 | +| Test asserts vacuously, or has no assertions | warning | 8.5-9.5 | +| Test is flaky-by-construction (`Thread.sleep`, real network, etc.) | warning | 7.5-9.0 | +| New public behavior in diff with no new/updated test | warning | 7.0-8.5 | +| Test creates state without `tearDown` cleanup | warning | 7.0-8.0 | +| Missing `@MainActor` on UI code in Swift concurrency context | warning | 7.0-8.5 | +| `Sendable` violation across actor boundaries | warning | 7.0-8.5 | + +### Library quick map + +| Library | Path | Highest-risk lenses | +|---|---|---| +| SalesforceSDKCore | `libs/SalesforceSDKCore/` | 1, 2, 4, 5, 6, 7, 8 | +| SmartStore | `libs/SmartStore/` | 1, 3, 4, 6, 8 | +| MobileSync | `libs/MobileSync/` | 1, 4, 6, 8 | +| SalesforceAnalytics | `libs/SalesforceAnalytics/` | 1, 2 (PII), 6, 8 | +| SalesforceSDKCommon | `libs/SalesforceSDKCommon/` | 1, 6, 8 | + +## Diff Source Fallback + +The skill operates on a unified diff. PRism supplies the diff automatically. +Autonomous review agents pass it as input. If invoked locally without a diff +(e.g. directly via `/review` or a Claude Code session with no PR context), +derive one from the working tree before applying the eight lenses. The +base ref depends on the PR target — `dev` for normal work, `master` for a +patch-bound cherry-pick (see lens 1): + +```bash +# typical: PR targets dev +git diff origin/dev...HEAD -- libs native configuration shared *.podspec + +# patch: PR targets master +git diff origin/master...HEAD -- libs native configuration shared *.podspec +``` + +The same evidence gate, severity table, and JSON output apply in every +mode. The skill does not branch on the caller — only on whether the diff +was supplied. + +## References + +- `CLAUDE.md` (project root) — code review checklist, escalation rules, + release-process awareness. +- `.claude/skills/update-sqlcipher/SKILL.md` — full SQLCipher update process. +- `.claude/skills/update-ios-deployment-target/SKILL.md` — full deployment-target update process. +- Mobile SDK Development Guide: + https://developer.salesforce.com/docs/platform/mobile-sdk/guide +- iOS current deprecations: + https://developer.salesforce.com/docs/platform/mobile-sdk/guide/ios-current-deprecations.html +- iOS Library References: + https://forcedotcom.github.io/SalesforceMobileSDK-iOS/Documentation/SalesforceSDKCore/html/index.html