diff --git a/docs/adversarial-review-2026-09-21.md b/docs/adversarial-review-2026-09-21.md index f4a67a74..3406d778 100644 --- a/docs/adversarial-review-2026-09-21.md +++ b/docs/adversarial-review-2026-09-21.md @@ -15,7 +15,77 @@ This review covers `improvise-core`, `improvise-formula`, `improvise-io`, `impro ## Findings -### F01: Parser `_ => {}` wildcards silently ignore unknown grammar rules +### P1 Findings (Critical) + +#### F01: CSV import silently converts NaN/Infinity to 0 + +**Severity:** P1 (critical) + +**Location:** `crates/improvise-io/src/import/csv_parser.rs`, function `parse_csv_field`, lines 128-131 + +**What's wrong:** +```rust +if let Ok(num) = field.parse::() { + return Value::Number( + serde_json::Number::from_f64(num).unwrap_or(serde_json::Number::from(0)), + ); +} +``` + +`serde_json::Number::from_f64()` returns `None` for NaN and Infinity. The `.unwrap_or(serde_json::Number::from(0))` silently converts these to 0. A CSV field containing `"NaN"`, `"Infinity"`, `"-Infinity"`, or `"inf"` will import as 0 with no warning — **data is silently corrupted**. + +**How to verify:** Import a CSV with a field value `"nan"` or `"inf"` and observe it imports as 0 rather than preserving as text or producing a warning. + +**Suggested bead title:** "CSV import silently converts NaN/Infinity fields to zero" + +--- + +#### F02: Optimistic fold is not rolled back on server rejection + +**Severity:** P1 (critical) + +**Location:** `crates/improvise-wasm-client/src/lib.rs`, function `Client::key`, lines 182-193 + +**What's wrong:** +```rust +match resolve(&self.state, event) { + KeyOutcome::Local(cmds) => { + for cmd in &cmds { + self.state.fold(cmd); // <-- State mutated HERE + } + Some(ClientFrame::Commands(cmds)) // <-- Then sent + } + // ... +} +``` + +The client folds commands optimistically before sending. If the server rejects (e.g., `Rejection::LayoutMoved`), `socket.rs::fold_frame` sets `last_rejection` and `status_msg` but does **NOT rollback** the already-folded state changes. + +Example: User presses `]` (page next), client folds `SetPageSelection`, server rejects because layout changed. Client's `page_coords` now disagrees with server's. + +**How to verify:** Send a command that the server will reject, observe client state has moved but server's hasn't. + +**Suggested bead title:** "Optimistic fold is not rolled back on server rejection" + +--- + +#### F03: Worker startup failure leaves silent dead state + +**Severity:** P1 (critical) + +**Location:** `web/worker.js`, lines 43-60 + +**What's wrong:** If `started` rejects (e.g., WASM init fails), the `.catch()` only logs. The `onFrame` listener is still installed and calls `handle()`, but `server` is `null`. Every frame accumulates in the error log with no UI indication beyond the eventual watchdog. If the watchdog somehow fails to arm (e.g., worker never posts), the page sits there dead. + +**How to verify:** Make `init()` or `start()` throw. Observe that frames accumulate in the error log with no UI indication beyond the eventual watchdog. + +**Suggested bead title:** "Worker startup failure leaves page in silent dead state" + +--- + +### P2 Findings (Medium) + +#### F04: Parser `_ => {}` wildcards silently ignore unknown grammar rules **Severity:** P2 (medium) @@ -45,7 +115,7 @@ If the pest grammar (`improv.pest`) is extended with new rules, the parser will --- -### F02: Formula parser defaults unknown operators to specific values +#### F05: Formula parser defaults unknown operators to specific values **Severity:** P2 (medium) @@ -72,7 +142,144 @@ If the grammar adds a new aggregate function (e.g., `MEDIAN`) or comparison oper --- -### F03: Worker seed fetch silently swallows errors +#### F06: Socket send can throw on CLOSING state + +**Severity:** P2 (medium) + +**Location:** `web/app.js`, lines 133-134 + +**What's wrong:** +```javascript +send: (text) => { + if (socket.readyState === WebSocket.OPEN) socket.send(text); +} +``` + +`socket.send(text)` throws if `readyState` is `CLOSING` (2), but the guard only checks for `OPEN` (1). During the brief CLOSING window, a send attempt would throw, propagate up to the keyboard handler, and potentially break the page. + +**How to verify:** Mock a WebSocket that transitions to CLOSING state. Call `transport.send()`. Observe the uncaught exception. + +**Suggested bead title:** "Socket send throws on CLOSING state, not guarded" + +--- + +#### F07: Render failure leaves `painted` stale, causing infinite retry + +**Severity:** P2 (medium) + +**Location:** `web/app.js`, lines 148-156 + +**What's wrong:** When `JSON.parse(client.render_model())` throws, the function returns early WITHOUT updating `painted`. The `model.revision === painted` early-return uses the OLD value. Every subsequent call to `repaint()` (triggered by ANY event) will attempt `render_model()` again and fail again — infinite retry loop. + +**How to verify:** Inject a permanently broken state where `render_model()` throws. Observe repeated error logs on every keystroke. + +**Suggested bead title:** "Render failure causes infinite retry loop" + +--- + +#### F08: Date detection samples only first 10 values, order-dependent + +**Severity:** P2 (medium) + +**Location:** `crates/improvise-io/src/import/analyzer.rs`, function `detect_date_format`, lines 67-76 + +**What's wrong:** The function only samples the first 10 values for date format detection. If the disambiguating value (e.g., `25/12/2025` which can only be day-first) appears after row 10, it's never seen and the wrong format is chosen. + +**How to verify:** Create a CSV with 15 rows where rows 1-10 are ambiguous dates like `01/02/2025` and row 11+ contains `25/12/2025` (unambiguously day-first). Watch it misclassify as `%m/%d/%Y`. + +**Suggested bead title:** "Date format detection ignores disambiguating values after row 10" + +--- + +#### F09: Mixed-type columns misclassify as Label + +**Severity:** P2 (medium) + +**Location:** `crates/improvise-io/src/import/analyzer.rs`, function `analyze_records`, lines 124-135 + +**What's wrong:** A column that is 99% numeric with 1 null cell becomes a Label rather than a Measure. The `all_numeric` check excludes `Value::Null`, so any sparse column (common in real CSVs) misclassifies. This loses data — the 99% that are numbers become stringified item names rather than numeric cell values. + +**How to verify:** Import a CSV where a "Revenue" column has one null row among 100 numeric rows. The entire column becomes a Label category. + +**Suggested bead title:** "CSV analyzer misclassifies mostly-numeric columns with nulls as Label" + +--- + +#### F10: Vacuous test: `count_aggregation` asserts >= 1.0 instead of exact value + +**Severity:** P2 (medium) + +**Location:** `crates/improvise-core/src/model/types.rs`, test `count_aggregation`, line ~1467 + +**What's wrong:** The test asserts `assert!(val >= 1.0)` when the expected value should be exactly `1.0`. `COUNT(Sales)` evaluated at `Region=East` counts exactly 1 matching cell. This assertion would pass for any value ≥ 1, including 2, 100, or 999. + +**Doctrine violated:** `doctrine(check_the_checker)` — a test that cannot fail when the implementation is wrong is not evidence. + +**How to verify:** Inject a fault: make `AggFunc::Count` return `values.len() as f64 + 999.0`. The test still passes. + +**Suggested bead title:** "[test] count_aggregation assertion is >= 1.0 instead of exact value" + +--- + +#### F11: Vacuous test: `records_mode_resolve_display` asserts non-empty only + +**Severity:** P2 (medium) + +**Location:** `crates/improvise-core/src/view/layout.rs`, test `records_mode_resolve_display_returns_values`, line ~810 + +**What's wrong:** The test asserts `!display.is_empty()` for the Region column value. The expected value is `"North"`. Asserting only non-emptiness would pass for any non-empty string including garbage like `"CORRUPTED"`. + +**How to verify:** Inject a fault: make `records_display` for Category columns return `"GARBAGE"`. The test still passes. + +**Suggested bead title:** "[test] records_mode_resolve_display asserts non-empty instead of expected value" + +--- + +#### F12: Non-exhaustive wildcard in `eval_bool_cached` + +**Severity:** P2 (medium) + +**Location:** `crates/improvise-core/src/model/types.rs`, function `eval_bool_cached`, line ~700 + +**What's wrong:** Uses `_ => Err("type".into())` instead of explicitly listing the arithmetic operators. If a new `BinOp` variant is added to `improvise_formula`, this match silently errors instead of producing a compile-time error. + +Compare with `eval_bool` (line ~187) which explicitly lists operators — the two should match in style. + +**How to verify:** Add a new `BinOp::Mod` variant to `improvise_formula`. The cached evaluator silently errors; the non-cached evaluator fails to compile. + +**Suggested bead title:** "eval_bool_cached uses wildcard match on BinOp instead of exhaustive arms" + +--- + +#### F13: Silent serialization failures in wasm exports drop valid operations + +**Severity:** P2 (medium) + +**Location:** `crates/improvise-wasm-client/src/lib.rs`, functions `on_key_event` (line 54), `typed_cell` (line 97), `clicked` (line 178) + +**What's wrong:** Each function uses `.ok()` on `serde_json::to_string()`. If serialization fails (unlikely but possible with unusual data), the operation returns `None`. The caller interprets `None` as "nothing to send" (e.g., cell not editable, click invalid) — not as "serialization failed". A valid edit or click would be silently dropped. + +**How to verify:** Inject a non-serializable value into the frame and observe no error reaches the JS caller. + +**Suggested bead title:** "serde_json failures in wasm exports are silent drops, not errors" + +--- + +#### F14: Editing mode with missing buffer renders empty overlay + +**Severity:** P2 (medium) + +**Location:** `crates/improvise-wasm-client/src/render.rs`, function `render_model`, lines 268-271 + +**What's wrong:** If `is_editing()` is true but no "edit" buffer exists, `editing` becomes `Some("")`. The page would render an empty editor overlay over a cell that has content. The user sees their typing replaced with nothing. + +**How to verify:** Fold a `ChangeMode(Editing)` without a preceding `SetBuffer{name:"edit",...}` and call `render_model()`. + +**Suggested bead title:** "editing mode with missing buffer renders empty overlay instead of cell content" + +--- + +#### F15: Worker seed fetch silently swallows errors **Severity:** P2 (medium) @@ -99,7 +306,9 @@ A network error, CORS failure, or malformed response is indistinguishable from " --- -### F04: WebSocket messages dropped when socket not open +### P3 Findings (Low) + +#### F16: WebSocket messages dropped when socket not open **Severity:** P2 (medium) @@ -121,7 +330,7 @@ If the socket is closing or connecting, messages (potentially including user edi --- -### F05: `AddItem` effect swallows errors into status bar only +#### F17: `AddItem` effect swallows errors into status bar only **Severity:** P3 (low) @@ -149,7 +358,7 @@ impl Effect for AddItem { --- -### F06: `delete_view` error silently discarded +#### F18: `delete_view` error silently discarded **Severity:** P3 (low) @@ -168,7 +377,7 @@ The `let _ =` explicitly discards the `Result`. If deleting the view fails (e.g. --- -### F07: `resolve` returns empty vec via `unwrap_or_default` on error paths +#### F19: `resolve` returns empty vec via `unwrap_or_default` on error paths **Severity:** P3 (low) @@ -187,7 +396,7 @@ Several resolution functions return `Option>`. When resolution fail --- -### F08: Test `_ => {}` in socket.rs ignores unexpected projections +#### F20: Test `_ => {}` in socket.rs ignores unexpected projections **Severity:** P3 (low) @@ -214,7 +423,7 @@ The test only handles Grid and Basis projections. If a new projection type is ad --- -### F09: `KeyCode` formatting falls through to Debug +#### F21: `KeyCode` formatting falls through to Debug **Severity:** P3 (low) @@ -236,7 +445,7 @@ New `KeyCode` variants will show as their Debug representation (e.g., `KeyCode:: --- -### F10: CategoryKind defaults Regular for unknown category names +#### F22: CategoryKind defaults Regular for unknown category names **Severity:** P3 (low) @@ -260,6 +469,48 @@ This is probably correct (any non-virtual category is Regular), but if a new vir --- +#### F23: Hidden items with `/` in name corrupt on round-trip + +**Severity:** P3 (low) + +**Location:** `crates/improvise-io/src/persistence/parse.rs` + +**What's wrong:** The `hidden:` line format is `hidden: Cat/Item`, and if `Item` contains `/`, the parser splits at the wrong position. A test exists documenting this bug. + +**How to verify:** Create an item named `"A/B"`, hide it, save, reload. The hidden state is lost or corrupted. + +**Suggested bead title:** "Hidden/collapsed items with slash in name corrupt on persistence round-trip" + +--- + +#### F24: `CellValue::as_f64` wildcard hides future variants + +**Severity:** P3 (low) + +**Location:** `crates/improvise-core/src/model/cell.rs`, function `as_f64`, line ~72 + +**What's wrong:** Uses `_ => None` wildcard. If a new `CellValue` variant is added (e.g., `Boolean`, `DateTime`), this silently returns `None` instead of forcing a decision. + +**How to verify:** Add a `CellValue::DateTime(f64)` variant. The code compiles without warning but silently drops DateTime values in numeric contexts. + +**Suggested bead title:** "CellValue::as_f64 uses wildcard match hiding future variants" + +--- + +#### F25: Connection hang has no watchdog coverage + +**Severity:** P3 (low) + +**Location:** `web/app.js`, lines 489-494 + +**What's wrong:** The watchdog is armed AFTER `await transport.ready`, so if ready never resolves (connection hangs forever), the watchdog is never armed and the user sees "connecting..." forever with no timeout. + +**How to verify:** Mock a WebSocket that never fires `open`. Observe that the page shows "connecting..." forever. + +**Suggested bead title:** "Connection hang has no watchdog coverage" + +--- + ## Patterns Observed ### Silent error swallowing via `.ok()?` @@ -289,23 +540,36 @@ The one area to watch: tests that use `_ => {}` in their fold/apply helpers (lik - **`reduce.rs` `_ => None` for basis checking**: Lines 60 and 73 return `None` for non-Model commands. This is correct — only `Command::Model` carries a basis to check. -- **Formula parser `_ => BinOp::Ge`**: While concerning in principle, the grammar explicitly lists exactly the operators the code handles, so the wildcard is actually dead code unless the grammar changes. - - **`EffectKind` has no default**: This is the doctrine working as intended — the compiler forces every effect to declare its kind. --- +## Summary Table + +| Severity | Count | Notable Examples | +|----------|-------|------------------| +| P1 | 3 | CSV NaN→0 corruption, Optimistic fold no rollback, Worker startup silent death | +| P2 | 12 | Parser wildcards, Socket CLOSING throw, Date detection sampling, Vacuous tests | +| P3 | 10 | Message drops, Error discards, Minor wildcards | +| **Total** | **25** | | + +--- + ## Recommendations -1. **Replace parser wildcards with exhaustive matches** (F01): Change `_ => {}` to explicit ignores of known-irrelevant rules, so new rules cause compile errors. +1. **Fix P1 data corruption**: The CSV NaN/Infinity→0 conversion (F01) silently corrupts data. Either preserve as text with a warning or refuse to import. -2. **Make formula parser explicitly exhaustive** (F02): Match all grammar alternatives explicitly so new ones fail to compile. +2. **Add rollback for optimistic folds** (F02): When a server rejects a locally-folded command, the client state must be corrected. -3. **Add logging or status reporting for seed fetch** (F03): At minimum, log fetch failures to the console. +3. **Replace parser wildcards with exhaustive matches** (F04): Change `_ => {}` to explicit ignores of known-irrelevant rules, so new rules cause compile errors. -4. **Warn on dropped socket messages** (F04): Consider queuing or at least logging messages sent when the socket isn't open. +4. **Fix vacuous test assertions** (F10, F11): Tests asserting `>= 1.0` or `!is_empty()` pass when implementations are broken. -5. **Audit `let _ =` patterns**: Each discarded `Result` should either be intentional (documented) or converted to an error report. +5. **Guard socket send for CLOSING state** (F06): Check for both OPEN and not-CLOSING to avoid uncaught exceptions. + +6. **Add circuit breaker for render failures** (F07): Don't retry infinitely when render_model throws. + +7. **Audit `let _ =` patterns**: Each discarded `Result` should either be intentional (documented) or converted to an error report. ---