|
| 1 | +# Review guide — chat.agent on Sessions, row-agnostic addressing |
| 2 | + |
| 3 | +Scope: the 12 uncommitted files. **No new behaviour beyond the public surface |
| 4 | +already on this branch** — this is plumbing cleanup that: |
| 5 | + |
| 6 | +1. Eliminates the transport's session-creation step |
| 7 | +2. Makes `chatId` the universal addressing string everywhere |
| 8 | +3. Makes the server-side stream/append/wait routes row-agnostic |
| 9 | + |
| 10 | +## The two design moves |
| 11 | + |
| 12 | +**Move 1 — agent owns session lifecycle.** `chat.agent` and |
| 13 | +`chat.customAgent` upsert the backing `Session` row at bind, fire-and-forget, |
| 14 | +keyed on `externalId = payload.chatId`. The transport, server-side |
| 15 | +`AgentChat`, and `chat.createTriggerAction` no longer create sessions at all. |
| 16 | +Browsers cannot mint sessions either (`POST /api/v1/sessions` is now |
| 17 | +secret-key-only). One owner, one path. |
| 18 | + |
| 19 | +**Move 2 — `chatId` is the only address.** The transport, server-side |
| 20 | +`AgentChat`, JWT scopes, and S2 stream paths all use `chatId` directly. The |
| 21 | +Session's friendlyId is informational. To make this safe, the three stream |
| 22 | +routes (`.in/.out` PUT, GET, POST append, plus the run-engine `wait` |
| 23 | +endpoint) became "row-optional" and derive a *canonical addressing key* |
| 24 | +(`row.externalId ?? row.friendlyId`, fallback to the URL param when the row |
| 25 | +hasn't been upserted yet). Same canonical key is used to build the S2 stream |
| 26 | +path, the waitpoint cache key, and the JWT resource set — so any caller |
| 27 | +addressing by either form converges on the same physical stream. |
| 28 | + |
| 29 | +Together these remove an entire class of "did the row land yet?" races. The |
| 30 | +transport can subscribe to `/sessions/{chatId}/out` before the agent boots, |
| 31 | +the agent's `void sessions.create({externalId: chatId})` lands a moment |
| 32 | +later, and any earlier reads/writes are already on the right S2 key. |
| 33 | + |
| 34 | +--- |
| 35 | + |
| 36 | +## Read in this order |
| 37 | + |
| 38 | +### 1. `apps/webapp/app/services/realtime/sessions.server.ts` (+34 lines) |
| 39 | + |
| 40 | +The new primitive. Two helpers: |
| 41 | + |
| 42 | +- `isSessionFriendlyIdForm(value)` — `value.startsWith("session_")`. Used to |
| 43 | + decide whether a missing row is a hard 404 (opaque friendlyId) or a soft |
| 44 | + "row will land later" (externalId form). |
| 45 | +- `canonicalSessionAddressingKey(row, paramSession)` — `row.externalId ?? |
| 46 | + row.friendlyId` if the row exists, else `paramSession`. **This is the load- |
| 47 | + bearing function.** Read its docstring. |
| 48 | + |
| 49 | +**Question to ask:** can two callers addressing the "same" session ever get |
| 50 | +different canonical keys? Only if the row exists for one and not the other, |
| 51 | +*and* the URL forms differ — but in that case the row-less caller used the |
| 52 | +externalId form (friendlyId-form would have 404'd earlier), and the row-ful |
| 53 | +caller computes `row.externalId ?? row.friendlyId`. If the row's externalId |
| 54 | +matches the URL, they converge. If it doesn't, there's no row to find by |
| 55 | +that string anyway. The interesting edge is "row exists with no externalId", |
| 56 | +addressed via friendlyId — both sides read `row.friendlyId`. ✓ |
| 57 | + |
| 58 | +### 2. `apps/webapp/app/routes/realtime.v1.sessions.$session.$io.ts` (+47/-12) |
| 59 | + |
| 60 | +PUT initialize + GET subscribe (SSE). Both use the helper. The interesting |
| 61 | +part is the loader's `findResource` + `authorization.resource`: |
| 62 | + |
| 63 | +```ts |
| 64 | +findResource: async (params, auth) => { |
| 65 | + const row = await resolveSessionByIdOrExternalId(...); |
| 66 | + if (!row && isSessionFriendlyIdForm(params.session)) return undefined; // 404 |
| 67 | + return { row, addressingKey: canonicalSessionAddressingKey(row, params.session) }; |
| 68 | +}, |
| 69 | +authorization: { |
| 70 | + resource: ({ row, addressingKey }) => { |
| 71 | + const ids = new Set<string>([addressingKey]); |
| 72 | + if (row) { |
| 73 | + ids.add(row.friendlyId); |
| 74 | + if (row.externalId) ids.add(row.externalId); |
| 75 | + } |
| 76 | + return { sessions: [...ids] }; |
| 77 | + }, |
| 78 | + superScopes: ["read:sessions", "read:all", "admin"], |
| 79 | +}, |
| 80 | +``` |
| 81 | + |
| 82 | +**Why three IDs in the resource set?** `checkAuthorization` is "any-match" |
| 83 | +across the resource values. We want a JWT scoped to *either* form to |
| 84 | +authorize *either* URL form. Smoke test verified the 4-cell matrix passes. |
| 85 | + |
| 86 | +**The PUT path** (action handler) is simpler — it just resolves the row, |
| 87 | +builds an addressing key, and hands it to `initializeSessionStream`. Worth |
| 88 | +noting the `closedAt` check is now `maybeSession?.closedAt` — no row means |
| 89 | +no closedAt to enforce. |
| 90 | + |
| 91 | +### 3. `apps/webapp/app/routes/realtime.v1.sessions.$session.$io.append.ts` (+22/-13) |
| 92 | + |
| 93 | +POST append (browser writes a record to `.in` or server writes to `.out`). |
| 94 | +Same row-optional pattern. Both the S2 append and the waitpoint drain use |
| 95 | +`addressingKey`. |
| 96 | + |
| 97 | +**Question to ask:** what fires the waitpoint? An agent's |
| 98 | +`session.in.wait()` registers a waitpoint keyed on `(addressingKey, io)` via |
| 99 | +the wait endpoint (file 4). The append handler drains by the *same* key — |
| 100 | +even if the agent registered with externalId form and the transport |
| 101 | +appended via friendlyId form, both compute the same canonical key, so they |
| 102 | +converge. ✓ |
| 103 | + |
| 104 | +### 4. `apps/webapp/app/routes/api.v1.runs.$runFriendlyId.session-streams.wait.ts` (+18/-13) |
| 105 | + |
| 106 | +The agent's `.in.wait()` endpoint. Run-engine creates the waitpoint, then |
| 107 | +registers it in Redis under `(addressingKey, io)`. The race-check that runs |
| 108 | +right after creation reads from S2 by the same key. Three call sites — |
| 109 | +`addSessionStreamWaitpoint`, `readSessionStreamRecords`, |
| 110 | +`removeSessionStreamWaitpoint` — all consistent. |
| 111 | + |
| 112 | +### 5. `apps/webapp/app/routes/api.v1.sessions.ts` (+4/-2) |
| 113 | + |
| 114 | +**Security tightening.** Removed `allowJWT: true` and `corsStrategy: "all"` |
| 115 | +from the `POST /api/v1/sessions` action — secret-key only now. |
| 116 | + |
| 117 | +**Question to ask:** was the JWT path actually used? Until this branch, the |
| 118 | +transport called it via `ensureSession` (now deleted). After this branch, |
| 119 | +nobody reaches it from the browser. `chat.createTriggerAction` (server |
| 120 | +secret key) is the only browser-adjacent path. |
| 121 | + |
| 122 | +### 6. `packages/trigger-sdk/src/v3/ai.ts` (+62/-39) |
| 123 | + |
| 124 | +Two near-identical edits — one in `chatAgent`, one in `chatCustomAgent`. |
| 125 | +Both bind on `payload.chatId` and fire-and-forget the upsert: |
| 126 | + |
| 127 | +```ts |
| 128 | +locals.set(chatSessionHandleKey, sessions.open(payload.chatId)); |
| 129 | +void sessions |
| 130 | + .create({ type: "chat.agent", externalId: payload.chatId }) |
| 131 | + .catch(() => { /* best effort */ }); |
| 132 | +``` |
| 133 | + |
| 134 | +**Question to ask:** why `void`-and-`catch`? Awaiting the upsert would gate |
| 135 | +the agent's bind on a network round-trip that doesn't unblock anything |
| 136 | +user-visible — `.in/.out` routes are row-agnostic and the waitpoint cache |
| 137 | +is keyed on the addressing string, not the row id. If the upsert genuinely |
| 138 | +fails, the next bind retries the same idempotent call (`sessions.create` |
| 139 | +upserts on `externalId`, so concurrent triggers on one chatId converge to |
| 140 | +one row). The row matters for downstream metadata + listing, not for live |
| 141 | +addressing. |
| 142 | + |
| 143 | +The PAT scope minting in `chatAgent` (two call sites — preload and |
| 144 | +sendMessage) now uses `payload.chatId` for the `sessions:` resource. That |
| 145 | +matches what the transport/AgentChat use as the JWT resource and what the |
| 146 | +JWT's resource set in the loader includes. Cross-form addressing works |
| 147 | +either way (smoke-tested), but using `chatId` keeps the chain tight. |
| 148 | + |
| 149 | +`createChatTriggerAction` is the most visibly trimmed: no pre-create, no |
| 150 | +threading `sessionId` into payload, scope mint uses `chatId`. Return type |
| 151 | +no longer carries `sessionId` — note `TriggerChatTaskResult.sessionId` was |
| 152 | +already declared optional, so this isn't a public-API break. |
| 153 | + |
| 154 | +**Stale docstring to flag:** `chat.ts:59` and `chat.ts:112` still describe |
| 155 | +PAT scopes as `read:sessions:{sessionId}` and |
| 156 | +`write:sessions:{sessionId}`. Functionally either ID works (row lookup |
| 157 | +canonicalises), but the doc text is now out of date — it should say |
| 158 | +`{chatId}`. Worth a tidy-up before merge but not blocking. |
| 159 | + |
| 160 | +### 7. `packages/trigger-sdk/src/v3/chat.ts` (+63/-117) |
| 161 | + |
| 162 | +**The biggest mechanical edit.** Net -54 lines from deleting `ensureSession` |
| 163 | +and untangling its callers. |
| 164 | + |
| 165 | +What disappeared: |
| 166 | +- `private async ensureSession(chatId)` — gone |
| 167 | +- The "lazy upsert from the browser if no triggerTask callback" branch in |
| 168 | + `sendMessages` and `preload` — gone |
| 169 | +- The "throw if neither path surfaced a sessionId" guard — gone |
| 170 | +- All `state.sessionId` URL params replaced with `chatId` |
| 171 | +- `subscribeToSessionStream`'s `chatId?` (optional) is now `chatId` (required) |
| 172 | + |
| 173 | +What stayed: |
| 174 | +- `state.sessionId` in `ChatSessionState` — optional, informational |
| 175 | +- The `restore from external storage` branch in the constructor still |
| 176 | + hydrates `sessionId` if persisted, just doesn't *require* it |
| 177 | +- `notifySessionChange` still surfaces `sessionId` if known |
| 178 | + |
| 179 | +**Question to ask:** does the transport ever still need the friendlyId? The |
| 180 | +only place is the `onSessionChange` callback's payload (so consumers |
| 181 | +persisting state can save it for later display). The transport itself never |
| 182 | +puts it in a URL or a waitpoint key. |
| 183 | + |
| 184 | +The `sendMessages` path is worth re-reading: when state.runId is set, it |
| 185 | +appends to `.in/append` and subscribes to `.out`. If the append fails with |
| 186 | +a non-auth error, it falls through to triggering a new run (legacy "run is |
| 187 | +dead" detection — unchanged from pre-Sessions, doesn't depend on |
| 188 | +addressing). |
| 189 | + |
| 190 | +### 8. `packages/trigger-sdk/src/v3/chat-client.ts` (+34/-33) |
| 191 | + |
| 192 | +Server-side `AgentChat`. Mirrors the transport changes — every URL uses |
| 193 | +`this.chatId`. `triggerNewRun` no longer pre-creates a session. `ChatSession` |
| 194 | +and internal `SessionState` types now have optional `sessionId`. |
| 195 | + |
| 196 | +The shape of the diff is identical to the transport: delete the upsert, |
| 197 | +swap addressing identifiers, optionalise the friendlyId. If you've read |
| 198 | +`chat.ts` carefully, this one is mostly mechanical confirmation that both |
| 199 | +client surfaces (browser transport + server-side AgentChat) speak the same |
| 200 | +addressing protocol. |
| 201 | + |
| 202 | +### 9. Test infrastructure — `sessions.ts` (+18) + `mock-chat-agent.ts` (+25) |
| 203 | + |
| 204 | +`__setSessionCreateImplForTests` mirrors the existing |
| 205 | +`__setSessionOpenImplForTests`. `mockChatAgent` installs a no-op create stub |
| 206 | +returning a synthetic `CreatedSessionResponseBody` so the agent's bind-time |
| 207 | +`void sessions.create(...)` doesn't try to hit a real API. Cleanup runs in |
| 208 | +the same `.finally` as the open override. |
| 209 | + |
| 210 | +**Question to ask:** is the synthetic response shape correct? It mirrors |
| 211 | +`CreatedSessionResponseBody` — `id`, `externalId`, `type`, `tags`, |
| 212 | +`metadata`, `closedAt`, `closedReason`, `expiresAt`, `createdAt`, |
| 213 | +`updatedAt`, `isCached`. Tests don't currently assert on this object, so |
| 214 | +the bar is "doesn't crash + matches the type". Met. |
| 215 | + |
| 216 | +### 10. `packages/trigger-sdk/src/v3/chat.test.ts` (+13/-12) |
| 217 | + |
| 218 | +Three classes of test edits, all consequences: |
| 219 | + |
| 220 | +- Stream URL assertion: `chat-1` (the chatId) instead of |
| 221 | + `session_streamurl` (the friendlyId) |
| 222 | +- `renewRunAccessToken` callback: `sessionId: undefined` (was |
| 223 | + `DEFAULT_SESSION_ID` because the mocked trigger doesn't surface it) |
| 224 | +- Token resolve count: `1` (was `2` — second resolve was for `ensureSession`) |
| 225 | +- One `onSessionChange` matchObject loses `sessionId` |
| 226 | + |
| 227 | +### 11. `apps/webapp/app/routes/_app.../playground/.../route.tsx` (1 line) |
| 228 | + |
| 229 | +`sessionId: string` → `sessionId?: string` in the playground sidebar prop |
| 230 | +to track the transport type change. |
| 231 | + |
| 232 | +--- |
| 233 | + |
| 234 | +## Edge cases I checked, so you don't have to |
| 235 | + |
| 236 | +- **Cross-form JWT auth (curl matrix).** JWT scoped to externalId can call |
| 237 | + externalId URL ✓ and friendlyId URL ✓. JWT scoped to friendlyId can call |
| 238 | + externalId URL ✓ and friendlyId URL ✓. Smoke-tested. |
| 239 | +- **Row materialises after subscribe.** Transport opens |
| 240 | + `GET /sessions/{chatId}/out` before agent's bind upsert lands → 200 OK, |
| 241 | + `addressingKey = chatId` (paramSession fallback). Once the row lands |
| 242 | + with `externalId = chatId`, addressingKey resolves to the same value via |
| 243 | + `row.externalId`. Same S2 key throughout. |
| 244 | +- **Concurrent triggers on one chatId.** Two browser tabs trigger two runs |
| 245 | + → two binds → two `sessions.create({externalId: chatId})` calls. Upsert |
| 246 | + semantics: both return the same row. |
| 247 | +- **Closed session enforcement.** Still enforced when a row exists. |
| 248 | + `maybeSession?.closedAt` is null-safe; no row = no close-state to honour. |
| 249 | +- **Agent run cancellation.** Frontend doesn't auto-detect — unchanged from |
| 250 | + pre-Sessions; messages sit in S2 until the next trigger (the existing |
| 251 | + run-PAT auth-error path is the only reaper). Out of scope for this branch. |
| 252 | +- **Idle timeout in dev.** Runs stay `EXECUTING_WITH_WAITPOINTS` past the |
| 253 | + configured idle because dev runs don't snapshot/restore; the in-process |
| 254 | + idle clock advances locally without touching the row. Expected, not a |
| 255 | + regression. |
| 256 | + |
| 257 | +## Things explicitly **not** in this branch |
| 258 | + |
| 259 | +- Run-state subscription on the transport side (the "run died, re-trigger |
| 260 | + silently" UX gap) |
| 261 | +- Session auto-close on agent exit (still client-driven by design) |
| 262 | +- Any change to `Session` schema, `sessions.create` semantics, or |
| 263 | + `chatAccessTokenTTL` |
| 264 | +- Docstring updates for `read:sessions:{sessionId}` / `write:sessions:{sessionId}` |
| 265 | + in `chat.ts:59` and `chat.ts:112` (functional but textually stale — |
| 266 | + follow-up nit) |
| 267 | + |
| 268 | +--- |
| 269 | + |
| 270 | +## What I'd be ready to answer cold |
| 271 | + |
| 272 | +- Why fire-and-forget upsert (vs. `await`) in the agent's bind step |
| 273 | +- Why the route's authorization resource set has three IDs (cross-form JWT |
| 274 | + auth) |
| 275 | +- Why `POST /api/v1/sessions` lost `allowJWT` (security tightening — no |
| 276 | + caller needs it after the transport's `ensureSession` is gone) |
| 277 | +- What converges two callers using different URL forms onto the same S2 |
| 278 | + stream (`canonicalSessionAddressingKey`, identical computation on both |
| 279 | + sides for any given row) |
| 280 | +- What makes `sessions.create` race-safe under concurrent triggers |
| 281 | + (`externalId` upsert) |
| 282 | +- Why `state.sessionId` stayed on `ChatSessionState` at all (pure |
| 283 | + informational, surfaced via `onSessionChange` for consumer persistence; |
| 284 | + zero addressing role) |
| 285 | +- Why the chat-client (server-side AgentChat) and chat (transport) edits |
| 286 | + look near-identical (they implement the same client protocol against the |
| 287 | + same row-agnostic routes) |
0 commit comments