Add final subagent findings (33 total)

New P1 findings: - F04: Auto-persist failure in worker not communicated to client - F05: dispatch() swallows interactive() errors silently New P2 findings: - F15: law_toggle_group_involution test doesn't apply effects - F16: law_sequence_associativity test proves tautology - F17: ServerFrame serialization failure silently dropped - F18: Unparseable ClientFrame silently ignored - F19: ClientFrame for closed document produces no response - F20: Debug-string tests verify effect names, not correctness Total: 5 P1, 18 P2, 10 P3 = 33 findings Co-authored-by: Ed Langley <fiddlerwoaroof@users.noreply.github.com>
2026-09-21 20:45:56 +00:00
parent b3c941172a
commit 220944e26c
1 changed files with 144 additions and 10 deletions
@@ -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.

 ---