Asked in review: "does the latest code have the PIN input field?" It did.
## Hiding was weaker than it looked
pin_input, form_pin and the reveal toggle were all present, and the default
build hid them at runtime in on_after_new. Three problems:
1. The DSL declared the control visible and Rust hid it afterwards, so
anything re-applying the UI definition — a hot reload, a re-instantiated
sheet — brought it back. Defect B11 already names this class.
2. Hidden is not absent. The TextInput stayed in the widget tree, and a
hidden input can still be focused or filled programmatically.
3. form_pin was compiled into every build, so any path reaching it could
populate it.
Item 0.3 asks for removal, not concealment.
## The field no longer exists without `demo`
form_pin, pin_visible, the eye-toggle handler, the text-input handler, both
PIN checks, the request construction and try_build_ussd_request are all
#[cfg(feature = "demo")]. A default build has no field to write to.
The DSL now declares visible: false on the PIN input and its reveal button,
so hidden is the default state rather than a runtime correction; demo
unhides them on init. That closes the reload path in (1).
One runtime call remains as belt-and-braces: if a hot-reloaded definition
surfaces the input, the non-demo arm wipes what was typed. It has no
form_pin to clear, because there isn't one.
## Proving absence rather than asserting it
A grep for form_pin proves nothing — it passes just as happily against a
field still present behind a runtime if. tools/check-no-pin-capture.sh is a
compile probe: it references the field outside any cfg block and requires
the default build to fail with "no field `form_pin`" while demo succeeds.
The script restores the file on every exit path.
Verified both directions: passes on current code, exits 1 when the field is
re-exposed un-gated.
## Validation
cargo check -p {nigig-pay-ui,nigig-pay,nigig-mpesa,nigig-core} pass
cargo check -p {nigig-pay,nigig-mpesa} --features demo pass
nigig-pay-ui: cargo test --lib pass (61)
domain / storage / platform / mpesa harness pass
no-PIN-capture guard: verified to fail on a re-exposed field pass
## What 0.3 still leaves open
The Java-side KEY_PIN scrubbing was already done, so 0.3 is complete for the
default product. A demo build still captures a PIN by design — that path
exists for authorised device testing and is covered by the unresolved
Play-policy decision in ADR 0007. If that decision goes against the USSD
rail, the demo path and its PIN capture are deleted with it.
Phase 6 and item 7.3 of REVIEWS/NIGIG_PAY_CONSOLIDATED_REVIEW.md.
Rationale in REVIEWS/adr/0008.
## The rule, made into a type
Phase 6's exit criterion is that the UI cannot call a transaction
successful without trusted confirmation, or failed without known
rejection. The old code violated it structurally: the sheet set its
status inline in about a dozen places, each from whatever local signal
was nearest, so there was nowhere to put the rule.
The worst instance is U4. A payment whose confirmation SMS had not
arrived in five minutes was marked Failed, the sheet closed, and the user
saw "✗ Payment failed" — with a retry button — while the money was gone.
payment_view_state.rs makes the rule a type. PaymentPresentation has no
Success variant reachable from untrusted evidence and no Failed variant
reachable from a missing SMS. VerificationExpired, Unknown and
UnknownNeedsReconciliation all present as PendingConfirmation: not
success, not failure, and no retry offered. Two defence-in-depth rows:
Confirmed without a provider reference reads as unknown rather than
settled, and an unclassified failure is not evidence nothing was sent.
6.2: ConfirmationSummary makes every required term a mandatory field, so
a confirmation missing the fee or total is not constructible. It is built
from the same quote that gets dispatched. material_digest() binds consent
to the exact terms shown and is re-checked on OK — if anything material
moved in between, the authorisation is void and the screen is shown
again. The modal previously showed only recipient and amount.
6.3: must_stay_open() keeps pending payments visible, and the status line
begins with the exact required wording, asserted by a test.
6.5: evidence rows are exposed and labelled untrusted; pending payments
offer receipt, problem-reporting and data-deletion actions.
## Three defects found by writing the tests
1. Bulk quotes silently under-charged. compute_bulk_costs used filter_map
over the fee lookup, so a contact outside the tariff was dropped from
the fee total and the user was quoted less than they would pay.
2. The batch total could overflow — .sum() panics in debug, wraps in
release. A wrapped total is a quote for the wrong amount.
3. The cost preview showed unknown fees as free via unwrap_or(0).
All three are B5/B6 territory. Item 3 is B5 resurfacing in the preview
path after tranche 1 fixed it in the dispatch path: fixing a defect at one
call site is not the same as fixing the defect.
## A pre-existing failing test, diagnosed rather than deleted
money::tests::ksh_rounds_to_nearest_cent asserted format_ksh(1.005) ==
"1.01" and had been failing on every run — confirmed pre-existing by
stashing this work and re-running clean.
The expectation is impossible, not the formatter wrong: 1.005 has no
binary representation, the nearest f64 is 1.00499999999999989..., so the
correctly rounded result is 1.00. This is defect B6 at its smallest. The
test now says so, with a companion showing Money handling it exactly.
Deleting it would have hidden a live argument for finishing B6.
## 7.3 UI tests in CI
The NIGIG_TEST_PAY gate was already gone; what blocked CI was the Makepad
link step. tools/makepad-native-libs.sh listed the libraries needed to
compile but not libasound2-dev, libpulse-dev and libssl-dev, which are
needed to link a test binary — cargo check succeeds and then
"unable to find library -lasound" appears much later. The helper now
installs and checks them, and a payment-ui-tests job runs the UI tests
plus cargo check on nigig-pay-ui, nigig-pay and nigig-mpesa.
## Validation
domain : 95 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: cargo check pass
Domain 75 -> 95 tests. UI 55 -> 61, with the long-standing failure fixed.
Every new CI step was run locally before commit.
## Not claimed complete
Phase 6 wiring is partial. PaymentViewState exists and is tested, and the
confirmation path uses ConfirmationSummary, but the sheet's remaining
status strings are still set inline and the thread_local PayFlowHandler
still exists. The type makes that migration mechanical; it does not
perform it. 6.6 bulk stays demo-gated pending ADR 0007's unresolved 5.2.
Phase 5 of REVIEWS/NIGIG_PAY_CONSOLIDATED_REVIEW.md. Design and the one
item engineering cannot close are in REVIEWS/adr/0007.
## Phase 4 is now verified, not just written
Tranche 5 implemented the draw_walk fixes and said plainly that the
nigig-pay widget edits were unbuilt. That blocker was environmental:
installing the packages tools/makepad-native-libs.sh already lists makes
the UI graph compile. Both commands the status doc listed as required
now pass:
cargo check -p nigig-pay pass
cargo check -p nigig-pay-ui pass
No code changed for this; the claim is now evidence rather than assertion.
## Phase 5: new nigig-pay-platform crate
ADR 0002 reserved this crate and marked it "not yet created".
5.1 One crate owns the seam. It is the only crate in the payment stack
that may name a platform SDK. CI enforces both directions: payment crates
may not import Makepad, and domain/storage may not import jni or the
robius platform crates.
5.3 Correlation is mandatory and single-flight. SessionRegistry admits an
event as Accepted, Duplicate or Ignored; PlatformEvent cannot be built
without a CorrelationId. Closed session ids are retired permanently, so
an abandoned session's confirmation cannot settle the payment that
replaced it. There is a test named after exactly that scenario.
2.8/5.3 Progress decides retry safety, not error kind. classify_failure
takes the failure and the DispatchProgress reached before it, and
progress is the authority. The same TemporarilyUnavailable is safely
retryable before the dial and ambiguous once the menu is being driven —
the distinction the old code could not make, which is defect B3's
mechanism. A property test asserts across the whole failure space that
nothing which may have reached the provider authorises a fresh attempt.
5.4 Fakes cannot ship. MockGateway is cfg-gated, is a compile_error! in a
release build unless allow-mock-in-release is named explicitly, and
stamps every session id with MOCK-. CI asserts the release build fails.
5.5 No unsafe, no PIN. The crate is #![forbid(unsafe_code)] so the JNI
surface stays in robius-ussd. UssdGateway is !Send/!Sync by construction,
making the main-thread requirement a compile error. The adapter leaves
the pin field empty and a test asserts it.
5.6 The web claim is withdrawn. No browser API can drive USSD and a
Daraja credential must never reach a browser, so WebGateway refuses every
call and maps to Fatal — "never sent" — which owes no reconciliation.
7.5 Adversarial SMS corpus. StrictMpesaSms is the payment-boundary
reader, deliberately separate from nigig-core's permissive tracker parser
(ADR 0007 explains why this is not the duplication ADR 0002 forbids). It
requires an exact 10-char code, exact sender-ID match so MPESA-REFUNDS
and FAKE-MPESA are refused, rejects fractional shillings instead of
rounding, and caps body length. Corpus covers spoofing, forged code
shapes, out-of-range amounts, unicode and NUL injection, and replay. The
closing test asserts the honest limit: a well-crafted forgery is still
only evidence, because the output type has no settled state to reach.
## 5.2 is not done and is not closeable here
The AccessibilityService Play-policy review is a business decision. ADR
0007 records it as blocking, states the termination exposure, and names
what must happen before the rail is enabled. USSD dispatch stays behind
the default-off demo feature. If the review fails, ADR 0001's
tracker/launcher position applies and only the dispatch adapter is lost.
## Validation
platform: 56 tests, 64 with --features ussd, fmt, clippy -D warnings
(both feature sets), cargo-deny, mock-in-release guard
asserted to fail pass
domain: 75 tests --locked pass
storage: 36 + 41 tests --locked, incl. sqlcipher pass
nigig-pay, nigig-pay-ui: cargo check pass
cargo-deny reports advisories/bans/licenses/sources ok. The isolated
runner gained a `platform` target and it runs in CI on every push that
touches the crate.
Phase 6 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md: "Real PDFs. Real
regressions. No test theater."
Step 6.1, corpus. 27 fixtures across basic, fonts, forms, annotations,
images, edge and malformed, in the layout the review specifies. They are
produced by tests/corpus/generate.py rather than committed as opaque blobs,
because a corpus you cannot read is a corpus you cannot trust; CI regenerates
them and fails if they differ. Hand-rolled rather than library-produced,
since fixtures for a parser must contain constructs a library refuses to
emit.
Step 6.2, corpus tests (pdf-document/tests/corpus.rs, 27 tests). Text,
vectors, Flate, multipage, CID fonts, every form field type, inherited field
keys, link actions, hidden annotations, XObjects, inline images with
embedded EI bytes, rotation, crop boxes, nested CTMs and content arrays.
Step 6.3, robustness (pdf-document/tests/robustness.rs, 3 tests) plus five
cargo-fuzz targets. cargo-fuzz needs nightly and libFuzzer so it cannot gate
a stable CI run; the harness covers the same ground deterministically by
mutating the real corpus with a fixed-seed PRNG, so a failure is reproducible
from the seed rather than only from a saved artefact. The fuzz targets remain
the deeper coverage-guided search and run on a schedule.
Three crashes on untrusted input, all found by this work and all previously
reachable from a malformed file:
- collect_pages_ref recursed forever on a /Kids cycle. Stack overflow aborts
the process; it cannot be caught. Now tracks visited nodes and bounds depth.
- PdfDocument::resolve and the COS lexer recursed once per nesting level, so
a file of 5000 open brackets overflowed the stack. Both are now bounded.
- decode_85_group multiplied an accumulator that a malformed group can
overflow, and subtracted below zero on a digit outside the valid range.
Both panic in a debug build. Now saturating.
Step 6.4, CI (.forgejo/workflows/pdf.yml). An engine job that runs the
corpus and robustness suites under rustfmt and clippy -D warnings; a separate
makepad-integration job so a missing system library is not reported as a PDF
regression; and a scheduled fuzz job. The engine job also enforces the two
architectural rules mechanically rather than in prose: no Makepad dependency
or import in the engine crates, and no process or URL launching anywhere in
them.
Also fixes .gitignore: the blanket *.pdf rule silently excluded all 27
fixtures, which would have left CI unable to run them on a fresh clone.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (233 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (268 tests)
Both rustfmt and clippy -D warnings clean; all five fuzz targets compile.
Integration Tests:
- Add tile_decode_integration.rs with 17 test cases
- Test simple and complex geometry decoding
- Test security limit enforcement
- Test error handling for malformed input
- Test various OSM feature types
- Test different zoom levels
- Test Unicode tag handling
Benchmarks:
- Add tile_decode_bench.rs with 6 benchmark suites
- Benchmark simple JSON decoding
- Benchmark scaling with feature count (10-1000 features)
- Benchmark MVT parsing performance
- Benchmark geometry tessellation
- Benchmark POI extraction (100 POIs)
- Benchmark label extraction (50 labels)
Fuzz Testing:
- Add fuzz testing infrastructure with cargo-fuzz
- Create 3 fuzz targets: MVT parser, Overpass parser, full pipeline
- Test parsers with random input to find crashes and edge cases
Test Runner:
- Add tools/run_map_tests.sh for easy test execution
- Support for unit, integration, bench, fuzz, and coverage tests
- CI-friendly test execution
Documentation:
- Add PHASE6_TESTING_VALIDATION.md with comprehensive guide
- Document test categories, running tests, and best practices
- Include CI workflow examples and future improvements
This completes Phase 6 of the code quality improvements.
Phases 4 and 5 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md.
pdf-makepad had never compiled. It was missing eight PdfDevice methods, had
a non-exhaustive RenderCommand match, called a function through the wrong
path and held a borrow conflict. Everything previously claimed about the
Makepad rendering path was therefore unverified. It now builds and is tested
under a new pdf-ui target; the makepad build needs system GUI libraries, so
that target checks for them with pkg-config and names the missing packages
rather than failing in the linker.
Phase 4 (new pdf-makepad/src/interaction.rs):
- Viewport maps between PDF space (y up) and screen space (y down) in one
place, so hit testing and rendering cannot disagree. Inverted /Rect
corners are normalised.
- Click routing follows the review order: form fields first, then annotation
hit testing. A field overlapping a link takes the click.
- Clicking a link emits PdfAction::OpenUri to the host. The viewer never
opens a URL itself (rule 5).
- Text editing is buffered: typing changes a working copy and only Enter,
Tab or blur commits it, so Escape abandons an edit with the document
untouched. Caret arithmetic is in characters, not bytes, so a non-ASCII
value cannot panic.
- Checkboxes toggle on click using the state the widget declares.
- Hover reporting for cursor feedback; read-only fields are not targets.
- render_fields() returns placement data so a focused field shows its
uncommitted buffer while drawing stays trivial.
Phase 5:
- New pdf-graphics/src/cid.rs: composite font support. Codespace ranges give
variable-width code decoding (the previous CMap was u8 to char, so every
two-byte CID font decoded as garbage), plus cidchar/cidrange, ToUnicode
bfchar/bfrange including array destinations and multi-character values
such as ligatures, /W and /DW CID widths, and Identity-H/V. An unknown
predefined CMap returns None instead of silently substituting Identity;
unmapped codes decode to U+FFFD instead of vanishing. Width ranges are
bounded so a malformed /W cannot exhaust memory.
- New pdf-graphics/src/text.rs: the PageText extraction API. Segments carry
origin, advance and font size; find() returns matches with rectangles;
selection spans runs and works in either drag direction. Runs sharing a
baseline stay on one line even with mixed font sizes.
- renderer.rs: the two unexplained `fs * 0.75` constants are replaced by a
named DEFAULT_EM_TO_CAP_RATIO used only as a fallback, with the real
ascent preferred when the descriptor supplies one. Ts (text rise) was
stored but never applied, so superscripts drew on the baseline.
Also fixes clippy across pdf-makepad, which had never been linted.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (167 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (191 tests)
Both rustfmt and clippy -D warnings clean.
Phase 0 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md requires a clean
baseline before any feature work. The four pdf crates did not compile at all,
so every test claim about them was unverified.
Compile fixes:
- decode_lzw/decode_run_length returned Vec<u8> where callers expected
PdfResult, so decode_stream_with_params did not type-check.
- interpret_ops called a current_state_mut() method that PdfDevice does not
have; colour-space tracking now goes through explicit device hooks.
- image.rs used miniz_oxide without depending on it; PNG inflate now reuses
the COS crate through a new pdf_cos::filter::inflate_zlib.
Correctness fixes found while making the code build:
- LZW and RunLength silently truncated malformed input and indexed unchecked;
both now return typed errors (rule 6: no silent degradation).
- Tw/Tc/Tz were parsed and thrown away, and the " operator dropped its word
and character spacing, so every advance after them drifted.
- TJ attached each kern to the preceding string instead of the following one
and discarded a trailing kern entirely.
- GlyphWidths::width() fell back to default_width for out-of-range codes;
PDF 32000-1 9.6.2.1 requires /MissingWidth.
- Text advances silently substituted a guessed font_size * 0.6 when no width
table was present; ShowTextWithMetrics now carries advance_is_measured so
callers can distinguish a measurement from an unknown.
Tests: two tests had never compiled and were wrong once they ran (WinAnsi
0x99 is U+2122 not U+2019; q/cm/l/Q records four commands not three). The
document test asserted only that the writer emits a %PDF header; replaced
with real page-tree, out-of-range and malformed-input coverage.
Adds a pdf target to tools/test-rust-clean.sh that tests the three
UI-independent crates bottom-up under rustfmt and clippy -D warnings.
Validation: TEST_TARGET=pdf ./tools/test-rust-clean.sh
72 tests pass; rustfmt and clippy -D warnings clean.
Phase 1 of NIGIG_PAY_CONSOLIDATED_REVIEW.md is now closed.
1.1 build governance:
- Declare license = "MIT" on both payment crates. cargo-deny correctly
reported them as unlicensed, which would block any distribution review.
- Version-pin the nigig-pay-domain path dependency; a bare path dependency
is a wildcard requirement.
1.2 quality gates:
- Add deny.toml and a CI job running cargo-deny over both payment crates.
Advisories, bans, licences and sources all pass. The config bans the
makepad-* crates outright and restricts sources to crates.io.
1.3 canonical ownership (ADR 0002):
- Record the domain/storage/platform/UI layering and its one-way deps.
- The review's A5 "fork farm" table is stale: one copy each of parser.rs,
store.rs, pending_store.rs and pay_flow_handler.rs, not three.
- Fix B1: store.rs parsed category, sub_category, status and confidence
from disk then overwrote them with Default::default(), losing every user
categorisation on reload. Persistence also wrote display names, which are
not reversible, so this adds stable storage tokens with a legacy-display
fallback so existing rows still load.
- Fix B4: clean/restore mapped '|' to '~' and reversed every '~', so
"JOHN~DOE" loaded as "JOHN|DOE". Replaced with bijective backslash
escaping covering the separator, newlines and carriage returns.
- B6 (f64 money) deliberately deferred to Phase 6: it is a type change that
ripples into UI consumers.
These were previously recorded as untestable because nigig-core is not a
workspace member. That was wrong: the three files involved need only serde,
chrono, one log! macro and one app_data_dir() helper. The new
tools/test-mpesa-store-clean.sh supplies those shims in a throwaway crate
and runs 9 tests, two of which reproduced the defects before the fix.
1.4 boundary: enforced twice, by a CI manifest/import check and by the
deny.toml ban list.
1.5 shims: three re-exports in nigig-pay/src/lib.rs had zero callers and are
deleted. The remaining four carry a caller count and a named migration
target so they have a deletion plan rather than an open-ended lifetime.
1.6 scope (ADR 0001): accepted that Nigig Pay is a read-only tracker and
launcher, not a payment processor, until an authorised provider integration
exists. This is the decision the review required before further UI work.
SECURITY: a live Cloudflare API token was found committed in README.md,
present since the initial commit and pushed to a public remote. Removed and
recorded as R-SEC-001 in PAYMENT_RISK_REGISTER.md. Redaction does not revoke
it; it remains in history and must be rotated by the owner.
Validated on rustc 1.97.1: domain and storage each pass test --locked, fmt,
clippy -D warnings and cargo-deny; storage also passes --features sqlcipher;
domain benches run; 9 M-Pesa store tests pass. 49 manifests parse.
No new phase was started. This finishes every item tranches 1-2 left
partial, so Phases 1, 3, 7 and 8 are complete for the two pure crates or
explicitly blocked on crates that cannot be built here.
Phase 1 (build governance):
- Check in Cargo.lock for both payment crates and build with --locked.
.gitignore excluded them, which would have made item 1.1 a false claim.
- tools/test-rust-clean.sh now reads the channel from rust-toolchain.toml
instead of a hardcoded default that had already drifted, and gates on
fmt, clippy -D warnings, the sqlcipher feature and the benchmarks.
- CI enforces lockfile presence/freshness and a Makepad-boundary check that
inspects manifests and use/extern lines rather than prose (1.4).
Building on the declared 1.97.1 toolchain surfaced five lints 1.85 missed;
all are fixed. The two Default impls are annotated rather than derived
because each encodes a security or state-machine decision.
Phase 3 (secure repository):
- 3.1 encryption at rest: new encryption.rs and an opt-in sqlcipher feature.
PRAGMA key is applied first and verified by a forced read, so a wrong key
fails as KeyRejected rather than as corruption. DatabaseKey redacts its
Debug and zeroes on drop. No key is ever derived or persisted here, and
there is no unencrypted fallback. A test asserts the recipient MSISDN is
absent from the raw database bytes.
- 3.5: preferences.rs replaces the ANDROID_DATA marker file whose existence
was the value; every field fails safe.
- 3.6: redact.rs. PaymentIntent's derived Debug leaked a customer MSISDN
into any log line; it now masks phone, name and ids, with a regression
test. StorageError no longer prints a full intent id.
Phase 7:
- 7.7 benchmarks over money, validation, fees, the intent lifecycle and
evidence handling, using the stable harness so they run on the pinned
toolchain. BASELINE.md records measured output.
- 7.1/7.2/7.3 verified already satisfied; the review text is stale.
Phase 8:
- 8.1 simulator.rs with a scripted gateway and biometric, no clock or I/O.
- 8.2 property tests over every permutation of a representative event set:
no ordering dispatches twice, untrusted events never settle a payment,
ambiguity never becomes failure, Confirmed is terminal, duplicate
confirmations are idempotent, and replayed SMS cannot fake a conflict.
Validated on rustc 1.97.1 outside the incomplete workspace graph:
domain 66 tests, storage 20 tests, storage+sqlcipher 25 tests, fmt clean,
clippy -D warnings clean on both crates and both feature sets, benches run.
49 manifests parse; git diff --check clean.
Still unclaimed and blocked on uncompilable crates: UI wiring, Keystore key
provisioning, Phase 4 draw_walk I/O, Phase 5 adapters and legal review, the
Java PIN scrub, and B1/B4/B6 in nigig-core.
