amadan
fix: whole-branch review fixes W5-1..W5-5
- W5-1: assert the "Agent" ui/badge (rst-badge + word), not just the lucide-bot icon's class, in TestAPIWritesAreMarkedAsTheAgents. - W5-2: new test, two agents of one operator each create a meeting over the API; each list row shows its own principal. - W5-3: apiBody reads v.CreatedByPrincipal instead of re-resolving through creatorLabel; drops its labels parameter. - W5-4: spec §15.17 cites rastrillo's icons.go for the icon rule alongside AGENTS.md's own never-colour-alone line. - W5-5: record §3 carries apiServerError's raw-driver-error logging as informational; §2 records the whole-branch review's merge verdict.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
4 files changed,
+70
−19
docs/superpowers/reviews/2026-09-11-slice-5-record.md+10 −0docs/superpowers/specs/2026-09-07-meet-design.md+4 −3internal/meet/api.go+13 −16internal/meettest/api_test.go+43 −0
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 d5ed9a4..fb7508b 100644 |
| --- a/docs/superpowers/reviews/2026-09-11-slice-5-record.md |
| +++ b/docs/superpowers/reviews/2026-09-11-slice-5-record.md |
| @@ -117,6 +117,8 @@ followed: |
| | 3 — the API: bearer, reach, five operations | `api.go` (`requireAgent`, `apiJSON`, `apiNotFound`, `refuseAPI`, `apiServerError`, `apiDecode`, `apiRoutes`, the five handlers), `hub.AdmittedCount`, `store.meetingViewOf` (factored out of `listMeetings`), `store.agentAnyByID`/`creatorLabel`/`withOperator`; eight tests, one shipped `t.Skip`ped for Task 4 | **Clean**, 8 mutations run (6 killed; 2 survivors: a `link` key left addable to a GET response, an `admitted` key left readable off an ended meeting's own GET) | Addendum folded into Task 4 rather than a separate round: both survivors closed by new assertions in `TestAPIListsCreatesLinksAndEnds`; title length moved from bytes to runes on both surfaces, one shared helper; the list row's six-key set pinned. §15.17 recorded the csrf mount, the header-only 401 rule, the API's own non-promises (§3 below), and the badge | Complete, addendum closed by Task 4's commit. | |
| | 4 — the badge, the record, the spec note | `meetingView.CreatedByPrincipal`/`ByAgent` (`meetingViewOfCached`), `icons.go`'s `bot`, `index.html`'s badge and "by `<principal>`" line, the Task 3 addendum (rune-counted titles, the closed mutations, the pinned key set), spec §15.17, this record | This commit's own work; no separate review round within the task | — | This commit. | |
| +**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. |
| + |
| --- |
| ## 3. Carried items, and where they go |
| @@ -162,6 +164,14 @@ followed: |
| fleet it would be that host's count, not the meeting's. |
| **Destination: unchanged**, consistent with everything else the hub |
| answers and already noted for slices 2 through 4. |
| +- **`apiServerError` logs the raw driver error.** (Task 3's review item |
| + 6, which never reached a destination until the whole-branch review |
| + named it W5-5.) It is the one API log line whose content is not a |
| + fixed vocabulary. GORM renders placeholders, not bound arguments, so |
| + a token hash cannot appear in it; no change was asked for. |
| + **Destination: informational, watch on driver/logging changes** — the |
| + line to re-examine if the driver or the logging setup ever changes in |
| + a way that could put a bound value into an error's text. |
| - **The whole-branch review.** `whole-branch-review.md`'s brief was |
| written into the ledger before Task 4 started and has not been run |
| as of this commit — its own six questions (trust end to end, the |
diff --git a/docs/superpowers/specs/2026-09-07-meet-design.md b/docs/superpowers/specs/2026-09-07-meet-design.md| index d379053..ef2b5fc 100644 |
| --- a/docs/superpowers/specs/2026-09-07-meet-design.md |
| +++ b/docs/superpowers/specs/2026-09-07-meet-design.md |
| @@ -1986,7 +1986,8 @@ only when `ByAgent` is true, so a member's own meeting carries neither. |
| The principal is rendered as an ordinary Go string through |
| `html/template`'s own escaping, never marked `template.HTML` and never |
| truncated before the `~` — the badge's icon needs no separate |
| -`aria-label` either, since AGENTS.md's own rule ("every icon sits |
| -beside its own visible text label") is already satisfied by the |
| -badge's own word. |
| +`aria-label` either, since rastrillo's own icon rule (`icons.go`: |
| +every icon "sits beside its own visible text label") is already |
| +satisfied by the badge's own word, alongside AGENTS.md's own line — |
| +"State — never colour alone — always a text label too." |
diff --git a/internal/meet/api.go b/internal/meet/api.go| index 774aa16..59a2b17 100644 |
| --- a/internal/meet/api.go |
| +++ b/internal/meet/api.go |
| @@ -209,19 +209,17 @@ func apiTime(t *time.Time) *string { |
| return &s |
| } |
| -// apiBody renders one view, resolving created_by through labels, a |
| -// per-request memo so a list of meetings one agent made costs one |
| -// lookup rather than one per row. |
| -func (a *app) apiBody(ctx context.Context, v meetingView, labels map[string]string) apiMeetingBody { |
| - by, seen := labels[v.CreatedBy] |
| - if !seen { |
| - by = a.store.creatorLabel(ctx, v.CreatedBy) |
| - labels[v.CreatedBy] = by |
| - } |
| +// apiBody renders one view. created_by is v.CreatedByPrincipal, |
| +// already resolved by meetingViewOfCached (a list shares one label |
| +// cache across its rows; a single-row read passes its own throwaway |
| +// one) — apiBody used to re-resolve it a second time through its own |
| +// labels map, which cost every single-row API path two lookups where |
| +// one would do. |
| +func (a *app) apiBody(v meetingView) apiMeetingBody { |
| return apiMeetingBody{ |
| Sqid: v.Sqid, Title: v.Title, |
| StartsAt: apiTime(v.StartsAt), EndedAt: apiTime(v.EndedAt), |
| - LinkPresent: v.LinkPresent, CreatedBy: by, |
| + LinkPresent: v.LinkPresent, CreatedBy: v.CreatedByPrincipal, |
| } |
| } |
| @@ -231,10 +229,9 @@ func (a *app) apiListMeetings(w http.ResponseWriter, r *http.Request) { |
| a.apiServerError(w, r, "list meetings", err) |
| return |
| } |
| - labels := map[string]string{} |
| out := make([]apiMeetingBody, 0, len(views)) |
| for _, v := range views { |
| - out = append(out, a.apiBody(r.Context(), v, labels)) |
| + out = append(out, a.apiBody(v)) |
| } |
| apiJSON(w, http.StatusOK, map[string]any{"meetings": out}) |
| } |
| @@ -281,7 +278,7 @@ func (a *app) apiCreateMeeting(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| apiJSON(w, http.StatusCreated, map[string]any{ |
| - "meeting": a.apiBody(r.Context(), view, map[string]string{}), |
| + "meeting": a.apiBody(view), |
| "link": a.linkURL(token), |
| }) |
| } |
| @@ -296,7 +293,7 @@ func (a *app) apiGetMeeting(w http.ResponseWriter, r *http.Request) { |
| a.apiServerError(w, r, "read meeting", err) |
| return |
| } |
| - body := map[string]any{"meeting": a.apiBody(r.Context(), view, map[string]string{})} |
| + body := map[string]any{"meeting": a.apiBody(view)} |
| // While it runs, and only while it runs: how many peers are in the |
| // call, never who they are or anything they said (spec §8.2). |
| if m.EndedAt == nil { |
| @@ -326,7 +323,7 @@ func (a *app) apiReplaceLink(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| apiJSON(w, http.StatusOK, map[string]any{ |
| - "meeting": a.apiBody(r.Context(), view, map[string]string{}), |
| + "meeting": a.apiBody(view), |
| "link": a.linkURL(token), |
| }) |
| } |
| @@ -364,7 +361,7 @@ func (a *app) apiEndMeeting(w http.ResponseWriter, r *http.Request) { |
| a.apiServerError(w, r, "read the ended meeting", err) |
| return |
| } |
| - apiJSON(w, http.StatusOK, map[string]any{"meeting": a.apiBody(r.Context(), view, map[string]string{})}) |
| + apiJSON(w, http.StatusOK, map[string]any{"meeting": a.apiBody(view)}) |
| } |
| // apiMeetingFor loads the meeting named in the path, or answers the |
diff --git a/internal/meettest/api_test.go b/internal/meettest/api_test.go| index 5ded266..04d2034 100644 |
| --- a/internal/meettest/api_test.go |
| +++ b/internal/meettest/api_test.go |
| @@ -576,9 +576,52 @@ func TestAPIWritesAreMarkedAsTheAgents(t *testing.T) { |
| if !strings.Contains(agentRow, "lucide-bot") { |
| 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) |
| + } |
| 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) |
| } |
| + if strings.Contains(memberRow, "rst-badge") || strings.Contains(memberRow, "Agent") { |
| + t.Fatalf("a member's own meeting carries an agent badge:\n%s", memberRow) |
| + } |
| +} |
| + |
| +// Spec §8.1's principal is per agent, not per operator: two agents |
| +// minted by the same member must not share a label on the list. This |
| +// pins meetingViewOfCached's label cache key at m.CreatedBy — keying |
| +// it on anything coarser (a constant, the operator) would let the |
| +// first-resolved agent's principal leak onto the second's row. |
| +func TestListShowsEachAgentsOwnPrincipal(t *testing.T) { |
| + x := newHarness(t, "paul@example.com") |
| + c := x.browser(t) |
| + c.signIn("paul@example.com") |
| + tokenA := mintAgent(t, x, c, "agent-a") |
| + tokenB := mintAgent(t, x, c, "agent-b") |
| + |
| + apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings", tokenA, |
| + `{"title":"Meeting A"}`), http.StatusCreated) |
| + apiBody(t, api(t, x, http.MethodPost, "/api/v1/meetings", tokenB, |
| + `{"title":"Meeting B"}`), http.StatusCreated) |
| + |
| + body := c.get("/").Body.String() |
| + rowA := rowFor(t, body, "Meeting A") |
| + rowB := rowFor(t, body, "Meeting B") |
| + if !strings.Contains(rowA, "agent-a~paul@example.com") { |
| + t.Fatalf("agent-a's meeting shows the wrong principal:\n%s", rowA) |
| + } |
| + if strings.Contains(rowA, "agent-b~paul@example.com") { |
| + t.Fatalf("agent-a's meeting also shows agent-b's principal:\n%s", rowA) |
| + } |
| + if !strings.Contains(rowB, "agent-b~paul@example.com") { |
| + t.Fatalf("agent-b's meeting shows the wrong principal:\n%s", rowB) |
| + } |
| + if strings.Contains(rowB, "agent-a~paul@example.com") { |
| + t.Fatalf("agent-b's meeting also shows agent-a's principal:\n%s", rowB) |
| + } |
| } |
| // rowFor is one meeting's <article> on the list page, so a badge can be |