From 9863d96eef3ebf94749e87f322d9b6f5ced941d9 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 13:56:20 -0700 Subject: [PATCH 1/5] fix(harness): restore the close-routing guard PR #61 dropped, on a faked clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of PR #61 found one real coverage loss, and it is the defect class that PR's own body names: an assertion that survives the deletion of the thing it was about. `shell.spec.js` lost "a socket that opened and then dropped reads as disconnected, and stays that way" and both MutationObserver negative assertions. Reverting PR #21's routing half in `web/app.js` — the socket `error` and `close` arms calling `setShell` instead of `fail` — left the node suite 26/26 and Playwright 13/13 green. `shell.test.js` cannot cover it, and why is the whole boundary argument: "a loud failure DISARMS the watchdog" tests `createShell` in isolation and cannot see whether `app.js` ROUTES the close event through `fail()`. That routing is the half gotcha 48 says keeps getting lost; this was the third time, after two earlier mis-confirmations that simulated the `opened` flag and not the routing. Both tests are back under their original names, so the two `####` headings in context/test-evidence/clients.md name live tests again. They cost ~75ms each rather than ~9s, because `page.clock` fakes the PAGE's timers: the page, the socket, the listeners and the `setTimeout` call are all real and only the passage of time is simulated. That is not a weakening, and the injections are the proof in both directions — if `runFor` did not advance the page's timers, a broken routing could not surface the watchdog's message and they would pass: - both arms reverted to `setShell` -> "…and stays that way" fails - close arm only -> "…and stays that way" fails - close arm drops the `opened` flag -> "…the watchdog stays out of it" fails Three injections, three outcomes, none of them the unit test. The helper guards its own premise, because it turned out not to: `runFor` without `clock.install()` does nothing AND throws nothing, so a test that forgot the install asserted "the watchdog never spoke" against a watchdog that never ran — 6/6 green. `outlastTheWatchdog` now requires the page's own `Date.now()` to have moved, and that omission fails by name. Also: `render_surface.test.js` matched `model.` over source INCLUDING comments, so un-painting a field and mentioning it in a comment passed — demonstrated against the pre-fix version, then fixed and re-demonstrated. And the PR's speed claim is corrected where it is documented. The eleven deleted per-widget tests cost 3.3s between them against 3.4s for the three contract tests replacing them: they were free. Essentially all of 36.9s -> 15.5s was two real-timer waits, one of them this guard. The wait was the cost, not the test. Filed, not fixed: improvise-igxp (the help overlay's rows are asserted as present, never as content — inherited verbatim from the test contract.spec.js replaced) and improvise-4ktt (the js job's bundle-freedom is a shell-glob convention, not a gate). The `####`-vs-`###` blind spot in --check-evidence, which is why the dangling heading survived the gate, is a comment on improvise-r0qa. Co-Authored-By: Claude Opus 5 (1M context) --- context/code/clients.md | 34 ++++++++++++++--- context/test-evidence/clients.md | 42 +++++++++++++++++++-- harness/tests/shell.spec.js | 65 +++++++++++++++++++++++++++++--- web/tests/render_surface.test.js | 9 ++++- 4 files changed, 134 insertions(+), 16 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index a054d12c..816df389 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1189,9 +1189,20 @@ it about", and the browser half has four: |---|---|---| | `RenderModel` → DOM | every field the render model projects reaches an element | `web/tests/render_surface.test.js` (statically) **and** `harness/tests/contract.spec.js` (on the screen) | | JS → wasm | the calls the page makes exist, with the arity the bundle declares, and their answers are painted | `web/tests/bundle/wasm_surface.test.js` (the surface) **and** `contract.spec.js`'s pointer test (the wiring) | -| page → peer | the handshake goes out first, a frame is answered whole, and a failure is named | `harness/tests/shell.spec.js`, `harness/tests/drill.spec.js` | +| page → peer | the handshake goes out first, a frame is answered whole, a failure is named, **and the page routes each failure through the call that disarms the watchdog** | `harness/tests/shell.spec.js`, `harness/tests/drill.spec.js` | | worker → OPFS | `storage_contract`, run against a real `OpfsFs` | `harness/tests/opfs.spec.js` | +**A unit test of a rule does not test the wiring that reaches it**, and the +sharpest case in this file is the watchdog. `shell.test.js` proves that +`createShell.fail()` disarms rather than pre-empts; it cannot prove that +`app.js`'s socket `close` and `error` listeners call `fail()` rather than +`setShell()`. That routing is PR #21's other half, the one gotcha 48 says keeps +getting lost — and PR #61 lost it a third time by keeping the unit test and +dropping the browser assertion, with 26/26 and 13/13 green. Restored, and the +rule generalises: **when a unit test replaces a boundary test, ask which of the +two the regression would have been in.** Here it is the routing, so the guard +stays in Playwright. + Everything **between** those seams is `node --test web/tests`, and it may assume the boundary holds rather than re-establishing it: transport selection from `?ws=`, the two facts `#status` composes, the watchdog's arm and disarm, the @@ -1242,12 +1253,23 @@ Measured on one machine at `1af51c9` versus this branch, both on a private | suite | tests | execution | whole job | |---|---|---|---| | Playwright, before | 29 (+3 skipped) | 36.9s | ~4m30s in CI | -| Playwright, after | 16 (+3 skipped) | 15.7s | ~4m30s in CI | -| `node --test web/tests` | 26 | 0.11s | seconds | +| Playwright, after | 18 (+3 skipped) | ~16s | ~4m30s in CI | +| `node --test web/tests` | 26 | 0.12s | seconds | -**Cutting Playwright tests barely moves the CI number**, because roughly four -minutes of that job is three `wasm-pack` builds, `npm ci` and a browser -download — not test execution. The feedback-time win is the `js` job, which +**Where that saving actually came from is worth stating, because the obvious +answer is wrong.** The eleven per-widget tests that were deleted cost **3.3 +seconds between them** — restored verbatim and re-measured — against 3.4s for +the three contract tests that replace them. They were free. Essentially all of +36.9s → 15.5s was two `shell.spec.js` tests that **waited out the 8s watchdog in +real time**, and one of those two was the routing guard this section now +explains should never have gone. So the deletion of the eleven bought shape, not +speed; the speed came from a deletion that cost coverage. Both are now back, at +~75ms each, because `page.clock` fakes the page's timers — which is the honest +version of the trade: *the wait was the cost, not the test.* + +**Cutting Playwright tests barely moves the CI number** in any case, because +roughly four minutes of that job is three `wasm-pack` builds, `npm ci` and a +browser download — not test execution. The feedback-time win is the `js` job, which needs **no Rust toolchain and no wasm bundle at all**, and that is a constraint rather than an accident: the moment a `web/tests/*.test.js` file needs `web/pkg/`, it inherits the four-minute build and the argument for it diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index f68ffa00..9aff23c3 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -538,8 +538,9 @@ handshake with a frame the client cannot fold — so this exercises the real improvise-hwpi: without an `error` listener, a connection that never opened left `#status` blank, and "the server is not running" read exactly like "still -connecting". The watchdog half is asserted on the same page load rather than in -its own test, because it needs the same nine-second wait. It holds for a reason +connecting". The watchdog half is asserted on the same page load, and since +PR #61's follow-up it costs no wall clock at all: `page.clock` fakes the PAGE's +timers, so `runFor(10s)` advances the watchdog without the test sleeping. It holds for a reason its own comment does not lead you to expect: `armWatchdog` runs only after `transport.ready` resolves, which happens on `open` — so on a socket that never opened the watchdog is never armed at all. @@ -550,7 +551,8 @@ The other arm, which must keep working when the first is fixed: a socket that really did open and then dropped IS a disconnect, and saying so is the whole reason `close` cannot simply be suppressed. -**It runs past `HANDSHAKE_TIMEOUT_MS` on purpose.** On this arm the watchdog IS +**It runs past `HANDSHAKE_TIMEOUT_MS` on purpose** — through `page.clock`, not +through a real wait. On this arm the watchdog IS armed — `armWatchdog` runs after `transport.ready` resolves, i.e. on `open` — so "disconnected" is written while a timer is pending, and the only reason it survives is that `close` goes through `setShellFailure`, which clears it. That @@ -566,6 +568,35 @@ assertion rather than another paragraph — proved discriminating by making `setShellFailure` skip its `clearTimeout`, which makes it fail. Same rule as `doctrine(an_injected_fault_must_be_the_fault_you_named)`, one level up. +**And it was lost a third time, by the PR that was arguing about exactly this.** +PR #61 split the browser tests into boundary contracts (Playwright) and unit +tests (`node --test`), and merged these two tests into one that kept only the +last-value assertions. `shell.test.js`'s "a loud failure DISARMS the watchdog" +covers `createShell` in isolation and **cannot see whether `app.js` routes the +close event through `fail()` rather than `setShell()`** — which is the half +PR #21 fixed and the half that keeps getting lost. Reverting both socket arms +to `setShell` left the node suite 26/26 and Playwright 13/13 green. Restored +under their original names, and re-proved with that exact revert: the two-arm +revert and the close-arm-only revert each fail "…and stays that way", and +dropping the `opened` flag fails "…the watchdog stays out of it". **Three +injections, three different tests, none of them the unit test.** + +**And the helper guards its own premise.** `page.clock.runFor` on a page where +`clock.install()` was never called does **nothing and throws nothing**, so a test +that forgot the install would assert "the watchdog never spoke" against a +watchdog never given the chance — vacuous, and green. `outlastTheWatchdog` reads +`Date.now()` inside the page either side of the jump and requires it to have +moved. Demonstrated both ways: with the install removed the suite passed 6/6, +and with the guard in place the same omission fails by name. + +**The faked clock is what makes the restoration cheap without weakening it**, +and the injections are the proof of that too: if `page.clock.runFor` did not +really advance the page's timers, a broken routing could not surface the +watchdog's message and those injections would have passed. They failed. The +test keeps the real page, the real socket, the real listeners and the real +`setTimeout` call — only the *passage of time* is simulated, and 9 seconds of +wall clock became 75 milliseconds. + **The measurement that could not become a CI guard.** Both of the above were once asserted as `test.fail` expected-failures and **had to be removed**: neither reproduced on the GitHub runner, where `close` does not follow `error` within the @@ -1164,6 +1195,11 @@ Every test below was proved able to fail by patching the code it names, checking the patch was on disk, running the suite and reading the failure — never by the absence of an error. Thirteen injections, thirteen failures: +**What `shell.test.js` cannot reach, and the PR found out the hard way:** it +tests `createShell` in isolation, so it sees the watchdog RULE and never the +`app.js` wiring that calls it. That distinction is now written into +`context/code/clients.md` and guarded in `shell.spec.js`. + **`shell.test.js`** — `socketUrl` stops upgrading `ws://` on an https page; `composeStatus` returns the model fact alone (which fails five tests, because the watchdog's messages are shell facts and vanish with it); `fail()` stops diff --git a/harness/tests/shell.spec.js b/harness/tests/shell.spec.js index a0f60fdf..4f27c2f7 100644 --- a/harness/tests/shell.spec.js +++ b/harness/tests/shell.spec.js @@ -11,10 +11,38 @@ const status = (page) => page.locator("#status"); /// the harness's OWN server, so it follows the port the config chose. const NO_WEBSOCKET_HERE = `127.0.0.1:${process.env.IMPROVISE_HARNESS_PORT ?? "8099"}`; -/// `web/app.js`'s own HANDSHAKE_TIMEOUT_MS. One test has to outlast it, and a -/// number that silently disagreed with the page would pass for the wrong reason. +/// `web/app.js`'s own HANDSHAKE_TIMEOUT_MS. Three tests have to outlast it, and +/// a number that silently disagreed with the page would pass for the wrong reason. const HANDSHAKE_TIMEOUT_MS = 8_000; +/// Record every distinct #status the page shows, not just the last one: the +/// properties below are about a message being REPLACED, which a final read +/// cannot see. Installed before the page's own script runs. +function logStatus() { + window.__statusLog = []; + new MutationObserver(() => { + const text = document.getElementById("status")?.textContent ?? ""; + if (text && window.__statusLog.at(-1) !== text) window.__statusLog.push(text); + }).observe(document, { childList: true, subtree: true, characterData: true }); +} + +const json = (v) => JSON.stringify(v); + +/// Walk past the watchdog without waiting for it. `page.clock` fakes the +/// page's own timers, so the 8s is the PAGE's and the wall clock is not. +async function outlastTheWatchdog(page) { + const jump = HANDSHAKE_TIMEOUT_MS + 2_000; + const before = await page.evaluate(() => Date.now()); + await page.clock.runFor(jump); + // `runFor` without `clock.install()` does NOTHING and throws nothing, so a + // test that forgot it would assert "the watchdog never spoke" vacuously. + const after = await page.evaluate(() => Date.now()); + expect( + after - before, + "page.clock.install() was not called: the page's timers never advanced", + ).toBeGreaterThanOrEqual(jump); +} + test.describe("the shell reports what its peer is doing", () => { // improvise-hwpi. The fault is injected at the *peer*, not in the page, so // this exercises the real `on_frame` and the real catch. @@ -26,15 +54,30 @@ test.describe("the shell reports what its peer is doing", () => { await expect(status(page)).toContainText("bad frame", { timeout: 15_000 }); }); - // A socket that never opened did not "disconnect" (improvise-taxx): nothing - // on the close event can discriminate, so the page records `opened` itself. - // That it then STAYS is the watchdog's disarming, which is Node's now. - test("a peer that cannot be reached is named, and one that dropped is not confused with it", async ({ + // improvise-hwpi, plus the half `shell.test.js` cannot reach: that app.js + // ROUTES this arm through `shell.fail`, which disarms the watchdog. + test("a peer that cannot be reached is named, and the watchdog stays out of it", async ({ page, }) => { + await page.clock.install(); + await page.addInitScript(logStatus); await page.goto(`/app/index.html?ws=${NO_WEBSOCKET_HERE}`); await expect(status(page)).toContainText("cannot reach", { timeout: 15_000 }); + await outlastTheWatchdog(page); + const log = await page.evaluate(() => window.__statusLog); + expect(log.some((t) => t.includes("sent no document")), `#status said: ${json(log)}`).toBe( + false, + ); + }); + + // The arm PR #21 fixed and that gotcha 48 says keeps getting lost: a socket + // that never opened did not "disconnect", and the one that did STAYS so. + test("a socket that opened and then dropped reads as disconnected, and stays that way", async ({ + page, + }) => { + await page.clock.install(); + await page.addInitScript(logStatus); // Closes on the page's first frame, so "it opened first" is true by // construction rather than by timer. await page.routeWebSocket("ws://127.0.0.1:8790/", (ws) => { @@ -43,6 +86,16 @@ test.describe("the shell reports what its peer is doing", () => { await page.goto("/app/index.html?ws=127.0.0.1:8790"); await expect(status(page)).toContainText("connected", { timeout: 15_000 }); await expect(status(page)).toContainText("disconnected", { timeout: 15_000 }); + + // Past the watchdog it was armed against: the routing, not the rule, is + // what this can see and `shell.test.js` cannot. + await outlastTheWatchdog(page); + await expect(status(page)).toContainText("disconnected"); + + const log = await page.evaluate(() => window.__statusLog); + expect(log.some((t) => t.includes("sent no document")), `#status said: ${json(log)}`).toBe( + false, + ); }); // The one assertion that needs the real timer: that the watchdog is ARMED diff --git a/web/tests/render_surface.test.js b/web/tests/render_surface.test.js index 1fe8e1b2..aa0766aa 100644 --- a/web/tests/render_surface.test.js +++ b/web/tests/render_surface.test.js @@ -8,6 +8,13 @@ import { fileURLToPath } from "node:url"; const read = (rel) => readFileSync(fileURLToPath(new URL(rel, import.meta.url)), "utf8"); +/// Comments name model fields routinely, so a field mentioned only in one +/// would read as painted. Strip them before asking what the code touches. +const code = (rel) => + read(rel) + .replace(/\/\*[\s\S]*?\*\//g, " ") + .replace(/(^|[^:"'`])\/\/.*$/gm, "$1"); + /// Fields the page deliberately does not read, each with the reason. A field /// belongs here only with an argument; the list is the debt, written down. // improvise-z754 put the title bar's facts in one shared cache and taught @@ -32,7 +39,7 @@ function renderModelFields() { test("every field the render model projects is painted, or named as debt", () => { // Both halves of the page: `app.js` paints, `shell.js` plans the grid. - const page = read("../app.js") + read("../shell.js"); + const page = code("../app.js") + code("../shell.js"); const unpainted = renderModelFields().filter((field) => !page.includes(`model.${field}`)); assert.deepEqual( unpainted.sort(), From bf01469cf8158205ae5c51590ea8c9824aef433e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 14:01:16 -0700 Subject: [PATCH 2/5] docs(clients): label the test counts, and correct a contaminated baseline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the fix found the speed table quotes one number where there are two: every spec but `opfs.spec.js` is Chromium-only, so a count read off a run is three larger than the count of tests in the files, with three more skipped. The table now has a chromium column and an all-projects column, and says so — the same "two numbers, not one" the Rust suite learned in design-principles §7. Chasing that ambiguity turned up a worse error in the number itself. The `before` figure was measured with `git checkout 1af51c9 -- harness web` inside the working branch, which restores TRACKED files and leaves untracked ones where they are — so `contract.spec.js` stayed behind and the "baseline" ran main's 23 tests plus three of the branch's own. It read 29/36.9s, which looked entirely plausible. The honest figure, measured on a clean tree, is 23/32.8s. A baseline taken by checking paths out over your own work measures a tree that never existed, and that is now written down where the number is. Also records the probe for the open question the faked clock left. A peer that opens, stays silent past the watchdog window and is closed afterwards produces a byte-identical `#status` transcript under `page.clock` and under a real nine-second wait — four states, same order, same text. So `runFor` does not flatten that ordering. What stays uncovered is an event landing strictly INSIDE the collapsed window, which a synchronous `runFor` cannot deliver and which is a race the page promises nothing about. Stated rather than left to be re-derived; the probe itself is not kept, being a characterization of Playwright's clock rather than of this repo. Co-Authored-By: Claude Opus 5 (1M context) --- context/code/clients.md | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index 816df389..1b7a4acd 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1203,6 +1203,18 @@ rule generalises: **when a unit test replaces a boundary test, ask which of the two the regression would have been in.** Here it is the routing, so the guard stays in Playwright. +**The faked clock is faithful outside the window and silent inside it.** +`shell.spec.js` walks past the 8s handshake watchdog with `page.clock`, and the +question that owes an answer is whether collapsing ten seconds into one +synchronous step *flattens* an ordering rather than merely skipping the wait. +Probed for the sharpest case — a peer that opens, stays silent past the window +and is closed afterwards — the `#status` transcript is **byte-identical** under +the fake clock and under a real nine-second wait: four states, same order, same +text. What that does not cover is an event landing **strictly inside** the +collapsed window, which a synchronous `runFor` gives no opportunity to deliver; +that is a race whose outcome the page does not promise, so there is nothing to +assert. State it rather than re-derive it. + Everything **between** those seams is `node --test web/tests`, and it may assume the boundary holds rather than re-establishing it: transport selection from `?ws=`, the two facts `#status` composes, the watchdog's arm and disarm, the @@ -1250,17 +1262,32 @@ the repaint is the boundary; the other seven were the same instance again. Measured on one machine at `1af51c9` versus this branch, both on a private `IMPROVISE_HARNESS_PORT`: -| suite | tests | execution | whole job | -|---|---|---|---| -| Playwright, before | 29 (+3 skipped) | 36.9s | ~4m30s in CI | -| Playwright, after | 18 (+3 skipped) | ~16s | ~4m30s in CI | -| `node --test web/tests` | 26 | 0.12s | seconds | +| suite | chromium | all projects | execution | whole job | +|---|---|---|---|---| +| Playwright, before (`1af51c9`) | 23 | 29 (+3 skipped) | 32.8s | ~4m30s in CI | +| Playwright, after | 14 | 17 (+3 skipped) | ~15s | ~4m30s in CI | +| `node --test web/tests` | — | 26 | 0.12s | seconds | + +**Two columns because one number was ambiguous twice.** Every spec but +`opfs.spec.js` is Chromium-only, so a count taken off a run is three larger +than the count of tests in the files (Firefox's OPFS battery) with three more +skipped (WebKit's). `npx playwright test --list` prints the per-project truth; +quote which one, and this is the same "two numbers, not one" the Rust suite +learned in `design-principles.md` §7. + +**And the trap that made the first `before` number wrong.** It was measured by +`git checkout 1af51c9 -- harness web` inside a working branch — which restores +*tracked* files and leaves untracked ones **where they are**, so the new +`contract.spec.js` stayed and the "baseline" run was main's 23 tests **plus +three of the branch's own**. It read 29/36.9s and looked plausible; the honest +figure, measured on a clean tree, is 23/32.8s. A baseline taken by checking +paths out over your own work measures a tree that never existed. **Where that saving actually came from is worth stating, because the obvious answer is wrong.** The eleven per-widget tests that were deleted cost **3.3 seconds between them** — restored verbatim and re-measured — against 3.4s for the three contract tests that replace them. They were free. Essentially all of -36.9s → 15.5s was two `shell.spec.js` tests that **waited out the 8s watchdog in +32.8s → 15s was two `shell.spec.js` tests that **waited out the 8s watchdog in real time**, and one of those two was the routing guard this section now explains should never have gone. So the deletion of the eleven bought shape, not speed; the speed came from a deletion that cost coverage. Both are now back, at From 481f1da444ec280872bb9496467b9de4d74ec2fb Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 14:05:04 -0700 Subject: [PATCH 3/5] docs(clients): measure the baseline on a clean tree, and quote only that MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third correction to one number, and this time by measurement rather than arithmetic. `1af51c9` reached by a FULL checkout — which removes a tracked file the target does not have, where `checkout -- ` leaves it — then built and run twice on a private port, and the same for this branch in the same session: before 23 chromium, 26 passed + 3 skipped, 36.2s / 36.3s after 14 chromium, 17 passed + 3 skipped, 15.1s / 14.9s Three things that changes. The `before` row was internally inconsistent: it paired a corrected chromium count (23) with the contaminated run count beside it (29), and 23 + 3 firefox is 26. A row whose columns do not add up is a row built from two trees. The note now says the columns must agree, because that identity is what caught this every time it was caught at all. `32.8s` is gone. It was not measured by anyone who could hand me its provenance — I attributed it to the reviewer, who had only ever counted `1af51c9` statically and never ran a full pre-PR suite — and it does not reproduce here. Quoting an unattributable number inside the paragraph that exists to explain an unattributable number was the wrong repair. And the contamination's cost was the COUNT, not the clock: the contaminated run read 29 against the clean tree's 26, but 36.9s against 36.2s, because the three extra tests cost under a second between them. A wall clock absorbs a contaminated tree without a trace; an arithmetic identity does not. That is now the stated lesson, and the same error surviving three corrections — into PR #61's body as "26 chromium", then through a fix that corrected one column and left its neighbour — is the worked example of `clients.md`'s own rule that a fact stated in more than one place is changed by a grep, not an edit. Co-Authored-By: Claude Opus 5 (1M context) --- context/code/clients.md | 65 ++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index 1b7a4acd..3cc0a542 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1259,40 +1259,53 @@ the repaint is the boundary; the other seven were the same instance again. ### Two numbers, and neither is the other -Measured on one machine at `1af51c9` versus this branch, both on a private -`IMPROVISE_HARNESS_PORT`: +Measured on one machine, in one session, on a private +`IMPROVISE_HARNESS_PORT`, with **both trees clean** — `1af51c9` reached by a +full checkout rather than by restoring paths over a working branch (see the +trap below). Two runs each; the pair is the variance. | suite | chromium | all projects | execution | whole job | |---|---|---|---|---| -| Playwright, before (`1af51c9`) | 23 | 29 (+3 skipped) | 32.8s | ~4m30s in CI | -| Playwright, after | 14 | 17 (+3 skipped) | ~15s | ~4m30s in CI | +| Playwright, before (`1af51c9`) | 23 | 26 passed + 3 skipped | 36.2s / 36.3s | ~4m30s in CI | +| Playwright, after | 14 | 17 passed + 3 skipped | 15.1s / 14.9s | ~4m30s in CI | | `node --test web/tests` | — | 26 | 0.12s | seconds | -**Two columns because one number was ambiguous twice.** Every spec but -`opfs.spec.js` is Chromium-only, so a count taken off a run is three larger -than the count of tests in the files (Firefox's OPFS battery) with three more -skipped (WebKit's). `npx playwright test --list` prints the per-project truth; -quote which one, and this is the same "two numbers, not one" the Rust suite -learned in `design-principles.md` §7. +**Two columns because one number was ambiguous three times.** Every spec but +`opfs.spec.js` is Chromium-only, so a count taken off a run is three larger than +the count of tests in the files (Firefox's OPFS battery) with three more skipped +(WebKit's). The columns must also **agree**: chromium + 3 is the passed total, +and a row where they do not is a row built from two different trees. +`npx playwright test --list` prints the per-project truth; quote which one, and +this is the same "two numbers, not one" the Rust suite learned in +`design-principles.md` §7. -**And the trap that made the first `before` number wrong.** It was measured by +**The trap that made the first `before` figure wrong.** It was taken by `git checkout 1af51c9 -- harness web` inside a working branch — which restores -*tracked* files and leaves untracked ones **where they are**, so the new -`contract.spec.js` stayed and the "baseline" run was main's 23 tests **plus -three of the branch's own**. It read 29/36.9s and looked plausible; the honest -figure, measured on a clean tree, is 23/32.8s. A baseline taken by checking -paths out over your own work measures a tree that never existed. +*tracked* files and leaves untracked ones **where they are**, so the branch's own +`contract.spec.js` stayed and the "baseline" ran main's 23 tests **plus three of +the branch's**. A baseline taken by checking paths out over your own work +measures a tree that never existed. The full checkout above is the repair. -**Where that saving actually came from is worth stating, because the obvious -answer is wrong.** The eleven per-widget tests that were deleted cost **3.3 -seconds between them** — restored verbatim and re-measured — against 3.4s for -the three contract tests that replace them. They were free. Essentially all of -32.8s → 15s was two `shell.spec.js` tests that **waited out the 8s watchdog in -real time**, and one of those two was the routing guard this section now -explains should never have gone. So the deletion of the eleven bought shape, not -speed; the speed came from a deletion that cost coverage. Both are now back, at -~75ms each, because `page.clock` fakes the page's timers — which is the honest -version of the trade: *the wait was the cost, not the test.* +**What gave it away was the count, not the clock — and that is the lesson.** The +contaminated run read 29 passed where a clean `1af51c9` reads 26, because three +extra tests really were there; but it read 36.9s against the clean tree's 36.2s, +because those three cost under a second between them. **A wall clock absorbs a +contaminated tree without a trace; an arithmetic identity does not.** The same +error survived three separate corrections — it reached PR #61's body as "26 +chromium" (23 + the 3), survived a fix that corrected the chromium column and +left the run count beside it, and was caught each time only by someone checking +that the columns add up. + +**Where the saving actually came from, since the obvious answer is wrong.** The +eleven per-widget tests that were deleted cost **3.3 seconds between them** — +restored verbatim and re-measured — against 3.4s for the three contract tests +replacing them. They were free. Essentially all of 36.2s → 15s was two +`shell.spec.js` tests that **waited out the 8s watchdog in real time**, and one +of those two was the routing guard this section explains should never have gone. +So the deletion of the eleven bought shape, not speed; the speed came from a +deletion that cost coverage. Both are back at ~75ms each, because `page.clock` +fakes the page's timers — the honest version of the trade: *the wait was the +cost, not the test.* **Cutting Playwright tests barely moves the CI number** in any case, because roughly four minutes of that job is three `wasm-pack` builds, `npm ci` and a From 692aa0b1d396fc8505d0753cd0a537809a6afc68 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 14:06:00 -0700 Subject: [PATCH 4/5] docs(clients): compare test time, not two invocations' wall clocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The eleven deleted per-widget tests and the three contract tests replacing them were compared at 3.3s against 3.4s, which reads as a near-tie. It is not one: those were two separate `npx playwright test` invocations, each paying its own cold start (~0.6s and ~1.9s), so the wall clocks measured startup as much as tests and flattered the replacements. The per-test sums are 2.7s against 1.55s. The conclusion is unchanged and has more margin: eleven tests at 2.7s cannot buy a twenty-second saving, so the deletion of the eleven bought shape and the two real-timer watchdog waits bought the speed. It also explains, without needing anything else, why the contaminated baseline's clock barely moved — 1.55s of contract tests, warm inside a full suite, lands under a second against a 36s total. Same discipline as the row above it in the table, applied to the other side of the comparison: two numbers are only a comparison when they were measured the same way. Co-Authored-By: Claude Opus 5 (1M context) --- context/code/clients.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index 3cc0a542..e01c7b53 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1297,9 +1297,17 @@ left the run count beside it, and was caught each time only by someone checking that the columns add up. **Where the saving actually came from, since the obvious answer is wrong.** The -eleven per-widget tests that were deleted cost **3.3 seconds between them** — -restored verbatim and re-measured — against 3.4s for the three contract tests -replacing them. They were free. Essentially all of 36.2s → 15s was two +eleven per-widget tests that were deleted cost **2.7 seconds of test time +between them** — restored verbatim and re-measured — against **1.55s** for the +three contract tests replacing them. Those are the per-test sums, not the two +wall clocks, which read 3.3s and 3.4s and **flattered the replacements**: they +were separate `npx playwright test` invocations, so each paid its own cold +start (~0.6s and ~1.9s) and a near-tie appeared between two things not measured +the same way. Warm inside a full suite those three cost under a second, which is +why the contaminated baseline's clock barely moved. + +Eleven tests at 2.7s cannot buy a twenty-second saving, and they did not. +Essentially all of 36.2s → 15s was two `shell.spec.js` tests that **waited out the 8s watchdog in real time**, and one of those two was the routing guard this section explains should never have gone. So the deletion of the eleven bought shape, not speed; the speed came from a From 89f67afd4c162ec1fe4d714f0e311b345095be3c Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 14:24:25 -0700 Subject: [PATCH 5/5] fix(harness): make a vacuous watchdog denial fail, and single-home the window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #65's review found this PR had planted a fresh instance of the defect it exists to repair. Both blocking findings fixed, each demonstrated. FINDING 1. "a peer that cannot be reached is named, and the watchdog stays out of it" could not see the property its comment claimed. `shell.arm` runs only after `await transport.ready`, which never resolves on a socket that never opens — so the watchdog is never armed there and denying "sent no document" denies an unreachable event. Reverting PR #21's routing left that clause passing. The aggravating half: `context/test-evidence/clients.md` already said so, and this PR EDITED that paragraph — inserting "runFor(10s) advances the watchdog" immediately above the sentence explaining it is never armed. Not text the PR left alone; text the PR walked past mid-sentence. The clause is gone and the test keeps the naming property it really guards: a `close` on a socket that never opened must not read as "disconnected". And the shape cannot return — `outlastTheWatchdog` now asks the page `timesArmed() > 0` before jumping, so a watchdog denial re-added to that path FAILS BY NAME. It asks *ever armed* rather than *armed now* on purpose: by the time the disconnect test jumps, a correct page has already disarmed, which is the property, so the obvious spelling would have denied the passing case. Building the guard is what surfaced that distinction; reading had not. FINDING 2. The spec kept its own `HANDSHAKE_TIMEOUT_MS` beside a comment saying a silent disagreement "would pass for the wrong reason". It did: the spec at 1000 against the page's 8000 made the suite green over the reverted routing, because the jump landed inside the window. The constant is now exported from `web/shell.js` and imported by both, so the disagreement is unconstructible. Shrinking it in its one home still catches the routing revert — that is the demonstration the guard did not go quiet. Filed, not fixed: improvise-78v6 (#61 also deleted the help-overlay spec and nothing replaced it — a PR about a dropped guard that did not diff the dropping PR's test names) and improvise-5ray (--check-evidence collects only `### ` and resolves only Rust fns, so every browser entry's `####` is invisible; appending one that names nothing exits 0, which is how the dangling heading cleared the gate for a release). Verified on the merged tree: node 26/26, bundle 3/3, Playwright 17 passed + 3 skipped, cargo test --workspace 1378 passed, comment-runs clean, --check-evidence 501 resolving, all cited ids resolve, no .beads diff. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/clients.md | 18 ++++++++++++++ context/test-evidence/clients.md | 41 ++++++++++++++++++++++++++------ harness/tests/shell.spec.js | 28 +++++++++++++++------- web/app.js | 5 +++- web/shell.js | 10 ++++++++ 5 files changed, 85 insertions(+), 17 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index e01c7b53..33f187f4 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1215,6 +1215,24 @@ collapsed window, which a synchronous `runFor` gives no opportunity to deliver; that is a race whose outcome the page does not promise, so there is nothing to assert. State it rather than re-derive it. +**A negative assertion must demonstrate the denied event is reachable on that +exact code path** — not on a sibling path, and not in the rule's own unit test. +This file's worked example cost four review rounds. `shell.arm` runs only after +`await transport.ready`, so on a socket that never opens the watchdog is never +armed; a test denying "sent no document" there denies nothing, and passed +against both the routing revert and the disarm regression. `outlastTheWatchdog` +now asks the page `timesArmed() > 0` first, which turns that review finding into +a failure by name. Note *ever armed* and not *armed now*: a correct page has +already disarmed by the time the disconnect test jumps — that is the property — +so the obvious spelling would have denied the passing case. + +**And a constant a test must exceed has one home.** `HANDSHAKE_TIMEOUT_MS` lived +in `web/app.js` and again in the spec, under a comment saying a silent +disagreement "would pass for the wrong reason". It did: the spec at 1000 against +the page's 8000 made the suite green over the reverted routing, because the jump +landed inside the window. It is exported from `web/shell.js` and imported by +both, so the disagreement is unconstructible rather than discouraged. + Everything **between** those seams is `node --test web/tests`, and it may assume the boundary holds rather than re-establishing it: transport selection from `?ws=`, the two facts `#status` composes, the watchdog's arm and disarm, the diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 9aff23c3..cb394e24 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -534,16 +534,33 @@ injected at the *peer*, not in the page** — a mocked socket that answers the handshake with a frame the client cannot fold — so this exercises the real `on_frame` and the real catch. -#### "a peer that cannot be reached is named, and the watchdog stays out of it" +#### "a peer that cannot be reached is named, and not called a disconnect" improvise-hwpi: without an `error` listener, a connection that never opened left `#status` blank, and "the server is not running" read exactly like "still -connecting". The watchdog half is asserted on the same page load, and since -PR #61's follow-up it costs no wall clock at all: `page.clock` fakes the PAGE's -timers, so `runFor(10s)` advances the watchdog without the test sleeping. It holds for a reason -its own comment does not lead you to expect: `armWatchdog` runs only after -`transport.ready` resolves, which happens on `open` — so on a socket that never -opened the watchdog is never armed at all. +connecting". **This test no longer says anything about the watchdog, and the +paragraph below is why it never could.** `shell.arm` runs only after +`await transport.ready`, which resolves on `open` — so on a socket that never +opens the watchdog is **never armed at all**, and denying its message here +denies an unreachable event. + +**That sentence was already in this file, five lines above an assertion that +contradicted it.** PR #65 added the watchdog clause *and edited this very +paragraph*, inserting "`runFor(10s)` advances the watchdog" immediately before +the sentence saying it is never armed. Reverting PR #21's routing, or stripping +`fail()` of its `sawFrame()`, both left that clause **passing**. So a PR whose +subject was "a deleted guard went unnoticed" planted the same shape one level +up: three injections, three distinct test names, and two of them proving +nothing. What the test guards now is the **naming** — `close` on a socket that +never opened must not read as "disconnected" (improvise-taxx) — which is the +half injection 3 really does exercise. + +**The mechanism, so it cannot come back.** `outlastTheWatchdog` now asks the +page `timesArmed() > 0` before jumping, and a watchdog denial re-added to this +path **fails by name**. It asks *ever armed* rather than *armed now*: by the +time the disconnect test jumps, a correct page has already disarmed — that is +the property — so `armed()` would have denied the passing case. Building that +guard is what surfaced the distinction, which no amount of reading had. #### "a socket that opened and then dropped reads as disconnected, and stays that way" @@ -589,6 +606,16 @@ watchdog never given the chance — vacuous, and green. `outlastTheWatchdog` rea moved. Demonstrated both ways: with the install removed the suite passed 6/6, and with the guard in place the same omission fails by name. +**The window is single-homed, and that is the second half of the same +finding.** `HANDSHAKE_TIMEOUT_MS` lived twice — once in `web/app.js`, once in +the spec — with a comment saying a silent disagreement "would pass for the +wrong reason" and nothing checking it. Setting the spec's copy to 1000 against +the page's 8000 made the whole suite green **against the reverted routing**: +the jump landed inside the window, the watchdog had not yet fired, and the +denial held vacuously. It is now exported from `web/shell.js` and imported by +both, so the two cannot disagree; shrinking it in its one home still catches +the routing revert, which is the demonstration that the guard did not go quiet. + **The faked clock is what makes the restoration cheap without weakening it**, and the injections are the proof of that too: if `page.clock.runFor` did not really advance the page's timers, a broken routing could not surface the diff --git a/harness/tests/shell.spec.js b/harness/tests/shell.spec.js index 4f27c2f7..f494c340 100644 --- a/harness/tests/shell.spec.js +++ b/harness/tests/shell.spec.js @@ -4,6 +4,8 @@ import { test, expect } from "@playwright/test"; +import { HANDSHAKE_TIMEOUT_MS } from "../../web/shell.js"; + const status = (page) => page.locator("#status"); /// A peer that accepts the TCP connection and then refuses the WebSocket @@ -11,9 +13,9 @@ const status = (page) => page.locator("#status"); /// the harness's OWN server, so it follows the port the config chose. const NO_WEBSOCKET_HERE = `127.0.0.1:${process.env.IMPROVISE_HARNESS_PORT ?? "8099"}`; -/// `web/app.js`'s own HANDSHAKE_TIMEOUT_MS. Three tests have to outlast it, and -/// a number that silently disagreed with the page would pass for the wrong reason. -const HANDSHAKE_TIMEOUT_MS = 8_000; +/// The page's own constant, imported rather than copied: a spec that +/// understated it would jump short of the window and every watchdog assertion +/// below would pass for the wrong reason — demonstrated, PR #65 review. /// Record every distinct #status the page shows, not just the last one: the /// properties below are about a message being REPLACED, which a final read @@ -31,6 +33,14 @@ const json = (v) => JSON.stringify(v); /// Walk past the watchdog without waiting for it. `page.clock` fakes the /// page's own timers, so the 8s is the PAGE's and the wall clock is not. async function outlastTheWatchdog(page) { + // A negative assertion about the watchdog is vacuous where the watchdog was + // never armed, and `shell.arm` runs only after `await transport.ready`. It + // asks whether it was EVER armed: by here a correct page has disarmed it, + // which is the property, so `armed()` would deny the passing case. + expect( + await page.evaluate(() => window.__improviseShell?.timesArmed() ?? 0), + "the watchdog was never armed on this path: denying its message proves nothing", + ).toBeGreaterThan(0); const jump = HANDSHAKE_TIMEOUT_MS + 2_000; const before = await page.evaluate(() => Date.now()); await page.clock.runFor(jump); @@ -54,19 +64,19 @@ test.describe("the shell reports what its peer is doing", () => { await expect(status(page)).toContainText("bad frame", { timeout: 15_000 }); }); - // improvise-hwpi, plus the half `shell.test.js` cannot reach: that app.js - // ROUTES this arm through `shell.fail`, which disarms the watchdog. - test("a peer that cannot be reached is named, and the watchdog stays out of it", async ({ + // improvise-hwpi. This peer NEVER OPENS, so `await transport.ready` never + // resolves and `shell.arm` never runs — the watchdog is not on this path. + test("a peer that cannot be reached is named, and not called a disconnect", async ({ page, }) => { - await page.clock.install(); await page.addInitScript(logStatus); await page.goto(`/app/index.html?ws=${NO_WEBSOCKET_HERE}`); await expect(status(page)).toContainText("cannot reach", { timeout: 15_000 }); - await outlastTheWatchdog(page); + // The naming is the whole property here: `close` on a socket that never + // opened must not read as "disconnected" (improvise-taxx). const log = await page.evaluate(() => window.__statusLog); - expect(log.some((t) => t.includes("sent no document")), `#status said: ${json(log)}`).toBe( + expect(log.some((t) => t.includes("disconnected")), `#status said: ${json(log)}`).toBe( false, ); }); diff --git a/web/app.js b/web/app.js index c1fb2f15..5b1d14da 100644 --- a/web/app.js +++ b/web/app.js @@ -4,6 +4,7 @@ import init, { Client } from "./pkg/improvise_wasm_client.js"; import { COL_PX, + HANDSHAKE_TIMEOUT_MS, ROW_PX, createShell, extentOf, @@ -40,13 +41,15 @@ const client = new Client(); // A peer that never answers the handshake, and the two facts `#status` shows. // Both are decided in shell.js; this is the wire to the element. -const HANDSHAKE_TIMEOUT_MS = 8000; const shell = createShell({ onStatus: (text) => { el.status.textContent = text; }, timeoutMs: HANDSHAKE_TIMEOUT_MS, }); +// The harness asks whether the watchdog is ARMED, because a negative assertion +// about it is vacuous where it is not (clients.md). Read by tests, nothing else. +window.__improviseShell = shell; // What the grid's own frame costs, in whole cells, as of the last repaint — // measured off the painted model, never predicted. context/code/clients.md. diff --git a/web/shell.js b/web/shell.js index ced087f1..3bbf7508 100644 --- a/web/shell.js +++ b/web/shell.js @@ -27,6 +27,11 @@ export function indicator(collapsed) { export const ROW_PX = 26; export const COL_PX = 110; +/** How long a peer has to answer the handshake. Exported because the harness + * must jump *past* it: a spec with its own copy can understate it silently + * and every watchdog assertion then passes vacuously (clients.md). */ +export const HANDSHAKE_TIMEOUT_MS = 8000; + /** The extent a box of this size can show, in whole DATA cells: the frame is * charged for first, and at least one cell is always asked for. */ export function extentOf(box, frameCost, metrics = { rowPx: ROW_PX, colPx: COL_PX }) { @@ -107,6 +112,7 @@ export function createShell({ onStatus, timeoutMs, setTimer = setTimeout, clearT let shell = ""; let model = ""; let watchdog = null; + let arms = 0; const paint = () => onStatus(composeStatus(shell, model)); return { @@ -121,6 +127,7 @@ export function createShell({ onStatus, timeoutMs, setTimer = setTimeout, clearT /** A peer that never answers the handshake. Deliberately about the *first * frame*, not either peer's own failure modes — clients.md. */ arm(message) { + arms += 1; clearTimer(watchdog); watchdog = setTimer(() => { watchdog = null; @@ -140,5 +147,8 @@ export function createShell({ onStatus, timeoutMs, setTimer = setTimeout, clearT this.setShell(text); }, armed: () => watchdog !== null, + /** How many times the watchdog has been armed. A test denying its message + * needs this to be nonzero, or it is denying an unreachable event. */ + timesArmed: () => arms, }; }