refactor(provider, message-v2): unify SSEStallError on MessageV2 schema (F9) by tesdal · Pull Request #12 · tesdal/opencode

tesdal · 2026-04-27T18:49:02Z

Summary

Audit finding F9 (codex-5.3 diamond review, 2026-04-22): provider.ts and message-v2.ts each defined their own SSEStallError class. Unify on the schema-backed MessageV2.SSEStallError so there is one identity.

Changes

Drop class SSEStallError extends Error from provider.ts. wrapSSE() now throws new MessageV2.SSEStallError({ message }) directly.
Add extractStallMessage() in message-v2.ts. Required because namedSchemaError calls super(tag, options), so .message on the schema instance is the tag ("SSEStallError"), not the timing text. The helper walks the cause chain and reads .data.message.
Update three test files to construct the schema variant directly.
Keep hasSSEStallCause + SSE_STALL_MESSAGE_RE (hardened in F4) as the cross-realm safety net for vendor SDKs that strip name/_tag on rethrow.

Spike evidence

In-process identity (throw/catch, AbortController.abort, AbortSignal.any, ReadableStream.cancel, Promise.reject, end-to-end wrapSSE shape) survives via the schema's name-based isInstance check — 6/6 spike tests green before deletion.

Risk

Behavior change: thrown SSEStallError .message flips from "SSE read timed out after Nms" to "SSEStallError" (the tag). Audited consumers — only retry.ts:25 reads it, and already uses .data.message. extractStallMessage covers the fromError path.

Diamond review

codex-5.3: APPROVED_WITH_COMMENTS (5 nits, 0 blocking)
opus: APPROVED_WITH_NITS (6 nits, 0 blocking; 3 applied this round)

Verification

bun typecheck clean (turbo, all 13 packages)
bun test test/provider/chunk-timeout.test.ts test/session/message-v2-sse-stall.test.ts test/session/retry.test.ts → 60/60 pass

Net delta: +46 / -18 LOC.

github-actions · 2026-04-27T18:49:12Z

Hey! Your PR title refactor(provider, message-v2): unify SSEStallError on MessageV2 schema (F9) doesn't follow conventional commit format.

Please update it to start with one of:

feat: or feat(scope): new feature
fix: or fix(scope): bug fix
docs: or docs(scope): documentation changes
chore: or chore(scope): maintenance tasks
refactor: or refactor(scope): code refactoring
test: or test(scope): adding or updating tests

Where scope is the package name (e.g., app, desktop, opencode).

See CONTRIBUTING.md for details.

Copilot

Pull request overview

Unifies SSEStallError to a single schema-backed identity (MessageV2.SSEStallError) so throw/catch and isInstance checks are consistent across provider + session error serialization.

Changes:

Remove the local SSEStallError class from provider.ts and throw MessageV2.SSEStallError from wrapSSE().
Add extractStallMessage() to preserve the user-meaningful timeout text when MessageV2.fromError() round-trips schema errors whose .message is the tag.
Update tests to construct/assert against the schema error variant (MessageV2.SSEStallError).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
packages/opencode/src/provider/provider.ts	Switch `wrapSSE()` to throw the schema-backed SSE stall error (and remove the local error class).
packages/opencode/src/session/message-v2.ts	Add helper to extract stall timing text through cause chains; use it in `fromError()` SSE stall mapping.
packages/opencode/test/provider/chunk-timeout.test.ts	Update integration test to assert schema error identity and preserved `.data.message`.
packages/opencode/test/session/message-v2-sse-stall.test.ts	Update fixtures to use `MessageV2.SSEStallError` instances.
packages/opencode/test/session/retry.test.ts	Update retry round-trip test to use schema-backed stall error.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-27T18:54:08Z

+// Lazy reference inside wrapSSE only — message-v2 also imports from @/provider,
+// so this back-edge only resolves at call time. Do not use MessageV2 at module
+// top-level (e.g., for a pre-allocated error constant) or it will TDZ-fault.
+import { MessageV2 } from "@/session/message-v2"


The new import { MessageV2 } from "@/session/message-v2" introduces a circular dependency: message-v2.ts imports from @/provider (the barrel), and src/provider/index.ts re-exports ./provider, so loading message-v2 will load provider.ts which now loads message-v2 again. Also, the comment says the back-edge “only resolves at call time”, but the import is eager during module linking.

To avoid hard-to-debug initialization/TDZ issues, consider breaking the cycle (e.g., have message-v2.ts import ProviderError directly from @/provider/error instead of the barrel, or move SSEStallError to a small shared module that both provider.ts and message-v2.ts can import without crossing provider↔session).

Good catch — you're right that is eager during module linking, the lazy-call-time framing was wrong. Fixed in 26dc319 by taking your first suggestion: message-v2.ts now imports * as ProviderError from "@/provider/error" directly instead of going through the @/provider barrel that re-exports provider.ts. provider/error.ts is leaf (only depends on ai, http, @/util/iife, and ./schema types), so no cycle.

Updated the comment in provider.ts to document the new invariant. Tests still 60/60, typecheck clean across all 13 packages.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-28T08:03:33Z

    case hasSSEStallCause(e):
      return new SSEStallError(
-        { message: e instanceof Error ? e.message : String(e) },
+        { message: extractStallMessage(e) },
        { cause: e instanceof Error ? e : undefined },
      ).toObject()


extractStallMessage() is meant to preserve the real timeout text when fromError() receives an in-process MessageV2.SSEStallError (whose .message is the tag). The current tests cover the fallback path (data.message without the after Nms suffix) but don’t assert the main regression being fixed: when data.message does match the canonical ^SSE read timed out after \d+ms$ format, fromError() should still emit that exact string (not "SSEStallError"). Consider adding a test that passes new MessageV2.SSEStallError({ message: "SSE read timed out after 2ms" }) into fromError() and asserts result.data.message === "SSE read timed out after 2ms".

Good point — the canonical happy path was implicit in the chunk-timeout integration test but not asserted directly at the fromError boundary. Added a focused test in 3c31b2b that constructs a top-level MessageV2.SSEStallError with the canonical wrapSSE message and asserts result.data.message preserves it verbatim. Fails loudly if extractStallMessage ever regresses to copying .message (which would emit the literal tag 'SSEStallError'). 10/10 pass in message-v2-sse-stall.test.ts.

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).

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tesdal · 2026-04-28T12:48:31Z

Review-only PR — Copilot R3 clean (no new comments). Closing per per-finding workflow; merging local/audit-f9-sse-stall-unify into local/integration-v2 with --no-ff next.

Audit finding F9 (codex-5.3 diamond review, 2026-04-22). Diamond: codex-5.3 APPROVED_WITH_COMMENTS, opus APPROVED_WITH_NITS. Copilot review: PR #12 (review-only, closed). 3 rounds: R1: 1 substantive (circular import) — fixed by importing ProviderError directly from @/provider/error. R2: 1 substantive (canonical-path coverage gap) — fixed by adding focused fromError round-trip test. R3: clean (no new comments). Net delta: +65 / -19 LOC (60 → 70 tests pass for F9 suite).

github-actions Bot added the needs:title label Apr 27, 2026

tesdal requested a review from Copilot April 27, 2026 18:49

Copilot AI reviewed Apr 27, 2026

View reviewed changes

tesdal force-pushed the local/audit-f9-sse-stall-unify branch from 96b2a0c to 26dc319 Compare April 27, 2026 19:08

tesdal requested a review from Copilot April 28, 2026 07:59

Copilot started reviewing on behalf of tesdal April 28, 2026 07:59 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

tesdal force-pushed the local/audit-f9-sse-stall-unify branch from 26dc319 to 3c31b2b Compare April 28, 2026 12:42

tesdal requested a review from Copilot April 28, 2026 12:42

Copilot started reviewing on behalf of tesdal April 28, 2026 12:43 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

tesdal closed this Apr 28, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(provider, message-v2): unify SSEStallError on MessageV2 schema (F9)#12

refactor(provider, message-v2): unify SSEStallError on MessageV2 schema (F9)#12
tesdal wants to merge 1 commit intophase-ab-basefrom
local/audit-f9-sse-stall-unify

tesdal commented Apr 27, 2026

Uh oh!

github-actions Bot commented Apr 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

tesdal Apr 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

tesdal Apr 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

tesdal commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

tesdal commented Apr 27, 2026

Summary

Changes

Spike evidence

Risk

Diamond review

Verification

Uh oh!

github-actions Bot commented Apr 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

tesdal Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

tesdal Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

tesdal commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants