feat: server-render sessions with snapshot handoff - #1321
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change introduces validated session bootstrap snapshots, separate authenticated sandbox access, revisioned timeline events, transactional session persistence, WebSocket synchronization readiness, and a server-rendered session client. ChangesSession synchronization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![open-inspect[bot]](https://avatars.githubusercontent.com/in/2661131?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Reviewed PR #1321, feat: add session view delta foundation, by @ColeMurray. The PR adds shared V2 session bootstrap/delta contracts, SessionDO-local revision/delta storage primitives, retention/replay helpers, architecture documentation, and focused tests across 13 files (+1255/-5). Overall the foundation is coherent and keeps the production protocol unwired as described.
Critical Issues
None found.
Suggestions
- Added one inline non-blocking suggestion on
packages/shared/src/types/server-messages.ts:116to consider rejecting emptystate_patchobjects so no-op deltas do not consume revisions.
Nitpicks
None.
Positive Feedback
- The new repository API validates deltas before mutating projections and keeps projection writes plus revision/delta insertion inside a synchronous transaction boundary.
- Contiguous replay and prefix-only retention behavior are covered by both mock repository tests and real Durable Object SQLite integration tests.
- The docs clearly separate legacy compatibility, V2 synchronization ordering, credentials handling, and rollout/rollback constraints.
Questions
None.
Verification
- Ran
gh pr diff 1321and reviewed the changed files from a detached PR worktree. - Attempted focused shared/control-plane Vitest runs, but dependencies were not installed in the temporary PR worktree (
vitestunavailable), so tests were not executed locally.
Verdict
Approve.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/control-plane/test/integration/session-view-deltas.test.ts`:
- Around line 16-17: Add per-test cleanup for the shared D1 state in the
“session view delta storage” suite, using the existing cleanup helper if
available or clearing the affected tables in beforeEach or afterEach. Ensure
cleanup runs for every test and preserves the current atomic revision
assertions.
In `@packages/shared/src/types/server-messages.ts`:
- Around line 116-145: The sessionStatePatchSchema currently accepts empty
objects, allowing non-changing state patches to be persisted. Require at least
one patch field in sessionStatePatchSchema while preserving the existing
optional field semantics, and add a regression test verifying that an empty
state_patch is rejected.
In `@packages/shared/src/types/websocket.ts`:
- Around line 7-14: Update the subscribe schema object containing viewProtocol,
resumeRevision, and forceSnapshot with object-level validation requiring
viewProtocol to equal 2 whenever either V2 field is present, while preserving
legacy subscriptions without those fields. Add rejection tests covering
resumeRevision and forceSnapshot supplied without viewProtocol.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 856703a4-f7d6-42f2-b8d4-5939d7f6796c
📒 Files selected for processing (13)
docs/plans/server-rendered-session-websocket-deltas.mdpackages/control-plane/src/session/repository.test.tspackages/control-plane/src/session/repository.tspackages/control-plane/src/session/schema.test.tspackages/control-plane/src/session/schema.tspackages/control-plane/src/session/types.tspackages/control-plane/test/integration/session-view-deltas.test.tspackages/shared/src/types/boundary-schemas.test.tspackages/shared/src/types/index.tspackages/shared/src/types/server-messages.test.tspackages/shared/src/types/server-messages.tspackages/shared/src/types/type-contracts.test.tspackages/shared/src/types/websocket.ts
…ta-foundation # Conflicts: # packages/control-plane/src/session/repository.ts # packages/shared/src/types/type-contracts.test.ts
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/control-plane/src/session/repository.ts (1)
386-402: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDerive the age cutoff from the first non-expired revision.
MAX(revision)over expired rows can select a later revision after an unexpired row. For example, if revisions 1 and 3 are expired but revision 2 is not, this code deletes through revision 3 and removes revision 2 early.Use the revision immediately before the first row where
created_at >= createdBefore. If no such row exists, usecurrentRevision. Add a test with out-of-ordercreated_atvalues.The PR objective requires prefix-only retention.
Proposed SQL direction
- SELECT MAX(revision) AS revision FROM session_view_deltas WHERE created_at < ? + SELECT COALESCE(MIN(revision) - 1, ?) AS revision + FROM session_view_deltas + WHERE created_at >= ?🤖 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/control-plane/src/session/repository.ts` around lines 386 - 402, Update the retention logic in the transactionSync callback to derive ageCutoff from the revision immediately preceding the first session_view_deltas row with created_at >= options.createdBefore, rather than MAX(revision) among expired rows. If no non-expired row exists, use currentRevision, preserving prefix-only deletion and existing cutoff validation; add coverage for out-of-order created_at values where an expired later revision must not delete an unexpired earlier revision.
🤖 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 `@packages/control-plane/src/session/repository.ts`:
- Around line 331-337: Update updateSessionTitleWithViewDelta and the underlying
updateSessionTitle transaction flow to verify that exactly one session row is
affected, throwing inside transactionSync when the count is not one so the
revision and delta insertion roll back. Add a test covering an absent or invalid
sessionId that confirms current_revision and the session delta log remain
unchanged.
---
Outside diff comments:
In `@packages/control-plane/src/session/repository.ts`:
- Around line 386-402: Update the retention logic in the transactionSync
callback to derive ageCutoff from the revision immediately preceding the first
session_view_deltas row with created_at >= options.createdBefore, rather than
MAX(revision) among expired rows. If no non-expired row exists, use
currentRevision, preserving prefix-only deletion and existing cutoff validation;
add coverage for out-of-order created_at values where an expired later revision
must not delete an unexpired earlier revision.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4adcea36-7fb8-427c-bc97-16ef74f46d32
📒 Files selected for processing (10)
packages/control-plane/src/session/repository.test.tspackages/control-plane/src/session/repository.tspackages/control-plane/src/session/types.tspackages/control-plane/test/integration/session-view-deltas.test.tspackages/shared/src/types/boundary-schemas.test.tspackages/shared/src/types/index.tspackages/shared/src/types/server-messages.test.tspackages/shared/src/types/server-messages.tspackages/shared/src/types/type-contracts.test.tspackages/shared/src/types/websocket.ts
💤 Files with no reviewable changes (1)
- packages/shared/src/types/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/control-plane/src/session/types.ts
- packages/shared/src/types/type-contracts.test.ts
- packages/control-plane/src/session/repository.test.ts
- packages/control-plane/test/integration/session-view-deltas.test.ts
- packages/shared/src/types/server-messages.ts
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
open-inspect
Bot
changed the title
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
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 (2)
packages/control-plane/src/session/pull-request-service.ts (1)
102-128: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOptional view-delta methods let the pull-request flow persist state without a delta.
PullRequestRepositorydeclaresupdateSessionBranchWithViewDelta,updateSessionRepositoryBranchWithViewDelta, andcreateArtifactWithViewDeltaas optional. Each call site therefore carries a fallback that writes the projection and records no delta. A protocol-v2 client that misses those deltas keeps a stale view until the next snapshot.SessionRepositoryimplements all three, so the optionality exists only to accommodate test doubles; other doubles in this pull request alias the delta methods onto the double instead.
packages/control-plane/src/session/pull-request-service.ts#L102-L128: remove the?from all three method declarations so every implementation must record a delta.packages/control-plane/src/session/pull-request-service.ts#L302-L326: delete both fallback branches and callupdateSessionRepositoryBranchWithViewDeltaandupdateSessionBranchWithViewDeltadirectly.packages/control-plane/src/session/pull-request-service.ts#L373-L376: delete the??fallback and callcreateArtifactWithViewDeltadirectly.🤖 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/control-plane/src/session/pull-request-service.ts` around lines 102 - 128, Make all three view-delta methods required in PullRequestRepository by removing their optional markers at packages/control-plane/src/session/pull-request-service.ts:102-128. At packages/control-plane/src/session/pull-request-service.ts:302-326, remove both fallback branches and call updateSessionRepositoryBranchWithViewDelta and updateSessionBranchWithViewDelta directly; at packages/control-plane/src/session/pull-request-service.ts:373-376, remove the ?? fallback and call createArtifactWithViewDelta directly.packages/web/src/lib/session-socket/reducer.ts (1)
285-309: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReset
lastAppliedRevisionwhen the connection falls back to the legacy protocol.This case sets
protocol: "legacy"and clearsviewEvents, but it keepslastAppliedRevisionfrom the previous state. After a V2 session downgrades to legacy, the ref inpackages/web/src/hooks/use-session-socket.tsstill reports that revision throughgetResumeRevision, and the nextsubscribeframe carries aresumeRevisionthat no longer describes any state the client holds.Clear the field so the two protocols cannot mix revision state.
🐛 Proposed fix
ready: true, protocol: "legacy", + lastAppliedRevision: null, recovering: false, sync: 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/web/src/lib/session-socket/reducer.ts` around lines 285 - 309, Reset lastAppliedRevision to its initial empty value in the reducer branch that constructs the legacy protocol state alongside protocol: "legacy" and the cleared viewEvents. Ensure the legacy fallback no longer preserves the previous revision used by getResumeRevision and subsequent subscribe frames.
🧹 Nitpick comments (19)
packages/control-plane/src/session/repository.ts (3)
56-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
SESSION_VIEW_RETENTION_INTERVALto state its unit.The constant counts revisions, not milliseconds. The name reads like a duration next to
DEFAULT_SESSION_VIEW_RETENTION_AGE_MS. Rename it toSESSION_VIEW_RETENTION_INTERVAL_REVISIONS.As per coding guidelines: "Use milliseconds for TypeScript durations and timeouts, and encode the unit in names such as
timeoutMsorINACTIVITY_TIMEOUT_MS; never use a baretimeout."🤖 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/control-plane/src/session/repository.ts` around lines 56 - 58, Rename the constant SESSION_VIEW_RETENTION_INTERVAL to SESSION_VIEW_RETENTION_INTERVAL_REVISIONS and update every reference to it, preserving its value and revision-count semantics.Source: Coding guidelines
1434-1454: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the event-row validation and drop the unreachable
nullreturn.Two points:
- The "read the row, validate
timeline_sequence, throw" block is repeated increateGitSyncEventWithViewDelta(lines 1319-1324) andcreateArtifactAndEventWithViewDelta(lines 1572-1577). Extract one private helper that returns the validatedSessionViewEvent.writeEventViewDeltais declarednumber | null, butappendSessionViewDeltareturnsnumberand throws on any failure. Thenullbranch is unreachable, socreateEventWithViewDelta,upsertTokenEventWithViewDelta,upsertToolCallEventWithViewDelta, andupsertExecutionCompleteEventWithViewDeltaall advertise a result their callers can never receive. Narrow all five signatures tonumber.🤖 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/control-plane/src/session/repository.ts` around lines 1434 - 1454, Extract the repeated event-row lookup and timeline_sequence validation from createGitSyncEventWithViewDelta, writeEventViewDelta, and createArtifactAndEventWithViewDelta into one private helper returning SessionViewEvent, and reuse it when constructing event upsert items. Since appendSessionViewDelta and writeEventViewDelta do not return null, narrow writeEventViewDelta plus createEventWithViewDelta, upsertTokenEventWithViewDelta, upsertToolCallEventWithViewDelta, and upsertExecutionCompleteEventWithViewDelta from number | null to number.
485-495: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSession-scoped delta wrappers do not verify the affected row count.
updateSessionBranchandupdateSessionStatusboth accept asessionIdand returnvoid. If the id does not match the stored session row, the projection stays unchanged while the transaction still advances the revision and stores a state patch.updateSessionTitleWithViewDeltaalready guards against this by consumingrowsWrittenand throwing inside the transaction. Apply the same invariant to the remaining session-scoped wrappers.
packages/control-plane/src/session/repository.ts#L485-L495: makeupdateSessionBranchreturn whether exactly one row changed, and throw insideupdateSessionBranchWithViewDeltawhen it does not.packages/control-plane/src/session/repository.ts#L538-L548: makeupdateSessionStatusreturn whether exactly one row changed, and throw insideupdateSessionStatusWithViewDeltawhen it does not.🤖 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/control-plane/src/session/repository.ts` around lines 485 - 495, Update packages/control-plane/src/session/repository.ts lines 485-495 and 538-548: make updateSessionBranch and updateSessionStatus return whether exactly one row was affected, then have updateSessionBranchWithViewDelta and updateSessionStatusWithViewDelta validate that result and throw inside the transaction when it is not exactly one, matching updateSessionTitleWithViewDelta.packages/control-plane/test/integration/helpers.ts (1)
383-394: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale comment above the collector.
The comment at lines 379-381 states that the helper terminates on the
subscribedmessage. The predicate now terminates onsession_readyfor protocol-v2 clients. Extend the comment to cover both cases.🤖 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/control-plane/test/integration/helpers.ts` around lines 383 - 394, Update the comment above the collector in the subscription helper to state that collection terminates on “subscribed” for non-v2 clients and “session_ready” for protocol-v2 clients; leave the collector predicate and surrounding logic unchanged.packages/control-plane/src/session/durable-object.ts (3)
1720-1742: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
satisfies ServerMessageinstead ofas ServerMessage.Both branches cast with
as, which suppresses shape checking. The new v2 synchronization path in the same file usessatisfies ServerMessage(lines 1592, 1601, 1611, 1625). Usesatisfieshere so a future change tosession_history_pageorhistory_pagefails at compile time.🤖 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/control-plane/src/session/durable-object.ts` around lines 1720 - 1742, Replace the `as ServerMessage` assertions in both `safeSend` calls within the history-page branch of the session handler with `satisfies ServerMessage`, preserving the existing `session_history_page` and `history_page` payloads while enabling compile-time shape checking.
1547-1562: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe snapshot retry loop cannot change its outcome.
No
awaitoccurs betweengetSessionBootstrap()andreadBoundedViewCatchUp(...). A Durable Object processes one request at a time, so no mutation can land between those two reads. If the first attempt returnsnulldeltas, the second attempt observes the same state and returnsnullagain.Replace the loop with a single attempt, or document the concurrency the retry is intended to absorb.
🤖 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/control-plane/src/session/durable-object.ts` around lines 1547 - 1562, Remove the two-attempt retry loop around getSessionBootstrap and readBoundedViewCatchUp, making the snapshot path perform one read and return its result directly. Preserve the existing null handling when bootstrap or deltas are unavailable, and keep sendV2SyncMessages unchanged.
1841-1845: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
updateSessionTitleIfUnsetpath.
updateSessionTitleIfUnsethas no production callers; production code now usesonlyIfUnsetplusupdateSessionTitleWithViewDelta, which preserves the title-matches-session invariant without a separate conditional update. Keeping the dead method invites future writes that update the title without a matching view delta.🤖 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/control-plane/src/session/durable-object.ts` around lines 1841 - 1845, Remove the unused updateSessionTitleIfUnset method and its conditional-update path from the session title handling in durable-object.ts. Ensure production callers continue using onlyIfUnset together with updateSessionTitleWithViewDelta so title changes always produce the matching view delta.packages/control-plane/src/routes/session-runtime-proxy.test.ts (1)
64-86: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that the access response stays uncacheable.
The access route returns decrypted code-server and ttyd credentials.
SessionDO.handleSessionAccesssetsCache-Control: private, no-store(packages/control-plane/src/session/durable-object.tsline 2016). This test does not verify that the proxy preserves that header on the accepted response.Add an assertion on
accepted.headers.get("Cache-Control")so a future proxy change cannot silently make credentials cacheable.🤖 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/control-plane/src/routes/session-runtime-proxy.test.ts` around lines 64 - 86, Add an assertion after the accepted response in the “keeps session access client-only” test, verifying accepted.headers.get("Cache-Control") equals "private, no-store".packages/control-plane/src/session/event-stream.ts (1)
56-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the duplication between the v1 and v2 read paths.
getViewReplayduplicatesgetReplayandgetViewHistoryPageduplicatesgetHistoryPage. Only the row-parsing function differs. The cursor-mapping expression is now written twice, verbatim.Two suggestions:
- Extract the
page.nextCursormapping into one helper and share it betweengetHistoryPageandgetViewHistoryPage.- Name the
getViewHistoryPagereturn type, asgetHistoryPagedoes withSessionHistoryPage. The inline object type cannot be referenced by callers.Also applies to: 92-120
🤖 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/control-plane/src/session/event-stream.ts` around lines 56 - 63, Reduce duplication in getReplay/getViewReplay and getHistoryPage/getViewHistoryPage by extracting the shared page.nextCursor-to-cursor mapping into a helper and reusing it in both read paths, while keeping only the row-parsing function different. Define and use a named return type for getViewHistoryPage, matching getHistoryPage’s SessionHistoryPage pattern, so callers can reference its result type.packages/control-plane/src/session/websocket-manager.ts (1)
247-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse named defaults for client view state.
Lines 251-252 and Lines 281-282 repeat the protocol and revision defaults. Define named shared constants for these defaults, then use them for persistence and recovery.
As per coding guidelines, “Define each TypeScript default value exactly once as a named constant and import it wherever needed.”
🤖 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/control-plane/src/session/websocket-manager.ts` around lines 247 - 283, Define shared named constants for the default client view protocol and applied view revision, then use them in persistClientMapping and getClientViewState for method defaults and recovered mapping fallbacks. Ensure each default value is declared exactly once and reused consistently.Source: Coding guidelines
packages/web/src/app/(app)/session/[id]/error.tsx (1)
6-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport the error instead of discarding it.
The component declares
errorbut never uses it. Bootstrap failures, schema-validation failures, and unexpected control-plane statuses all reach this boundary and disappear silently. Log the error on mount and showerror.digest, so a user can quote the digest and an operator can correlate it with server logs.♻️ Proposed refactor
"use client"; +import { useEffect } from "react"; import { Button } from "`@/components/ui/button`"; import { ErrorBanner } from "`@/components/ui/error-banner`"; export default function SessionError({ + error, reset, }: { error: Error & { digest?: string }; reset: () => void; }) { + useEffect(() => { + console.error("session route error", error); + }, [error]); + return ( <div className="flex h-full items-center justify-center p-6"> <ErrorBanner role="alert" className="flex max-w-md flex-col items-center gap-4 p-6 text-center" > <p>Session data is temporarily unavailable.</p> + {error.digest ? ( + <p className="text-xs opacity-70">Reference: {error.digest}</p> + ) : null} <Button type="button" onClick={reset}> Try again </Button> </ErrorBanner> </div> ); }🤖 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/web/src/app/`(app)/session/[id]/error.tsx around lines 6 - 25, Update SessionError to use the declared error: log it when the component mounts and render error.digest when available so users can report the identifier. Preserve the existing retry behavior and error banner messaging.packages/web/src/app/(app)/layout.test.tsx (1)
27-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
AuthenticationUnavailableErrorbranch.The suite covers the authenticated and unauthenticated paths. It does not cover the third branch in
AppLayout, which renders the unavailability banner instead of redirecting or hydrating. That branch also decides that unrelated errors are rethrown. Add two cases to lock this behavior.🧪 Proposed additional tests
+ it("renders the unavailable state instead of redirecting", async () => { + mocks.getServerAuthSession.mockRejectedValue(new AuthenticationUnavailableError()); + + const result = await AppLayout({ children: <div>protected</div> }); + + expect(result.type).not.toBe(AuthSessionHydration); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); + + it("rethrows unrelated failures", async () => { + mocks.getServerAuthSession.mockRejectedValue(new Error("boom")); + await expect(AppLayout({ children: <div>protected</div> })).rejects.toThrow("boom"); + });Import
AuthenticationUnavailableErrorfrom@/lib/authentication-unavailable-errorat the top of the file.🤖 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/web/src/app/`(app)/layout.test.tsx around lines 27 - 47, Extend the “protected app layout” suite around AppLayout with two tests: verify AuthenticationUnavailableError from getServerAuthSession renders the authentication-unavailability banner, and verify any unrelated error is rethrown. Import AuthenticationUnavailableError from the indicated authentication-unavailable-error module and preserve the existing authenticated and redirect expectations.packages/web/src/app/api/sessions/[id]/access/route.ts (1)
5-23: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap the proxy call in the conventional outer try/catch.
controlPlaneUserFetchrejects on transport failure. This handler has no error handling, so the rejection escapes to Next.js and produces an unstructured 500. Sibling proxy routes underpackages/web/src/app/api/use a single outertry/catchthat returns HTTP 500. Align this route with that convention.🛡️ Proposed fix
export async function GET(_request: Request, { params }: { params: Promise<{ id: string }> }) { const session = await getServerAuthSession(); if (!session) return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); - const { id } = await params; - const response = await controlPlaneUserFetch(`/sessions/${encodeURIComponent(id)}/access`, { - cache: "no-store", - headers: { Accept: "application/json" }, - }); - return new Response(response.body, { - status: response.status, - statusText: response.statusText, - headers: { - "Content-Type": response.headers.get("Content-Type") ?? "application/json", - "Cache-Control": "private, no-store", - Vary: "Cookie", - }, - }); + try { + const { id } = await params; + const response = await controlPlaneUserFetch(`/sessions/${encodeURIComponent(id)}/access`, { + cache: "no-store", + headers: { Accept: "application/json" }, + }); + return new Response(response.body, { + status: response.status, + statusText: response.statusText, + headers: { + "Content-Type": response.headers.get("Content-Type") ?? "application/json", + "Cache-Control": "private, no-store", + Vary: "Cookie", + }, + }); + } catch { + return NextResponse.json({ error: "Internal Server Error" }, { status: 500 }); + } }Based on learnings: for Next.js API route handlers under
packages/web/src/app/api/, use a single outertry/catchthat returns HTTP 500 for any thrown 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/web/src/app/api/sessions/`[id]/access/route.ts around lines 5 - 23, Wrap the authenticated proxy flow in GET with a single outer try/catch, including the controlPlaneUserFetch call and response construction. Return a structured HTTP 500 response from the catch block when transport or other errors are thrown, while preserving the existing unauthorized response and successful proxy status, headers, and body behavior.Source: Learnings
packages/web/src/lib/session-socket/reducer.test.ts (1)
298-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the ready state before the close, and cover the snapshot rejection guards.
snapshotholds the state aftersocket_closed, soexpect(snapshot.ready).toBe(false)passes whether or notsession_readyever took effect. Capture the state before the close to prove the snapshot path reached readiness.The
session_snapshotguards atpackages/web/src/lib/session-socket/reducer.tsLines 327-337 are untested. Each one drives a full reconnect, so a regression there is costly.💚 Proposed additions
- const snapshot = reduce( - started, + const synced = reduce( + started, serverMessage({ type: "session_snapshot", bootstrap: createBootstrap({ viewRevision: 8, state: createSessionState({ title: "Snapshot" }), replay: { events: [], hasMore: false, cursor: null }, }), }), serverMessage({ type: "session_ready", sessionId: "session-1", participantId: "participant-1", appliedRevision: 8, - }), - { type: "socket_closed" } + }) ); + const snapshot = reduce(synced, { type: "socket_closed" }); + expect(synced.ready).toBe(true); expect(snapshot.ready).toBe(false); expect(snapshot.sessionState?.title).toBe("Snapshot"); expect(snapshot.events).toEqual([]); }); + + it("rejects a snapshot for a different session", () => { + const started = reduce( + createSessionSocketState(createBootstrap()), + serverMessage({ type: "session_sync_started", mode: "snapshot", targetRevision: 8 }) + ); + const rejected = reduce( + started, + serverMessage({ + type: "session_snapshot", + bootstrap: createBootstrap({ + sessionId: "session-2", + viewRevision: 8, + state: createSessionState({ id: "session-2" }), + }), + }) + ); + + expect(rejected.recoveryNonce).toBe(1); + expect(rejected.sessionState?.id).toBe("session-1"); + }); + + it("rejects a snapshot that arrives without an active sync", () => { + const rejected = reduce( + createSessionSocketState(createBootstrap()), + serverMessage({ type: "session_snapshot", bootstrap: createBootstrap({ viewRevision: 8 }) }) + ); + + expect(rejected.recoveryNonce).toBe(1); + });🤖 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/web/src/lib/session-socket/reducer.test.ts` around lines 298 - 325, Update the test around the snapshot flow to capture and assert the reducer state immediately after session_ready, before applying socket_closed, verifying ready is true along with the snapshot state. Add focused coverage for each session_snapshot rejection guard in the reducer, exercising full reconnect scenarios and asserting invalid snapshots are rejected without replacing the existing canonical state.packages/web/src/hooks/use-session-transport.test.tsx (1)
115-136: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for
reconnect()without arguments.This test only exercises
reconnect(true). The unforced path is untested. A test that callsreconnect()and assertsforceSnapshot: falsewithresumeRevision: 7would fail if a caller accidentally supplies a truthy first argument.That failure mode is live today:
packages/web/src/app/(app)/session/[id]/session-client.tsxLine 331 passes the hookreconnectstraight toonClick, so aMouseEventbecomesforceSnapshot.💚 Proposed additional case
expect(FakeWebSocket.instances[1].sentMessages[0]).not.toHaveProperty("resumeRevision"); + + act(() => rendered.result.current.reconnect()); + await waitFor(() => expect(FakeWebSocket.instances).toHaveLength(3)); + act(() => FakeWebSocket.instances[2].open()); + expect(FakeWebSocket.instances[2].sentMessages[0]).toEqual( + expect.objectContaining({ viewProtocol: 2, resumeRevision: 7, forceSnapshot: false }) + ); });🤖 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/web/src/hooks/use-session-transport.test.tsx` around lines 115 - 136, Add a separate assertion path in the “sends the current resume revision and can force snapshot recovery” test that invokes rendered.result.current.reconnect() without arguments, opens the resulting websocket, and verifies the initial message contains forceSnapshot: false and resumeRevision: 7. Keep the existing reconnect(true) coverage unchanged.packages/web/src/hooks/use-session-socket.ts (2)
171-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the access-clearing sandbox status set into one shared constant.
The list
["spawning", "stale", "stopped", "failed"]is repeated here at Lines 175 and 181.packages/web/src/lib/session-socket/reducer.tsrepeats the same set twice more, inapplyDelta(Lines 184-188) and in thesandbox_statuscase (Lines 422-427). A new status value must be added in four places.Export one constant and a predicate from
packages/web/src/lib/session-socket/reducer.ts, then use it at all sites.♻️ Proposed shared predicate
// packages/web/src/lib/session-socket/reducer.ts export const ACCESS_CLEARING_SANDBOX_STATUSES = ["spawning", "stale", "stopped", "failed"] as const; export function clearsSandboxAccess(status: string | undefined): boolean { return ( status !== undefined && (ACCESS_CLEARING_SANDBOX_STATUSES as readonly string[]).includes(status) ); }- const clearsAccess = - message.type === "sandbox_spawning" || - message.type === "sandbox_error" || - (message.type === "sandbox_status" && - ["spawning", "stale", "stopped", "failed"].includes(message.status)) || - (message.type === "session_delta" && - message.delta.operations.some( - (operation) => - operation.type === "state_patch" && - operation.patch.sandboxStatus !== undefined && - ["spawning", "stale", "stopped", "failed"].includes(operation.patch.sandboxStatus) - )); + const clearsAccess = + message.type === "sandbox_spawning" || + message.type === "sandbox_error" || + (message.type === "sandbox_status" && clearsSandboxAccess(message.status)) || + (message.type === "session_delta" && + message.delta.operations.some( + (operation) => + operation.type === "state_patch" && clearsSandboxAccess(operation.patch.sandboxStatus) + ));🤖 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/web/src/hooks/use-session-socket.ts` around lines 171 - 183, Export ACCESS_CLEARING_SANDBOX_STATUSES and clearsSandboxAccess from reducer.ts, then replace every inline ["spawning", "stale", "stopped", "failed"] check in applyDelta, the sandbox_status case, and the use-session-socket clearsAccess logic with the shared predicate. Remove the duplicated status arrays while preserving the existing undefined and access-clearing behavior.
85-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deriving the resume revision only from reducer state.
revisionRefhas two writers. The effect at Line 97 copiesstate.lastAppliedRevision.handleMessagealso writes it directly at Lines 149, 155, and 158. When the reducer rejects a message,handleMessagecan still advance the ref, and the effect does not re-sync becausestate.lastAppliedRevisiondid not change.The current flow recovers, because every reducer rejection raises
recoveryNonceand triggersreconnect(true), and a forced snapshot omitsresumeRevision. The redundancy is still a latent hazard if the recovery path changes later.A single writer removes the hazard. Keep the effect and delete the direct writes in
handleMessage. The ref then always matches committed reducer state.🤖 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/web/src/hooks/use-session-socket.ts` around lines 85 - 99, Make the reducer state the sole writer of revisionRef: retain the useEffect that assigns state.lastAppliedRevision and remove all direct revisionRef.current assignments from handleMessage. Preserve the existing message handling and recovery behavior while ensuring rejected messages cannot advance the resume revision.packages/web/src/hooks/use-session-transport.ts (1)
202-217: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive the V2 message-type list from the shared contract.
This literal list must stay in step with the V2 message types in
@open-inspect/shared. If the shared package adds a revisioned message type and this list is not updated, a malformed frame of that type is dropped silently and the client keeps a stale revision without recovering.Export a set of revisioned message type names from the shared server-messages module and use it here.
♻️ Proposed change
// packages/shared/src/types/server-messages.ts export const REVISIONED_MESSAGE_TYPES = new Set([ "session_sync_started", "session_delta", "session_snapshot", "session_ready", "session_history_page", ]);- if ( - type === "session_sync_started" || - type === "session_delta" || - type === "session_snapshot" || - type === "session_ready" || - type === "session_history_page" - ) { + if (typeof type === "string" && REVISIONED_MESSAGE_TYPES.has(type)) { handlersRef.current.onProtocolError?.(); }🤖 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/web/src/hooks/use-session-transport.ts` around lines 202 - 217, Export a shared REVISIONED_MESSAGE_TYPES set from the server-messages module containing all revisioned V2 message names, then import and use it in the malformed-data handling of the session transport hook instead of the local literal type comparisons. Preserve invoking onProtocolError only when the extracted message type belongs to that shared set.packages/web/src/lib/session-socket/reducer.ts (1)
103-130: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBatch the upserts and use a locale-independent tie-break.
Two concerns in these helpers.
First, ordering.
localeCompareresolves against the runtime default locale and its collation data. Two browsers can order the same twoeventIdvalues differently. The client timeline then differs from the server order for events that share atimelineSequence. Use a plain comparison for opaque identifiers.Second, cost.
upsertViewEventre-sorts the whole array for each single item.session_history_pageat Lines 380-382 calls it in a loop, so a page of k items over n existing events costs O(k · n log n).withViewEventsthen remaps every event throughtoUiSandboxEvent.Add a batch upsert and sort once.
♻️ Proposed refactor
function sortedViewEvents(items: SessionViewEvent[]): SessionViewEvent[] { return [...items].sort( - (a, b) => a.timelineSequence - b.timelineSequence || a.eventId.localeCompare(b.eventId) + (a, b) => + a.timelineSequence - b.timelineSequence || + (a.eventId < b.eventId ? -1 : a.eventId > b.eventId ? 1 : 0) ); } +function upsertViewEvents( + items: SessionViewEvent[], + nextItems: SessionViewEvent[] +): SessionViewEvent[] { + if (nextItems.length === 0) return items; + const byId = new Map(items.map((item) => [item.eventId, item])); + for (const nextItem of nextItems) byId.set(nextItem.eventId, nextItem); + return sortedViewEvents([...byId.values()]); +} + function upsertViewEvent( items: SessionViewEvent[], nextItem: SessionViewEvent ): SessionViewEvent[] { - const existingIndex = items.findIndex((item) => item.eventId === nextItem.eventId); - if (existingIndex === -1) return sortedViewEvents([...items, nextItem]); - const next = [...items]; - next[existingIndex] = nextItem; - return sortedViewEvents(next); + return upsertViewEvents(items, [nextItem]); }Then replace the loop in the
session_history_pagecase:- let viewEvents = state.viewEvents; - for (const item of message.items) viewEvents = upsertViewEvent(viewEvents, item); + const viewEvents = upsertViewEvents(state.viewEvents, message.items);🤖 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/web/src/lib/session-socket/reducer.ts` around lines 103 - 130, Update sortedViewEvents to use a deterministic plain comparison for eventId ties instead of localeCompare. Add a batch upsert helper that applies all incoming SessionViewEvent items, resolves existing eventIds, and sorts once; have the session_history_page case use it instead of repeatedly calling upsertViewEvent. Ensure withViewEvents performs the final sort and toUiSandboxEvent mapping only once for the batch.
🤖 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 `@packages/control-plane/src/routes/session-runtime-proxy.ts`:
- Around line 243-248: Replace simpleProxyRoute for the /sessions/:id/bootstrap
route with the authentication-aware proxy that validates ctx.principal, while
preserving the existing SessionInternalPaths.bootstrap target and custom
“Session not found” response.
In `@packages/control-plane/src/session/durable-object.ts`:
- Around line 1565-1571: Update readBoundedViewCatchUp to measure each
serialized delta’s UTF-8 encoded byte length rather than
JSON.stringify(...).length before comparing the total with
MAX_VIEW_CATCH_UP_BYTES. Preserve the existing revision and null-result checks
and the byte-limit behavior.
- Around line 1966-1969: Update the bootstrap construction around
getSessionState so environmentName is resolved with resolveEnvironmentName
outside the synchronous transactionSync block instead of being hardcoded to
null. Thread the asynchronous result through sendV2Synchronization and
handleSubscribe as needed, while preserving null only when the environment has
been deleted; ensure handleBootstrap and WebSocket synchronization receive the
resolved name.
In `@packages/control-plane/src/session/repository.ts`:
- Around line 640-662: Extract the repository-state projection logic from
getSessionRepositoryStateProjection and SessionDO.getSessionRepositoryStates
into one shared function, then call it from both snapshot and live-delta paths.
Preserve the Durable Object semantics for repoId when a member row exists, and
filter out artifacts with null URLs before findPrArtifactForRepo so both callers
produce identical results.
- Around line 1694-1701: Update parseArtifactMetadata to catch JSON.parse
failures and return null for malformed metadata, while preserving the existing
object-validation behavior for valid JSON. Ensure artifactFromRow no longer
propagates parsing errors into the appendSessionViewDelta transaction.
- Around line 341-347: Wrap the periodic pruneSessionViewDeltas call in
appendSessionViewDelta with failure containment so pruning errors do not
propagate after the mutation is committed. Preserve the existing retention
arguments and always return the committed revision, including when pruning
fails.
In `@packages/control-plane/src/session/websocket-manager.ts`:
- Around line 286-292: Update advanceClientViewRevision to ignore revisions that
are less than or equal to the client’s current appliedViewRevision, and ensure
updateWsClientViewRevision only persists a revision when it is greater than the
stored value. Preserve the existing state lookup and client/repository update
flow for newer revisions.
In `@packages/control-plane/test/integration/session-bootstrap-v2.test.ts`:
- Line 11: Add an afterEach cleanup hook within the “session bootstrap and V2
synchronization” describe block to remove retained session fixtures or clear the
relevant shared D1 tables after every test, preventing state from leaking
between tests.
In `@packages/web/src/app/`(app)/session/[id]/session-client.tsx:
- Around line 717-725: Extract the 300-millisecond delay used by
handleInputChange into a module-level constant named to include its millisecond
unit, then use that constant in setTimeout instead of the bare literal.
In `@packages/web/src/hooks/use-session-socket.ts`:
- Around line 208-215: Update packages/web/src/hooks/use-session-socket.ts lines
208-215 around the recoveryNonce effect to track recovery attempts, apply
exponential backoff, enforce a maximum retry limit, and reset the counter when
state.ready becomes true. In packages/web/src/lib/session-socket/reducer.ts
lines 327-337, update the bootstrap state.id validation to compare against the
existing session id only when state.sessionState is non-null, and apply the same
guard to the session_ready validation around line 365. In
packages/web/src/hooks/use-session-transport.ts lines 344-369, preserve
reconnectAttempts.current = 0 for user-initiated reconnects but skip that reset
when forceSnapshot is true so recovery reconnects retain transport backoff.
- Line 352: Wrap the returned reconnect handler in a zero-argument function so
React click events cannot be passed as forceSnapshot; update
UseSessionSocketReturn and the hook return around reconnect while preserving the
internal recovery effect’s explicit reconnect(true) call.
In `@packages/web/src/lib/session-socket/swr-revalidation.ts`:
- Around line 41-50: Update the session_access_changed and session_ready
branches in the SWR revalidation key builder to use the shared sessionAccessKey
helper, ensuring the access key applies encodeURIComponent to sessionId
consistently with use-session-access.ts. If the helper cannot be imported from
the hook module, move it to a shared key module and reuse it from both
locations.
---
Outside diff comments:
In `@packages/control-plane/src/session/pull-request-service.ts`:
- Around line 102-128: Make all three view-delta methods required in
PullRequestRepository by removing their optional markers at
packages/control-plane/src/session/pull-request-service.ts:102-128. At
packages/control-plane/src/session/pull-request-service.ts:302-326, remove both
fallback branches and call updateSessionRepositoryBranchWithViewDelta and
updateSessionBranchWithViewDelta directly; at
packages/control-plane/src/session/pull-request-service.ts:373-376, remove the
?? fallback and call createArtifactWithViewDelta directly.
In `@packages/web/src/lib/session-socket/reducer.ts`:
- Around line 285-309: Reset lastAppliedRevision to its initial empty value in
the reducer branch that constructs the legacy protocol state alongside protocol:
"legacy" and the cleared viewEvents. Ensure the legacy fallback no longer
preserves the previous revision used by getResumeRevision and subsequent
subscribe frames.
---
Nitpick comments:
In `@packages/control-plane/src/routes/session-runtime-proxy.test.ts`:
- Around line 64-86: Add an assertion after the accepted response in the “keeps
session access client-only” test, verifying
accepted.headers.get("Cache-Control") equals "private, no-store".
In `@packages/control-plane/src/session/durable-object.ts`:
- Around line 1720-1742: Replace the `as ServerMessage` assertions in both
`safeSend` calls within the history-page branch of the session handler with
`satisfies ServerMessage`, preserving the existing `session_history_page` and
`history_page` payloads while enabling compile-time shape checking.
- Around line 1547-1562: Remove the two-attempt retry loop around
getSessionBootstrap and readBoundedViewCatchUp, making the snapshot path perform
one read and return its result directly. Preserve the existing null handling
when bootstrap or deltas are unavailable, and keep sendV2SyncMessages unchanged.
- Around line 1841-1845: Remove the unused updateSessionTitleIfUnset method and
its conditional-update path from the session title handling in
durable-object.ts. Ensure production callers continue using onlyIfUnset together
with updateSessionTitleWithViewDelta so title changes always produce the
matching view delta.
In `@packages/control-plane/src/session/event-stream.ts`:
- Around line 56-63: Reduce duplication in getReplay/getViewReplay and
getHistoryPage/getViewHistoryPage by extracting the shared
page.nextCursor-to-cursor mapping into a helper and reusing it in both read
paths, while keeping only the row-parsing function different. Define and use a
named return type for getViewHistoryPage, matching getHistoryPage’s
SessionHistoryPage pattern, so callers can reference its result type.
In `@packages/control-plane/src/session/repository.ts`:
- Around line 56-58: Rename the constant SESSION_VIEW_RETENTION_INTERVAL to
SESSION_VIEW_RETENTION_INTERVAL_REVISIONS and update every reference to it,
preserving its value and revision-count semantics.
- Around line 1434-1454: Extract the repeated event-row lookup and
timeline_sequence validation from createGitSyncEventWithViewDelta,
writeEventViewDelta, and createArtifactAndEventWithViewDelta into one private
helper returning SessionViewEvent, and reuse it when constructing event upsert
items. Since appendSessionViewDelta and writeEventViewDelta do not return null,
narrow writeEventViewDelta plus createEventWithViewDelta,
upsertTokenEventWithViewDelta, upsertToolCallEventWithViewDelta, and
upsertExecutionCompleteEventWithViewDelta from number | null to number.
- Around line 485-495: Update packages/control-plane/src/session/repository.ts
lines 485-495 and 538-548: make updateSessionBranch and updateSessionStatus
return whether exactly one row was affected, then have
updateSessionBranchWithViewDelta and updateSessionStatusWithViewDelta validate
that result and throw inside the transaction when it is not exactly one,
matching updateSessionTitleWithViewDelta.
In `@packages/control-plane/src/session/websocket-manager.ts`:
- Around line 247-283: Define shared named constants for the default client view
protocol and applied view revision, then use them in persistClientMapping and
getClientViewState for method defaults and recovered mapping fallbacks. Ensure
each default value is declared exactly once and reused consistently.
In `@packages/control-plane/test/integration/helpers.ts`:
- Around line 383-394: Update the comment above the collector in the
subscription helper to state that collection terminates on “subscribed” for
non-v2 clients and “session_ready” for protocol-v2 clients; leave the collector
predicate and surrounding logic unchanged.
In `@packages/web/src/app/`(app)/layout.test.tsx:
- Around line 27-47: Extend the “protected app layout” suite around AppLayout
with two tests: verify AuthenticationUnavailableError from getServerAuthSession
renders the authentication-unavailability banner, and verify any unrelated error
is rethrown. Import AuthenticationUnavailableError from the indicated
authentication-unavailable-error module and preserve the existing authenticated
and redirect expectations.
In `@packages/web/src/app/`(app)/session/[id]/error.tsx:
- Around line 6-25: Update SessionError to use the declared error: log it when
the component mounts and render error.digest when available so users can report
the identifier. Preserve the existing retry behavior and error banner messaging.
In `@packages/web/src/app/api/sessions/`[id]/access/route.ts:
- Around line 5-23: Wrap the authenticated proxy flow in GET with a single outer
try/catch, including the controlPlaneUserFetch call and response construction.
Return a structured HTTP 500 response from the catch block when transport or
other errors are thrown, while preserving the existing unauthorized response and
successful proxy status, headers, and body behavior.
In `@packages/web/src/hooks/use-session-socket.ts`:
- Around line 171-183: Export ACCESS_CLEARING_SANDBOX_STATUSES and
clearsSandboxAccess from reducer.ts, then replace every inline ["spawning",
"stale", "stopped", "failed"] check in applyDelta, the sandbox_status case, and
the use-session-socket clearsAccess logic with the shared predicate. Remove the
duplicated status arrays while preserving the existing undefined and
access-clearing behavior.
- Around line 85-99: Make the reducer state the sole writer of revisionRef:
retain the useEffect that assigns state.lastAppliedRevision and remove all
direct revisionRef.current assignments from handleMessage. Preserve the existing
message handling and recovery behavior while ensuring rejected messages cannot
advance the resume revision.
In `@packages/web/src/hooks/use-session-transport.test.tsx`:
- Around line 115-136: Add a separate assertion path in the “sends the current
resume revision and can force snapshot recovery” test that invokes
rendered.result.current.reconnect() without arguments, opens the resulting
websocket, and verifies the initial message contains forceSnapshot: false and
resumeRevision: 7. Keep the existing reconnect(true) coverage unchanged.
In `@packages/web/src/hooks/use-session-transport.ts`:
- Around line 202-217: Export a shared REVISIONED_MESSAGE_TYPES set from the
server-messages module containing all revisioned V2 message names, then import
and use it in the malformed-data handling of the session transport hook instead
of the local literal type comparisons. Preserve invoking onProtocolError only
when the extracted message type belongs to that shared set.
In `@packages/web/src/lib/session-socket/reducer.test.ts`:
- Around line 298-325: Update the test around the snapshot flow to capture and
assert the reducer state immediately after session_ready, before applying
socket_closed, verifying ready is true along with the snapshot state. Add
focused coverage for each session_snapshot rejection guard in the reducer,
exercising full reconnect scenarios and asserting invalid snapshots are rejected
without replacing the existing canonical state.
In `@packages/web/src/lib/session-socket/reducer.ts`:
- Around line 103-130: Update sortedViewEvents to use a deterministic plain
comparison for eventId ties instead of localeCompare. Add a batch upsert helper
that applies all incoming SessionViewEvent items, resolves existing eventIds,
and sorts once; have the session_history_page case use it instead of repeatedly
calling upsertViewEvent. Ensure withViewEvents performs the final sort and
toUiSandboxEvent mapping only once for the batch.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f360b30-0bbf-4632-808c-12648a719ac3
📒 Files selected for processing (52)
packages/control-plane/src/router.tspackages/control-plane/src/routes/session-runtime-proxy.test.tspackages/control-plane/src/routes/session-runtime-proxy.tspackages/control-plane/src/session/contracts.tspackages/control-plane/src/session/diffs/service.test.tspackages/control-plane/src/session/diffs/service.tspackages/control-plane/src/session/durable-object.tspackages/control-plane/src/session/event-stream.tspackages/control-plane/src/session/http/handlers/sandbox.handler.test.tspackages/control-plane/src/session/http/handlers/sandbox.handler.tspackages/control-plane/src/session/http/routes.test.tspackages/control-plane/src/session/http/routes.tspackages/control-plane/src/session/message-queue.test.tspackages/control-plane/src/session/message-queue.tspackages/control-plane/src/session/messenger.test.tspackages/control-plane/src/session/messenger.tspackages/control-plane/src/session/presence-service.test.tspackages/control-plane/src/session/pull-request-refresh.tspackages/control-plane/src/session/pull-request-service.tspackages/control-plane/src/session/repository.test.tspackages/control-plane/src/session/repository.tspackages/control-plane/src/session/sandbox-events.test.tspackages/control-plane/src/session/sandbox-events.tspackages/control-plane/src/session/schema.test.tspackages/control-plane/src/session/schema.tspackages/control-plane/src/session/session-status-service.test.tspackages/control-plane/src/session/session-status-service.tspackages/control-plane/src/session/websocket-manager.test.tspackages/control-plane/src/session/websocket-manager.tspackages/control-plane/src/types.tspackages/control-plane/test/integration/helpers.tspackages/control-plane/test/integration/session-bootstrap-v2.test.tspackages/control-plane/test/integration/session-view-deltas.test.tspackages/web/src/app/(app)/layout.test.tsxpackages/web/src/app/(app)/layout.tsxpackages/web/src/app/(app)/session/[id]/error.tsxpackages/web/src/app/(app)/session/[id]/loading.tsxpackages/web/src/app/(app)/session/[id]/page.tsxpackages/web/src/app/(app)/session/[id]/session-client.tsxpackages/web/src/app/api/sessions/[id]/access/route.tspackages/web/src/hooks/use-session-access.test.tsxpackages/web/src/hooks/use-session-access.tspackages/web/src/hooks/use-session-socket.tspackages/web/src/hooks/use-session-transport.test.tsxpackages/web/src/hooks/use-session-transport.tspackages/web/src/lib/auth-session.tsxpackages/web/src/lib/session-bootstrap.test.tspackages/web/src/lib/session-bootstrap.tspackages/web/src/lib/session-socket/reducer.test.tspackages/web/src/lib/session-socket/reducer.tspackages/web/src/lib/session-socket/swr-revalidation.test.tspackages/web/src/lib/session-socket/swr-revalidation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/control-plane/src/session/repository.test.ts
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
ColeMurray
changed the title
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/web/src/hooks/use-session-transport.ts (1)
186-195: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRecover from invalid server messages.
Line 191 drops schema-invalid payloads while keeping the socket open. A corrupt initial
subscribedmessage then leavesconnectedtrue andreadyfalse indefinitely. Live actions remain blocked until the user manually reconnects.Route JSON and schema failures through the bounded reconnect path. Do not silently ignore a failed synchronization message.
🤖 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/web/src/hooks/use-session-transport.ts` around lines 186 - 195, Update handleSocketMessage to route both JSON parsing and parseWsMessage schema failures, including the current !data case, through the existing bounded reconnect path instead of silently returning or only logging. Ensure an invalid initial subscribed message triggers recovery so connection state cannot remain connected but unready, while preserving normal onMessage handling for valid payloads.packages/control-plane/src/session/durable-object.ts (1)
1802-1812: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSet
Cache-Control: private, no-storeon the early 404 and 409 responses.Lines 1804, 1808, and 1811 return without cache headers. The success path and the conflict path at line 1830 both set
private, no-store. A cached 409 would pin a client to "Sandbox access is unavailable" after the sandbox reachesready.Hoist the header object and reuse it for every response in this handler.
🐛 Proposed fix
private async handleSessionAccess(): Promise<Response> { + const headers = { "Cache-Control": "private, no-store" }; if (!this.getSession()) { - return Response.json({ error: "Session not found" }, { status: 404 }); + return Response.json({ error: "Session not found" }, { status: 404, headers }); } const sandbox = this.getSandbox(); if (!sandbox) { - return Response.json({ error: "Sandbox access is unavailable" }, { status: 409 }); + return Response.json({ error: "Sandbox access is unavailable" }, { status: 409, headers }); } if (sandbox.status !== "ready" && sandbox.status !== "running") { - return Response.json({ error: "Sandbox access is unavailable" }, { status: 409 }); + return Response.json({ error: "Sandbox access is unavailable" }, { status: 409, headers }); }🤖 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/control-plane/src/session/durable-object.ts` around lines 1802 - 1812, Update handleSessionAccess to define a shared Cache-Control private, no-store header object and reuse it on the early 404 session-not-found response and both 409 sandbox-unavailable responses, matching the existing success and conflict response behavior.
🧹 Nitpick comments (1)
packages/control-plane/src/session/durable-object.ts (1)
1717-1725: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the decryption failure before returning
null.
decryptStoredAccessValueswallows every decryption error. The access endpoint then reportscodeServer: nullorttyd: null, which is indistinguishable from "no credential stored". A rotated or mismatchedREPO_SECRETS_ENCRYPTION_KEYwould silently disable code-server and terminal access with no operator signal.Add a warning log in the catch branch. Do not log the ciphertext or the plaintext.
♻️ Proposed change
} catch { + this.log.warn("Failed to decrypt stored sandbox access value"); 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/control-plane/src/session/durable-object.ts` around lines 1717 - 1725, Update decryptStoredAccessValue to log a warning in its decryption-error catch branch before returning null, including useful failure context while excluding both the encrypted ciphertext and decrypted plaintext.
🤖 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 `@docs/adr/0003-session-snapshot-handoff.md`:
- Around line 19-33: Restore the V2 revisioned synchronization contract: in
docs/adr/0003-session-snapshot-handoff.md lines 19-33, document bootstrap
revision resume, ordered V2 delta delivery, and snapshot recovery when a
revision gap is detected; in packages/control-plane/src/session/schema.test.ts
lines 246-252, replace assertions that remove revision and delta persistence
with assertions protecting the required revision and delta schema contract.
In `@packages/control-plane/test/integration/session-from-environment.test.ts`:
- Around line 63-66: Update getSessionState to assert response.ok after fetching
/internal/bootstrap, then validate the parsed bootstrap payload before accessing
and returning state. Ensure endpoint errors and malformed responses fail at this
helper with a clear assertion rather than relying on the TypeScript JSON type
assertion.
In `@packages/shared/src/types/server-messages.ts`:
- Around line 73-98: Update the live sandbox event schema and inferred type
around the sandbox_event server message to use the same timeline envelope as
sessionTimelineEventSchema, including eventId, timelineSequence, and event.
Preserve sandbox event validation while ensuring live events expose stable
identities consistent with bootstrap and history events.
- Around line 117-130: Update the subscribed branch of serverMessageSchema to
refine the parsed object so its sessionId equals state.id, matching
sessionBootstrapSchema; reject mismatches while preserving valid messages. Add a
regression test covering a subscribed message with differing identities and
assert validation fails.
In `@packages/web/src/hooks/use-session-socket.ts`:
- Around line 72-80: Update useSessionSocket to reset all session-scoped state
when sessionId changes, including the sessionSocketReducer state, pending prompt
ref, and subscription waiters; ensure the new state is initialized from the
current initialBootstrap while preserving existing behavior within one session.
In `@packages/web/src/lib/session-socket/reducer.ts`:
- Around line 81-95: Normalize replay token events consistently in
createSessionSocketState and the history_page ingestion path by applying
collapseReplayTokenEvents after converting and sorting timeline items, matching
the subscribed path. Ensure timelineEvents and derived events use the collapsed
results, and add coverage for history pages containing multiple token events.
---
Outside diff comments:
In `@packages/control-plane/src/session/durable-object.ts`:
- Around line 1802-1812: Update handleSessionAccess to define a shared
Cache-Control private, no-store header object and reuse it on the early 404
session-not-found response and both 409 sandbox-unavailable responses, matching
the existing success and conflict response behavior.
In `@packages/web/src/hooks/use-session-transport.ts`:
- Around line 186-195: Update handleSocketMessage to route both JSON parsing and
parseWsMessage schema failures, including the current !data case, through the
existing bounded reconnect path instead of silently returning or only logging.
Ensure an invalid initial subscribed message triggers recovery so connection
state cannot remain connected but unready, while preserving normal onMessage
handling for valid payloads.
---
Nitpick comments:
In `@packages/control-plane/src/session/durable-object.ts`:
- Around line 1717-1725: Update decryptStoredAccessValue to log a warning in its
decryption-error catch branch before returning null, including useful failure
context while excluding both the encrypted ciphertext and decrypted plaintext.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fe569bf6-52b7-415e-9667-c09a065a08a5
📒 Files selected for processing (34)
docs/adr/0003-session-snapshot-handoff.mdpackages/control-plane/src/sandbox/lifecycle/manager.tspackages/control-plane/src/session/durable-object.tspackages/control-plane/src/session/event-stream.test.tspackages/control-plane/src/session/event-stream.tspackages/control-plane/src/session/http/handlers/sandbox.handler.test.tspackages/control-plane/src/session/http/handlers/sandbox.handler.tspackages/control-plane/src/session/message-queue.test.tspackages/control-plane/src/session/repository.test.tspackages/control-plane/src/session/repository.tspackages/control-plane/src/session/sandbox-events.test.tspackages/control-plane/src/session/sandbox-events.tspackages/control-plane/src/session/schema.test.tspackages/control-plane/src/session/websocket-manager.tspackages/control-plane/test/integration/create-pr.test.tspackages/control-plane/test/integration/helpers.tspackages/control-plane/test/integration/session-bootstrap.test.tspackages/control-plane/test/integration/session-from-environment.test.tspackages/control-plane/test/integration/websocket-client.test.tspackages/shared/src/types/boundary-schemas.test.tspackages/shared/src/types/index.tspackages/shared/src/types/server-messages.test.tspackages/shared/src/types/server-messages.tspackages/shared/src/types/type-contracts.test.tspackages/web/src/app/(app)/session/[id]/session-client.tsxpackages/web/src/hooks/use-session-socket.test.tsxpackages/web/src/hooks/use-session-socket.tspackages/web/src/hooks/use-session-transport.test.tsxpackages/web/src/hooks/use-session-transport.tspackages/web/src/lib/session-bootstrap.test.tspackages/web/src/lib/session-socket/reducer.test.tspackages/web/src/lib/session-socket/reducer.tspackages/web/src/lib/session-socket/swr-revalidation.test.tspackages/web/src/lib/session-socket/swr-revalidation.ts
💤 Files with no reviewable changes (2)
- packages/web/src/hooks/use-session-transport.test.tsx
- packages/control-plane/src/session/message-queue.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/web/src/lib/session-bootstrap.test.ts
- packages/shared/src/types/type-contracts.test.ts
- packages/control-plane/src/session/http/handlers/sandbox.handler.test.ts
- packages/control-plane/src/session/sandbox-events.test.ts
- packages/web/src/lib/session-socket/swr-revalidation.test.ts
- packages/web/src/lib/session-socket/reducer.test.ts
- packages/web/src/app/(app)/session/[id]/session-client.tsx
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
1 participant
Summary
GET /sessions/:idfor server renderingSessionSnapshotshape on every WebSocket subscribe or reconnect, then continue with the existing semantic live messageseventIdandtimelineSequenceenvelopes for deterministic replay and history pagination/sessions/:id/sandbox-accessendpointThe canonical representation is
{ session, artifacts, timeline, spawnError }. It has no duplicate session ID, bootstrap lifecycle terminology, or parallel public state endpoint. Sandbox access is explicitly limited to interactive sandbox services; integration and SCM credentials retain their own domain-specific flows.Simplified architecture
GET /sessions/:idon the server.subscribed, registers the socket, and persists its identity without anawaitbetween those operations.Removed from the previous design
session_view_metadataandsession_view_deltasstorage/sessions/:id/bootstrapresource, duplicatesessionId, and identity refinements/sessions/:id/accessname andsession_access_changedprotocol eventThe simplification passes reduce the review surface from 4,147 changed lines across 51 files to 1,975 lines across 41 files: 2,172 fewer changed lines (52%).
Correctness and security
Verification
git diff --checkThe client and control-plane protocol changes are intentionally coordinated; no legacy compatibility path remains.