docs: add review notes for PR 9764

CODE_QUALITY.md

@@ -0,0 +1,84 @@
+# Code Quality Review: PR #9764
+
+## Findings
+
+### High: `KiloClawProvider` can create duplicate chat/event clients on repeated initialization
+
+`packages/kilo-vscode/src/kiloclaw/KiloClawProvider.ts:250` handles every `kiloclaw.ready` by bootstrapping new clients, but it does not tear down existing `EventServiceClient`, `KiloChatClient`, event handlers, or timers before calling `openChatStream()`.
+
+- `packages/kilo-vscode/src/kiloclaw/KiloClawProvider.ts:359` assigns new `EventServiceClient` / `KiloChatClient` instances.
+- `packages/kilo-vscode/src/kiloclaw/KiloClawProvider.ts:398` attaches another full set of event handlers.
+- `packages/kilo-vscode/src/kiloclaw/KiloClawProvider.ts:510` stores those handlers in `chatSubs`.
+- `packages/kilo-vscode/src/kiloclaw/KiloClawProvider.ts:1167` only cleans them up during panel disposal / reattach, not before re-bootstrap.
+
+The previous implementation explicitly disconnected the old chat client before connecting a new one. The new provider appears vulnerable to duplicate WebSockets, duplicate message handlers, duplicate broadcasts, and stale subscriptions when the webview posts `kiloclaw.ready` more than once or a user retries after an error. A human should verify all `kiloclaw.ready` call paths, especially retained webview context, retry, and auth-expiry recovery.
+
+### High: Large duplicated chat clients will be difficult to maintain consistently
+
+The PR introduces near-duplicate implementations of the Kilo Chat HTTP client and Event Service WebSocket client in both VS Code and CLI paths:
+
+- `packages/kilo-vscode/src/kiloclaw/event-service-client.ts:1`
+- `packages/opencode/src/kilocode/claw/event-service-client.ts:1`
+- `packages/kilo-vscode/src/kiloclaw/kilo-chat-client.ts:1`
+- `packages/opencode/src/kilocode/claw/kilo-chat-client.ts:1`
+
+These files contain the same connection, reconnect, event dispatch, request, and error-handling logic with only small logging/name differences. This creates a high risk that auth, retry, protocol, payload, or security fixes land in one client and not the other. Since both are Kilo-specific, consider moving shared protocol/client code into a Kilo-owned shared module/package and keeping only UI/runtime adapters in each product.
+
+### Medium: `KiloClawProvider` has become a god object
+
+`packages/kilo-vscode/src/kiloclaw/KiloClawProvider.ts:57` now owns panel lifecycle, backend connection resolution, token management, WebSocket lifecycle, HTTP mutations, optimistic updates, pagination, typing timers, polling, bot nudges, event translation, and webview state projection in one 1,200+ line class.
+
+This makes race conditions and cleanup paths hard to reason about. Several sections already need generation checks, manual stale checks, manual subscription tracking, and duplicated state mutation patterns. Consider extracting focused modules such as connection/bootstrap, event subscription/reducer, mutation handlers, pagination state, and polling/nudge lifecycle.
+
+### Medium: Pagination requests can be duplicated heavily during scroll
+
+Both conversation and message infinite-scroll paths lack an in-flight guard.
+
+- `packages/kilo-vscode/webview-ui/kiloclaw/components/ConversationList.tsx:56` calls `claw.loadMoreConversations()` on every scroll event near the bottom.
+- `packages/kilo-vscode/src/kiloclaw/KiloClawProvider.ts:793` starts a fetch whenever `hasMoreConversations` and `conversationsCursor` are truthy, with no `loadingMoreConversations` flag.
+- `packages/kilo-vscode/webview-ui/kiloclaw/components/MessageArea.tsx:126` calls `claw.loadMoreMessages()` on every scroll event near the top.
+- `packages/kilo-vscode/src/kiloclaw/KiloClawProvider.ts:895` starts a fetch whenever the active conversation matches, with no `loadingMoreMessages` flag.
+
+The stale cursor checks avoid merging some duplicate responses, but they do not prevent duplicate network calls. This can waste API quota, increase latency, and amplify race conditions under fast scrolling or scroll position changes triggered by layout/auto-scroll.
+
+### Medium: External protocol payloads are trusted without runtime validation
+
+The new clients rely on TypeScript casts for network payloads instead of validating external data at the boundary.
+
+- `packages/kilo-vscode/src/kiloclaw/event-service-client.ts:143` accepts a typed handler but wraps an `unknown` payload with `payload as KiloChatEventMap[N]`.
+- `packages/kilo-vscode/src/kiloclaw/kilo-chat-client.ts:240` returns `(await res.json()) as T`.
+- `packages/opencode/src/kilocode/claw/event-service-client.ts:144` has the same event payload cast.
+- `packages/opencode/src/kilocode/claw/kilo-chat-client.ts:231` has the same response cast.
+
+Because these payloads come from remote services, a malformed response can flow into UI reducers and mutation logic as if it were valid. At minimum, validate envelopes and discriminants for WebSocket events and key HTTP response shapes, or centralize schema validation in the shared client mentioned above.
+
+### Medium: Gateway route bypasses the previous upstream credential boundary and reduces error handling
+
+`packages/kilo-gateway/src/server/routes.ts:584` changes `/claw/chat-credentials` from proxying the Kilo API to returning the locally stored Kilo token plus configured worker URLs directly.
+
+- `packages/kilo-gateway/src/server/routes.ts:614` no longer wraps the route in `try` / `catch`, unlike the neighboring `/claw/status` route.
+- `packages/kilo-gateway/src/server/routes.ts:628` returns the token directly.
+- `packages/kilo-gateway/src/api/constants.ts:15` introduces independent chat/event-service URL environment variables.
+
+This may be intentional, but it is an architectural shift worth human verification: the route now owns credential shape, endpoint selection, expiry behavior, and token exposure to local clients. It also no longer maps unexpected failures to a controlled JSON error the way nearby gateway routes do.
+
+### Low: New code frequently diverges from repo style conventions
+
+The repository guidance asks new code to prefer `const`, avoid `else`, avoid broad `any`, and favor short single-word names. This PR adds substantial code that does not consistently follow those conventions.
+
+Examples:
+
+- `packages/kilo-vscode/src/kiloclaw/event-service-client.ts:191` uses mutable `let settled`.
+- `packages/kilo-vscode/webview-ui/kiloclaw/components/MessageArea.tsx:18` / `packages/kilo-vscode/webview-ui/kiloclaw/components/MessageArea.tsx:159` use mutable `let` state outside signals.
+- `packages/opencode/src/kilocode/claw/client.ts:160` / `packages/opencode/src/kilocode/claw/client.ts:161` / `packages/opencode/src/kilocode/claw/client.ts:162` use mutable module-local state in a large closure.
+- `packages/kilo-gateway/src/server/routes.ts:23` continues the broad `any` dependency typing pattern while adding more route surface.
+
+Some mutable state is probably necessary for lifecycle and DOM refs, but the volume of new mutable state makes the code harder to audit and increases the risk of stale closure bugs.
+
+## Suggested Follow-Ups
+
+1. Add an initialization cleanup/reconnect test or manual verification for repeated `kiloclaw.ready` messages.
+2. Extract shared Kilo Chat / Event Service client code to remove VS Code / CLI duplication.
+3. Add in-flight guards for message and conversation pagination.
+4. Add lightweight runtime validation at the remote protocol boundaries.
+5. Revisit the gateway credential route's ownership model and error handling before merging.