Skip to content

Align KMP verification result contract with canonical SDK types#1831

Merged
transphorm merged 4 commits intodevfrom
justin/kmp-ns-06
Mar 10, 2026
Merged

Align KMP verification result contract with canonical SDK types#1831
transphorm merged 4 commits intodevfrom
justin/kmp-ns-06

Conversation

@transphorm
Copy link
Copy Markdown
Member

@transphorm transphorm commented Mar 10, 2026

This PR completes NS-06 by aligning the kmp-sdk public callback/result contract with the canonical SDK VerificationResult shape documented in OVERVIEW.md. It removes the non-canonical public type field from KMP
results, widens claims to Map<String, Any?>, and adds canonical error support. Android and iOS lifecycle handlers still accept legacy flat lifecycle { type } payloads from the embedded WebView bundle, but that
compatibility is now isolated internally instead of leaking through the public KMP API.

The change also adds focused serialization/compatibility tests and updates the native-shells backlog/spec docs to mark NS-06 complete. A small MiniPay sample adjustment stringifies heterogeneous claims before
persisting them so existing sample UI behavior remains stable.

Summary by CodeRabbit

  • New Features

    • Passport scanning now uses a consolidated core module, adds analytics event tracking (trackEvent, flush) and a requiresMainQueueSetup hook.
  • Improvements

    • Verification result payload standardized: richer nested claims, optional error (code/message), and explicit userId/verificationId/proof; legacy flat type field removed.
  • Tests

    • Added JSON round‑trip and lifecycle‑params tests for verification result handling.
  • Documentation

    • Updated SDK specs and native‑shell/contract docs to reflect canonical verification outputs.

@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 10, 2026

📝 Walkthrough

Walkthrough

Refactors VerificationResult shape and serialization (removes legacy type, widens claims, adds SelfSdkError), updates lifecycle handlers and platform SDK paths to use canonical (de)serialization helpers, delegates PassportReader scanning to a new PassportReaderCore, and updates tests and specs to match the canonical contract.

Changes

Cohort / File(s) Summary
VerificationResult API & Serialization
packages/kmp-sdk/shared/src/commonMain/kotlin/xyz/self/sdk/api/SelfSdkCallback.kt, packages/kmp-sdk/shared/src/commonMain/kotlin/xyz/self/sdk/api/VerificationResultJson.kt
Replaces legacy type with canonical VerificationResult (adds error: SelfSdkError?, changes claims to Map<String, Any?>?), adds a custom serializer and helpers for JSON (de)serialization and lifecycle-param factory.
Lifecycle handlers & SDK surface
packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/handlers/LifecycleBridgeHandler.kt, packages/kmp-sdk/shared/src/iosMain/kotlin/xyz/self/sdk/handlers/LifecycleBridgeHandler.kt, packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt, packages/kmp-sdk/shared/src/commonMain/kotlin/xyz/self/sdk/api/VerificationResultJson.kt
Switches lifecycle handling to use canonical helpers (serializeVerificationResult, deserializeVerificationResult, verificationResultFromLifecycleParams) and emit serialized EXTRA_RESULT_DATA containing full VerificationResult instead of the legacy flat type shim.
Sample app claim handling
packages/kmp-minipay-sample/composeApp/src/commonMain/kotlin/xyz/self/minipay/MainViewModel.kt
Adds stringifyClaims() helper converting arbitrary claim values to strings; replaces direct assignments to verifiedClaims with stringified map for UI consumption.
Passport reader consolidation (iOS & SDK)
app/ios/PassportReader.swift, app/ios/PassportReaderCore.swift, packages/mobile-sdk-alpha/ios/SelfSDK/PassportReader.swift, packages/mobile-sdk-alpha/ios/SelfSDK/PassportReaderCore.swift, .../project.pbxproj
Extracts MRZ/NFC passport scanning into new PassportReaderCore module, delegates scanning logic from higher-level PassportReader, wires analytics (trackEvent/flush), exposes requiresMainQueueSetup(), and updates Xcode project files to include new sources.
Tests
packages/kmp-sdk/shared/src/commonTest/kotlin/xyz/self/sdk/api/VerificationResultJsonTest.kt, packages/kmp-sdk/shared/src/commonTest/kotlin/xyz/self/sdk/models/ModelSerializationTest.kt
Adds/updates tests for canonical VerificationResult JSON parsing, lifecycle-param mapping, nested-claims round-trip, and error field serialization coverage.
Docs & Specs
specs/projects/sdk/OVERVIEW.md, specs/projects/sdk/workstreams/.../SPEC.md, specs/projects/sdk/workstreams/native-shells/...
Updates overview and workstream specs to reflect canonical VerificationResult, plan to remove legacy { type } shim, PassportReader parity, and related status/plan adjustments.

Sequence Diagram(s)

sequenceDiagram
    participant WebView as WebView / Native Shell
    participant Bridge as LifecycleBridgeHandler
    participant SDK as Self SDK (serialize/deserialize)
    participant App as Host App

    WebView->>Bridge: emit lifecycle params (may include legacy `type` or `data` JSON)
    Bridge->>SDK: verificationResultFromLifecycleParams(params) / deserializeVerificationResult(data)
    SDK-->>Bridge: VerificationResult (canonical)
    Bridge->>App: setResult(EXTRA_RESULT_DATA: serializeVerificationResult(VerificationResult))
    App->>App: handle canonical VerificationResult (claims, error, proof, ids)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

codex

Suggested reviewers

  • seshanthS

Poem

🔧 New result shape, claims set free,
Errors nest where they ought to be.
Passport core scans shared and neat,
Analytics hooked, tests complete.
Specs updated — tidy feat! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.63% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The PR description is comprehensive and covers what is being changed, why, and how the change maintains backward compatibility. However, it does not include testing details or QA instructions as specified in the template. Add explicit 'Tested' and 'How to QA' sections describing the test coverage and validation approach for this contract-aligning change.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main objective: aligning KMP verification result contract with canonical SDK types, which is the core focus of this PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch justin/kmp-ns-06

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 and usage tips.

@transphorm transphorm changed the title Justin/kmp ns 06 Align KMP verification result contract with canonical SDK types Mar 10, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (3)
packages/kmp-sdk/shared/src/commonMain/kotlin/xyz/self/sdk/api/SelfSdkCallback.kt (2)

46-47: Consider documenting the accepted claim value types.

Using Map<String, Any?> for claims is necessary for dynamic JSON but sacrifices compile-time type safety. The actual supported types are defined in toJsonElement() (String, Boolean, Int, Long, Float, Double, Number, Map, List). Consider adding KDoc to document the accepted types for SDK consumers.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@packages/kmp-sdk/shared/src/commonMain/kotlin/xyz/self/sdk/api/SelfSdkCallback.kt`
around lines 46 - 47, Document the accepted claim value types for the claims
property on SelfSdkCallback by adding KDoc to the claims declaration that lists
the allowed runtime types (String, Boolean, Int, Long, Float, Double, Number,
Map, List) and any nullability rules, and reference the toJsonElement()
conversion behavior; update the KDoc so callers know which types are supported
and that nested Maps/Lists must contain those same types.

115-134: Fallback toString() for unknown types could serialize unexpected data.

Line 133 converts any unrecognized type via toString(), which could inadvertently serialize object references (e.g., "ClassName@hashcode") or sensitive data. This is a reasonable fallback for flexibility, but ensure callers understand only the documented types will serialize meaningfully.

💡 Consider throwing or logging for unexpected types
-        else -> JsonPrimitive(toString())
+        else -> throw IllegalArgumentException("Unsupported claim value type: ${this::class.simpleName}")

Or if flexibility is required, at least log at debug level when hitting this fallback.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@packages/kmp-sdk/shared/src/commonMain/kotlin/xyz/self/sdk/api/SelfSdkCallback.kt`
around lines 115 - 134, The fallback branch in Any?.toJsonElement() currently
serializes unknown types with toString(), which can produce object references or
leak sensitive data; change this by either throwing a clear exception (e.g.,
IllegalArgumentException) or logging a debug message before returning JsonNull
(or another safe sentinel) instead of JsonPrimitive(toString()); update the
Any?.toJsonElement() implementation to handle unexpected types by selecting one
safe behavior (throw or debug-log+safe-serialization) and ensure callers of
toJsonElement() are documented/updated accordingly.
packages/kmp-sdk/shared/src/commonMain/kotlin/xyz/self/sdk/api/VerificationResultJson.kt (1)

35-38: Silent exception swallowing in lifecycleProofString.

Using runCatching { ... }.getOrNull() silently suppresses any exception without logging. If unexpected JSON structures appear in production, this could make debugging difficult. Consider logging the fallback case at debug level if the SDK has a debug mode.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@packages/kmp-sdk/shared/src/commonMain/kotlin/xyz/self/sdk/api/VerificationResultJson.kt`
around lines 35 - 38, The helper lifecycleProofString currently swallows
exceptions via runCatching { element.jsonPrimitive.contentOrNull }.getOrNull(),
which hides parsing errors; update lifecycleProofString to explicitly catch
exceptions from accessing element.jsonPrimitive (or other JsonElement
operations), and when an exception occurs log a debug-level message (including
the element and exception) if the SDK has a debug mode or logger, then return
the same fallback element.toString(); reference lifecycleProofString,
JsonElement/JsonObject, and element.jsonPrimitive.contentOrNull when locating
the code to add the try/catch + debug log and preserve the existing fallback
behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@packages/kmp-minipay-sample/composeApp/src/commonMain/kotlin/xyz/self/minipay/MainViewModel.kt`:
- Around line 37-38: stringifyClaims currently collapses nested maps/lists with
Kotlin toString() which loses JSON structure; instead, JSON-encode each claim
value or keep the original structured map until rendering. Update
stringifyClaims to iterate claims and for each value produce
Json.encodeToJsonElement(value).toString() (using
kotlinx.serialization.json.Json) so nested objects/lists become valid JSON
strings, or change the function signature to return Map<String, Any?>? and
propagate that structured map into HomeState so rendering can serialize at UI
time; reference stringifyClaims and HomeState when making the change.

In
`@packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/handlers/LifecycleBridgeHandler.kt`:
- Around line 76-82: The Kotlin native handler LifecycleBridgeHandler should not
translate legacy `{ type }` lifecycle payloads; remove the branch that detects
`type != null` and calls
`verificationResultFromLifecycleParams(...)`/`serializeVerificationResult(...)`
and instead pass the incoming params through unchanged into the intent extra
(`SelfVerificationActivity.EXTRA_RESULT_DATA`); ensure `LifecycleBridgeHandler`
becomes a thin pass-through and leave the legacy-normalization logic to the
TypeScript layer so
`serializeVerificationResult`/`verificationResultFromLifecycleParams` are not
invoked here.

In `@specs/projects/sdk/OVERVIEW.md`:
- Line 223: The spec still contains legacy result wrappers (top-level "type"
fields and legacy fields like verified, disclosedClaims, timestamp) that
contradict the "Contract lock (normative)" rule; remove all examples and
documentation that emit `{ "type": ... }` wrappers and legacy fields and replace
them with the canonical VerificationResult shape (reference VerificationResult)
in every example and system-level success/error example, change Person 2's
example `claims` to Map<String, Any?> to match TypeScript Record<string,
unknown>, and add a short note that native shells receiving legacy flat
lifecycle payloads (e.g., `{ type: "proofRequested" }`) must translate those
internally to the canonical success semantics rather than exposing `type` on
public results.

In `@specs/projects/sdk/workstreams/native-shells/SPEC.md`:
- Around line 112-113: The SPEC.md still shows the old result model; update all
example payloads and handling logic to use the canonical VerificationResult
shape (fields: success, userId, verificationId, proof, claims, error) and
convert any code that branches on a deprecated VerificationResult.type lifecycle
to read from the new claims Map<String, Any?> or the success flag instead; keep
references to the internal flat `{ type }` lifecycle only as an
implementation-compat shim (do not expose it in public examples), and update any
sample variable/function names that parse results (e.g., code that mentions
VerificationResult.type, claims usage, or lifecycle handling) to use
verificationId, proof, claims, error, userId, and success consistently so the
SPEC reflects NS-06 behavior.

---

Nitpick comments:
In
`@packages/kmp-sdk/shared/src/commonMain/kotlin/xyz/self/sdk/api/SelfSdkCallback.kt`:
- Around line 46-47: Document the accepted claim value types for the claims
property on SelfSdkCallback by adding KDoc to the claims declaration that lists
the allowed runtime types (String, Boolean, Int, Long, Float, Double, Number,
Map, List) and any nullability rules, and reference the toJsonElement()
conversion behavior; update the KDoc so callers know which types are supported
and that nested Maps/Lists must contain those same types.
- Around line 115-134: The fallback branch in Any?.toJsonElement() currently
serializes unknown types with toString(), which can produce object references or
leak sensitive data; change this by either throwing a clear exception (e.g.,
IllegalArgumentException) or logging a debug message before returning JsonNull
(or another safe sentinel) instead of JsonPrimitive(toString()); update the
Any?.toJsonElement() implementation to handle unexpected types by selecting one
safe behavior (throw or debug-log+safe-serialization) and ensure callers of
toJsonElement() are documented/updated accordingly.

In
`@packages/kmp-sdk/shared/src/commonMain/kotlin/xyz/self/sdk/api/VerificationResultJson.kt`:
- Around line 35-38: The helper lifecycleProofString currently swallows
exceptions via runCatching { element.jsonPrimitive.contentOrNull }.getOrNull(),
which hides parsing errors; update lifecycleProofString to explicitly catch
exceptions from accessing element.jsonPrimitive (or other JsonElement
operations), and when an exception occurs log a debug-level message (including
the element and exception) if the SDK has a debug mode or logger, then return
the same fallback element.toString(); reference lifecycleProofString,
JsonElement/JsonObject, and element.jsonPrimitive.contentOrNull when locating
the code to add the try/catch + debug log and preserve the existing fallback
behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 945239fb-ff73-4791-a433-b17f90b731d5

📥 Commits

Reviewing files that changed from the base of the PR and between 6b89976 and 7bd6287.

📒 Files selected for processing (12)
  • packages/kmp-minipay-sample/composeApp/src/commonMain/kotlin/xyz/self/minipay/MainViewModel.kt
  • packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt
  • packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/handlers/LifecycleBridgeHandler.kt
  • packages/kmp-sdk/shared/src/commonMain/kotlin/xyz/self/sdk/api/SelfSdkCallback.kt
  • packages/kmp-sdk/shared/src/commonMain/kotlin/xyz/self/sdk/api/VerificationResultJson.kt
  • packages/kmp-sdk/shared/src/commonTest/kotlin/xyz/self/sdk/api/VerificationResultJsonTest.kt
  • packages/kmp-sdk/shared/src/commonTest/kotlin/xyz/self/sdk/models/ModelSerializationTest.kt
  • packages/kmp-sdk/shared/src/iosMain/kotlin/xyz/self/sdk/handlers/LifecycleBridgeHandler.kt
  • specs/projects/sdk/OVERVIEW.md
  • specs/projects/sdk/workstreams/native-consolidation/SPEC.md
  • specs/projects/sdk/workstreams/native-shells/SPEC.md
  • specs/projects/sdk/workstreams/native-shells/plans/NS-06-kmp-callback-contract-alignment.md

Comment thread specs/projects/sdk/OVERVIEW.md
Comment thread specs/projects/sdk/workstreams/native-shells/SPEC.md Outdated
@gitguardian
Copy link
Copy Markdown

gitguardian Bot commented Mar 10, 2026

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
19414827 Triggered Generic Password 808c81a app/ios/PassportReaderCore.swift View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/ios/PassportReader.swift (1)

64-71: ⚠️ Potential issue | 🟠 Major

Mirror trackEvent in the E2E stub.

Line 64 adds a new public native entrypoint only in the non-E2E_TESTING branch. The #else class now exports a different module surface, so E2E builds no longer exercise the same bridge contract as production.

🛠️ Suggested change
 `#else`
 `@available`(iOS 15, *)
 `@objc`(PassportReader)
 class PassportReader: NSObject {
     override init() {
         super.init()
     }

     `@objc`(configure:enableDebugLogs:)
     func configure(token: String, enableDebugLogs: Bool) {
     }
+
+    `@objc`(trackEvent:properties:)
+    func trackEvent(_ name: String, properties: [String: Any]?) {
+    }
 
     `@objc`(scanPassport:dateOfBirth:dateOfExpiry:canNumber:useCan:skipPACE:skipCA:extendedMode:usePacePolling:sessionId:resolve:reject:)
     func scanPassport(

As per coding guidelines, Verify native modules work correctly if native code was modified in PR.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/ios/PassportReader.swift` around lines 64 - 71, The new native method
trackEvent(_:properties:) in PassportReader.swift is only present in the
non-E2E_TESTING branch, causing the E2E stub to export a different bridge
surface; update the E2E stub to also declare and forward the same
`@objc`(trackEvent:properties:) method so both branches expose an identical API.
Locate the E2E `#else` class and add a matching trackEvent(_ name: String,
properties: [String: Any]?) method (mirroring the call into
analytics?.trackEvent(name, properties: mpProps) or analytics?.trackEvent(name,
properties: nil) logic using Properties casting) so E2E builds exercise the same
bridge contract as production.
♻️ Duplicate comments (1)
specs/projects/sdk/workstreams/native-shells/SPEC.md (1)

113-114: ⚠️ Potential issue | 🟠 Major

Don’t mark NS-06 complete while later examples are still pre-canonical.

These notes say the public VerificationResult now includes error, but the Public API section later in this file still defines VerificationResult without that field and other examples still mix old/new payload shapes. The spec is still presenting two incompatible contracts.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@specs/projects/sdk/workstreams/native-shells/SPEC.md` around lines 113 - 114,
The spec prematurely marks NS-06 complete while later examples and the Public
API still use the old/incompatible payloads; update the Public API
`VerificationResult` type to include the `error` field and ensure every example
uses the canonical shape (`success`, `userId`, `verificationId`, `proof`,
`claims` as Map<String, Any?>, `error`) or revert the NS-06 completion status
(leave NS-06 as pending) and keep NS-07 tracking removal of
`VerificationResult.type` until all examples and the Public API are aligned;
specifically search for `VerificationResult`, `NS-06`, `NS-07`, and the "Public
API" section and change the definitions and examples to the canonical form or
undo the completion note.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@app/ios/PassportReader.swift`:
- Around line 110-119: The NativeLoggerBridge.logInfo call inside the onStart
closure in PassportReader.swift currently includes the raw passportNumber in its
payload; remove passportNumber (or replace it with an irreversible
redaction/hash) from the dictionary passed to NativeLoggerBridge.logInfo so no
MRZ/PII is emitted to native logs, while keeping other flags like useCAN,
skipPACE, skipCA, extendedMode, and usePacePolling unchanged; ensure any helper
function used (e.g., logNfc) does not re-add the raw passport number elsewhere.

In `@app/ios/PassportReaderCore.swift`:
- Around line 169-175: The JSONSerialization is failing because DataGroupId (an
enum) is being passed directly from convertDataGroupHashToSerializableFormat()
(via dataGroupHash.id) and causes an exception that is swallowed by the empty
catch, dropping dataGroupHashes; fix by normalizing the enum to a string before
serializing — update convertDataGroupHashToSerializableFormat() to call
dataGroupIdToString(dataGroupHash.id) (or otherwise replace dataGroupHash.id
with its string form) so the object passed to JSONSerialization is
JSON-compatible, and remove the silent empty catch or at least log the error so
failures are visible.

In `@packages/mobile-sdk-alpha/ios/SelfSDK/PassportReaderCore.swift`:
- Around line 90-95: The scanPassport method currently rejects directly in
several error branches (notably the invalid-CAN branch and SOD failure branches)
without invoking the supplied onFailure callback; update every reject path
inside scanPassport (and the other similar blocks referenced around lines
112-116 and 187-215) to call onFailure with the Error (or a mapped Error) before
invoking reject so cleanup/telemetry always runs; specifically, locate the
scanPassport function and any internal handlers that call
reject(resolve:reject:), and insert a safe call to onFailure?(error) immediately
prior to calling reject(...), preserving the original error passed to reject.
- Around line 32-50: PassportReaderCore currently implements MRZ derivation
(getMRZKey, pad, calcCheckSum) and other business logic; remove that logic from
Swift and replace these methods with thin bridge stubs that delegate to the
TypeScript layer (e.g., forward inputs and return outputs from the WebView/JS
handler) so Swift no longer performs MRZ calculation or other NFC/serialization
decisions; specifically, strip implementations from getMRZKey, pad and
calcCheckSum and implement them as pass-throughs that call the existing
TypeScript API (or a JS message handler) and propagate results/errors back to
the caller, keeping only minimal parameter marshalling and error mapping in
PassportReaderCore.

In
`@specs/projects/sdk/workstreams/native-consolidation/plans/NC-03-phase-2-passport-reader-parity.md`:
- Line 4: The plan was prematurely marked "Done" without iOS/native validation;
change the status back to a non-final state and add explicit validation steps:
require an iOS simulator build with "yarn ios" (app/ios/**) and a native-module
verification for any native changes (app/{ios,android}/**) before marking Done,
and add a gate that the new PassportReaderCore.swift diff is included and
validated in both iOS targets (ensure PassportReaderCore.swift is present and
builds clean in each target).

---

Outside diff comments:
In `@app/ios/PassportReader.swift`:
- Around line 64-71: The new native method trackEvent(_:properties:) in
PassportReader.swift is only present in the non-E2E_TESTING branch, causing the
E2E stub to export a different bridge surface; update the E2E stub to also
declare and forward the same `@objc`(trackEvent:properties:) method so both
branches expose an identical API. Locate the E2E `#else` class and add a matching
trackEvent(_ name: String, properties: [String: Any]?) method (mirroring the
call into analytics?.trackEvent(name, properties: mpProps) or
analytics?.trackEvent(name, properties: nil) logic using Properties casting) so
E2E builds exercise the same bridge contract as production.

---

Duplicate comments:
In `@specs/projects/sdk/workstreams/native-shells/SPEC.md`:
- Around line 113-114: The spec prematurely marks NS-06 complete while later
examples and the Public API still use the old/incompatible payloads; update the
Public API `VerificationResult` type to include the `error` field and ensure
every example uses the canonical shape (`success`, `userId`, `verificationId`,
`proof`, `claims` as Map<String, Any?>, `error`) or revert the NS-06 completion
status (leave NS-06 as pending) and keep NS-07 tracking removal of
`VerificationResult.type` until all examples and the Public API are aligned;
specifically search for `VerificationResult`, `NS-06`, `NS-07`, and the "Public
API" section and change the definitions and examples to the canonical form or
undo the completion note.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 324e03fa-d326-4d88-a0c3-673249a20b95

📥 Commits

Reviewing files that changed from the base of the PR and between 7bd6287 and 808c81a.

📒 Files selected for processing (12)
  • app/ios/PassportReader.swift
  • app/ios/PassportReaderCore.swift
  • app/ios/Self.xcodeproj/project.pbxproj
  • packages/kmp-minipay-sample/composeApp/src/commonMain/kotlin/xyz/self/minipay/MainViewModel.kt
  • packages/mobile-sdk-alpha/ios/SelfSDK/PassportReader.swift
  • packages/mobile-sdk-alpha/ios/SelfSDK/PassportReaderCore.swift
  • packages/mobile-sdk-alpha/ios/SelfSDK/SelfSDK.xcodeproj/project.pbxproj
  • specs/projects/sdk/OVERVIEW.md
  • specs/projects/sdk/workstreams/native-consolidation/CONTRACTS.md
  • specs/projects/sdk/workstreams/native-consolidation/SPEC.md
  • specs/projects/sdk/workstreams/native-consolidation/plans/NC-03-phase-2-passport-reader-parity.md
  • specs/projects/sdk/workstreams/native-shells/SPEC.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/kmp-minipay-sample/composeApp/src/commonMain/kotlin/xyz/self/minipay/MainViewModel.kt

Comment thread app/ios/PassportReader.swift
Comment thread app/ios/PassportReaderCore.swift
Comment thread packages/mobile-sdk-alpha/ios/SelfSDK/PassportReaderCore.swift
Comment thread packages/mobile-sdk-alpha/ios/SelfSDK/PassportReaderCore.swift
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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 (1)
app/ios/PassportReader.swift (1)

64-71: ⚠️ Potential issue | 🟠 Major

Redact or drop analytics properties before forwarding them.

trackEvent(_:properties:) is now public, and SelfAnalytics.trackEvent logs properties verbatim to print and NativeLoggerBridge. That turns any caller-supplied verification metadata into a normal-path device log leak. Please either stop mirroring properties into native logs or enforce a strict allowlist/redaction step before forwarding them.

Based on learnings, confirm no sensitive data is exposed in logs or comments in PR.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/ios/PassportReader.swift` around lines 64 - 71, The public method
trackEvent(_:properties:) in PassportReader.swift currently forwards
caller-supplied properties directly to SelfAnalytics.trackEvent (via
analytics?.trackEvent), which leads to verbatim logging; modify
trackEvent(_:properties:) to either drop the properties or run them through a
strict allowlist/redaction routine before calling analytics?.trackEvent(name,
properties: ...), ensuring SelfAnalytics.trackEvent never receives raw caller
properties; update or add a redaction helper (e.g., sanitizeProperties or
allowedEventProperties) and apply it here, and verify no sensitive data is
written to print/NativeLoggerBridge or present in any PR comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@specs/projects/sdk/workstreams/native-shells/SPEC.md`:
- Around line 896-950: The native handler setResult(params: Map<String,
JsonElement>) is doing business logic (parsing type/success/errorCode, branching
and calling verificationResultFromLifecycleParams/serializeVerificationResult)
which violates the thin-wrapper rule; change setResult to stop interpreting the
payload: remove all parsing and branching, do not call
verificationResultFromLifecycleParams or set VerificationResult.error here,
instead serialize/forward the raw params as the single canonical outcome (put
the serialized params into SelfVerificationActivity.EXTRA_RESULT_DATA on an
Intent), call activity.setResult(SelfVerificationActivity.RESULT_CODE_SUCCESS,
intent) and activity.finish() (so native only forwards the outcome), and remove
the legacy shim logic that inspects type/success/errorCode from this handler.

---

Outside diff comments:
In `@app/ios/PassportReader.swift`:
- Around line 64-71: The public method trackEvent(_:properties:) in
PassportReader.swift currently forwards caller-supplied properties directly to
SelfAnalytics.trackEvent (via analytics?.trackEvent), which leads to verbatim
logging; modify trackEvent(_:properties:) to either drop the properties or run
them through a strict allowlist/redaction routine before calling
analytics?.trackEvent(name, properties: ...), ensuring SelfAnalytics.trackEvent
never receives raw caller properties; update or add a redaction helper (e.g.,
sanitizeProperties or allowedEventProperties) and apply it here, and verify no
sensitive data is written to print/NativeLoggerBridge or present in any PR
comments.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 75710392-4df5-4459-b556-a3df97305d67

📥 Commits

Reviewing files that changed from the base of the PR and between 808c81a and 90a73fd.

📒 Files selected for processing (2)
  • app/ios/PassportReader.swift
  • specs/projects/sdk/workstreams/native-shells/SPEC.md

Comment thread specs/projects/sdk/workstreams/native-shells/SPEC.md
@transphorm transphorm merged commit d7c1556 into dev Mar 10, 2026
43 checks passed
@transphorm transphorm deleted the justin/kmp-ns-06 branch March 10, 2026 16:02
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.

1 participant