From 3c31b2b73ed3433b9888a8e88a87d60260b9e4b0 Mon Sep 17 00:00:00 2001 From: Helge Tesdal Date: Mon, 27 Apr 2026 20:09:39 +0200 Subject: [PATCH] refactor(provider, message-v2): unify SSEStallError on MessageV2 schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the duplicate `class SSEStallError extends Error` from provider.ts and have wrapSSE() throw `MessageV2.SSEStallError` (the schema-backed error) directly. Spike confirmed identity survives in-process rethrow paths (throw/catch, AbortController.abort, AbortSignal.any composition, ReadableStream.cancel, Promise.reject, end-to-end wrapSSE shape — 6/6). Kept hasSSEStallCause + SSE_STALL_MESSAGE_RE (hardened in F4) as the cross-realm safety net. The substring fallback defends rethrow paths that strip Error name/_tag (vendor SDK rewrapping, structured-clone boundaries) and is independent of which class is thrown. Added extractStallMessage() helper because the schema-error class sets `.message` to its tag (via `super(tag, options)`), so fromError can no longer copy `.message` directly when the input is already a schema instance — must read `.data.message`. Walks cause chain so nested wrappers still produce the original timing text. Updated test files to construct `MessageV2.SSEStallError` directly instead of the deleted runtime class. Net delta: -11 / +40 LOC. Eliminates one of two SSEStallError classes; remaining duplication is the substring fallback regex which intentionally defends a different threat model than the class identity. Addresses audit finding F9 (codex-5.3 diamond review, 2026-04-22). --- packages/opencode/src/provider/provider.ts | 14 +++------ packages/opencode/src/session/message-v2.ts | 31 +++++++++++++++++-- .../test/provider/chunk-timeout.test.ts | 16 ++++++++-- .../test/session/message-v2-sse-stall.test.ts | 20 ++++++++++-- packages/opencode/test/session/retry.test.ts | 3 +- 5 files changed, 65 insertions(+), 19 deletions(-) diff --git a/packages/opencode/src/provider/provider.ts b/packages/opencode/src/provider/provider.ts index 1651761b2ca6..b943c013b655 100644 --- a/packages/opencode/src/provider/provider.ts +++ b/packages/opencode/src/provider/provider.ts @@ -29,17 +29,13 @@ import { withStatics } from "@/util/schema" import * as ProviderTransform from "./transform" import { ModelID, ProviderID } from "./schema" +// message-v2 imports ProviderError directly from @/provider/error (not the +// @/provider barrel), so this back-edge does not form a cycle through +// provider.ts. Do not change the import in message-v2.ts back to the barrel. +import { MessageV2 } from "@/session/message-v2" const log = Log.create({ service: "provider" }) -export class SSEStallError extends Error { - readonly _tag = "SSEStallError" - constructor(message: string) { - super(message) - this.name = "SSEStallError" - } -} - function shouldUseCopilotResponsesApi(modelID: string): boolean { const match = /^gpt-(\d+)/.exec(modelID) if (!match) return false @@ -77,7 +73,7 @@ export function wrapSSE(res: Response, ms: number, ctl: AbortController) { async pull(ctrl) { const part = await new Promise>>((resolve, reject) => { const id = setTimeout(() => { - const err = new SSEStallError(`SSE read timed out after ${ms}ms`) + const err = new MessageV2.SSEStallError({ message: `SSE read timed out after ${ms}ms` }) ctl.abort(err) void reader.cancel(err) reject(err) diff --git a/packages/opencode/src/session/message-v2.ts b/packages/opencode/src/session/message-v2.ts index 1878571288d6..f2755eb734af 100644 --- a/packages/opencode/src/session/message-v2.ts +++ b/packages/opencode/src/session/message-v2.ts @@ -8,7 +8,10 @@ import { Snapshot } from "@/snapshot" import { SyncEvent } from "../sync" import { Database, NotFoundError, and, desc, eq, inArray, lt, or } from "@/storage" import { MessageTable, PartTable, SessionTable } from "./session.sql" -import { ProviderError } from "@/provider" +// Import directly from provider/error (not the @/provider barrel) to avoid a +// circular import: provider/provider.ts now imports MessageV2 from this file, +// and the barrel re-exports provider.ts. +import * as ProviderError from "@/provider/error" import { iife } from "@/util/iife" import { errorMessage } from "@/util/error" import { isMedia } from "@/util/media" @@ -1092,6 +1095,30 @@ function hasSSEStallCause(e: unknown, depth = 0): boolean { return false } +// Extract the user-meaningful "SSE read timed out after Nms" string from an +// error that hasSSEStallCause matched. NamedSchemaError sets Error.prototype.message +// to the tag (`super(tag, options)` in named-schema-error.ts), so an in-process +// throw of MessageV2.SSEStallError caught and passed back through fromError must +// read `.data.message`, not `.message`, to recover the timing text. Plain Error +// instances (legacy or cross-realm) put it in `.message`. Walk the cause chain +// so nested wrappers still produce the original timing text. +function extractStallMessage(e: unknown, depth = 0): string { + if (depth > 8) return String(e) + if (!e || typeof e !== "object") return String(e) + const err = e as { data?: { message?: unknown }; message?: unknown; cause?: unknown } + if (err.data && typeof err.data.message === "string" && SSE_STALL_MESSAGE_RE.test(err.data.message)) { + return err.data.message + } + if (typeof err.message === "string" && SSE_STALL_MESSAGE_RE.test(err.message)) return err.message + if (err.cause) return extractStallMessage(err.cause, depth + 1) + // hasSSEStallCause already matched; produce best-effort text even if no node + // has the canonical "after Nms" timing format (e.g., test fixture passes + // "SSE read timed out" without the suffix). + if (err.data && typeof err.data.message === "string") return err.data.message + if (typeof err.message === "string") return err.message + return String(e) +} + export function fromError( e: unknown, ctx: { providerID: ProviderID; aborted?: boolean }, @@ -1144,7 +1171,7 @@ export function fromError( ).toObject() case hasSSEStallCause(e): return new SSEStallError( - { message: e instanceof Error ? e.message : String(e) }, + { message: extractStallMessage(e) }, { cause: e instanceof Error ? e : undefined }, ).toObject() case APICallError.isInstance(e): diff --git a/packages/opencode/test/provider/chunk-timeout.test.ts b/packages/opencode/test/provider/chunk-timeout.test.ts index 6da10765b927..84632c38a585 100644 --- a/packages/opencode/test/provider/chunk-timeout.test.ts +++ b/packages/opencode/test/provider/chunk-timeout.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test" -import { resolveChunkTimeout, SSEStallError, wrapSSE } from "../../src/provider/provider" +import { resolveChunkTimeout, wrapSSE } from "../../src/provider/provider" +import { MessageV2 } from "../../src/session/message-v2" describe("provider.resolveChunkTimeout", () => { test("returns 120s default when undefined and reasoning=false", () => { @@ -51,7 +52,7 @@ describe("provider.resolveChunkTimeout", () => { }) describe("provider.wrapSSE — SSEStallError integration", () => { - test("throws SSEStallError when chunk read exceeds timeout", async () => { + test("throws MessageV2.SSEStallError when chunk read exceeds timeout", async () => { const stream = new ReadableStream({ pull() { return new Promise(() => {}) // never resolves — forces stall @@ -62,7 +63,16 @@ describe("provider.wrapSSE — SSEStallError integration", () => { const wrapped = wrapSSE(res, 2, ctl) const reader = wrapped.body!.getReader() - await expect(reader.read()).rejects.toBeInstanceOf(SSEStallError) + const caught = await reader.read().then( + () => undefined, + (e) => e, + ) + expect(MessageV2.SSEStallError.isInstance(caught)).toBe(true) + if (!MessageV2.SSEStallError.isInstance(caught)) throw new Error("expected SSEStallError") + expect(caught.data.message).toBe("SSE read timed out after 2ms") + // signal.reason and the cancel reason must share identity with the thrown error + expect(MessageV2.SSEStallError.isInstance(ctl.signal.reason)).toBe(true) + expect(ctl.signal.reason).toBe(caught) }) test("does not wrap non-SSE responses", () => { diff --git a/packages/opencode/test/session/message-v2-sse-stall.test.ts b/packages/opencode/test/session/message-v2-sse-stall.test.ts index ad35dbf1ae60..00cb6bf162ce 100644 --- a/packages/opencode/test/session/message-v2-sse-stall.test.ts +++ b/packages/opencode/test/session/message-v2-sse-stall.test.ts @@ -1,6 +1,5 @@ import { describe, expect, test } from "bun:test" import { APICallError } from "ai" -import { SSEStallError } from "../../src/provider/provider" import { ProviderID } from "../../src/provider/schema" import { MessageV2 } from "../../src/session/message-v2" @@ -40,7 +39,7 @@ describe("session.message-v2.fromError — SSEStallError", () => { }) test("detects SSE stall through two-deep cause chain", () => { - const stall = new SSEStallError("SSE read timed out") + const stall = new MessageV2.SSEStallError({ message: "SSE read timed out" }) const middle = new Error("middle") middle.cause = stall const outer = new Error("outer") @@ -52,7 +51,7 @@ describe("session.message-v2.fromError — SSEStallError", () => { }) test("detects SSE stall when APICallError wraps SSEStallError", () => { - const stall = new SSEStallError("SSE read timed out") + const stall = new MessageV2.SSEStallError({ message: "SSE read timed out" }) const apiError = new APICallError({ message: "stream error", url: "https://api.githubcopilot.com/chat/completions", @@ -66,6 +65,21 @@ describe("session.message-v2.fromError — SSEStallError", () => { expect(MessageV2.APIError.isInstance(result)).toBe(false) }) + test("preserves canonical wrapSSE message when fromError receives a top-level MessageV2.SSEStallError", () => { + // Regression for the F9 unification: schema-error instances have + // `.message === "SSEStallError"` (the tag, set by `super(tag, options)` in + // namedSchemaError), so fromError must read `.data.message` to recover the + // real timing text. If extractStallMessage ever regresses, the result here + // will be the literal string "SSEStallError" instead of the timing text. + const stall = new MessageV2.SSEStallError({ message: "SSE read timed out after 2ms" }) + + const result = MessageV2.fromError(stall, { providerID }) + + expect(MessageV2.SSEStallError.isInstance(result)).toBe(true) + if (!MessageV2.SSEStallError.isInstance(result)) throw new Error("Expected SSEStallError") + expect(result.data.message).toBe("SSE read timed out after 2ms") + }) + test("hasSSEStallCause: tag-based detection still works", () => { const tagged = Object.assign(new Error("anything"), { _tag: "SSEStallError" }) const result = MessageV2.fromError(tagged, { providerID }) diff --git a/packages/opencode/test/session/retry.test.ts b/packages/opencode/test/session/retry.test.ts index a56ec932498c..409291581ce1 100644 --- a/packages/opencode/test/session/retry.test.ts +++ b/packages/opencode/test/session/retry.test.ts @@ -5,7 +5,6 @@ import { setTimeout as sleep } from "node:timers/promises" import { Effect, Exit, Layer, Pull, Schedule } from "effect" import { SessionRetry } from "../../src/session/retry" import { MessageV2 } from "../../src/session/message-v2" -import { SSEStallError } from "../../src/provider/provider" import { ProviderID } from "../../src/provider/schema" import { AppRuntime } from "../../src/effect/app-runtime" import { SessionID } from "../../src/session/schema" @@ -236,7 +235,7 @@ describe("session.retry.retryable", () => { describe("SessionRetry.retryable — SSE stall round-trip", () => { test("retries SSEStallError after MessageV2.fromError round-trip", () => { - const err = new SSEStallError("SSE read timed out") + const err = new MessageV2.SSEStallError({ message: "SSE read timed out" }) const obj = MessageV2.fromError(err, { providerID }) expect(MessageV2.SSEStallError.isInstance(obj)).toBe(true) expect(SessionRetry.retryable(obj)).toBe("SSE read timed out")