Repository navigation
feat(host-service,mobile): host sessions sync v1 — canonical sessions over tRPC snapshots + /sessions/sync - #5940
saddlepaddle wants to merge 8 commits into
Conversation
… over tRPC snapshots + /sessions/sync, mobile rewired onto the sync client
Conflict resolution: - apps/mobile/(authenticated)/_layout.tsx: keep deletion of chat/acp route - host-service drizzle: keep main's 0010/0011, regenerate session_meta as 0012 Claude-Session: https://claude.ai/code/session_01XFhc7RWVyTo1VYgQKzq6GJ
…l sessions surface Claude-Session: https://claude.ai/code/session_01XFhc7RWVyTo1VYgQKzq6GJ
…ings - bound each session's in-memory event log at 5k events (ring with serial offset; eviction resets subscribers via the existing cold path and surfaces truncatedBefore in scrollback) - skip the session-meta orphan sweep when registry hydration failed so a transient read error cannot wipe titles/archive flags - park evicted dead runtimes as offline records so sessions stay addressable and resurrectable - reject submitTurn on closed sessions before resurrecting the adapter - disarm settings/permission causation attribution when the port call fails; validate settings batches before the first mutation - reap a finished turn's tool calls in the shared reducer so partial windows cannot leak active entries - rebuild the timeline's message→turn index on incremental folds so any fold granularity marks the same user message failed - log dropped frames and projection fold failures instead of recording them silently; measure sync frames in UTF-8 bytes; dispose guard on in-flight resurrection; turn-id caps use their own constant Claude-Session: https://claude.ai/code/session_01XFhc7RWVyTo1VYgQKzq6GJ
…artbeat, breaker semantics - retry failed host/session snapshot cold paths with epoch-guarded backoff instead of parking forever on one transient tRPC failure - keep retain intent across host-snapshot prunes and seed retained sessions when their upsert lands, so a session created during the cold path still gets a stream - ping/pong heartbeat + hello deadline so half-open sockets close into the reconnect path instead of reporting live forever - clear the reset breaker on socket close (documented 'cleared on reconnect' semantic) and park on the Nth reset as documented - late session snapshots resolving after socket loss downgrade to idle Claude-Session: https://claude.ai/code/session_01XFhc7RWVyTo1VYgQKzq6GJ
… scroll affordance - settled permission asks stay dismissed (retry-forever NOT_FOUND no longer restores an unanswerable card); other failures restore the card and surface in the thread error banner - a failed send restores the typed message into the composer - scroll-to-bottom button shows whenever the reader is not pinned at the true end (two-sided distance test mirroring the re-pin rule) - scrollToBottom cancels in-flight anchor legs; lineDiffStats memoized Claude-Session: https://claude.ai/code/session_01XFhc7RWVyTo1VYgQKzq6GJ
|
Too many files changed for review. ( Bypass the limit by tagging |
📝 WalkthroughWalkthroughThis PR introduces the canonical host sessions protocol, synchronized WebSocket client state, timeline folding, durable session metadata, and mobile chat migration from ACP routes to standard session routes. It also adds conversation scroll regression coverage, host integration tests, and protocol rollout documentation. ChangesCanonical host sessions
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Mobile
participant SyncClient
participant SyncHub
participant HostRuntime
Mobile->>SyncClient: Connect and retain session
SyncClient->>SyncHub: Send hello and subscribe
SyncHub->>HostRuntime: Request snapshot and replay
HostRuntime-->>SyncHub: Return session events
SyncHub-->>SyncClient: Send replay and caughtUp
HostRuntime-->>SyncHub: Emit live events
SyncHub-->>SyncClient: Deliver session updates
Mobile->>SyncClient: Submit turn or resolve permission
SyncClient->>HostRuntime: Call sessions procedure
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/mobile/screens/(authenticated)/(home)/home/HomeScreen.tsx (1)
257-263: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefresh sessions from pull-to-refresh.
refreshHostDatais used by focus handling, butonRefreshstill refetches only workspaces. After moving sessions touseHostSessions, pull-to-refresh can leave session rows stale until the periodic refetch or next focus. Refetch or invalidate the same sessions query inonRefreshand await it before clearingrefreshing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/screens/`(authenticated)/(home)/home/HomeScreen.tsx around lines 257 - 263, Update the pull-to-refresh handler on HomeScreen, specifically onRefresh, to invalidate or refetch the sessions query using the same ["sessions", "list"] key as refreshHostData. Await the sessions refresh alongside the workspace refresh before clearing the refreshing state, while preserving the existing pull-to-refresh behavior.
🧹 Nitpick comments (25)
apps/mobile/screens/(authenticated)/workspace/[id]/hooks/useWorkspaceHeaderActions/useWorkspaceHeaderActions.ts (1)
36-38: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
getSessionsQueryKeyinstead of the literal key.
FinishReviewSheet(same PR) invalidates viagetSessionsQueryKey(host.machineId); this hard-codes["sessions", "list"]. It works today only because it happens to be a prefix of the generated key — any change to the helper's shape silently breaks this invalidation and the new session won't appear in the list.♻️ Proposed fix
+import { + getSessionsQueryKey, +} from "`@/screens/`(authenticated)/(home)/home/hooks/useHostSessions";void queryClient.invalidateQueries({ - queryKey: ["sessions", "list"], + queryKey: getSessionsQueryKey(host.machineId), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/screens/`(authenticated)/workspace/[id]/hooks/useWorkspaceHeaderActions/useWorkspaceHeaderActions.ts around lines 36 - 38, Update the invalidateQueries call in useWorkspaceHeaderActions to use the shared getSessionsQueryKey helper with the relevant host.machineId instead of the literal ["sessions", "list"] key, matching FinishReviewSheet and preserving generated query-key consistency.apps/mobile/screens/(authenticated)/workspace/[id]/finish-review/FinishReviewSheet.tsx (1)
9-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
useHostSessionsnow has consumers outside(home)/home— promote it.
SessionThreadScreen,WorkspaceScreen, and this sheet all import it (plusgetSessionsQueryKey) from(home)/home/hooks/, so the hook should move to the highest shared parent (e.g.screens/(authenticated)/hooks/useHostSessions/) rather than being reached into across features.As per coding guidelines: "promote components used two or more times to the highest shared parent, and keep utils, hooks, constants, config, tests, and stories next to their consumers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/screens/`(authenticated)/workspace/[id]/finish-review/FinishReviewSheet.tsx around lines 9 - 14, Promote useHostSessions and getSessionsQueryKey from the `(home)/home` hooks location to the shared authenticated hooks area, then update SessionThreadScreen, WorkspaceScreen, and FinishReviewSheet imports to use the new location. Keep the hook and query-key exports unchanged.Source: Coding guidelines
apps/mobile/components/ai-elements/conversation.tsx (3)
794-821: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProps the component silently overrides should be removed from the public type.
ConversationPropsonly omitschildren | data | renderItem, so callers may legally passcontentInset,onLayout,onContentSizeChange,onMomentumScrollEnd,onScrollBeginDrag,onScrollEndDrag— all of which are spread first and then clobbered here.onScrollis the only one chained. Widening theOmitmakes the scroll-discipline ownership explicit at the type level instead of failing silently at runtime.♻️ Suggested type tightening
-export type ConversationProps<ItemT> = Omit< - LegendListProps<ItemT>, - "children" | "data" | "renderItem" -> & { +export type ConversationProps<ItemT> = Omit< + LegendListProps<ItemT>, + | "children" + | "contentInset" + | "data" + | "onContentSizeChange" + | "onLayout" + | "onMomentumScrollEnd" + | "onScrollBeginDrag" + | "onScrollEndDrag" + | "renderItem" +> & {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/components/ai-elements/conversation.tsx` around lines 794 - 821, Update the ConversationProps type to omit contentInset, onLayout, onContentSizeChange, onMomentumScrollEnd, onScrollBeginDrag, and onScrollEndDrag from the underlying list props, alongside the existing children, data, and renderItem omissions. Keep onScroll available because it is explicitly chained rather than overridden in the LegendList configuration.
762-766: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer destructuring
keyExtractorover castinglistProps.Pulling
keyExtractorout of the props signature removes the structural cast and keeps the anchor-by-key path type-checked againstItemT; pass it back explicitly toLegendList.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/components/ai-elements/conversation.tsx` around lines 762 - 766, Update the surrounding component logic to destructure keyExtractor from the props instead of casting listProps to access it. Store the destructured keyExtractor in keyExtractorRef, and pass it explicitly to LegendList so the anchor-by-key path remains type-checked against ItemT.
228-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
setPinnedBothnow sets only one thing.The "Both" suffix is leftover from when pinning lived in state and a ref; it now writes
pinnedRefonly. Renaming tosetPinnedkeeps the call sites honest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/components/ai-elements/conversation.tsx` around lines 228 - 230, Rename the setPinnedBoth callback to setPinned and update every reference to it, preserving its current behavior of assigning the value to pinnedRef.current.apps/mobile/screens/(authenticated)/debug/conversation-lab/ConversationLabScreen.tsx (1)
321-324: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAnchor index relies on
itemsRefbeing stale.
appendUserMessagequeues asetItems, soitemsRef.current.lengthread on the next line is the pre-append length — which happens to equal the new item's index. Correct today, but it silently becomes off-by-one if the append is ever flushed synchronously. Resolving the index after the wait makes the intent explicit.♻️ Suggested hardening
const id = appendUserMessage(); - const index = itemsRef.current.length; // the item lands at this index await wait(60); + const index = itemsRef.current.findIndex((item) => item.id === id); + if (index < 0) return "FAIL(appended row missing from data)"; controller.scrollToAnchor(index);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/screens/`(authenticated)/debug/conversation-lab/ConversationLabScreen.tsx around lines 321 - 324, Update the append-and-scroll flow around appendUserMessage so the anchor index is calculated after the wait from the current itemsRef state, ensuring it targets the newly appended item even if the state update flushes synchronously.apps/mobile/screens/(authenticated)/workspace/[id]/chat/[sessionId]/components/SessionThread/SessionThread.tsx (2)
217-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the Android branch on
behavior.
apps/mobileis iOS-only, soPlatform.OS === "ios" ? "padding" : undefinedis a dead fallback.As per coding guidelines: "
apps/mobileis iOS-only; do not add Android fallbacks or platform guards for iOS-only APIs".♻️ Proposed simplification
<KeyboardAvoidingView className="bg-background flex-1" - behavior={Platform.OS === "ios" ? "padding" : undefined} + behavior="padding" keyboardVerticalOffset={0} >(then drop the now-unused
Platformimport)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/components/SessionThread/SessionThread.tsx around lines 217 - 221, Update the KeyboardAvoidingView in SessionThread to set behavior directly to "padding" without the Platform.OS conditional, and remove the now-unused Platform import.Source: Coding guidelines
152-160: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGuard
loadOlderagainst re-entrancy.
onStartReachedcan fire again while a page is in flight (it stays wired as long ashasOlder), issuing duplicatefetchOlderEventscalls and letting the firstfinallyclear the indicator while a second request is still running. A ref guard keeps one page in flight at a time.♻️ Proposed guard
+ const loadingOlderRef = useRef(false); const loadOlder = useCallback(() => { + if (loadingOlderRef.current) return; + loadingOlderRef.current = true; setIsLoadingOlder(true); client .fetchOlderEvents(sessionId) .catch(() => { // Scrollback failures are retried by scrolling again; no banner. }) - .finally(() => setIsLoadingOlder(false)); + .finally(() => { + loadingOlderRef.current = false; + setIsLoadingOlder(false); + }); }, [client, sessionId]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/components/SessionThread/SessionThread.tsx around lines 152 - 160, Update loadOlder to use a ref-based in-flight guard, returning immediately when a page request is already active. Set the guard before calling client.fetchOlderEvents(sessionId), and clear it in the request’s finally handler alongside setIsLoadingOlder(false), so only one fetch runs at a time.apps/mobile/screens/(authenticated)/workspace/[id]/chat/[sessionId]/SessionThreadScreen.tsx (2)
21-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHeader title is set twice; this lookup is likely dead once the thread mounts.
SessionThreadalso renders<Stack.Screen options={{ headerTitle: () => <GlassHeaderTitle title={title} /> }} />from the synceduseSession(sessionId), which will win over theStack.Titlebuilt here. Keeping the pre-mountuseHostSessions(host)lookup only for the loading-state title costs a second per-host workspace/session query set on this screen. Consider limiting the local title to the!routingKeygate (or dropping it and lettingSessionThreadown the header) so there is one source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/SessionThreadScreen.tsx around lines 21 - 27, Remove the redundant session lookup via useHostSessions and the derived session title from SessionThreadScreen, allowing SessionThread’s useSession(sessionId)-based header title to be the sole source of truth. If a loading-state title is required, restrict the lookup and title usage to the !routingKey path only.
5-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
useHostSessionsis now shared across two screen trees — consider promoting it.The hook lives under
screens/(authenticated)/(home)/home/hooks/useHostSessionsbut is consumed here from the workspace chat tree. Per the co-location rule, hooks used by two or more consumers belong at the highest shared parent (e.g.screens/(authenticated)/hooks/useHostSessions).As per coding guidelines: "Co-locate components and their dependencies by usage: nest single-use components under the parent, promote components used two or more times to the highest shared parent, and keep utils, hooks, constants, config, tests, and stories next to their consumers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/SessionThreadScreen.tsx around lines 5 - 6, Promote useHostSessions from the home screen subtree to the highest shared authenticated screens hooks directory, then update all imports and references to use the new location while preserving the hook’s implementation and behavior.Source: Coding guidelines
apps/mobile/screens/(authenticated)/workspace/[id]/chat/[sessionId]/components/SessionThread/components/PermissionStack/PermissionStack.tsx (1)
18-22: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winShare the settled-permission prefix between host and client.
resolvePermissioncurrently throwsNo pending permission: ${input.permissionId}, soisAlreadySettledmatches the current wording, but keeping that prefix behind a shared symbol would prevent the already-settled sentinel from drifting into a retryable error if it is rewritten.
[maintenance_and_code_quality]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/components/SessionThread/components/PermissionStack/PermissionStack.tsx around lines 18 - 22, Define or reuse a shared constant for the “No pending permission” prefix and update isAlreadySettled to match against that symbol instead of an inline string. Ensure resolvePermission also uses the same shared prefix when constructing its error so the settled-permission sentinel remains consistent between host and client.packages/host-service-sync/src/projection/reduceProjection.ts (1)
31-144: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReducer never enforces the caps declared on
sessionProjectionSchema.
reduceProjectionreturns a plain object without ever callingsessionProjectionSchema.safeParse/.parse, so the entry caps defined inprojection.ts(10_000threads/tool calls,1_000turns/permissions) are only checked when something explicitly validates the projection. The cross-fileappendEventcall site incanonical-sessions.tsassignsreduceProjection(...)straight totracked.projectionwith no such validation, so a pathological event stream (e.g. terminal events dropped) could grow these records unbounded in memory with no backpressure.Please confirm whether cap enforcement is intentionally deferred to another layer (e.g. host-side eviction) not included in this batch, or whether the reducer/its caller should validate against
sessionProjectionSchemaperiodically.🛡️ Example: validate before returning
export function reduceProjection( current: SessionProjection | null, input: ProjectionInput, ): SessionProjection { ... - return next; + return sessionProjectionSchema.parse(next); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service-sync/src/projection/reduceProjection.ts` around lines 31 - 144, Enforce the declared projection caps in reduceProjection by validating every returned SessionProjection, including snapshot inputs and the reduced next projection, with sessionProjectionSchema before returning. Preserve the existing session/event mismatch checks and state transitions, and allow schema validation failures to propagate rather than storing an unchecked projection.packages/host-service-sync/src/protocol/sync.ts (1)
103-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated stream/
sessionIdinvariant.The identical
superRefinebody appears four times (subscribed,caughtUp,unsubscribed,reset). A shared helper keeps the invariant from drifting as packets evolve.♻️ Suggested helper
const refineStreamSessionId = ( packet: { stream: "host" | "session"; sessionId: string | null }, context: z.RefinementCtx, ) => { if ( (packet.stream === "host" && packet.sessionId !== null) || (packet.stream === "session" && packet.sessionId === null) ) { context.addIssue({ code: "custom", path: ["sessionId"], message: "sessionId presence does not match stream", }); } };Then each packet becomes
.superRefine(refineStreamSessionId).Also applies to: 204-215, 226-237, 253-264
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service-sync/src/protocol/sync.ts` around lines 103 - 114, Extract the duplicated stream/sessionId validation into a shared refineStreamSessionId helper near the packet schemas, preserving its existing z.RefinementCtx issue and message. Replace the four identical superRefine callbacks on subscribed, caughtUp, unsubscribed, and reset with .superRefine(refineStreamSessionId).packages/host-client/src/sessions/sessions.ts (1)
83-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the sync path from
SESSIONS_SYNC_PATHinstead of re-spelling it.
packages/host-service-sync/src/protocol/routes.ts(line 6) publishesSESSIONS_SYNC_PATH = "/sessions/sync"precisely so client and host can't drift; this hardcodes a second, slash-stripped copy. The package is already imported here.♻️ Proposed change
+import { SESSIONS_SYNC_PATH } from "`@superset/host-service-sync/protocol`";syncUrl: (routingKey) => - transport.streamUrl({ routingKey, path: "sessions/sync" }), + transport.streamUrl({ + routingKey, + path: SESSIONS_SYNC_PATH.replace(/^\//, ""), + }),Confirm whether
transport.streamUrlexpects a leading slash before applying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-client/src/sessions/sessions.ts` around lines 83 - 84, Update the syncUrl callback to derive its path from the imported SESSIONS_SYNC_PATH constant instead of hardcoding "sessions/sync"; confirm transport.streamUrl’s path format and strip or preserve the leading slash as required while keeping the resulting URL correct.packages/host-service-sync/src/protocol/api.ts (1)
314-341: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEvent arrays are validated only at their endpoints, never as an ordered sequence. Both projections carry cursor-ordered event lists, but the refinements only pin the first/last element, so an interleaved middle (e.g. cursors
c3, c1, c9) validates cleanly and downstream paging anchors and render order inherit the disorder.
packages/host-service-sync/src/protocol/api.ts#L314-L341: assertrecentEventscursors strictly ascend while iterating for the sessionId/duplicate checks;applySessionSnapshot(packages/host-service-sync/src/client/createSessionsSyncClient/state.ts, lines 515-584) treatsrecentEvents[0].cursorasoldestCursorand array order as event order.packages/host-service-sync/src/protocol/api.ts#L427-L452: apply the same strictly-ascending cursor assertion towindow.itemssorange.oldest/range.newestgenuinely bracket a monotonic page.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service-sync/src/protocol/api.ts` around lines 314 - 341, Validate cursor ordering in both event-array refinement sites: packages/host-service-sync/src/protocol/api.ts lines 314-341 and 427-452. While iterating snapshot.recentEvents and window.items, track each prior cursor and add a validation issue when the current cursor is not strictly greater; preserve the existing session, duplicate-ID, and endpoint checks.packages/host-service-sync/src/client/createSessionsSyncClient/state.ts (1)
324-374: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winTotal-byte eviction clones the whole event map per dropped event.
Each loop iteration drops exactly one event but re-sorts all candidate streams and clones
candidateStream.eventsByIdplusstreamsBySessionId. OncemaxTotalEstimatedEventBytesis reached, that's a full map clone (up tomaxEventsPerSessionkeys) on every live append, and dropping a large backlog is quadratic. Batching the drops for a chosen candidate — mutate one local copy until the stream is under budget or empty, then commit — keeps the same semantics at linear cost.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service-sync/src/client/createSessionsSyncClient/state.ts` around lines 324 - 374, The total-byte eviction loop in the stream state update performs full map and stream-state copies for every dropped event, causing quadratic work. Refactor the eviction logic to batch removals per selected candidate: create one local copy of the candidate event map and remove events repeatedly until the budget is satisfied or the stream is empty, accumulating bytes and counts, then commit the updated stream and total state once per candidate while preserving retention ordering and eviction metadata.packages/host-service/src/runtime/sessions/translate-acp.ts (1)
596-600: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
pendingPermissionsis the one unbounded tracked collection.
toolCalls,subagentThreads, andmintedTurnIdsall go through explicit caps (trackToolCall,trackSubagentThread, Line 736-739), butpendingPermissionsgrows without a bound. Entries only leave on apermission_resolvedframe (Line 628), so a card the adapter never resolves — adapter death mid-permission, journal gap — is retained for the whole incarnation. A matching cap would make the retention story uniform.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/runtime/sessions/translate-acp.ts` around lines 596 - 600, Bound the pendingPermissions collection in the permission-tracking flow around pendingPermissions.set, adding an explicit cap and eviction behavior consistent with trackToolCall, trackSubagentThread, and mintedTurnIds. Preserve removal on permission_resolved while ensuring unresolved requests cannot retain entries indefinitely.packages/host-service/src/runtime/sessions/canonical-sessions.ts (3)
834-852: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
port.listpagination loop.
hostSnapshotDataandsweepOrphanedSessionMetarun byte-identical do/while pagination. Extracting one private generator keeps the termination condition (and any future safety bound on a port that never returns a nullnextCursor) in a single place.♻️ Suggested helper
private *allSessionStates(): Generator<SessionScopedState> { let cursor: string | undefined; do { const page = this.port.list({ cursor, limit: DEFAULT_PAGE_LIMIT }); yield* page.items; cursor = page.nextCursor ?? undefined; } while (cursor !== undefined); }Also applies to: 868-876
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/runtime/sessions/canonical-sessions.ts` around lines 834 - 852, Extract the duplicated port.list pagination logic from hostSnapshotData and sweepOrphanedSessionMeta into a private allSessionStates generator. Have it yield each SessionScopedState while preserving DEFAULT_PAGE_LIMIT, cursor updates, and termination when nextCursor is undefined; update both callers to consume the generator and retain their existing filtering and session-processing behavior.
1100-1103: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRing eviction via
shift()is O(n) per append past the bound.Once a session log reaches
MAX_SESSION_LOG_EVENTS(5,000), every subsequent event pays an arrayshift(). SinceevictedCountalready gives you the serial→index mapping, a head-index ring (or chunked drop, e.g. splice the oldest 10% when the bound is hit) removes the per-event memmove from the fan-out path on chatty sessions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/runtime/sessions/canonical-sessions.ts` around lines 1100 - 1103, Replace the per-event Array.shift eviction in the session event tracking logic with an O(1) head-index ring approach, using evictedCount to map logical serials to the retained event positions. Preserve the MAX_SESSION_LOG_EVENTS retention bound and existing event ordering/lookup behavior, and update any affected access or serialization logic consistently.
1079-1080: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose or remove
TrackedSession.foldError.
foldErroris declared, initialized tonull, and set on translate/fold failures, but it is not read anywhere in the repo. If this field serves debugging/Observability, expose it through a diagnostics accessor/session error path; otherwise remove it so a future failure cannot be silently swallowed behind onlyconsole.warn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/runtime/sessions/canonical-sessions.ts` around lines 1079 - 1080, Handle the unused TrackedSession.foldError field consistently: either expose it through the existing diagnostics or session-error accessor used by consumers, or remove its declaration, null initialization, and assignment in the translate/fold failure path. Do not leave failures stored only in the unconsumed field and console.warn.packages/host-service/src/trpc/router/sessions/sessions.ts (1)
114-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFold
rethrowMappedinto the gate middleware to drop nine identical try/catch blocks.Every gated procedure repeats the same wrapper. A middleware that awaits
next()and maps thrown domain errors gives identical behavior in one place, and future procedures can't forget it.♻️ Sketch
-const gatedProcedure = protectedProcedure.use(({ ctx, next }) => { +const gatedProcedure = protectedProcedure.use(async ({ ctx, next }) => { if (!ctx.runtime.acpSessionsEnabled) { throw new TRPCError({ code: "PRECONDITION_FAILED", message: "Host sessions are disabled on this host (requires a canary build of the desktop app)", }); } - return next(); + try { + return await next(); + } catch (error) { + rethrowMapped(error); + } });Then each procedure body becomes a bare
ctx.runtime.sessions.<call>(input). Note tRPC middlewares surface handler failures through the returned result rather than only as throws in some versions, so confirm the mapping actually fires before dropping the per-procedure catches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/trpc/router/sessions/sessions.ts` around lines 114 - 206, Move the shared rethrowMapped error handling into gatedProcedure middleware that awaits next() and maps domain errors, then remove the duplicated try/catch wrappers from create, get, update, getEvents, submitTurn, cancelTurn, resolvePermission, and resolveToolCall. Verify middleware mapping handles handler failures for the project’s tRPC version before relying on bare session calls.packages/host-service/src/runtime/acp-sessions/acp-sessions.ts (1)
919-931: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParking evicted dead runtimes in
offlinecontradicts two nearby invariant comments.The
offlinefield doc (Line 208-211) says entries are "Seeded once at construction; entries leave only by successful resurrection", andensureLive's doc (Line 271-273) says "dead sessions stay dead within a host lifetime … and only become resurrectable after a restart turns them offline". Both are now false: eviction re-populatesofflinemid-lifetime and makes a graveyard-evicted session resurrectable without a restart. Worth updating those comments so the lifecycle stays readable — the resurrect path is subtle enough that a stale invariant will mislead the next change.Note also that
offlineStatedropslastError, so the crash reason that killed an evicted session becomes unreachable viaget(); if that diagnostic matters, consider carrying it into the parked record.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/runtime/acp-sessions/acp-sessions.ts` around lines 919 - 931, Update the lifecycle invariant comments for the offline field and ensureLive to reflect that evicted dead runtimes are parked in offline and can be resurrected during the same host lifetime. Also preserve the eviction diagnostic by carrying runtime.state.lastError into the parked record used by get().packages/host-service/src/runtime/sessions/sync-route.ts (1)
49-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
onErrorsilently discards the WebSocket error.
onErrordisposes the connection but never logs the underlying error, so a production socket failure leaves no trace to debug from.🔍 Sketch
- onError: () => { + onError: (event) => { + console.error("[sessions-sync] websocket error", event); connection?.dispose(); connection = null; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/runtime/sessions/sync-route.ts` around lines 49 - 52, Update the WebSocket onError handler to accept the underlying error and log its details before disposing the connection. Preserve the existing connection?.dispose() and connection = null cleanup behavior, using the surrounding session or route logger.packages/host-service/src/runtime/sessions/sync-hub.ts (1)
656-679: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
hostRing.shift()is O(n) on every event once the ring is full.Once
hostRing.lengthreacheshostRingLimit(10,000 by default), every subsequentfanOutHostChangecall pays an O(n)Array.shift()to evict the oldest entry — this runs on the hot fan-out path for the lifetime of a long-running host process. A head-index-based ring buffer (or a small deque) evicts in O(1) without shifting the remaining elements.♻️ Sketch: avoid the O(n) shift
- private readonly hostRing: HostRingEntry[] = []; + private readonly hostRing: HostRingEntry[] = []; + private hostRingStart = 0; ... private fanOutHostChange(change: HostChange): void { this.hostSerial += 1; const entry: HostRingEntry = { ... }; this.hostRing.push(entry); - if (this.hostRing.length > this.hostRingLimit) this.hostRing.shift(); + if (this.hostRing.length - this.hostRingStart > this.hostRingLimit) { + this.hostRingStart += 1; + // periodically compact, e.g. when hostRingStart grows large + if (this.hostRingStart > this.hostRingLimit) { + this.hostRing.splice(0, this.hostRingStart); + this.hostRingStart = 0; + } + } ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/runtime/sessions/sync-hub.ts` around lines 656 - 679, Replace the array-shift eviction in fanOutHostChange with an O(1) head-index ring buffer or deque for hostRing, updating related iteration and capacity logic to preserve insertion order and retain only hostRingLimit entries. Keep event fan-out behavior unchanged while ensuring full-ring eviction does not move all remaining elements.packages/host-service/src/runtime/sessions/testing/sync-harness.ts (1)
16-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSchema violations here get silently reinterpreted as transport failures by the hub.
send()throws synchronously on asyncServerPacketSchemamismatch, butSessionsSyncHub.send()wrapssocket.send()in a catch-all that treats any exception as a torn-socket send failure — it callsdropConnectionand returnsfalserather than letting the ZodError propagate. A future hub bug that emits a non-conforming packet would surface only as an unexpectedly closed connection in tests, not as a schema-validation failure — defeating the "doubles as a wire-conformance test" guarantee this docstring claims (lines 11-15).Consider recording parse failures on the fake instead of throwing inline, so tests can assert on them explicitly regardless of how the hub reacts to
send()exceptions:♻️ Sketch: decouple schema violations from the send-failure path
export class FakeSyncSocket { readyState = 1; bufferedAmount = 0; closed: { code?: number; reason?: string } | null = null; readonly packets: SyncServerPacket[] = []; + readonly invalidPackets: unknown[] = []; send = (data: string) => { - this.packets.push(syncServerPacketSchema.parse(JSON.parse(data))); + const raw = JSON.parse(data); + const parsed = syncServerPacketSchema.safeParse(raw); + if (!parsed.success) { + this.invalidPackets.push({ raw, issues: parsed.error.issues }); + return; + } + this.packets.push(parsed.data); };Then add an assertion helper (or an
afterEach) that fails the suite ifinvalidPacketsis non-empty, independent of connection-drop behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/runtime/sessions/testing/sync-harness.ts` around lines 16 - 36, Update FakeSyncSocket.send to record syncServerPacketSchema parse failures in an invalidPackets collection instead of throwing synchronously, while preserving valid packet recording in packets. Add an assertion helper or afterEach mechanism for tests using FakeSyncSocket that fails when invalidPackets is non-empty, so schema violations remain independently observable regardless of SessionsSyncHub.send connection handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/mobile/screens/`(authenticated)/debug/conversation-lab/ConversationLabScreen.tsx:
- Around line 22-27: Update the doc comment near ConversationLabScreen’s
regression-suite description to reference `.maestro/flows/conversation-lab.yml`
instead of the stale `.maestro/conversation-lab.yaml` path, preserving the
existing guidance and assertions.
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/components/SessionThread/components/TypingIndicator/TypingIndicator.tsx:
- Around line 256-261: Update the TypingIndicator component so its “Agent is
working” status is exposed to screen readers by marking the containing View as
accessible while retaining its accessibilityLabel. Use useReducedMotion() to
disable the indefinite flock animation and render a static dot cluster when
reduced motion is enabled; preserve the existing animation for normal motion.
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/components/SessionThread/SessionThread.tsx:
- Around line 88-109: Update handleSend in SessionThread so a null mainThreadId
rejects or otherwise blocks submission instead of returning successfully.
Preserve the existing error-restoration path in Composer.handleSubmit, ensuring
the typed prompt is retained and the action error is surfaced when no thread ID
is available.
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/components/SessionThread/utils/getSessionThreadPresentation/getSessionThreadPresentation.ts:
- Around line 25-33: Update canCompose in getSessionThreadPresentation to also
require the session to be loaded and error-free: exclude states represented by
isLoading and errorText, while preserving the existing runState checks for idle,
running, and cancelling. Ensure the composer remains hidden during
subscription/loading and whenever a load or stream error is present.
In
`@packages/host-service-sync/src/client/createSessionsSyncClient/createSessionsSyncClient.ts`:
- Around line 1040-1052: Reset reconnectAttempts at the start of connect()
before calling openConnection(), so an explicit reconnect begins as a fresh
connection, reports “connecting,” and uses the initial backoff delay on failure.
Preserve the existing stopped guard and stream/window setup.
In `@packages/host-service-sync/src/protocol/primitives.ts`:
- Around line 28-38: Update the JSON object schema around the superRefine
callback to enforce MAX_JSON_OBJECT_KEYS with a supported record/map key length
constraint, such as the schema’s length validation. If the existing schema
cannot use that constraint, retain the custom check but emit a valid Zod issue
payload rather than using too_big with origin object.
In `@packages/host-service-sync/src/timeline/timeline.ts`:
- Around line 228-241: Clear state.lastError at the start of each turn in the
turnStarted case before applying the active-turn logic, so a subsequent
successful turn removes any prior failure banner. Keep the existing main-thread
activeTurnId behavior unchanged.
In `@packages/host-service/src/runtime/sessions/canonical-sessions.ts`:
- Around line 628-632: Add a translator method corresponding to
clearSettingsCausation for clearing pending next-turn attribution, then update
the prompt flow around tracked.translator.attributeNextTurn and this.port.prompt
so a thrown prompt error disarms the attribution before propagating the failure.
Preserve attribution when prompt succeeds and follow the existing applySettings
and resolvePermission cleanup patterns.
In `@packages/host-service/src/runtime/sessions/session-meta-store.ts`:
- Around line 21-31: Update CanonicalSessionsRuntime.setOverrides to wrap the
synchronous metaStore write/delete operations, including
metaStore.delete(sessionId) for archive/close overrides, in the same best-effort
try/catch used for upsert. Ensure Drizzle persistence errors are swallowed while
the live override result is still returned unchanged.
In `@packages/host-service/src/runtime/sessions/translate-acp.ts`:
- Around line 1228-1246: Update toJsonValue so the MAX_RAW_JSON_BYTES limit is
measured using the serialized payload’s UTF-8 byte length rather than JavaScript
string length. Preserve the existing null fallback for undefined, oversized,
unserializable, or invalid JSON values.
In
`@packages/host-service/test/integration/sessions-sync-client.integration.test.ts`:
- Around line 183-189: Update the parity assertions around hostFullLog,
clientEvents, and latestCursor to first require that the client event window is
non-empty, then compare it against the corresponding host log slice. Remove the
self-referential stream?.latestCursor fallback from the latestCursor
expectation, retaining only the host log’s final cursor or the intended
empty-log value.
In
`@packages/host-service/test/integration/sessions-user-story.integration.test.ts`:
- Around line 864-906: Set an explicit generous timeout for the parity test
named “the wire stayed clean and the store matches the host's own answers” using
the file’s existing per-test timeout convention. Keep the test logic and
assertions unchanged, and ensure the timeout covers the full backwards tRPC
pagination after the long story run.
---
Outside diff comments:
In `@apps/mobile/screens/`(authenticated)/(home)/home/HomeScreen.tsx:
- Around line 257-263: Update the pull-to-refresh handler on HomeScreen,
specifically onRefresh, to invalidate or refetch the sessions query using the
same ["sessions", "list"] key as refreshHostData. Await the sessions refresh
alongside the workspace refresh before clearing the refreshing state, while
preserving the existing pull-to-refresh behavior.
---
Nitpick comments:
In `@apps/mobile/components/ai-elements/conversation.tsx`:
- Around line 794-821: Update the ConversationProps type to omit contentInset,
onLayout, onContentSizeChange, onMomentumScrollEnd, onScrollBeginDrag, and
onScrollEndDrag from the underlying list props, alongside the existing children,
data, and renderItem omissions. Keep onScroll available because it is explicitly
chained rather than overridden in the LegendList configuration.
- Around line 762-766: Update the surrounding component logic to destructure
keyExtractor from the props instead of casting listProps to access it. Store the
destructured keyExtractor in keyExtractorRef, and pass it explicitly to
LegendList so the anchor-by-key path remains type-checked against ItemT.
- Around line 228-230: Rename the setPinnedBoth callback to setPinned and update
every reference to it, preserving its current behavior of assigning the value to
pinnedRef.current.
In
`@apps/mobile/screens/`(authenticated)/debug/conversation-lab/ConversationLabScreen.tsx:
- Around line 321-324: Update the append-and-scroll flow around
appendUserMessage so the anchor index is calculated after the wait from the
current itemsRef state, ensuring it targets the newly appended item even if the
state update flushes synchronously.
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/components/SessionThread/components/PermissionStack/PermissionStack.tsx:
- Around line 18-22: Define or reuse a shared constant for the “No pending
permission” prefix and update isAlreadySettled to match against that symbol
instead of an inline string. Ensure resolvePermission also uses the same shared
prefix when constructing its error so the settled-permission sentinel remains
consistent between host and client.
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/components/SessionThread/SessionThread.tsx:
- Around line 217-221: Update the KeyboardAvoidingView in SessionThread to set
behavior directly to "padding" without the Platform.OS conditional, and remove
the now-unused Platform import.
- Around line 152-160: Update loadOlder to use a ref-based in-flight guard,
returning immediately when a page request is already active. Set the guard
before calling client.fetchOlderEvents(sessionId), and clear it in the request’s
finally handler alongside setIsLoadingOlder(false), so only one fetch runs at a
time.
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/SessionThreadScreen.tsx:
- Around line 21-27: Remove the redundant session lookup via useHostSessions and
the derived session title from SessionThreadScreen, allowing SessionThread’s
useSession(sessionId)-based header title to be the sole source of truth. If a
loading-state title is required, restrict the lookup and title usage to the
!routingKey path only.
- Around line 5-6: Promote useHostSessions from the home screen subtree to the
highest shared authenticated screens hooks directory, then update all imports
and references to use the new location while preserving the hook’s
implementation and behavior.
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/finish-review/FinishReviewSheet.tsx:
- Around line 9-14: Promote useHostSessions and getSessionsQueryKey from the
`(home)/home` hooks location to the shared authenticated hooks area, then update
SessionThreadScreen, WorkspaceScreen, and FinishReviewSheet imports to use the
new location. Keep the hook and query-key exports unchanged.
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/hooks/useWorkspaceHeaderActions/useWorkspaceHeaderActions.ts:
- Around line 36-38: Update the invalidateQueries call in
useWorkspaceHeaderActions to use the shared getSessionsQueryKey helper with the
relevant host.machineId instead of the literal ["sessions", "list"] key,
matching FinishReviewSheet and preserving generated query-key consistency.
In `@packages/host-client/src/sessions/sessions.ts`:
- Around line 83-84: Update the syncUrl callback to derive its path from the
imported SESSIONS_SYNC_PATH constant instead of hardcoding "sessions/sync";
confirm transport.streamUrl’s path format and strip or preserve the leading
slash as required while keeping the resulting URL correct.
In `@packages/host-service-sync/src/client/createSessionsSyncClient/state.ts`:
- Around line 324-374: The total-byte eviction loop in the stream state update
performs full map and stream-state copies for every dropped event, causing
quadratic work. Refactor the eviction logic to batch removals per selected
candidate: create one local copy of the candidate event map and remove events
repeatedly until the budget is satisfied or the stream is empty, accumulating
bytes and counts, then commit the updated stream and total state once per
candidate while preserving retention ordering and eviction metadata.
In `@packages/host-service-sync/src/projection/reduceProjection.ts`:
- Around line 31-144: Enforce the declared projection caps in reduceProjection
by validating every returned SessionProjection, including snapshot inputs and
the reduced next projection, with sessionProjectionSchema before returning.
Preserve the existing session/event mismatch checks and state transitions, and
allow schema validation failures to propagate rather than storing an unchecked
projection.
In `@packages/host-service-sync/src/protocol/api.ts`:
- Around line 314-341: Validate cursor ordering in both event-array refinement
sites: packages/host-service-sync/src/protocol/api.ts lines 314-341 and 427-452.
While iterating snapshot.recentEvents and window.items, track each prior cursor
and add a validation issue when the current cursor is not strictly greater;
preserve the existing session, duplicate-ID, and endpoint checks.
In `@packages/host-service-sync/src/protocol/sync.ts`:
- Around line 103-114: Extract the duplicated stream/sessionId validation into a
shared refineStreamSessionId helper near the packet schemas, preserving its
existing z.RefinementCtx issue and message. Replace the four identical
superRefine callbacks on subscribed, caughtUp, unsubscribed, and reset with
.superRefine(refineStreamSessionId).
In `@packages/host-service/src/runtime/acp-sessions/acp-sessions.ts`:
- Around line 919-931: Update the lifecycle invariant comments for the offline
field and ensureLive to reflect that evicted dead runtimes are parked in offline
and can be resurrected during the same host lifetime. Also preserve the eviction
diagnostic by carrying runtime.state.lastError into the parked record used by
get().
In `@packages/host-service/src/runtime/sessions/canonical-sessions.ts`:
- Around line 834-852: Extract the duplicated port.list pagination logic from
hostSnapshotData and sweepOrphanedSessionMeta into a private allSessionStates
generator. Have it yield each SessionScopedState while preserving
DEFAULT_PAGE_LIMIT, cursor updates, and termination when nextCursor is
undefined; update both callers to consume the generator and retain their
existing filtering and session-processing behavior.
- Around line 1100-1103: Replace the per-event Array.shift eviction in the
session event tracking logic with an O(1) head-index ring approach, using
evictedCount to map logical serials to the retained event positions. Preserve
the MAX_SESSION_LOG_EVENTS retention bound and existing event ordering/lookup
behavior, and update any affected access or serialization logic consistently.
- Around line 1079-1080: Handle the unused TrackedSession.foldError field
consistently: either expose it through the existing diagnostics or session-error
accessor used by consumers, or remove its declaration, null initialization, and
assignment in the translate/fold failure path. Do not leave failures stored only
in the unconsumed field and console.warn.
In `@packages/host-service/src/runtime/sessions/sync-hub.ts`:
- Around line 656-679: Replace the array-shift eviction in fanOutHostChange with
an O(1) head-index ring buffer or deque for hostRing, updating related iteration
and capacity logic to preserve insertion order and retain only hostRingLimit
entries. Keep event fan-out behavior unchanged while ensuring full-ring eviction
does not move all remaining elements.
In `@packages/host-service/src/runtime/sessions/sync-route.ts`:
- Around line 49-52: Update the WebSocket onError handler to accept the
underlying error and log its details before disposing the connection. Preserve
the existing connection?.dispose() and connection = null cleanup behavior, using
the surrounding session or route logger.
In `@packages/host-service/src/runtime/sessions/testing/sync-harness.ts`:
- Around line 16-36: Update FakeSyncSocket.send to record syncServerPacketSchema
parse failures in an invalidPackets collection instead of throwing
synchronously, while preserving valid packet recording in packets. Add an
assertion helper or afterEach mechanism for tests using FakeSyncSocket that
fails when invalidPackets is non-empty, so schema violations remain
independently observable regardless of SessionsSyncHub.send connection handling.
In `@packages/host-service/src/runtime/sessions/translate-acp.ts`:
- Around line 596-600: Bound the pendingPermissions collection in the
permission-tracking flow around pendingPermissions.set, adding an explicit cap
and eviction behavior consistent with trackToolCall, trackSubagentThread, and
mintedTurnIds. Preserve removal on permission_resolved while ensuring unresolved
requests cannot retain entries indefinitely.
In `@packages/host-service/src/trpc/router/sessions/sessions.ts`:
- Around line 114-206: Move the shared rethrowMapped error handling into
gatedProcedure middleware that awaits next() and maps domain errors, then remove
the duplicated try/catch wrappers from create, get, update, getEvents,
submitTurn, cancelTurn, resolvePermission, and resolveToolCall. Verify
middleware mapping handles handler failures for the project’s tRPC version
before relying on bare session calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| * (AGENTS.md next to conversation.tsx). Each scenario self-asserts and | ||
| * reports `PASS` / `FAIL(reason)`; the Maestro flow | ||
| * (.maestro/conversation-lab.yaml) taps "run all" and asserts every PASS — | ||
| * a simplification that breaks an invariant fails visibly instead of | ||
| * shipping. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale flow path in the doc comment.
The suite lives at .maestro/flows/conversation-lab.yml (as AGENTS.md states), not .maestro/conversation-lab.yaml. Worth fixing since this pointer is how anyone touching scroll discipline finds the regression suite.
📝 Proposed fix
- * reports `PASS` / `FAIL(reason)`; the Maestro flow
- * (.maestro/conversation-lab.yaml) taps "run all" and asserts every PASS —
+ * reports `PASS` / `FAIL(reason)`; the Maestro flow
+ * (.maestro/flows/conversation-lab.yml) taps "run all" and asserts every PASS —Based on learnings: "Before simplifying any Chat scroll-discipline behavior, run maestro test .maestro/flows/conversation-lab.yml and keep the regression suite green."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * (AGENTS.md next to conversation.tsx). Each scenario self-asserts and | |
| * reports `PASS` / `FAIL(reason)`; the Maestro flow | |
| * (.maestro/conversation-lab.yaml) taps "run all" and asserts every PASS — | |
| * a simplification that breaks an invariant fails visibly instead of | |
| * shipping. | |
| */ | |
| * (AGENTS.md next to conversation.tsx). Each scenario self-asserts and | |
| * reports `PASS` / `FAIL(reason)`; the Maestro flow | |
| * (.maestro/flows/conversation-lab.yml) taps "run all" and asserts every PASS — | |
| * a simplification that breaks an invariant fails visibly instead of | |
| * shipping. | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/mobile/screens/`(authenticated)/debug/conversation-lab/ConversationLabScreen.tsx
around lines 22 - 27, Update the doc comment near ConversationLabScreen’s
regression-suite description to reference `.maestro/flows/conversation-lab.yml`
instead of the stale `.maestro/conversation-lab.yaml` path, preserving the
existing guidance and assertions.
Source: Learnings
| return ( | ||
| <View | ||
| accessibilityLabel="Agent is working" | ||
| className="px-1 py-2" | ||
| style={{ height: BOX_H + 16 }} | ||
| > |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
accessibilityLabel on a plain View may not be announced.
Without accessible, VoiceOver typically won't surface this label, so the only "agent is working" signal is invisible to screen-reader users. Also consider honoring useReducedMotion() (fall back to a static dot cluster) since the flock animates indefinitely.
♿ Proposed change
<View
+ accessible
accessibilityLabel="Agent is working"
className="px-1 py-2"
style={{ height: BOX_H + 16 }}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <View | |
| accessibilityLabel="Agent is working" | |
| className="px-1 py-2" | |
| style={{ height: BOX_H + 16 }} | |
| > | |
| return ( | |
| <View | |
| accessible | |
| accessibilityLabel="Agent is working" | |
| className="px-1 py-2" | |
| style={{ height: BOX_H + 16 }} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/components/SessionThread/components/TypingIndicator/TypingIndicator.tsx
around lines 256 - 261, Update the TypingIndicator component so its “Agent is
working” status is exposed to screen readers by marking the containing View as
accessible while retaining its accessibilityLabel. Use useReducedMotion() to
disable the indefinite flock animation and render a static dot cluster when
reduced motion is enabled; preserve the existing animation for normal motion.
| const handleSend = useCallback( | ||
| async (text: string) => { | ||
| if (mainThreadId === null) return; | ||
| setActionError(null); | ||
| // The reply takes a while — drop the keyboard so the user can watch | ||
| // it stream in, and anchor their message to the top once it echoes. | ||
| Keyboard.dismiss(); | ||
| pendingAnchorRef.current = true; | ||
| try { | ||
| await submitTurn(routingKey, { | ||
| sessionId, | ||
| threadId: mainThreadId, | ||
| content: [{ type: "text", text }], | ||
| }); | ||
| } catch (cause) { | ||
| pendingAnchorRef.current = false; | ||
| setActionError(cause instanceof Error ? cause.message : String(cause)); | ||
| throw cause; | ||
| } | ||
| }, | ||
| [routingKey, sessionId, mainThreadId], | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Silent message loss when mainThreadId is null.
handleSend resolves without sending when mainThreadId === null, and Composer.handleSubmit only restores the typed text on rejection — so the prompt input is cleared and the message disappears with no banner. Reject (or block composing) instead so the text is preserved.
🐛 Proposed fix
const handleSend = useCallback(
async (text: string) => {
- if (mainThreadId === null) return;
+ if (mainThreadId === null) {
+ throw new Error("Session is not ready yet — try again in a moment.");
+ }
setActionError(null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleSend = useCallback( | |
| async (text: string) => { | |
| if (mainThreadId === null) return; | |
| setActionError(null); | |
| // The reply takes a while — drop the keyboard so the user can watch | |
| // it stream in, and anchor their message to the top once it echoes. | |
| Keyboard.dismiss(); | |
| pendingAnchorRef.current = true; | |
| try { | |
| await submitTurn(routingKey, { | |
| sessionId, | |
| threadId: mainThreadId, | |
| content: [{ type: "text", text }], | |
| }); | |
| } catch (cause) { | |
| pendingAnchorRef.current = false; | |
| setActionError(cause instanceof Error ? cause.message : String(cause)); | |
| throw cause; | |
| } | |
| }, | |
| [routingKey, sessionId, mainThreadId], | |
| ); | |
| const handleSend = useCallback( | |
| async (text: string) => { | |
| if (mainThreadId === null) { | |
| throw new Error("Session is not ready yet — try again in a moment."); | |
| } | |
| setActionError(null); | |
| // The reply takes a while — drop the keyboard so the user can watch | |
| // it stream in, and anchor their message to the top once it echoes. | |
| Keyboard.dismiss(); | |
| pendingAnchorRef.current = true; | |
| try { | |
| await submitTurn(routingKey, { | |
| sessionId, | |
| threadId: mainThreadId, | |
| content: [{ type: "text", text }], | |
| }); | |
| } catch (cause) { | |
| pendingAnchorRef.current = false; | |
| setActionError(cause instanceof Error ? cause.message : String(cause)); | |
| throw cause; | |
| } | |
| }, | |
| [routingKey, sessionId, mainThreadId], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/components/SessionThread/SessionThread.tsx
around lines 88 - 109, Update handleSend in SessionThread so a null mainThreadId
rejects or otherwise blocks submission instead of returning successfully.
Preserve the existing error-restoration path in Composer.handleSubmit, ensuring
the typed prompt is retained and the action error is surfaced when no thread ID
is available.
| const isDead = runState === "closed" || runState === "failed"; | ||
| // A blocking permission ask keeps runState "running" — the canonical run | ||
| // state has no separate awaiting_permission arm. | ||
| const canCompose = | ||
| status === "idle" || | ||
| status === "running" || | ||
| status === "awaiting_permission"; | ||
| runState === "idle" || runState === "running" || runState === "cancelling"; | ||
| const composerStatus = | ||
| status === "running" || status === "awaiting_permission" | ||
| runState === "running" || runState === "cancelling" | ||
| ? ("streaming" as const) | ||
| : ("ready" as const); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
canCompose ignores isLoading/errorText, contradicting the documented intent.
SessionThread.tsx Lines 304-306 state that "Dead, offline, and not-yet-loaded sessions cannot accept prompts… keep the composer hidden after a failed session/load", but canCompose is derived from runState alone. A session whose entity says idle while the stream is still subscribing (or errored) renders the composer next to the spinner/destructive banner.
🐛 Proposed fix
- const canCompose =
- runState === "idle" || runState === "running" || runState === "cancelling";
+ const canCompose =
+ !isLoading &&
+ errorText === null &&
+ (runState === "idle" || runState === "running" || runState === "cancelling");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isDead = runState === "closed" || runState === "failed"; | |
| // A blocking permission ask keeps runState "running" — the canonical run | |
| // state has no separate awaiting_permission arm. | |
| const canCompose = | |
| status === "idle" || | |
| status === "running" || | |
| status === "awaiting_permission"; | |
| runState === "idle" || runState === "running" || runState === "cancelling"; | |
| const composerStatus = | |
| status === "running" || status === "awaiting_permission" | |
| runState === "running" || runState === "cancelling" | |
| ? ("streaming" as const) | |
| : ("ready" as const); | |
| const isDead = runState === "closed" || runState === "failed"; | |
| // A blocking permission ask keeps runState "running" — the canonical run | |
| // state has no separate awaiting_permission arm. | |
| const canCompose = | |
| !isLoading && | |
| errorText === null && | |
| (runState === "idle" || runState === "running" || runState === "cancelling"); | |
| const composerStatus = | |
| runState === "running" || runState === "cancelling" | |
| ? ("streaming" as const) | |
| : ("ready" as const); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/chat/[sessionId]/components/SessionThread/utils/getSessionThreadPresentation/getSessionThreadPresentation.ts
around lines 25 - 33, Update canCompose in getSessionThreadPresentation to also
require the session to be loaded and error-free: exclude states represented by
isLoading and errorText, while preserving the existing runState checks for idle,
running, and cancelling. Ensure the composer remains hidden during
subscription/loading and whenever a load or stream error is present.
| connect(): void { | ||
| if (!stopped) return; | ||
| stopped = false; | ||
| resetWindows.clear(); | ||
| for (const [sessionId, stream] of Object.entries( | ||
| store.getState().streamsBySessionId, | ||
| )) { | ||
| if (stream.retention === "warm" && stream.retainCount === 0) { | ||
| scheduleWarmExpiry(sessionId); | ||
| } | ||
| } | ||
| openConnection(); | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
connect() inherits the previous backoff counter.
reconnectAttempts is only reset on helloAck (Line 599), so a disconnect() → connect() cycle starts with the stale attempt count: openConnection reports "reconnecting" instead of "connecting", and the first failure waits backoffDelay(n) (up to 30s) rather than restarting the ramp. An explicit connect is a fresh intent and should reset it.
🔧 Proposed fix
connect(): void {
if (!stopped) return;
stopped = false;
+ reconnectAttempts = 0;
resetWindows.clear();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| connect(): void { | |
| if (!stopped) return; | |
| stopped = false; | |
| resetWindows.clear(); | |
| for (const [sessionId, stream] of Object.entries( | |
| store.getState().streamsBySessionId, | |
| )) { | |
| if (stream.retention === "warm" && stream.retainCount === 0) { | |
| scheduleWarmExpiry(sessionId); | |
| } | |
| } | |
| openConnection(); | |
| }, | |
| connect(): void { | |
| if (!stopped) return; | |
| stopped = false; | |
| reconnectAttempts = 0; | |
| resetWindows.clear(); | |
| for (const [sessionId, stream] of Object.entries( | |
| store.getState().streamsBySessionId, | |
| )) { | |
| if (stream.retention === "warm" && stream.retainCount === 0) { | |
| scheduleWarmExpiry(sessionId); | |
| } | |
| } | |
| openConnection(); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/host-service-sync/src/client/createSessionsSyncClient/createSessionsSyncClient.ts`
around lines 1040 - 1052, Reset reconnectAttempts at the start of connect()
before calling openConnection(), so an explicit reconnect begins as a fresh
connection, reports “connecting,” and uses the initial backoff delay on failure.
Preserve the existing stopped guard and stream/window setup.
| tracked.translator.attributeNextTurn({ | ||
| requestId: input.requestId, | ||
| clientInstanceId: null, | ||
| }); | ||
| this.port.prompt({ sessionId: input.sessionId, prompt }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Armed turn attribution is not disarmed when prompt() throws.
attributeNextTurn is set before the port call, but unlike applySettings (Line 1322-1325) and resolvePermission (Line 707-713), a failing prompt() leaves the attribution armed — the next turn started by anything else then carries this requestId as its causationId. The translator has no counterpart to clearSettingsCausation; consider adding one and calling it here.
♻️ Sketch
tracked.translator.attributeNextTurn({
requestId: input.requestId,
clientInstanceId: null,
});
- this.port.prompt({ sessionId: input.sessionId, prompt });
+ try {
+ this.port.prompt({ sessionId: input.sessionId, prompt });
+ } catch (error) {
+ tracked.translator.clearTurnAttribution(input.requestId);
+ throw error;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/host-service/src/runtime/sessions/canonical-sessions.ts` around
lines 628 - 632, Add a translator method corresponding to clearSettingsCausation
for clearing pending next-turn attribution, then update the prompt flow around
tracked.translator.attributeNextTurn and this.port.prompt so a thrown prompt
error disarms the attribution before propagating the failure. Preserve
attribution when prompt succeeds and follow the existing applySettings and
resolvePermission cleanup patterns.
| /** | ||
| * Durable store behind CanonicalSessionsRuntime's session overrides. | ||
| * `loadAll` seeds the runtime at construction; `upsert` runs on every | ||
| * override write (create title, update) and is best-effort — a failure must | ||
| * never break the live path. | ||
| */ | ||
| export interface SessionMetaStore { | ||
| loadAll(): SessionMetaRecord[]; | ||
| upsert(record: SessionMetaRecord): void; | ||
| delete(sessionId: string): void; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Where does CanonicalSessionsRuntime call metaStore.upsert / delete, and is it guarded?
rg -nP -B6 -A6 '\bmetaStore\??\.(upsert|delete)\s*\(' packages/host-service/srcRepository: superset-sh/superset
Length of output: 2471
🏁 Script executed:
#!/bin/bash
set -e
echo "== canonical-sessions relevant section =="
sed -n '800,915p' packages/host-service/src/runtime/sessions/canonical-sessions.ts
echo "== meta store implementations and declarations =="
sed -n '1,90p' packages/host-service/src/runtime/sessions/session-meta-store.ts
sed -n '1,220p' `fd 'sqlite.*session.*meta|session.*meta.*sqlite' packages/host-service/src | head -n1` 2>/dev/null || true
echo "== direct upsert/delete call sites =="
rg -n -B4 -A12 '\b(metaStore|this\.metaStore)\.?(upsert|delete)\s*\(' packages/host-service/src/runtime/sessions packages/host-service/src | sed -n '1,220p'Repository: superset-sh/superset
Length of output: 6475
Guard the write path for session metadata deletes.
CanonicalSessionsRuntime guards upsert in setOverrides, but setOverrides also writes archive/close overrides from the live session object and then calls metaStore.delete(sessionId) synchronously, where Drizzle errors still propagate. Since the documented contract says metadata persistence failures must be best-effort, add the same surrounding try/catch around the store write/delete before returning the live override result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/host-service/src/runtime/sessions/session-meta-store.ts` around
lines 21 - 31, Update CanonicalSessionsRuntime.setOverrides to wrap the
synchronous metaStore write/delete operations, including
metaStore.delete(sessionId) for archive/close overrides, in the same best-effort
try/catch used for upsert. Ensure Drizzle persistence errors are swallowed while
the live override result is still returned unchanged.
| const MAX_RAW_JSON_BYTES = 256 * 1024; | ||
|
|
||
| /** | ||
| * Adapter raw input/output is arbitrary; the canonical log stores JSON. | ||
| * Round-tripping through JSON strips functions/undefined/cycles — anything | ||
| * unserializable or beyond the size budget becomes null. | ||
| */ | ||
| function toJsonValue(raw: unknown): JsonValue { | ||
| if (raw === undefined) return null; | ||
| try { | ||
| const serialized = JSON.stringify(raw); | ||
| if (serialized === undefined || serialized.length > MAX_RAW_JSON_BYTES) { | ||
| return null; | ||
| } | ||
| return JSON.parse(serialized) ?? null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
MAX_RAW_JSON_BYTES is compared against string length, not bytes.
serialized.length counts UTF-16 code units, so a payload of multi-byte characters passes the 256 KiB check at up to ~768 KiB of actual UTF-8 — which erodes the 1 MiB frame budget this constant exists to protect (Line 1222-1227).
🐛 Proposed fix
try {
const serialized = JSON.stringify(raw);
- if (serialized === undefined || serialized.length > MAX_RAW_JSON_BYTES) {
+ if (
+ serialized === undefined ||
+ Buffer.byteLength(serialized, "utf8") > MAX_RAW_JSON_BYTES
+ ) {
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const MAX_RAW_JSON_BYTES = 256 * 1024; | |
| /** | |
| * Adapter raw input/output is arbitrary; the canonical log stores JSON. | |
| * Round-tripping through JSON strips functions/undefined/cycles — anything | |
| * unserializable or beyond the size budget becomes null. | |
| */ | |
| function toJsonValue(raw: unknown): JsonValue { | |
| if (raw === undefined) return null; | |
| try { | |
| const serialized = JSON.stringify(raw); | |
| if (serialized === undefined || serialized.length > MAX_RAW_JSON_BYTES) { | |
| return null; | |
| } | |
| return JSON.parse(serialized) ?? null; | |
| } catch { | |
| return null; | |
| } | |
| } | |
| const MAX_RAW_JSON_BYTES = 256 * 1024; | |
| /** | |
| * Adapter raw input/output is arbitrary; the canonical log stores JSON. | |
| * Round-tripping through JSON strips functions/undefined/cycles — anything | |
| * unserializable or beyond the size budget becomes null. | |
| */ | |
| function toJsonValue(raw: unknown): JsonValue { | |
| if (raw === undefined) return null; | |
| try { | |
| const serialized = JSON.stringify(raw); | |
| if ( | |
| serialized === undefined || | |
| Buffer.byteLength(serialized, "utf8") > MAX_RAW_JSON_BYTES | |
| ) { | |
| return null; | |
| } | |
| return JSON.parse(serialized) ?? null; | |
| } catch { | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/host-service/src/runtime/sessions/translate-acp.ts` around lines
1228 - 1246, Update toJsonValue so the MAX_RAW_JSON_BYTES limit is measured
using the serialized payload’s UTF-8 byte length rather than JavaScript string
length. Preserve the existing null fallback for undefined, oversized,
unserializable, or invalid JSON values.
| const log = await hostFullLog(sessionId); | ||
| const window = clientEvents(target, sessionId); | ||
| expect(window).toEqual(log.slice(log.length - window.length)); | ||
| const stream = target.store.getState().streamsBySessionId[sessionId]; | ||
| expect(stream?.latestCursor).toBe( | ||
| log[log.length - 1]?.cursor ?? stream?.latestCursor ?? null, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The parity oracle silently degrades to a tautology on an empty window/log.
Two assertions self-cancel in the empty case: with window.length === 0, log.slice(log.length - 0) is [], so Line 185 always passes; and when the host log is empty, Line 188's ?? stream?.latestCursor compares the value to itself. Since this helper is the oracle for the whole suite, a stream that folded nothing would read as parity rather than a failure.
💚 Assert the window is non-empty and drop the self-referential fallback
const log = await hostFullLog(sessionId);
const window = clientEvents(target, sessionId);
+ expect(window.length).toBeGreaterThan(0);
expect(window).toEqual(log.slice(log.length - window.length));
const stream = target.store.getState().streamsBySessionId[sessionId];
- expect(stream?.latestCursor).toBe(
- log[log.length - 1]?.cursor ?? stream?.latestCursor ?? null,
- );
+ expect(stream?.latestCursor).toBe(log[log.length - 1]?.cursor ?? null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const log = await hostFullLog(sessionId); | |
| const window = clientEvents(target, sessionId); | |
| expect(window).toEqual(log.slice(log.length - window.length)); | |
| const stream = target.store.getState().streamsBySessionId[sessionId]; | |
| expect(stream?.latestCursor).toBe( | |
| log[log.length - 1]?.cursor ?? stream?.latestCursor ?? null, | |
| ); | |
| const log = await hostFullLog(sessionId); | |
| const window = clientEvents(target, sessionId); | |
| expect(window.length).toBeGreaterThan(0); | |
| expect(window).toEqual(log.slice(log.length - window.length)); | |
| const stream = target.store.getState().streamsBySessionId[sessionId]; | |
| expect(stream?.latestCursor).toBe(log[log.length - 1]?.cursor ?? null); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/host-service/test/integration/sessions-sync-client.integration.test.ts`
around lines 183 - 189, Update the parity assertions around hostFullLog,
clientEvents, and latestCursor to first require that the client event window is
non-empty, then compare it against the corresponding host log slice. Remove the
self-referential stream?.latestCursor fallback from the latestCursor
expectation, retaining only the host log’s final cursor or the intended
empty-log value.
| test("the wire stayed clean and the store matches the host's own answers", async () => { | ||
| // The client hard-validates every frame and force-closes on mismatch, | ||
| // so zero drops/resets makes the whole story a wire-conformance run. | ||
| const drops = logs.filter( | ||
| (event) => | ||
| event.event === "sessions_sync.socket_dropped" || | ||
| event.event === "sessions_sync.stream_reset", | ||
| ); | ||
| expect(drops).toEqual([]); | ||
| expect(client.store.getState().connection.error).toBeNull(); | ||
|
|
||
| // Parity oracle: page the whole log backwards over tRPC and compare it | ||
| // to the client store's fold, event for event. The client retained the | ||
| // session at birth (the seed tail held the entire young log), so its | ||
| // fold must equal the host's full journal, and the entities must agree. | ||
| const full: SessionEvent[] = []; | ||
| let beforeCursor: string | undefined; | ||
| while (true) { | ||
| const window = await host.trpc.sessions.getEvents.query({ | ||
| sessionId: sessionA.id, | ||
| ...(beforeCursor === undefined ? {} : { beforeCursor }), | ||
| limit: 100, | ||
| }); | ||
| full.unshift(...window.items); | ||
| if (!window.range.hasMoreBefore) break; | ||
| const oldest = window.range.oldest; | ||
| if (!oldest) throw new Error("hasMoreBefore with no oldest boundary"); | ||
| beforeCursor = oldest.cursor; | ||
| } | ||
| expect(full.length).toBeGreaterThan(0); | ||
| expect(events()).toEqual(full); | ||
|
|
||
| const snapshot = await host.trpc.sessions.get.query({ | ||
| sessionId: sessionA.id, | ||
| }); | ||
| expect(sessionEntity()).toEqual(snapshot.session); | ||
| expect(snapshot.session.runState).toBe("idle"); | ||
| expect(snapshot.pendingPermissions).toEqual([]); | ||
| expect(pendingPermissions()).toEqual([]); | ||
| // The story is quiescent, so the snapshot head is the fold's head. | ||
| const streamA = client.store.getState().streamsBySessionId[sessionA.id]; | ||
| expect(streamA?.latestCursor).toBe(snapshot.head); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Give the parity test an explicit timeout.
Every other test in this file sets a generous per-test timeout, but this one relies on the default. It pages the entire session log backwards at 100 events per tRPC round trip after a long story run, so a large log can plausibly exceed the default budget — and a spurious timeout here throws away a 30+ minute gated, token-spending run.
🛡️ Proposed fix
- test("the wire stayed clean and the store matches the host's own answers", async () => {
+ test("the wire stayed clean and the store matches the host's own answers", async () => {
// ...
- });
+ }, 120_000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("the wire stayed clean and the store matches the host's own answers", async () => { | |
| // The client hard-validates every frame and force-closes on mismatch, | |
| // so zero drops/resets makes the whole story a wire-conformance run. | |
| const drops = logs.filter( | |
| (event) => | |
| event.event === "sessions_sync.socket_dropped" || | |
| event.event === "sessions_sync.stream_reset", | |
| ); | |
| expect(drops).toEqual([]); | |
| expect(client.store.getState().connection.error).toBeNull(); | |
| // Parity oracle: page the whole log backwards over tRPC and compare it | |
| // to the client store's fold, event for event. The client retained the | |
| // session at birth (the seed tail held the entire young log), so its | |
| // fold must equal the host's full journal, and the entities must agree. | |
| const full: SessionEvent[] = []; | |
| let beforeCursor: string | undefined; | |
| while (true) { | |
| const window = await host.trpc.sessions.getEvents.query({ | |
| sessionId: sessionA.id, | |
| ...(beforeCursor === undefined ? {} : { beforeCursor }), | |
| limit: 100, | |
| }); | |
| full.unshift(...window.items); | |
| if (!window.range.hasMoreBefore) break; | |
| const oldest = window.range.oldest; | |
| if (!oldest) throw new Error("hasMoreBefore with no oldest boundary"); | |
| beforeCursor = oldest.cursor; | |
| } | |
| expect(full.length).toBeGreaterThan(0); | |
| expect(events()).toEqual(full); | |
| const snapshot = await host.trpc.sessions.get.query({ | |
| sessionId: sessionA.id, | |
| }); | |
| expect(sessionEntity()).toEqual(snapshot.session); | |
| expect(snapshot.session.runState).toBe("idle"); | |
| expect(snapshot.pendingPermissions).toEqual([]); | |
| expect(pendingPermissions()).toEqual([]); | |
| // The story is quiescent, so the snapshot head is the fold's head. | |
| const streamA = client.store.getState().streamsBySessionId[sessionA.id]; | |
| expect(streamA?.latestCursor).toBe(snapshot.head); | |
| }); | |
| test("the wire stayed clean and the store matches the host's own answers", async () => { | |
| // The client hard-validates every frame and force-closes on mismatch, | |
| // so zero drops/resets makes the whole story a wire-conformance run. | |
| const drops = logs.filter( | |
| (event) => | |
| event.event === "sessions_sync.socket_dropped" || | |
| event.event === "sessions_sync.stream_reset", | |
| ); | |
| expect(drops).toEqual([]); | |
| expect(client.store.getState().connection.error).toBeNull(); | |
| // Parity oracle: page the whole log backwards over tRPC and compare it | |
| // to the client store's fold, event for event. The client retained the | |
| // session at birth (the seed tail held the entire young log), so its | |
| // fold must equal the host's full journal, and the entities must agree. | |
| const full: SessionEvent[] = []; | |
| let beforeCursor: string | undefined; | |
| while (true) { | |
| const window = await host.trpc.sessions.getEvents.query({ | |
| sessionId: sessionA.id, | |
| ...(beforeCursor === undefined ? {} : { beforeCursor }), | |
| limit: 100, | |
| }); | |
| full.unshift(...window.items); | |
| if (!window.range.hasMoreBefore) break; | |
| const oldest = window.range.oldest; | |
| if (!oldest) throw new Error("hasMoreBefore with no oldest boundary"); | |
| beforeCursor = oldest.cursor; | |
| } | |
| expect(full.length).toBeGreaterThan(0); | |
| expect(events()).toEqual(full); | |
| const snapshot = await host.trpc.sessions.get.query({ | |
| sessionId: sessionA.id, | |
| }); | |
| expect(sessionEntity()).toEqual(snapshot.session); | |
| expect(snapshot.session.runState).toBe("idle"); | |
| expect(snapshot.pendingPermissions()).toEqual([]); | |
| expect(pendingPermissions()).toEqual([]); | |
| // The story is quiescent, so the snapshot head is the fold's head. | |
| const streamA = client.store.getState().streamsBySessionId[sessionA.id]; | |
| expect(streamA?.latestCursor).toBe(snapshot.head); | |
| }, 120_000); |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 881-885: Avoid SQL injection
Context: host.trpc.sessions.getEvents.query({
sessionId: sessionA.id,
...(beforeCursor === undefined ? {} : { beforeCursor }),
limit: 100,
})
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/host-service/test/integration/sessions-user-story.integration.test.ts`
around lines 864 - 906, Set an explicit generous timeout for the parity test
named “the wire stayed clean and the store matches the host's own answers” using
the file’s existing per-test timeout convention. Keep the test logic and
assertions unchanged, and ensure the timeout covers the full backwards tRPC
pagination after the long story run.
Supersedes #5630. This is @bra1nDump's host sessions sync v1 — the canonical session plane (tRPC snapshots +
/sessions/syncWS) with mobile fully rewired onto it — picked up, brought up to date withmain, put through a four-track deep review, and hardened. All of Kirill's commits are preserved in the branch history; see #5630 for the full protocol spec (three review layers) and rollout doc, which still apply verbatim.What this adds on top of #5630
Merge with main (139 commits of drift)
chat/acp/route against main's re-registration.0010; kept main's chain and regeneratedsession_metaas0012via drizzle-kit (identical DDL).FinishReviewSheet(feat(mobile): diff review surface — inline comments, native review toolbar, rebuilt diff renderer #5838) off the deleted ACP APIs ontouseHostSessions+submitTurn.getSessionThreadPresentationtest (still asserted the old ACP field names; mobile typecheck doesn't cover test files, so it shipped red on the original branch).Host runtime hardening (
packages/host-service)foreign_cursor→reset(CURSOR_INVALID)) and surfacestruncatedBeforein scrollback.submitTurnon a closed session no longer spawns an adapter child before rejecting.requestId; settings batches validate fully before the first port mutation (no more half-applied batches).foldTimeline(empty, all)and the incremental streaming path marked different user messages as failed — a violation of the shared-fold contract the package rests on.Sync client resilience (
packages/host-service-sync/client)errorforever — nothing retried. Both now retry with epoch-guarded exponential backoff.sessionUpsertlands (previously the pane the user just opened stayed blank until remount).idleinstead of a permanentsubscribingspinner.Mobile chat fixes (
apps/mobile)NOT_FOUND→ restore, with zero feedback); real failures restore the card and surface in the thread error banner.ai-elements/AGENTS.md).scrollToBottomcancels in-flight anchor legs;lineDiffStats(megabyte string splits) is memoized.Review coverage
Four parallel deep reviews (protocol/projection/timeline, host runtime + router + hub, client store + React bindings, mobile rewiring), cross-checked against the 66 existing review threads on #5630 (Kirill had fixed or explicitly deferred nearly all of them — the remainder are in this PR or the follow-ups below). Confirmed-clean areas worth recording: WS auth matches the other socket surfaces, first-write-wins permission resolution is genuinely atomic, no adapter double-spawn, reducer purity holds, no dangling references to the deleted ACP/Mastra planes anywhere in the repo, and Reanimated worklet usage in the new indicators is correct.
Known follow-ups (deliberately not in this PR)
planis projection state fed only byplanUpdatedevents, and snapshots don't carry it — a plan renders empty after reload/re-tracking until the agent rewrites it. Needsplanin the session snapshot (protocol + host + client fold).message, discardingdata.code— the mobile permission-stack fix has to classifyNOT_FOUNDby message text. Preserve the code inpackages/host-clienttransport and type it.useSessionTimeline, and a quadratic total-byte eviction loop — fine at current sizes, degrades on 10k-event sessions.turnStartedafter itsturnCompletedre-pinsrunning); today's single fold-at-mint caller is safe, but the contract should be tightened or start events guarded.frame_size_limit) — snapshot pagination is the real fix (Kirill's deferral, still open).warmSession(adapter spawn + replay) stalls pings behind it.withReceiptfingerprints withJSON.stringify(input)— key-order differences give spuriousCONFLICTs.handleSendresolves silently whenmainThreadIdis null (text lost with no feedback); duplicatepermissionRequestedids append a second unanswerable card view in the timeline fold (needs a re-emitted live elicitation to trigger).Verification
bun run typecheck— 37/37 tasks.bun run lint— clean;sherif— clean (fixed a@types/reactversion split the new package introduced).bun test: host-service 972 (0 fail, incl. the 11-test realistic-scenario suite and reboot/durability suites), host-service-sync 53, session-protocol 34, host-client 8, host-service-react 2, mobile 8.https://claude.ai/code/session_01XFhc7RWVyTo1VYgQKzq6GJ
Summary by cubic
Introduces the canonical sessions plane (tRPC snapshots +
/sessions/sync) and migrates mobile chat to it for more reliable live sessions and simpler APIs. Removes the ACP path, adds React bindings, and ships a scroll regression lab.New Features
sessions.*API and sync client (@superset/host-service-sync) with a sessions-capable@superset/host-client.@superset/host-service-reacthooks (SessionsSyncProvider,useSession*,useSessionTimeline) and rewired mobile to them; removed ACP chat screens and types.FinishReviewSheetand session list to the new surface; unified chat model picker (CHAT_MODELS).Bug Fixes
Written for commit d56feb6. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests