feat: raw-stream watchdog helpers + docs (DEV-723 5/5) - #776
Conversation
Raw chat.send / responses.send streaming users cannot go through callModel's integrated watchdog, so the wrappers are now public: - applyChatStreamWatchdog(stream, timeouts): chat-completions chunk classification - content/reasoning/refusal deltas, tool-call deltas, and audio count as content; role-only preludes, empty deltas, and usage-only chunks are neutral; finish reasons and chunk-level error payloads disarm the deadlines permanently (late usage chunks after the finish chunk are never misread as stalls). - applyResponsesStreamWatchdog(stream, timeouts): the OpenResponses classification callModel uses internally. Both are exported from the package root alongside StreamStalledError / StreamFailedError / StreamTimeoutOptions. README gains a 'Stalled-stream detection' section (outside Speakeasy managed regions) documenting the semantics, the callModel timeout option incl. maxStallRetries, the raw-stream wrappers, and StreamFailedError.
| const stream = await openRouter.chat.send({ | ||
| model: "openai/gpt-5", | ||
| messages: [{ role: "user", content: "Hello!" }], | ||
| stream: true, | ||
| }); |
There was a problem hiding this comment.
🟡 Documented streaming example uses the wrong request shape and will not work
The new streaming example passes the model/messages/stream fields directly to the chat send call (openRouter.chat.send({...}) at README.md:159-163) instead of nesting them under the required chatRequest wrapper, so anyone copying it gets a request the SDK rejects.
Impact: Users following the new stalled-stream docs hit a type/validation failure instead of a working stream.
Request shape required by the generated chat send operation
src/sdk/chat.ts:18-32 accepts operations.SendChatCompletionRequestRequest, whose only required member is chatRequest: models.ChatRequest (src/models/operations/sendchatcompletionrequest.ts:35-61). Existing tests use the correct nesting, e.g. tests/e2e/chat.test.ts:22-33. The pre-existing usage example earlier in the README has the same problem.
| const stream = await openRouter.chat.send({ | |
| model: "openai/gpt-5", | |
| messages: [{ role: "user", content: "Hello!" }], | |
| stream: true, | |
| }); | |
| const stream = await openRouter.chat.send({ | |
| chatRequest: { | |
| model: "openai/gpt-5", | |
| messages: [{ role: "user", content: "Hello!" }], | |
| stream: true, | |
| }, | |
| }); |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Perry's Review
Verdict: 💬 Comments / questions
Risk: 🟢 Low
This PR exports the existing applyChatStreamWatchdog / applyResponsesStreamWatchdog helpers and StreamTimeoutOptions from the package root (previously internal to callModel), adds 7 chat-stream watchdog tests, and documents the stalled-stream detection feature in the README. The watchdog implementation itself was added in earlier stack layers — this layer is exports + tests + docs.
The exports are correct: both functions and the type are already defined and consumed internally; making them public is the right call for raw-stream users who bypass callModel. The test coverage is solid for the behavior tests (prelude-then-silence stall, healthy stream with late usage chunk, mid-generation stall, terminal classification). The README accurately describes the semantics, the callModel timeout option, and the raw-stream wrappers.
Two test-coverage gaps in the classification test (details inline), neither blocking. The audio and reasoningDetails branches of isContentBearingChatChunk are exercised in production code paths but not verified — and the test title explicitly advertises audio coverage that the test body doesn't deliver.
Full review
What's good
- Correct export surface.
applyChatStreamWatchdog,applyResponsesStreamWatchdog, andStreamTimeoutOptionsare all already defined and used internally. Making them public is the right call for raw-stream users who bypasscallModel. - README is accurate. The
firstContentMs/contentIntervalMssemantics, theStreamStalledErrorfields (phase,retryable,elapsedMs), themaxStallRetriesbehavior, and theStreamFailedErrordescription all match the implementation. - Behavioral tests are well-constructed. The scripted-chunk-stream helper with cancellation, the late-usage-chunk-after-terminal test, and the mid-generation stall test all exercise real edge cases of the watchdog's state machine.
Findings
-
[suggestion] Audio classification untested despite the test title claiming it. The test
it('classifies content, reasoning, refusal, tool-call, and audio deltas as content', …)has no audio case, yet the implementation checksdelta.audio !== undefined. The PR description also claims the tests cover "content/reasoning/refusal/tool/audio." Add an audio fixture so the test matches its title. -
[nit]
reasoningDetailsbranch untested.isContentBearingChatChunkchecksdelta.reasoningDetails !== undefined && delta.reasoningDetails.length > 0separately from thereasoningstring field, but onlyreasoningis tested. These are distinct fields onChatStreamDelta— worth a test case to cover the branch. -
[nit] README mentions
StreamFailedErrorwithout an import example. The section referencesStreamFailedErrorand its fields but doesn't show how to import it. It is exported from@openrouter/sdk(already, before this PR), but a reader would have to guess. A one-line note alongside theStreamStalledErrorimport would close the gap.
Risk assessment
Risk: 🟢 Low
| Dimension | Severity | Risk | Reasoning |
|---|---|---|---|
| Implementation risk | 🟩 | Low | Re-exports of already-tested functions plus new tests and docs; no logic changes in this layer. |
| Premise risk | 🟩 | Low | Raw-stream users bypassing callModel need the wrappers as public API — well-motivated and straightforward. |
| Estimated impact | 🟩 | Low | Worst case: a user imports a function that behaves differently than the docs describe — caught by tests, non-destructive, and trivially fixable. |
| Risk Factor | Severity | Risk | Reasoning |
|---|---|---|---|
| Reversibility | 🟩 | Low | Reverting the exports and docs restores the previous state fully. |
| Detectability | 🟩 | Low | Any mismatch is caught immediately by the new and existing test suites. |
| Blast radius | 🟩 | Low | A single SDK package's public export surface. |
| Data integrity | None | — | No persisted state is touched. |
| Financial exposure | None | — | No billing or payment paths. |
| Security and privacy exposure | None | — | No credentials, auth, or tenant-isolation code. |
| Propagation | 🟩 | Low | Downstream consumers opt in explicitly; no silent behavior change. |
| Availability | 🟩 | Low | The watchdog is opt-in; without it, streams behave as before. |
| Recovery cost | 🟩 | Low | A revert or docs fix is a single PR. |
| Time to correct | 🟩 | Low | Test gaps are add-a-case fixes, deployable in the next PR. |
|
|
||
| describe('isContentBearingChatChunk', () => { | ||
| it('classifies content, reasoning, refusal, tool-call, and audio deltas as content', () => { | ||
| expect(isContentBearingChatChunk(chunk({ delta: { content: 'hi' } }))).toBe(true); |
There was a problem hiding this comment.
The test title advertises audio coverage, but there's no audio case in the body. isContentBearingChatChunk checks delta.audio !== undefined, and the PR description also claims the tests cover "content/reasoning/refusal/tool/audio" — but audio is untested.
Could you add an audio fixture so the test matches its title? e.g.:
expect(
isContentBearingChatChunk(
chunk({ delta: { audio: { id: 'a1', data: 'base64...', transcript: 'hi', expiresAt: 0 } } }),
),
).toBe(true);▶ Prompt for agents: add an audio delta test case to the isContentBearingChatChunk classification test so the test body matches its stated scope.
| it('classifies content, reasoning, refusal, tool-call, and audio deltas as content', () => { | ||
| expect(isContentBearingChatChunk(chunk({ delta: { content: 'hi' } }))).toBe(true); | ||
| expect(isContentBearingChatChunk(chunk({ delta: { reasoning: 'hmm' } }))).toBe(true); | ||
| expect(isContentBearingChatChunk(chunk({ delta: { refusal: 'no' } }))).toBe(true); |
There was a problem hiding this comment.
This tests the reasoning string field, but isContentBearingChatChunk also has a separate branch for delta.reasoningDetails !== undefined && delta.reasoningDetails.length > 0 (a distinct array field on ChatStreamDelta). That branch is currently untested.
Would you add a reasoningDetails case to cover the branch?
▶ Prompt for agents: add a reasoningDetails array test case to the isContentBearingChatChunk classification test to cover the untested branch.
|
|
||
| (`applyResponsesStreamWatchdog` is the equivalent for the Responses API.) | ||
|
|
||
| Separately from stalls, server-reported stream failures (`response.failed` |
There was a problem hiding this comment.
StreamFailedError is mentioned here with its fields, but the section doesn't show how to import it. It is exported from @openrouter/sdk (already, before this PR), but a reader would have to guess the import path.
Could you add a brief import note, e.g. alongside the StreamStalledError import in the callModel example, or a one-liner here?
▶ Prompt for agents: add an import example for StreamFailedError in the README's stalled-stream section so users know it's importable from @openrouter/sdk.
Stack layer 5/5 — raw-stream watchdog helpers + docs
Base: #775 (stall retries). Top of the DEV-723 stack.
Changes
Raw
chat.send/responses.sendstreaming users don't go throughcallModel, so the watchdog wrappers become public API:applyChatStreamWatchdog(stream, timeouts)— chat-completions chunk classification: content/reasoning/refusal deltas, tool-call deltas, and audio count as content; role-only preludes, empty deltas, and usage-only chunks are neutral; finish reasons and chunk-level error payloads disarm deadlines permanently (a late usage chunk after the finish chunk is never misread as a stall).applyResponsesStreamWatchdog(stream, timeouts)— the OpenResponses classificationcallModeluses internally.StreamStalledError/StreamFailedError/StreamTimeoutOptions.callModeltimeoutoption incl.maxStallRetries, the raw-stream wrappers, andStreamFailedError.Tests
7 new tests: chunk classification matrix (content/reasoning/refusal/tool/audio vs role prelude/empty/usage), terminal classification (finish reasons, error payloads), role-prelude-then-silence stall, healthy stream with late usage chunk, mid-generation stall.
Verification
241 unit tests passing; lint / typecheck / build clean; package-root exports verified against built ESM.
Follow-ups (separate tickets)
projects/docs/client-sdks) once this ships in a release