| new file mode 100644 |
| index 0000000..9013ca0 |
| --- /dev/null |
| +++ b/docs/superpowers/reviews/2026-09-10-slice-2-record.md |
| @@ -0,0 +1,293 @@ |
| +# Meet slice 2 — the record |
| + |
| +Branch `slice/2-room`, `77227de..` through the whole-branch fix wave. |
| +Spec: `docs/superpowers/specs/2026-09-07-meet-design.md`, revision 9 |
| +(§15 is this slice's departures from revision 8). |
| + |
| +Slice 2 built the room: the hub, `POST /signal`, the SSE stream, the |
| +holding screen, the browser client, and a Playwright gate that makes a |
| +real call between two real Chromium contexts. |
| + |
| +This file exists because everything below was, until now, written down |
| +only in `.superpowers/`, which is gitignored. The reasoning did not |
| +merge and did not survive the worktree. What follows is that ledger's |
| +substance: the decisions, why each one was taken, what the reviews |
| +found, and what is still open with the slice it belongs to. |
| + |
| +--- |
| + |
| +## 1. Rulings and decisions |
| + |
| +Ten of these changed what the spec says and are written into §15 of the |
| +spec itself, with their reasoning. They are listed here by name only, so |
| +this file stays the index and the spec stays the contract: |
| + |
| +§15.1 the hub session key · §15.2 the `Accept` gate and its 406 · |
| +§15.3 the `?probe=1` resume probe · §15.4 the scoped 410 · |
| +§15.5 `/signal`'s refusals before the limiter · §15.6 the narrowed |
| +room-page row · §15.7 the resume window enforced without waiting for the |
| +tick · §15.8 `leave` is not idempotent · §15.9 the stale-gen 409 carries |
| +no gen · §15.10 the four client-only terminal reasons. |
| + |
| +The rest are implementation decisions the spec has no opinion about. |
| + |
| +**The hub** |
| + |
| +- A hub method that dereferences `h.rooms[sqid]` must nil-check the |
| + room and return `ErrUnknownPeer`. Cost if wrong: a nil-map panic on a |
| + bad sqid. |
| +- A second resume of an already-attached peer replaces the attachment, |
| + and the old stream is told `lost {reason: "replaced"}`. No tombstone is |
| + written, so the participation itself continues. Two tabs of one |
| + browser are the one-peer-per-session rule, working as intended. |
| +- `Kind` is written in exactly one place (`setKindLocked`) and |
| + `Admitted` in exactly one (`admitLocked`), because a kind change owes |
| + every admitted peer a fresh roster and a second writer would be a |
| + second place to forget it. |
| +- `StandingFor` is never called under the hub lock. Every caller drops |
| + the lock, reads, retakes it, and **re-finds** the participation, |
| + because it may have ended while the read was in flight. |
| +- An admitted guest is never ended by a grant expiring (ruling 14): the |
| + participation is the authority once someone is in. `standingLostLocked` |
| + is the one place that decides what a loss of standing costs, because |
| + what it costs depends on the kind held. |
| +- `Admit` settles the actor's authority completely — found, standing |
| + applied, admitted, member — before it looks at the target at all. |
| + The other order let any joined participation learn from the shape of |
| + the refusal whether a candidate peer id existed in the room. |
| +- Headstone rooms are never reaped and `h.rooms` is never deleted from. |
| + Any future reaper must wait out `TombstoneLife`; the note sits beside |
| + `End`. |
| +- `gap` is the highest evicted `seq` per sender above `last[from]` — a |
| + deviation from the Task 3 brief, which asked for a simpler test. |
| + |
| +**The signal endpoint** |
| + |
| +- The rate limiter is keyed on session + room, never on anything the |
| + request body claims, and the key is built before the body is read. |
| + Keyed on the claimed peer id, one session could drain another's bucket |
| + (peer ids are published in rosters) or evade its own by rotating ids. |
| +- `peer` is 32 lowercase hex or 400, and `type` is logged only from a |
| + fixed list. Attacker-chosen text never becomes a map key or a log |
| + argument. |
| +- Hub errors other than the four sentinels are logged and answered 500. |
| + `ErrNotYours` and `ErrUnknownPeer` share one byte-identical 404 body |
| + including the boot id. |
| + |
| +**The pages** |
| + |
| +- The removal confirm page checks fresh member standing *and* an |
| + admitted member participation (`hub.PeerNameFor`), and names the |
| + person, not the meeting. |
| +- After a removal the confirm tab goes to `/m/{sqid}/join` — inert, |
| + opens no stream — not back to the room. Both confirm pages' Cancel and |
| + back links go there too. |
| +- The takeover confirm page 303s to `/join` when this session holds |
| + nothing to take over, and re-checks standing before `hub.Takeover`. |
| +- Toggle buttons keep a fixed accessible name (`aria-label` Microphone / |
| + Camera) with `aria-pressed`, plus a child span carrying the state text |
| + — "On"/"Muted", "On"/"Off" — marked `aria-hidden`. Never colour alone, |
| + and never a label that changes under the person's cursor. |
| +- The three device pickers use `ui/field-select`. AGENTS.md's `[x]` |
| + Fields rule is hard: `devices.js` fills the vendored component rather |
| + than hand-rolling an equivalent. |
| +- Load-time `getUserMedia` on the holding screen stands, with a |
| + `visibilityState` guard and a `visibilitychange` retry: it is a page |
| + the person navigated to on purpose. |
| + |
| +**The client** |
| + |
| +- Every terminal state carries a way out: a link to `/m/{sqid}/join`. |
| +- A stream that dies moments after its `hello` has proved nothing: the |
| + backoff is cleared only by a stream that stayed open five seconds. |
| +- The stall ceiling is gated on time as well as count — eight stalls |
| + *and* sixty seconds since the last `hello` — so a room that is merely |
| + slow is not declared unreachable. |
| +- `terminal()` clears `admitted` and keeps `peer`: the peer id names |
| + what this tab held, which is worth having in a diagnosis, and nothing |
| + reconnects with it once `stopped` is set. |
| + |
| +**The gates** |
| + |
| +- The browser gate reports a client failure as `t.Skip` carrying the |
| + exact text, never by deleting or loosening a scenario. |
| +- The buffer-overrun scenario pins `SO_SNDBUF` through a custom |
| + listener: without it the gate depended on the machine's `tcp_wmem`. |
| +- The renewal scenario is production-shaped — a six-second TTL through |
| + the development-only `X-Meet-Test-TTL` header, with a floor of four — |
| + rather than lowering the client's `HEALTHY_STREAM`. |
| + |
| +--- |
| + |
| +## 2. Whole-branch review findings, and what happened to them |
| + |
| +Whole-branch review at `cfc0dc0`: 53 files, 13,338 insertions, all three |
| +gates green, 48 mutations applied to a throwaway copy — 40 killed, 8 |
| +survived, every survivor a test gap rather than a bug. Verdict: merge |
| +with W-1 and W-2 fixed first. |
| + |
| +| # | Finding | Disposition | |
| +|---|---|---| |
| +| W-1 | The five guest-isolation audience filters (§4.3, ruling 9) are gated by nothing: each could be mutated away with the whole suite green, including two §13 names as slice-2 gates and one the browser `guest` scenario was written to catch | **Fixed in this wave.** Five hub tests, three meettest tests over HTTP, and a `guest` scenario with a second guest left knocking. Every mutation the reviewer lists is now killed. | |
| +| W-2 | The branch's whole record lives in a gitignored directory, and the spec was never revised past 8 | **Fixed in this wave.** Spec revision 9 (§15) carries the ten departures with their reasons; this file carries the rest. | |
| +| W-3 | Nothing proves the boot id changes across a restart | **Fixed in this wave.** The browser `restart` scenario compares `state().boot` before and after. | |
| +| W-4 | Dropping the subject from the hub session key is caught by nothing | **Fixed in this wave.** A test signs the same browser out and in again and asserts the old key no longer answers. | |
| +| W-5 | Two guards with no test: `Remove`'s self-target check, `hubStanding`'s ended-meeting early return | **Fixed in this wave.** The first has a test; the second is belt-and-braces behind callers that check `EndedAt` first, and its comment now says so. | |
| +| W-6 | `-race` cannot be run the way the repo is configured, on the first slice with real concurrency | **Fixed in this wave**, with a caveat: `make race` exists and AGENTS.md names it as the one exception to the cgo-free rule. The CI step is present but guarded — see §4. | |
| +| W-7 | `lost{buffer}` is sent for three endings that are not buffer overruns | **Carried — next fix wave.** See §3. | |
| +| W-8 | `TERMINAL.badlast` is unreachable | **Fixed in this wave.** The dead arm is gone; the 400 an attaching stream returns is invisible to `EventSource` and is handled as a stall, which is what actually happens. | |
| +| W-9 | Three wordings for one move | **Carried — the copy pass.** Already in the tracked copy catalogue. | |
| +| W-10 | The test hook ships to production | **Recorded, not fixed.** See §3. | |
| +| W-11 | Three hub maps only ever grow | **Recorded, not fixed.** See §3. | |
| +| W-12 | The tick has no per-pass deadline and does serial database reads | **Fixed in this wave.** Each pass runs under a deadline of `tickEvery`, with a test that a read which never returns cannot stall the pass. | |
| + |
| +Mutation survivors the reviewer reported, and where each is killed now: |
| + |
| +| Predicate | Killed by | |
| +|---|---| |
| +| `rosterFor`'s member-only waiting queue | hub, meettest, browser | |
| +| `rosterFor`'s admitted-only mesh | hub, meettest | |
| +| `admitLocked`'s admitted-only `peer` fan-out | hub, meettest, browser | |
| +| `refreshRostersLocked`'s admitted-only refresh | hub | |
| +| `knock`'s admitted-members audience | hub, meettest, browser | |
| + |
| +--- |
| + |
| +## 3. Carried items, and where they go |
| + |
| +**Next fix wave** |
| + |
| +- **W-7, `lost{buffer}` for three endings that are not buffer |
| + overruns** (`admission.go`'s `Leave`, `hub.go`'s unjoined timeout and |
| + lapsed resume window). `buffer` is the one reason the wire contract |
| + defines as *not* terminal — "followed by a resume". Today this is |
| + harmless because `room.js` sets `stopped` and closes the stream before |
| + the event can be read, but the safety lives entirely in the client, and |
| + §8's drive surface will have other callers. A peer whose join keeps |
| + failing can loop: unjoined timeout → `lost{buffer}` → resume → 404 same |
| + boot → fresh stream → join → thirty seconds → repeat, and the stall |
| + counter never fires because each round gets a `hello`. **Not fixed here |
| + because either fix changes the wire contract** — a new terminal reason, |
| + or an ending that sends no event at all — and this wave took no |
| + wire-contract changes. Fix it with the client change in the same |
| + commit, and re-run the browser gate. |
| +- **W-10, the test hook in the release binary.** `window.meet.state()` |
| + is defined unconditionally and `TestHandle` compiles into the release |
| + build (unreachable without `NewForTest`, which `main` never calls). |
| + Neither exposes anything secret, and the CSP keeps third-party script |
| + off the page. Confining either one costs more than it buys today: |
| + build-tagging `TestHandle` means `make ci` grows a `-tags` argument, |
| + and gating `window.meet` on the environment means the browser gate |
| + tests a page production does not serve. |
| +- **W-11, three hub maps that only grow.** `h.rooms` is never deleted |
| + from (declared; `End`'s comment explains why a reaper is dangerous). |
| + `h.bySession[sqid]` keeps an empty inner map per room forever, and |
| + `h.byPerson` a zero entry per person forever. All three are bounded by |
| + real meetings and real people — an outsider cannot mint a room id that |
| + reaches `Open`, because standing is checked first — so this is a slow |
| + leak on a long-lived process, not an attack surface. It is also |
| + invisible while every deploy empties the hub (§4). Fix it with, or |
| + after, whatever ends that. |
| +- **A just-added member gets 404 from the removal confirm page until the |
| + next tick.** Recorded in spec §15.11. |
| +- **Watch: the limiter now sits behind a gate that takes the hub lock.** |
| + Revisit if hub-lock hold time grows. Recorded in spec §15.5. |
| + |
| +**The copy pass** |
| + |
| +- W-9's three wordings for one move: "Use this tab instead" (holding |
| + notice and `room.js`'s `wayOut`), "Use this tab" (takeover confirm |
| + submit), and "This meeting is already open in another tab" versus |
| + "This meeting is open in another tab." |
| +- The holding notice's "already open in another tab" is untrue during |
| + the resume window; "You are already in this meeting from this browser." |
| + is the candidate. |
| +- "Remove `<name>` from the call?" reads oddly for a guest who is not in |
| + the call yet. |
| + |
| +All three are in `docs/superpowers/copy/2026-09-10-slice-2-strings.md`. |
| + |
| +**Slice 3 — relay and `/ice`** |
| + |
| +- `createPC` passes `{ iceServers: [] }` (`room.js`), with the floor |
| + notice keyed on `cfg.RelayConfigured`. Those are the two places to |
| + change. |
| +- `GET /m/{sqid}/ice` is §2.5's "participation, admitted" row — the same |
| + authority as a non-`join` signal. `hub.Find` plus `p.Admitted` is the |
| + whole check; do not consult standing (ruling 14). |
| +- Renewal (§5.3) needs a timer that survives a resume. The browser gate |
| + already has a `renewal` scenario for the *stream* TTL using the |
| + development-only `X-Meet-Test-TTL` header; copy that shape for the |
| + credential rather than inventing a second one. |
| +- ICE restart already has a client path: `restartIce()` with |
| + `requestReset` as the fallback, and the `replay` scenario asserts |
| + `have-local-offer` to prove it fires. |
| +- W-12 is now bounded, but `/ice` adds a per-request credential mint on |
| + a path that already does a database read. Watch the tick's deadline. |
| + |
| +**Slice 4 — chat and screen share** |
| + |
| +- `chat` and `share` are already named in `messages.go` and refused with |
| + 400 in `signal.go`. Removing that arm and adding the fan-out is the |
| + whole server change; `broadcastLocked(r, m, admittedPeers)` is the |
| + audience. |
| +- §4.4's 2 KB chat cap has no code yet. It must be checked against the |
| + decoded text, not the body, and it must not read the text for anything |
| + else — the captured-log test extends to *accepted* chat in slice 4, |
| + which is the first time an accepted message has content worth |
| + protecting. |
| +- W-1 is fixed, so chat's "arrives at the admitted context and not at a |
| + waiting one" is now two lines: extend |
| + `internal/meet/hub/isolation_test.go` and |
| + `internal/meettest/isolation_test.go`, which exist for exactly this. |
| +- Screen share adds a second video track per peer. `tile()` keys on peer |
| + id and `renderTiles` rebuilds from `pcs`, so the tile model needs a |
| + track dimension **before** the share arrives, not after. |
| + |
| +--- |
| + |
| +## 4. Deploying this slice |
| + |
| +**Every deploy drops every live call.** The hub is per process and dies |
| +with it. This is spec'd (§4.2, §4.5) and not solved: slice 2 is the |
| +first release where it has a user-visible cost. |
| + |
| +What a deploy does to people who are in a meeting: |
| + |
| +- every browser's resume is refused with a 404 carrying the new boot id; |
| +- each one closes every peer connection, then joins afresh; |
| +- **members** re-enter at once and negotiate again — a few seconds of |
| + black tiles; |
| +- **guests go back to waiting** and must be admitted again by a member. |
| + |
| +So: **deploy outside meeting hours** until slice 3, or until a drain |
| +exists. There is no partial mitigation worth the words — the participants |
| +are in memory, and the memory goes. |
| + |
| +Nothing else about a deploy is unusual. Migrations apply at boot, once |
| +each; `GET /healthz` and `GET /api/version` are answered by the framework |
| +outside app middleware. |
| + |
| +--- |
| + |
| +## 5. The gates, and how to run them |
| + |
| +| Gate | Command | Notes | |
| +|---|---|---| |
| +| The one gate | `CGO_ENABLED=0 make ci` | vet, gofmt, tests, migration check. Cgo-free, as AGENTS.md requires. | |
| +| Race | `CGO_ENABLED=1 go test -race ./internal/...`, or `make race` | **Needs `CGO_ENABLED=1`**: `-race` requires cgo, and the repo is cgo-free everywhere else. This builds a test binary, never a release, which is why it is the one exception. | |
| +| Browser | `make browser` | ~130 s, 18 scenarios. Needs node, a network fetch of Playwright and Chromium, and **UDP** — inside the Claude Code bash sandbox Chromium gathers zero ICE candidates and every call fails with frames stuck at 0, so **run this target with the sandbox off**. | |
| + |
| +`make browser` is deliberately not part of `make ci`: the one gate the |
| +team runs on every push must not depend on a toolchain the app itself |
| +does not use. Run it before every review that touches a page or the |
| +client. Nothing enforces that, which is a known weakness and the reason |
| +this row is here rather than in a comment. |
| + |
| +**The race step in CI.** `.amadan/ci.d/50-race` runs `make race` when |
| +the runner has a C compiler and prints a skip line when it does not. The |
| +guard is there because the cloud runner's provisioning |
| +(`amadan/docs/ci-cloud-runner.md`) installs the Go toolchain and Node and |
| +never names a C compiler, and a step that fails on every push would be |
| +worse than no step. **When somebody confirms `cc` on the runner, delete |
| +the guard** — a race gate that can silently skip is half a gate. |