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:

// 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:

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:

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.