nigig-org/REVIEWS/REMAINING_EXECUTION_PLAN.md

# 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.