Move all theme rules (fill, road, waterway, railway) from DSL to pure
Rust functions. This eliminates the 'cannot push to frozen vec' errors
that occurred when MapThemeStyle children were pushed to frozen objects
on re-evaluation.
Changes:
- Remove all MapFillRule/MapRoadRule/etc from view.rs DSL
- Add default_light_theme() and default_dark_theme() in style.rs
- These functions build complete CompiledMapTheme with all rules
- Simplifies view.rs DSL to only set background/status_text/label
This fix ensures themes are properly initialized without triggering
the Makepad script VM's frozen vec protection, allowing map tiles
to render with the correct styling rules.
Both active workspace DSL sites (desktop dock docs_workspace and mobile
doc_page m_doc_content) now instantiate CrdtDocWorkspace instead of the
legacy DocWorkspace; the classic widget stays registered as fallback.
CrdtDocWorkspace graduates from the runtime-test shell to the full
workspace surface: Open/Save/SaveAs round-trip the shared #MP_CRDT_V1
wire via projection_session::{crdt_save_wire, crdt_engine_from_saved},
Undo/Redo and B/I/U share the editor's keyboard-driven engine paths,
alignment buttons route the legacy DocAlign wire strings, and
+Table/+Img/+Div insert through new CrdtDocEditor helpers with legacy
anchored-after-caret semantics. Stats count words/chars from the
projection (new projected_stats helper). The mobile Edit/Done IME
toggle stays legacy-only until the native widget owns an IME mode.
Review defect S2, plus an amendment to ADR 0007.
## Attempting the migration found a defect in the plan
The stated next step was replacing PayFlowHandler with PaymentCoordinator,
which means writing a BiometricAuthorizer adapter for Android. It cannot
be written correctly, and why is the substance of this change.
The trait contract is blocking: "authenticate must be a blocking call that
waits for user action. Returns Ok(()) on success."
robius_fingerprinting::authenticate does not do that. Reading through
sys/android/prompt.rs: it calls the Java authenticate static method and
returns Ok(()) as soon as the prompt is on screen. The user's answer
arrives later through next_event().
So an adapter can either return Ok(()) when the prompt opens — and
authorize_and_dispatch then sends money before the user has touched the
sensor, which is defect S2 reintroduced through the type system — or
block, and deadlock the thread that must pump the callback.
Had the migration been done without noticing, the result would have been a
correct-looking refactor that silently reopened the review's most serious
security finding.
## AuthorizationAttempt
Authorization is a state machine, not a function call:
Requested -> Prompting -> Granted | Denied, advanced by inbound signals.
One rule, enforced by the type: only an explicit success grants dispatch.
- A displayed prompt does not authorise. A touched sensor does not
authorise. A non-match keeps the prompt up and stays retryable.
- A grant is bound to one intent, so a callback for an abandoned payment
cannot authorise the current one.
- A grant is spent on use, so one fingerprint cannot authorise two
dispatches (B3), and a replayed success cannot re-arm it.
- A late success after a cancel is ignored, not resurrecting the payment.
- Backgrounding mid-prompt denies; it never silently allows.
dispatch_ussd now consumes a grant before dispatching and refuses without
one. auth == None means no biometric gate was configured, which is
deliberately distinct from an ungranted one. Both cancel paths abandon the
grant.
A CI guard asserts the gate exists and was tested with the check disabled
to confirm it fails.
The trait keeps its blocking contract for synchronous authorizers and test
doubles, and now documents that Android must not use it.
## Validation
domain : 129 tests --locked, fmt, clippy -D warnings, bench pass
storage : 36 + 41 sqlcipher --locked, fmt, clippy pass
platform: 56 + 64 ussd --locked, fmt, clippy, mock guard pass
nigig-pay-ui: cargo test --lib pass (61)
nigig-pay-ui / nigig-pay / nigig-mpesa / nigig-core: check pass
authorization guard: verified to fail with the gate removed pass
batch-counter and settlement-tick guards: still passing pass
Domain tests 117 -> 129.
## Where the migration stands
The blocker is no longer unknown. PaymentCoordinator needs an
authorization path that does not assume a blocking authorizer, and
AuthorizationAttempt is that path, built and tested. What remains is a
coordinator entry point taking an already-granted attempt instead of
calling biometric.authenticate() itself, then moving USSD session
ownership across.
That is an API change that should be designed against ADR 0007's device
matrix — permission denial, cancellation, backgrounding, app restart,
out-of-order callbacks — none of which can be exercised here.
CrdtDocEditor now handles the desktop keyboard surface natively through
doc-engine operations: Ctrl/Cmd+Z/Shift+Z undo-redo via CrdtHistory,
Ctrl/Cmd+B/I/U selection style toggles, arrow-key caret moves with
shift-extend across block boundaries via the step_glyph stream,
selection-aware Backspace/Delete with block-boundary merges, Return
splitting paragraphs or appending table rows, and TextInput that
replaces the active selection at the session caret block.
Engine additions: Operation::SplitBlock/MergeBlocks with symmetric
Compensation pairs; materialization keeps a split parent's trailing
runs as a synthetic child spliced after the parent so peers converge
without a new InsertBlock op; merge_runs coalesces equal style runs.
Review items 6.6 / U7, and B3 at batch scope.
## A progress bar that could read 3/2
While working toward retiring the thread_local handler, the bulk
accounting turned out to hold a defect worth fixing on its own.
bulk_done was a bare usize incremented at eight independent match arms,
none bounded, and bulk_current_idx added one more while an item was in
flight. Nothing tied the counter to the batch contents.
The USSD pump does not guarantee one terminal event per item — an Error
can be followed by a SessionEnded for the same session, and each arm
increments separately:
one item, two terminal events -> bulk_done=2/2
BulkItemDispatched reports 3/2 <-- exceeds total
That was reproduced before changing any code, so this answers a
demonstrated defect rather than a suspected one.
## PaymentBatch
Progress is now a function of the batch contents, not of how many code
paths ran. Each item carries exactly one BatchItemOutcome and settle()
refuses a second instead of counting it, which makes finished > total
unrepresentable rather than merely unlikely. Every former bulk_done += 1
routes through one settle_bulk_item helper.
A CI guard rejects direct increments and was tested against the
reintroduced bug to confirm it fails, same as the tick guard in tranche 8.
Three properties follow from putting this somewhere testable:
- finished is not succeeded. confirmed counts only provider
confirmations, so a batch that ended with unknown outcomes reports
"Batch finished — N confirmed, M awaiting confirmation. Do not
re-send" and never carries a tick. Only all-confirmed may be ticked.
- A pending item is never offered for re-run. safe_to_recreate returns
only NotSent and Rejected; replaying a possibly-settled item is B3.
- Cancelling does not rewrite history. cancel_remaining marks only items
that never got an outcome, because cancelling a batch does not recall a
request the provider may already hold.
A stray callback for an unknown item id is refused rather than counted —
the batch-level counterpart of ADR 0007's correlation rule.
## Validation
domain : 117 tests --locked, fmt, clippy -D warnings, bench pass
storage : 36 + 41 sqlcipher --locked, fmt, clippy pass
platform: 56 + 64 ussd --locked, fmt, clippy, mock guard pass
nigig-pay-ui: cargo test --lib pass (61)
nigig-pay-ui / nigig-pay / nigig-mpesa / nigig-core: check pass
batch-counter guard: verified to fail on the reintroduced bug pass
Domain tests 104 -> 117.
## On the thread_local, again
Still there; this change did not remove it. What changed is that the state
carrying correctness risk — the batch accounting — no longer lives in it.
PayFlowHandler now delegates every outcome to PaymentBatch, so the
thread-local holds queue plumbing and platform handles rather than the
numbers a user is shown.
That is a smaller and more honest claim than "Phase 6 wiring complete".
Replacing PayFlowHandler with PaymentCoordinator moves ownership of the
USSD session and the biometric prompt, and belongs in its own change with
ADR 0007's device matrix, not folded into one that also touches
accounting.
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.
- word_atom_range mirrors the legacy word_bounds whitespace-pivot
semantics and returns a word's first/last atoms for SelectWord
- selection_handles normalizes anchor/focus into document order and
emits 12px handle rects with a 6px touch-slop hit test; collapsed
selections (caret) produce no handles
- CrdtDocEditor drives the shared MobileGestureRouter: TouchUpdate Start
grabs visible handles or arms long-press on the frame clock, SelectWord
applies the atom word range, handle drags move selection endpoints,
drags past threshold stay unclaimed for ScrollYView, and short taps
end as passive cursor moves without the IME
- synthesized finger events are ignored once a real touch sequence is
seen; desktop mouse/IME behavior unchanged
- draw_selection_handle live field (default #x1f73e6) + 5 geometry tests
Phase 6 and item 7.3 of REVIEWS/NIGIG_PAY_CONSOLIDATED_REVIEW.md.
Rationale in REVIEWS/adr/0008.
## The rule, made into a type
Phase 6's exit criterion is that the UI cannot call a transaction
successful without trusted confirmation, or failed without known
rejection. The old code violated it structurally: the sheet set its
status inline in about a dozen places, each from whatever local signal
was nearest, so there was nowhere to put the rule.
The worst instance is U4. A payment whose confirmation SMS had not
arrived in five minutes was marked Failed, the sheet closed, and the user
saw "✗ Payment failed" — with a retry button — while the money was gone.
payment_view_state.rs makes the rule a type. PaymentPresentation has no
Success variant reachable from untrusted evidence and no Failed variant
reachable from a missing SMS. VerificationExpired, Unknown and
UnknownNeedsReconciliation all present as PendingConfirmation: not
success, not failure, and no retry offered. Two defence-in-depth rows:
Confirmed without a provider reference reads as unknown rather than
settled, and an unclassified failure is not evidence nothing was sent.
6.2: ConfirmationSummary makes every required term a mandatory field, so
a confirmation missing the fee or total is not constructible. It is built
from the same quote that gets dispatched. material_digest() binds consent
to the exact terms shown and is re-checked on OK — if anything material
moved in between, the authorisation is void and the screen is shown
again. The modal previously showed only recipient and amount.
6.3: must_stay_open() keeps pending payments visible, and the status line
begins with the exact required wording, asserted by a test.
6.5: evidence rows are exposed and labelled untrusted; pending payments
offer receipt, problem-reporting and data-deletion actions.
## Three defects found by writing the tests
1. Bulk quotes silently under-charged. compute_bulk_costs used filter_map
over the fee lookup, so a contact outside the tariff was dropped from
the fee total and the user was quoted less than they would pay.
2. The batch total could overflow — .sum() panics in debug, wraps in
release. A wrapped total is a quote for the wrong amount.
3. The cost preview showed unknown fees as free via unwrap_or(0).
All three are B5/B6 territory. Item 3 is B5 resurfacing in the preview
path after tranche 1 fixed it in the dispatch path: fixing a defect at one
call site is not the same as fixing the defect.
## A pre-existing failing test, diagnosed rather than deleted
money::tests::ksh_rounds_to_nearest_cent asserted format_ksh(1.005) ==
"1.01" and had been failing on every run — confirmed pre-existing by
stashing this work and re-running clean.
The expectation is impossible, not the formatter wrong: 1.005 has no
binary representation, the nearest f64 is 1.00499999999999989..., so the
correctly rounded result is 1.00. This is defect B6 at its smallest. The
test now says so, with a companion showing Money handling it exactly.
Deleting it would have hidden a live argument for finishing B6.
## 7.3 UI tests in CI
The NIGIG_TEST_PAY gate was already gone; what blocked CI was the Makepad
link step. tools/makepad-native-libs.sh listed the libraries needed to
compile but not libasound2-dev, libpulse-dev and libssl-dev, which are
needed to link a test binary — cargo check succeeds and then
"unable to find library -lasound" appears much later. The helper now
installs and checks them, and a payment-ui-tests job runs the UI tests
plus cargo check on nigig-pay-ui, nigig-pay and nigig-mpesa.
## Validation
domain : 95 tests --locked, fmt, clippy -D warnings, bench pass
storage : 36 + 41 sqlcipher --locked, fmt, clippy pass
platform: 56 + 64 ussd --locked, fmt, clippy, mock guard pass
nigig-pay-ui: cargo test --lib pass (61)
nigig-pay-ui / nigig-pay / nigig-mpesa: cargo check pass
Domain 75 -> 95 tests. UI 55 -> 61, with the long-standing failure fixed.
Every new CI step was run locally before commit.
## Not claimed complete
Phase 6 wiring is partial. PaymentViewState exists and is tested, and the
confirmation path uses ConfirmationSummary, but the sheet's remaining
status strings are still set inline and the thread_local PayFlowHandler
still exists. The type makes that migration mechanical; it does not
perform it. 6.6 bulk stays demo-gated pending ADR 0007's unresolved 5.2.
Register style::script_mod before view::script_mod so MapThemeStyle and
related types are available when NigigMapView's DSL is parsed.
Fixes:
- type mismatch for property style_light/style_dark
- 0 rendered features (empty theme rules)
Phase 5 of REVIEWS/NIGIG_PAY_CONSOLIDATED_REVIEW.md. Design and the one
item engineering cannot close are in REVIEWS/adr/0007.
## Phase 4 is now verified, not just written
Tranche 5 implemented the draw_walk fixes and said plainly that the
nigig-pay widget edits were unbuilt. That blocker was environmental:
installing the packages tools/makepad-native-libs.sh already lists makes
the UI graph compile. Both commands the status doc listed as required
now pass:
cargo check -p nigig-pay pass
cargo check -p nigig-pay-ui pass
No code changed for this; the claim is now evidence rather than assertion.
## Phase 5: new nigig-pay-platform crate
ADR 0002 reserved this crate and marked it "not yet created".
5.1 One crate owns the seam. It is the only crate in the payment stack
that may name a platform SDK. CI enforces both directions: payment crates
may not import Makepad, and domain/storage may not import jni or the
robius platform crates.
5.3 Correlation is mandatory and single-flight. SessionRegistry admits an
event as Accepted, Duplicate or Ignored; PlatformEvent cannot be built
without a CorrelationId. Closed session ids are retired permanently, so
an abandoned session's confirmation cannot settle the payment that
replaced it. There is a test named after exactly that scenario.
2.8/5.3 Progress decides retry safety, not error kind. classify_failure
takes the failure and the DispatchProgress reached before it, and
progress is the authority. The same TemporarilyUnavailable is safely
retryable before the dial and ambiguous once the menu is being driven —
the distinction the old code could not make, which is defect B3's
mechanism. A property test asserts across the whole failure space that
nothing which may have reached the provider authorises a fresh attempt.
5.4 Fakes cannot ship. MockGateway is cfg-gated, is a compile_error! in a
release build unless allow-mock-in-release is named explicitly, and
stamps every session id with MOCK-. CI asserts the release build fails.
5.5 No unsafe, no PIN. The crate is #![forbid(unsafe_code)] so the JNI
surface stays in robius-ussd. UssdGateway is !Send/!Sync by construction,
making the main-thread requirement a compile error. The adapter leaves
the pin field empty and a test asserts it.
5.6 The web claim is withdrawn. No browser API can drive USSD and a
Daraja credential must never reach a browser, so WebGateway refuses every
call and maps to Fatal — "never sent" — which owes no reconciliation.
7.5 Adversarial SMS corpus. StrictMpesaSms is the payment-boundary
reader, deliberately separate from nigig-core's permissive tracker parser
(ADR 0007 explains why this is not the duplication ADR 0002 forbids). It
requires an exact 10-char code, exact sender-ID match so MPESA-REFUNDS
and FAKE-MPESA are refused, rejects fractional shillings instead of
rounding, and caps body length. Corpus covers spoofing, forged code
shapes, out-of-range amounts, unicode and NUL injection, and replay. The
closing test asserts the honest limit: a well-crafted forgery is still
only evidence, because the output type has no settled state to reach.
## 5.2 is not done and is not closeable here
The AccessibilityService Play-policy review is a business decision. ADR
0007 records it as blocking, states the termination exposure, and names
what must happen before the rail is enabled. USSD dispatch stays behind
the default-off demo feature. If the review fails, ADR 0001's
tracker/launcher position applies and only the dispatch adapter is lost.
## Validation
platform: 56 tests, 64 with --features ussd, fmt, clippy -D warnings
(both feature sets), cargo-deny, mock-in-release guard
asserted to fail pass
domain: 75 tests --locked pass
storage: 36 + 41 tests --locked, incl. sqlcipher pass
nigig-pay, nigig-pay-ui: cargo check pass
cargo-deny reports advisories/bans/licenses/sources ok. The isolated
runner gained a `platform` target and it runs in CI on every push that
touches the crate.
- layout_projection walks the unified DocumentProjection::order so
advanced nodes interleave with paragraphs and tables at their anchor
positions; block_origins are assigned by index for both paths, with a
blocks-only fallback for projections assembled without the op log
- projected_node_metrics mirrors legacy AdvancedLayout heights, labels
and interactivity per kind (unknown kinds follow the bridge's
EmbeddedWidget mapping: 'Widget: <kind>')
- ProjectionRenderer::draw_node_projection reuses the legacy
draw_advanced_blocks colors; dividers collapse to a centered 1px line;
new draw_node_fill/draw_node_border live fields on CrdtDocEditor
- fix(doc-engine): the after-chain was block-only, so any block anchored
after an advanced node was unreachable during materialization and
vanished from the projection; nodes now participate as chain connectors
- tests: unified-order interleaving, node metrics, node hit test, mixed
table/node stacking, order-less fallback (nigig-build); node-anchored
block materialization and mixed sibling ordering (doc-engine)
- projection_layout builds ProjectedTableLayout geometry (fixed 160x28
cells matching the legacy bridge width, merge spans with covered-cell
flags) and per-block origins so blocks after a table clear it instead
of using the fixed line step
- ProjectionRenderer draws the table grid through a new draw_table_border
live field and consumes layout block origins for text placement
- table_hit_test maps points to merge-anchor cells for future cell editing
- doc-engine: split the cramped style-patch line clippy flagged as
possible_missing_else; the crate is clippy-clean again
- README: tick verified CRDT bridge boxes (dependency, wiring, selected
replacement/formatting/table/toolbar migration, font size/color
migration) and document the table milestone
- CI: add doc-engine workflow (engine tests + clippy -D warnings +
nigig-build consumer check/test) and trigger nigig-build CI on doc
changes
Phase 8 #4 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md
("Advanced colour"). Design and merge criteria in
REVIEWS/adr/0006-pdf-advanced-color.md.
The interpreter tracked only the *name* of the active colour space and
then passed sc/scn operands to the device as if they were already RGBA.
Every non-device space therefore rendered a confident wrong colour with
no error:
/Spot cs 1.0 scn full tint of a spot ink -> pure red
/Idx cs 3 scn palette entry 3 -> near-black
/Lab cs 50 0 0 scn mid gray -> white (clamped)
/DevN cs (5 inks) five colorants -> inks 5+ discarded
/ICCBased profile-defined colour -> profile discarded
DeviceCMYK also used the additive 1-c-k conversion, which crushes any
colour printed over black.
Three new modules in pdf-graphics:
- function.rs PDF functions, all four types. Type 4 runs on a bounded
interpreter: depth 32, 32768 tokens, stack 100, 100000
steps, and an unknown operator is an error rather than a
no-op that would leave a plausible wrong colour.
- icc.rs ICC matrix/TRC and gray kTRC profiles, applied exactly.
LUT-class profiles are reported as such and the caller
falls back to /Alternate; they are never pretended to be
matrix profiles.
- colorspace.rs All eleven families, converting through XYZ with
Bradford adaptation and a real sRGB transfer function.
Wiring:
- PdfDevice gains set_stroke_components/set_fill_components, so SC/SCN
reach the device as components of the active space instead of being
read positionally as RGBA.
- cs/CS now resets to the space's initial colour (table 74), which is
why golden/colors.txt gains a line.
- PdfPage::color_spaces carries /Resources /ColorSpace fully
dereferenced with streams decoded; a half-resolved space would make
every ICC profile, palette and type 0/4 transform silently fall back.
- A space that cannot be resolved keeps the previous colour and records
a typed ColorError. No colour is invented, and no error is swallowed.
Tests: 12 corpus fixtures under tests/corpus/color/, 14 acceptance
tests in pdf-document/tests/color.rs asserting numeric RGB (the broken
code produced a colour for every one of these; only the value was
wrong), plus unit tests per function type and per curve type. Two fuzz
targets added: eval_function and parse_colorspace.
TEST_TARGET=pdf 387 -> 447 passing, TEST_TARGET=pdf-ui 431 -> 491.
rustfmt and clippy -D warnings clean.
- Increased MAX_ELEMENTS_PER_TILE from 100,000 to 250,000 to handle
Kenya MBTiles that contain 101k-140k elements per tile
- Updated security limit test to use valid JSON with 260k elements
- Added .forgejo/workflows/nigig-map.yml CI workflow to catch
map-related regressions in tile parsing, style compilation,
and tessellation
Fixes runtime errors:
- 'failed to triangulate local mbtile: too many elements (N > 100000)'
- Tiles with 101k-140k elements now process successfully
origin/main was red: navigation_and_lifecycle_are_headless asserted that
after `exchange_sheet_data(0, &mut external)` the caller's buffer holds
"active". Nothing in the test ever writes that string.
`exchange_sheet_data` is a two-way `mem::swap`, so `external` comes back
with sheet 0's contents. Sheet 0 is empty at that point: "adapter" was
written while sheet 1 was active, and `remove_sheet(1)` then deleted the
sheet holding it.
Corrected to assert the empty string, and added the assertion the test
was missing -- that the sheet now holds "grid". Checking only one side of
a swap would pass for a function that merely cleared the buffer.
Verified pre-existing: fails identically on origin/main without my
commits.
Went looking for a clippy ratchet and found that clippy had never run on
this crate at all: two dependencies fail deny-by-default lints, so
`cargo clippy -p nigig-build` aborted before linting nigig-build. Fixing
those two unblocked the crate and immediately exposed a real defect
class.
THE BUG. A bare identifier in a match pattern that is not a known variant
is parsed by Rust as a NEW BINDING that matches everything. Nine such
names were used as KeyCode patterns:
KeyEnter, KeyBackspace, BracketLeft, BracketRight (cad/viewport.rs)
Digit0..Digit9, Equal, LeftBracket, RightBracket,
Apostrophe (doc, invoice, pm)
Real names are ReturnKey, Backspace, LBracket, RBracket, Key0..Key9,
Equals, Quote.
Consequences, in severity order:
- cad/viewport.rs: `BracketLeft => { .. }` is UNGUARDED and sits above
the numeric arms, so it swallowed every remaining key. All
direct-distance-entry input -- digits, '.', '-', ',' -- was
unreachable. The entire DDE feature was dead.
- The guarded ones fired for any key satisfying the guard: pressing Q
while drawing with a non-empty buffer committed the coordinate.
- doc/invoice/project_management: `Digit0 => '0'` swallowed the rest of
the keymap, so every digit and symbol typed produced '0'.
This compiles cleanly and no test catches it. rustc's only signal is
`unreachable_pattern` plus `unused variable: \`Capitalised\`` -- both
buried in the 200+ warnings nobody could see, because clippy never ran.
85 unreachable-pattern warnings before, 1 after (a benign catch-all).
UNBLOCKING CLIPPY. Two deny-level errors in dependencies:
- nigig-uikit user_project_pill.rs: a `for` loop returning on its first
iteration (never_loop). Rewritten as `.next()`.
- spreadsheet-engine formula2.rs: CellRef had an inherent to_string
shadowing Display (inherent_to_string_shadow_display). The naive fix is
a trap: Display::fmt was `f.write_str(&self.to_string())`, which
resolved to the inherent method -- delete it and the same call resolves
to ToString::to_string, which calls Display::fmt, recursing until the
stack overflows. Verified with a standalone repro before fixing. The
body moved into Display; Range got the Display impl it never had.
Tests: keycode_variant_tests names the four correct variants, so it stops
compiling if any is renamed -- the point being that the old code compiled
precisely because the names were wrong. Two formula2 tests pin that
`x.to_string()` and `format!("{x}")` agree, which is what the shadowing
lint exists to protect.
CI gate added and negative-tested: greps for a capitalised
`unused variable`, which is the signature of this bug. Restoring
BracketLeft makes it fire.
625 lib + 154 integration + 225 spreadsheet-engine + 44 doc-engine, 0
failed.
ARCHITECTURE.md invariant 2 has said since it was written that the
scattered clear_mesh_cache() calls are "unnecessary and expensive",
because MeshCache keys on (NodeId, ParamHash) and an edited node
invalidates itself. Phase 5.2 removed the first one. This removes the
rest: 8 of the 11 live call sites, leaving only invalidate_all (whose
contract is exactly that) and the definition.
Investigating them turned up something the invariant did not say, and
which makes the cleanup a bug fix rather than tidying:
MeshCache has NO eviction for deleted nodes.
Entries are keyed by NodeId and nothing ever removes one when its node
goes away. The wholesale clears were, accidentally, the cache's only
garbage collector. Deleting them naively would have converted a
performance problem into an unbounded memory leak over a session.
So the fix is a targeted replacement, not a deletion: MeshCache::retain,
SceneCache::retain_meshes and CadViewport::evict_dead_meshes drop exactly
the entries whose node is gone and leave every live one warm. Each call
site was then classified rather than pattern-matched:
- deletes (delete_selected, delete_part, CommandContext::delete_node)
-> evict dead entries
- pure additions (create_node, insert_node_at, paste, and the wall /
circle / rect / area / column / beam / polygon / polyline tool
completions) -> nothing at all. A new node has no cache entry to
invalidate and cannot affect any other node's mesh. These were
discarding the entire scene's meshes to make room for nothing.
- subdivide_selected -> invalidate only the selected ids. It builds a
fresh Arc<Solid> per part, so ParamHash already differs; the unselected
parts were being thrown away for no reason.
Measured: deleting 1 of 100 parts costs 62.1 us with clear+rebuild versus
21.9 us with evict+reuse, a 3x saving on every delete.
Two tests, both negative-tested by stubbing out the retain body:
retain_nodes_evicts_dead_entries_and_keeps_live_ones checks both
directions (dead gone, live still an Arc::ptr_eq hit), and
cache_does_not_grow_past_the_live_node_set simulates 50 add/delete rounds
and asserts the cache holds 1 entry, not 50. With retain stubbed it holds
exactly 50, which is the leak.
Also documented a sharp edge found while reading ParamHash: it hashes a
Csg node's Arc POINTER, not its geometry. Re-wrapping identical geometry
in a new Arc is therefore a safe false-positive, but mutating a Solid
behind a shared Arc would go unnoticed. Nothing does that today and
ARCHITECTURE.md now says it must stay that way.
624 lib + 154 integration, 0 failed.
Blocked since Phase 0 because cargo-deny resolves from the lockfile, and
unblocked once Cargo.lock was committed in 0.2. I predicted this would
surface a large backlog. It did: 5 advisories, 6 licence rejections, 126
git-source errors and 20 unlicensed crates.
The triage is the work here, not the config.
ONE ACTUAL VULNERABILITY. RUSTSEC-2026-0187: unbounded recursion parsing
nested PDF objects in lopdf 0.31, reached via printpdf 0.7. A ~21 KB
crafted file aborts the process with SIGABRT, and because it is a stack
overflow rather than a panic, catch_unwind cannot contain it.
Not exploitable here, and I checked rather than assumed. The advisory is
specifically about Document::load*, the parsing entry points. This crate
never parses a PDF -- arch_pdf.rs only writes them through printpdf's
drawing API, there is no lopdf import anywhere in the workspace, and
there is no PDF read path of any kind. There is also no fix in range:
printpdf 0.7 pins lopdf 0.31 and `cargo update -p lopdf` moves nothing.
Clearing it means printpdf 0.8+, a breaking change across arch_pdf.rs.
So it is ignored with the reasoning written down AND with the condition
that invalidates it: the moment anything in this crate reads a PDF -- an
import feature, a thumbnailer, a preview pane -- this becomes a live DoS
and the ignore must go. An ignore without its expiry condition is how
real vulnerabilities get inherited.
The other four advisories are unmaintained/unsound transitive crates
(proc-macro-error, ttf-parser, atomic-polyfill, glib VariantStrIter),
none with a safe upgrade, all arriving through the GUI/platform stack.
Listed individually rather than disabling the unmaintained class, so a
new one still fails.
Six licence rejections were permissive licences simply absent from the
allow list. MPL-2.0 (option-ext) is weak copyleft, so it is granted to
that one crate rather than added globally -- MPL reciprocity is per-file
and only bites if the crate is vendored and edited.
The 126 git-source errors and 20 unlicensed crates are artifacts, not
findings. Every git dependency is pinned to a full rev, which is stronger
than a version range and already enforced by its own CI step; the
unlicensed crates are first-party and vendored forks with no license
field. Both handled WITHOUT allow-ing the class: sources are governed by
the rev-pinning step, and the 20 crates are clarified by name, so a
genuinely new unlicensed dependency still fails. Blanket-disabling would
have hidden exactly the case worth catching.
Separate file from deny.toml on purpose. Merging them would mean
loosening the payment rules to fit a Makepad + Robius graph, and the
payment policy is the one worth keeping tight. Verified both pay crates
still pass unchanged.
Negative-tested in both directions: removing the lopdf ignore fails the
gate, and removing MIT from the allow list produces 79 rejections. A gate
that cannot fail is worse than no gate.
622 lib + 154 integration, 0 failed.
Every SVG floor plan containing a rotated element was wrong. arch_svg read
only transform.translation and transform.scale, at all six projection
sites, so a wall yawed 90 degrees exported axis-aligned. A 2 x 6 footprint
came out 2 x 6 instead of 6 x 2.
This is the worst failure mode of the rotation bugs found in this project:
the export succeeds, the file opens cleanly in any viewer, and it shows a
different building. Nothing announces it.
I deferred this during Phase 1 as "a missing feature, not a wrong
calculation". That was the wrong call and PHASE1_STATUS.md now says so.
Silently emitting a different building is not a missing feature.
Fixed by routing all six sites -- box, cylinder/sphere/circle, polygon,
extruded polygon, CSG mesh and arc -- through one project_local() helper
that uses part_model_matrix, the renderer's own transform. Deliberately
not re-derived here: every hand-rolled rotation in an exporter has been
wrong in its own way (arch_stl destructured sin_cos backwards, arch_gltf
produced norm-0.125 quaternions, arch_pdf fed degrees to cos). This
exporter does not get another private copy.
The convention is subtler than it looks and I got it wrong twice while
writing the test. Both plan exporters map to world XZ then negate Z, so
local +X projects to (cos yaw, -sin yaw) and local +Z to
(sin yaw, -cos yaw). That is a reflection, not a rotation: the two model
axes are NOT perpendicular in plan space -- at yaw 30 they are 30 degrees
apart. My first two attempts asserted a rotation and then
perpendicularity, and both failed against real output. Verified against
rot_y_mat arithmetic before believing it. Written down in ARCHITECTURE.md
invariant 6, which previously recorded the bug.
Five tests, chosen so no single symmetry can hide a wrong fix:
- 0 degrees: baseline dimensions.
- 90 degrees: footprint must swap, 2x6 -> 6x2.
- 180 degrees: extents unchanged. Passes even when broken, and is here
precisely to stop a fix that rotates by the wrong factor from looking
correct on the 90 degree case alone.
- 45 degrees: no symmetry to hide behind; asserts the diagonal extent
8/sqrt(2). Tolerance is 0.011 because coordinates are emitted with two
decimals.
- The projection convention itself, cross-checked against arch_pdf.
Negative-tested: restoring the translation-and-scale projection fails
three of the five.
Also replaced an unchecked mesh.vertices[idx] index in the CSG arm with a
get() -- a malformed mesh would have panicked the exporter.
622 lib + 154 integration, 0 failed.
Phase 2.4, which I deferred during Phase 2 on a wrong assumption.
An unterminated CAD script hung the evaluator permanently. vm.eval is a
single blocking call, so `while true { s = s.translate(..) }` never
returns: the rebuild worker blocks forever, every later edit queues behind
it, and the UI sits on "Computing 3D model..." until the app is killed.
The source is user- and AI-authored, and an LLM will emit a loop like that
without much provocation.
Reproduced before fixing. A probe test was still running at 60 seconds,
and again at 90 with the budget removed.
I deferred this originally saying it needed "a cooperative check inside
the Makepad script VM or a watchdog that can kill and respawn the worker
thread", and called it a VM design decision. That was wrong, and I should
have read the VM before concluding it. makepad_script already provides
ScriptRunBudget::from_durations(soft, hard, sample_interval), checked in
run_core against a sampled Instant. The CAD module simply never set it.
The fix is six lines in eval_cad_script_in_vm. PHASE2_STATUS.md now
records the bad call rather than quietly marking the item done.
Only the hard deadline is used. A soft hit sets TimeBudgetYield, which
expects a host that drives the VM back to completion; there is no such
driver here, so a soft deadline would present as a script that silently
produced nothing at all. Soft is set equal to hard so the budget can only
ever produce a real, reported error. The previous budget is saved and
restored, so one evaluation cannot starve the next.
5 seconds, sampled every 4096 instructions. Generous deliberately: a real
document is a few hundred CSG ops and finishes in milliseconds, so
anything still running is a runaway rather than a slow model.
Result: the same script now returns "script time budget exceeded" in 5.2s.
Three tests. The runaway case is #[ignore]d because it burns the whole
budget by design; the other two guard the ways a timeout typically breaks
things -- that a normal script is unaffected across repeated evaluations
(catching a budget that is not reset), and that a syntax error still
reports itself rather than being misattributed to the timeout.
CI gate added and negative-tested: losing the budget line reintroduces a
hang that no test failure announces, only a frozen app.
617 lib + 154 integration, 0 failed.
The module had two independent paths from a node to triangles.
ensure_part_geometry called part.build_solid() directly; the exporters and
pick_part went through MeshCache::get_or_build -> CadSolid::build_mesh().
Neither saw the other's work, so a part that had just been exported or
hovered was still re-meshed from scratch for the GPU upload.
Proved they agree before merging them. pipeline_equivalence_tests builds
all 12 CadSolid variants through both paths and compares vertex positions,
triangle indices and winding exactly. They match. A second test asserts
the variant list has 12 entries, so adding a CadSolid variant without
covering it fails the build rather than silently narrowing the guarantee.
Both negative-tested: swapping width/height in build_solid's Rect2D arm
fails with "Rect2D: vertex 0 differs: (-1.5, -2, 0) vs (-2, -1.5, 0)";
dropping a variant from the list fails the coverage test.
The change itself: build_mesh_buffers now takes &TriMesh rather than
&Solid -- it only ever read solid.mesh() -- and ensure_part_geometry pulls
from the shared MeshCache. Added cad_mesh_data_from_mesh and
part_mesh_buffers_from_mesh as the mesh-taking entry points; the
Solid-taking ones remain as thin wrappers for callers that hold a Solid.
Measured, and the number is smaller than the section title suggests:
2x on a warm cache (boxes 0.27 -> 0.178 us/part, extruded 24-gons 1.11 ->
0.516 us/part), and *negative* on a cold one -- 0.79 vs 0.27 us/part,
because a first build now pays a ParamHash and a map insert. That is
recorded in BENCH_BASELINE.md with a note not to "optimise" it back by
special-casing cheap primitives.
The reason to do it anyway is correctness, not speed. Two match arms over
the same enum drift, and this drift would have been invisible: no crash,
no failing test, just a preview that quietly disagreed with the exported
file. Now there is one pipeline and a test that fails if it forks again.
build_mesh and build_solid both still exist because a Csg node needs a
real Solid to compose with. ARCHITECTURE.md's "Two meshing pipelines"
section, which told readers to apply every meshing change twice, is now
"One meshing pipeline".
615 lib + 154 integration, 0 failed.
Measured first. bench_viewport_snapshot_sync_per_frame puts the cost of
one sync frame at 98.5 us for 100 parts: 21.2 us of Vec<CadNode> deep
copies and 77.2 us of forced scene rebuilds. A generation comparison over
the same data is 0.051 us. Dragging a part sets script_dirty on every
MouseMove, so that was the steady-state per-frame cost of a drag.
Three separate wastes, each removed:
1. The source viewport was written back to. sync took a snapshot from the
dirty viewport and then handed it to all three, including the one it
came from -- a deep copy plus a full cache reset to install state that
viewport already had. It now pushes to the other two only.
2. Destinations were re-pushed unconditionally. They now compare
parts_generation() against the source's and skip if equal. During a
drag the same list was being reinstalled every frame; a frame where
nothing changed now costs three integer comparisons.
3. replace_parts_snapshot cleared part_geoms and the entire mesh cache.
The part_geoms.clear() was the expensive one and it is not in the
benchmark: it dropped every uploaded GPU buffer, so the next draw had
to re-mesh and re-upload the whole scene through ensure_part_geometry.
Now retains geometry for ids that survive the replacement.
The clear_mesh_cache() was redundant. MeshCache keys on
(NodeId, ParamHash) and ParamHash covers the solid discriminant, all
its parameters and the full transform, so a node whose geometry
changed misses on its own. ARCHITECTURE.md invariant 2 has said this
was unnecessary since it was written; this removes the first of them.
Three tests, each negative-tested by reintroducing the defect:
- mesh_cache_self_invalidates_on_parameter_and_transform_edits proves the
claim that justifies dropping the wipe. Deleting the transform hashing
from ParamHash makes it fail.
- replace_all_bumps_the_generation_so_destinations_can_compare pins the
precondition for the skip. Removing the bump makes it fail, which is
the failure mode that matters: a stale generation would make the sync
silently drop a real edit.
- geometry_is_retained_for_surviving_ids_only covers the retain
predicate. Geometry needs a live Cx, so this tests the id set logic --
keeping a buffer for a deleted id, or dropping one for a survivor.
Also collapsed six copies of the widget-borrow chain into with_viewport,
taking a callback rather than returning a guard: the WidgetRef is a
temporary, so returning a borrow of it does not compile (E0515, the same
shape as the let-else bug fixed in cf29ba7).
Not done: the three viewports still hold three copies of the parts list.
Sharing one Rc<RefCell<CadDocument>> and deleting the snapshot pair is
the rest of 5.2. This bounds the cost of the copy in the meantime, and
invariant 4 now says so.
BENCH_BASELINE.md records that this benchmark measures the primitives,
not the widget method, so its numbers do not move when the sync path is
fixed. It sizes the prize; the behaviour tests verify the change.
613 lib + 154 integration, 0 failed.
origin/main did not compile: E0716 in exchange_grid_with_model.
let Some(mut grid) = self.view.widget(cx, ids!(grid))
.borrow_mut::<SpreadsheetGrid>() else { return false; };
`widget()` returns a temporary WidgetRef and `borrow_mut` borrows from
it. In a `let ... else`, temporaries in the scrutinee are dropped at the
end of the statement rather than living to the end of the enclosing
block as they do in `if let`. So `grid` outlives what it borrows from.
The three neighbouring call sites use `if let` and are unaffected, which
is why this reads as correct next to them.
Bound the WidgetRef to a local first, with a comment recording the
`if let` / `let else` difference so the next copy-paste from a
neighbouring line does not reintroduce it.
Collapsing the nine duplicated properties-panel blocks exposed that all
nine were silent no-ops, and so was the Extend tool.
CadNode::pos/rot/size return Vec3f BY VALUE. `p.pos().x = val` therefore
assigns to a temporary and discards it. It compiles, warns about nothing,
and produces a control that does nothing: type a position into the panel
and the part does not move. Confirmed with a standalone repro before
touching anything. Eleven call sites, all wrong the same way:
- the nine position/size/rotation inputs in workspace.rs
- viewport.rs Extend, which carefully computes new_w/new_h and throws
both away, so the tool has never extended a part
Fixed by reading into a local, mutating it and calling set_pos/set_rot/
set_size, which have existed in cad_scene.rs the whole time.
The duplication is what hid this. Nine copies of a 25-line block, each
differing in one token, is not a thing anyone reads closely. Phase 4.3
was scheduled as tidying; it turned out to be the only reason the bug
was found. The blocks now call one helper,
apply_field_to_selected_part(cx, value, set), which also fixes the other
hazard of the copy-paste: each block had to remember four separate
invalidation steps (part_geoms.remove, invalidate_node, mark_scene_dirty,
script_dirty), and missing the first one fails silently -- the part keeps
rendering its old shape until something else evicts it. part_geoms.remove
call sites in workspace.rs: 13 -> 5.
Three defences, each negative-tested by reintroducing the defect:
- cad_scene::size_tests::setters_write_through_but_getters_are_copies
asserts both halves: that writing through the getter does NOT reach the
node, and that the set_* methods do. It documents the trap rather than
just guarding against it.
- workspace::properties_panel_setter_tests applies all nine closures
exactly as the call sites do and asserts each reaches its own axis and
leaves the other two groups alone -- so a wrong-axis copy-paste fails
too. Verified: restoring one broken closure fails with
"size.y: setter did not reach the node (got 1)".
- A CI grep for `.pos()/.rot()/.size().[xyz] =` outside comments.
Verified it fires on the restored Extend defect.
610 lib + 154 integration, 0 failed.
src/.../cad/tests.rs -> tests/cad_integration.rs. It was 2,840 lines of
integration tests living inside the library, compiled into every `--lib`
build and able to reach anything in the crate.
The move is worth more than the line count suggests, because a test
target compiles as a separate crate and so sees only the public API. That
turned an invisible question into a compile error: six items would have
needed `pub` for the file to build in its new home —
cad_mesh_data_from_solid, part_mesh_buffers, CommandBorrows,
CadRenderMode, DrawingState, SnapSettings, plus the private fields of the
last two.
Visibility was not widened. Promoting editor internals to a public
contract so a file can sit in a different directory is the wrong trade,
and `pub` is far harder to take back than to grant. Instead the 16 tests
that reach those items moved to where the items live:
- viewport.rs gains `mod viewport_helper_tests` (10 tests: the mesh
producers and the plain-data helpers).
- mod.rs gains `mod editor_state_tests` (6 tests: SnapSettings polar
defaults, DrawingState beam sections, CadEditorActivePane).
Two of the relocated tests are worthless and are now visible as such:
cad_render_mode_variants and command_borrows_fields_accessible assert
that a type exists and derives Debug, which the compiler already
guarantees. Left in place rather than deleted in the same commit as a
move; they are for the Phase 6 sweep.
The rule and its rationale are now written down in ARCHITECTURE.md under
"Where a test goes", alongside corrected LOC figures for the six files
that changed size since the table was written.
CI gained a step. The existing job ran only `--lib`, which does not build
a test target, so all 154 relocated tests would have run in no pipeline.
The step names cad_integration explicitly instead of testing the whole
crate, because tests/cost_estimator.rs and tests/cost_estimator_ui.rs do
not compile — pre-existing upstream breakage, verified against a clean
checkout, documented in a comment with instructions to fold them in once
fixed.
Test names diffed before and after: 0 lost, 12 gained (the Phase 4.4
characterization tests). 608 lib + 154 integration = 762.
Removed 73 "v1".."v19" references. The rule applied: keep the what,
delete the what-it-used-to-be. A comment saying a cache "now keys on
(NodeId, ParamHash) instead of just NodeId" tells a reader about a
decision made in a version they cannot see; the same comment saying it
keys on (NodeId, ParamHash) so a parameter edit produces a fresh mesh
tells them why the code in front of them is shaped that way.
Three headers were not merely useless but actively wrong, because they
described mechanisms deleted in earlier phases:
- mod.rs: 40 lines of "Rewrite status (v4)" whose last bullet documented
the SceneCache length heuristic ("rebuilds if parts.len() differs from
the cached scene's node count") as a live safety net. Phase 5.1
replaced that with generation tracking. Replaced with a short note on
why the file exists at all (Script derive must sit next to script_mod!)
and a pointer to ARCHITECTURE.md, which has the tests to keep it honest.
- cad_scene.rs: 44 lines enumerating the 24 compile errors fixed during a
past integration, including item 7 describing the DofConstraint bridge
deleted in Phase 4.5. Replaced with what a CadScene actually is.
- commands.rs: a "Migration strategy" section explaining how the module
coexists with "the existing CadCommand enum in mod.rs (line ~3538)" and
how CadCommandWrapper adapts it. That enum was deleted in Phase 4.1;
the wrapper never existed. Replaced with the current design.
Also deleted the orphaned "Cross-impl: legacy DofConstraint" banner in
cad_scene.rs, left behind when the From impls under it were removed.
The one surviving vN match is math.rs "Triangle: v0, v1, v2", which is
vertex naming.
762 tests pass, unchanged. Comment-only except for the three headers.
Three pairs of near-identical functions collapsed into one each, behind a
parameter naming the difference. No behaviour change: 12 characterization
tests were written first to pin the current output of every pair, and each
merge was negative-tested by reintroducing the defect and confirming the
tests fail.
- cad_mesh_data_from_solid / part_mesh_buffers were two copies of the same
120-line triangle walk. They differ in exactly two ways, now named:
MeshSpace (ViewNormalised recentres and fits to 1.75 units for the
single-solid preview; Model keeps model coordinates for scene parts,
where rescaling would break the layout) and Winding (the scene path
emits p0,p2,p1). The winding difference is preserved rather than
"fixed" — the shader disables backface culling so both render the
same, and nothing in the code proves which the depth paths expect.
Naming it makes it reviewable instead of invisible.
- extract_cad_script / extract_streaming_cad_script differed only in how
they treat an incomplete response, now a ResponseState parameter. The
streaming path keeps trailing whitespace, takes the body of an unclosed
fence, and seeks the first script token past any preamble; the complete
path trims both ends and treats an unclosed fence as unfenced. The
script-token list is now a named const next to the enum documenting
that it tracks system_prompt.md.
- toggle_view_mode carried its own copy of set_view_mode's gesture-state
reset (part/view/pan dragging, touches, pinch, orbit anchor, 2D pan).
It now computes the target and delegates. The copies had already
diverged: toggle called cx.redraw_all() without self.area.redraw(cx).
The characterization tests are worth more than the merge. They document
that the two mesh producers disagree on winding and that the two
extractors disagree on trailing whitespace — facts that were previously
only discoverable by diffing 120 lines by eye.
`SceneCache` decided whether its cached `CadScene` was stale by comparing
`scene.node_count()` against `parts.len()`. That is a guess, and its own
doc comment called it "a safety net for missing `mark_dirty()` calls" —
the real contract was "call `mark_dirty()` after every mutation", enforced
only by discipline across ~15 write sites and already violated by the
drawing tools, which push nodes directly.
The guess has a blind spot: delete one node and add another in the same
frame and the length is unchanged, so the cache reports clean and hands
back a scene describing the previous contents. Exports and picking then
run on stale geometry.
`CadViewport.parts` is now a `PartsStore` — the `Vec<CadNode>` plus a
monotonic `generation` that every mutating method bumps. You cannot get
`&mut` to the nodes without going through a method that bumps it, so
`SceneCache::scene_for(&store)` can compare generations exactly. Both the
length heuristic and the manual-`mark_dirty` requirement are gone as a
class, not fixed case by case.
`scene(&[CadNode])` is kept for callers that hold no store (tests, the
export path's detached copies) and now always rebuilds: without a
generation there is nothing to compare, and guessing is what caused the
bug.
Migration is incremental by design. `as_mut_vec()` is an escape hatch that
bumps unconditionally; three call sites still use it rather than turning
this into one 118-site commit. Sharing one document across the three
viewports (5.2) and retiring the remaining `mark_dirty` calls are separate
changes.
Also converted 10 duplicated `vp.parts.iter_mut().find(..)` blocks in
workspace.rs to the checked accessor.
Tests: 741 -> 750 passing, 0 failing. 9 new, including the delete-plus-add
case the old heuristic missed and an in-place edit with no `mark_dirty()`;
both were verified to fail against the restored length check.
