Two defects in my own Phase 0 work, both found by re-checking the
plan's task list against what actually shipped.
1. Task 0.7 was never done. The previous commit's subject line claimed
"Phase 0.2-0.7" but no lint attribute was ever added. sms_utils.rs
now carries module-level
#![warn(clippy::indexing_slicing)]
#![warn(clippy::string_slice)]
This module formats untrusted text -- SMS bodies straight off the
wire -- and every function in it runs inside draw_walk, once per
visible row per frame.
warn rather than deny: the two known sites are still present and
deny would make the crate fail to build. The point is that a THIRD
site cannot appear silently. Raise to deny when A3 lands.
2. The byte-offset slice gate had a false negative. It required a
leading `&`:
&[A-Za-z_][A-Za-z0-9_]*\[\.\.[A-Za-z0-9_]+\]
truncate_preview() contains two slices one line apart:
193: if let Some(pos) = trimmed[..end].rfind(..) <- MISSED
196: format!("{}…", &trimmed[..end]) <- caught
Line 193 is autoref'd, panics identically, and being first is the
one that actually fires. The gate reported "1" and looked audited
while missing the instance that runs. Pattern no longer requires the
`&` and also covers `[n..]` and `[..=n]`; baseline 1 -> 2.
The clippy ratchet moves 50 -> 52: the two new diagnostics are exactly
the clippy::string_slice reports for those two lines, which is the
intended effect of 0.7 -- A3 is now visible in CI instead of only in a
markdown file.
Verified with the pinned toolchain (1.97.1):
gates: lock().unwrap() 19/19 PASS, byte-slice 2/2 PASS
robius-sms: clippy -D warnings PASS, test PASS
nigig-sms: check PASS, test 2 passed, clippy ratchet 52/52 PASS
supply-chain: metadata --locked PASS, lock unchanged, whitespace PASS
repo-hygiene: markers PASS, workflow YAML PASS, 40-char SHA PASS
The SMS stack had no CI of any kind. crates/robius-sms (2,019 LOC)
and crates/apps/nigig-sms (5,706 LOC) shipped with two tests, both of
which assert derived trait impls (BulkSubTab::default() and a
PartialEq) and neither of which touches SMS. That is 7,725 lines with
no automated signal, for a feature whose job is to send real, billable
messages.
Five jobs:
gates Two source scans, both for defect classes already
present here and both invisible in review.
robius-sms check + clippy -D warnings + test, host.
android The job that matters. Everything in sys/android/ --
all ~600 lines of JNI, every function this crate
actually performs on a device -- is behind
#[cfg(target_os = "android")] and is not compiled by
any host job. The three clippy errors fixed in the
preceding commit were invisible until this job
existed.
nigig-sms check + test + a scoped clippy ratchet.
supply-chain --locked lockfile, whitespace. Only enforceable
because the preceding commit tracked Cargo.lock.
The android job pins JDK 17 via setup-java. build.rs compiles two
.java files and dexes them with d8, and d8 (build-tools 34) rejects
class file major version > 61. A runner defaulting to JDK 21 fails
inside d8 with "Unsupported class file major version 65", which reads
like a toolchain bug rather than a JDK mismatch. Reproduced locally on
21, green on 17.
Three steps are RATCHETS against a recorded baseline rather than hard
gates, because each has pre-existing violations whose fixes belong to
later phases:
.lock().unwrap() 19 lines. Poisoning one mutex bricks contact
resolution for the process lifetime.
byte-offset slicing 1. truncate_preview() panics on any inbound
SMS containing emoji or non-Latin text, per
visible row per frame.
nigig-sms clippy 50. All mechanical, but `clippy --fix`
cannot apply them through Makepad's
script_mod! proc macro, so they need hand
edits. 34 of the 50 are dead-code reports
for the duplicated contact subsystem in
inbox/sms_screen.rs.
A ratchet fails if the count RISES, and also if it FALLS without the
baseline being lowered, so progress cannot be silently undone. The
alternative -- a hard gate red on its first run -- gets switched off,
which is the reasoning already recorded in doc-engine.yml for not
gating cargo fmt.
Clippy for nigig-sms is filtered by package id: a plain -D warnings
would also fail on ~89 pre-existing warnings in nigig-core,
nigig-uikit and matrix_client, which are out of scope here.
--no-deps does not help, since those are workspace members rather
than registry deps.
Every job was executed locally against this tree before commit:
toolchain 1.97.1 per rust-toolchain.toml, JDK 17, SDK platform 34 /
build-tools 34.0.0. gates PASS, robius-sms PASS (0 tests),
android PASS, nigig-sms PASS (2 tests), supply-chain PASS.
These are only visible when cross-compiling: a host build of
robius-sms compiles sys/linux.rs, a stub whose every function returns
PermanentlyUnavailable, so none of sys/android/ is type-checked at
all. Host clippy was already clean; the Android target was not.
receivers.rs unnecessary `unsafe` block. The block wrapped nothing
that needs it -- taking `&mut env` and calling a safe
Rust fn -- inside an already-`extern "C"` function.
Removing it does not change what the JNI entry point
does; it stops the block from implying the body has
been audited for an invariant it does not have.
schedule.rs redundant closure |env, activity| load_schedules(env,
thread.rs activity) -> load_schedules.
All three are mechanical. No behaviour change.
Verified with the pinned toolchain (1.97.1), JDK 17, SDK 34:
cargo clippy -p robius-sms --all-targets -- -D warnings OK
cargo clippy -p robius-sms --target aarch64-linux-android
-- -D warnings OK
CI already runs with --locked: nigig-build.yml asserts
`cargo metadata --locked` and `git diff --exit-code -- Cargo.lock`,
and doc-engine.yml builds every target with --locked. None of that
can hold while the root lockfile is gitignored -- `--locked` fails
outright with no lockfile to check against, so in practice every
build re-resolved and a transitive dependency could change under CI
without any commit recording it.
The nigig-build workflow comment already claimed this was done
("Blocked until Cargo.lock was committed in Phase 0.2"); it was not.
This commits it.
The ignore rule becomes `**/Cargo.lock` + `!/Cargo.lock` so the root
lock is tracked while incidental nested locks stay ignored. The three
payment-crate locks keep their existing negations and stay tracked.
Generated with the pinned toolchain from rust-toolchain.toml (1.97.1)
against makepad rev a79f0dce; 566 packages. Verified `cargo metadata
--locked` exits 0.
456cfa5 rewrote tests/ui.rs to import makepad_test through the
makepad-widgets re-export, but did not add the `test` feature that gates
that re-export, so the pdf-ui target stopped compiling:
error[E0432]: unresolved import `makepad_widgets::makepad_test`
note: the item is gated behind the `test` feature
Enabling the feature alone is not sufficient. The `#[makepad_test]`
attribute expands to an absolute `::makepad_test::` path, which resolves
only if the crate is also a direct dependency:
error[E0433]: cannot find `makepad_test` in the crate root
So both are needed, and the manifest now says why - the direct
dev-dependency looks redundant next to the re-export and would otherwise
be an obvious thing for someone to delete. crates/apps/map keeps the
same pair for the same reason.
TEST_TARGET=pdf-ui 580 passing (was: failed to compile).
Phase 8 #6 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md
("Signatures"). Design and merge criteria in
REVIEWS/adr/0010-pdf-signatures.md.
Two defects, and the first is not about signatures at all.
1. acroform() dereferenced /AcroForm with self.resolve(), which recurses.
That replaced every /Fields [5 0 R] entry with an inline dictionary,
so AcroForm::walk saw node.as_ref() == None, decided the field had no
identity to key an edit on, and dropped it. The form came back EMPTY
rather than wrong, which is indistinguishable from a document with no
fields, so nothing announced the loss.
This is the third appearance of one mistake: page_annotations once
destroyed every annotation's obj_ref the same way, and
extract_xobjects resolved a reference then asked the resolved object
for as_ref(), leaving every page's XObject map empty.
It was invisible because both existing fixtures declare /AcroForm as
an INDIRECT reference, where only one level is dereferenced and the
refs survive. A direct /AcroForm dictionary - equally legal - hits it.
The new fixture uses one deliberately; reverting the one-line fix
fails 9 of the 12 new acceptance tests.
2. Nothing read signatures. FieldType::Signature was classified and then
ignored; /ByteRange, /Contents, /SubFilter and /DocMDP appear nowhere
in the codebase. A signed contract was presented exactly like an
unsigned one.
New pdf-document/src/signature.rs reads the signature dictionary and
checks BYTE-RANGE INTEGRITY, which needs no cryptography and catches the
common real-world tampering: whether the signed range reaches the end of
the file. A signature that stops short leaves appended bytes uncovered,
which is exactly how an incremental-update attack hides content behind a
signature that still verifies.
Cryptographic verification is NOT implemented and cannot be faked:
VerificationStatus has no Valid variant. That is enforced by the type,
not by convention, because the failure mode for a signature feature is
not "it doesn't work" - it is a green tick beside a document nobody
checked. A test asserts the capability's absence so adding Valid without
the cryptography breaks the build rather than shipping a false tick.
Signing is out of scope entirely: no private keys in this crate.
Also verified ADR 0003's claim that an append-only save preserves a
signed byte range, byte for byte, rather than leaving it asserted.
Implementation bug worth recording: /Contents was initially hex-decoded,
but the COS lexer already decodes <...>. Running it twice on the common
128-zero-byte placeholder - which contains no hex digits - produced an
EMPTY vector, silently discarding the signature blob while every other
field looked right. Caught by asserting the blob is non-empty rather
than asserting the parse returned Ok.
6 corpus fixtures, 12 acceptance tests, 28 unit tests, and a
parse_signature fuzz target because /ByteRange is four
attacker-controlled integers used to index the file.
TEST_TARGET=pdf 497 -> 536 passing. rustfmt and clippy -D warnings clean.
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.
Remove direct dependencies on makepad-test and use the re-exported
version from makepad-widgets instead. This avoids path dependency
issues and follows the correct pattern for using Makepad crates.
Changes:
- Add 'test' feature to makepad-widgets dependencies
- Remove direct makepad-test dependencies
- Update imports to use makepad_widgets::makepad_test
Affected crates:
- crates/apps/map
- crates/apps/pdf/pdf-makepad
- crates/apps/spreadsheet/spreadsheet-ui
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.
Every workflow in .forgejo/workflows is scoped with `paths:`. That is
right for build and test jobs -- no reason to compile the CAD module
because a payment file moved -- but it leaves a structural hole: a commit
touching only unfiltered paths runs no checks at all.
Commit 8c9ccb9 fell straight through it, pushing 30 unresolved
conflict-marker lines across five files. The worst of them was
.forgejo/workflows/nigig-build.yml itself: with markers in it the file is
not valid YAML, so all seven CAD gates silently stopped running. A
workflow that does not parse does not fail -- it does not run, which is
strictly worse than a red build because nothing announces it.
`git diff --check` would have caught this and was already a step. It was
in the file the same commit broke, and only ran for that workflow's
filtered paths.
So this workflow has no path filter, no toolchain dependency and no
network dependency -- it cannot be knocked out by an unrelated build
break -- and it validates the CI configuration itself:
1. No conflict markers anywhere in the tree.
2. Every workflow file parses as YAML and has a top-level `jobs:`.
3. Whitespace errors.
Verified against the real failure, not a synthetic one: checked out
8c9ccb9 and confirmed check 1 reports all 30 marker lines and check 2
names both unparseable workflows. Also probed for false positives --
`>>`/`<<` shift operators, `// =======` comments and a "=======" string
literal do not trip it, because the patterns are anchored to column 0 and
the closing marker requires the trailing space git writes.
Also fixes the fifth file from that commit, which I missed while
repairing the others because I only grepped .rs and .yml: ARCHITECTURE.md
still had two live conflicts on main. Both were stale-vs-current
versions of my own lines; kept the current text. While there, refreshed
every LOC figure in the file table from the actual files -- 14 of them
were stale, workspace.rs by 64 lines.
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
- Remove invalid property overrides on CostEstimatorMetric child widgets
- Remove unsupported 'visible' property from Svg widget
- Fix 'align' type mismatch by using Align{} constructor
These were runtime errors that didn't prevent compilation but caused
warnings in the Makepad script system.
origin/main (8c9ccb9) shipped an unfinished merge. Two problems, both in
files it merged from my recent commits.
1. COMMITTED CONFLICT MARKERS. Eleven `<<<<<<< / ======= / >>>>>>>`
lines were committed in workspace.rs and formula2.rs, so neither file
parses. The markers are literally in the pushed tree, not a local
artifact -- `git show origin/main:<file>` contains them.
2. ~3,000 LINES OF CadWorkspace DELETED. workspace.rs went from 3,908
lines to 883. Every method of `impl CadWorkspace` is gone --
handle_actions, draw_walk, all five export entry points, bake_parts,
with_viewport, export_scene_source. The impl block opens at line 185
and never closes, which is the "unclosed delimiter" the compiler
reports.
Resolution:
- workspace.rs restored from 36c9c0b, my last verified commit. Checked
first that upstream's truncated version added nothing: the set of
function names in it is a strict subset of mine, so nothing of theirs
is lost by restoring. Both helpers from the conflicting merge --
save_status_message and ExportBlocked/export_blocked_message -- are
present and were what the markers were fighting over. Both are kept.
- formula2.rs conflicts were resolved by hand. Two were whitespace-only.
The other two were the same test data formatted two ways; I kept
upstream's rustfmt output since they had just run a formatter over the
crate.
Deliberately NOT changed: the makepad rev stays at d6d1f99c and
Cargo.lock is untouched. The repo still does not resolve, for a separate
reason in the fork (see below), and quietly pinning back to 2c5cd97
would hide their bump rather than fix it. I verified this commit by
temporarily repinning locally, then reverted the manifests -- 685 lib
tests and 225 spreadsheet-engine tests pass against the restored source.
STILL BLOCKED, and it needs a fix in gitdab.com/andodeki/makepad, not
here: at rev d6d1f99c, widgets/Cargo.toml has three dependency lines
makepad-gltf = { path = "../libs/gltf", optional = true }
makepad-csg = { path = "../libs/csg/csg", optional = true }
makepad-test = { path = "../libs/makepad_test", optional = true }
sitting BELOW the `[features]` header instead of inside `[dependencies]`.
Cargo parses them as feature definitions, hence "invalid type: map,
expected a sequence", and the gltf/csg/test/maps features then do not
exist. The block was relocated by the "Update fork to upstream dev
5d4483f" merge; at 2c5cd97 the same three lines are inside
[dependencies].
Confirmed the one-block move is the whole fix: with those lines returned
to [dependencies] in a local clone, `cargo metadata --features
maps,csg,gltf,test` on makepad-widgets resolves cleanly.
A [patch] override is not a workaround here -- cargo rejects patching a
source with itself, and pointing at a corrected path clone makes the
patched and unpatched copies coexist, producing E0659 ambiguity across
makepad-ai and makepad-xr.
Phase 8 #5 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md
("Advanced transparency"). Design and merge criteria in
REVIEWS/adr/0009-pdf-transparency.md.
The headline defect was not that transparency was missing. `gs` was
MIS-PARSED as CloseStroke:
/GS0 gs 1 0 0 rg 100 100 200 200 re f
-> [CloseStroke, RgbFill, Rectangle, FillWinding]
so every page setting a graphics state gained a stroked path the
document never asked for, and lost its alpha, blend mode and soft mask
silently. Downstream everything was dead: StrokeExtGState/FillExtGState
were never constructed, set_fill_opacity was never called and emitted no
command when it was, and PdfPage::ext_gstate was read by no code at all.
New pdf-graphics/src/transparency.rs: full /ExtGState (ca, CA, BM,
SMask, LW, LC, LJ, ML, D, AIS, TK), all sixteen blend modes including
the four non-separable ones, soft masks with /S, /G, /BC and a /TR
evaluated through ADR 0006's PdfFunction, and /Group parsing. Wired end
to end: parser -> PdfOp::SetExtGState -> device -> RenderCommand ->
Makepad renderer.
Compositing is NOT claimed. Backdrop blending needs render-to-texture,
which an engine-neutral crate has no framebuffer for. Constant alpha is
applied because it needs no backdrop; blend modes and soft masks are
reported through TransparencyError::Unsupported rather than dropped,
because a silently ignored /Multiply looks exactly like a correct
/Normal.
The ADR required a test enumerating every multi-character operator, on
the grounds that fixing the third instance of a shadowing bug (after cm
and rg) without preventing the fourth is not a fix. It immediately found
three more live bugs, none of them transparency-related:
- `b` and `b*` dropped their close-path, mapping to FillStroke* instead
of CloseFillStroke*, so every closed-and-stroked path drew with a gap.
- Text operators within three bytes of the end of a content stream were
mis-parsed: the b'T' arm guarded `*i + 3 < len` while reading only two
bytes, so a stream ending in `/F1 12 Tf` parsed as FillWinding.
And the transparency-group fixture found a fourth:
- PdfPage::xobjects was empty for every page of every document.
extract_xobjects called doc.resolve(), which follows the reference,
then asked the resolved object for as_ref() - always None - and only
accepted a bare dict when every XObject is a stream. No `Do` operator
could be resolved through the page model. Same defect as the one that
once destroyed annotation object references; it now has its own
regression test.
T* and BMC are genuinely unimplemented and are deliberately excluded
from the guard's table rather than papered over.
7 corpus fixtures, 12 acceptance tests, 32 unit tests asserting the
§11.3.5 formulas (not our own output), and a parse_ext_gstate fuzz
target.
TEST_TARGET=pdf 451 -> 497, TEST_TARGET=pdf-ui 495 -> 541.
rustfmt and clippy -D warnings clean.
Went looking for remaining defects before starting the viewport split and
found silent data loss in the two Save buttons.
cad_store::save_cad_script(&project.id, &source).ok();
self.view.label(..).set_text(cx, "Saved");
save_cad_state(&source, &solid).ok();
self.view.label(..).set_text(cx, "Saved with mesh");
Both discard a Result carrying a real io::Error and then report success
unconditionally. The user is told their work is safe when nothing was
written.
This is reachable, not theoretical. `cad_store::cad_projects_dir()` only
*logs* a `create_dir_all` failure and returns the path anyway, so the
following `fs::write` fails with a genuine error -- which was thrown
away. An unwritable data directory, a full disk or a permissions problem
all render as "Saved".
Save-As had a third silent failure on the same line of reasoning: if
`bake_parts_solid()` returned None the save never even ran, and it still
said "Saved with mesh".
Fixed via save_status_message(what, result), so the label is derived from
the outcome instead of assumed. Three distinct messages now: the caller's
success wording, "Save failed: <cause>" with the underlying error text
preserved, and a specific message for the no-mesh and no-active-project
cases, which were previously indistinguishable from success.
Also replaced the hand-rolled widget borrow in Save-As with
with_viewport, the helper added in 5.2.
Tests assert the two properties that matter: a failed save must not read
as success and must carry its cause through to the user, and a
successful save must use the caller's wording. Negative-tested.
CI gate added and verified in both directions: greps for a discarded
Result on a save/write/persist/store/export call. A dropped Result is
merely untidy in most places; on a save path it is data loss.
These were the only two instances -- the CAD module now has zero
discarded write Results.
685 lib + 154 integration, 0 failed. All seven gates pass.
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.
export_floor_plan_pdf, export_3d_viewer, export_stl and export_svg each
opened with a byte-identical 23-line block: borrow the main viewport,
bail if it is missing, bail if it has no parts, then take the cached
scene, the shared mesh cache and the part count. Verified identical with
a regex over all four -- the only difference was the label prefix
("PDF", "3D", "STL", "SVG"), and even that was consistent within each
copy.
Now one method, export_scene_source(cx, kind), returning Option of the
triple and setting the status label itself. Four call sites, each two
lines.
This is the same duplication shape that hid the last three real bugs in
this module: nine copies concealed the properties-panel no-op, one
uninspected copy concealed the dead Extend tool, and nine keymap arms
concealed the KeyCode bindings. Nobody re-reads the fourth copy of a
23-line block. There is no bug hiding in these four -- I checked -- but
the next edit to one of them would only have landed in one.
The user-visible strings are the behaviour here, so they are pinned
rather than assumed: ExportBlocked + export_blocked_message hold the two
messages, and export_prologue_tests asserts all eight exact strings the
copies produced. A second test asserts the two reasons stay
distinguishable -- "no viewport" is a wiring fault, "no parts" is the
user having drawn nothing, and reporting one as the other sends someone
to debug the wrong thing.
Negative-tested: making NoViewport render the NoParts text fails both.
bake_parts deliberately keeps its own prologue. It needs a different
tuple (solid + script, not scene + cache + count), so folding it in
would mean a helper with an unused half.
683 lib + 154 integration, 0 failed. All six CI gates pass.
Three defects found by auditing the ADR merge criteria against the code
rather than against memory.
1. The scheduled fuzz job ran five hardcoded targets. Four had been
added since and were never fuzzed: parse_revision_chain, decrypt,
eval_function and parse_colorspace. eval_function is the sharpest of
those - it executes PostScript taken verbatim from an untrusted file.
The list now comes from `cargo fuzz list` and the job fails rather
than passing vacuously if it comes back empty.
`cargo fuzz list` reads the manifest, so a target file added without
its [[bin]] entry would still be skipped silently. The engine job,
which runs on every push, now checks the two agree. Both directions
of that guard were exercised before committing.
2. ADR 0004 rule 3 promises an annotation whose appearance cannot be
generated "keeps its original /AP and is reported as skipped". The
keeping worked - to_dict clones the source dictionary - but nothing
reported it: SaveReport only tracked skipped appearances for form
fields. A caller who moved a stamp was never told its artwork still
showed the old position. Adds
SaveReport::annotation_appearances_skipped and
appearance_is_generated, which enumerates the out-of-scope types
explicitly so a new AnnotationType fails to compile until classified.
3. SetContents and SetFlags had no round-trip test. Both were
implemented and unit-tested against the in-memory model, but neither
was ever reparsed from written bytes - the assertion ADR 0004 calls
central.
New fixture annotations/stamp.pdf carries real /AP artwork for a Stamp
(undrawable: must be preserved and reported) beside a Square (drawable:
must not be reported), so the reporting cannot pass by reporting
everything. The stamp test was mutation-checked: it fails when the
reporting line is removed.
ADR 0003 and 0004 merge criteria are now ticked. 0003's were genuine
paperwork - every box traced to an existing named test. 0004's were not,
and its ADR now records what was missing rather than implying it always
worked.
TEST_TARGET=pdf 447 -> 451 passing. rustfmt and clippy -D warnings clean.
The "No build-time paths used at runtime" gate has been failing since the
commit that introduced it, and I did not notice because I verified the
gate's intent rather than running its exact shell pipeline.
It greps the CAD directory for env!("CARGO_MANIFEST_DIR") and filters out
lines starting with a // comment marker. ARCHITECTURE.md documents this
very rule, so it necessarily names the macro -- and a Markdown list item
is not a // comment, so it survived the filter. The gate failed on its
own documentation.
Both files were added in the same commit (b5471e3), so this was broken
from the start rather than regressed later.
Fixed with --include='*.rs' on that gate, and pre-emptively on the other
two source-scanning gates (RFC1918 endpoints, by-value Vec3f getter
writes). Neither matches a non-.rs file today, but both would break the
same way the moment their rule gets documented in the same directory --
which is exactly what a reviewer is likely to do next.
Verified by extracting every grep-based step from the workflow and
running them in a shell with set -euo pipefail, rather than by
re-reasoning about the regexes. All six pass. Negative-tested the fixed
gate: a real env!("CARGO_MANIFEST_DIR") call in persistence.rs still
fires it.
Lesson worth recording: a gate must be executed as CI executes it. This
one asserted something true about the code and still failed, which is the
failure mode that erodes trust in CI fastest.
The orphaned doc comment at the end of the test module in style.rs
caused 'expected item after doc comment' error, which prevented the
entire style module from compiling. This made MapThemeStyle,
CompiledMapTheme, default_light_theme, and default_dark_theme
invisible to view.rs, causing 8 cascading compilation errors.
ProjectionLayoutTree::hit_test's nearest-glyph fallback resolved any
point to a glyph, so taps landing inside tables or advanced nodes never
reached the widget's table_hit_test/node_hit_test branches — production
cell taps silently moved the text caret instead of parking in cells.
The fallback now returns None when the point is inside a table or node
rect (those layers own their taps), while empty-space snapping beside
text still places the caret.
Close the "widget input, selection, mobile gestures" test gap with a
draw-free Area::Rect stub harness (stub_hit_area): tests seed the input
area a GPU frame normally fills and mirror the two platform
pre-dispatch steps (first_mouse_button, committed key focus), so raw
MouseDown/MouseMove and IME TextInput dispatch through event.hits
against a real Cx — exactly like a running app. Covered end-to-end:
hit-dispatch sanity, caret placement from a tap, shift-press selection
extension, drag selection over glyphs, TextInput inserting at the caret
and replacing an active selection, and a cell tap + TextInput whole-cell
round trip. The harness also exposed the test fixture actor drifting
from production's single "local" actor (cross-actor mid-run anchoring
is deterministic but not chronological — now a documented doc-engine
projection invariant); the runtime fixture now uses "local" for
production fidelity.
READMEs: doc runtime-tests section documents the stub harness and the
two bugs it unearthed; roadmap item updated (only the renderer draw
pass remains GPU/Studio-bound); doc-engine projection invariants gain
the per-actor counter boundary bullet.
6 new tests, 675 -> 681.
Reported four times, most recently with the detail that it appears at
runtime when tapping "Pay via M-Pesa" — not during a build.
That confirms the diagnosis rather than changing it. The reported string
was removed in 3e77bf5 and `git log -S` shows it in no source file today,
so a binary built from current main cannot print it. The reports are
coming from a binary built before that commit — most likely an installed
APK, which `git pull` does not update.
The real gap is that there was no way to tell those two builds apart from
the screen. A stale APK and a current one showed a message of the same
shape, so each report restarted the same investigation.
The message now ends with `[tracker-2]`. If the screen shows that marker,
the binary contains every fix to date and the message is the intended
product boundary. If it does not, the binary is stale and needs a rebuild
and reinstall.
Cheap to read aloud in a bug report, and it settles the question in one
round trip instead of four.
Makes table merge reachable on touch devices, closing the last gap of
the CRDT-native merge/split milestone (Shift+Arrow has no touch
equivalent):
- Long-press inside a table cell falls through the text glyph hit test
into table_hit_test and starts a TableCellSelection with anchor =
focus = the pressed cell (start_cell_range), clearing any text
selection; long-press on text keeps selecting word atoms unchanged.
- While the router idles in Selecting, the continued drag moves the
focus cell (extend_cell_range_to) through the tap hit geometry; drags
outside the anchor's table keep the focus and the caret tracks it.
Text word selections share the Selecting state but carry no cell
range, so their behavior is untouched.
- Lifting the finger keeps the range for the workspace Merge button or
Ctrl/Cmd+M (merge_selected_cells); Split already worked from a cell
tap. No new draw code — the existing cell-range highlight renders the
span.
Tests: 3 new runtime suites on a real Cx (long-press entry, drag
extension with caret tracking and full-grid normalization then merge
consumption on the wire, out-of-table drag keeping focus, and the text
word-selection regression guard) for 675 total in nigig-build.
Third report of "Payment dispatch is unavailable in this build".
That exact string is gone from source as of 3e77bf5, so a build from
current main cannot print it. But the bulk path still carried the same
phrasing ("Bulk payment dispatch is unavailable in this build"), which is
the same defect: it describes the binary rather than the product and gives
the user no next step.
Both gated paths now say what the app does and what to do instead. A CI
guard rejects `set_status(...unavailable in this build...)` so the phrasing
cannot come back; it checks displayed strings only, so the explanatory
comments remain legal. Verified to fail against the reintroduced wording.
Reported twice as a bug: "Payment dispatch is unavailable in this build;
no M-Pesa PIN was used".
The gate itself is correct — review items 0.1/0.3 require that a default
build cannot move money. Two things around it were not.
## 1. The obvious command did not work
$ cargo check -p nigig-pay --features demo
error: the package 'nigig-pay' does not contain this feature: demo
Only nigig-pay-ui declared the feature. Neither app crate had a [features]
section at all, so the only way in was --features nigig-pay-ui/demo, which
nobody would guess. Anyone following the natural path hit a hard error,
concluded the build was broken, and had no way forward.
nigig-pay and nigig-mpesa now forward it: demo = ["nigig-pay-ui/demo"].
A CI guard asserts both crates forward it and that both compile with it,
tested against a reverted manifest to confirm it fails.
## 2. The message read like a build error
"unavailable in this build" describes the binary, not the product, and
offers no next step. It now says what the app does and what to do:
This app tracks payments — it doesn't send them. Send in the M-Pesa app
or dial *334#, and it will appear here automatically. Your PIN was not
requested or stored.
The internal log strings that say "compile with --features demo" are left
alone: those are for developers reading logs, not users reading a sheet.
## 3. No build instructions existed
Review item 1.1 asks for them and the README had none. Added: default vs
demo build, the two caveats that matter (USSD has an Android backend only,
so demo does nothing useful on desktop; and demo must not ship because of
the unresolved Play-policy risk in ADR 0007), native library setup, and
the test commands.
## Validation
domain / storage / platform / mpesa harness pass
nigig-pay-ui: cargo test --lib pass (61)
cargo check -p {nigig-pay,nigig-mpesa} --features demo pass
cargo check -p {nigig-pay,nigig-mpesa} (default) pass
demo-forwarding guard: verified to fail on a reverted manifest pass
No change to what the gate permits. Enabling demo is still a deliberate
act with the Play-policy exposure recorded in ADR 0007.
Closes the last documented CrdtDocWorkspace toolbar parity gap by
porting the legacy DocEditor's mobile interaction policy to the
CRDT-native editor:
- The widget carries the shared InteractionMode (Edit default, View)
and latches mobile_mode_initialized on the first real touch, which
drops the session to View; desktop sessions stay editable until
actually touched.
- Mobile View mode reduces the surface to the gesture router: passive
caret taps (text and table cells), long-press word selection and
handles keep working, KeyDown handling is fully gated, and the
area-hit match is skipped so drags fall through to a parent
ScrollYView.
- The workspace toolbar gains the mobile-only Edit/Done AdaptiveView
control; toggle_interaction_mode flips the mode, mirrors the button
label, takes key focus entering Edit, hides the IME and resets
in-flight gestures entering View.
- Edit-mode touch taps request the IME at the tap point and draw_walk
reasserts show_text_ime every frame while Edit holds key focus,
anchored bottom-left of the live text or cell caret in clipped-area
coordinates — the frame-driven reassert mobile backends require.
Tests: 3 new runtime suites on a real Cx with real TouchUpdate/KeyDown
events (first-touch drop + key gating incl. Ctrl+B, in-cell Backspace
gating, Edit-mode tap placement with continued editing) for 672 total
in nigig-build. Physical IME open/geometry remains device verification.
Closes the roadmap's table merge command item on the CRDT editor
surface with a rectangular cell-range selection model driven from the
keyboard and the workspace toolbar:
- ProjectionSession::cell_selection holds stable anchor/focus row and
column ids; Shift+Arrow at a cell boundary starts the range and steps
the focus cell on an active one, with table-edge clamping, caret
follow, plain-action collapse, and undo-stale self-clearing.
- table_cell_range normalizes the selection against the projected
table; cell_selection_rects renders the highlight over visible cells
only, so merged spans contribute one expanded anchor rect.
- Ctrl/Cmd+M (merge_selected_cells, toolbar Merge) routes through the
engine's MergeTableCells op; undo splits through the symmetric
compensation. cell_range_mergeable rejects ranges overlapping an
existing merge UI-side, where ambiguous nested spans belong.
- Ctrl/Cmd+Shift+M (split_cell_at_cursor, toolbar Split) resolves the
containing merge via merge_at_cell even from a covered cell and
reveals the preserved hidden text; undo re-merges through
RestoreTableMerge.
- Workspace toolbar gains Merge/Split buttons with status-line
guidance (the only split path on touch devices).
Tests: 9 new (4 pure layout: range normalization/staleness, mergeable
rules, merge_at_cell, covered-skip highlight rects; 5 runtime on a real
Cx: shift-span + merge + undo, edge clamp, split + re-merge, plain
collapse, overlap rejection) for 669 total in nigig-build. The
doc-engine README gains the merge/split materialization invariant.
Taps park a TableCellCursor (stable row/column ids + char offset) inside
cells; typing and Backspace/Delete edit cells through whole-cell
SetTableCell replacements with symmetric undo. Arrows walk cell text in
reading order, hop between cells across rows, and exit into the nearest
text block in unified order at the table edges. Return inserts a row
immediately below the cursor's row and follows with the caret. Block
boundaries upgraded: backspace at a text start after a table enters its
trailing cell, forward-delete at a text end before a table enters its
first cell instead of merging structure. Style toggles stay text-only.
The cell caret draws between rendered characters using the renderer's
shared 6px inset / 7px-per-char advance, and stale cursors clear on
structural undos. Also fixed while wiring the paths: pointer hit tests
and layout-space decorations (selection, handles, carets) now account
for the widget origin, so taps and visuals agree at any dock position.
Runtime integration tests cover in-cell editing with undo restore,
arrow traversal/exits/clamping, and return-inserts-row-below; layout
unit tests cover the char-safe edit helpers, cursor resolution and
clamping, caret geometry, neighbor wrapping, and neighbor_text_block.
InsertTableRow/InsertTableColumn carried an after anchor on the wire but
materialization ignored it, ordering the row/column vectors by op id only;
an 'insert below X' could land anywhere once ids diverged. Rows and
columns now materialize over the anchor chain with RGA-style sibling
order (counter descending, actor ascending), matching text atoms, so a
newer insert below an existing row renders right after it and peers
converge on the same grid order.
The Makepad VM freezes children vecs in MapThemeStyle after initial DSL
evaluation. On re-evaluation (view switch, theme change), pushing new
MapFillRule/MapRoadRule children triggers 'cannot push to frozen vec'
errors, leaving the compiled theme empty (0 rendered features).
Fix: Strip all rule definitions from the DSL blocks in view.rs and build
complete CompiledMapTheme structs in pure Rust via default_light_theme()
and default_dark_theme() functions in style.rs.
- view.rs: Remove 80+ DSL rule entries, call Rust theme builders
- style.rs: Add theme builder functions with all fill/road/waterway/rail rules
- Fixes all 86 'frozen vec' errors per startup
- Fixes 0 rendered features on all map tiles
- Preserves identical visual output (same colors and widths)
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.
Drive the production widget end-to-end in lib tests: instances are built
through the registry's ScriptNew::script_new factory (bare ScriptVm with
unit host/std), the engine via set_engine, and real KeyDown events with
real KeyEvent/KeyModifiers payloads through Widget::handle_event.
Covered: caret anchoring from an uncursored editor, arrow stepping,
shift selection extension, Ctrl+B bolding across the selection, Enter
split at the caret with caret following the trailing half, Ctrl+Z merge,
and Backspace merging an empty split tail with the previous block.
The #[makepad_test] Studio harness was evaluated and rejected for this
milestone: it builds and launches the full app through the StudioHub
buildbox, and the repo's only examples (map tests/ui.rs and
makepad_visual_tests.rs) target aspirational APIs that do not compile.
Consolidate navigation onto a single CRDT docs destination now that the
DSL switch made the runtime-test tab an exact duplicate: drop the
'Docs CRDT' dock tab, its crdt_docs_content view, the sidebar's
'Documents CRDT Test' button and its select_tab handler. The desktop
'Docs' tab and 'Documents' sidebar button land on CrdtDocWorkspace, and
mobile's workspace drawer resolves 'Documents' to the CrdtDocWorkspace
doc_page through the new pure workspace_page_id function, pinned by unit
tests (destination, label table, page distinctness, CAD fallback).
Three orphan closers left over from the CategoryDropdown/AddRoomModal
restructure broke the script_mod block and with it the nigig-build build
at HEAD; AddRoomModal now follows CategoryDropdown at DSL top level.
The API change tranche 10 scoped, in REVIEWS/adr/0007.
## dispatch_with_authorization
Tranche 10 established that Android cannot implement BiometricAuthorizer
without reopening defect S2, and built AuthorizationAttempt as the
replacement. It named the remaining work: a coordinator entry point taking
an already-granted attempt instead of calling biometric.authenticate().
coordinator.dispatch_with_authorization(&id, &mut attempt)
It does not prompt, does not wait, and does not consult the injected
BiometricAuthorizer at all. That is asserted rather than assumed: one test
injects an authorizer reporting no hardware that also errors, and shows a
granted attempt still dispatches. If the coordinator ever fell back to it,
that test fails.
Three refusals, each with a test that fails when the gate is removed:
- An unanswered prompt does not dispatch. PromptShown plus SensorEngaged
is not consent — the exact shape the Android adapter would produce.
- A grant belongs to one payment. A foreign grant is refused before
anything else, so a stray callback cannot even fail the payment on
screen; the victim intent stays in AwaitingUserAuthorization rather
than being transitioned to Failed by a stranger.
- A grant is spent on use. One authorization, one dispatch (B3).
Denied, still-prompting and already-spent attempts all fail the intent
closed and never reach the gateway.
## Verifying the tests can fail
The gate was removed (if !attempt.consume() -> if false) and the suite
re-run: 5 tests failed. A fail-closed test that passes against fail-open
code is worthless, so this is the check that matters.
Worth recording: a_grant_cannot_dispatch_twice still passed with the gate
removed, because the existing duplicate-dispatch budget caught it
independently. Two unrelated mechanisms refuse the second dispatch. That
is defence in depth working, and the reason that test is not sufficient
evidence on its own.
## Validation
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
fail-open injection: 5 tests fail with the gate removed pass
Domain tests 129 -> 137.
## What remains
The authorization half is done and verified. PayFlowHandler still owns the
USSD session lifecycle, the pending-store writes that shadow the
coordinator's repository, and the bulk queue plumbing. Moving those makes
the coordinator own the gateway session, which changes who cancels on
teardown and who observes an out-of-order callback — ADR 0007's device
matrix, which cannot be exercised here.
