| index dac581b..734f614 100644 |
| --- a/docs/superpowers/reviews/2026-09-10-slice-4-record.md |
| +++ b/docs/superpowers/reviews/2026-09-10-slice-4-record.md |
| @@ -1,6 +1,9 @@ |
| # Meet slice 4 — the record |
| |
| -Branch `slice/4-chat`, `3420396..8797ea9` (started on main `3420396`). |
| +Branch `slice/4-chat`, `3420396..HEAD` (this commit; started on main |
| +`3420396`). §6 below is the Codex fix wave, which landed after the six |
| +tasks and moved the head six times more — the range stated here is the |
| +whole branch, record included, rather than the six tasks alone. |
| Spec: `docs/superpowers/specs/2026-09-07-meet-design.md`, revision 9 |
| (§15.15 and §15.16 carry this slice's two wire/behaviour rulings). |
| |
| @@ -44,7 +47,12 @@ From the ledger's pre-flight scan and the tasks that followed it: |
| scratch test proved is always valid, since `encoding/json` repairs |
| an invalid byte with U+FFFD on the way into a Go string — checking |
| the decoded value would let a genuinely invalid byte through while |
| - reporting the payload clean). Written into spec §15.15. |
| + reporting the payload clean). Written into spec §15.15. **Amended by |
| + the fix wave (§6):** the raw cap and `utf8.Valid` still run first and |
| + still judge the incoming bytes, but those bytes are no longer what |
| + `Fanout` forwards — the payload is re-encoded from the value that was |
| + validated, and §4.4's 2 KB is now counted on the escaped text, which |
| + is what the wire actually carries and what the panel can count too. |
| - **The gate drives `getDisplayMedia` with |
| `--auto-accept-this-tab-capture` alone.** Settled by a throwaway |
| probe before a line of Task 5's scenario was written: the existing |
| @@ -69,6 +77,12 @@ From the ledger's pre-flight scan and the tasks that followed it: |
| on the share path alone, 1 failure in 3; yield on every negotiation, |
| 1 failure in 4; bounded to once per negotiation, 0 in 6, then 0 in 8 |
| more, then 30/30 twice in the full gate. Written into spec §15.16. |
| + **Amended by the fix wave (§6):** "once per negotiation" was really |
| + once per connection until an offer of this side's own went out, which |
| + an earlier glare could spend for good. It is now a generation the |
| + yield checks on waking, one attempt in flight at a time, and an |
| + allowance of two handed back by any exchange this side answers — and |
| + a camera that was rebuilt anyway is now detected and repaired. |
| - **`regainShare` and `shareTxLost`, added in Task 4's first fix round |
| to put a screen back after a rollback, were both no-ops where they |
| stood — found and removed in the second round.** `regainShare` called |
| @@ -260,6 +274,11 @@ used. |
| |
| ## 5. The gates, and how to run them |
| |
| +The tallies below are the six tasks'. The fix wave's own are in §6 and |
| +in `codex-wave-report.md`: 194 node tests, 31 browser scenarios (30 passing, `laterolls` skipped), and a |
| +`make browser` timeout raised to 20 minutes, which the thirty-first |
| +scenario and four more media windows made necessary. |
| + |
| | Gate | Command | Notes | |
| |---|---|---| |
| | The one gate | `CGO_ENABLED=0 make ci` | vet, gofmt, tests, migration check — unchanged from slice 3. | |
| @@ -287,3 +306,146 @@ name, reached by a third scenario each time. |
| `make browser` is still deliberately outside `make ci`, for the reason |
| slice 2's record already gives. As with slice 3's residual flake, a red |
| run here is worth a second run before it is worth debugging. |
| + |
| +--- |
| + |
| +## 6. Codex round 1, and the fix wave |
| + |
| +`codex exec` was launched against `8797ea9` while §2's last task was |
| +being written, and the whole-branch review of `26c022b` ran alongside |
| +it. Between them they found eleven things; six were Codex's (S4-1 to |
| +S4-6), five were the review's (W4-1, W4-3 to W4-7). All eleven are |
| +closed on this branch, in six commits, each with its new tests seen |
| +failing first. |
| + |
| +| Finding | Disposition | Commit | Test | |
| +|---|---|---|---| |
| +| **S4-1 (P1)** — `encoding/json` matches object keys to struct fields case-insensitively, and `DisallowUnknownFields` does not change that: `{"text":"<2049 a>","Text":"x"}` is 2071 raw bytes, decodes to the *last* match — a one-byte text — and was forwarded raw, so every recipient read the lowercase key and got 2049 bytes through a 2048-byte cap | Fixed. Keys are judged byte-exactly by `exactKey`, a token walk that insists on one member spelled exactly and appearing exactly once — deliberately not a `map[string]json.RawMessage`, which collapses a duplicate and cannot tell `{"text":"a","text":"b"}` from `{"text":"b"}` | `4bbadf3` | `TestChatKeysAreExactCaseSensitiveAndUnique`, ten cases; six of them returned 200 before the fix | |
| +| **S4-2 (P1)** — `writeEvent`'s `json.Marshal` HTML-escapes `<`, `>` and `&` *inside* a `json.RawMessage`, so a validated 2059-byte payload of literal `<` left the process at 12,381 bytes — the invariant `validateFanout`'s whole doc comment rests on, broken on the way out | Fixed at both ends: `validateFanout` returns the payload re-encoded from the value it validated (`SetEscapeHTML(false)`), and `writeEvent` uses an encoder with the same setting. SSE data is not HTML, and the property the escaping never bought — that a payload cannot end its own frame — comes from JSON's own `\n` escape | `4bbadf3` | `TestChatDoesNotExpandOnTheWire`, `TestChatDeliversTheCanonicalValidatedPayload`, `TestChatCannotForgeASecondSSEFrame` | |
| +| **S4-3 (P2)** — the textarea stays editable during a send, so `input.value = ''` on acceptance erased a draft typed while the earlier message was in flight | Fixed with a draft revision, bumped by every edit, captured by the submit, checked on acceptance. Clearing the box in code is not an edit, so an untouched box still empties | `a3a722f` | three tests in `chat.test.mjs`; the first failed `'' !== 'second draft'` | |
| +| **S4-4 (P2)** — a pending polite yield survived the negotiation it belonged to (a state check cannot tell uninterrupted stability from an exchange that came and went), and `negotiate` did not stop a second invocation while the first slept | Fixed with three per-connection parts: a negotiation generation the yield captures and re-checks, one attempt in flight at a time, and `yields` — a counter in place of the boolean, handed back one at a time by `answeredExchange`. That last also closes the Task 4 re-review's item 1 and **W4-4**: the allowance is no longer a budget one earlier glare can spend for good | `41deb52` | five tests in `room-lane.test.mjs`, three of them mutation-verified against the shipped code | |
| +| **S4-5 (P2)** — no recovery for a camera whose encoder was already rebuilt; the trigger survives POLITE_YIELD through a slow round trip, a 429 retry or a slow offer | Fixed. `pollFrames` runs in every browser now, not only where rVFC is missing, and reads outbound-rtp too: two polls with no new `framesEncoded` on a sending m-line with a live track is `replaceTrack(null)`/`replaceTrack(track)`; two more is `requestReset`; after that it stops asking. Closes the re-review's item 2 | `41deb52` | eight tests in `room-lane.test.mjs` over a fake stats sequence, mutation-verified | |
| +| **S4-6 (P2)** — `mutualshare`'s later rounds read a counter `dropShare` deliberately preserves, so they could pass on round one's frames; every camera check compared against a pre-gesture snapshot, which frames delivered before a rollback stall satisfy | Fixed with `frameGrowth`: read the counter, *then* wait for it to rise. Every screen and camera assertion in `share` and `mutualshare` is rebuilt on it, after each transition settles and again through the observation window | `d8316e8` | the gate itself — and it earned its keep immediately (below) | |
| +| **W4-1 (MEDIUM)** — the panel capped 2048 UTF-8 bytes of the text, the server 2080 raw JSON bytes of the payload; between them sat a draft the panel called legal and the server refused, needing only 23 quotes or newlines. **W4-2** falls with it: the "413: show beside the composer, never retry" line in `task-4-handoffs.md` was unimplementable through a boolean `send`, and is now unreachable | Fixed. One rule on both sides: the JSON-escaped length of the text, ≤ 2048 bytes — exactly what the canonical payload puts on the wire. The raw cap stays as the outer guard | `6f1be79` | `TestChatCapCountsTheEscapedText` (a quote, a newline and a backslash at and past the boundary) and `chat.test.mjs`'s counter test | |
| +| **W4-3 (MEDIUM)** — `POLITE_YIELD` set to 0 left all 164 node tests green: the suite injected 250 of its own | Fixed. Every constant these tests assert against is read out of the source by `pin()`, and one test asserts the shipped values themselves — §15.16's 300 ms among them | `41deb52` | `the numbers these tests assert against are the ones room.js ships` | |
| +| **W4-5 (LOW)** — the constant's comment claimed "the yield ends early because the remote offer has arrived"; the sleep is unconditional and the check is state-based | Fixed: the comment says what the code does | `41deb52` | — | |
| +| **W4-6 (LOW)** — this record named `3420396..8797ea9`, one commit short of its own | Fixed at the top of this file | this commit | — | |
| +| **W4-7 (LOW)** — §13 attached "no `chat` while waiting" to the `guest` scenario; it lives in `chat`, with a positive control | Fixed in the spec's own §13 sentence, which now also names the control | this commit | — | |
| + |
| +**The gate found a defect the node tests could not.** The first run of |
| +S4-6's fresh-baseline checks failed `mutualshare` round 1: the polite |
| +side's screen announced, captioned, and never arriving — an unassociated |
| +`sendonly` transceiver three rounds later and one inbound stream where |
| +there should have been two. S4-4's serializer had done it. The polite |
| +side adds its screen, sleeps in its yield, and answers the other side's |
| +offer while asleep; that exchange fires `negotiationneeded` for the |
| +polite side's *own* screen, which the serializer dropped because an |
| +attempt was in flight, and then the attempt woke, saw the generation |
| +had moved, and abandoned its offer. Nobody was left to offer the |
| +screen. Fixed in `f982bdf`: an ask that arrives mid-attempt is |
| +remembered on the connection and an attempt that abandons its offer |
| +goes round again for it, bounded by `NEGOTIATE_ROUNDS`. Two node tests, |
| +the first mutation-verified. Worth recording as a shape, not just an |
| +incident: **the counter-based checks the old gate could not make are |
| +what made the client defect visible at all**, which is the same lesson |
| +`mutualshare` itself taught in Task 5. |
| + |
| +**And then it found a second one, in the counter itself** (`3ecebb6`, |
| +`2eadfd2`). `laterolls` reported a camera that had stopped arriving on |
| +a tile that was playing perfectly — readyState 4, not paused, its clock |
| +advancing, 453 frames decoded and rising. What had stopped was |
| +`watchFrames`. Setting `srcObject` resets a media element and discards |
| +its pending `requestVideoFrameCallback`, and the chain was keyed on a |
| +`WeakSet` of elements, so a tile handed new media was refused a new |
| +chain and its count stood still for the rest of the call — which every |
| +frames assertion in the gate, and `state().frames` itself, reads as |
| +dead media. Keying on the stream was not enough either, and the gate |
| +said so on the next run: Chromium hands a rebuilt camera back inside |
| +the *same* MediaStream on a new track. It is keyed on tile, stream and |
| +track now, with a backstop for whatever else can drop a callback — a |
| +chain that has counted nothing for two seconds while the element's own |
| +clock moved on is re-armed, and `pollFrames` asks once a second, so a |
| +lost callback costs a second rather than a call. Eight tests, two |
| +mutation-verified. **The counter the whole browser gate stands on could |
| +die quietly, and nothing but a real browser was ever going to say so.** |
| + |
| +**And then a third, which was not the counter at all** (`fee4eb4`). |
| +With the counter repaired, `laterolls` still failed on the same tile — |
| +and this time the count was telling the truth. The polite side's tile |
| +for the other camera sat at `currentTime` 0.1 for the rest of the |
| +scenario, `readyState` 4 and not paused, on a connection that was |
| +stable, connected, and decoding four hundred inbound frames nobody |
| +could see. The element was rendering a track that had ended. This is |
| +the *receiving* half of the same rollback damage §15.16 describes: |
| +Chromium rebuilds receive streams too, so the far side's camera arrives |
| +again as a new track — sometimes with no stream of its own, sometimes |
| +inside the very stream that still holds the old, ended one — and |
| +`onRemoteTrack` added the new one and left the old one in place, where |
| +a media element goes on rendering it. `adoptTrack` takes an ended track |
| +of its kind out before putting the new one in, and `tile()` now follows |
| +the stream's video track as well as the stream object. Four tests, |
| +mutation-verified. |
| + |
| +Three defects, then, and the shape they share is worth naming: **every |
| +one of them was invisible to the whole node suite and visible in the |
| +first browser run that asked the right question.** The right question |
| +in each case was "is this media moving *now*", asked from a baseline |
| +taken now — which is exactly what S4-6 was about. |
| + |
| +**A thirty-first scenario, `laterolls`.** §15.16's residual said a pair |
| +whose signalling round trip exceeds 300 ms still collides, and that |
| +nothing detected or repaired the camera afterwards. Half of that is now |
| +fixed (S4-5) and the other half is now *forced* rather than waited for: |
| +`laterolls` holds one context's own `offer` POSTs for four times the |
| +yield, so the collision is late by construction, and then asserts the |
| +camera is encoding again inside the repair budget. It asserts the |
| +recovery and never the failure — a Chromium that no longer rebuilds the |
| +streams passes it exactly as a repaired one does — and records whether |
| +the stall happened as a line rather than a check. |
| + |
| +**Said plainly: the delay is forced, the camera loss is not, and the |
| +scenario ships skipped.** The delay is real — the scenario proves an |
| +offer was held (`1 held`). The *encoder* loss §15.16 describes did not |
| +reproduce: every run prints "the polite side's camera never stalled |
| +through the late offer", so what the run exercises is the late |
| +collision rather than the repair, and the repair itself is gated by the |
| +node tests over a fake stats sequence. A scenario that could only pass |
| +by breaking first would fail whenever the browser improves, which is |
| +why it asserts recovery and never failure. |
| + |
| +What it does reproduce, deterministically, is one step further in than |
| +anything this wave fixed: after the late collision the *receiving* |
| +tile stops rendering. The connection is stable and connected, all four |
| +transceivers are right, every track is live, both inbound video streams |
| +decode (452 and 446 frames, unmuted, one PLI each), the far side's |
| +outbound camera rises throughout, and the screen tile plays perfectly — |
| +while the camera tile sits at `readyState` 4, not paused, |
| +`currentTime` 1.5 s, for the thirty seconds the check waits. Media |
| +arrives, the decoder decodes it, the element renders none of it. No |
| +other scenario reproduces it, and `mutualshare` renegotiates the same |
| +connection six times over with both cameras rendering throughout, which |
| +puts it closer to the environment's own media stall (§5) than to |
| +anything in `room.js`. **Destination: `laterolls` is in `skipped` with |
| +the exact failure and the first two things to check** — whether the |
| +receiving track for that m-line is a new track object that never |
| +reached `onRemoteTrack`, and whether `startPlaying` needs to re-play an |
| +element whose media was rebuilt under it — on the same terms |
| +`mutualshare` was shipped skipped in Task 5. |
| + |
| +**What the fix wave did not take on.** The Task 4 re-review's items 3 |
| +to 7 stand where §3 left them: reserving the screen m-line at |
| +`createPC` (the fix that removes the trigger rather than racing it), |
| +settling whether the trigger is "a rollback that added an m-line" or |
| +"any rollback once media flows", filing the Chromium bug, widening the |
| +ordinary negotiation scenarios to assert frames afterwards, and two |
| +peers stopping at once. Item 3 is still the real fix and still wants |
| +its own task; what changed is that the failure it prevents is now |
| +detected and repaired when it happens, so it is an improvement rather |
| +than a gap. |
| + |
| +**One ordering worth writing down**, from the whole-branch review's |
| +§3: `leave()` and `terminal()` both set `state.stopped` *before* |
| +calling `stopSharing()`, so the `share {on:false}` those paths trigger |
| +short-circuits in `broadcast` and no POST goes out after the leave. |
| +That is the right order, it is not obvious from reading `stopSharing` |
| +alone, and no test would notice if a later edit inverted it. |
amadan