From e18f53a39bdc0c485381193b43e02d4361b447d9 Mon Sep 17 00:00:00 2001 From: Henry Chu Date: Thu, 5 Nov 2026 13:43:37 -0500 Subject: [PATCH] Fix low findings from the Fable review of the 337c5858c3 sync - Cut the client half of upstream's socket frame parts (#23758) and shared socketFrameParts.ts: the split lives on upstream's uncarried /ws upgrade and the gateway forwards whole helper stdio frames. - Drop the unused KiroIcon. - Seed the server-config provider stream and the synthesized lifecycle stream with rpcInitialItems, as upstream does, so delivered snapshots are released. - mapSessionRpcError maps settings errors to upstream's remote-unavailable reason. - Carry upstream's subscription-defect resubscribe test (#31071). - Seams doc: useOpenLink.ts entry, frame-parts and KiroIcon cuts; sync.md lists useOpenLink.ts as kept in web browser/. - Port upstream's rich-text marker editing paragraph to composer.md. - The fork-only worktree rename refusal says T3 Coder. Co-Authored-By: Claude Opus 5.5 --- apps/server/src/coderWs.ts | 2 +- apps/server/src/ws.ts | 6 +- apps/web/src/components/Icons.tsx | 18 ---- docs/internals/coder-only.md | 8 +- docs/internals/sync.md | 4 +- docs/user/composer.md | 11 ++- knip.jsonc | 1 - .../client-runtime/src/rpc/client.test.ts | 90 ++++++++++++++++++- .../client-runtime/src/rpc/session.test.ts | 31 ------- packages/client-runtime/src/rpc/session.ts | 21 ++--- packages/shared/package.json | 4 - packages/shared/src/socketFrameParts.ts | 81 ----------------- scripts/sync-resolve-deleted.mjs | 3 + 13 files changed, 118 insertions(+), 162 deletions(-) delete mode 100644 packages/shared/src/socketFrameParts.ts diff --git a/apps/server/src/coderWs.ts b/apps/server/src/coderWs.ts index 1259b8fc88685..b053bb0b1dfad 100644 --- a/apps/server/src/coderWs.ts +++ b/apps/server/src/coderWs.ts @@ -232,7 +232,7 @@ export const makeHandlers = Effect.fn("makeHandlers")(function* ({ path.dirname(currentParent) !== worktreesRoot ) { return yield* renameThreadBranchError( - "Only worktree folders managed by T3 Code can be renamed from the UI.", + "Only worktree folders managed by T3 Coder can be renamed from the UI.", ); } diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts index 9d384bf74b1a3..337e6a54a2266 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -2157,7 +2157,7 @@ const layerWsRpc = () => workspaceSnapshotsOnDemand ? withoutWorkspaceSnapshots(providers) : providers, ), // Compare initial paired values with the snapshot; blindly dropping them loses changes between snapshot and subscription. - (updates) => Stream.concat(Stream.make(config.providers), updates), + (updates) => Stream.concat(rpcInitialItems([config.providers]), updates), Stream.changesWith( (previous, next) => JSON.stringify(previous) === JSON.stringify(next), ), @@ -2249,7 +2249,7 @@ const layerWsRpc = () => Stream.flatMap((event) => Stream.fromIterable(migrationEvent(event))), ); return Stream.concat( - Stream.make( + rpcInitialItems([ { version: 1 as const, sequence: 1, @@ -2269,7 +2269,7 @@ const layerWsRpc = () => payload: { at: yield* nowIso, environment: environment.descriptor }, }, ...snapshotEvents, - ), + ]), liveEvents, ); }), diff --git a/apps/web/src/components/Icons.tsx b/apps/web/src/components/Icons.tsx index 551312e23f1dc..f4eef6a6d82cb 100644 --- a/apps/web/src/components/Icons.tsx +++ b/apps/web/src/components/Icons.tsx @@ -146,24 +146,6 @@ export const TraeIcon: Icon = (props) => ( ); -export const KiroIcon: Icon = (props) => ( - - - - - - -); - export const VisualStudioCode: Icon = (props) => { const id = useId(); const maskId = `${id}-vscode-a`; diff --git a/docs/internals/coder-only.md b/docs/internals/coder-only.md index 5dd17c69e1b8e..d358d31ddc814 100644 --- a/docs/internals/coder-only.md +++ b/docs/internals/coder-only.md @@ -814,6 +814,9 @@ checks that every name here still exists and that every modified upstream file i its primary-environment factory, is not carried. - client-runtime's `threadSnapshotHttp.ts`, `shellSnapshotHttp.ts`, and `threadHistoryHttp.ts` keep upstream's paths but load over helper stdio; shell snapshots use upstream's socket fallback. +- client-runtime's `rpc/session.ts` opens the socket without upstream's socket frame parts + (#23758) and `shared/socketFrameParts.ts` is not carried: the gateway forwards whole helper stdio + frames, and the split lives on upstream's uncarried `/ws` upgrade. - client-runtime's `rpc/protocol.ts` builds its client from `CoderWsRpcGroup`, and `rpc/index.ts` exports it without the HTTP client; `client-runtime/environment/index.ts` and `client-runtime/platform/index.ts` export no endpoint, descriptor, capability, or storage document @@ -862,6 +865,9 @@ checks that every name here still exists and that every modified upstream file i ### Web chat, composer, and files +- `browser/useOpenLink.ts` is the only carried file of web's cut `browser/`: upstream's link opener + without the in-app browser preview, sending every link to the system browser through the + HTTP(S)-only `shell.openExternal`. - `ChatView.tsx`: no browser or device panels, mini-player, automatic machine placement, self-update or version-skew banners, usage-limit panel, feedback upload, `OpenInPicker`, or favicon store; the active workspace stands in for the primary environment and drafts read their @@ -1125,7 +1131,7 @@ them, the file is named here. `window.location.origin` where upstream calls `configuredHostedAppUrl`, and shared `threadLinks.ts` declares `DEFAULT_HOSTED_APP_URL` (upstream's `connectAuth.ts` is cut), so upstream's hosted links still parse. Cloud link state (`cloud/cloudLinkState.ts`) is cut from - `ScheduledTasksSettings.tsx` and `useAvailableSettingsSearchItems.ts`. `Icons.tsx` drops `OnePasswordIcon`. + `ScheduledTasksSettings.tsx` and `useAvailableSettingsSearchItems.ts`. `Icons.tsx` drops `OnePasswordIcon` and `KiroIcon`. `media/mediaContent.ts`'s `fetchMediaBlob` has no desktop native read after a failed fetch (#29079). - **Auth, pairing, and relay.** No HTTP/WebSocket listener, session scopes, pairing, DPoP, relay, diff --git a/docs/internals/sync.md b/docs/internals/sync.md index e51780dcbd7bc..46d8d53119dab 100644 --- a/docs/internals/sync.md +++ b/docs/internals/sync.md @@ -192,8 +192,8 @@ mobile's `microphonePriority.ts` into `packages/shared`) arrives as an added fil (top-level directories in brackets): - Auth and pairing (`apps/server/src/auth`, web `components/auth`, client-runtime `authorization`). -- Preview and browser (`apps/server/src/preview` except `PortScanner.ts`, web `browser`, - `components/preview`). +- Preview and browser (`apps/server/src/preview` except `PortScanner.ts`, web `browser` except + `useOpenLink.ts`, `components/preview`). - Device (`apps/server/src/device`, web `components/device`, client-runtime `device`). - HTTP routes (`http.ts` route files other than the server's signed asset route in `apps/server/src/http.ts`, and the uncarried `assets/` modules). diff --git a/docs/user/composer.md b/docs/user/composer.md index 80b81c5b5761b..bf14cf8cb63d5 100644 --- a/docs/user/composer.md +++ b/docs/user/composer.md @@ -36,9 +36,14 @@ conversation. ## Rich text and multiple models -The composer formats supported Markdown as you type. Turn off **Rich text composer** in -**Settings → Chat** to show formatting markers literally. File mentions, skills, images, -terminal context, and review comments remain editable context chips in either mode. +The composer formats supported Markdown as you type. When your cursor is on styled text, its +Markdown shows instead, such as `**bold**`, and the markers edit like any other text: delete them +to remove the style, or change `**` to `*` for italic. The style updates when your cursor leaves. +Some styled text containing or touching literal Markdown characters, such as a bold `path\` or +`2*3`, keeps its styling without showing markers; use formatting shortcuts or turn off rich text to +edit those ranges. Turn off **Rich text composer** in **Settings → Chat** to show formatting +markers literally. File mentions, skills, images, terminal context, and review comments remain +editable context chips in either mode. In a new draft, select multiple models to send the same prompt to separate background threads. Each selected model gets its own new worktree in the current Coder workspace. Select a base branch diff --git a/knip.jsonc b/knip.jsonc index f199370983e89..86f89aef9c931 100644 --- a/knip.jsonc +++ b/knip.jsonc @@ -127,7 +127,6 @@ "packages/shared/src/orchestrationV2PendingBackgroundWork.ts": ["exports", "nsExports"], "packages/shared/src/serverSettings.ts": ["exports", "nsExports"], "packages/shared/src/shell.ts": ["exports", "nsExports"], - "packages/shared/src/socketFrameParts.ts": ["exports", "nsExports"], "packages/shared/src/sourceControl.ts": ["exports", "nsExports"], "packages/shared/src/t3McpToolPresentation.ts": ["exports", "nsExports"], "packages/shared/src/themePalettes.ts": ["exports", "nsExports"], diff --git a/packages/client-runtime/src/rpc/client.test.ts b/packages/client-runtime/src/rpc/client.test.ts index b09a1292fc954..3329f7de06db5 100644 --- a/packages/client-runtime/src/rpc/client.test.ts +++ b/packages/client-runtime/src/rpc/client.test.ts @@ -1,5 +1,6 @@ import { EnvironmentId, GitManagerError, WS_METHODS } from "@t3tools/contracts"; import { describe, expect, it } from "@effect/vitest"; +import * as Cause from "effect/Cause"; import * as Effect from "effect/Effect"; import * as Deferred from "effect/Deferred"; import * as Fiber from "effect/Fiber"; @@ -7,6 +8,7 @@ import * as Option from "effect/Option"; import * as Ref from "effect/Ref"; import * as Stream from "effect/Stream"; import * as SubscriptionRef from "effect/SubscriptionRef"; +import * as TestClock from "effect/testing/TestClock"; import { AVAILABLE_CONNECTION_STATE, @@ -17,7 +19,13 @@ import { import * as EnvironmentSupervisor from "../connection/supervisor.ts"; import type { WsRpcProtocolClient } from "./protocol.ts"; import type * as RpcSession from "./session.ts"; -import { requestGuarded, RpcPermissionGuard, runStreamGuarded, subscribe } from "./client.ts"; +import { + requestGuarded, + RpcPermissionGuard, + runStreamGuarded, + subscribe, + subscribeDynamicWithSession, +} from "./client.ts"; import { setErrorDiagnosticReporter, type ErrorDiagnostic } from "../errors/diagnostics.ts"; const TARGET = new ConnectionTarget({ @@ -27,6 +35,18 @@ const TARGET = new ConnectionTarget({ wsBaseUrl: "wss://environment.example.test", }); +function session(client: WsRpcProtocolClient): RpcSession.RpcSession { + return { + client, + initialConfig: Effect.never, + subscribeServerConfig: (input = {}) => client.subscribeServerConfig(input), + ready: Effect.void, + probe: Effect.void, + inboundFrames: Effect.succeed(0), + closed: Effect.never, + }; +} + const makeHarness = Effect.fn("TestEnvironmentRpc.makeHarness")(function* () { const state = yield* SubscriptionRef.make(AVAILABLE_CONNECTION_STATE); const activeSession = yield* SubscriptionRef.make>( @@ -121,4 +141,72 @@ describe("environment RPC", () => { expect(JSON.stringify(diagnostics)).not.toContain("/private-repo"); }), ); + + it.effect.each(["input", "stream"] as const)( + "reports %s subscription defects and resubscribes on the next session", + (where) => + Effect.gen(function* () { + const defect = new Error("subscription invariant failed"); + let expectedFailureCount = 0; + let inputs = 0; + let streams = 0; + const observedDefects: unknown[] = []; + const defectObserved = yield* Deferred.make(); + const resubscribed = yield* Deferred.make(); + const firstClient = { + [WS_METHODS.subscribeTerminalMetadata]: () => { + streams += 1; + return where === "stream" ? Stream.die(defect) : Stream.never; + }, + } as unknown as WsRpcProtocolClient; + const secondClient = { + [WS_METHODS.subscribeTerminalMetadata]: () => { + Deferred.doneUnsafe(resubscribed, Effect.void); + return Stream.never; + }, + } as unknown as WsRpcProtocolClient; + const { activeSession, supervisor } = yield* makeHarness(); + + yield* SubscriptionRef.set(activeSession, Option.some(session(firstClient))); + const subscriptionFiber = yield* subscribeDynamicWithSession( + WS_METHODS.subscribeTerminalMetadata, + () => + Effect.suspend(() => { + inputs += 1; + return where === "input" && inputs === 1 ? Effect.die(defect) : Effect.succeed({}); + }), + { + onDefect: (cause) => + Effect.sync(() => { + observedDefects.push(Cause.squash(cause)); + Deferred.doneUnsafe(defectObserved, Effect.void); + }), + onExpectedFailure: () => + Effect.sync(() => { + expectedFailureCount += 1; + }), + retryExpectedFailureAfter: "250 millis", + }, + ).pipe( + Stream.runDrain, + Effect.provideService(EnvironmentSupervisor.EnvironmentSupervisor, supervisor), + Effect.forkChild, + ); + yield* Deferred.await(defectObserved); + + expect(inputs).toBe(1); + expect(streams).toBe(where === "input" ? 0 : 1); + expect(expectedFailureCount).toBe(0); + expect(observedDefects).toEqual([defect]); + + // Let the defect settle before the next session arrives, as it does on a real device. + yield* TestClock.adjust("1 second"); + expect(subscriptionFiber.pollUnsafe()).toBeUndefined(); + yield* SubscriptionRef.set(activeSession, Option.some(session(secondClient))); + yield* Deferred.await(resubscribed); + yield* Fiber.interrupt(subscriptionFiber); + + expect(inputs).toBe(2); + }), + ); }); diff --git a/packages/client-runtime/src/rpc/session.test.ts b/packages/client-runtime/src/rpc/session.test.ts index 512f728b15f39..5e34bf3aff2e4 100644 --- a/packages/client-runtime/src/rpc/session.test.ts +++ b/packages/client-runtime/src/rpc/session.test.ts @@ -5,7 +5,6 @@ import { type ServerConfig as ServerConfigType, WS_METHODS, } from "@t3tools/contracts"; -import { splitSocketFrame } from "@t3tools/shared/socketFrameParts"; import { describe, expect, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; import * as Fiber from "effect/Fiber"; @@ -183,36 +182,6 @@ const completeInitialConfig = Effect.fn("TestRpcSessionFactory.completeInitialCo }); describe("RpcSessionFactory", () => { - it.effect("connects while a large config snapshot downloads at 4000 bytes per second", () => - Effect.gen(function* () { - const { factory, sockets } = yield* makeFactory(); - const session = yield* factory.connect(PREPARED); - const readyFiber = yield* Effect.forkChild(session.ready); - const socket = yield* awaitSocket(sockets); - socket.open(); - const request = yield* awaitRequest(socket); - // Multi-byte text, so parts must be sized in UTF-8 bytes and never split a surrogate pair. - const config = { ...SERVER_CONFIG, cwd: "/" + "é€😀x".repeat(32 * 1024) }; - const frame = encodeJson({ - _tag: "Chunk", - requestId: request.id, - values: [{ version: 1, type: "snapshot", config: encodeServerConfig(config) }], - }); - const parts = new URL(socket.url).searchParams.has("frameParts") - ? splitSocketFrame(frame) - : [frame]; - - for (const part of parts) { - const bytes = new TextEncoder().encode(part); - yield* TestClock.adjust((bytes.length / 4000) * 1000); - socket.serverMessage(new TextDecoder().decode(bytes)); - } - yield* Fiber.join(readyFiber); - - expect(yield* session.initialConfig).toEqual(config); - }), - ); - it.effect("uses one config subscription for initial sync", () => Effect.scoped( Effect.gen(function* () { diff --git a/packages/client-runtime/src/rpc/session.ts b/packages/client-runtime/src/rpc/session.ts index e3eb44c588315..cdb23b162080d 100644 --- a/packages/client-runtime/src/rpc/session.ts +++ b/packages/client-runtime/src/rpc/session.ts @@ -4,10 +4,6 @@ import { WsSubscribeServerConfigRpc, WS_METHODS, } from "@t3tools/contracts"; -import { - joinSocketFrameParts, - SOCKET_FRAME_PARTS_QUERY_PARAM, -} from "@t3tools/shared/socketFrameParts"; import * as Cause from "effect/Cause"; import * as Context from "effect/Context"; import * as Deferred from "effect/Deferred"; @@ -122,7 +118,7 @@ function mapSessionRpcError( case "KeybindingsConfigParseError": case "ServerSettingsError": return new ConnectionTransientErrorClass({ - reason: "transport", + reason: "remote-unavailable", detail: error.message, }); case "RpcClientError": @@ -180,17 +176,10 @@ const make = Effect.gen(function* () { }); return socket; }; - const socketUrl = new URL(connection.socketUrl); - socketUrl.searchParams.set(SOCKET_FRAME_PARTS_QUERY_PARAM, "1"); - const layerSocket = Layer.effect( - Socket.Socket, - Effect.map(Effect.service(Socket.Socket), joinSocketFrameParts), - ).pipe( - Layer.provide( - Socket.layerWebSocket(socketUrl.toString(), { - openTimeout: SOCKET_OPEN_TIMEOUT, - }), - ), + // Coder: no socket frame parts (#23758); the gateway forwards whole helper stdio frames. + const layerSocket = Socket.layerWebSocket(connection.socketUrl, { + openTimeout: SOCKET_OPEN_TIMEOUT, + }).pipe( Layer.provide(Layer.succeed(Socket.WebSocketConstructor, countingWebSocketConstructor)), ); const layerProtocol = Layer.effect( diff --git a/packages/shared/package.json b/packages/shared/package.json index 51156ddac620e..cb84df5ac077d 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -200,10 +200,6 @@ "types": "./src/delimitedPreview.ts", "import": "./src/delimitedPreview.ts" }, - "./socketFrameParts": { - "types": "./src/socketFrameParts.ts", - "import": "./src/socketFrameParts.ts" - }, "./thirdPartyLicenses": { "types": "./src/thirdPartyLicenses.ts", "import": "./src/thirdPartyLicenses.ts" diff --git a/packages/shared/src/socketFrameParts.ts b/packages/shared/src/socketFrameParts.ts deleted file mode 100644 index e0c82a87de3bb..0000000000000 --- a/packages/shared/src/socketFrameParts.ts +++ /dev/null @@ -1,81 +0,0 @@ -import * as Arr from "effect/Array"; -import * as Effect from "effect/Effect"; -import * as Socket from "effect/socket/Socket"; - -// Old clients omit this; old servers ignore it. -export const SOCKET_FRAME_PARTS_QUERY_PARAM = "frameParts"; - -// WebSocket messages arrive whole. At 4,000 bytes/s each part takes about -// 12 seconds, leaving margin before the client's 15-second liveness timeout. -const SOCKET_FRAME_PART_BYTES = 48 * 1024; - -// No JSON message starts with a record separator. It marks every part but the last. -const PART_PREFIX = "\u001e"; - -// The RPC client only counts frames that decode to messages as liveness, and -// ignores pongs, so each held-back part reads to it as one. -const PART_RECEIVED_FRAME = '{"_tag":"Pong"}'; - -/** Splits on the UTF-8 size the frame has on the wire, never inside a surrogate pair. */ -export function splitSocketFrame( - frame: string, - partBytes = SOCKET_FRAME_PART_BYTES, -): Arr.NonEmptyReadonlyArray { - // UTF-8 spends at most 3 bytes on a UTF-16 code unit. - if (frame.length * 3 <= partBytes) return [frame]; - const parts: Array = []; - let start = 0; - let bytes = PART_PREFIX.length; - for (let index = 0; index < frame.length;) { - const code = frame.charCodeAt(index); - const pair = code >= 0xd800 && code <= 0xdbff && index + 1 < frame.length; - const size = pair ? 4 : code < 0x80 ? 1 : code < 0x800 ? 2 : 3; - if (bytes + size > partBytes) { - parts.push(PART_PREFIX + frame.slice(start, index)); - start = index; - bytes = PART_PREFIX.length; - } - bytes += size; - index += pair ? 2 : 1; - } - return Arr.append(parts, frame.slice(start)); -} - -/** Server side: sends string frames longer than one part as consecutive parts. */ -export function splitLargeSocketFrames(socket: Socket.Socket): Socket.Socket { - const split = (chunk: string | Uint8Array): Arr.NonEmptyReadonlyArray => - typeof chunk === "string" ? splitSocketFrame(chunk) : [chunk]; - return Socket.make({ - reader: socket.reader, - writer: Effect.map(socket.writer, (writer) => ({ - write: (chunk) => - typeof chunk === "string" ? writer.writeAll(splitSocketFrame(chunk)) : writer.write(chunk), - writeAll: (chunks) => writer.writeAll(Arr.flatMap(chunks, split)), - })), - }); -} - -/** Client side: joins parts back into whole frames. */ -export function joinSocketFrameParts(socket: Socket.Socket): Socket.Socket { - return Socket.make({ - reader: Effect.map(socket.reader, (reader) => { - let pending = ""; - const join = (frame: string | Uint8Array) => { - if (typeof frame !== "string") return frame; - if (frame.startsWith(PART_PREFIX)) { - pending += frame.slice(PART_PREFIX.length); - return PART_RECEIVED_FRAME; - } - if (pending === "") return frame; - const whole = pending + frame; - pending = ""; - return whole; - }; - return { - upgrade: reader.upgrade, - pull: Effect.map(reader.pull, Arr.map(join)), - }; - }), - writer: socket.writer, - }); -} diff --git a/scripts/sync-resolve-deleted.mjs b/scripts/sync-resolve-deleted.mjs index b39d9680a21f0..d54ad7da06e90 100644 --- a/scripts/sync-resolve-deleted.mjs +++ b/scripts/sync-resolve-deleted.mjs @@ -98,6 +98,9 @@ const DELETED_PATHS = { "apps/server/src/scheduledTasks/webhookRoute.ts", "packages/client-runtime/src/rpc/http.ts", "packages/contracts/src/environmentHttp.ts", + // Socket frame parts (#23758) split frames on upstream's /ws upgrade; the gateway forwards + // whole helper stdio frames. + "packages/shared/src/socketFrameParts.ts", ], // Hosted source-control providers: GitLab is the only one carried. "Forgejo, GitHub, Azure DevOps, Bitbucket, and GitCafe packages": [