Add adversarial code review 2026-09-21

Documents patterns that could hide bugs or silently swallow errors: - Parser wildcards that ignore unknown grammar rules (F01) - Formula parser defaults unknown operators (F02) - Worker seed fetch swallows errors (F03) - WebSocket drops messages during state transitions (F04) - Effect error handling asymmetries (F05-F06) - Test coverage gaps for new projection types (F08) Review follows repository doctrine from .claude/skills/code-review/SKILL.md Co-authored-by: Ed Langley <fiddlerwoaroof@users.noreply.github.com>
2026-09-21 20:40:46 +00:00
parent bb69a157e0
commit 5225a3e0f8
1 changed files with 312 additions and 0 deletions
@@ -0,0 +1,312 @@
 # 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
 ### F01: 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"
 ---
 ### F02: 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"
 ---
 ### F03: 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"
 ---
 ### F04: 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"
 ---
 ### F05: `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"
 ---
 ### F06: `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"
 ---
 ### F07: `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"
 ---
 ### F08: 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"
 ---
 ### F09: `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"
 ---
 ### F10: 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"
 ---
 ## 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.
 - **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.
 ---
 ## Recommendations
 1. **Replace parser wildcards with exhaustive matches** (F01): Change `_ => {}` to explicit ignores of known-irrelevant rules, so new rules cause compile errors.
 2. **Make formula parser explicitly exhaustive** (F02): Match all grammar alternatives explicitly so new ones fail to compile.
 3. **Add logging or status reporting for seed fetch** (F03): At minimum, log fetch failures to the console.
 4. **Warn on dropped socket messages** (F04): Consider queuing or at least logging messages sent when the socket isn't open.
 5. **Audit `let _ =` patterns**: Each discarded `Result` should either be intentional (documented) or converted to an error report.
 ---
 _End of adversarial review._