build_report_email (pure, tested) attaches the report PDF as
application/pdf in a multipart message to a comma-separated finance
recipient list. spawn_email_trip_report fetches the inbox, extracts trip
receipts, builds the report, and emails it via the signed-in SMTP account;
the proxy backend reports 'attachments unsupported' honestly rather than
failing silently. The Finance card gains a recipients field and an
'Email report to finance' button.
An end-to-end test sends the attached PDF through the SMTP sink and asserts
the recipient, application/pdf type and filename land in DATA. Domain tests
234 -> 237.
spawn_export_trip_report fetches the inbox (shared fetch_inbox_messages
helper), extracts trip receipts, builds the report, and writes
trip-expense-report.pdf into app-data; it posts TripReportExported with the
path and a summary. The More page gains a Finance card with an
'Export trip report (PDF)' button and a status line.
finance_report.rs renders TripReceipts into a self-contained PDF with the
nigig PDF stack (nigig-pdf-graphics, base-14 Helvetica): a summary table of
dates and amounts with a total row, then one receipt block per trip, A4
with pagination and a bookmark outline. build_trip_report is pure and its
tests read the output back through PdfDocument, asserting page sizes,
dates, amounts and the total landed. Native-only (gated behind
not(wasm32)), so the wasm build stays free of the PDF dependency. 6 tests,
plus a runnable example.
email_receipts.rs turns inbox messages into a TripReceipt — date, amount,
currency, route, receipt id — for the finance department's expense report.
Sender detection (bolt/uber/taxify), a tolerant currency+amount finder
(total > fare > amount priority, comma/space grouping, KSh→KES), a date
extractor (ISO, d/m/y, '17 Aug 2026', 'Aug 17, 2026', with the email
timestamp as fallback), and label-prefix value extraction for the route and
receipt id. A message without an amount is not a receipt. 12 host tests over
realistic fixtures.
The assessment's 'What I have not verified' listed the TLS handshake and a
MITM test as gaps — the S1/S3 analysis of relay()/TlsParameters::new was a
read of lettre's source, never observed. Two tests now execute the PRODUCTION
build_transport path over a real TCP + TLS socket:
- a_starttls_downgrade_is_refused_without_sending_credentials: a server that
cannot STARTTLS receives no AUTH/MAIL FROM/RCPT TO/DATA — credentials and
the message never cross a cleartext link (S3/T-E1 downgrade protection).
- a_self_signed_certificate_is_rejected: a server presenting a self-signed
cert (minted with rcgen, served by tokio-rustls/rustls) is rejected by the
transport, whose accept_invalid_certs is false — the active-MITM scenario
(S1), observed rather than assumed.
The SMTP sink now records every command line so a test can assert a command
was never sent. Domain tests 214 -> 216.
Extract build_email_message (the pure message construction) and send_bounded
(the whole send wrapped in platform::timeout). A hand-rolled SMTP sink on
127.0.0.1 now receives a real send and asserts the envelope, every
recipient, and the DATA payload — the first time the SMTP conversation has
been executed in this repo.
This surfaced a real defect: lettre's .timeout() only bounds the TCP
connect, not the greeting/command reads, so a server that accepts and never
greets hangs the send indefinitely (the review's A6/P4 '60s default' claim
was wrong for the read path). send_bounded closes that gap, and
a_send_to_a_silent_server_errors_instead_of_hanging pins it.
proptest dev-dependency (the same one the SMS crate uses) and a new
email_properties module pinning 'never panic + structural invariants' for
the untrusted-input parsers: looks_like_email, looks_like_hostname,
parse_recipients, is_plausible_address, preview_line (the A3 byte-offset
class of bug) and parse_imap_date. Arbitrary input must not panic, and an
accepted verdict must satisfy the structural checks it exists to enforce.
C7: pull-to-refresh on the inbox, mirroring the SMS/M-Pesa transaction
lists (scrolled + scroll_position over a threshold, throttled to 1.2s and
guarded by the in-flight flag). The Refresh button remains for platforms
without a gesture.
C1f: a real KeyringCredentialStore behind the keystore feature -- the OS
credential vault (Linux Secret Service, Windows Credential Manager, macOS
Keychain) via the keyring crate, so IMAP credentials can survive a restart.
Native only; without the feature active_store() stays fail-closed. The
runtime vault is not host-verified (no secret service in CI), which is the
same honest caveat as the IMAP transport.
The Bulk tab was capped at MAX_RECIPIENTS (100): a 500-recipient list was
refused with TooManyRecipients, not paced. That is a capped single send,
not bulk.
email_bulk.rs: bulk_send_plan splits a list into provider-sized batches
with a pacing schedule (pure, tested), and run_bulk_send executes the plan
— gap between batches, rate-limiter backstop, abandon check between every
step, per-batch progress. Tested against a mock send (batching, delays,
failed-batch counting, abandon).
email_send.rs: validate_bulk_message accepts a list over the cap (the
caller batches it) while still enforcing subject/body limits.
The Bulk page now sends <=100 recipients as one message and anything over
as paced batches, posting BulkSendProgress after each batch and at the end.
Domain tests 195 -> 206.
D4: a one-slot transport pool reuses the SMTP connection across sends
instead of rebuilding a transport (TCP+TLS+AUTH) per operation. Safe
because SEND_IN_FLIGHT already serialises sends, so a single reused
transport is exactly the right size. The entry is moved out of the pool
while held (a MutexGuard is !Send and would poison the spawned future),
and the reuse-vs-rebuild keying -- server/port/username AND password --
is a pure, tested function. Connection-level reuse is a read of lettre's
contract, not an observed handshake (no live relay in CI).
D5: EmailWorkerAction::None is gone, with its Default and
ActionDefaultRef impls. The action is consumed only via downcast_ref()
(needs 'static + Debug), so no default was ever required.
Also: EmailSessionAction (SignedIn/SignedOut) so the More page's sign-out
reaches the inbox across the sibling-page PageFlip boundary.
C1e: imap_client.rs — ImapTransport trait, ImapClient (verify + list_inbox),
a pure INTERNALDATE parser and envelope mapping, and a feature-gated
async-imap transport (native only; wasm never compiles it).
C1f: credential_store.rs — CredentialStore trait and a fail-closed default;
the platform keystore (AndroidKeyStore, per the SMS precedent) is the
follow-on that cannot be host-tested.
C3: email_cache.rs — a local mail cache whose bodies are pushed through a
BodyCipher before hitting disk; PlaintextBodyCipher is the honest default
until C1f lands a real key.
C6: email_pacing.rs — SendRateLimiter + SendPacing ported from robius-sms
with email-shaped limits (100/hour); fixes a zero-capacity panic in the port.
C5/C4b plumbing: email_session.rs (shared signed-in account), InboxFetched
action, spawn_fetch_inbox and spawn_send_message (backend-aware dispatch),
and EmailSendRequest::build_without_config for the proxy send path.
Domain tests 154 -> 193.
SetupDraft ties account identity to the backend choice and validates
them together (the direct backend must also have a usable SMTP server;
the proxy backend leaves SMTP fields unset). The setup form grows a
chooser -- Direct (IMAP + SMTP) vs the Nigig mail service -- with an
honest per-backend summary, and two forms swapped by the chooser. The
inbox branches: proxy accounts verify against the mail service via
spawn_proxy_verify instead of an SMTP handshake.
Also pins the Proton Bridge provider default (local bridge, not a
remote imap.protonmail.ch).
The proxy backend's network half: a thin client over a ProxyTransport
trait (reqwest on native, fetch on wasm, mock in tests). verify,
list_inbox and send build typed requests and map status+body onto
structured errors, so the parsing logic -- where the bugs live -- is
host-tested without a server. The token rides in an Authorization
header and never in a request type that can be Debug-printed.
Also: spawn_proxy_verify posts a ProxyVerifyResult action, and
build_transport is pinned to construct for every port (A2/A3).
You chose to support IMAP-on-device AND a server-side proxy, user
selectable. This is the seam that makes that contained rather than two
parallel apps.
Why it is cheaper than it sounds: wasm cannot open a raw TCP socket, so a
proxy always had to exist for the browser target. The second backend was
never optional -- it was implied scope nobody had named.
C1a, mail_backend.rs:
BackendKind { ImapSmtp, ProxyApi } with three predicates that exist so
the UI cannot get them wrong:
is_available_on_wasm() IMAP is raw TCP; a browser cannot open one,
so the chooser must not offer a dead option
stores_reusable_password() IMAP keeps a REUSABLE mailbox password on
the device. For most people that is the
password-reset channel for every other
account they own. A revocable proxy token
is strictly safer, and the chooser must say
so rather than presenting a free choice
summary() the honest one-liner, asserted by test to
actually mention "password" / "revoke"
BackendSettings is the PERSISTABLE half and carries no secret, exactly
as EmailAccount does for the password (S2). BackendDraft::validate
returns (settings, Secret) and reports every problem in one pass.
The trait is deliberately synchronous and tiny -- kind(), is_configured(),
describe(). Anything computable above the line (grouping, previews,
threading) is NOT a backend concern, which is why email_store did not
change at all. I/O stays in the free functions that already own the async
context, so this file is host-testable with no runtime.
ImapSmtpBackend exists with validation but no protocol client yet; that
is C1e and nothing here claims a connection works.
C1b: EmailAccount gained `backend: BackendSettings`, #[serde(default)] so
existing persisted accounts still load. A test asserts the serialised
account -- including the backend section -- contains neither the token nor
a field named password/token.
Provider defaults now fill IMAP too, so a Gmail user still fills one
field. Outlook is special-cased: its IMAP host is outlook.office365.com,
not imap.outlook.com, so the naive smtp->imap rewrite would produce a name
that does not resolve.
New gate, negative-tested both ways: stores_reusable_password() and
is_available_on_wasm() must exist, and the persisted settings structs must
not declare password/token/secret fields.
Domain tests 99 -> 126. Test floor 95 -> 120.
B1 was the live Critical from the assessment. The recipient field is
labelled "To (comma-separated)" and the worker did:
let to_mbox: Mailbox = to.parse()?; // ONE address
Mailbox parses a single address, so ANY comma-separated list failed with
"Invalid to: ..." -- the user got an error for doing exactly what the
placeholder told them to do. The tab named "Bulk" could reach exactly one
person. The crate's headline feature did not work.
B2: new email_send.rs, the seam the widget could not provide.
parse_recipients accepts commas, semicolons and newlines, because a user
pasting from a spreadsheet or a mail client produces any of them. It
understands `Name <addr>` including a quoted name containing a comma --
"Doe, Jane" <jane@x.com> -- which a naive split(',') breaks in half and
which is the normal shape when pasting a To: header.
Partial failure does not fail the batch: bad entries are rejected with a
reason and the good ones still send. Failing everything because one
address had a typo is what made the directory CSV importer unusable.
Duplicates are collapsed case-insensitively. Sending one person two
copies of the same message is a bug that costs money and looks like
spam.
Addresses with control characters are rejected. lettre encodes headers
so this is defence in depth today -- but the C1d proxy backend will NOT
go through lettre (THREAT_MODEL T-E4), so the check belongs in the
domain layer, not the transport.
MAX_RECIPIENTS = 100. Not a protocol limit; providers cap RCPT TO per
message and exceeding it fails the WHOLE message rather than the excess,
so refusing locally with a number beats a provider error nobody can
decode.
B5: SEND_IN_FLIGHT, an AtomicBool swap. Both spawn_* functions used to
fire unconditionally, so a double tap sent the message twice --
irreversible, to a real person. Same control robius-sms uses, and it lives
in the domain layer so every entry point is covered rather than each page
remembering.
B6: partial, and named honestly. abandon_send() clears the guard and marks
the pending result stale so it cannot overwrite what the user does next.
It does NOT stop delivery: tokio's JoinHandle is not retained and lettre's
async send is not cancel-safe mid-transaction -- once DATA is accepted the
message is delivered whether we wait for the reply or not. Called
abandon_send rather than cancel_send for that reason; a function called
cancel that does not cancel is worse than no function. The 20s timeout
from A6 bounds the window.
Domain tests 72 -> 99.
Measured line coverage with llvm-cov rather than assuming it:
secret.rs 100.00%
email_account.rs 100.00% (was 95.42%)
email_store.rs 99.42%
email_worker.rs 70.16%
Four tests added to reach that:
every_error_variant_has_a_usable_message
AccountError::message() had uncovered match arms, which means a
validation could fire and show the user nothing. Also asserts the
messages are distinct -- if two errors share text the form cannot
say which field is wrong -- and that each is a sentence rather than
a token.
a_malformed_address_is_reported_as_malformed_not_missing
A present-but-wrong address takes a different path from a missing
one, and it is the path an actual typo takes.
states_without_an_account_return_none
SignedOut/Verifying must not hand the form a stale account.
an_empty_body_previews_as_empty_without_panicking
A whitespace-only body must still yield a row.
68 -> 72 tests.
On email_worker.rs staying at 70%: of its 80 uncovered lines, 30 are the
network layer -- smtp_test_impl, send_email_impl, build_transport and the
two spawn_* wrappers -- plus the whole #[cfg(target_arch = "wasm32")]
block, which cannot execute on Linux at all. Every PURE function in that
file is at 100%: is_incomplete, validate_send, config_warning,
tls_mode_for_port, email_api_url_is_safe.
Reaching 100% there needs a local SMTP sink, which is plan item E5. I am
not mocking Cx::post_action to inflate the number: that would test the
mock, not the send, and a coverage figure propped up by a fake is worse
than an honest 70% with the reason recorded.
Assessment finding S2, the one Critical in Phase A. SmtpConfig carried
#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct SmtpConfig { pub password: String, ... }
so on wasm the ENTIRE struct -- password included -- was serde_json
encoded and POSTed to /api/email. Every request carried the credential in
clear text, and any reverse proxy or APM tool logging request bodies
captured it. Nothing in the code said so.
A1. New `Secret` type (nigig-core/src/secret.rs):
* Debug always renders Secret("***"). No verbose mode.
* Display is NOT implemented, so format!("{s}") will not compile.
* Serialize/Deserialize are NOT implemented, and are REMOVED from
SmtpConfig. A struct holding a secret cannot be serialised wholesale;
the compiler stops it. That is the point -- a build failure rather
than a code review someone has to remember to perform.
* expose() is the only reader, named to be conspicuous in a diff.
The wasm request body is now assembled field by field, so `password`
appears at exactly ONE line and "what leaves the device?" is answerable
by reading one function instead of trusting a derive.
AccountDraft::validate now returns a Secret rather than a String, so the
plaintext never lands back in a UI-held field. The inbox widget's
session credential is a Secret too.
A2. build_transport uses lettre's own relay() for port 465 instead of
reassembling it from builder_dangerous + Tls::Wrapper.
To be clear, since I flagged this as critical and was wrong: relay() is
IMPLEMENTED as exactly those calls, and TlsParameters::new already sets
accept_invalid_certs: false, accept_invalid_hostnames: false and a TLS
1.2 floor. Certificate validation was always on. It is still worth
replacing -- a reviewer reading `builder_dangerous` assumes the worst (I
did), and hand-rolling inherits nothing if upstream hardens relay().
A3. TLS policy is now named and asserted rather than inherited:
tls_mode_for_port() maps every port to Implicit or StartTls, with NO
cleartext arm, and it is unit tested. Previously the policy lived in a
bare port match and a refactor could have removed encryption with no test
failing.
A4. validate_send() refuses locally what needs no server to know is
wrong: incomplete config, empty recipient, subject over the RFC 5322
998-byte limit, body over 5 MB. Also config_warning(), which flags a
from/username mismatch -- not an error, since some providers allow
send-as aliases, but it is the commonest cause of a silent rejection.
A5. set_email_api_url() now validates. It accepted any String, including
http://, which sends the credential in clear text. Now same-origin
relative or https:// only -- and it rejects protocol-relative //host/path,
which is http on an http page and is easy to mistake for a relative path.
Split into email_api_url_is_safe() so it is host-testable; the wasm target
cannot run cargo test here, and an unvalidated validator is not a control.
A6 (partial). SMTP timeout cut from lettre's 60s-per-command default to
20s. A mobile user on a bad connection needs an error, not a two-minute
stall.
Domain tests 38 -> 68.
One thing I will not overclaim: `Secret` does NOT zero its buffer on
drop. Without a zeroize-style crate the plaintext can persist in freed
heap memory. It is a leak-through-code control, not an anti-forensics
one, and it is recorded as an open risk rather than papered over.
The repo has a CI gate requiring full-length revs, added deliberately in
5e71457 with a comment explaining that an abbreviated rev resolves only
while no other object shares its prefix -- a property of the repository's
current object count, not a guarantee. Git's abbreviation length grows as
a repo grows, so a short pin silently becomes ambiguous, and an attacker
able to push to the fork can try to manufacture a colliding prefix.
That gate has been failing. 42 declarations across 34 crates used
abbreviated revs:
41x rev = "ecf5a572" (the current makepad pin)
1x rev = "5efe6e24c" (map/tests/makepad_test_app, left behind
by the ce0eaae bump)
Resolved both against the remote and rewrote them:
ecf5a572 -> ecf5a572ab62a1c1598909971f602f99083671cc
5efe6e24c -> 5efe6e24c9f732e9f11b783757f196f4f1c402b2
Verified this changes the LABEL and not the dependency: Cargo.lock holds
exactly one makepad commit id and zero references to the old one, so
nothing was silently upgraded. The stray makepad_test_app pin did move to
the current rev, which is the intent -- it pointed at a stale branch head.
Cargo.lock also picks up unrelated churn (brotli et al in,
makepad-android-state/jni-sys out). That staleness is PRE-EXISTING, not
caused by this change: confirmed by stashing every edit and running
`cargo metadata` on a pristine tree, which produces the identical diff.
Gate now passes:
$ grep -rn 'rev = ' --include=Cargo.toml . | grep -vE 'rev = "[0-9a-f]{40}"'
(no output)
86c9595 synced the fork to upstream/dev at abd70f4, which dropped three
fork-local optional dependencies from widgets/Cargo.toml and their
re-exports from lib.rs. They were fork additions, so the merge lost them.
Every Makepad UI target then failed to resolve:
package `nigig-pdf-makepad` depends on `makepad-widgets` with feature
`test` but `makepad-widgets` does not have that feature.
help: available features: default, serde
failed to select a version for `makepad-widgets`
The "available features" list is misleading: with no `test` feature on
widgets 2.0.0, cargo falls back to the stale old/widgets copy, which is
1.0.0 and offers only default and serde. Same fallback that produced the
bogus makepad-fonts-chinese-bold error in an earlier sync.
libs/makepad_test was never removed - only the manifest entries and the
re-export. The fork's ecf5a572 restores both. This bumps all 34 crates.
Verified against the real fork, not a local copy:
TEST_TARGET=pdf-ui 682 passing (was: failed to resolve)
TEST_TARGET=pdf 637 passing
Pin bump only: every hunk changes the rev and nothing else.
Groundwork for an inbox that shows mail instead of a placeholder. Both
modules are transport-free and Makepad-free so they run in CI on Linux,
where the whole SMTP path is untestable -- the same reasoning that put
BulkSendRequest::validate and SendPacing in robius-sms rather than in a
page widget.
email_account.rs -- identity and session state.
SessionState is what the UI reads to choose between the inbox and the
setup form: SignedOut / Verifying / SignedIn / Failed. Failed carries
the account so the form can be repopulated instead of making the user
retype six fields to fix one.
AccountDraft::validate returns EVERY error, not the first, so the form
marks all bad fields in one pass.
Two deliberate choices:
- EmailAccount has NO password field. It is the persistable half; the
secret is returned separately and held in memory by the caller. This
is assessment finding S2 -- SmtpConfig derives Serialize with a
plaintext password, so anything reusing it for storage leaks. A test
asserts the serialised account contains neither the password nor a
field named "password", so a future field addition trips it.
- A malformed port is an ERROR, not a silent default. The existing code
does `parse().unwrap_or(587)`, so "465x" silently becomes 587 and
thereby silently changes the transport (finding B3). Empty still
means default; garbage now says so.
guess_provider fills SMTP settings for the seven common consumer
domains, which is why the form is one field for a Gmail user. It
returns None for unknown domains rather than guessing smtp.<domain> --
that heuristic is right often enough to look like a feature and wrong
often enough to produce confusing failures.
email_store.rs -- messages grouped into per-sender threads.
group_by_sender / thread_for_sender give the inbox the same shape the
SMS inbox has: rows keyed by sender, a timeline per row. Grouping is
case-insensitive, because Alerts@Bank.co.ke and alerts@bank.co.ke are
one sender and two rows is the email version of the SMS duplicate-
recipient bug. Sort ties break on address so HashMap iteration order
cannot leak into the UI and reshuffle rows between frames.
preview_line uses char_indices, not `&body[..n]`. That is bug A3 in
the SMS crate -- one inbound message with emoji or non-Latin text
panicked the list on every frame -- and there is a CI gate forbidding
byte-offset slicing in SMS text helpers for exactly this reason. Email
bodies are equally untrusted. Tested against emoji, Swahili, Arabic,
Japanese and deliberately misaligned mixed text, which is the case
that actually triggers A3 (uniform emoji happens to land on a
boundary).
sample_thread() is explicit development data. There is still no
receive path (finding A1) and the IMAP-vs-proxy decision is open, so
without it the list can only render an empty state and the
list/thread transition cannot be exercised at all. Named sample_ so
it is obvious in a diff when a real fetch replaces it.
38 tests, all passing. Verified against 2faadb7 -- origin/main currently
does not resolve because the makepad bump in 86c9595 dropped the `maps`
feature that pageflipnav requires. That break is pre-existing and
unrelated; confirmed by stashing these changes and reproducing it on a
pristine tree.
Updated makepad fork to include all latest APIs needed by map widget:
- pack_vector_vertices and VECTOR_PACKED_FLOATS_PER_VERTEX
- TileArchiveReader for MKMap archive support
- get_tile_decoded method on MbtilesReader
- set_trust_fill_winding and fill_fringe_into on Tessellator
- retain_queued method on TagThreadPool
- set_camera_delta method on DrawRotatedText
This resolves all compilation errors in the map widget code.
All 35 Cargo.toml pins move from d82756a to 5efe6e24c on the gitdab fork
(portallist base + makepad_test Android adb / standalone terminal wiring).
Lockfile regenerated; pdf crates compile against the new rev.
Updated makepad fork to d82756a which includes latest map improvements:
- Baked fills/faces support
- Enhanced 3D building rendering
- Improved road geometry and elevation
- Better theme matching and styling
Removed tile_makepad.rs (12k+ lines) and reverted to using makepad-widgets
map functionality directly. This avoids maintaining a separate copy and
ensures we get all upstream improvements automatically.
Changes:
- Updated all Cargo.toml files to use makepad fork d82756a
- Removed crates/apps/map/src/tile_makepad.rs
- Removed tile_makepad module from lib.rs
- Reverted tile_disk.rs to use mbtiles_tile_to_overpass_response
50 tests -> 102, and the two that were there at the start of this work
are deleted.
Where this started: robius-sms had ZERO tests, and nigig-sms had two --
bulk_sub_tab_default_is_contacts and bulk_sub_tab_variants_distinct.
Both asserted a derived Default and a derived PartialEq. Neither
mentioned SMS. Neither could fail short of the compiler breaking. That
is the defect that produced every other defect in this plan: nothing
could prove a change was safe, so nothing was ever deleted and every
bug survived contact with review.
Property tests (proptest, new dev-dependency)
Seven over truncate_preview, format_timestamp, badge_text, and five
more over segment_count, the rate limiter and ScheduleRequest.
These are the ones that matter, because the hand-written cases in this
repo all encode a bug someone had ALREADY found. proptest searches the
space instead. I verified that by reinstating the original byte-slicing
truncate_preview and confirming
prop_truncate_preview_survives_mixed_scripts and
prop_truncate_preview_respects_the_char_limit both fail against it --
they would have caught A3 before it shipped.
prop_rate_limiter_respects_capacity models the window independently
and asserts the invariant across random clock sequences, rather than
re-implementing the limiter's own arithmetic in the assertion.
Integration tests (2 new files, public API only)
robius-sms/tests/sms_pipeline.rs and nigig-core/tests/sms_store.rs go
through the public surface the application actually uses. The unit
tests inside src/ can see private helpers; these cannot, which is the
point -- they catch a refactor that keeps every unit test green while
breaking the caller-visible contract.
Two of them are privacy canaries. e1_message_bodies_are_never_persisted
and e1_no_body_text_reaches_the_serialised_store fail if anyone removes
#[serde(skip)] from OfflineSmsMessage.body. Verified by removing it:
both fail, the other six pass. Nothing else in the tree would have
noticed the inbox silently going back to plaintext on disk.
Named regression tests
One per defect, named for it -- c1_*, d1_*, d3_*, e1_*, e7_*, a4_*,
c3_*, c7_* -- so a future reader goes from a failing test straight to
the bug it guards rather than to a git archaeology session.
New coverage for logic that had none
- build_timeline_items / build_filtered_timeline_items: date-divider
placement and the message indices the draw loop uses to index
conv_data.messages. An off-by-one there renders the wrong body in
the wrong bubble; it had no test at all.
- kind_to_offline / kind_from_offline round-trip: the only thing
stopping a cached Sent message reappearing as Inbox after a restart,
which would flip the bubble to the wrong side of the screen.
- normalize_number: what C1 groups on, across five formatting variants
plus short codes and alphanumeric senders.
MessageKind::from_android_type / to_android_type were hoisted out of
sys/android/inbox.rs onto the type, the same way ScheduleRequest::validate
was in A4, so the provider mapping is testable off-device. An
unrecognised TYPE value is preserved verbatim in Unknown rather than
defaulted, and there is a property test asserting the round trip is
total over every i32.
CI: a test-count FLOOR at 100. A floor rather than a ratchet -- unlike
the clippy count, there is no reason to ever want this number to fall.
Deliberately NOT faked: the JNI cursor loop, the keystore round-trip and
broadcast delivery still need an emulator. A mock returning what I expect
would test my expectations, not Android. Those remain called out in the
Phase A and E commit messages.
Verified: 11/11 checks. 48 robius-sms + 46 nigig-sms + 8 sms_store = 102.
clippy -D warnings clean on host and aarch64-linux-android; nigig-sms
ratchet holds at 32 (my first draft added an orphaned `use super::*`,
caught by the ratchet and removed rather than baselined).
E1 -- the inbox was written to disk in plaintext.
offline_store wrote every SMS body to
app_data_dir/offline_store/sms_messages.json as pretty-printed JSON.
SMS is the transport for OTPs, banking codes and M-Pesa
confirmations, so that file was the user's complete authentication
history sitting in app-private storage -- readable by anything running
as the same UID, and included in backups.
This repository already knew the answer. THREAT_MODEL.md T-I2 records
"Raw SMS persisted to PSV file" as fixed in Phase 0, with raw_message
omitted from save_to_disk() so it "lives in memory only". The SMS app
then re-introduced the same defect at larger scale: the entire inbox
rather than just M-Pesa messages, and with no retention limit until
D6.
OfflineSmsMessage.body is now #[serde(skip)]. Dropping the field
rather than encrypting it is the deliberate choice: every consumer
already reads the device provider FIRST and writes the cache second
(nigig-sms fetch_from_device, and the mpesa and pay transaction
pages), so the provider is the system of record and no body needs to
survive a restart. Encryption would keep the plaintext reachable to
anything holding the key. Not writing it removes the asset.
Two consequences handled: sms_key() no longer hashes the body, since
a reloaded row has an empty one and dedupe would otherwise never match
its own cached entry and grow a duplicate per refresh; and the cached
first paint shows a neutral placeholder rather than a blank preview
for the instant before the provider read lands.
E9 -- the Linux backend's dependencies were pure cost.
robius-sms declared polkit =0.17.0 and gio =0.17.0 for target_os
= "linux". sys/linux.rs references neither: all twelve functions
return Err(PermanentlyUnavailable). Those two crates dragged in glib
and proc-macro-error and were the origin of RUSTSEC-2024-0370 and
RUSTSEC-2024-0429 for every consumer of this crate.
Deleting the block removes 340 lines from Cargo.lock. polkit, gio,
glib and proc-macro-error no longer appear in the workspace at all,
which also closes the LGPL-2.1 linkage question outright rather than
routing around it as Phase B did for nigig-build alone.
E4 -- ROBIUS_SMS_BOOT_LIB was a code-injection vector.
build.rs interpolated that environment variable straight into a Java
string literal, which is then compiled, dexed and loaded at runtime
with the app's full permissions. A value containing a quote closes the
literal and injects arbitrary Java that runs on the device at boot.
Build-time environment is not trusted input. Now validated against
[A-Za-z0-9_]+ and the build fails loudly otherwise. Tested both ways:
an exec payload is rejected, a legitimate name builds.
E5 -- undefined behaviour in the dex loader.
new_direct_byte_buffer was handed RECEIVER_BYTECODE.as_ptr() as
*mut u8 -- a &'static [u8] in .rodata cast to a mutable pointer, when
the API is documented as taking writable memory and
InMemoryDexClassLoader may write through it. Now copies into an owned
allocation and leaks it, which is correct rather than lazy: the buffer
backs a ClassLoader cached in a OnceLock for the process lifetime.
E6 -- two bindings for one native method.
rustRestoreSchedules was both exported #[no_mangle] and registered
dynamically via register_native_methods. Which one won was
unspecified. Kept the dynamic one, because the class is loaded from an
in-memory dex and is not on the JVM's search path, so symbol binding
is not guaranteed to find it.
E8 -- no send rate limiting.
Nothing capped send rate, and the bulk UI exists to blast a scraped
directory. Android's practical throttle is ~30 messages per 30 minutes
per app, past which sends are silently dropped -- so an unthrottled
batch both overspends and fails opaquely. Adds SendRateLimiter, a pure
token bucket taking an explicit clock so it is unit-testable without
sleeping, wired into the bulk sender. A 200-recipient blast now stops
at 30 and says why.
E10 -- robius-sms carried no license field, so cargo-deny needed a
[[licenses.clarify]] override asserting one. Stated in the manifest;
override removed.
E2 was already satisfied by D6 (retention capped at 5,000).
E7/E11 are documented rather than fixed, which is the honest status:
delivery confirmation needs real PendingIntents plumbed through
(A8 documents that Ok != delivered), and sender validation cannot be
solved client-side. Both are now rows in THREAT_MODEL.md instead of
findings in a markdown report -- along with T-I2b for E1 and T-I4 for
E3, which is NOT done: scheduled message bodies are still plaintext in
SharedPreferences.
CI: adds a gate asserting OfflineSmsMessage.body keeps #[serde(skip)].
Removing that attribute silently resumes writing plaintext and nothing
else would fail. Negative-tested.
deny-nigig-build.toml drops to 2 exemptions (from 5 before Phase B).
Tests: robius-sms 21 -> 25.
Verified: 12/12 CI jobs, clippy -D warnings clean on host and
aarch64-linux-android, cargo deny "advisories ok, bans ok, licenses ok,
sources ok", clippy ratchet holds at 49.
D1 -- the SMS read blocked the render thread.
fetch_from_device() called robius_sms::list_messages() directly from
draw_walk. That is a blocking cross-process ContentProvider query over
Binder, plus ~10 JNI calls per message row, plus a full rewrite of the
offline JSON store. On a populated inbox it is a multi-hundred-
millisecond stall, taken on the render thread.
robius_android_env::with_activity() calls
attach_current_thread_permanently() and ContentResolver.query is
thread-safe, so the read is safe off the UI thread. It now runs on a
worker and hands back through a results queue drained on Event::Signal
-- the same shape the contact lookup in this file already used.
D2 -- the app never idled.
draw_walk ran a 5-second wall-clock refresh, and that refresh called
redraw(), which scheduled the next frame, which re-entered draw_walk.
A self-sustaining loop, running whether or not the SMS tab was even
visible, each cycle paying D1's cost plus D3's clone.
Refresh is now event-driven: first load, Event::Resume,
pull-to-refresh, and the permission-granted callback. NOT yet a
ContentObserver on content://sms -- that is the remaining half of D2
and is called out in the code. Until it lands, a message arriving
while the app is open appears on the next Resume or pull rather than
within 5s. That is a deliberate trade: a bounded staleness window in
exchange for an app that can reach idle.
D3 -- a deep clone per refresh, purely to diff.
`let previous = self.conversations.clone()` copied every message in
every conversation on each cycle so the result could be compared for
equality. Replaced with a u64 digest over (count, address,
message_count, newest date_ms). Bodies are immutable once stored, so
that is sufficient to notice an insert, a delete or a new message --
and the test asserts exactly those three cases.
D4 -- ~900 pointless worker kicks per second.
start_contact_lookup_worker() was called once per visible row per
frame (~10-15 per frame, ~900/s at 60fps). Each call took 3-4 mutex
locks before early-returning, contending with the worker thread trying
to write results back. Now called once, after the draw pass. The
per-row code only enqueues.
D6 -- the offline store grew without bound.
upsert_sms_messages re-read, re-hashed, re-sorted and rewrote the
whole file on every call, with no cap -- while append_location() a few
lines below has always truncated to 512. Capped at MAX_CACHED_SMS
(5000), newest-first so truncate drops the oldest. This is a display
cache; the device provider stays the system of record.
D7 -- O(rows x selected) per frame in the bulk list.
get_selected_companies() returns a Vec and the draw path called
`selected.contains(..)` once per visible row, so "Select All" over the
Nairobi directory made every frame quadratic. Now a HashSet. Same fix
in sync_selected_to_recipients, whose phone de-duplication was also
O(n^2) via `phones.contains()`.
Refactor note: group_into_conversations() and conversations_digest_of()
were lifted out as free functions. ConversationsList holds a Makepad
View and is not Default, so nothing in it could be unit tested; the
logic D1/D3 depend on now can be.
CI: adds a gate rejecting blocking robius_sms provider calls inside any
draw_walk. Negative-tested by reinserting the call and confirming it
fails.
Tests: nigig-sms 19 -> 22, nigig-core +1.
Verified: clippy ratchet holds at 49, clippy -D warnings clean on host
and aarch64-linux-android, nigig-build still builds.
NOT measured. The plan asked for before/after frame timings on a 5,000
message fixture and this sandbox has no device or emulator, so the
figures above are reasoned from the code, not profiled. D5 (bulk send
still blocks the UI thread) is untouched and needs the same worker
treatment as D1.
Pre-existing and unrelated: nigig-core's pending_tx_lifecycle test fails
identically on pristine origin/main (wall-clock assumption in an M-Pesa
expiry test).
nigig-build declared robius-sms and robius-location and called neither:
zero references to robius_sms or robius_location anywhere under
nigig-build/src. That dead dependency pulled in
robius-sms -> polkit -> gio -> glib -> glib-macros -> proc-macro-error
which is the ONLY reason deny-nigig-build.toml carried
RUSTSEC-2024-0370 (proc-macro-error, unmaintained)
RUSTSEC-2024-0429 (glib VariantStrIter unsoundness)
plus an unanswered LGPL-2.1 distribution question about linking polkit
into a shipped mobile binary -- all of it for code that never ran.
Removing the direct dependency alone was NOT enough, which is the part
worth recording. `cargo tree -p nigig-build -i polkit` showed three
paths, not one: the direct declaration, and two more through nigig-core
and nigig-uikit. Both of those also declare robius-sms and also never
use it. All three declarations had to go before polkit left the graph.
nigig-core keeps robius-location: unlike the others it genuinely uses it,
in src/location.rs.
Verified, not assumed:
- before: cargo tree -p nigig-build -i polkit resolved, three paths
- after: polkit, gio and proc-macro-error no longer resolve at all
- cargo deny check -> "advisories ok, bans ok, licenses ok, sources ok"
with two fewer ignores (5 -> 3)
- nigig-build, nigig-core, nigig-uikit and nigig-sms all still compile
- Cargo.lock loses 5 lines
Also in this commit:
- A CI gate so the declarations cannot come back. Deliberately scoped
to robius-sms/robius-location on these three manifests rather than a
blanket `cargo machete`: five other unused dependencies exist here
(chrono, futures, postcard, rand, serde_json) and a gate that is red
on its first run gets switched off. Negative-tested by re-adding the
dependency and confirming the gate fails.
- Three pages carried the same placeholder string telling the user
they were looking at a "RobrixStackNavigationView destination ...
just like SMS conversation screens". That is user-visible UI copy,
not a comment. Replaced with text describing the page. The identical
"This follows the SMS/Home pattern" comment in the same three files
now says what the code does instead of naming another module.
Note the scope limit: 26 of the 29 crates in this workspace declare
robius-sms and never use it. This commit fixes the three that put
advisories on nigig-build. The rest are the same latent problem and
should be swept separately.
Upstream adopted the makepad fork fix (8fdff3f) but pinned it as
`rev = "a79f0dc"` -- a 7-character abbreviation. The pinning gate passed,
because it only checked that `rev = ` was present at all. Its own error
message has said "Add rev = \"<full-40-char-sha>\"" since it was written,
without ever enforcing it.
An abbreviated rev resolves only while no other object in the repository
shares its prefix. That is a property of the current object count, not a
guarantee -- it is why git's own auto-abbreviation length grows with a
repo. A short pin therefore degrades on its own over time, and someone
who can push to the fork can attempt to manufacture a colliding prefix.
For a dependency that executes at build time, that is a supply-chain
weakness rather than a style preference.
Checked before assuming: a79f0dc currently resolves uniquely in the fork
(exactly one matching object), so nothing is broken today. This closes it
while it is still cheap.
- All 34 manifests expanded to the full SHA
a79f0dce4d477e2232344facca0798d3f25043ec. Cargo.lock is unchanged by
the expansion, confirming it is the same commit and purely notational.
- The gate now also rejects any rev that is not exactly 40 hex chars.
Negative-tested: restoring the 7-char form makes it fire.
685 lib tests pass; all nine gates pass.
Commit a79f0dc fixes the duplicate dependency declarations that were
causing TOML parsing errors. This is the correct commit to use after
the parallel fixes in 5eda8056 and 11375214.
All 34 Cargo.toml files updated to reference the correct commit.
The repository has been uncompilable since the d6d1f99c fork bump. Root
cause was in gitdab.com/andodeki/makepad, not here, so the fix went there
first: commit 5eda8056 on portallist_flow_adaptive_view.
TWO defects, both introduced by the "Update fork to upstream dev 5d4483f"
merge, both pure losses rather than intentional changes:
1. widgets/Cargo.toml: the makepad-gltf / makepad-csg / makepad-test
dependency lines were relocated from [dependencies] to below
[features]. Cargo then parses each as a feature whose value should be
an array, giving "invalid type: map, expected a sequence", and the
gltf/csg/test/maps features cease to exist.
2. widgets/src/lib.rs: the feature-gated re-export block for those same
crates (plus makepad_fast_inflate and makepad_mbtile_reader) was
deleted outright. Fixing only the manifest surfaced this as
"no `makepad_csg` in the root".
Both restored verbatim from 2c5cd97, the last rev that resolved. Neither
is a judgement call: the moved lines are byte-identical and the deleted
block is copied back unchanged.
This repo is then repinned from d6d1f99c to the fixed rev, full 40-char
SHA per the pinning convention CI enforces.
Verified end to end after removing the local git redirect used during
development, so this resolves against the real remote:
cargo metadata resolves
nigig-build --lib 685 passed
cad_integration 154 passed
spreadsheet-engine 225 passed
doc-engine 53 passed
nigig-map (maps feature) compiles
Cargo.lock unchanged, --locked passes
Also resolved committed conflict markers in two workflow files, which
had made nigig-build.yml invalid YAML -- the CI config could not be
parsed at all:
- nigig-build.yml: kept --include='*.rs' on the by-value-getter gate.
Without it the gate scans ARCHITECTURE.md and fails on its own
documentation, which is the bug fixed in 4f32b1c.
- pdf.yml: kept upstream's side. Enumerating targets via
`cargo fuzz list` and failing when the list is empty is strictly
better than a hardcoded target list that silently passes vacuously if
a target is renamed.
That makes four files in three commits now carrying committed conflict
markers from this merge. Worth checking how they are reaching main --
`git diff --check` catches exactly this and is already a step in the
nigig-build workflow, but it only runs on paths under that workflow's
filter.
- Fixed TOML parsing error where fork-specific dependencies were in wrong section
- Dependencies now correctly placed in [dependencies] before [features]
- Maps feature should now be properly recognized
Updated all makepad dependencies from rev 2c5cd97 to 817d881 which includes:
- Terrain hillshade landcover draping (drape.rs)
- Route overlays, markers, and position puck (overlay.rs)
- Map icon management system (icons.rs + 50 SVG icons)
- 3D road elevation and seamless joins
- Building shadow geometry and terrain shadows
- Night themes and emissive roads
- Water, grass, and shrub rendering
- Optimized road geometry with 2D/3D mode transitions
- i_overlay library for polygon boolean operations
This brings nigig-map in sync with the latest makepad dev branch improvements.
B6 is the last unfinished P0 on the review's priority table.
## Why the boundary and not the format
The obvious reading of B6 is "change f64 to Money on disk", which is a data
migration and belongs with the SQLite move. But measuring first shows the
stored representation is not where the damage is:
- The f64 round trip is exact. 1500.0, 1500.5, 0.1+0.2, 1e20 and
12345678.995 all write and re-read bit-identically.
- The read path is the damage. parts[2].parse().unwrap_or(0.0) turned any
unreadable amount into a confident KSh 0 row.
- And NaN gets in. "NaN" and "inf" both parse as f64 and the writer emits
them back verbatim, so they survive a round trip. One corrupt SMS then
poisons every total it enters, permanently — once a NaN is in a sum,
every comparison against that sum is false.
## What changed
validate_money refuses three classes, on parse, on load and on save:
non-finite; negative (direction lives in TransactionType, so a negative
amount is a contradiction); and beyond 2^53, where f64 can no longer
represent consecutive shillings.
A row whose amount cannot be trusted is skipped with a log line rather than
zeroed. Dropping a row is visible and recoverable by rescanning the inbox;
a silent KSh 0 is neither. balance and cost are optional context, so they
degrade to None rather than discarding the row, but can no longer be NaN.
The writer refuses to persist an invalid amount, which is what stops a
poisoned value becoming permanent.
Smaller parse fix: "Ksh ,5" used to read as 5. A separator before any digit
means the text is not the expected shape, and guessing is worse than
declining.
## The parser had no tests
parser.rs — the file deciding what every observed amount is — had zero
tests. It now has 13: validation, extraction, end-to-end parsing, and
unicode/NUL bodies that must not panic on a byte-index slice.
M-Pesa harness: 7 -> 24 tests.
## Verifying the tests can fail
validate_money was reduced to Some(value) and the harness re-run: 4 tests
failed. A guard that cannot fail is decoration.
## Validation
mpesa harness: 24 tests pass
domain : 137 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 / batch / settlement-tick guards pass
defect injection: 4 tests fail with validation removed pass
## What B6 still leaves open
MpesaTransaction::amount is still f64 on disk. That is deliberate and now
bounded: nothing untrustworthy can enter or leave the store, so the
remaining exposure is precision within the validated range, which for
whole-shilling amounts under 2^53 is none. Converting the field means
rewriting the PSV format and migrating existing files. ADR 0002 records B6
as partially addressed with the migration deferred to the SQLite move.
Continues Phase 6 (REVIEWS/adr/0008) and addresses defect B6.
## Two labels were still claiming settlement
Tranche 7 noted the sheet's remaining status strings were composed
inline. Two of them were not untidiness, they were the Phase 6 violation
still shipping:
ObservedEvidence -> "✓ SMS received — ref: {code} …"
BulkItemObserved -> "✓ {current}/{total} sent"
A tick is a settlement claim. The first put one beside an SMS, which
proves nothing and can be forged. The second counted dispatches as
confirmations, so five unknown outcomes rendered as "✓ 5/5 sent".
FlowStage now owns the vocabulary next to the presentation types, with
one rule: a tick requires a provider confirmation. Evidence reads
"Message received (ref X) — not yet confirmed. Do not send again until
this is resolved." A finished batch reads "Batch finished — N sent,
awaiting confirmation", which is what actually happened.
Nine call sites derive their text instead of composing it. The two
remaining ✓ in that file are fingerprint-sensor feedback — a real local
event, not a settlement claim.
A CI guard backs this, and it was tested against the old code to confirm
it fails when the regression returns. A guard that cannot fail is
decoration.
## B6, scoped to where it is real
Each f64 site was measured before changing it, because "replace f64
everywhere" is easy to claim and easy to get wrong.
format_amount was checked exhaustively from 0.00 to 2000.00 against an
exact integer reference: zero mismatches. It also rounds 1234.567 and
2.675 correctly. It is not the defect and rewriting it would be churn.
Accumulation is the defect. 10,000 realistic amounts showed no visible
drift, but f64 silently discards additions past 2^53 and offers no
overflow signal at all, so a corrupt record yields a confident wrong
total with nothing to indicate it.
So the fix went to the accumulators. update_summary in nigig-mpesa and
nigig-pay — exact duplicates of each other, review item A5 — now build a
PeriodSummary from exact minor units with checked arithmetic, and render
"—" rather than a wrapped figure when overflowed is set. The conversion
from stored f64 is explicit and validated.
store.rs::totals() has no callers; it is now #[deprecated] with the
reason rather than deleted, since nigig-core has no domain dependency and
that is a separate change.
Two regression tests state it executably: f64 silently swallows 2^53 + 1
while the exact path keeps both entries, and an impossible total is
reported rather than shown.
## Validation
domain : 104 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
settlement-tick guard: verified to fail on the old code pass
Domain tests 95 -> 104.
## Not claimed complete
The thread_local PayFlowHandler still exists. The status vocabulary is now
domain-owned, which was the part carrying correctness risk, but the
widgets still do not talk to PaymentCoordinator. That remains the Phase 6
architectural item. The parser's stored f64 amount/balance/cost are
unchanged: that is a migration, not an edit, and belongs with SQLite.
Phases 0-2 of CAD_ASSESSMENT_AND_PLAN.md. The crate did not compile and no
test had ever run; it now builds clean with a green suite.
Build and CI (Phase 0)
- Pin all 33 git dependency manifests to an explicit rev. A branch
dependency re-resolves on every build and is a code-execution path into
CI if force-pushed.
- Commit Cargo.lock (540 packages). Producing it required fixing three
resolution failures the workspace had always had: a non-existent
makepad-widgets feature, two rusqlite versions both linking sqlite3, and
four missed CellId call sites in spreadsheet-ui.
- Add .forgejo/workflows/nigig-build.yml.
- Replace five stale CAD docs that contradicted the code with one
ARCHITECTURE.md; add PHASE0/1/2_STATUS.md and TEST_BASELINE.md.
Correctness (Phase 1)
- Rotation units: transform_point bound sin_cos() backwards, transposed X
and Z, and applied axes in reverse order, so every exported STL was wrong
even at zero rotation. It now shares the renderer's matrix helpers.
- GLB quaternions had norm 0.125 (half-angle applied to cos/sin, degrees
read as radians) - invalid per the glTF spec.
- PDF wall/door/window yaw fed degrees to cos/sin.
- Fix a TOCTOU unwrap in touch picking; viewport.rs now has no unwrap().
- CommandContext gains update_node/insert_node_at/node_index: resize and
modify were delete+create, silently moving nodes to the end of the scene.
- Wire MAX_UNDO_LEVELS (defined, exported, never read) and switch the undo
stack to VecDeque; this also made the existing drag-merge logic reachable.
- CadNode::size() returned a fake 1x1x1 for CSG and extruded solids, making
them unpickable outside a 1x1x1 box at their origin.
- Reject non-finite script input; makepad_csg clamps NaN rather than
propagating it, so bad input produced silently wrong geometry.
Test baseline: 0 -> 722 passing, 0 failing
- 17 pre-existing failures fixed: 10 real defects (dependency-cycle
detection, over-allocation of unassigned tasks, quote/backslash
corruption on save, default rooms lost for all but the first region,
RGA text ordering) and 7 tests that were themselves wrong, each checked
against its production caller first.
Security (Phase 2)
- env!("CARGO_MANIFEST_DIR") was used as a runtime path in three places,
including as the AI agent's working directory. All runtime data now goes
under app_data_dir().
- Remove the hardcoded LAN LLM endpoint. It is now opt-in via
NIGIG_CAD_LOCAL_OPENAI_URL/_MODEL and refuses plaintext HTTP to anything
but loopback.
- Bound and content-sniff AI image attachments (8 MB cap, magic bytes);
the MIME type came from the filename extension.
- Escape SVG/HTML output, and add SRI to the exported viewer's script tag.
The pinned model-viewer@3.5.1 does not exist, so every exported viewer
was silently broken; now 4.0.0 with a verified hash.
- Stop embedding $USER in exported PDFs and logging document content in
release builds.
- CI now rejects reintroducing the runtime-path and hardcoded-endpoint
classes; both gates were verified to fail on a reintroduced defect.
Add system_prompt.md and embed it with include_str!. The file was missing
from the repository, so the agent silently used a one-line fallback.
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.
