improvise/docs/adversarial-review-2026-09-21.md

# Adversarial Code Review — 2026-09-21

_Posted by Claude (Opus 4.5), reviewing the whole-repo sweep for correctness and conformance against this repository's doctrine._

## Summary

This review covers `improvise-core`, `improvise-formula`, `improvise-io`, `improvise-protocol`, `improvise-command`, `improvise-wasm-client`, `improvise-worker-server`, `improvise-ws-server`, and `web/`. It identifies patterns that could hide bugs or silently swallow errors, focusing on issues the doctrine specifically warns about.

**Exclusions (already filed or known intentional):**
- improvise-16af, improvise-iobc, improvise-9cma, improvise-tg27, improvise-x0dc, improvise-l3hm
- Upstream Key black holes for `[`/`]`/drill etc.
- Aggregate Profit `i` decline (fixed)

---

## Findings

### 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::<f64>() {
    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"

---

#### F04: Auto-persist failure not propagated to client

**Severity:** P1 (critical)

**Location:** `crates/improvise-worker-server/src/worker.rs`, function `on_frame`, lines 52-59

**What's wrong:** If `effect::Save` fails (e.g., OPFS quota exceeded, handle closed), the dirty flag stays `true` but the failure is only reported to `status_msg` (which the worker-server has no UI to show). The client sees `dirty: true` projected but doesn't know the save failed vs. is pending. In the browser, this could lead to **data loss on page close** if the user believes auto-save is working.

**How to verify:** Inject a storage backend that fails writes after N successful ones. Edit the document past the failure point. Observe that `dirty: true` is projected but no explicit save-failure indication reaches the client.

**Suggested bead title:** "Auto-persist failure in worker not communicated to client"

---

#### F05: `dispatch` silently swallows unresolvable command names

**Severity:** P1 (critical)

**Location:** `crates/improvise-command/src/command/keymap.rs`, line 37

**What's wrong:** The `interactive()` method can return `Err` (e.g., argument parse failures, mode mismatch for `commit-buffer item`). Swallowing it with `.ok()?` makes these failures silent — the user presses a key and nothing happens, no error message. This violates `doctrine(an_upstream_key_is_a_black_hole)`.

**How to verify:** Bind a key to `commit-buffer item` in Normal mode (not ItemAdd mode). Press the key. The command fails to construct but no status message appears.

**Suggested bead title:** "dispatch swallows interactive() errors silently"

---

### P2 Findings (Medium)

#### F04: Parser `_ => {}` wildcards silently ignore unknown grammar rules

**Severity:** P2 (medium)

**Location:** `crates/improvise-io/src/persistence/parse.rs`, lines 180, 293, 315

**What's wrong:**
The `.improv` parser has multiple `_ => {}` arms that silently skip any grammar rule the walker doesn't explicitly handle:

```rust
// Line 180 in walk_category_section
_ => {}

// Line 293 in walk_view_section  
_ => {}

// Line 315 in walk_md
_ => {}
```

If the pest grammar (`improv.pest`) is extended with new rules, the parser will silently ignore them rather than failing loudly. This violates the principle that a grammar/parser mismatch should be visible.

**Doctrine violated:** `doctrine(check_the_checker)` — a parse that silently ignores valid grammar rules is indistinguishable from a parse that rejects them.

**How to verify:** Add a new rule to `improv.pest` (e.g., a new directive like `color_line`), emit it in `format_md`, and observe that `parse_md` round-trips without error but the data is lost. The grammar accepts it, the walker drops it.

**Suggested bead title:** "Parser wildcards silently ignore unknown grammar rules"

---

#### F05: Formula parser defaults unknown operators to specific values

**Severity:** P2 (medium)

**Location:** `crates/improvise-formula/src/parser.rs`, lines 158-159 and 189

**What's wrong:**
The formula parser uses `_ =>` fallback arms that default unknown values to specific operators:

```rust
// Line 158-159: Unknown aggregate becomes COUNT
_ => AggFunc::Count

// Line 189: Unknown comparison becomes Ge  
_ => BinOp::Ge
```

If the grammar adds a new aggregate function (e.g., `MEDIAN`) or comparison operator, the parser will silently treat it as `COUNT` or `>=` rather than failing.

**Doctrine violated:** Exhaustive matches should be preferred over wildcards so adding a variant produces compile errors at every site that needs updating.

**How to verify:** Add `| ^"MEDIAN"` to `agg_func` in `formula.pest`, parse `MEDIAN(Revenue)`, observe it evaluates as `COUNT(Revenue)` with no error.

**Suggested bead title:** "Formula parser defaults unknown operators silently"

---

#### 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: Vacuous test: `law_toggle_group_involution` doesn't apply effects

**Severity:** P2 (medium)

**Location:** `crates/improvise-command/src/command/cmd/grid.rs`, lines 26-36

**What's wrong:** The test asserts `first == second` where both are the debug output of `cmd.execute(&ctx)` called twice with the **same unchanged context**. It claims to test involution (toggle twice = identity) but never applies the first effect before calling execute again. The `ctx` is immutable — `CmdContext` is borrowed. The test proves "two calls return structurally identical effects" which is trivially true for any deterministic pure function.

**Doctrine violated:** `doctrine(fold_tests_must_test_order_not_the_axiom)` — the test asserts a tautology.

**How to verify:** Inject a fault where `ToggleGroupAtCursor` returns different effects on odd/even calls. The test still passes because it doesn't track state changes.

**Suggested bead title:** "[test] toggle-group-involution test doesn't apply effects between calls"

---

#### F16: Vacuous test: `law_sequence_associativity` proves a tautology

**Severity:** P2 (medium)

**Location:** `crates/improvise-command/src/command/cmd/navigation.rs`, lines 388-431

**What's wrong:** The test calls three commands with the same context and concatenates their effects in two groupings `(a+b)+c` vs `a+(b+c)`. Since effects are just pushed to a `Vec`, concatenation is trivially associative — it's the definition of a list. The test cannot fail regardless of what commands do.

**Doctrine violated:** `doctrine(fold_tests_must_test_order_not_the_axiom)` — asserting that list concatenation is associative is the axiom, not a testable property.

**How to verify:** Replace all three commands with `panic!()` calls and the test still passes if effects are non-empty Vec concats.

**Suggested bead title:** "[test] sequence-associativity test proves a mathematical tautology"

---

#### F17: ServerFrame serialization failure silently dropped

**Severity:** P2 (medium)

**Location:** `crates/improvise-ws-server/src/transport.rs`, writer task, lines 179-183

**What's wrong:** A `ServerFrame` that fails to serialize is silently dropped with `continue`. The client never receives it, creating state divergence. The comment claims "unreachable" but this is an assertion the compiler cannot verify.

**How to verify:** Add a `ServerFrame` variant containing a type that fails `serde_json::to_string`. Observe that the frame is lost without logging.

**Suggested bead title:** "ServerFrame serialization failure silently dropped in ws transport"

---

#### F18: Unparseable ClientFrame text messages silently ignored

**Severity:** P2 (medium)

**Location:** `crates/improvise-ws-server/src/transport.rs`, function `next_client_msg`, lines 240-248

**What's wrong:** Text messages that don't parse as `ClientFrame` are silently dropped with no logging. This hides protocol mismatches, version skew, or corruption. A client sending malformed commands gets no feedback.

**How to verify:** Send `{"Unknown": {}}` as a text message over the WebSocket. Observe no response and no log entry.

**Suggested bead title:** "Unparseable ClientFrame messages silently dropped without logging"

---

#### F19: ClientFrame for closed document produces no response

**Severity:** P2 (medium)

**Location:** `crates/improvise-ws-server/src/registry.rs`, function `handle`, lines 193-197

**What's wrong:** If a document is closed (evicted or never existed), the client's frame produces an empty response. The client receives nothing — no rejection, no error. A client might continue composing against a document the server no longer holds.

**How to verify:** Connect a client to document A, then have the document close. Send a command — observe no response.

**Suggested bead title:** "ClientFrame for unregistered document silently dropped"

---

#### F20: Debug-string assertion tests cannot verify effect application

**Severity:** P2 (medium)

**Location:** Multiple files in `crates/improvise-command/src/command/cmd/`

**What's wrong:** Numerous tests assert that the debug representation of an effect list contains a substring (e.g., `dbg.contains("AddCategory")`). This verifies the **name** of an effect type is present but not that the effect's fields have correct values, that the effect actually applies correctly, or that the effect isn't shadowed by a later effect.

**Doctrine violated:** `doctrine(check_the_checker)` — the tests verify syntax presence, not semantic correctness.

**How to verify:** Change an effect's fields to incorrect values. The test still passes because the type name is present.

**Suggested bead title:** "[test] Debug-string tests verify effect names, not correctness"

---

#### F21: Worker seed fetch silently swallows errors

**Severity:** P2 (medium)

**Location:** `web/worker.js`, lines 19-22

**What's wrong:**
```javascript
async function seedBytes() {
  try {
    const res = await fetch(SEED);
    if (!res.ok) return undefined;
    return new Uint8Array(await res.arrayBuffer());
  } catch (_) {
    return undefined;  // Error silently becomes "no seed"
  }
}
```

A network error, CORS failure, or malformed response is indistinguishable from "no seed file exists." The worker proceeds with an empty model rather than reporting the actual problem.

**How to verify:** Set `SEED` to a URL that returns a 500 error or is CORS-blocked. The worker starts with an empty model and no indication that the seed fetch failed.

**Suggested bead title:** "Worker seed fetch errors are silently swallowed"

---

### P3 Findings (Low)

#### F16: WebSocket messages dropped when socket not open

**Severity:** P2 (medium)

**Location:** `web/app.js`, lines 133-134

**What's wrong:**
```javascript
send: (text) => {
  if (socket.readyState === WebSocket.OPEN) socket.send(text);
  // No else: message is silently dropped
},
```

If the socket is closing or connecting, messages (potentially including user edits) are silently discarded with no feedback to the user.

**How to verify:** Trigger a socket state transition (reconnection, network hiccup) while the user is typing. The edit command is silently lost. No status message, no retry.

**Suggested bead title:** "WebSocket transport silently drops messages during state transitions"

---

#### F17: `AddItem` effect swallows errors into status bar only

**Severity:** P3 (low)

**Location:** `crates/improvise-command/src/ui/effect.rs`, lines 102-110

**What's wrong:**
```rust
impl Effect for AddItem {
    fn apply(&self, app: &mut App) {
        if let Err(e) = app.model_state.workbook.add_item(...) {
            app.view_state.status_msg = e.to_string();
            // No abort_effects = true, unlike AddCategory
        }
    }
}
```

`AddCategory` sets `abort_effects = true` on failure (line 87), aborting the batch. `AddItem`, `AddItemInGroup`, and `AddFormula` (parse error branch) only set a status message but let the batch continue. A sequence like `[AddItem("BadCat", "X"), SetCell(...)]` will show an error but still apply the SetCell.

**Doctrine context:** `invariant(an_abort_stops_the_chain_not_the_bookkeeping)` discusses abort semantics. The asymmetry here is whether downstream effects that depend on the item existing should run.

**How to verify:** Run a `Binding::Sequence` where the first command adds an item to a non-existent category and the second does something that assumes it exists. The second command runs.

**Suggested bead title:** "AddItem/AddFormula errors don't abort the effect batch"

---

#### F18: `delete_view` error silently discarded

**Severity:** P3 (low)

**Location:** `crates/improvise-command/src/ui/effect.rs`, line 321

**What's wrong:**
```rust
let _ = app.model_state.workbook.delete_view(&self.0);
```

The `let _ =` explicitly discards the `Result`. If deleting the view fails (e.g., it's the last view), no error reaches the user. The code below assumes the deletion succeeded.

**How to verify:** Attempt to delete the only view in a workbook. No error message appears, but the view still exists.

**Suggested bead title:** "DeleteView effect discards error result"

---

#### F19: `resolve` returns empty vec via `unwrap_or_default` on error paths

**Severity:** P3 (low)

**Location:** `crates/improvise-protocol/src/resolve.rs`, line 101

**What's wrong:**
```rust
resolved.unwrap_or_default()
```

Several resolution functions return `Option<Vec<Command>>`. When resolution fails (e.g., the click didn't hit anything), `unwrap_or_default()` turns it into an empty vec. This is often correct ("nothing to do"), but some callers may want to distinguish "resolved to nothing" from "resolution failed."

**Context:** This is borderline intentional — "no commands" is the right answer for most missed clicks. But it's worth noting that failures and empty successes are conflated.

**Suggested bead title:** (Low priority — may be intentional) "Click resolution conflates failures with empty results"

---

#### F20: Test `_ => {}` in socket.rs ignores unexpected projections

**Severity:** P3 (low)

**Location:** `crates/improvise-ws-server/tests/socket.rs`, lines 78-82 and 4978-4982

**What's wrong:**
```rust
for projection in &snapshot.seed {
    match projection {
        Projection::Grid(update) => grid.apply(update),
        Projection::Basis(update) => basis.apply(update),
        _ => {}  // Other projections silently ignored
    }
}
```

The test only handles Grid and Basis projections. If a new projection type is added (e.g., `Projection::Views`), the test won't see it. The socket works, but the test coverage has a gap.

**Doctrine violated:** `doctrine(check_the_checker)` — a test that ignores some inputs cannot verify them.

**How to verify:** Add a new `Projection` variant, emit it in the snapshot, observe the test still passes (it doesn't check for it).

**Suggested bead title:** "Socket tests ignore non-Grid/Basis projections"

---

#### F21: `KeyCode` formatting falls through to Debug

**Severity:** P3 (low)

**Location:** `crates/improvise-protocol/src/keymap.rs`, lines 42-46

**What's wrong:**
```rust
match code {
    KeyCode::PageUp => "PgUp".to_string(),
    KeyCode::PageDown => "PgDn".to_string(),
    KeyCode::F(n) => format!("F{n}"),
    _ => format!("{code:?}"),  // Debug format for unknown keys
}
```

New `KeyCode` variants will show as their Debug representation (e.g., `KeyCode::Media(Play)`) in the UI rather than a human-friendly string. This is a user-facing display issue, not a correctness bug.

**Suggested bead title:** "KeyCode display falls back to Debug for unhandled variants"

---

#### F22: CategoryKind defaults Regular for unknown category names

**Severity:** P3 (low)

**Location:** `crates/improvise-core/src/model/category.rs`, lines 98-102

**What's wrong:**
```rust
pub fn of_name(name: &str) -> Self {
    match name {
        INDEX_CATEGORY => CategoryKind::VirtualIndex,
        DIM_CATEGORY => CategoryKind::VirtualDim,
        MEASURE_CATEGORY => CategoryKind::VirtualMeasure,
        _ => CategoryKind::Regular,
    }
}
```

This is probably correct (any non-virtual category is Regular), but if a new virtual category is added, it will be treated as Regular until this function is updated.

**Suggested bead title:** (Very low priority) "CategoryKind::of_name defaults unknown names to Regular"

---

#### 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()?`

Multiple locations use `.ok()?` to convert errors to `None` and early-return. This is often appropriate (e.g., parsing optional data), but some uses may hide real failures:

- `crates/improvise-io/src/opfs.rs` lines 39-41, 185-187: OPFS operations that fail silently
- `crates/improvise-command/src/command/keymap.rs` line 37: Command resolution failures become "no command"

These are worth auditing individually but don't rise to the level of individual findings.

### `.unwrap_or_default()` masking Option contents

Heavy use throughout the codebase. Most are appropriate (empty string, empty vec, zero), but some may mask meaningful `None` values. No specific bugs found, but the pattern density suggests caution when adding new code.

---

## Tests Not Finding (Vacuousness check)

The test suite appears well-constructed against the repository's own doctrine. No obviously vacuous tests were found where assertions would pass regardless of behavior. The lockstep tests in particular show good discipline about driving both directions.

The one area to watch: tests that use `_ => {}` in their fold/apply helpers (like F08) may silently ignore new variants.

---

## Not Findings (Reviewed and correct)

- **`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.

- **`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 | 5 | CSV NaN→0 corruption, Optimistic fold no rollback, Auto-persist failure silent, dispatch swallows errors |
| P2 | 18 | Parser wildcards, Socket CLOSING throw, Vacuous involution/associativity tests, ServerFrame serialization drops |
| P3 | 10 | Message drops, Error discards, Minor wildcards |
| **Total** | **33** | |

---

## Recommendations

### P1 Fixes (Critical)

1. **Fix CSV data corruption** (F01): The NaN/Infinity→0 conversion silently corrupts data. Either preserve as text with a warning or refuse to import.

2. **Add rollback for optimistic folds** (F02): When a server rejects a locally-folded command, the client state must be corrected.

3. **Propagate auto-persist failures to client** (F04): The worker must communicate save failures, not just set `dirty: true`.

4. **Report dispatch errors** (F05): The `.ok()?` pattern swallowing command construction errors must show a status message.

### P2 Fixes (Medium)

5. **Replace parser wildcards with exhaustive matches** (F06): Change `_ => {}` to explicit ignores of known-irrelevant rules.

6. **Fix vacuous test assertions** (F10, F11): Tests asserting `>= 1.0` or `!is_empty()` pass when implementations are broken.

7. **Fix vacuous "law" tests** (F15, F16): The involution and associativity tests prove tautologies, not properties.

8. **Guard socket send for CLOSING state** (F08): Check for both OPEN and not-CLOSING to avoid uncaught exceptions.

9. **Add circuit breaker for render failures** (F09): Don't retry infinitely when render_model throws.

10. **Log dropped frames in ws-server** (F17, F18, F19): Serialization and parse failures should be logged for debugging.

### General

11. **Audit `let _ =` patterns**: Each discarded `Result` should either be intentional (documented) or converted to an error report.

12. **Audit debug-string tests** (F20): Tests asserting `dbg.contains("EffectName")` should also verify the effect's fields.

---

_End of adversarial review._