oficina / meet Public

test: Codex round 3 fixes S5-8, S5-9

S5-8: a t.Fatal between starting the held request and releasing it left
that request parked inside the GORM callback for the rest of the
package run, and the wait for its response had no timeout either. The
held request is now a heldRequest whose cleanup is registered before
its goroutine starts and which releases and joins it exactly once;
release and join are both sync.Once, and waitReached, the join and the
response are all bounded at five seconds so a regression fails by name
here rather than by the package timeout. Checked by injecting a
t.Fatal after waitReached in a throwaway copy: it fails in 0.14s
instead of hanging. Ran 10x under -race.

S5-9: the 400 row said "anything at all after the first object", which
round 2's own whitespace acceptance had made false; it now reads
"anything other than JSON whitespace after the first value". The 413
row reads "reading the body encounters bytes beyond 16,384", and the
bodyless paragraph is corrected the same way, because the boundary is
inclusive: a create padded with whitespace to exactly 16,384 bytes is
201, a bodyless write carrying those same bytes is 400, and one byte
more is 413 on both. Nothing asserted the boundary before — every
other body test uses 17 KB — so TestAPIBodyCapBoundary pins all four.

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 1 month ago 4f896c25e2d986c1927d666e11a2a35212461d39 parent 11fb573

3 files changed, +168 −41

docs/superpowers/reviews/2026-09-11-slice-5-record.md +6 −1
docs/superpowers/specs/2026-09-07-meet-design.md +22 −10
internal/meettest/api_test.go +140 −30

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 822075e..11371e2 100644

--- a/docs/superpowers/reviews/2026-09-11-slice-5-record.md

+++ b/docs/superpowers/reviews/2026-09-11-slice-5-record.md

@@ -130,7 +130,12 @@ followed:

**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.

+- **S5-7 — P3, the universal 413 wording exceeded the contract. Fixed (documentation only), and refined again in round 3.** `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 runs past it (round 3 sharpened that wording again, S5-9), 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.

+**Codex round 3** (independent, static, scope `11fb573`): verdict **accept with corrections** — S5-6 and S5-7 confirmed closed, two P3 advisories, S5-8 and S5-9. Both applied in one commit (report: the round-3 section of `.superpowers/sdd/2026-09-11-meet-slice-5-drive/codex-r1-fixes-report.md`):

+- **S5-8 — P3, a failed assertion could leave the stalled request blocked. Fixed.** In `TestAPIAStaleReadCannotWriteOverAnEnd`, a `t.Fatal` between starting the held request and releasing it — a `waitReached` timeout, or the competing end failing its status assertion — skipped the release and parked that request on the stall channel for the rest of the package run; removing the callback does not unblock one already inside it, and the `<-done` wait had no timeout either. The held request is now a `heldRequest` whose `t.Cleanup` is registered **before** its goroutine starts and which releases and joins it exactly once (`sync.Once` on both the release and the join), with every wait — `waitReached`, the join, the response — bounded at five seconds and failing by name here rather than by the package timeout. Verified by injecting a `t.Fatal` immediately after `waitReached` in a throwaway copy: the test fails in 0.14s instead of hanging. Run 10× under `-race`, green.

+- **S5-9 — P3, the body table still contradicted the whitespace rule and got the boundary wrong. Fixed.** The 400 row now reads "anything other than JSON whitespace after the first value" rather than "anything at all after the first object", which round 2's whitespace acceptance had made false. The 413 row now reads "reading the body encounters bytes beyond 16,384", and a new paragraph states the inclusive boundary and the two answers on the inside of it: a create padded with JSON whitespace to exactly 16,384 bytes is 201, a bodyless write carrying the same 16,384 bytes is 400 (it takes no body whatever its size), and one byte more is 413 on both. The bodyless paragraph's "reaching the cap is 413" is corrected the same way. Nothing had ever asserted the boundary — every other body test uses 17 KB, comfortably past the edge — so `TestAPIBodyCapBoundary` now pins all four answers, and the meeting being unchanged afterwards.

---

diff --git a/docs/superpowers/specs/2026-09-07-meet-design.md b/docs/superpowers/specs/2026-09-07-meet-design.md

index f2f638d..b32eb49 100644

--- a/docs/superpowers/specs/2026-09-07-meet-design.md

+++ b/docs/superpowers/specs/2026-09-07-meet-design.md

@@ -1894,9 +1894,9 @@ input.

| unknown `sqid`, a `sqid` not shaped like `newSqid` produces, a path or method this surface does not have | 404 | `{"error":"not-found"}` |

| over 60 requests a minute for one agent | 429 | `{"error":"rate"}` + `Retry-After` (whole seconds, never below 1) |

| `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"}` |

+| malformed JSON, unknown field, anything other than JSON whitespace after the first value, 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"}` |

-| a body whose reading reaches the 16 KB cap, on any operation that reads one (`POST /meetings`, `links`, `end`) | 413 | `{"error":"too-large"}` |

+| reading the body encounters bytes beyond 16,384, 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, corrected by S5-7).** Every

@@ -1923,16 +1923,28 @@ 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: 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.

+the same cap and refused if it carried anything: a body that runs past

+16,384 bytes is 413, and any other content — a JSON object, a single

+space, 16,384 bytes of padding — 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 cap is 16,384 bytes and the boundary is inclusive (review

+S5-9).** `MaxBytesReader` refuses only when a read encounters a byte

+*beyond* the limit, so a body of exactly 16,384 bytes is within it and

+16,385 is not. The two surfaces that read a body then answer

+differently on the inside of that boundary, because the cap is not

+what they disagree about: a create padded with JSON whitespace to

+exactly 16,384 bytes is a 201, while a bodyless write carrying the

+same 16,384 bytes is a 400 — it takes no body at all, whatever its

+size. One byte more is 413 on both. `TestAPIBodyCapBoundary` pins all

+four.

**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:

+413 happens when a handler's consumption of the body runs past 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

@@ -1943,7 +1955,7 @@ all. Concretely, and deliberately:

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

+ its first bytes can answer 400 before the reader ever runs past 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.

diff --git a/internal/meettest/api_test.go b/internal/meettest/api_test.go

index 61dd9f3..02f6e36 100644

--- a/internal/meettest/api_test.go

+++ b/internal/meettest/api_test.go

@@ -498,6 +498,59 @@ func TestAPIBodyRules(t *testing.T) {

}

+// Review S5-9: the cap is 16,384 bytes and the boundary is inclusive —

+// http.MaxBytesReader refuses only when reading encounters a byte

+// *beyond* the limit, so a body of exactly 16,384 is within it and

+// 16,385 is not. Nothing asserted this before: the other body tests all

+// use 17 KB, which is comfortably past the edge and says nothing about

+// where the edge is. Both surfaces that read a body are pinned here,

+// and the two answer differently on the inside of the boundary: a

+// create of exactly the cap is a 201 (its padding is JSON whitespace),

+// a bodyless write of exactly the cap is a 400 (it takes no body at

+// all) — and one byte more is 413 on both.

+func TestAPIBodyCapBoundary(t *testing.T) {

+ x := newHarness(t, "paul@example.com")

+ c := x.browser(t)

+ c.signIn("paul@example.com")

+ token := mintAgent(t, x, c, "claude")

+ const limit = 16 * 1024

+ create := `{"title":"Standup"}`

+ atCap := create + strings.Repeat(" ", limit-len(create))

+ overCap := atCap + " "

+ if len(atCap) != limit || len(overCap) != limit+1 {

+ t.Fatalf("the bodies are %d and %d bytes, want %d and %d", len(atCap), len(overCap), limit, limit+1)

+ }

+ created := apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings", token, atCap), http.StatusCreated)

+ if got := apiString(t, apiMeeting(t, created), "title"); got != "Standup" {

+ t.Fatalf("title %q, want Standup", got)

+ }

+ sqid := apiString(t, apiMeeting(t, created), "sqid")

+ if body := apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings", token, overCap), http.StatusRequestEntityTooLarge); body["error"] != "too-large" {

+ t.Fatalf("one byte over the cap: %v, want too-large", body)

+ }

+ // The bodyless writes measure the same cap, and refuse a body of

+ // any size — so at the cap they answer 400 for its content and over

+ // it 413 for its size.

+ for _, op := range []string{"links", "end"} {

+ if body := apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings/"+sqid+"/"+op, token, atCap), http.StatusBadRequest); body["error"] != "bad-request" {

+ t.Fatalf("%s at exactly the cap: %v, want bad-request", op, body)

+ }

+ if body := apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings/"+sqid+"/"+op, token, overCap), http.StatusRequestEntityTooLarge); body["error"] != "too-large" {

+ t.Fatalf("%s one byte over the cap: %v, want too-large", op, body)

+ }

+ // And the refusals wrote nothing: the meeting made by the create at

+ // the cap is still running, with its first link.

+ m := apiMeeting(t, apiBody(t, api(t, x, http.MethodGet, "/api/v1/meetings/"+sqid, token, ""), http.StatusOK))

+ if m["ended_at"] != nil || m["link_present"] != true {

+ t.Fatalf("a refused body changed the meeting: %v", m)

+ }

// 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

@@ -684,17 +737,17 @@ func TestAPIAStaleReadCannotWriteOverAnEnd(t *testing.T) {

`{"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)

+ // goStalledAPI registers the cleanup that lets it go before

+ // it ever starts, so a failure in any assertion below leaves

+ // nothing blocked (review S5-8).

+ held := goStalledAPI(t, x, 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)

+ body := apiBody(t, held.response(t), http.StatusConflict)

if body["error"] != "ended" {

t.Fatalf("the stalled %s: %v, want error ended", op, body)

}

@@ -716,28 +769,26 @@ func TestAPIAStaleReadCannotWriteOverAnEnd(t *testing.T) {

type stallKey struct{}

type stall struct {

- reached chan struct{}

- releasing chan struct{}

- once sync.Once

+ reached chan struct{}

+ releasing chan struct{}

+ holding sync.Once

+ releasing1 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")

- }

+// release lets the held request go, and may be called any number of

+// times (review S5-8): the test calls it on its way to the response,

+// and the cleanup calls it again in case the test never got that far.

+func (s *stall) release() { s.releasing1.Do(func() { close(s.releasing) }) }

-func (s *stall) release() { close(s.releasing) }

+// stallWait bounds every wait in this test. Long enough that a loaded

+// machine does not trip it, short enough that a regression is a named

+// failure here rather than the package timeout killing the whole run

+// with a goroutine dump (review S5-8).

+const stallWait = 5 * time.Second

// installMeetingReadStall registers a test-only GORM callback on the

// harness's own *gorm.DB — the seam the round-1 report wrongly said did

@@ -763,7 +814,7 @@ func installMeetingReadStall(t *testing.T, x *harness) {

if !ok {

return

}

- s.once.Do(func() {

+ s.holding.Do(func() {

close(s.reached)

<-s.releasing

})

@@ -774,22 +825,81 @@ func installMeetingReadStall(t *testing.T, x *harness) {

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 {

+// heldRequest is one bodyless API request running on its own goroutine

+// and stalled at its meeting read. Nothing is asserted on that

+// goroutine — t.Fatalf from one is not allowed — so the recorder comes

+// back to the test's own goroutine through done.

+type heldRequest struct {

+ stall *stall

+ done chan *httptest.ResponseRecorder

+ joined sync.Once

+ rec *httptest.ResponseRecorder

+ stuck bool

+ reported bool

+// goStalledAPI starts the request, having first registered the cleanup

+// that releases and joins it (review S5-8). Order matters: a t.Fatal in

+// any assertion between here and the response would otherwise skip the

+// release and leave the request parked on the stall channel for the

+// rest of the package run, since removing the callback does not unblock

+// one already inside it.

+func goStalledAPI(t *testing.T, x *harness, method, target, token string) *heldRequest {

t.Helper()

- done := make(chan *httptest.ResponseRecorder, 1)

+ h := &heldRequest{stall: newStall(), done: make(chan *httptest.ResponseRecorder, 1)}

+ t.Cleanup(func() {

+ h.join()

+ if h.stuck && !h.reported {

+ h.reported = true

+ t.Errorf("the held %s %s never finished", method, target)

+ }

+ })

req := httptest.NewRequest(method, target, nil)

req.Header.Set("Authorization", "Bearer "+token)

- req = req.WithContext(context.WithValue(req.Context(), stallKey{}, s))

+ req = req.WithContext(context.WithValue(req.Context(), stallKey{}, h.stall))

go func() {

rec := httptest.NewRecorder()

x.h.ServeHTTP(rec, req)

- done <- rec

+ h.done <- rec

}()

- return done

+ return h

+// 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 and says so instead.

+func (h *heldRequest) waitReached(t *testing.T) {

+ t.Helper()

+ select {

+ case <-h.stall.reached:

+ case <-time.After(stallWait):

+ t.Fatal("the request never reached its meeting read")

+ }

+// join releases the held request and waits for it, at most once.

+func (h *heldRequest) join() {

+ h.joined.Do(func() {

+ h.stall.release()

+ select {

+ case h.rec = <-h.done:

+ case <-time.After(stallWait):

+ h.stuck = true

+ }

+ })

+// response is join for a caller that wants the answer: the same wait,

+// bounded the same way, failing here rather than in the cleanup.

+func (h *heldRequest) response(t *testing.T) *httptest.ResponseRecorder {

+ t.Helper()

+ h.join()

+ if h.stuck {

+ h.reported = true

+ t.Fatal("the held request never finished")

+ }

+ return h.rec

}

// race fires two API requests from two goroutines released together,