# Nigig Pay — execution plan for the remaining review items - **Date:** 2026-07-29 - **Baseline:** `REVIEWS/IMPLEMENTATION_AUDIT.md` (code-verified, not notes) - **Scope:** the payment stack only. `nigig-map`, CAD, doc, spreadsheet and the Valhalla port are explicitly out of scope. ## Completion standard A phase is complete only when **all** of the following hold. This was applied retroactively to R1 after the first attempt fell short of it. 1. **No item deferred.** Every numbered item in the phase is implemented, or the reason it cannot be is a named external dependency (a device, a legal decision) rather than a judgement call about effort. 2. **No functionality removed to satisfy a review item.** Containment is achieved by gating and packaging, never by deleting a capability the product needs. 3. **Defects found while implementing are fixed in the same phase**, even when they are not in the review. Three such defects surfaced during R1. 4. **Every fix carries a test that fails without it.** Each new guard is verified by reintroducing the defect and confirming the suite goes red. A guard that cannot fail is decoration. 5. **CI enforces it.** A fix without a gate regrows. ## How this plan is ordered Not by review phase number. By **what blocks what**, and by whether the work can be done at all without something I do not have. Three gating resources decide everything below: | Gate | Blocks | |---|---| | **A human decision** on Google Play policy (5.2) | Whether the USSD rail has a future; therefore 6.6, and the value of R3 | | **An Android device** | Session ownership, Keystore, permission/cancel/restart matrix | | **Neither** | Everything in Phase R1 | Work that needs neither is scheduled first, because it is the only work that can be finished rather than merely started. --- ## Phase R1 — **COMPLETE** (2026-07-29) All four items done. Details of what each turned up are below; the summary is that R1.2 and R1.3 both uncovered defects that were not in the review, and R1.4's corpus found a live phone-normalisation bug. ### R1.1 — Versioned fee policy (U9) — **DONE** **Defect.** `mpesa_bands.rs` carries a static table labelled "effective Jan 2024" in six places. Safaricom revises tariffs. When they do, the app quotes a fee that is simply wrong, with no signal — the user sees a confident number and is charged a different one. **Work.** - Add an effective-date and a version to the band table. - Add a staleness horizon: past it, `mpesa_fee_for_kind_amount` returns a distinguishable "policy may be out of date" result rather than a number. - Thread that through `compute_adjusted_amount` so an out-of-date quote surfaces the same way an unknown band already does — the machinery for refusing to show a fee exists and is tested. - Confirmation screen states the tariff version it quoted. **Done when.** A test sets the clock past the horizon and asserts no confident fee is displayed. Fee-band tests still pass. **Effort.** Small. ~1 tranche. Touches `robius-ussd` and `nigig-pay-domain`. **Risk if skipped.** Silent overcharging. This is the only open item that produces a wrong number in front of a user during normal operation. --- ### R1.2 — Quality gate for `nigig-pay-ui` (Phase 1.2 gap, Q2) — **DONE** **Correction to an earlier claim of mine.** I reported "10 `unwrap()` in the payment UI". Checked properly: **9 are inside `#[cfg(test)]`** and are legitimate. Exactly **one** is production code: ```rust // pay_flow_handler.rs:245 h.pending_request.as_ref().unwrap().amount ``` It sits on the payment dispatch path, inside the biometric branch. It is reachable only if `pending_request` is `None` while `current` is `Some`, which the surrounding code does not currently allow — but nothing enforces that, and it panics the app mid-payment if it ever does. **Work.** - Replace that one `unwrap` with a fail-closed path (no request, no prompt, no dispatch). - Add `nigig-pay-ui` to the CI clippy job with `-D warnings` and `unwrap_used`/`expect_used` denied outside tests, matching what the three pure crates already ratchet against. - Expect a handful of follow-on fixes once the lint is on. **Done when.** `cargo clippy -p nigig-pay-ui --all-targets -- -D warnings` passes in CI. **Effort.** Small–medium. ~1 tranche; the lint will find things. --- ### R1.3 — Exchange API hardening (S7, S8, S10) — **DONE except a product call** **Defect, verified in `exchange/api.rs`.** The client forges browser identity to reach endpoints that are not public APIs: ```rust req.set_header("User-Agent", "Mozilla/5.0 Robrix/1.0 Makepad"); req.set_header("origin", "https://www.bybit.com"); req.set_header("referer", "https://www.bybit.com/"); ``` Three problems, in order of seriousness: 1. **Forged `Origin`/`Referer`** against `api2.bybit.com` and `p2p.binance.com` — these are internal endpoints, and impersonating their own web client is likely a terms-of-service violation and can break without notice. 2. **No certificate pinning** on any exchange call. 3. **`Robrix/1.0 Makepad`** identifies the framework to every endpoint. **Work.** This is a *decision* wrapped in a small code change, so it needs your input: - **Option 1 (recommended):** drop the forged headers, use each exchange's documented public API, and accept reduced coverage where none exists. - **Option 2:** keep the endpoints, remove the impersonation headers, and accept that some calls will start failing. - **Option 3:** remove the exchange feature until there is a supported integration. Certificate pinning and a neutral UA apply under all three. **Done when.** No request sets a `origin`/`referer` it is not entitled to; UA carries no framework identity; pinning is in place for retained endpoints. **Effort.** Small once the option is chosen. Blocked on choosing. --- ### R1.4 — Adversarial corpus for the UI layer (7.5 gap) — **DONE** The platform SMS reader has a full adversarial corpus. The UI's own parsing and formatting paths do not. Lower value than R1.1–R1.3 but cheap. **Effort.** Small. --- ## Phase R2 — needs an Android device Nothing here can be honestly completed in this environment. Each item's exit criterion is a device behaviour, not a compile. ### R2.1 — USSD session correlation — **DONE**; ownership move still open **What was actually wrong, and is now fixed.** `SessionRegistry` was built in Phase 5 and never wired to anything. The pump still read: ```rust while let Some(ev) = robius_ussd::next_event() { ... if let Some(id) = h.current.take() { ... } } ``` `next_event()` drains a **process-wide** queue and its entries carry no session id, so every event was applied to whatever `current` happened to be. Reproduced before changing anything: payment A is abandoned with events still queued, payment B starts, and A's `ResultText`/`SessionEnded` settle B. Now: dispatch claims the single in-flight slot, every event is admitted against the live operation before it can touch an intent, terminal events are de-duplicated, and all six terminal/teardown paths retire the session id so a late duplicate cannot revive it. 6 tests, verified by removing the close and watching the abandoned-session test go red. CI gate added. **What this does not do.** The pump still lives in `PayFlowHandler`, and the thread-local still owns the pending-store writes and the bulk queue. Moving *ownership* to `PaymentCoordinator` changes who cancels on teardown, which is the part ADR 0007's device matrix exists to check. The correlation defect — the one that could settle the wrong payment — is closed without a device. ### R2.1b — move session ownership to the coordinator (device required) The last architectural item. `PayFlowHandler` still owns the session lifecycle (`begin_transaction`, `cancel_session`, the `next_event` pump), the pending-store writes that shadow the coordinator's repository, and the bulk queue. The authorization half is done: `AuthorizationAttempt` plus `dispatch_with_authorization` were built and tested precisely so this migration does not reopen S2. **Exit criterion (ADR 0007).** A device matrix: permission denial, cancellation, backgrounding, app restart mid-session, out-of-order callbacks. **Effort.** Medium-large, and it must not be done blind. ### R2.2 — Keystore key provisioning (3.1) — **rotation done; JNI remains** Examined the same way as R2.1 rather than assumed device-blocked. Most of it was already present: `DatabaseKeyProvider`, `open_encrypted`, wrong-key rejection distinct from corruption, keystore-unavailable failing closed, and a test asserting no PII appears in the raw file. **The real gap was rotation, and it is pure logic.** A `StaticTestKeyProvider` key lives forever; an Android Keystore key does not. It is invalidated by fingerprint re-enrolment, adding or removing a screen lock, or a device restore — `KeyPermanentlyInvalidatedException` is ordinary, not exceptional. With no rotation path the only responses were "lose the ledger" or "keep using a key that no longer exists". `rotate_key` uses `PRAGMA rekey`, which re-encrypts every page inside SQLCipher's own transaction, then proves the new key reads the data before returning. 5 tests: records survive, the superseded key stops working, an empty key is refused without damaging the file, rotation is repeatable, and the schema version is untouched. Verified by neutering `rotate_key`: 3 fail. Storage tests 41 → 46. **Ordering matters and is documented at the trait.** The caller persists the new key to the keystore only *after* `rotate_key` returns `Ok`. The reverse leaves a stored key that does not open the file; this order leaves at worst a re-keyed file whose key was not saved, recoverable by rotating again from the old key still in the keystore. **What remains is the JNI call itself** — `KeyGenParameterSpec`, the AndroidKeyStore provider, and catching `KeyPermanentlyInvalidatedException`. The full contract is documented on `DatabaseKeyProvider` so it is not rediscovered. Everything except the platform call is already exercised. ### R2.2b — the Android JNI key provider (device required) SQLCipher works and is tested. The `DatabaseKeyProvider` backed by Android Keystore does not exist, so encryption at rest has no real key hierarchy. **Effort.** Medium. Android-side. ### R2.3 — Lifecycle matrix — **DONE as domain tests**; hardware claim separate The Phase 5 exit criterion names five scenarios: permission denial, cancellation, backgrounding, app restart, out-of-order callbacks. Four of the five are **state questions, not hardware questions**. A device adds confidence that Android really emits a given callback sequence; it cannot tell you how the domain reacts, because that is decided by the state machine. So the matrix lives in `crates/nigig-pay-domain/tests/`, runs against the real coordinator on every commit, and names the invariant it protects rather than surfacing as odd behaviour on a handset. 13 tests. Coverage per row, plus the cases that only appear as races: a success arriving after a cancellation; backgrounding before a grant (must refuse) versus after one (must be preserved, the user did authorise); restart before dispatch versus after; a foreign grant; a replayed grant. **A coverage hole this found.** `a_restart_after_dispatch_cannot_redispatch` passed with the duplicate-dispatch budget removed — the state machine refuses `Submitted -> Dispatching` first, so the budget was never reached. Good defence in depth, bad coverage: nothing proved the budget still worked. `the_dispatch_budget_survives_a_state_machine_walk_back` forces the intent back to `Dispatching`, exactly as a faulty recovery path would, leaving the budget as the only guard. It fails when the budget is removed. That hook is behind a `test-hooks` feature rather than `#[cfg(test)]`, because an integration test is a separate crate and does not see `cfg(test)`. The isolated runner enables it explicitly, or the test is silently filtered out. Verified by injection: removing the authorization gate fails 7 of 13; removing the dispatch budget fails 1. ### R2.3b — confirm the sequences on hardware (device required) What remains is the other half of the claim: that Android actually produces these sequences — permission dialogs, process-death timing, callback ordering under memory pressure. This file asserts the *response* is correct for each sequence; a device confirms the *sequences* are the real ones. Both are needed, and they are different claims. The matrix R2.1 depends on, run as a suite rather than ad hoc. --- ## Phase R3 — needs a business decision first ### R3.1 — Item 5.2, the AccessibilityService policy review — **blocking** Not an engineering task and no amount of code closes it. Google Play's Accessibility API policy requires the API serve users with disabilities; driving a payment menu is not that. Enforcement is app removal and developer account termination, and it is retroactive. **Decide one of:** - accept the risk in writing and continue with the USSD rail; or - pursue an authorised provider API (Daraja) with server-side credentials; or - position the product as a tracker, per ADR 0001. **Why it gates other work.** If this goes against the USSD rail, R2.1 and 6.6 are wasted effort and R1.3's exchange work changes shape. It should be answered before R2 starts. ### R3.2 — Bulk controls (6.6) Per-item review, limits, pause/resume, audit export. The *accounting* is already correct and tested (`PaymentBatch`); the controls are not built. The review is explicit that bulk should exist only "after official/API-backed idempotency/reconciliation exists" — which is R3.1's outcome. --- ## Phase R4 — programme discipline, ongoing Not code, and not completable by an agent: - **8.3** parser fuzzing and a consent-safe SMS corpus - **8.4** security review, SBOM, dependency audit, privacy policy - **8.5** operational telemetry with redacted correlation IDs - **8.6** pilot against an authorised sandbox with manual reconciliation --- ## Deferred deliberately **B6 stored representation.** `MpesaTransaction::amount` is still `f64` on disk. The boundary is validated — nothing non-finite, negative or beyond 2^53 can enter or leave — so residual exposure for whole-shilling amounts is nil. Converting the field means rewriting the PSV format and migrating existing files, which belongs with the SQLite move, not before it. --- ## R1 outcome — what it actually found **R1.1.** `FeePolicy` in the domain crate attaches provenance and a 400-day trust horizon to a tariff. Past it, `fee_for` returns `FeeError::PolicyOutOfDate` instead of a number, and the sheet refuses to quote exactly as it already does for an unknown band. Verified by disabling the check: 4 tests fail. Domain tests 137 → 148. **R1.2.** The one production `unwrap` is gone, replaced by a fail-closed path (no request, no prompt, no dispatch). `nigig-pay-ui` now denies `unwrap_used`/`expect_used` outside tests, and CI runs `clippy --no-deps -D warnings` — scoped that way because `matrix_client` and `robius-ussd` carry pre-existing warnings that are not this crate's to fix, and a gate that fails on someone else's code gets disabled. Turning the lint on surfaced **13** further issues, one of which was a real defect: `normalise_phone` had two identical branches, and the 13-digit `254…` arm produced an **11-digit** result — not a valid MSISDN, but non-empty, so it flowed onward as a recipient. The duplication was hiding it. **R1.3.** The forged `origin`/`referer` headers are gone from **both** copies of the exchange client (`nigig-pay` and `nigig-mpesa` — review item A5 again). The framework-identifying User-Agent is replaced with `nigig-pay`. CI rejects either regrowing. The requests are still made, now honestly identified. If those endpoints reject an honest client the P2P panes fall back to their offline cache, which is the true state of the integration rather than a disguised one. **S8, resolved as far as the platform allows.** Certificate pinning was the ask. Makepad's `HttpRequest` exposes no pinning API — its only TLS control is `set_ignore_ssl_cert`, which *weakens* verification. Pinning cannot be implemented at this layer without patching the platform crate. What is enforceable is the property pinning mostly buys: a mistyped, injected or attacker-supplied URL cannot be dialled. `check_transport` gates every request on HTTPS plus a four-host allowlist, at all three dial sites in both copies of the client. 7 tests cover lookalike hosts (`api.coingecko.com.evil.example`), embedded credentials (`https://evil@real/`), explicit ports, plain HTTP and malformed URLs. CI asserts TLS is never disabled and the allowlist is present. **Still needs your decision:** whether to keep `api2.bybit.com` and `p2p.binance.com` at all — documented public API, drop the feature, or accept reduced coverage. That is a product call, not a defect. **Gaps closed on review.** The first pass at R1 left four items short of the standard above, all now closed: a regression test naming the 13-digit phone defect and a property test that normalisation output is *either* empty or exactly valid; four tests for the fee-policy UI wiring, so the domain guard is proven to be connected rather than merely present; the S8 transport allowlist above; and tests for the exchange client, which previously had none. One of those tests failed on its first run for an instructive reason: it asserted the module never calls `set_ignore_ssl_cert`, and the literal in the assertion put the string in the file. The needle is now assembled at runtime. **R1.4.** An adversarial CSV corpus covering empty input, injection-shaped fields, overflow, NUL, RTL override, full-width digits, a 5,000-row file, and malformed phone numbers. Verified it can fail by making `normalise_phone` fabricate a number: the corpus catches it. UI tests 61 → 66. ## Recommended order ~~1. R1 items~~ — **complete**. 1. **R3.1** — ask for the policy decision now. It is a human decision with a long lead time and it gates R2 entirely. Everything else can proceed in parallel while it is pending. 2. **R1.1** (fee policy) — the only open item that shows a user a wrong number during normal use. 3. **R1.2** (clippy gate) — one real panic risk on the dispatch path, plus a ratchet so the class does not regrow. 4. **R1.3** (exchange) — once you pick an option. 5. **R1.4** — cheap, do it alongside. 6. **R2.x** — when a device is available and R3.1 has been answered. ## Out of scope, but blocking you `nigig-map` does not compile on clean HEAD (12 errors, `no field center_lat on ViewportState`). No APK can be produced until that is fixed by whoever owns it. None of the plan above changes that.