| new file mode 100644 |
| index 0000000..d5ed9a4 |
| --- /dev/null |
| +++ b/docs/superpowers/reviews/2026-09-11-slice-5-record.md |
| @@ -0,0 +1,246 @@ |
| +# Meet slice 5 — the record |
| + |
| +Branch `slice/5-drive`, `1c66309..HEAD` (this commit; started on main |
| +`1c66309`). Spec: `docs/superpowers/specs/2026-09-07-meet-design.md`, |
| +revision 9 (§15.17 carries this slice's wire ruling). |
| + |
| +Slice 5 built the drive surface: the `agents` table and its store |
| +methods, Settings' mint-once/revoke screen, the `/api/v1/meetings` |
| +bearer API and its five operations, and the robot badge and principal |
| +on the meetings list — four tasks, each with its own review and fix |
| +rounds, kept in |
| +`.superpowers/sdd/2026-09-11-meet-slice-5-drive/progress.md` and the |
| +`task-N-report.md` files. Like slices 2 through 4's, that ledger is |
| +gitignored and does not survive the worktree; this file is its |
| +substance. |
| + |
| +--- |
| + |
| +## 1. Rulings and decisions |
| + |
| +From the ledger's pre-flight scan, its rulings, and the tasks that |
| +followed: |
| + |
| +- **`/api/v1` is mounted outside `csrf.Protect`.** **Ruling**, verified |
| + against rastrillo's own `csrf.SameOrigin` before Task 3 was written: |
| + a state-changing request carrying none of `Sec-Fetch-Site`, `Origin` |
| + or `Referer` is refused, and an agent — not a browser — sends none of |
| + the three, so a POST from one would 403 before reaching a handler. |
| + Safe because the surface honours no cookie: there is no ambient |
| + credential a cross-origin page could spend through it, which is the |
| + whole of what CSRF defends against. `/api/v1` is its own chi group, |
| + registered before the group that carries `csrf.Protect` and every |
| + session middleware, so a bearer never reaches `RequireSession` and a |
| + cookie never reaches the API. Written into spec §15.17; |
| + `TestABearerOnAnHTMLRouteIsIgnored` and |
| + `TestAPIRequiresABearerAndIgnoresCookies` prove both directions. |
| +- **A missing or malformed bearer is 401; every token-shaped string |
| + goes to the store.** **Ruling.** Unknown, revoked and reach-less |
| + tokens all answer the identical not-found body — `apiNotFound()`, |
| + one map through one writer — so nothing about which of the three it |
| + was is ever observable. A malformed bearer is judged on the header |
| + alone, never the token's own shape: there is no `mt_` prefix check |
| + anywhere on this path, deliberately, which is what keeps a made-up |
| + token indistinguishable by status from a real one that was revoked |
| + (Task 3's deviation 5, resolving an apparent conflict in the brief in |
| + the test's favour). Written into spec §15.17. |
| +- **`end`/`links` on an ended meeting is 409 `{"error":"ended"}`; the |
| + per-agent limit is 60/min, burst 60, keyed on `actorForAgent` and run |
| + after the token resolves.** **Ruling.** Keying the limiter on the raw |
| + header first would let 4096 fake tokens (`maxAgentBuckets`) evict |
| + every real agent's bucket before one is ever checked to exist. |
| + Deliberately no client-address limiter on this surface: behind the |
| + platform's relay socket every request the app sees shares one |
| + address, so such a limiter would let one flood lock out every real |
| + agent along with whatever was flooding — carried to carlos/platform |
| + as an edge concern rather than solved here (§3 below). |
| +- **`created_by` is the principal for an agent's write, the person's |
| + display name for a member's, and the raw actor string if the row |
| + behind it no longer resolves.** **Ruling**, `store.creatorLabel` |
| + (Task 3, reused by Task 4 rather than duplicated). A label, never an |
| + authorization input — nothing is decided on it, and every member |
| + already sees every meeting on the list, so naming which agent made |
| + one discloses nothing new (`agentAnyByID`'s own doc comment). |
| +- **The API ignores cookies entirely; HTML routes ignore bearers.** |
| + **Ruling**, proven both directions by Task 3's tests as above. |
| +- **Title length is 200 characters counted as runes, on both the form |
| + and the API.** Task 3 shipped a byte count (deviation 6: "two |
| + surfaces disagreeing about what a 200-character title is seemed |
| + worse than the brief's wording", which itself said characters); the |
| + Task 3 review's addendum overrode it — a byte count lets the API |
| + refuse a title the screen's own copy ("Keep the title under 200 |
| + characters.") promises is fine the moment it holds anything outside |
| + ASCII. Task 4 folded in the fix: one shared `titleTooLong` |
| + (`handlers_meetings.go`) measuring `utf8.RuneCountInString` against |
| + `maxTitleLen`, called from both `createMeeting` and |
| + `apiCreateMeeting`, with a 200-rune (600-byte) accept and a 201-rune |
| + refuse added to `TestAPIBodyRules`. Written into spec §15.17. |
| +- **The meeting object is exactly six keys, and three of the five API |
| + responses never carry a `"link"` key.** Task 3's review found a |
| + surviving mutation (a `link` field added to the GET response would |
| + not have been caught) and the addendum asked for the gap closed: |
| + `TestAPIListsCreatesLinksAndEnds` now asserts `"link"` absent on the |
| + bare GET, on the end response, and on every list row, asserts |
| + `"admitted"` absent on a GET taken *after* ending (not just on the |
| + end response itself, closing the `m.EndedAt == nil` guard's own |
| + blind spot), and pins the list row's key set to the six names on the |
| + wire. |
| +- **The robot badge is `ui/badge` plus the lucide `bot` icon, never a |
| + hand-rolled equivalent.** AGENTS.md: `[x]` conventions are enforced |
| + by a vendored component. The icon alone would not satisfy AGENTS.md's |
| + "state is never colour alone" rule on its own account — a shape is |
| + not a colour, but it is still not text — so the badge's own word |
| + ("Agent") is what actually carries the meaning; the icon needs no |
| + separate `aria-label` because it sits beside that word. The `bot` |
| + icon's own class carries `lucide-bot` alongside this app's usual |
| + `icon`, a one-icon deviation from the pattern every other slug |
| + follows, recorded so the next reader of `icons.go` does not wonder |
| + why (§3 below has the reasoning in full). |
| +- **`meetingView.CreatedByPrincipal` is resolved once per distinct |
| + actor across a whole list, not once per row.** Task 3's report |
| + flagged the N+1 this move would create if the label ever joined |
| + `meetingView`; Task 4 closed it with `meetingViewOfCached`, a shared |
| + `labels map[string]string` `listMeetings` passes across its whole |
| + loop, while `meetingViewOf` (still called by the API's four |
| + single-row paths, whose call sites were not touched) passes a |
| + throwaway map of its own. The list's separate N+1 on link-presence |
| + counts is unrelated and still stands, unaddressed by this slice. |
| + |
| +--- |
| + |
| +## 2. Per-task review dispositions |
| + |
| +| Task | Built | Review | Fix rounds | Final | |
| +|---|---|---|---|---| |
| +| 1 — the `agents` table and store | `Agent` model, `0002_agents.sql` (generated, then renamed from `rastrillo`'s own `0002_create_agents.sql`), `newAgentToken`/`hashAgentToken`, `createAgent`/`agentsFor`/`revokeAgent`/`agentByToken`/`agentByID`, `principal`, `actorForAgent`; three tests | Needs fixes: MEDIUM `operatorFor` fell back to a bare-ID `Person` instead of failing closed; MEDIUM `agentByID` was unscoped to its operator; LOW-MED `createAgent` accepted a zero operator, no cap on live agents, no duplicate-name check; LOW test hygiene (tokens printed on failure, ordering/randomness/shape unproven) | Round 1 (`e9539a3`): all fixed — fail-closed lookups via a new `withOperator`/`personByID` path, `agentByID` gained an operator parameter and the owned-row 404 shape, `createAgent` refuses a zero operator and enforces the 20-live cap and the live-duplicate-name check inside its own transaction, new tests for the token's shape, ordering and the fail-closed operator case | Complete. | |
| +| 2 — Settings: mint, show once, revoke | `handlers_settings.go`, `settings.html`, `agent_once.html`, four routes inside `requireMember`, `Viewer.Member`, `page`/`ErrorPage` as `*app` methods; nine tests | Needs fixes, minor: no `Cache-Control: no-store` on the mint and list pages; no length guard on a token before rendering it (a 64-hex sanity check); `Autofocus` on a field that did not need it; a double `WriteHeader` on the 422 path; a report count off by one | Round 1 (folded into Task 1's fix round, same commits `e9539a3`/`e8517d1`): no-store added to both once-shown pages, the hex guard added to the list, `Autofocus` dropped, `renderSettings` made to own the status write once, the report corrected | Complete. | |
| +| 3 — the API: bearer, reach, five operations | `api.go` (`requireAgent`, `apiJSON`, `apiNotFound`, `refuseAPI`, `apiServerError`, `apiDecode`, `apiRoutes`, the five handlers), `hub.AdmittedCount`, `store.meetingViewOf` (factored out of `listMeetings`), `store.agentAnyByID`/`creatorLabel`/`withOperator`; eight tests, one shipped `t.Skip`ped for Task 4 | **Clean**, 8 mutations run (6 killed; 2 survivors: a `link` key left addable to a GET response, an `admitted` key left readable off an ended meeting's own GET) | Addendum folded into Task 4 rather than a separate round: both survivors closed by new assertions in `TestAPIListsCreatesLinksAndEnds`; title length moved from bytes to runes on both surfaces, one shared helper; the list row's six-key set pinned. §15.17 recorded the csrf mount, the header-only 401 rule, the API's own non-promises (§3 below), and the badge | Complete, addendum closed by Task 4's commit. | |
| +| 4 — the badge, the record, the spec note | `meetingView.CreatedByPrincipal`/`ByAgent` (`meetingViewOfCached`), `icons.go`'s `bot`, `index.html`'s badge and "by `<principal>`" line, the Task 3 addendum (rune-counted titles, the closed mutations, the pinned key set), spec §15.17, this record | This commit's own work; no separate review round within the task | — | This commit. | |
| + |
| +--- |
| + |
| +## 3. Carried items, and where they go |
| + |
| +- **The list's link-presence count is still N+1, unrelated to the |
| + label move.** `listMeetings` now shares one `creatorLabel` cache |
| + across a whole render (this task), but `meetingViewOfCached` still |
| + runs one `MeetingLink` count query per row, exactly as `listMeetings` |
| + always has since before this slice. **Destination: unchanged** — a |
| + team's meeting list is small (§8.2 does not ask for paging either, |
| + for the same reason), and fixing both N+1s at once was judged a |
| + larger, unrelated change than this task's brief asked for. Worth a |
| + look if a team's list ever grows enough to notice. |
| +- **No client-address limiter on the API, and unauthenticated flood |
| + control generally.** Recorded as a ruling in §1 above and in spec |
| + §15.17: behind the platform's relay socket every request shares one |
| + address, so a limiter keyed on it would be worse than none. |
| + **Destination: carlos/platform**, the same edge question Task 3's |
| + review raised and the addendum confirmed does not belong in this |
| + app. |
| +- **NUL and lone-surrogate acceptance in titles, on both surfaces.** |
| + Titles are unfiltered data once they pass the length and non-empty |
| + checks; a NUL byte or an unpaired UTF-16 surrogate (reachable only |
| + through a client encoding one, since `encoding/json` itself accepts |
| + a lone surrogate escape) is stored and returned as sent. |
| + **Destination: informational, not a defect** — recorded in spec |
| + §15.17 rather than fixed, on the same reasoning `TestAPIBodyRules`'s |
| + `<script>` case already establishes: a title is never rendered as |
| + anything but an escaped JSON string or HTML text, so nothing |
| + downstream is at risk from what it contains. |
| +- **§8.3, the MCP adapter.** Deferred by the spec itself ("until a |
| + consumer asks by name"), not touched this slice. **Destination:** |
| + unchanged — no consumer has asked yet. |
| +- **The per-agent limiter shares no budget with the signalling |
| + limiter, and is keyed only on the agent, not the agent *and* the |
| + operation.** Noted in Task 3's report as a concern, not acted on: |
| + sixty writes a minute is as available to an agent as sixty reads. |
| + **Destination: watched, not fixed this slice** — nothing in the spec |
| + asks for a finer-grained budget, and a single 60/min ceiling is the |
| + one §8's Global Constraints actually specifies. |
| +- **`admitted` is per-process.** Read from the in-process hub exactly |
| + as every other count the hub answers is (§4.2); on a multi-host |
| + fleet it would be that host's count, not the meeting's. |
| + **Destination: unchanged**, consistent with everything else the hub |
| + answers and already noted for slices 2 through 4. |
| +- **The whole-branch review.** `whole-branch-review.md`'s brief was |
| + written into the ledger before Task 4 started and has not been run |
| + as of this commit — its own six questions (trust end to end, the |
| + five operations against §8.2, coherence with earlier slices, the |
| + three gates, that every carried item above has a destination, and |
| + what blocks merge versus deploy) are for whoever runs it next. |
| + **Destination: its own entry in this file**, the way slice 4's |
| + Codex rounds were appended to that slice's record after the fact — |
| + not run or fabricated here ahead of the result. |
| + |
| +--- |
| + |
| +## 4. Deploying this slice |
| + |
| +**One migration, `0002_agents.sql`, applied at boot.** `Schema` |
| +(`migrations.go`) embeds `internal/meet/migrations/*.sql` in filename |
| +order and `migrate.Apply` runs whatever a given database has not yet |
| +seen, once, on boot — never re-run, never reversed. This slice adds |
| +exactly one file to that directory, generated by `rastrillo migration |
| +generate` after `Agent` was added to `models.go`: one `CREATE TABLE |
| +agents`, a unique index on `token_hash`, an index on |
| +`operator_person_id`, nothing else. `rastrillo migration check` |
| +("models and migrations agree") passes on every gate run in this |
| +slice's ledger, including this commit's. |
| + |
| +**The filename was renamed before it ever shipped, and that is safe.** |
| +`rastrillo migration generate` first wrote `0002_create_agents.sql`; |
| +Task 1 renamed it to `0002_agents.sql` to match the plan before |
| +committing (Task 1's report, deviation 1). `migrate.FromFS` derives |
| +each migration's identity from its filename, so the two names are, to |
| +the migration runner, two different migrations — **a developer |
| +database that had already applied the pre-rename |
| +`0002_create_agents` would fail at boot**, seeing `0002_agents` as a |
| +new, unapplied migration racing to create a table that already exists. |
| +This is safe precisely because no such database exists: the rename |
| +happened in the same commit as the migration's first appearance on |
| +this branch, `0002_create_agents` was never pushed, deployed, or |
| +merged under that name, and `git log` for this repository has no |
| +commit carrying it. Anyone resuming this worktree from a stash or an |
| +old checkout predating `533b629` should discard rather than reuse a |
| +database file from that window, on the same reasoning. |
| + |
| +**Every deploy still drops every live call**, exactly as slice 2's |
| +record already states (spec §15.11 — the hub is per process and dies |
| +with it) and slices 3 and 4 did not change: this slice adds nothing to |
| +the hub's own durability, and the drive surface itself never touches a |
| +live call — its five operations are metadata (list, create, replace a |
| +link, end) and none of them joins a room (§8.2, "no operation joins a |
| +call", ruling 11). An agent's own in-flight request at deploy time |
| +either completes against the old process or is refused by the load |
| +balancer the way any other request would be; nothing about this slice |
| +changes that. |
| + |
| +**A revoked or reach-lost agent takes effect at its very next |
| +request, with no deploy or restart needed.** §8.1's reach re-derivation |
| +runs inside `requireAgent` on every call, so removing an operator from |
| +the allowlist or revoking their agent through Settings is live the |
| +moment the write commits — `TestAPIReachIsTheOperatorsStandingOnEveryRequest` |
| +is the test-level proof, and it needs no server restart to pass. |
| + |
| +--- |
| + |
| +## 5. The gates, and how to run them |
| + |
| +| Gate | Command | Notes | |
| +|---|---|---| |
| +| The one gate | `GOCACHE=/home/paulca/.cache/tmp/claude-1001/gocache CGO_ENABLED=0 make ci` | vet, gofmt, `go test ./...`, `rastrillo migration check` — unchanged from slice 4. Green on this commit; `internal/meettest` alone (the package Tasks 2-4 mostly touch) runs in the twenty-to-thirty-second range each pass. | |
| +| Race | `CGO_ENABLED=1 go test -race ./internal/...` | Run through every task's fix rounds this slice; green throughout, including this commit's. | |
| +| Browser | `make browser` | **Unchanged by this slice** — no file under `internal/meet/hub`'s signalling path, `room.js`, or any `test/browser/*.test.mjs` was touched; the drive surface is a second, independent HTTP surface beside the one the browser gate exercises. Still needs the sandbox off (Chromium gathers zero ICE candidates and `getDisplayMedia` produces nothing crossing the connection inside the bash sandbox, as every slice's record from 2 onward already notes). Run once on this task's own final head, in the foreground: `go vet -tags browser` clean, 232 node tests across the five `test/browser/*.test.mjs` files, and **`TestCallInARealBrowser` 31 of 31, 0 skipped, 522.71s** — no scenario lost to the environment's own media-stall flake this run. The whole-branch review (§3 above) or slice 6 should run it again on whatever head actually ships, since nothing here guarantees no later fix round in this slice touches those files. | |
| + |
| +**Why a slice that added a JSON API and a database table needed no new |
| +browser scenario.** §8.4 already draws this line: the meeting API is a |
| +plain JSON surface a native client would use directly, with nothing in |
| +it that assumes a browser, so it is proven by `internal/meettest`'s Go |
| +HTTP tests (`api_test.go`) rather than by driving Chromium. The one |
| +place this slice's work is *visible* inside a browser — the robot |
| +badge and principal on `index.html` — is server-rendered HTML with no |
| +script involvement at all (AGENTS.md: screens work with JavaScript |
| +disabled), so `TestAPIWritesAreMarkedAsTheAgents` (a Go test using the |
| +existing browser-cookie harness to sign in and read the rendered page, |
| +not a real browser) is the right level to prove it at, and `make |
| +browser`'s own 31 scenarios needed nothing added. |
amadan