Skip to content

audit(F4): narrow SSE_STALL_MESSAGE_RE to the emitted format #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tesdal wants to merge 1 commit into from
+48 −5
Closed

audit(F4): narrow SSE_STALL_MESSAGE_RE to the emitted format #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/opencode/src/session/message-v2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1076,9 +1076,11 @@ export const filterCompactedEffect = Effect.fnUntraced(function* (sessionID: Ses
return filterCompacted(stream(sessionID))
})

// Legacy substring fallback for rare paths where the tagged class gets stripped
// during cross-realm rethrow. Primary signal is `name`/`_tag` set by SSEStallError.
const SSE_STALL_MESSAGE_RE = /SSE (read|chunk) time(d out|out)/
// Message-based exact-match fallback for the wrapSSE() emission format.
// The primary signals are `name === "SSEStallError"` and `_tag === "SSEStallError"`;
// this regex only matches when that structured error identity is stripped during
// cross-realm rethrow.
const SSE_STALL_MESSAGE_RE = /^SSE read timed out after \d+ms$/
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/^...$/ in JS will also match before a final trailing newline, so this can still accept "SSE read timed out after 120000ms\n", which isn’t the emitted wrapSSE() format. If the goal is truly exact-format matching, either add a trailing negative lookahead to reject \r?\n or switch to a non-regex check that enforces both the prefix and digit+"ms" suffix without $’s newline behavior.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

@tesdal tesdal Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the careful review — applied fixes for comments 1 and 3.

Respectfully pushing back on this one with empirical evidence: in V8/Bun (and ECMAScript generally, without the m flag), $ is true end-of-string and does NOT match before a trailing \n. Verified in Bun 1.3.13:

const re = /^SSE read timed out after \d+ms$/
re.test("SSE read timed out after 120000ms")       // true
re.test("SSE read timed out after 120000ms\n")     // false
re.test("SSE read timed out after 120000ms\r\n")   // false
re.test("SSE read\n timed out after 120000ms")     // false

The "$ matches before final \n" behavior is Python/Ruby semantics, not JS (in JS you'd need the m flag, which we don't set).

That said, the concern is legitimate in spirit, so I added the trailing-newline case explicitly to the regression test suite at message-v2-sse-stall.test.ts:99 and documented this behavior in a code comment. If the regex engine semantics ever change, we'll know.


function hasSSEStallCause(e: unknown, depth = 0): boolean {
if (depth > 8) return false
Expand Down
45 changes: 43 additions & 2 deletions packages/opencode/test/session/message-v2-sse-stall.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,14 @@ describe("session.message-v2.fromError — SSEStallError", () => {
})

test("detects SSE stall by timeout message without SSEStallError name", () => {
const error = new Error("SSE chunk timeout after 120000ms")
const error = new Error("SSE read timed out after 120000ms")

const result = MessageV2.fromError(error, { providerID })

expect(result.name).toBe("SSEStallError")
expect(MessageV2.SSEStallError.isInstance(result)).toBe(true)
if (!MessageV2.SSEStallError.isInstance(result)) throw new Error("Expected SSEStallError")
expect(result.data.message).toBe("SSE chunk timeout after 120000ms")
expect(result.data.message).toBe("SSE read timed out after 120000ms")
})

test("detects SSE stall through two-deep cause chain", () => {
Expand Down Expand Up @@ -65,4 +65,45 @@ describe("session.message-v2.fromError — SSEStallError", () => {
expect(result.name).toBe("SSEStallError")
expect(MessageV2.APIError.isInstance(result)).toBe(false)
})

test("hasSSEStallCause: tag-based detection still works", () => {
const tagged = Object.assign(new Error("anything"), { _tag: "SSEStallError" })
const result = MessageV2.fromError(tagged, { providerID })
expect(MessageV2.SSEStallError.isInstance(result)).toBe(true)
})

test("hasSSEStallCause: exact wrapSSE-format message is detected through cause chain", () => {
const taggedless = new Error("SSE read timed out after 120000ms")
const outer = new Error("outer")
outer.cause = taggedless
const result = MessageV2.fromError(outer, { providerID })
expect(MessageV2.SSEStallError.isInstance(result)).toBe(true)
})

test("hasSSEStallCause: message-regex fallback rejects speculative 'chunk timeout' variants", () => {
const speculative = new Error("SSE chunk timeout")
const result = MessageV2.fromError(speculative, { providerID })
expect(MessageV2.SSEStallError.isInstance(result)).toBe(false)
})

test("hasSSEStallCause: narrowed regex rejects shapes the old loose regex accepted", () => {
// The previous regex /SSE (read|chunk) time(d out|out)/ matched all of these;
// the narrowed /^SSE read timed out after \d+ms$/ rejects them all.
// Any future wrapSSE format change must update the regex in lockstep.
// Note: In JS regexes, `$` without the `m` flag matches only end-of-input
// and does NOT match before a trailing "\n", so newline-suffixed messages
// are rejected.
const cases = [
"SSE read timed out", // missing "after Nms" suffix
"SSE chunk timed out after 120000ms", // wrong verb ("chunk" never emitted)
"SSE read timeout after 120000ms", // "timeout" not "timed out"
"prefix: SSE read timed out after 120000ms", // non-anchored prefix
"SSE read timed out after 120000ms ", // trailing whitespace
"SSE read timed out after 120000ms\n", // trailing newline ($ does not match before \n)
]
for (const message of cases) {
const result = MessageV2.fromError(new Error(message), { providerID })
expect(MessageV2.SSEStallError.isInstance(result)).toBe(false)
}
})
})
Loading