docs: improvise-xps8 describes a bug that does not exist — turn it into a guard
Review findings on PR #30, all three applied. 1 [P1]. The claim that the handshake watchdog overwrites a genuine mid-session disconnect 8s later is FALSE on any tree that has PR #21. `close` routes BOTH arms through `setShellFailure` (app.js:248-250), which clears the timer synchronously (app.js:165-169). It was believed because it was measured honestly — against a tree predating PR #21 — and then reported as "surviving" that PR on the strength of a simulation that re-created only the `opened` flag and not the other half of the fix, the routing. A simulated fix that is not the fix you named reproduces a bug the real code does not have, and reads exactly like a confirmation. Rather than delete the claim, execute the property it denied: the open-then-dropped test now runs past HANDSHAKE_TIMEOUT_MS + 2s and asserts "sent no document" never appears. Proved discriminating by removing `setShellFailure`'s `clearTimeout` — the assertion then fails with exactly the message xps8 described. gotcha 48 and the spec's comment block corrected; the ticket is cited in neither. Recorded as doctrine(a_simulated_fix_must_be_the_fix_you_named), the sibling of an_injected_fault_must_be_the_fault_you_named that fails the other way: a wrong fault proves a check that cannot fire, a wrong simulated fix proves a bug that does not exist. 2 [P1]. The previous commit created the contradiction it was meant to remove: design-principles recorded the measured ~2.2s while kb.pl still said "under 2 seconds". The ambiguity between per-binary and wall clock is what let it drift, so both now state two numbers — no single binary over 2s, wall clock is the sum, measured 1309 tests / 33 binaries / ~2.2s / longest 1.42s — in kb.pl, design-principles and repo-map alike. Also 16 -> 17 in doctrine(binding_dump_diff). 3 [P2]. The new audit exclusion was unanchored, so `improvise-gp` was a prefix match that would silently exclude a real future ticket. Anchored `^…$`. It immediately caught the example id the explanatory paragraph itself spelled out — a recipe is code, twice over. CLAUDE.md: declined, with evidence. Both copies of the duplicated tail sit inside `<!-- BEGIN BEADS INTEGRATION v:1 profile:minimal hash:ca08a54f -->` markers written by `bd setup claude` and `bd setup codex` and refreshed against that hash. A process change there is one tool invocation, not two hand edits, and a pointer makes `bd setup <recipe> --check` report it modified. Recorded as kb_issue_found(claude_md_tail_is_generated_not_duplicated) so it is not re-proposed. kb.pl: doctrine 54->55, kb_issue_found 11->12; invariant 51, decision 47, location 34 unchanged. kb_consistent passes. cargo test --workspace: 1309 passed, 0 failed, 2 ignored. harness (Playwright): 12 passed, 3 skipped (webkit has no OPFS). Dead-ID audit with the anchored recipe: 157 tokens, all resolve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+8
-5
@@ -250,7 +250,7 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text
|
||||
|
||||
Two corrections to the ticket's own gap list, both worth more than the code. **Panel cursor movement was never a gap**: improvise-edp had already made `viewmodel::flatten_cat_tree` the one home both ends call, so it is the *shared* derivation, not a second one — worth re-checking the "known gaps" in other old tickets against what has landed since. And the "unmodified characters only" rule was **mis-scoped, not wrong**: it was about `KeyPattern::AnyChar`, which is bound only in the text-entry modes, so `Ctrl-d` in Normal has an exact binding and mirroring it is a faithful subset. Each resolver mirrors its `Cmd` **line for line, including where siblings disagree** — `Move` and `MoveTileCursor` emit even when they clamp, `MovePanelCursor` returns nothing when the cursor does not move. Tidying that into one rule would make the client a second derivation, which is what `tests/lockstep.rs` exists to rule out. What still needs the host is a **named list** in `keys.rs` and in `the_normal_mode_keys_that_still_need_the_host_are_these`, so closing one has to come there and say so.
|
||||
|
||||
48. **The browser JavaScript is argued *and now partly executed*, and the two rules it can break are both about *when*.** `web/app.js` and `web/worker.js` were syntax-checked, `.d.ts`-checked for call arity and read — never run — and every bug in them was found by reading. `harness/tests/shell.spec.js` is the first execution of either: it runs `app.js` under Chromium against a mocked peer, and it exists because PR #21 declined to add regression tests for its own close-listener fix on the ground that there was nowhere to put them. That decline is only honest because the assertions live there now, and **a change to `web/*.js` belongs in that spec directory** rather than in a second ad-hoc surface beside it. Playwright rather than a headless-DOM shim because what goes wrong in these files is a real Worker, a real WebSocket, real OPFS and real event ordering, none of which jsdom has. Two classes of bug still follow from *when*, both found by review. **An `async` `message` listener does not serialize.** `worker.js`'s `handle()` awaits `server.flush()`, so the event loop may start the next message while an earlier one is suspended, interleaving two `on_message`/`flush` pairs and letting a later frame's replies **overtake** an earlier frame's — which gotcha 33 answers by *dropping* the overtaken update, not by replaying it. Every frame is therefore chained onto **one tail promise** and the listener is deliberately not `async`; that also turns "frames that arrived before `start()` finished" into an ordinary case of the ordering rule instead of a drain loop, which had the same race twice over because `server` was assigned *before* the loop ran. **A watchdog must be disarmed, not pre-empted.** The handshake watchdog exists for one condition — the peer never answered — and every *loud* failure (worker `error`/`messageerror`, socket `error`) has already written something more precise; the timer then fired 8s later and replaced it, so `setShellFailure` clears it. Its neighbour, the socket's `close` listener overwriting a precise `error` with "disconnected", **is fixed** (improvise-taxx, PR #21) — and the ticket's own proposed mechanism was wrong: `readyState` cannot discriminate, an `opened` flag is the only thing that can. What remains open is the other half: the handshake watchdog still overwrites a **genuine mid-session disconnect** eight seconds later (**improvise-xps8**), and that one is not the same fix, because suppressing it could hide a real drop.
|
||||
48. **The browser JavaScript is argued *and now partly executed*, and the two rules it can break are both about *when*.** `web/app.js` and `web/worker.js` were syntax-checked, `.d.ts`-checked for call arity and read — never run — and every bug in them was found by reading. `harness/tests/shell.spec.js` is the first execution of either: it runs `app.js` under Chromium against a mocked peer, and it exists because PR #21 declined to add regression tests for its own close-listener fix on the ground that there was nowhere to put them. That decline is only honest because the assertions live there now, and **a change to `web/*.js` belongs in that spec directory** rather than in a second ad-hoc surface beside it. Playwright rather than a headless-DOM shim because what goes wrong in these files is a real Worker, a real WebSocket, real OPFS and real event ordering, none of which jsdom has. Two classes of bug still follow from *when*, both found by review. **An `async` `message` listener does not serialize.** `worker.js`'s `handle()` awaits `server.flush()`, so the event loop may start the next message while an earlier one is suspended, interleaving two `on_message`/`flush` pairs and letting a later frame's replies **overtake** an earlier frame's — which gotcha 33 answers by *dropping* the overtaken update, not by replaying it. Every frame is therefore chained onto **one tail promise** and the listener is deliberately not `async`; that also turns "frames that arrived before `start()` finished" into an ordinary case of the ordering rule instead of a drain loop, which had the same race twice over because `server` was assigned *before* the loop ran. **A watchdog must be disarmed, not pre-empted.** The handshake watchdog exists for one condition — the peer never answered — and every *loud* failure (worker `error`/`messageerror`, socket `error`) has already written something more precise; the timer then fired 8s later and replaced it, so `setShellFailure` clears it. Its neighbour, the socket's `close` listener overwriting a precise `error` with "disconnected", **is fixed** (improvise-taxx, PR #21) — and the ticket's own proposed mechanism was wrong: `readyState` cannot discriminate, an `opened` flag is the only thing that can. PR #21's *other* half is the one that keeps getting lost: it routes **both** close arms through `setShellFailure`, which disarms the watchdog, so a genuine mid-session disconnect is **not** overwritten eight seconds later. A bug report said it was, measured it honestly against a pre-#21 tree, and then "confirmed" it survived #21 by simulating the `opened` flag **and not the routing** — a simulated fix that is not the fix you named reproduces a bug the real code does not have, and reads exactly like a confirmation. The ticket was filed and closed invalid; the property is now asserted by `a socket that opened and then dropped reads as disconnected, and stays that way`, which runs past the 8s window, rather than argued in a comment that was wrong twice.
|
||||
|
||||
**What the harness still cannot see**, and why the argued-not-tested discipline stays: the grouped and multi-level DOM, the CSS, and the `ResizeObserver` extent arithmetic. What a careful read established about the last of these — the observer cannot feed back into itself (`#grid` is a flex child with `overflow: auto`, so `replaceChildren` cannot resize it) and `ROW_PX`/`COL_PX` under-estimate, so the worst case is a row of over-fetch — is still a read and not a test.
|
||||
|
||||
@@ -558,10 +558,13 @@ Import flags: `--category`, `--measure`, `--time`, `--skip`, `--extract`, `--axi
|
||||
|
||||
Full guidance lives in `context/design-principles.md` §7. Quick reminders:
|
||||
|
||||
- **Measured at `22d43b2`**: 1309 tests over 33 binaries, ~2.2s of run time (longest
|
||||
single binary 1.42s), excluding compilation. The "<2s" this used to assert is now
|
||||
a number to *re-measure* rather than re-assert — `cargo test --workspace` prints
|
||||
a `finished in` per binary. Don't let it drift.
|
||||
- **Two numbers, not one** — the ambiguity between them is what let the old
|
||||
"suite runs in <2s" drift: **no single test binary over 2 seconds**, and the
|
||||
workspace wall clock is the sum. Measured at `22d43b2`: 1309 tests over 33
|
||||
binaries, ~2.2s wall, longest single binary 1.42s — so the workspace is already
|
||||
past 2s and the per-binary rule is the one that bites. Excludes compilation, and
|
||||
excludes `harness/` (Playwright, minutes, its own CI job). *Re-measure* rather
|
||||
than re-assert — `cargo test --workspace` prints a `finished in` per binary.
|
||||
- Commands: build a `CmdContext`, call `execute`, assert on returned effects. No terminal needed.
|
||||
- Property tests (`proptest`, default 256 cases) cover invariants: CellKey sort, axis consistency, save/load roundtrip, `parse(format(parse(generate())))` stability.
|
||||
- **Bug-fix workflow**: write a failing test *before* the fix (regression guard). Document the bug in the test's doc-comment (see `model/types.rs::formula_tests`).
|
||||
|
||||
Reference in New Issue
Block a user