amadan
fix: Codex round 1 review fixes S5-1..S5-5
S5-1: apiDecode requires EOF after the one object rather than asking dec.More(), so a trailing ], } or scalar is a 400 and a valid object followed by whitespace past the cap is the 413 it should always have been. The 16 KB MaxBytesReader moved to a group middleware, so the bodyless writes /links and /end are capped too; both now refuse any body at all (400, or 413 over the cap) before the write. S5-2: the ended check moved inside both write transactions. store.endMeeting reports whether its conditional update changed a row and the API maps false to 409; store.replaceLink re-reads ended_at inside its transaction and answers errMeetingEnded, mapped to the same 409. The HTML end and replace keep the answers they gave. S5-3: TestAPILogsCarryNoToken marks the log immediately before each of five successful operations and asserts no line from any of them; the operator's address joins the secret needles. S5-4: the badge test uses neutral titles and asserts the rst-badge element's own text, and now checks the actor recorded by a replacement (created_by) and an end (ended_by) as well as by a create. S5-5: the five operator-side agent queries run through one agentsOwnedBy helper over scope.OwnedBy; agentByToken, agentAnyByID and creatorLabel stay unscoped, each with the reason in a comment. Spec §15.17 carries the body rules and the in-transaction 409; the slice record §2 carries the five dispositions.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
8 files changed,
+593
−51
docs/superpowers/reviews/2026-09-11-slice-5-record.md+8 −0docs/superpowers/specs/2026-09-07-meet-design.md+47 −2internal/meet/agents.go+39 −10internal/meet/api.go+93 −20internal/meet/handlers_meetings.go+15 −1internal/meet/store.go+39 −3internal/meet/store_test.go+59 −1internal/meettest/api_test.go+293 −14
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 fb7508b..d9635f0 100644 |
| --- a/docs/superpowers/reviews/2026-09-11-slice-5-record.md |
| +++ b/docs/superpowers/reviews/2026-09-11-slice-5-record.md |
| @@ -119,6 +119,14 @@ followed: |
| **Whole-branch review** (Opus, read-only, scope `1c66309..2f92b0a`): verdict **merge with named fixes** W5-1 through W5-5. All five applied in one fix-wave commit (`.superpowers/sdd/2026-09-11-meet-slice-5-drive/fix-wave-report.md`): W5-1 and W5-2 as new/extended tests in `internal/meettest/api_test.go`, W5-3 in `api.go`, W5-4 in spec §15.17, W5-5 as the line added to §3 below. |
| +**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-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. |
| + |
| --- |
| ## 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 ef2b5fc..940cb04 100644 |
| --- a/docs/superpowers/specs/2026-09-07-meet-design.md |
| +++ b/docs/superpowers/specs/2026-09-07-meet-design.md |
| @@ -1894,10 +1894,55 @@ 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, trailing content, empty body, title empty or over 200 characters, `starts_at` not RFC 3339 | 400 | `{"error":"bad-request"}` | |
| -| body over 16 KB | 413 | `{"error":"too-large"}` | |
| +| 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 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. |
| + |
| +`POST /meetings` decodes exactly one JSON object 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 |
| +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 |
| +rather than the 413 the cap promises. A second `Decode` asks the |
| +honest question and hands back the honest error, and that error is |
| +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 409 is decided inside the write, not before it (review S5-2).** |
| +Both write handlers used to read the meeting, check `ended_at`, and |
| +then enter the store's transaction — a snapshot, not a guarantee. Two |
| +ends could each read a running meeting and each answer 200, and a |
| +replacement could read a running meeting, pause while another request |
| +ended it, and leave an unrevoked link on an ended meeting with |
| +`link_present:true`. SQLite's single writer serializes the |
| +transactions; it does nothing about the reads in front of them. Now |
| +`store.endMeeting` reports whether its conditional `UPDATE … WHERE id |
| += ? AND ended_at IS NULL` actually changed a row — false is the 409 — |
| +and `store.replaceLink` re-reads `ended_at` inside its own |
| +transaction, answering `errMeetingEnded`, which the API maps to the |
| +same 409. Exactly one of two racing ends gets the 200, and an ended |
| +meeting never keeps a live link. The HTML side shares both store |
| +paths and keeps the answers it always gave: the confirm page's end |
| +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 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/meet/agents.go b/internal/meet/agents.go| index d1473f7..0bc42d3 100644 |
| --- a/internal/meet/agents.go |
| +++ b/internal/meet/agents.go |
| @@ -11,6 +11,7 @@ import ( |
| "strconv" |
| "strings" |
| + "amadan.net/rastrillo/rastrillo/scope" |
| "gorm.io/gorm" |
| ) |
| @@ -81,8 +82,8 @@ func (s *store) createAgent(ctx context.Context, name string, operator Person) ( |
| var ag Agent |
| err = s.g.WithContext(ctx).Transaction(func(tx *gorm.DB) error { |
| var live int64 |
| - if err := tx.Model(&Agent{}). |
| - Where("operator_person_id = ? AND revoked_at IS NULL", operator.ID). |
| + if err := agentsOwnedBy(tx.Model(&Agent{}), operator). |
| + Where("revoked_at IS NULL"). |
| Count(&live).Error; err != nil { |
| return err |
| } |
| @@ -90,8 +91,8 @@ func (s *store) createAgent(ctx context.Context, name string, operator Person) ( |
| return errAgentLimit |
| } |
| var dup int64 |
| - if err := tx.Model(&Agent{}). |
| - Where("operator_person_id = ? AND name = ? AND revoked_at IS NULL", operator.ID, name). |
| + if err := agentsOwnedBy(tx.Model(&Agent{}), operator). |
| + Where("name = ? AND revoked_at IS NULL", name). |
| Count(&dup).Error; err != nil { |
| return err |
| } |
| @@ -107,17 +108,28 @@ func (s *store) createAgent(ctx context.Context, name string, operator Person) ( |
| return ag, token, nil |
| } |
| +// agentsOwnedBy is the one ownership filter every operator-side agent |
| +// query runs through (review S5-5, AGENTS.md's scoped-query rule): an |
| +// agent is owned by the person who minted it, and its owner column is |
| +// operator_person_id rather than the convention's user_id, so this is |
| +// scope.OwnedBy rather than scope.Owned. Written once so no caller |
| +// hand-writes the predicate — including the ones inside createAgent's |
| +// transaction, which take tx rather than s.g. |
| +func agentsOwnedBy(g *gorm.DB, operator Person) *gorm.DB { |
| + return scope.OwnedBy(g, "operator_person_id", operator.ID) |
| +} |
| + |
| func (s *store) agentsFor(ctx context.Context, operator Person) ([]Agent, error) { |
| var out []Agent |
| - err := s.g.WithContext(ctx).Where("operator_person_id = ?", operator.ID).Order("created_at DESC, id DESC").Find(&out).Error |
| + err := agentsOwnedBy(s.g.WithContext(ctx), operator).Order("created_at DESC, id DESC").Find(&out).Error |
| return out, err |
| } |
| // revokeAgent revokes one of the operator's own agents. False when the |
| // row is not theirs or is already revoked: a 404 shape, never a 403. |
| func (s *store) revokeAgent(ctx context.Context, id int64, operator Person) (bool, error) { |
| - res := s.g.WithContext(ctx).Model(&Agent{}). |
| - Where("id = ? AND operator_person_id = ? AND revoked_at IS NULL", id, operator.ID). |
| + res := agentsOwnedBy(s.g.WithContext(ctx).Model(&Agent{}), operator). |
| + Where("id = ? AND revoked_at IS NULL", id). |
| Update("revoked_at", s.now().UTC()) |
| return res.RowsAffected == 1, res.Error |
| } |
| @@ -128,6 +140,13 @@ func (s *store) revokeAgent(ctx context.Context, id int64, operator Person) (boo |
| // (fix round 1, item 1: fail closed, not a bare-ID Person). Whether |
| // the operator currently holds member standing is a separate question |
| // the caller re-derives (spec §8.1, §13). |
| +// |
| +// Not an ownership query, and so not agentsOwnedBy's (review S5-5): |
| +// there is no operator on this path to scope to. The bearer is what |
| +// establishes who the caller is, and the row it resolves to is what |
| +// names the operator — scoping the lookup by an operator would mean |
| +// already knowing the answer it exists to find. The token hash is the |
| +// filter, and it is a secret the caller had to present. |
| func (s *store) agentByToken(ctx context.Context, token string) (Agent, Person, bool, error) { |
| var ag Agent |
| err := s.g.WithContext(ctx).Where("token_hash = ? AND revoked_at IS NULL", hashAgentToken(token)).Take(&ag).Error |
| @@ -136,10 +155,12 @@ func (s *store) agentByToken(ctx context.Context, token string) (Agent, Person, |
| // agentByID resolves an agent scoped to its operator (fix round 1, |
| // item 2): a foreign or unknown id both answer false, the same 404 |
| -// shape scope.Owned gives every other owned row. |
| +// shape scope.Owned gives every other owned row — through |
| +// agentsOwnedBy, so the predicate is the framework's and not this |
| +// file's (review S5-5). |
| func (s *store) agentByID(ctx context.Context, id int64, operator Person) (Agent, Person, bool, error) { |
| var ag Agent |
| - err := s.g.WithContext(ctx).Where("id = ? AND operator_person_id = ?", id, operator.ID).Take(&ag).Error |
| + err := agentsOwnedBy(s.g.WithContext(ctx), operator).Where("id = ?", id).Take(&ag).Error |
| return s.withOperator(ctx, ag, err) |
| } |
| @@ -150,6 +171,11 @@ func (s *store) agentByID(ctx context.Context, id int64, operator Person) (Agent |
| // one — agentByID is that — but every member already sees every |
| // meeting on the list, so naming which agent created one tells them |
| // nothing the list did not. |
| +// |
| +// Deliberately outside agentsOwnedBy (review S5-5): the reader of a |
| +// list is not the operator of the agents on it, and scoping this |
| +// lookup to the reader would blank the principal on every meeting made |
| +// by somebody else's agent. Nothing is decided on what it returns. |
| func (s *store) agentAnyByID(ctx context.Context, id int64) (Agent, Person, bool, error) { |
| var ag Agent |
| err := s.g.WithContext(ctx).Where("id = ?", id).Take(&ag).Error |
| @@ -180,7 +206,10 @@ func (s *store) withOperator(ctx context.Context, ag Agent, err error) (Agent, P |
| // creatorLabel renders a stored actor (§7.2) as the identity a reader |
| // is shown: an agent's principal, <name>~<operator email>, or a |
| // person's display name. It is a label and not an authority — nothing |
| -// is decided on it — so an actor that no longer resolves comes back as |
| +// is decided on it, which is also why neither lookup it makes is a |
| +// scoped one (review S5-5): the row was already shown to this reader |
| +// by the list, and all this adds is the name of who made it — so an |
| +// actor that no longer resolves comes back as |
| // the raw actor string rather than failing the whole list, logged the |
| // way store.title logs an unreadable title. |
| func (s *store) creatorLabel(ctx context.Context, actor string) string { |
diff --git a/internal/meet/api.go b/internal/meet/api.go| index 59a2b17..753dd40 100644 |
| --- a/internal/meet/api.go |
| +++ b/internal/meet/api.go |
| @@ -4,6 +4,7 @@ import ( |
| "context" |
| "encoding/json" |
| "errors" |
| + "io" |
| "net/http" |
| "strconv" |
| "strings" |
| @@ -78,6 +79,7 @@ func apiNotFound() map[string]any { return map[string]any{"error": "not-found"} |
| // never run. |
| func (a *app) apiRoutes(r chi.Router) { |
| r.Route("/api/v1", func(r chi.Router) { |
| + r.Use(capAPIBody) |
| r.Use(a.requireAgent) |
| // A path or method this surface does not have is the same |
| // not-found body everything else answers — and it is answered |
| @@ -97,6 +99,23 @@ func (a *app) apiRoutes(r chi.Router) { |
| }) |
| } |
| +// capAPIBody puts every request on this surface under the 16 KB cap |
| +// 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 |
| +// the bodyless writes (`/links`, `/end`) reading nothing and refusing |
| +// nothing, so a megabyte posted at `/end` would perform the write |
| +// (review S5-1). MaxBytesReader takes the ResponseWriter so an |
| +// over-cap body also stops the connection being reused for a body the |
| +// server never read. |
| +func capAPIBody(next http.Handler) http.Handler { |
| + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + if r.Body != nil { |
| + r.Body = http.MaxBytesReader(w, r.Body, maxAPIBody) |
| + } |
| + next.ServeHTTP(w, r) |
| + }) |
| +} |
| + |
| // requireAgent is spec §2.5's `/api/v1/*` row: the bearer names an |
| // unrevoked agent AND its operator holds member standing right now |
| // (§8.1). Both halves are re-derived per request, so an operator |
| @@ -307,12 +326,19 @@ func (a *app) apiReplaceLink(w http.ResponseWriter, r *http.Request) { |
| if !ok { |
| return |
| } |
| - if m.EndedAt != nil { |
| - a.refuseAPI(w, r, http.StatusConflict, map[string]any{"error": "ended"}, "ended") |
| + if !a.apiNoBody(w, r) { |
| return |
| } |
| + // No ended check on the snapshot read above. A meeting can end |
| + // between that read and this write (review S5-2); the authority is |
| + // store.replaceLink's own re-read of ended_at inside its own |
| + // transaction, which answers errMeetingEnded. |
| ap, _ := currentAgent(r) |
| token, err := a.store.replaceLink(r.Context(), m.ID, actorForAgent(ap.agent)) |
| + if errors.Is(err, errMeetingEnded) { |
| + a.refuseAPI(w, r, http.StatusConflict, map[string]any{"error": "ended"}, "ended") |
| + return |
| + } |
| if err != nil { |
| a.apiServerError(w, r, "replace link", err) |
| return |
| @@ -333,21 +359,29 @@ func (a *app) apiEndMeeting(w http.ResponseWriter, r *http.Request) { |
| if !ok { |
| return |
| } |
| - if m.EndedAt != nil { |
| - a.refuseAPI(w, r, http.StatusConflict, map[string]any{"error": "ended"}, "ended") |
| + if !a.apiNoBody(w, r) { |
| return |
| } |
| + // As with replace: the ended check is the conditional UPDATE inside |
| + // store.endMeeting, not the snapshot read above (review S5-2). The |
| + // end that changed no row is the 409, so exactly one of two racing |
| + // ends gets the 200. |
| ap, _ := currentAgent(r) |
| - if err := a.store.endMeeting(r.Context(), m.ID, actorForAgent(ap.agent)); err != nil { |
| + ended, err := a.store.endMeeting(r.Context(), m.ID, actorForAgent(ap.agent)) |
| + if err != nil { |
| a.apiServerError(w, r, "end meeting", err) |
| return |
| } |
| + if !ended { |
| + a.refuseAPI(w, r, http.StatusConflict, map[string]any{"error": "ended"}, "ended") |
| + return |
| + } |
| // The row is the authority; the hub is told so every open stream |
| // gets lost{ended} now rather than at the next tick (spec §2.4), |
| // exactly as the screen's own end does. |
| a.hub.End(m.Sqid) |
| - ended, ok, err := a.store.meetingBySqid(r.Context(), m.Sqid) |
| + row, ok, err := a.store.meetingBySqid(r.Context(), m.Sqid) |
| if err != nil { |
| a.apiServerError(w, r, "read the ended meeting", err) |
| return |
| @@ -356,7 +390,7 @@ func (a *app) apiEndMeeting(w http.ResponseWriter, r *http.Request) { |
| a.refuseAPI(w, r, http.StatusNotFound, apiNotFound(), "meeting") |
| return |
| } |
| - view, err := a.store.meetingViewOf(r.Context(), ended) |
| + view, err := a.store.meetingViewOf(r.Context(), row) |
| if err != nil { |
| a.apiServerError(w, r, "read the ended meeting", err) |
| return |
| @@ -387,25 +421,64 @@ func (a *app) apiMeetingFor(w http.ResponseWriter, r *http.Request) (Meeting, bo |
| return m, true |
| } |
| -// apiDecode reads the body as dst: capped at 16 KB before anything is |
| -// parsed, unknown fields refused, and nothing accepted after the one |
| -// object — so a second document smuggled behind the first is a 400 |
| -// rather than a silently ignored tail. |
| +// apiDecode reads the body as dst: under capAPIBody's 16 KB cap, |
| +// unknown fields refused, and EOF required after the one object — so a |
| +// second document, a stray `]` or `}`, or any other tail behind the |
| +// first value is a 400 rather than a silently ignored remainder. |
| +// |
| +// EOF is required rather than dec.More() asked (review S5-1). More() |
| +// answers "is another array or object element waiting", which is not |
| +// the same question: it is false for a trailing `]`, false for a |
| +// trailing `}`, and — worse — it swallows the decoder's stored read |
| +// error, so a valid object followed by 17 KB of whitespace came back as |
| +// a clean decode rather than the 413 the cap promises. A second Decode |
| +// asks the honest question and hands back the honest error. |
| func (a *app) apiDecode(w http.ResponseWriter, r *http.Request, dst any) bool { |
| - r.Body = http.MaxBytesReader(w, r.Body, maxAPIBody) |
| dec := json.NewDecoder(r.Body) |
| dec.DisallowUnknownFields() |
| if err := dec.Decode(dst); err != nil { |
| - var tooBig *http.MaxBytesError |
| - if errors.As(err, &tooBig) { |
| - a.refuseAPI(w, r, http.StatusRequestEntityTooLarge, map[string]any{"error": "too-large"}, "size") |
| - return false |
| - } |
| - a.refuseAPI(w, r, http.StatusBadRequest, map[string]any{"error": "bad-request"}, "shape") |
| + return a.refuseBody(w, r, err) |
| + } |
| + // Anything at all after the first value — a second value (err nil), |
| + // a syntax error, or bytes past the cap — is a refusal. Only io.EOF |
| + // means "that was the whole body". |
| + var tail json.RawMessage |
| + if err := dec.Decode(&tail); !errors.Is(err, io.EOF) { |
| + return a.refuseBody(w, r, err) |
| + } |
| + return true |
| +} |
| + |
| +// refuseBody is the one place a body error becomes a status: a cap |
| +// breach is 413 and everything else — malformed JSON, an unknown field, |
| +// a trailing value, an empty body, a read that failed — is 400. Always |
| +// returns false, so a caller can `return a.refuseBody(...)`. |
| +func (a *app) refuseBody(w http.ResponseWriter, r *http.Request, err error) bool { |
| + var tooBig *http.MaxBytesError |
| + if errors.As(err, &tooBig) { |
| + a.refuseAPI(w, r, http.StatusRequestEntityTooLarge, map[string]any{"error": "too-large"}, "size") |
| return false |
| } |
| - if dec.More() { |
| - a.refuseAPI(w, r, http.StatusBadRequest, map[string]any{"error": "bad-request"}, "shape") |
| + a.refuseAPI(w, r, http.StatusBadRequest, map[string]any{"error": "bad-request"}, "shape") |
| + return false |
| +} |
| + |
| +// apiNoBody is the bodyless writes' half of the same rule (review |
| +// S5-1): `/links` and `/end` take no body at all, so the body is read |
| +// to its end — under capAPIBody's cap — and refused if it carried |
| +// anything, before the write rather than after it. Over the cap is 413; |
| +// any other content is 400. An absent body and a Content-Length: 0 body |
| +// are the two shapes that pass. |
| +func (a *app) apiNoBody(w http.ResponseWriter, r *http.Request) bool { |
| + if r.Body == nil { |
| + return true |
| + } |
| + n, err := io.Copy(io.Discard, r.Body) |
| + if err != nil { |
| + return a.refuseBody(w, r, err) |
| + } |
| + if n > 0 { |
| + a.refuseAPI(w, r, http.StatusBadRequest, map[string]any{"error": "bad-request"}, "body") |
| return false |
| } |
| return true |
diff --git a/internal/meet/handlers_meetings.go b/internal/meet/handlers_meetings.go| index b85721b..d9313bb 100644 |
| --- a/internal/meet/handlers_meetings.go |
| +++ b/internal/meet/handlers_meetings.go |
| @@ -1,6 +1,7 @@ |
| package meet |
| import ( |
| + "errors" |
| "net/http" |
| "net/url" |
| "strings" |
| @@ -191,6 +192,14 @@ func (a *app) replaceLink(w http.ResponseWriter, r *http.Request) { |
| } |
| p, _ := currentPerson(r) |
| token, err := a.store.replaceLink(r.Context(), m.ID, actorFor(p)) |
| + // The screen's own answer for an ended meeting is the not-found |
| + // page, above; a meeting that ended between that read and the write |
| + // gets the same page rather than a 500, since the store now settles |
| + // the question inside its transaction (review S5-2). |
| + if errors.Is(err, errMeetingEnded) { |
| + a.ErrorPage(w, r, http.StatusNotFound, "") |
| + return |
| + } |
| if err != nil { |
| a.logger.Error("meet: replace link", "err", err) |
| a.ErrorPage(w, r, http.StatusInternalServerError, "") |
| @@ -218,7 +227,12 @@ func (a *app) endMeeting(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| p, _ := currentPerson(r) |
| - if err := a.store.endMeeting(r.Context(), m.ID, actorFor(p)); err != nil { |
| + // The screen's answer is the same either way: ending a meeting that |
| + // is already ended is the list page, not an error — a member who |
| + // submits the confirm page twice has got what they asked for. The |
| + // bool endMeeting now returns is the API's 409 (review S5-2), which |
| + // this side has never had and does not gain here. |
| + if _, err := a.store.endMeeting(r.Context(), m.ID, actorFor(p)); err != nil { |
| a.logger.Error("meet: end meeting", "err", err) |
| a.ErrorPage(w, r, http.StatusInternalServerError, "") |
| return |
diff --git a/internal/meet/store.go b/internal/meet/store.go| index 2e13f1b..b02015b 100644 |
| --- a/internal/meet/store.go |
| +++ b/internal/meet/store.go |
| @@ -262,8 +262,22 @@ func (s *store) liveLinkRow(ctx context.Context, l MeetingLink) (MeetingLink, Me |
| return l, m, true, nil |
| } |
| +// errMeetingEnded is replaceLink's answer when the meeting ended before |
| +// the transaction reached it (review S5-2). A caller's own earlier read |
| +// of ended_at is a snapshot, not a guarantee: the only read that can be |
| +// trusted against a concurrent end is the one inside the write's own |
| +// transaction, and this is that read's refusal. |
| +var errMeetingEnded = errors.New("meet: the meeting has ended") |
| + |
| // replaceLink revokes every live link for the meeting and mints one |
| // new one, in one transaction, returning the new token once. |
| +// |
| +// The meeting's ended_at is re-read inside that transaction, matched in |
| +// the same WHERE as the id: a meeting row is never deleted, so the one |
| +// way the row fails to match is that it ended, and that is |
| +// errMeetingEnded. Without it a replacement could read a running |
| +// meeting, pause while another request ended it, and leave an |
| +// unrevoked link on an ended meeting. |
| func (s *store) replaceLink(ctx context.Context, meetingID int64, actor string) (string, error) { |
| token, hash, err := newLinkToken() |
| if err != nil { |
| @@ -271,6 +285,13 @@ func (s *store) replaceLink(ctx context.Context, meetingID int64, actor string) |
| } |
| now := s.now().UTC() |
| err = s.g.WithContext(ctx).Transaction(func(tx *gorm.DB) error { |
| + var m Meeting |
| + if err := tx.Where("id = ? AND ended_at IS NULL", meetingID).Take(&m).Error; err != nil { |
| + if errors.Is(err, gorm.ErrRecordNotFound) { |
| + return errMeetingEnded |
| + } |
| + return err |
| + } |
| if err := tx.Model(&MeetingLink{}).Where("meeting_id = ? AND revoked_at IS NULL", meetingID).Update("revoked_at", now).Error; err != nil { |
| return err |
| } |
| @@ -282,17 +303,32 @@ func (s *store) replaceLink(ctx context.Context, meetingID int64, actor string) |
| return token, nil |
| } |
| -// endMeeting sets ended_at and revokes every link. |
| -func (s *store) endMeeting(ctx context.Context, meetingID int64, actor string) error { |
| +// endMeeting sets ended_at and revokes every link, and reports whether |
| +// this call is the one that ended the meeting. False means the |
| +// conditional UPDATE matched no row — the meeting was already ended, |
| +// by another request or an earlier one of this caller's — and nothing |
| +// was written, so the links the winning end revoked stay revoked |
| +// (review S5-2). Two racing ends therefore get one true and one false |
| +// rather than two 200s. |
| +func (s *store) endMeeting(ctx context.Context, meetingID int64, actor string) (bool, error) { |
| now := s.now().UTC() |
| - return s.g.WithContext(ctx).Transaction(func(tx *gorm.DB) error { |
| + ended := false |
| + err := s.g.WithContext(ctx).Transaction(func(tx *gorm.DB) error { |
| res := tx.Model(&Meeting{}).Where("id = ? AND ended_at IS NULL", meetingID). |
| Select("ended_at", "ended_by").Updates(map[string]any{"ended_at": now, "ended_by": actor}) |
| if res.Error != nil { |
| return res.Error |
| } |
| + if res.RowsAffected == 0 { |
| + return nil |
| + } |
| + ended = true |
| return tx.Model(&MeetingLink{}).Where("meeting_id = ? AND revoked_at IS NULL", meetingID).Update("revoked_at", now).Error |
| }) |
| + if err != nil { |
| + return false, err |
| + } |
| + return ended, nil |
| } |
| func (s *store) grantFor(ctx context.Context, meetingID, personID int64) (GuestGrant, bool, error) { |
diff --git a/internal/meet/store_test.go b/internal/meet/store_test.go| index 7841f1f..b18e289 100644 |
| --- a/internal/meet/store_test.go |
| +++ b/internal/meet/store_test.go |
| @@ -3,6 +3,7 @@ package meet |
| import ( |
| "bytes" |
| "context" |
| + "errors" |
| "io" |
| "log/slog" |
| "path/filepath" |
| @@ -73,14 +74,71 @@ func TestEndMeetingKillsItsLinks(t *testing.T) { |
| s, _ := testStore(t) |
| ctx := context.Background() |
| m, tok, _ := s.createMeeting(ctx, "x", nil, "person:1") |
| - if err := s.endMeeting(ctx, m.ID, "person:1"); err != nil { |
| + ended, err := s.endMeeting(ctx, m.ID, "person:1") |
| + if err != nil { |
| t.Fatal(err) |
| } |
| + if !ended { |
| + t.Fatal("the first end reports it changed nothing") |
| + } |
| if _, _, ok, _ := s.liveLink(ctx, tok); ok { |
| t.Fatal("link live after end") |
| } |
| } |
| +// Review S5-2: the ended check belongs inside the write, not in the |
| +// handler's earlier read of the row. endMeeting's conditional UPDATE |
| +// is the authority — the second end changes no row and says so, which |
| +// is what the API's 409 is built on — and replaceLink re-reads |
| +// ended_at inside its own transaction, so a replacement cannot land an |
| +// unrevoked link on a meeting that ended while it was being prepared. |
| +// |
| +// Sequential rather than concurrent on purpose: the interleaving that |
| +// matters (read, pause, other request ends, write) is exactly what |
| +// calling the second method after the first reproduces, and it does so |
| +// deterministically. The API-level concurrent case is |
| +// TestAPIConcurrentEndsAndReplacesSeeTheEndedRow in meettest. |
| +func TestASecondEndChangesNothingAndAReplaceAfterEndIsRefused(t *testing.T) { |
| + s, _ := testStore(t) |
| + ctx := context.Background() |
| + m, _, _ := s.createMeeting(ctx, "x", nil, "person:1") |
| + |
| + first, err := s.endMeeting(ctx, m.ID, "person:1") |
| + if err != nil || !first { |
| + t.Fatalf("the first end: ended=%v err=%v", first, err) |
| + } |
| + // The stored end is the first one's, not the second's: the second |
| + // changes no row at all, so ended_by is not overwritten. |
| + second, err := s.endMeeting(ctx, m.ID, "person:2") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if second { |
| + t.Fatal("a second end reports it ended the meeting") |
| + } |
| + after, ok, err := s.meetingBySqid(ctx, m.Sqid) |
| + if err != nil || !ok { |
| + t.Fatalf("re-read: ok=%v err=%v", ok, err) |
| + } |
| + if after.EndedBy != "person:1" { |
| + t.Fatalf("ended_by %q, want the end that won", after.EndedBy) |
| + } |
| + |
| + // And a replacement that arrives after the end is refused by the |
| + // store itself, with no link row minted. |
| + tok, err := s.replaceLink(ctx, m.ID, "person:1") |
| + if !errors.Is(err, errMeetingEnded) { |
| + t.Fatalf("replace after end: token=%q err=%v, want errMeetingEnded", tok, err) |
| + } |
| + var live int64 |
| + if err := s.g.Model(&MeetingLink{}).Where("meeting_id = ? AND revoked_at IS NULL", m.ID).Count(&live).Error; err != nil { |
| + t.Fatal(err) |
| + } |
| + if live != 0 { |
| + t.Fatalf("%d live links on an ended meeting, want none", live) |
| + } |
| +} |
| + |
| func TestGrantExpiresTwoHoursAfterStartOrCreation(t *testing.T) { |
| s, now := testStore(t) |
| ctx := context.Background() |
diff --git a/internal/meettest/api_test.go b/internal/meettest/api_test.go| index 04d2034..2e2ca1a 100644 |
| --- a/internal/meettest/api_test.go |
| +++ b/internal/meettest/api_test.go |
| @@ -13,6 +13,7 @@ import ( |
| "net/url" |
| "strconv" |
| "strings" |
| + "sync" |
| "testing" |
| "time" |
| @@ -90,6 +91,19 @@ func apiMeeting(t *testing.T, body map[string]any) map[string]any { |
| return m |
| } |
| +// apiMeetings is the list as the surface answers it, for the tests |
| +// that assert a refused request changed nothing: the list afterwards |
| +// is the state, read the same way an agent would read it. |
| +func apiMeetings(t *testing.T, x *harness, token string) []any { |
| + t.Helper() |
| + list := apiBody(t, api(t, x, http.MethodGet, "/api/v1/meetings", token, ""), http.StatusOK) |
| + rows, ok := list["meetings"].([]any) |
| + if !ok { |
| + t.Fatalf("meetings %#v, want a list", list["meetings"]) |
| + } |
| + return rows |
| +} |
| + |
| func apiString(t *testing.T, m map[string]any, key string) string { |
| t.Helper() |
| s, ok := m[key].(string) |
| @@ -403,8 +417,19 @@ func TestAPIBodyRules(t *testing.T) { |
| {"a start in the form screen's own format", `{"title":"Standup","starts_at":"2026-09-12T09:00"}`, http.StatusBadRequest, "bad-request"}, |
| {"a title that is not a string", `{"title":7}`, http.StatusBadRequest, "bad-request"}, |
| {"trailing content after the object", `{"title":"Standup"} {}`, http.StatusBadRequest, "bad-request"}, |
| + // Review S5-1: dec.More() answers "is another array or object |
| + // element waiting", which these three are not — they got |
| + // through and created meetings. |
| + {"a trailing close bracket", `{"title":"Standup"}]`, http.StatusBadRequest, "bad-request"}, |
| + {"a trailing close brace", `{"title":"Standup"}}`, http.StatusBadRequest, "bad-request"}, |
| + {"a trailing scalar", `{"title":"Standup"} 7`, http.StatusBadRequest, "bad-request"}, |
| {"no body at all", ``, http.StatusBadRequest, "bad-request"}, |
| {"seventeen kilobytes", `{"title":"` + strings.Repeat("a", 17*1024) + `"}`, http.StatusRequestEntityTooLarge, "too-large"}, |
| + // And the size error survives a valid object: the excess is |
| + // whitespace the decoder would otherwise have skipped past |
| + // while swallowing the cap's own error (review S5-1). |
| + {"a valid object then seventeen kilobytes of space", `{"title":"Standup"}` + strings.Repeat(" ", 17*1024), |
| + http.StatusRequestEntityTooLarge, "too-large"}, |
| } { |
| t.Run(tc.name, func(t *testing.T) { |
| rec := api(t, x, http.MethodPost, "/api/v1/meetings", token, tc.body) |
| @@ -412,6 +437,12 @@ func TestAPIBodyRules(t *testing.T) { |
| if body["error"] != tc.err { |
| t.Fatalf("%v, want error %q", body, tc.err) |
| } |
| + // A refused body writes nothing: no meeting exists yet, and |
| + // the refusal did not make one (review S5-1 — the shapes |
| + // above used to create meetings and answer 201). |
| + if n := len(apiMeetings(t, x, token)); n != 0 { |
| + t.Fatalf("a refused request left %d meetings behind", n) |
| + } |
| }) |
| } |
| @@ -465,6 +496,158 @@ func TestAPIBodyRules(t *testing.T) { |
| } |
| } |
| +// 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 |
| +// the surface is capped by the group's own middleware, and the two |
| +// bodyless writes refuse anything they are sent: over the cap is 413, |
| +// any other content is 400, and both answer before the write. |
| +func TestAPIBodylessWritesRefuseABody(t *testing.T) { |
| + x := newHarness(t, "paul@example.com") |
| + c := x.browser(t) |
| + c.signIn("paul@example.com") |
| + token := mintAgent(t, x, c, "claude") |
| + |
| + created := apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings", token, |
| + `{"title":"Standup"}`), http.StatusCreated) |
| + sqid := apiString(t, apiMeeting(t, created), "sqid") |
| + link := apiString(t, created, "link") |
| + |
| + oversize := strings.Repeat("a", 17*1024) |
| + for _, tc := range []struct { |
| + name, op, body string |
| + status int |
| + err string |
| + }{ |
| + {"a JSON object at links", "links", `{"title":"x"}`, http.StatusBadRequest, "bad-request"}, |
| + {"a JSON object at end", "end", `{}`, http.StatusBadRequest, "bad-request"}, |
| + {"a single space at links", "links", ` `, http.StatusBadRequest, "bad-request"}, |
| + {"a single space at end", "end", ` `, http.StatusBadRequest, "bad-request"}, |
| + {"seventeen kilobytes at links", "links", oversize, http.StatusRequestEntityTooLarge, "too-large"}, |
| + {"seventeen kilobytes at end", "end", oversize, http.StatusRequestEntityTooLarge, "too-large"}, |
| + } { |
| + t.Run(tc.name, func(t *testing.T) { |
| + body := apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings/"+sqid+"/"+tc.op, token, tc.body), tc.status) |
| + if body["error"] != tc.err { |
| + t.Fatalf("%v, want error %q", body, tc.err) |
| + } |
| + // Nothing was written: the meeting still runs and still has |
| + // exactly the link it was created with. |
| + m := apiMeeting(t, apiBody(t, api(t, x, http.MethodGet, "/api/v1/meetings/"+sqid, token, ""), http.StatusOK)) |
| + if m["ended_at"] != nil { |
| + t.Fatalf("a refused %s ended the meeting: %v", tc.op, m) |
| + } |
| + if m["link_present"] != true { |
| + t.Fatalf("a refused %s left no live link: %v", tc.op, m) |
| + } |
| + }) |
| + } |
| + |
| + // The link the meeting was created with is still the live one: a |
| + // refused replace minted nothing that could have taken its place. |
| + if rec := x.browser(t).get("/j/" + strings.TrimPrefix(link, testOrigin+"/j/")); rec.Code == http.StatusNotFound { |
| + t.Fatal("the original link stopped working after the refusals") |
| + } |
| + |
| + // The positive control: with no body at all, both writes work. |
| + replaced := apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings/"+sqid+"/links", token, ""), http.StatusOK) |
| + if apiString(t, replaced, "link") == link { |
| + t.Fatal("replace handed back the same link") |
| + } |
| + ended := apiMeeting(t, apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings/"+sqid+"/end", token, ""), http.StatusOK)) |
| + if ended["ended_at"] == nil { |
| + t.Fatalf("a bodyless end did not end the meeting: %v", ended) |
| + } |
| +} |
| + |
| +// Review S5-2: both write handlers used to decide "has this meeting |
| +// 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. |
| +// |
| +// 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. |
| +func TestAPIConcurrentEndsAndReplacesSeeTheEndedRow(t *testing.T) { |
| + x := newHarness(t, "paul@example.com") |
| + c := x.browser(t) |
| + c.signIn("paul@example.com") |
| + token := mintAgent(t, x, c, "claude") |
| + |
| + // Two ends at once: one 200, one 409, never two of either. |
| + first := apiString(t, apiMeeting(t, apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings", token, |
| + `{"title":"Standup"}`), http.StatusCreated)), "sqid") |
| + ends := race(t, x, token, |
| + http.MethodPost, "/api/v1/meetings/"+first+"/end", |
| + http.MethodPost, "/api/v1/meetings/"+first+"/end") |
| + codes := []int{ends[0].Code, ends[1].Code} |
| + if !(codes[0] == http.StatusOK && codes[1] == http.StatusConflict) && |
| + !(codes[1] == http.StatusOK && codes[0] == http.StatusConflict) { |
| + t.Fatalf("two racing ends answered %d and %d, want one 200 and one 409\n%s\n%s", |
| + codes[0], codes[1], ends[0].Body.String(), ends[1].Body.String()) |
| + } |
| + for _, rec := range ends { |
| + if rec.Code == http.StatusConflict { |
| + if body := apiBody(t, rec, http.StatusConflict); body["error"] != "ended" { |
| + t.Fatalf("the losing end: %v, want error ended", body) |
| + } |
| + } |
| + } |
| + |
| + // 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. |
| + 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, |
| + http.MethodPost, "/api/v1/meetings/"+second+"/end", |
| + http.MethodPost, "/api/v1/meetings/"+second+"/links") |
| + if both[0].Code != http.StatusOK { |
| + t.Fatalf("the end lost to the replacement: %d\n%s", both[0].Code, both[0].Body.String()) |
| + } |
| + if both[1].Code != http.StatusOK && both[1].Code != http.StatusConflict { |
| + t.Fatalf("the racing replacement: %d, want 200 or 409\n%s", both[1].Code, both[1].Body.String()) |
| + } |
| + m := apiMeeting(t, apiBody(t, api(t, x, http.MethodGet, "/api/v1/meetings/"+second, token, ""), http.StatusOK)) |
| + if m["ended_at"] == nil { |
| + t.Fatalf("the meeting is not ended after the race: %v", m) |
| + } |
| + if m["link_present"] != false { |
| + t.Fatalf("an ended meeting still has a live link: %v — the replacement landed after the end", m) |
| + } |
| +} |
| + |
| +// 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 |
| +// test's own goroutine afterwards. |
| +func race(t *testing.T, x *harness, token, m1, t1, m2, t2 string) [2]*httptest.ResponseRecorder { |
| + t.Helper() |
| + var out [2]*httptest.ResponseRecorder |
| + start := make(chan struct{}) |
| + var wg sync.WaitGroup |
| + for i, call := range [2][2]string{{m1, t1}, {m2, t2}} { |
| + wg.Add(1) |
| + go func() { |
| + defer wg.Done() |
| + req := httptest.NewRequest(call[0], call[1], nil) |
| + req.Header.Set("Authorization", "Bearer "+token) |
| + rec := httptest.NewRecorder() |
| + <-start |
| + x.h.ServeHTTP(rec, req) |
| + out[i] = rec |
| + }() |
| + } |
| + close(start) |
| + wg.Wait() |
| + return out |
| +} |
| + |
| func mustJSON(t *testing.T, v any) []byte { |
| t.Helper() |
| b, err := json.Marshal(v) |
| @@ -485,10 +668,43 @@ func TestAPILogsCarryNoToken(t *testing.T) { |
| c.signIn("paul@example.com") |
| token := mintAgent(t, x, c, "claude") |
| - created := apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings", token, |
| - `{"title":"Quarterly review"}`), http.StatusCreated) |
| + // Success is silent, measured around each successful operation |
| + // rather than before the first one (review S5-3: the mark used to |
| + // be taken *after* the create, so a line logged by every successful |
| + // create would have gone unnoticed). silent runs the operation and |
| + // asserts nothing at all was logged by it. |
| + silent := func(what string, op func()) { |
| + t.Helper() |
| + mark := buf.Len() |
| + op() |
| + if n := strings.Count(buf.String()[mark:], "meet: api"); n != 0 { |
| + t.Fatalf("%s logged %d api lines, want none:\n%s", what, n, buf.String()[mark:]) |
| + } |
| + } |
| + |
| + var created map[string]any |
| + silent("a successful create", func() { |
| + created = apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings", token, |
| + `{"title":"Quarterly review"}`), http.StatusCreated) |
| + }) |
| link := apiString(t, created, "link") |
| linkToken := strings.TrimPrefix(link, testOrigin+"/j/") |
| + sqid := apiString(t, apiMeeting(t, created), "sqid") |
| + |
| + silent("a successful list", func() { |
| + apiBody(t, api(t, x, http.MethodGet, "/api/v1/meetings", token, ""), http.StatusOK) |
| + }) |
| + silent("a successful read", func() { |
| + apiBody(t, api(t, x, http.MethodGet, "/api/v1/meetings/"+sqid, token, ""), http.StatusOK) |
| + }) |
| + var replaced map[string]any |
| + silent("a successful link replacement", func() { |
| + replaced = apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings/"+sqid+"/links", token, ""), http.StatusOK) |
| + }) |
| + newLinkToken := strings.TrimPrefix(apiString(t, replaced, "link"), testOrigin+"/j/") |
| + silent("a successful end", func() { |
| + apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings/"+sqid+"/end", token, ""), http.StatusOK) |
| + }) |
| before := buf.Len() |
| apiBody(t, api(t, x, http.MethodGet, "/api/v1/meetings/"+unknownSqid, token, ""), http.StatusNotFound) |
| @@ -496,7 +712,11 @@ func TestAPILogsCarryNoToken(t *testing.T) { |
| api(t, x, http.MethodGet, "/api/v1/meetings", "nope", "") |
| log := buf.String() |
| - for _, secret := range []string{token, meetHash(token), linkToken, "Quarterly review"} { |
| + // The operator's own address is a secret this log has no business |
| + // carrying either (review S5-3): the agent is named by id, and the |
| + // principal that spells out an operator's address is a rendering |
| + // for a member's screen, not a log field. |
| + for _, secret := range []string{token, meetHash(token), linkToken, newLinkToken, "Quarterly review", "paul@example.com"} { |
| if secret != "" && strings.Contains(log, secret) { |
| t.Fatalf("the log carries a secret (%q):\n%s", secret, log) |
| } |
| @@ -507,8 +727,7 @@ func TestAPILogsCarryNoToken(t *testing.T) { |
| if !strings.Contains(log[before:], "agent_id") { |
| t.Fatalf("a resolved agent's refusal carries no agent_id:\n%s", log[before:]) |
| } |
| - // A success is silent: the only lines since the refusals began are |
| - // the refusal lines themselves. |
| + // And the refusals log one line each, no more. |
| if n := strings.Count(log[before:], "meet: api"); n != 3 { |
| t.Fatalf("want exactly three api log lines (one per refusal), got %d:\n%s", n, log[before:]) |
| } |
| @@ -563,13 +782,17 @@ func TestAPIWritesAreMarkedAsTheAgents(t *testing.T) { |
| c.signIn("paul@example.com") |
| token := mintAgent(t, x, c, "claude") |
| - apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings", token, |
| - `{"title":"Agent meeting"}`), http.StatusCreated) |
| - createMeeting(t, c, "Member meeting", "") |
| + // Neutral titles on both rows (review S5-4): the old agent row was |
| + // called "Agent meeting", so searching the row for "Agent" passed |
| + // on the title alone and an empty badge would have gone unnoticed. |
| + created := apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings", token, |
| + `{"title":"Quarterly review"}`), http.StatusCreated) |
| + sqid := apiString(t, apiMeeting(t, created), "sqid") |
| + createMeeting(t, c, "Standup", "") |
| body := c.get("/").Body.String() |
| - agentRow := rowFor(t, body, "Agent meeting") |
| - memberRow := rowFor(t, body, "Member meeting") |
| + agentRow := rowFor(t, body, "Quarterly review") |
| + memberRow := rowFor(t, body, "Standup") |
| if !strings.Contains(agentRow, "claude~paul@example.com") { |
| t.Fatalf("the agent's meeting shows no principal:\n%s", agentRow) |
| } |
| @@ -577,10 +800,11 @@ func TestAPIWritesAreMarkedAsTheAgents(t *testing.T) { |
| t.Fatalf("the agent's meeting shows no robot badge:\n%s", agentRow) |
| } |
| // The icon is decorative (aria-hidden); the badge's own word is |
| - // what a screen reader actually gets. Assert both, so either half |
| - // going missing is caught rather than just the icon's class. |
| - if !strings.Contains(agentRow, "rst-badge") || !strings.Contains(agentRow, "Agent") { |
| - t.Fatalf("the agent's meeting shows no \"Agent\" badge:\n%s", agentRow) |
| + // what a screen reader actually gets — so the assertion is on the |
| + // badge element's own text, not on the row containing the word |
| + // somewhere. |
| + if got := badgeText(t, agentRow); got != "Agent" { |
| + t.Fatalf("the badge reads %q, want \"Agent\":\n%s", got, agentRow) |
| } |
| if strings.Contains(memberRow, "lucide-bot") || strings.Contains(memberRow, "~") { |
| t.Fatalf("a member's own meeting is badged as an agent's:\n%s", memberRow) |
| @@ -588,6 +812,61 @@ func TestAPIWritesAreMarkedAsTheAgents(t *testing.T) { |
| if strings.Contains(memberRow, "rst-badge") || strings.Contains(memberRow, "Agent") { |
| t.Fatalf("a member's own meeting carries an agent badge:\n%s", memberRow) |
| } |
| + |
| + // A create is not the only write an agent makes (review S5-4): the |
| + // replacement link and the end record the acting agent too, in the |
| + // same actor shape, read straight out of the rows. |
| + agentID := agentRevokeID(t, c.get("/settings").Body.String()) |
| + wantActor := "agent:" + agentID |
| + |
| + apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings/"+sqid+"/links", token, ""), http.StatusOK) |
| + var linkActor string |
| + if err := x.sqlDB().QueryRow( |
| + `SELECT created_by FROM meeting_links WHERE meeting_id = (SELECT id FROM meetings WHERE sqid = ?) AND revoked_at IS NULL`, |
| + sqid).Scan(&linkActor); err != nil { |
| + t.Fatal(err) |
| + } |
| + if linkActor != wantActor { |
| + t.Fatalf("the replacement link's created_by is %q, want %q", linkActor, wantActor) |
| + } |
| + |
| + apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings/"+sqid+"/end", token, ""), http.StatusOK) |
| + var endActor string |
| + if err := x.sqlDB().QueryRow(`SELECT ended_by FROM meetings WHERE sqid = ?`, sqid).Scan(&endActor); err != nil { |
| + t.Fatal(err) |
| + } |
| + if endActor != wantActor { |
| + t.Fatalf("the meeting's ended_by is %q, want %q", endActor, wantActor) |
| + } |
| + |
| + // And the member's own meeting records a person, not an agent — so |
| + // the two assertions above are about the actor and not about every |
| + // write recording the same string. |
| + var memberActor string |
| + if err := x.sqlDB().QueryRow(`SELECT created_by FROM meetings WHERE sqid <> ?`, sqid).Scan(&memberActor); err != nil { |
| + t.Fatal(err) |
| + } |
| + if !strings.HasPrefix(memberActor, "person:") { |
| + t.Fatalf("the member's own meeting records %q, want a person actor", memberActor) |
| + } |
| +} |
| + |
| +// badgeText is the text inside the first ui/badge span of a fragment — |
| +// <span rst-badge>Agent</span> — so a badge that renders empty fails |
| +// the assertion the way a missing one does (review S5-4). |
| +func badgeText(t *testing.T, fragment string) string { |
| + t.Helper() |
| + i := strings.Index(fragment, "<span rst-badge") |
| + if i < 0 { |
| + t.Fatalf("no badge element:\n%s", fragment) |
| + } |
| + rest := fragment[i:] |
| + open := strings.Index(rest, ">") |
| + end := strings.Index(rest, "</span>") |
| + if open < 0 || end < 0 || end < open { |
| + t.Fatalf("the badge element is not closed:\n%s", fragment) |
| + } |
| + return rest[open+1 : end] |
| } |
| // Spec §8.1's principal is per agent, not per operator: two agents |