Repository navigation
Stream the model completions - #71
maxime-pharmania wants to merge 10 commits into
Conversation
![proval-0e6d07[bot]](https://avatars.githubusercontent.com/in/4828992?s=60&v=4){kind=small}
| }, | ||
| finishReason: choice.finish_reason, | ||
| requestId: completion.id ?? null, | ||
| finishReason: accumulated.finishReason ?? "stop", |
There was a problem hiding this comment.
⚫️ [PROBLEM] Empty provider stream is silently reported as a successful completion
⚫ [PROBLEM] A stream that never yields a choice chunk is reported as a successful completion.
In accumulateChatCompletionStream, if no chunk carries choices[0], finishReason stays null and error stays null (the if (!choice) continue; path at apps/api/src/agent/llm/openai.ts:112). send() then coalesces the missing finish reason with finishReason: accumulated.finishReason ?? "stop" at this line and returns content: null with no error.
Impact: runAgentLoop sees no tool calls (apps/api/src/agent/llm/loop.ts:227) and returns a "completed" result with finalMessage: null (loop.ts:245) — no logAgentError, no retry, and usage stays at zero. The old buffered code did completion.choices[0].message, which threw a loud TypeError on an empty/invalid body, so this is a regression in failure signaling. Users get an empty answer that looks like a successful run.
Suggested fix: after accumulation in send(), fail loudly when no choice chunk ever arrived:
if (accumulated.finishReason === null && !accumulated.error) {
throw new Error("Provider stream ended without a completion");
}This routes the degenerate stream through the loop's existing catch path (including context-length handling and retries).
|
Thanks for this change — moving the OpenAI sender to streaming directly addresses the silent-origin problem, and the rationale comment at a. Summary This PR replaces the buffered b. Good points
Main Issues:
Additional Notes:
|
The model provider screen now carries a "Stream responses" switch. The value is stored per provider (default on, migration 0046) and passed to the sender, which keeps the original buffered path when the switch is off.
A stream that yields no choice chunks produced a null finish reason, a null content and no error, so the loop reported a completed run with an empty message. The buffered call raised a TypeError on that shape; the streaming path now raises an explicit error. The accumulator also keeps the provider error code as a status for the case where a chunk carrying an error reaches it.
|
Fixed in Empty stream. The streaming path now throws when the accumulated finish reason is In-stream error status. I looked at where that error actually comes from: the OpenAI SDK converts an error frame into an |
| baseURL: modelProvider.baseUrl, | ||
| model: repository.modelName, | ||
| timeoutSecond: modelProvider.timeoutSecond, | ||
| stream: modelProvider.stream, |
There was a problem hiding this comment.
⚫️ [PROBLEM] Misindented stream property breaks prettier --check
This added stream property is indented with 8 spaces while every sibling in the same object literal uses 12. The api package's lint script is prettier --check . && eslint . (apps/api/package.json:15), so bun run lint fails on this file until the line is reformatted.
timeoutSecond: modelProvider.timeoutSecond,
stream: modelProvider.stream,
reasoningEffort: repository.reasoningEffort,
| const accumulated = await accumulateChatCompletionStream(stream); | ||
|
|
||
| // A provider can answer 200 and report the failure inside the stream. | ||
| if (accumulated.error) { | ||
| const error = new Error(accumulated.error.message) as Error & { status?: number }; | ||
| if (accumulated.error.status !== undefined) { | ||
| error.status = accumulated.error.status; | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
⚫️ [PROBLEM] In-stream 429 status never attaches on the real SDK path
Attaching the provider code as status here only runs when an error chunk reaches the accumulator, but your own test comment at apps/api/src/agent/llm/openai.streaming.test.ts:191-193 says the OpenAI SDK converts the error frame into an APIError before the accumulator sees it, and that error carries no status. On the path the comment describes, these lines never execute, and the sender-level test (lines 194-196) asserts only error.message, so the retry contract stays unpinned: runAgentLoop's check at apps/api/src/agent/llm/loop.ts:196 requires status === 429 || status === 400, so an in-stream rate limit fails the run outright instead of getting the retry/backoff that HTTP-level 429s get. Could be wrong if the SDK actually lets the chunk through to the accumulator — but then the sender-level test should assert status to pin that contract either way.
Fix: wrap stream accumulation, attach the numeric code from the error payload when status is absent, and assert the status in the sender-level test.
let accumulated: AccumulatedChatCompletion;
try {
accumulated = await accumulateChatCompletionStream(stream);
} catch (err) {
const apiError = err as OpenAI.APIError & { status?: number; error?: { code?: number } };
if (apiError.status === undefined && typeof apiError.error?.code === "number") {
apiError.status = apiError.error.code;
}
throw err;
}And in the test: expect((error as { status?: number }).status).toBe(429);
let accumulated: AccumulatedChatCompletion;
try {
accumulated = await accumulateChatCompletionStream(stream);
} catch (err) {
const apiError = err as OpenAI.APIError & { status?: number; error?: { code?: number } };
if (apiError.status === undefined && typeof apiError.error?.code === "number") {
apiError.status = apiError.error.code;
}
throw err;
}
| if (client.baseURL?.includes("openrouter.ai")) { | ||
| const result = openRouterErrorSchema.safeParse(completion); |
There was a problem hiding this comment.
⚫️ [PROBLEM] Buffered OpenRouter error drops retry status and can fall through to a TypeError
In the restored buffered path, throw new Error(result.data.error.message) at openai.ts:92 (just below this hunk) drops the numeric result.data.error.code, so apps/api/src/agent/llm/loop.ts:196 (status === 429 || status === 400) can never retry a rate limit reported in a 200 body, even though the streaming branch now carefully carries status. Separately, openRouterErrorSchema at openai.ts:8 requires code to be a number — if a provider sends a string or missing code, safeParse fails, execution falls through to completion.choices[0] at openai.ts:96, and because an error body has no choices this throws Cannot read properties of undefined (reading '0'), masking the provider's real message. (This mirrors the pre-PR buffered code, but it is re-entered under the new stream: false option alongside the streaming side's status handling.)
Fix: attach the code as error.status, make the schema tolerant of non-numeric/missing codes, and guard a missing choice before dereferencing it.
if (result.success) {
const error = new Error(result.data.error.message) as Error & { status?: number };
error.status = result.data.error.code;
throw error;
}
const choice = completion.choices[0];
if (!choice) {
throw new Error("The provider returned a completion without choices");
}if (result.success) {
const error = new Error(result.data.error.message) as Error & { status?: number };
error.status = result.data.error.code;
throw error;
}
const choice = completion.choices[0];
if (!choice) {
throw new Error("The provider returned a completion without choices");
}
|
Thanks for the follow-up — this push lands both fixes from the last review (the empty-stream completion now fails loudly, and in-stream errors carry a numeric status) and adds the per-provider Stream toggle end to end. a. Summary This push adds a NOT NULL b. Good points
c. Main Issues
d. Additional Notes
|
| }; | ||
| } | ||
|
|
||
| const completion = await client.chat.completions.create({ ...request }); |
There was a problem hiding this comment.
⚫️ [PROBLEM] Buffered path dereferences completion.choices[0] without a guard
⚫ [PROBLEM] The buffered branch introduced in this push reads completion.choices[0] at apps/api/src/agent/llm/openai.ts:86 (then choice.message at line 87) with no guard. The only protection is the OpenRouter check at lines 79–84, which runs solely when client.baseURL contains openrouter.ai and the body matches openRouterErrorSchema exactly — the schema requires code: z.number() (line 8), so an error body with a missing or string code fails safeParse and falls through. For any other baseURL (custom gateways are a first-class config in this product), a 200 error-shaped JSON leaves completion.choices undefined and the sender throws TypeError: Cannot read properties of undefined (reading '0') instead of the provider's actual error message.
Impact: as soon as someone opts into buffered mode, provider failures surface as an inscrutable TypeError, and the provider's own message plus any machine-readable code are lost — exactly the debuggability the OpenRouter check was written to preserve.
Fix: surface the body error when choices is absent:
const choice = completion.choices?.[0];
if (!choice) {
const bodyError = (completion as { error?: { message?: string } }).error;
throw new Error(bodyError?.message ?? "Provider returned a completion without choices");
}
const message = choice.message;
|
Thanks for the follow-up work — this round turns the streaming rewrite from last time into a deliberate per-provider choice, and the flag travels cleanly from the model-provider form through the schema to every sender. a. Summary This push adds a b. Good points
Main Issues:
Additional Notes:
|
The limit is Proval's own constant (USER_PROMPT_MAX_LENGTH); the model provider and repository screens enforce it. The API test now derives its bound from the constant instead of hardcoding 2000.
…LENGTH The limit was a build-time constant. It now resolves at runtime from PROVAL_USER_PROMPT_MAX_LENGTH (any value that is not a positive integer is ignored), the API enforces the resolved value and announces it on /api/auth/me, and the repository form uses the announced limit instead of the bundled one.
|
|
Thanks for the follow-up — this push lands the fixes for every open thread from the last round: the buffered path now guards missing choices and attaches the provider status, the GitHub indent is reformatted, the Anthropic toggle is hidden, and the empty-stream hole is closed with a real guard. a. Summary The OpenAI sender's error/status contract is now uniform: b. Good points
Main Issues: none — no critical or problem-level findings survived verification this round. Additional Notes:
|
A 200 body without a choices key (an error shaped object for instance) made the buffered path dereference undefined and throw a raw TypeError instead of the explicit completion-without-choices error.
The repository update and GET /api/auth/me resolved PROVAL_USER_PROMPT_MAX_LENGTH separately, so the announced and the enforced limit could drift. Both now call currentUserPromptMaxLength().
|
Both addressed.
Validation on this branch: |
|
Thanks for the follow-up — this push closes both remaining notes from the last round: the buffered path now survives a 200 body with no a. Summary Two small, surgical fixes. b. Good points
Main Issues: none. Additional Notes: none this round — both prior notes are resolved in this push. |
Brings in the review fixes of seoes#71 (choices guard on the buffered path, single call site for the custom instructions limit) and of seoes#73 (typed layout props, .env.example entry, environment cleanup in the repository tests, getMe announcement test, inclusive boundary test). The fork keeps 9000 as its default limit.
2 participants
Adds streaming to the OpenAI-compatible sender, with a switch in the model provider screen.
stream: truewithstream_options.include_usage; the completion is rebuilt from the chunks (content, tool calls merged by index, usage from the final chunk)accumulateChatCompletionStream()helperstreamsetting per model provider (default on, migration0046) and a "Stream responses" toggle in the create and edit form; when it is off the original buffered path runs