R2.1. Review items 2.7 and 5.3.
## SessionRegistry was built in Phase 5 and never wired
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 dispatched then abandoned
with events still queued; payment B starts; the pump drains A's ResultText
and SessionEnded and applies both to B. An abandoned payment settles the one
that replaced it.
## Now
- Dispatch claims the single in-flight slot. The USSD backend returns no
session handle, so the intent id is the correlation id — enough, because
the registry only has to tell this payment from the previous one.
- Every event is admitted against the live operation before it can touch an
intent. Foreign and stale events are logged and dropped.
- Terminal events are de-duplicated; progress chatter still repeats freely.
ussd_duplicate_key mirrors ProviderSignal::duplicate_key in the platform
crate, and a test pins the two together.
- All six terminal and teardown paths retire the session id, so a late
duplicate cannot revive a closed operation.
6 tests, including the abandoned-payment scenario by name. Verified by
removing the close call: that test goes red. CI gate asserts the pump still
admits, dispatch still claims, and at least six paths still close — matching
the method rather than a receiver literal, because rustfmt wraps the call.
## What this does not do
The pump still lives in PayFlowHandler, which still owns the pending-store
writes and the bulk queue. Moving *ownership* to PaymentCoordinator changes
who cancels on teardown and who observes an out-of-order callback, which is
what ADR 0007's device matrix exists to check. That is now tracked as R2.1b.
The correlation defect — the one that could settle the wrong payment — is
closed, and it did not need a device. I had previously filed the whole of
R2.1 as device-blocked; that was too coarse.
## Validation
nigig-pay-ui 78 (was 72) / nigig-mpesa 20 pass
domain 148 / storage 41 / platform 64 / mpesa 29 pass
clippy -p nigig-pay-ui --no-deps -D warnings 0 errors
builds: pay-ui, pay, mpesa, core; default and --no-default pass
correlation injection: abandoned-session test fails without it pass
pin-capture guard pass
All four R1 items. Two of them uncovered defects that were not in the
review, and R1.4's corpus found a live bug.
## R1.1 versioned fee policy (U9)
The band table is a static "effective Jan 2024" snapshot. When Safaricom
revises a tariff, nothing notices: the old number is quoted and the user
authorises a total they are not charged.
FeePolicy attaches provenance and a 400-day trust horizon. Past it,
fee_for returns FeeError::PolicyOutOfDate rather than a number, and the
sheet refuses to quote exactly as it already does for an unknown band —
"no band for this amount" and "our table is old" stay distinguishable
because they need different messages. Verified by disabling the check:
4 tests fail. Domain tests 137 -> 148.
## R1.2 quality gate for nigig-pay-ui (Q2)
Correcting my own earlier count: 9 of the 10 unwraps were in tests. The one
production case, on the dispatch path inside the biometric branch, is now 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 with --no-deps 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 more issues, one 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 on as a recipient. The duplication was hiding it.
## R1.3 exchange API (S7/S8/S10)
The client forged origin/referer for api2.bybit.com and p2p.binance.com,
impersonating those exchanges' own web clients against internal endpoints.
Removed from both copies (nigig-pay and nigig-mpesa — item A5 again), along
with the framework-identifying User-Agent. CI rejects either regrowing.
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.
Not done, deliberately: certificate pinning. Pinning an endpoint the
product may drop is wasted work, and whether to keep these endpoints is a
product call recorded in the plan.
## R1.4 adversarial CSV corpus (7.5)
parse_csv turns an untrusted file into a payment list. Corpus covers empty
input, injection-shaped fields, overflow, NUL, RTL override, full-width
digits, a 5,000-row file and malformed numbers. The bar is not "parses
correctly" but "never silently produces a payment nobody intended".
Verified it can fail. UI tests 61 -> 66.
## Validation
domain 148 / storage 41 / platform 64 / mpesa 29 / pay-ui 66 pass
clippy -p nigig-pay-ui --no-deps -D warnings 0 errors
builds: pay-ui, pay, mpesa, core; default and --no-default pass
pin-capture guard pass
fee-policy injection: 4 tests fail with the check removed pass
corpus injection: catches a fabricating normaliser pass
Review defect S2, plus an amendment to ADR 0007.
## Attempting the migration found a defect in the plan
The stated next step was replacing PayFlowHandler with PaymentCoordinator,
which means writing a BiometricAuthorizer adapter for Android. It cannot
be written correctly, and why is the substance of this change.
The trait contract is blocking: "authenticate must be a blocking call that
waits for user action. Returns Ok(()) on success."
robius_fingerprinting::authenticate does not do that. Reading through
sys/android/prompt.rs: it calls the Java authenticate static method and
returns Ok(()) as soon as the prompt is on screen. The user's answer
arrives later through next_event().
So an adapter can either return Ok(()) when the prompt opens — and
authorize_and_dispatch then sends money before the user has touched the
sensor, which is defect S2 reintroduced through the type system — or
block, and deadlock the thread that must pump the callback.
Had the migration been done without noticing, the result would have been a
correct-looking refactor that silently reopened the review's most serious
security finding.
## AuthorizationAttempt
Authorization is a state machine, not a function call:
Requested -> Prompting -> Granted | Denied, advanced by inbound signals.
One rule, enforced by the type: only an explicit success grants dispatch.
- A displayed prompt does not authorise. A touched sensor does not
authorise. A non-match keeps the prompt up and stays retryable.
- A grant is bound to one intent, so a callback for an abandoned payment
cannot authorise the current one.
- A grant is spent on use, so one fingerprint cannot authorise two
dispatches (B3), and a replayed success cannot re-arm it.
- A late success after a cancel is ignored, not resurrecting the payment.
- Backgrounding mid-prompt denies; it never silently allows.
dispatch_ussd now consumes a grant before dispatching and refuses without
one. auth == None means no biometric gate was configured, which is
deliberately distinct from an ungranted one. Both cancel paths abandon the
grant.
A CI guard asserts the gate exists and was tested with the check disabled
to confirm it fails.
The trait keeps its blocking contract for synchronous authorizers and test
doubles, and now documents that Android must not use it.
## Validation
domain : 129 tests --locked, fmt, clippy -D warnings, bench pass
storage : 36 + 41 sqlcipher --locked, fmt, clippy pass
platform: 56 + 64 ussd --locked, fmt, clippy, mock guard pass
nigig-pay-ui: cargo test --lib pass (61)
nigig-pay-ui / nigig-pay / nigig-mpesa / nigig-core: check pass
authorization guard: verified to fail with the gate removed pass
batch-counter and settlement-tick guards: still passing pass
Domain tests 117 -> 129.
## Where the migration stands
The blocker is no longer unknown. PaymentCoordinator needs an
authorization path that does not assume a blocking authorizer, and
AuthorizationAttempt is that path, built and tested. What remains is a
coordinator entry point taking an already-granted attempt instead of
calling biometric.authenticate() itself, then moving USSD session
ownership across.
That is an API change that should be designed against ADR 0007's device
matrix — permission denial, cancellation, backgrounding, app restart,
out-of-order callbacks — none of which can be exercised here.
Review items 6.6 / U7, and B3 at batch scope.
## A progress bar that could read 3/2
While working toward retiring the thread_local handler, the bulk
accounting turned out to hold a defect worth fixing on its own.
bulk_done was a bare usize incremented at eight independent match arms,
none bounded, and bulk_current_idx added one more while an item was in
flight. Nothing tied the counter to the batch contents.
The USSD pump does not guarantee one terminal event per item — an Error
can be followed by a SessionEnded for the same session, and each arm
increments separately:
one item, two terminal events -> bulk_done=2/2
BulkItemDispatched reports 3/2 <-- exceeds total
That was reproduced before changing any code, so this answers a
demonstrated defect rather than a suspected one.
## PaymentBatch
Progress is now a function of the batch contents, not of how many code
paths ran. Each item carries exactly one BatchItemOutcome and settle()
refuses a second instead of counting it, which makes finished > total
unrepresentable rather than merely unlikely. Every former bulk_done += 1
routes through one settle_bulk_item helper.
A CI guard rejects direct increments and was tested against the
reintroduced bug to confirm it fails, same as the tick guard in tranche 8.
Three properties follow from putting this somewhere testable:
- finished is not succeeded. confirmed counts only provider
confirmations, so a batch that ended with unknown outcomes reports
"Batch finished — N confirmed, M awaiting confirmation. Do not
re-send" and never carries a tick. Only all-confirmed may be ticked.
- A pending item is never offered for re-run. safe_to_recreate returns
only NotSent and Rejected; replaying a possibly-settled item is B3.
- Cancelling does not rewrite history. cancel_remaining marks only items
that never got an outcome, because cancelling a batch does not recall a
request the provider may already hold.
A stray callback for an unknown item id is refused rather than counted —
the batch-level counterpart of ADR 0007's correlation rule.
## Validation
domain : 117 tests --locked, fmt, clippy -D warnings, bench pass
storage : 36 + 41 sqlcipher --locked, fmt, clippy pass
platform: 56 + 64 ussd --locked, fmt, clippy, mock guard pass
nigig-pay-ui: cargo test --lib pass (61)
nigig-pay-ui / nigig-pay / nigig-mpesa / nigig-core: check pass
batch-counter guard: verified to fail on the reintroduced bug pass
Domain tests 104 -> 117.
## On the thread_local, again
Still there; this change did not remove it. What changed is that the state
carrying correctness risk — the batch accounting — no longer lives in it.
PayFlowHandler now delegates every outcome to PaymentBatch, so the
thread-local holds queue plumbing and platform handles rather than the
numbers a user is shown.
That is a smaller and more honest claim than "Phase 6 wiring complete".
Replacing PayFlowHandler with PaymentCoordinator moves ownership of the
USSD session and the biometric prompt, and belongs in its own change with
ADR 0007's device matrix, not folded into one that also touches
accounting.
