oficina / meet Public

test: Codex round 2 fixes S5-6, S5-7

S5-6: the concurrent test proved nothing it claimed — closing one
channel does not make either request read the running meeting before
the other writes, and the schedule where the first finishes before the
second reads passes under the old code too. It stays as a smoke test
with honest claims, and the gate is now a test that forces the
interleaving: a test-only GORM query callback on the harness's own
x.db.G holds one request just past its meetingBySqid read while a
second request ends the meeting and commits, then lets it go on into
its write. No production code gained a hook. Mutated on a throwaway
copy under $TMPDIR to prove it: with the pre-transaction checks back
and the in-transaction ones gone, it fails 5 runs out of 5, both a
stalled end and a stalled replacement.

S5-7: the 413 wording promised more than MaxBytesReader does. §15.17
now says the cap is hit when reading reaches it, gives the precedence
401 → 404 → 429 → the body rules, and records that an oversized body
malformed from its first bytes may answer 400 first and that a GET
carrying a body is answered 200 with the body unread. No GET-body
enforcement added. New positive tests pin the other half: a create
ending in a newline or padded with whitespace is 201, which is
deliberately not the bodyless rule.

The round-1 report's determinism note and its S5-3 decline rationale
are corrected in place; the record §2 carries a Codex round 2 block.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
Paul Campbell pushed by paul@keymail.dev 11fb5731b7c565b2fed86e6afe3cc00af8711249 parent c523a12
3 files changed, +242 −25
  • docs/superpowers/reviews/2026-09-11-slice-5-record.md +7 −2
  • docs/superpowers/specs/2026-09-07-meet-design.md +56 −14
  • internal/meettest/api_test.go +179 −9

CI passed — run details

diff --git a/docs/superpowers/reviews/2026-09-11-slice-5-record.md b/docs/superpowers/reviews/2026-09-11-slice-5-record.md
index d9635f0..822075e 100644
--- a/docs/superpowers/reviews/2026-09-11-slice-5-record.md
+++ b/docs/superpowers/reviews/2026-09-11-slice-5-record.md
@@ -122,11 +122,16 @@ followed:
**Codex round 1** (independent, static, scope `1c66309..bed664b`): verdict **request changes** — two P2 defects and three P3 advisories, S5-1 through S5-5. All five applied in one commit (`.superpowers/sdd/2026-09-11-meet-slice-5-drive/codex-r1-fixes-report.md`):
- **S5-1 — P2, malformed and oversized bodies could execute writes. Fixed.** `apiDecode` required `dec.More()` to be false, which is not EOF: `{"title":"x"}]`, `{"title":"x"}}` and a valid object followed by 17 KB of whitespace all created meetings, the last of them swallowing the cap's own `MaxBytesError`. It now decodes one value and requires EOF after it, with a `*http.MaxBytesError` anywhere in the chain answering 413 and everything else 400. The 16 KB cap moved to a group middleware (`capAPIBody`), so the bodyless writes `links` and `end` are capped too; both now read their body to its end and refuse any content (400, or 413 over the cap) before the write. Spec §15.17 carries both rules. Tests: six new `TestAPIBodyRules` cases, each asserting the list afterwards is still empty, and a new `TestAPIBodylessWritesRefuseABody`.
-- **S5-2 — P2, concurrent endings bypassed the 409. Fixed.** The ended check moved inside both write transactions: `store.endMeeting` returns `(bool, error)` from its conditional update's `RowsAffected` and the API maps false to 409; `store.replaceLink` re-reads `ended_at` inside its transaction and answers the `errMeetingEnded` sentinel, which the API maps to the same 409. The HTML handlers keep their existing answers (end redirects either way, replace answers the not-found page). Tests: `TestASecondEndChangesNothingAndAReplaceAfterEndIsRefused` (store-level, sequential and deterministic) and `TestAPIConcurrentEndsAndReplacesSeeTheEndedRow` (two requests through the real router released together: one 200 and one 409, and no live link on an ended meeting).
-- **S5-3 — P3, the log test did not prove success silence. Fixed.** `TestAPILogsCarryNoToken` now marks the log immediately before each of five successful operations and asserts zero new `meet: api` lines from each; the operator's address joins the secret needles. The slow/failing-query half was **not** taken: exercising a driver failure needs a fault-injection seam the store does not have, and the claim this test makes is about the API's own lines.
+- **S5-2 — P2, concurrent endings bypassed the 409. Fixed.** The ended check moved inside both write transactions: `store.endMeeting` returns `(bool, error)` from its conditional update's `RowsAffected` and the API maps false to 409; `store.replaceLink` re-reads `ended_at` inside its transaction and answers the `errMeetingEnded` sentinel, which the API maps to the same 409. The HTML handlers keep their existing answers (end redirects either way, replace answers the not-found page). Tests: `TestASecondEndChangesNothingAndAReplaceAfterEndIsRefused` (store-level, sequential and deterministic) and `TestAPIConcurrentEndsAndReplacesSeeTheEndedRow` (two requests through the real router released together: one 200 and one 409, and no live link on an ended meeting) — the second of which round 2 corrected to a smoke test and replaced as the gate, see S5-6 below.
+- **S5-3 — P3, the log test did not prove success silence. Fixed.** `TestAPILogsCarryNoToken` now marks the log immediately before each of five successful operations and asserts zero new `meet: api` lines from each; the operator's address joins the secret needles. The slow/failing-query half was **not** taken, and round 2 corrected the reason: the harness does expose `x.db.G` and GORM supports query callbacks, so fault and delay injection need no production change (the seam S5-6's test now uses). The decline stands on the narrower ground — the claim this test makes is about the API surface's own log lines — and database-log secrecy stays untested here, with rastrillo's `ParameterizedQueries: true` as the standing static evidence.
- **S5-4 — P3, the badge's text assertion was vacuous. Fixed.** Both rows carry neutral titles ("Quarterly review", "Standup"), and the assertion reads the `rst-badge` span's own inner text rather than searching the row. A replacement and an end over the API are now checked against the rows they write: `meeting_links.created_by` and `meetings.ended_by` both `agent:<id>`, with the member's own meeting's `person:` actor as the control.
- **S5-5 — P3, operator-owned queries bypassed the scope helper. Fixed.** The cap count, the duplicate-name check, `agentsFor`, `revokeAgent` and `agentByID` all run through one `agentsOwnedBy` helper over `scope.OwnedBy(g, "operator_person_id", …)`, transactions included. `agentByToken`, `agentAnyByID` and `creatorLabel` stay unscoped, each with a comment saying why: the first has no operator to scope to (the bearer is what names one), and the other two are label lookups for rows the reader already sees, deciding nothing.
+**Codex round 2** (independent, static, scope `c523a12`): verdict **accept with corrections** — S5-1 through S5-5 confirmed fixed, two P3 advisories about what the fixes *claimed*, S5-6 and S5-7. Both applied in one commit (`.superpowers/sdd/2026-09-11-meet-slice-5-drive/codex-r1-fixes-report.md`, round-2 section):
+
+- **S5-6 — P3, the concurrent test did not deterministically expose the race. Fixed.** Closing one channel makes two goroutines runnable; it does not make either read the running meeting before the other writes, so the schedule where the first request finishes before the second reads satisfies every assertion under the old code too. Twenty green runs proved nothing about that. `TestAPIConcurrentEndsAndReplacesSeeTheEndedRow` stays as a smoke test with its claims corrected, and the gate is now `TestAPIAStaleReadCannotWriteOverAnEnd`, which forces the interleaving with a **test-only GORM query callback** registered on the harness's own `x.db.G` (`installMeetingReadStall`, removed again by `t.Cleanup`): it fires after `gorm:query`, only for a request carrying a stall in its context and only for the first `meetings` read — the handler's own `meetingBySqid` — and holds that request there while a second request ends the meeting and commits. No production code gained a hook. Proved by mutation on a throwaway copy under `$TMPDIR`: with the pre-transaction checks restored and the in-transaction ones removed, the held end answers 200 (a second end) and the held replacement mints a link on an ended meeting, failing 5 runs out of 5, both subtests. Spec §15.17 names the gate and demotes the smoke test.
+- **S5-7 — P3, the universal 413 wording exceeded the contract. Fixed (documentation only).** `MaxBytesReader` limits reading, not receiving, so 413 is reachable only where a handler consumes a body. §15.17 now says the cap is hit "when reading reaches it", lists the precedence — 401 (no credential) → 404 (unresolved token or reach) → 429 (budget) → the body rules — and records the two consequences the old wording got wrong: an oversized body that is malformed from its first bytes may answer 400 before the reader reaches the cap, and a GET carrying a body is answered 200 with the body discarded unread. **GET-body enforcement was deliberately not added**: there is no operation where a GET body could reach a write, and capping bytes nobody reads buys nothing. New positive tests (`TestAPICreateAcceptsTrailingWhitespace`) pin the other half: a create ending in a newline, or padded with whitespace, is 201 — the EOF rule is about content, not about the object being the last byte, and it is deliberately not the bodyless rule, where a single space is 400.
+
---
## 3. Carried items, and where they go
diff --git a/docs/superpowers/specs/2026-09-07-meet-design.md b/docs/superpowers/specs/2026-09-07-meet-design.md
index 940cb04..f2f638d 100644
--- a/docs/superpowers/specs/2026-09-07-meet-design.md
+++ b/docs/superpowers/specs/2026-09-07-meet-design.md
@@ -1896,20 +1896,24 @@ input.
| `end` or `links` on an ended meeting | 409 | `{"error":"ended"}` |
| malformed JSON, unknown field, anything at all after the first object, empty body, title empty or over 200 characters, `starts_at` not RFC 3339 | 400 | `{"error":"bad-request"}` |
| any body at all on `links` or `end` | 400 | `{"error":"bad-request"}` |
-| body over 16 KB, on any operation | 413 | `{"error":"too-large"}` |
+| a body whose reading reaches the 16 KB cap, on any operation that reads one (`POST /meetings`, `links`, `end`) | 413 | `{"error":"too-large"}` |
| a driver failure | 500 | `{"error":"server"}` |
-**The body rules, in full (review S5-1).** Every request on this
-surface goes through `http.MaxBytesReader` at 16 KB in the group's own
-middleware (`capAPIBody`), before a handler runs — the operations that
-read a body and the ones that do not alike. A cap applied inside the
-decoder only would leave `links` and `end` reading nothing and
-refusing nothing, so a megabyte posted at `/end` performed the write.
+**The body rules, in full (review S5-1, corrected by S5-7).** Every
+request on this surface is wrapped in `http.MaxBytesReader` at 16 KB by
+the group's own middleware (`capAPIBody`), before a handler runs — the
+operations that read a body and the ones that do not alike. A cap
+applied inside the decoder only would leave `links` and `end` reading
+nothing and refusing nothing, so a megabyte posted at `/end` performed
+the write.
-`POST /meetings` decodes exactly one JSON object and then **requires
+`POST /meetings` decodes exactly one JSON value and then **requires
EOF**: a second value, a stray `]` or `}`, a trailing scalar, anything
-but whitespace-then-end is 400. `encoding/json`'s `Decoder.More()` is
-not that question — it answers "is another array or object element
+but whitespace-then-end is 400. Trailing whitespace *is* accepted — a
+newline on the end of a body is what a file, and `curl --data-binary
+@file`, sends, and whitespace after a value is part of JSON's own
+grammar, not content. `encoding/json`'s `Decoder.More()` is not the
+question to ask — it answers "is another array or object element
waiting", which is false for a trailing `]` and false for a trailing
`}`, and it swallows the decoder's stored read error, so a valid
object followed by 17 KB of whitespace came back as a clean decode
@@ -1919,10 +1923,38 @@ what separates 413 from 400: a `*http.MaxBytesError` anywhere in the
chain is 413, everything else is 400.
`links` and `end` take **no body**. Theirs is read to its end under
-the same cap and refused if it carried anything: over the cap is 413,
-any other content — a JSON object, a single space — is 400. An absent
-body and a `Content-Length: 0` body are the two shapes that pass. Both
-answer before the write, never after it.
+the same cap and refused if it carried anything: reaching the cap is
+413, any other content — a JSON object, a single space — is 400. An
+absent body and a `Content-Length: 0` body are the two shapes that
+pass. Both answer before the write, never after it.
+
+**413 is lazy, and the order of refusals says when it can happen
+(review S5-7).** `MaxBytesReader` limits *reading*, not receiving: the
+413 happens when a handler's consumption of the body reaches the cap,
+so it is reachable only on the three operations that read a body at
+all. Concretely, and deliberately:
+
+1. **401** — no credential. Judged on the `Authorization` header alone,
+ before a byte of body is read, so an oversized request with no
+ bearer is 401 rather than 413.
+2. **404** — a token that does not resolve, or an operator without
+ member standing. Also before the body.
+3. **429** — the agent's own budget, checked in `requireAgent` after
+ the token resolves and still before the body.
+4. **The body rules** — 413 or 400, in the handler, in whatever order
+ the bytes arrive: an oversized body that is *also* malformed from
+ its first bytes can answer 400 before the reader ever reaches the
+ cap, because the decoder fails on what it has already read. Both are
+ refusals with no write behind them, so which one an agent gets is
+ not a promise this surface makes.
+
+`GET /api/v1/meetings` and `GET /api/v1/meetings/{sqid}` read no body,
+so a GET carrying one is answered 200 on its merits and the body is
+discarded unread with the request. That is Go's own behaviour for a
+handler that ignores `r.Body`, and this surface adds nothing to it: a
+body on a GET has no meaning here, enforcing a size on bytes nobody
+reads would buy nothing, and there is no operation where a GET body
+could reach a write.
**The 409 is decided inside the write, not before it (review S5-2).**
Both write handlers used to read the meeting, check `ended_at`, and
@@ -1943,6 +1975,16 @@ redirects to the list either way (ending an already-ended meeting is
what the member asked for), and its replace answers the not-found
page.
+The gate for this is `TestAPIAStaleReadCannotWriteOverAnEnd`, which
+*forces* the interleaving rather than hoping for it (review S5-6): a
+test-only GORM query callback on the harness's own `*gorm.DB` holds one
+request at the point its handler has read the running meeting, a second
+request ends the meeting and commits, and only then is the first let go
+into its write. Firing two requests at once
+(`TestAPIConcurrentEndsAndReplacesSeeTheEndedRow`) is a smoke test
+beside it, not a proof: nothing in it stops the first request
+finishing before the second reads.
+
**The 401/404 rule, and why it is judged on the header alone.** 401
means "you presented no credential" — no header, a non-bearer scheme,
an empty token — and is true of every caller alike, telling an attacker
diff --git a/internal/meettest/api_test.go b/internal/meettest/api_test.go
index 2e2ca1a..61dd9f3 100644
--- a/internal/meettest/api_test.go
+++ b/internal/meettest/api_test.go
@@ -2,6 +2,7 @@ package meettest
import (
"bytes"
+ "context"
"crypto/sha256"
"encoding/base64"
"encoding/hex"
@@ -18,6 +19,7 @@ import (
"time"
meet "amadan.net/oficina/meet/internal/meet"
+ "gorm.io/gorm"
)
// mintAgent drives Settings as the signed-in member and hands back the
@@ -496,6 +498,36 @@ func TestAPIBodyRules(t *testing.T) {
}
}
+// Review S5-7: requiring EOF after the object is not requiring the
+// object to be the last byte. Whitespace is not content — a trailing
+// newline is what most tools put on the end of a file, and curl
+// --data-binary @file sends it — so a create that ends in one is a 201,
+// and so is one padded with spaces and tabs. This is deliberately
+// *not* the bodyless rule: `links` and `end` take no body at all, and a
+// single space there is a 400 (TestAPIBodylessWritesRefuseABody). The
+// asymmetry is that a create's body is parsed as JSON, where trailing
+// whitespace is part of the grammar, and the other two are not parsed
+// at all.
+func TestAPICreateAcceptsTrailingWhitespace(t *testing.T) {
+ x := newHarness(t, "paul@example.com")
+ c := x.browser(t)
+ c.signIn("paul@example.com")
+ token := mintAgent(t, x, c, "claude")
+
+ for _, tc := range []struct{ name, body string }{
+ {"a trailing newline", `{"title":"Standup"}` + "\n"},
+ {"trailing whitespace", `{"title":"Standup"}` + " \t\r\n "},
+ {"whitespace on both sides", "\n " + `{"title":"Standup"}` + "\n"},
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ created := apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings", token, tc.body), http.StatusCreated)
+ if got := apiString(t, apiMeeting(t, created), "title"); got != "Standup" {
+ t.Fatalf("title %q, want Standup", got)
+ }
+ })
+ }
+}
+
// Review S5-1: `/links` and `/end` take no body, and until this fix
// they read none either — so the 16 KB cap did not apply to them and a
// megabyte posted at `/end` performed the write. Now every request on
@@ -564,14 +596,17 @@ func TestAPIBodylessWritesRefuseABody(t *testing.T) {
// ended?" from a read taken before the store's transaction, so two
// requests could each read a running meeting and each go on to write.
// The decision now lives inside the write — a conditional UPDATE for
-// end, a re-read of ended_at inside the transaction for replace — and
-// this is the race in the shape that found it: two requests fired at
-// once through the real router.
+// end, a re-read of ended_at inside the transaction for replace.
//
-// The invariants hold whichever order the two land in, which is what
-// makes the assertions deterministic even though the interleaving is
-// not: exactly one end gets the 200, and after an end and a
-// replacement race there is never a live link on an ended meeting.
+// This is a smoke test and not a proof (review S5-6). Closing one
+// channel makes both goroutines runnable; it does not make either read
+// the running meeting before the other writes, and the schedule where
+// the first request finishes before the second reads at all satisfies
+// every assertion below under the old code too. What it does check is
+// that two requests arriving together answer sanely and leave the row
+// consistent — worth having, and worth being honest about. The
+// interleaving that actually exposes the defect is forced, one read at
+// a time, in TestAPIAStaleReadCannotWriteOverAnEnd.
func TestAPIConcurrentEndsAndReplacesSeeTheEndedRow(t *testing.T) {
x := newHarness(t, "paul@example.com")
c := x.browser(t)
@@ -600,8 +635,7 @@ func TestAPIConcurrentEndsAndReplacesSeeTheEndedRow(t *testing.T) {
// An end and a replacement at once: the replacement either wins
// outright or is refused as ended, and either way the meeting ends
- // with no live link. A replacement that landed after the end would
- // leave link_present true on an ended meeting — the defect.
+ // with no live link.
second := apiString(t, apiMeeting(t, apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings", token,
`{"title":"Retro"}`), http.StatusCreated)), "sqid")
both := race(t, x, token,
@@ -622,6 +656,142 @@ func TestAPIConcurrentEndsAndReplacesSeeTheEndedRow(t *testing.T) {
}
}
+// Review S5-6: the interleaving that broke both writes, forced rather
+// than hoped for. One request is held at the exact point the old code
+// went wrong — just after its handler has read the meeting and found it
+// running — while a second request ends the meeting and commits. Only
+// then is the held request let go, so it reaches its write with a
+// snapshot that is now stale, which is the state the old handlers
+// trusted.
+//
+// Under the shipped code both refusals come from inside the write:
+// end's conditional UPDATE changes no row, replace's re-read of
+// ended_at inside its transaction answers errMeetingEnded, and both
+// are 409. Under the old code the held end wrote a second time and
+// answered 200, and the held replacement minted a link on a meeting
+// that had already ended — both of which this test fails on, every
+// run. Proved by mutation: see the round-2 report.
+func TestAPIAStaleReadCannotWriteOverAnEnd(t *testing.T) {
+ x := newHarness(t, "paul@example.com")
+ c := x.browser(t)
+ c.signIn("paul@example.com")
+ token := mintAgent(t, x, c, "claude")
+ installMeetingReadStall(t, x)
+
+ for _, op := range []string{"end", "links"} {
+ t.Run("a stalled "+op, func(t *testing.T) {
+ sqid := apiString(t, apiMeeting(t, apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings", token,
+ `{"title":"Standup"}`), http.StatusCreated)), "sqid")
+
+ // The held request reads the running meeting, then waits.
+ held := newStall()
+ done := goStalledAPI(t, x, held, http.MethodPost, "/api/v1/meetings/"+sqid+"/"+op, token)
+ held.waitReached(t)
+
+ // With that read held, the meeting ends and commits.
+ apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings/"+sqid+"/end", token, ""), http.StatusOK)
+
+ // And now the held request runs on into its write.
+ held.release()
+ rec := <-done
+ body := apiBody(t, rec, http.StatusConflict)
+ if body["error"] != "ended" {
+ t.Fatalf("the stalled %s: %v, want error ended", op, body)
+ }
+
+ m := apiMeeting(t, apiBody(t, api(t, x, http.MethodGet, "/api/v1/meetings/"+sqid, token, ""), http.StatusOK))
+ if m["ended_at"] == nil {
+ t.Fatalf("the meeting is not ended: %v", m)
+ }
+ if m["link_present"] != false {
+ t.Fatalf("an ended meeting has a live link: %v — the stalled %s wrote over the end", m, op)
+ }
+ })
+ }
+}
+
+// stallKey marks a request whose first `meetings` read should block.
+// The value is the request's own stall, so the callback stalls only the
+// request the test built, and only once.
+type stallKey struct{}
+
+type stall struct {
+ reached chan struct{}
+ releasing chan struct{}
+ once sync.Once
+}
+
+func newStall() *stall {
+ return &stall{reached: make(chan struct{}), releasing: make(chan struct{})}
+}
+
+// waitReached blocks until the request has made its meeting read and is
+// being held. A request that never makes one would hang the test, so
+// this gives up rather than waiting forever.
+func (s *stall) waitReached(t *testing.T) {
+ t.Helper()
+ select {
+ case <-s.reached:
+ case <-time.After(5 * time.Second):
+ t.Fatal("the request never reached its meeting read")
+ }
+}
+
+func (s *stall) release() { close(s.releasing) }
+
+// installMeetingReadStall registers a test-only GORM callback on the
+// harness's own *gorm.DB — the seam the round-1 report wrongly said did
+// not exist (review S5-3's note, review S5-6's correction) — and takes
+// it off again when the test ends.
+//
+// It runs after gorm:query, so the rows are already scanned and closed
+// and the held request is holding no connection: the request that ends
+// the meeting behind it can use the same pool. It fires only for a
+// request carrying a stall in its context and only for the `meetings`
+// table, which on these paths is the handler's own meetingBySqid — the
+// read the old code went on to trust. sync.Once keeps it to that first
+// read, so the meeting read inside replaceLink's transaction, later in
+// the same request, runs unimpeded.
+func installMeetingReadStall(t *testing.T, x *harness) {
+ t.Helper()
+ const name = "meettest:stall_meeting_read"
+ err := x.db.G.Callback().Query().After("gorm:query").Register(name, func(tx *gorm.DB) {
+ if tx.Statement.Context == nil || tx.Statement.Table != "meetings" {
+ return
+ }
+ s, ok := tx.Statement.Context.Value(stallKey{}).(*stall)
+ if !ok {
+ return
+ }
+ s.once.Do(func() {
+ close(s.reached)
+ <-s.releasing
+ })
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ t.Cleanup(func() { _ = x.db.G.Callback().Query().Remove(name) })
+}
+
+// goStalledAPI runs one bodyless API request on its own goroutine with
+// the stall in its context, and hands back a channel carrying the
+// recorder once the request has finished. Nothing is asserted on that
+// goroutine — t.Fatalf from one is not allowed.
+func goStalledAPI(t *testing.T, x *harness, s *stall, method, target, token string) <-chan *httptest.ResponseRecorder {
+ t.Helper()
+ done := make(chan *httptest.ResponseRecorder, 1)
+ req := httptest.NewRequest(method, target, nil)
+ req.Header.Set("Authorization", "Bearer "+token)
+ req = req.WithContext(context.WithValue(req.Context(), stallKey{}, s))
+ go func() {
+ rec := httptest.NewRecorder()
+ x.h.ServeHTTP(rec, req)
+ done <- rec
+ }()
+ return done
+}
+
// race fires two API requests from two goroutines released together,
// and hands back both recorders. Nothing is asserted in the goroutines
// — t.Fatalf from one is not allowed — so every check happens on the