diff --git a/docs/adversarial-review-2026-09-21.md b/docs/adversarial-review-2026-09-21.md index 3406d778..3a462ec5 100644 --- a/docs/adversarial-review-2026-09-21.md +++ b/docs/adversarial-review-2026-09-21.md @@ -83,6 +83,34 @@ Example: User presses `]` (page next), client folds `SetPageSelection`, server r --- +#### 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 @@ -279,7 +307,97 @@ Compare with `eval_bool` (line ~187) which explicitly lists operators — the tw --- -#### F15: Worker seed fetch silently swallows errors +#### 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) @@ -548,28 +666,44 @@ The one area to watch: tests that use `_ => {}` in their fold/apply helpers (lik | 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 | +| 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** | **25** | | +| **Total** | **33** | | --- ## Recommendations -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. +### 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. **Replace parser wildcards with exhaustive matches** (F04): Change `_ => {}` to explicit ignores of known-irrelevant rules, so new rules cause compile errors. +3. **Propagate auto-persist failures to client** (F04): The worker must communicate save failures, not just set `dirty: true`. -4. **Fix vacuous test assertions** (F10, F11): Tests asserting `>= 1.0` or `!is_empty()` pass when implementations are broken. +4. **Report dispatch errors** (F05): The `.ok()?` pattern swallowing command construction errors must show a status message. -5. **Guard socket send for CLOSING state** (F06): Check for both OPEN and not-CLOSING to avoid uncaught exceptions. +### P2 Fixes (Medium) -6. **Add circuit breaker for render failures** (F07): Don't retry infinitely when render_model throws. +5. **Replace parser wildcards with exhaustive matches** (F06): Change `_ => {}` to explicit ignores of known-irrelevant rules. -7. **Audit `let _ =` patterns**: Each discarded `Result` should either be intentional (documented) or converted to an error report. +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. ---