From 905b035184c4af0f2528d8d5eb2a8e32aba6cd24 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Sun, 18 Oct 2026 16:10:02 +0200 Subject: [PATCH] docs(19.1): add code review report --- .../19.1-chat-satellites/19.1-REVIEW.md | 198 ++++++++++++++++++ 1 file changed, 198 insertions(+) create mode 100644 .planning/phases/19.1-chat-satellites/19.1-REVIEW.md diff --git a/.planning/phases/19.1-chat-satellites/19.1-REVIEW.md b/.planning/phases/19.1-chat-satellites/19.1-REVIEW.md new file mode 100644 index 00000000..88191d15 --- /dev/null +++ b/.planning/phases/19.1-chat-satellites/19.1-REVIEW.md @@ -0,0 +1,198 @@ +--- +phase: 19.1-chat-satellites +reviewed: 2026-10-18T14:09:10Z +depth: standard +files_reviewed: 72 +files_reviewed_list: + - scripts/check-phase19.1.sh + - .planning/notes/2026-10-18-bluemonday-forum-policy.md + - .planning/notes/2026-10-18-webpush-go.md + - .planning/notes/2026-10-18-livekit-server-sdk-go.md + - modules/breakwater/breakwater.go + - ../sm-userfriends-plugin/plugin.go + - ../sm-userfriends-plugin/boot.go + - ../sm-userfriends-plugin/routes.go + - ../sm-userfriends-plugin/api/auth.go + - ../sm-userfriends-plugin/api/errors.go + - ../sm-userfriends-plugin/api/friends.go + - ../sm-userfriends-plugin/api/groups.go + - ../sm-userfriends-plugin/classes/friendship_service.go + - ../sm-userfriends-plugin/classes/group_service.go + - ../sm-userfriends-plugin/classes/group_policy.go + - ../sm-userfriends-plugin/classes/group_visibility_gate.go + - ../sm-userfriends-plugin/classes/friends_context_adapter.go + - ../sm-userfriends-plugin/updates/202610180001_create_golem15_userfriends_baseline.go + - ../sm-userfriends-plugin/testdata/goldens/friend.json + - ../sm-forum-plugin/plugin.go + - ../sm-forum-plugin/boot.go + - ../sm-forum-plugin/routes.go + - ../sm-forum-plugin/api/auth.go + - ../sm-forum-plugin/api/errors.go + - ../sm-forum-plugin/api/json.go + - ../sm-forum-plugin/api/posts.go + - ../sm-forum-plugin/api/threads.go + - ../sm-forum-plugin/classes/deny_all.go + - ../sm-forum-plugin/classes/forum_service.go + - ../sm-forum-plugin/classes/lifecycle.go + - ../sm-forum-plugin/contracts/access_resolver.go + - ../sm-forum-plugin/updates/202610180001_create_golem15_forum_baseline.go + - ../sm-forum-plugin/testdata/goldens/html_xss.json + - ../sm-forum-plugin/testdata/goldens/post.json + - ../sm-chatbots-plugin/plugin.go + - ../sm-chatbots-plugin/boot.go + - ../sm-chatbots-plugin/routes.go + - ../sm-chatbots-plugin/api/auth.go + - ../sm-chatbots-plugin/api/commands.go + - ../sm-chatbots-plugin/classes/deny_all.go + - ../sm-chatbots-plugin/classes/dispatcher.go + - ../sm-chatbots-plugin/classes/help.go + - ../sm-chatbots-plugin/classes/registry.go + - ../sm-chatbots-plugin/contracts/slash_command.go + - ../sm-chatbots-plugin/contracts/bot_auth_resolver.go + - ../sm-herald-plugin/plugin.go + - ../sm-herald-plugin/boot.go + - ../sm-herald-plugin/routes.go + - ../sm-herald-plugin/api/auth.go + - ../sm-herald-plugin/api/push.go + - ../sm-herald-plugin/classes/deny_all.go + - ../sm-herald-plugin/classes/sender.go + - ../sm-herald-plugin/classes/subscription_service.go + - ../sm-herald-plugin/classes/payload.go + - ../sm-herald-plugin/console/vapid_generate.go + - ../sm-herald-plugin/jobs/send.go + - ../sm-herald-plugin/updates/202610180001_create_golem15_herald_baseline.go + - ../sm-herald-plugin/testdata/goldens/web_push_payload.json + - ../sm-chatvideo-plugin/plugin.go + - ../sm-chatvideo-plugin/boot.go + - ../sm-chatvideo-plugin/routes.go + - ../sm-chatvideo-plugin/schedule.go + - ../sm-chatvideo-plugin/api/auth.go + - ../sm-chatvideo-plugin/api/errors.go + - ../sm-chatvideo-plugin/api/video.go + - ../sm-chatvideo-plugin/api/call_links.go + - ../sm-chatvideo-plugin/classes/livekit.go + - ../sm-chatvideo-plugin/classes/authorizer.go + - ../sm-chatvideo-plugin/classes/video_service.go + - ../sm-chatvideo-plugin/updates/202610180001_create_golem15_chatvideo_baseline.go + - ../sm-chatvideo-plugin/testdata/goldens/livekit_jwt_claims.json + - ../sm-chatvideo-plugin/golden_test.go +findings: + critical: 0 + warning: 5 + info: 4 + total: 9 +status: issues_found +--- + +# Phase 19.1: Code Review Report + +**Reviewed:** 2026-10-18T14:09:10Z +**Depth:** standard +**Files Reviewed:** 72 +**Status:** issues_found + +## Summary + +Reviewed the five sibling chat-satellite plugins (`sm-userfriends`, `sm-forum`, `sm-chatbots`, `sm-herald`, `sm-chatvideo`) plus the phase 19.1 summercms.go artifacts (decision notes, `scripts/check-phase19.1.sh`, and `modules/breakwater` for the forum sanitizer boundary). Focus was generic JWT APIs, DenyAll defaults, GDPR cascades, Herald push/VAPID, the LiveKit token issuer, the forum Render/Purify boundary, and secrets in goldens. + +The JWT surface is consistent and fail-closed: every generic prefix names `jwt.auth` (surf refuses unknown middleware at boot), handlers take the actor from `bouncer.User` and reject backend principals, and missing/unauthorized IDs collapse to 404. Forum, chatbots, and Herald DenyAll defaults plus nil-setter coercion are correct. Forum write paths call `breakwater.Render` (which always `Purify`s); GET serves stored `content_html`. Herald subscribe rejects non-HTTPS / private IP literals and sends through `fetchguard.PublicOnlyMode`. Goldens hold no VAPID private keys and no LiveKit API secrets (the HS256 secret stays in the test binary). Userfriends group heir + forum authorship-null + chatbots bot_flag delete + Herald `ON DELETE CASCADE` match the intended GDPR story. + +The defects that remain are on chatvideo's RoomService hop and session lifecycle, chatbots dispatch authorization, and chatvideo's starter CASCADE. + +## Warnings + +### WR-01: LiveKit RoomService never uses the TrustedMode fetchguard client + +**File:** `../sm-chatvideo-plugin/classes/livekit.go:125-146` +**Issue:** D-13 and the LiveKit decision note require HTTP to the operator RoomService URL to go through `fetchguard.Policy{Mode: TrustedMode}` (loopback allowed; no redirects). `NewProductionRooms` builds that client and stores it on `sdkRooms.http`, but `lksdk.NewRoomServiceClient(admin, key, secret)` always constructs its own `newAPIHTTPClient()` (default transport plus region-failover retries). None of `CreateRoom` / `DeleteRoom` / `ListRooms` / `ListParticipants` reads `s.http`. The documented SSRF/redirect boundary is dead code. The public v2.19.0 constructor does not take an `*http.Client`. +**Fix:** Either stop claiming the fetchguard hop and delete the unused client, or construct the Twirp client yourself with the TrustedMode `*http.Client` (the unexported `newRoomServiceClient` already accepts one). Do not pass the SDK's default failover client. + +```go +// If keeping the documented hop, wrap the RoomService protobuf client +// with the TrustedMode http.Client instead of lksdk.NewRoomServiceClient. +client := livekit.NewRoomServiceProtobufClient( + signalling.ToHttpURL(admin), + httpClient, // TrustedMode + CheckRedirect = ErrUseLastResponse +) +``` + +### WR-02: Start commits an active session before CreateRoom succeeds + +**File:** `../sm-chatvideo-plugin/classes/video_service.go:123-155` +**Issue:** `Start` inserts the session with `active_conversation_id` set (unique), invites participants, then calls `CreateRoom`. A LiveKit failure returns the error but leaves the pending unique row. The next `Start` hits `activeSession` and mints a join token for a room that does not exist. There is no compensating delete / clear of `active_conversation_id`. +**Fix:** Create the LiveKit room first, or wrap persist + `CreateRoom` so a room error deletes/clears the row before returning. On unique-violation recovery, do not mint a token unless the room actually exists. + +```go +if err := s.rooms.CreateRoom(ctx, room); err != nil { + _ = s.db.WithContext(ctx).Unscoped().Delete(session) + return nil, err +} +if err := s.db.WithContext(ctx).Create(session).Error; err != nil { + _ = s.rooms.DeleteRoom(ctx, room) + return nil, err +} +``` + +### WR-03: Reconcile ends empty rooms with no grace, including ringing calls + +**File:** `../sm-chatvideo-plugin/classes/video_service.go:284-328` +**Issue:** The 5-minute reconciler treats `ParticipantCount == 0` (or a missing room name) as "end now" for both `pending` and `active` sessions. A just-started call is `active` after `CreateRoom` but typically has zero LiveKit participants until invitees connect. If the scheduled command fires in that window, the call is ended and the room deleted. There is no minimum age. +**Fix:** Skip empty-room expiry unless `started_at` is older than a grace window (for example one cadence, 5 minutes), and keep the `max_duration` path as the hard cap. + +```go +grace := 5 * time.Minute +if n == 0 && now.Sub(session.StartedAt) < grace { + continue +} +``` + +### WR-04: Generic chatbots dispatch ignores IsAvailable + +**File:** `../sm-chatbots-plugin/classes/dispatcher.go:30-42` +**Issue:** `GET /commands` filters `IsAvailable`, but `POST /dispatch` (JWT actor from the token) looks the trigger up and calls `Handle` with no availability check. A host that `Register`s a restricted command (admin-only, conversation-scoped, elevated) can have any frontend JWT invoke it through `/_chatbots/api/v1/dispatch`. Production Boot only installs `/help` (always available), so this is latent until a host command lands. +**Fix:** Refuse unavailable commands the same way as a missing trigger (API 422 `unknown_command`). + +```go +cmd := d.registry.Get(trigger) +if cmd == nil || !cmd.IsAvailable(actor, cmdCtx) { + return nil +} +``` + +### WR-05: Chatvideo starter delete cascades the whole session and skips LiveKit cleanup + +**File:** `../sm-chatvideo-plugin/updates/202610180001_create_golem15_chatvideo_baseline.go:27-28` +**Issue:** `started_by` is `NOT NULL … ON DELETE CASCADE`. There is no `user.Deleting` listener. Deleting the starter drops the session and every participant row for the other people on the call. The LiveKit room is not `DeleteRoom`'d; outstanding join JWTs (`token_ttl` default 900s) remain valid against that room. Forum nulls authorship and keeps bodies; userfriends reassigns `owner_id` specifically so CASCADE does not drop groups. Chatvideo is the outlier. +**Fix:** Listen `user.Deleting` on `ev.Tx`: end active sessions this user started (clear `active_conversation_id`, `DeleteRoom`), then either reassign `started_by` to another participant or SET NULL (requires a nullable column). Do not rely on CASCADE to erase other users' call history. + +## Info + +### IN-01: Forum SetAccessResolver is a silent no-op before Boot + +**File:** `../sm-forum-plugin/plugin.go:82-88` +**Issue:** `SetAccessResolver` returns immediately when `p.service == nil`. Chatbots/Herald store the seam on the plugin so a pre-Boot setter survives `bootService`. A host that calls the forum setter from `Register` (or before forum Boot) keeps DenyAll and only a Warn line explains why every route is 404. +**Fix:** Store the resolver on `Plugin` and apply it in `bootService`, matching Herald `SetPresenceGate`. + +### IN-02: screen_share config is copied and never applied to tokens + +**File:** `../sm-chatvideo-plugin/boot.go:64` +**Issue:** `VIDEO_SCREEN_SHARE` / `golem15.chatvideo.screen_share` is overlaid at Boot. `MintToken` always sets `canPublish: true` and never reads the key. The config is dead. +**Fix:** Thread the flag into `TokenClaims` / the `video` grant, or drop the key from config and the README. + +### IN-03: Herald Deliver swallows every per-subscription send error + +**File:** `../sm-herald-plugin/classes/sender.go:192-196` +**Issue:** `sendOne` failures `continue`; `Deliver` still returns nil. A total outage looks like success to River (`MaxAttempts: 1`). Prune of 404/410 still runs. This hides operator-visible delivery failure beyond the 60s-cooled Warn. +**Fix:** Count failures and return an error when every subscription fails, or increment a metric the worker already logs. + +### IN-04: Group invite `created_by` has no FK and is not cleared on user delete + +**File:** `../sm-userfriends-plugin/updates/202610180001_create_golem15_userfriends_baseline.go:69-73` +**Issue:** `golem15_userfriends_group_invites.created_by` is nullable with no `REFERENCES users`. `OnUserDeleting` reassigns groups and deletes member/ban rows but leaves invite `created_by` pointing at a deleted user. +**Fix:** Add `ON DELETE SET NULL` (or clear `created_by` in `OnUserDeleting`). + +--- + +_Reviewed: 2026-10-18T14:09:10Z_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: standard_