Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (20 files)
Fix these issues in Kilo Cloud Reviewed by gpt-5.5-20260423 · 6,103,680 tokens |
iscekic
changed the title
iscekic
requested review from
RSO,
St0rmz1,
eshurakov,
jeanduplessis,
joshavant and
pandemicsyn
| // member could approve exec actions running on the conversation-owner's Fly | ||
| // machine. Gate on session ownership before enabling multi-party. | ||
| // boundary, and KiloClaw currently keeps bot-created approval conversations | ||
| // owner-only by not forwarding additionalMembers. | ||
| authorizeActorAction: () => ({ authorized: true }), |
There was a problem hiding this comment.
The current invariant is accurate today, but this plugin's authorizeActorAction is permissive precisely because the safety property is enforced elsewhere: the human createConversationRequestSchema has no additionalMembers field, createBotConversationFor rejects any additionalMembers, and this plugin no longer forwards them either. If any of those three relax later, any conversation member could approve exec commands running on the conversation owner's Fly machine.
The previous comment captured that hazard for future readers. Suggest adding a sentence alongside the current text along these lines:
If kilo-chat ever supports more than the owner plus the bot in a conversation, gate this on session ownership before relaxing the createConversationRequestSchema or createBotConversationFor constraints.
| @@ -193,6 +187,8 @@ const nativeRuntime: ChannelApprovalNativeRuntimeAdapter< | |||
| }, | |||
|
|
|||
| updateEntry: async ({ entry, payload }) => { | |||
| if (hasResolvedActionsBlock(payload)) return; | |||
There was a problem hiding this comment.
Worth adding a comment here explaining why this returns early: inputActionsBlockSchema declares resolved: z.never().optional(), so any edit or create with a resolved block would return 400 at the kilo-chat HTTP boundary. Skipping the call is mandatory once that schema landed; the transition into the resolved state is owned by /v1/conversations/:id/execute-action.
Without that comment, the skip looks like a defensive guard. It is actually a hard requirement coupled to a sibling schema, and hasResolvedActionsBlock is doing shape inspection on a payload built two functions away. If buildResolvedBlocks ever changes to omit the actions block (the way buildExpiredBlocks already does), the skip would silently stop firing and this code would start producing 400s.
Cleaner alternative if you want to drop the implicit coupling: have buildResolvedResult return an action that the SDK treats as no-op so transport never sees a resolved payload to inspect. If that surface does not exist on the SDK, the inline comment is enough.
| } | ||
| const sandboxId = parsed.targetBotId.slice(botPrefix.length); | ||
| const sandboxId = targetBotId.slice(botPrefix.length); |
There was a problem hiding this comment.
With chatWebhookRpcSchema.parse(payload) removed, the one downstream identifier that the prior schema validated is no longer checked. sandboxId derived from targetBotId.slice('bot:kiloclaw:'.length) then flows into idFromName(...), the registry lookup, the Postgres fallback, and gateway token derivation. The shared sandboxIdSchema already encodes the rule (/^[A-Za-z0-9_-]{1,64}$/).
Suggest one guard immediately after the prefix strip, so the property the parse used to provide for free is preserved without bringing back full payload parsing:
if (!sandboxIdSchema.safeParse(sandboxId).success) {
throw new Error(`Invalid sandboxId derived from targetBotId: ${targetBotId}`);
}Practical impact today is small (Cloudflare DOs accept any string, downstream reads would mostly fail safely), but an empty sandboxId would still take the legacy path and resolve to whatever userIdFromSandboxId('') returns. That state produces confusing labels in registry lookups during incidents.
3 participants
Summary
This PR completes the Stream Chat cutover to first-party Kilo Chat across web, mobile, shared packages, and the Worker services that back KiloClaw chat. It introduces the Kilo Chat service as the canonical chat API, Event Service as the realtime/presence transport, Notifications as the badge/push owner, and KiloClaw runtime/plugin changes needed to run the new chat path.
High-level breaking backwards compatibility changes
POST /connect-ticketfollowed by/connect?ticket=...with thekilo.events.v1subprotocol.lastSeenMessageId, delete-style mutations return JSON instead of204, and strict clients must handle canonicalmessage,conversation, cursor, reply, action, and reaction-operation fields.sandboxId/conversationIdKilo Chat payloads. The old SQLchannel_badge_countstable is dropped and unread badge state moves to per-user Notification DO buckets without a backfill.Web app
/claw/chat/[conversationId]and/organizations/[id]/claw/chat/[conversationId]routes, with guards for missing instances and mismatched sandbox conversations.EventServiceProvider, reusing one Event Service and Kilo Chat client across the app.Compatibility notes:
/claw/chatand/organizations/[id]/claw/chatnow render Kilo Chat instead of the old Stream Chat channel UI./claw/kilo-chatand/claw/kilo-chat/:conversationIdto/claw/chatand/claw/chat/:conversationId, with equivalent organization redirects.GET /api/kiloclaw/chat-credentials,kiloclaw.getStreamChatCredentials,kiloclaw.sendChatMessage,organizations.kiloclaw.getStreamChatCredentials, andorganizations.kiloclaw.sendChatMessage./api/kilo-chat/tokenandkiloChat.getTokennow return{ token, expiresAt, userId }.Mobile app
Compatibility notes:
/(app)/chat/[instance-id]to/(app)/chat/[sandbox-id]and/(app)/chat/[sandbox-id]/[conversation-id]. Existing one-segment chat links now land on the conversation list.instanceId-based chat/lifecycle shapes to shared notifications payloads: chat messages requiretype: "chat.message",sandboxId,conversationId, andmessageId; lifecycle notifications usesandboxId.KILO_CHAT_URL,EVENT_SERVICE_URL, andNOTIFICATIONS_URL.Shared packages and data model
@kilocode/kilo-chat-hooks, a shared React Query hook layer for conversations, messages, bot status, optimistic mutations, event-driven cache updates, mark-read retry state, and pending action state.@kilocode/kilo-chatschemas/types/client contracts for reply snapshots, action resolution state, mark-read results, reaction removal results, execute-action results, canonical create responses, and paginated message pages.replyTo, reactionoperationId, and fullconversation.createddata.@kilocode/event-servicefor connection tickets, ref-counted context subscriptions, KiloClaw event/presence context builders, and retry/unauthorized recovery.@kilocode/notificationswith shared badge bucket helpers, push notification data schemas, and notification RPC schemas.verifyKiloBearerAgainstCurrentPepperin@kilocode/worker-utils/kilo-token-authto validate token environment and current user pepper.channel_badge_countsand generated Drizzle metadata, plus remaining Stream Chat package/patch lockfile wiring.Compatibility notes:
channel_badge_countsandChannelBadgeCountare removed; unread/badge state moves to badge bucket contracts such askiloclaw:<sandboxId>andkiloclaw:<sandboxId>:<conversationId>.markConversationRead(conversationId)now requires{ lastSeenMessageId }and returns{ ok, applied, lastReadAt, badgeClear };removeReactionreturns{ removed, id };executeActionreturns updated message content/resolution.message, create-conversation responses includeconversation, message events includereplyTo, conversation-created events includeconversation, and reaction events includeoperationId.hasMoreandnextCursor.Kilo Chat service
lastReadAt, and notification badge clearing.NOTIFICATIONSservice binding.conversation.read, andreplyTodata onmessage.created.ConversationDO, skips textless message webhooks, and reports/reverts failed message/action delivery.Backwards compatibility:
WORKER_ENVand carry the current user pepper; they do not need a kilo-chat-specific token source.POST /v1/conversations/:id/mark-readnow requires{ lastSeenMessageId }and returns{ ok, applied, lastReadAt, badgeClear }; it can return503if badge clearing fails.200JSON instead of204; reaction delete returns{ removed, id }.dedupe: "fresh"field.typing.stop, andconversation.readis targeted to the relevant user.WORKER_ENV,NOTIFICATIONS, and working Hyperdrive access for auth, ownership, and sandbox labels.Event service
POST /connect-ticketaccepts a bearer Kilo JWT, thenGET /connect?ticket=...consumes the ticket for the WebSocket upgrade.ConnectionTicketDOwith 30-second TTL, consume-once semantics, and alarm cleanup.kilo.events.v1./connect-ticket, includingAuthorization, and allowshttp://localhost:3000for local development.isUserInContext(userId, context), backed byUserSessionDO.hasContext, so notification fanout can suppress pushes for live subscribers.CONNECTION_TICKET_DO,HYPERDRIVE, andWORKER_ENV.Backwards compatibility:
Sec-WebSocket-Protocol: kilo.jwt.<base64url-jwt>must migrate to/connect-ticket,/connect?ticket=..., andkilo.events.v1.CONNECTION_TICKET_DO,HYPERDRIVE,WORKER_ENV, and the new Durable Object migration before the ticket flow works./healthremains public./connectstill requires WebSocket upgrade but now rejects missing/invalid tickets instead of missing/invalid JWT subprotocols./connect-ticketis public but bearer-authenticated.Notifications service
/v1/badgesand/v1/badges/mark-readroutes for mobile badge hydration and read clearing.channel_badge_countsinto per-userNotificationChannelDObucket storage.sandboxId.NEXTAUTH_SECRET,WORKER_ENV, andEVENT_SERVICE.Backwards compatibility:
/webhooks/stream-chatis removed; existing Stream Chat webhook delivery must be disabled or migrated to the new RPC producers./v1/*badge routes requireAuthorization: Bearer <Kilo JWT>and return{ buckets }or{ badgeCount }; no unauthenticated badge access is supported.sendInstanceLifecycleNotificationno longer usesinstanceIdin request/push data and now returnsticketErrors.type: "chat.message"withsandboxId,conversationId, andmessageId; lifecycle data usessandboxId.{ ticketTokenPairs }.NEXTAUTH_SECRET,WORKER_ENV,EVENT_SERVICE,EXPO_ACCESS_TOKEN,HYPERDRIVE,RECEIPTS_QUEUE, andNOTIFICATION_CHANNEL_DO;STREAM_CHAT_API_SECRETis no longer required.KiloClaw service and runtime
/stream-chat-*HTTP routes.openclaw.jsonbeforeopenclaw doctor, deletingchannels.streamchat, the legacy plugin path, allow entry, and plugin entry.channels["kilo-chat"],/usr/local/lib/node_modules/@kiloclaw/kilo-chat, andkilo-chatin an existing plugin allowlist.additionalMembersfrom bot-created conversations, returns pagination from read actions, and avoids sending resolved approval action blocks through edit payloads.targetBotId=bot:kiloclaw:<sandboxId>, stripstargetBotId, and forwards the webhook body to/plugins/kilo-chat/webhook.image-content-hash.sh,FLY_IMAGE_CONTENT_MODE, local OpenClaw tarball mode, and CI/dev handoff checks.@kilocode/notificationstypes, sendsandboxIdwithoutinstanceId, and log ticket-error counts as warnings.Backwards compatibility:
/api/kiloclaw/chat-credentials,/api/platform/stream-chat-credentials,/api/platform/send-chat-message, or stored Stream Chat credentials must migrate to Kilo Chat.doctor; Kilo Chat is added without creatingplugins.allowwhen it was absent, preserving permissive plugin loading.plugins.allowconfigs must includekilo-chat; this code appends it when the allowlist already exists.FLY_IMAGE_CONTENT_MODEdefaults to production. Local-image dev flows need exactly oneopenclaw-build/openclaw-*.tgzor an explicit production/local mode.instanceId; it is compatible with the updated shared binding becausesandboxIdis now the lifecycle identifier. MissingNOTIFICATIONSstill no-ops.KILOCLAW_SANDBOX_IDandKILOCHAT_BASE_URL; workerKILO_CHATcleanup andNOTIFICATIONSremain optional/failure-tolerant.targetBotIdis used only for routing, then stripped; plugin-side validation still accepts historicalmessage.createdpayloads without atypefield.message,conversation,hasMore,nextCursor) and no longer allow bot-createdadditionalMembers.Dev, deployment, and local tooling
Verification
Visual Changes
Reviewer Notes