fix: surface BYOK stream response failures#345
Merged
Conversation
Treat provider streams that finish with an error reason as provider failures so BYOK handling can finalize the pending message with a safe user-facing error instead of leaving the generic empty-message fallback. Add regression coverage for OpenAI Responses stream failures that do not throw provider exceptions. Co-authored-by: Codex <[email protected]>
Contributor
📝 WalkthroughWalkthroughThis PR adds provider failure detection to the streaming path in convex/ai/resilience.ts by checking the accumulator's finish reason after streaming completes, and extends error classification in byokErrors.ts to map provider_response_failed errors to byok_unknown_error. A comprehensive test validates the end-to-end flow. ChangesProvider Failure Detection and Classification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
convex/ai/byokErrors.ts (1)
53-53: ⚡ Quick winExtract
"provider_response_failed"to a shared named constant.The sentinel
"provider_response_failed"is a raw string literal here and again inresilience.tsline 269. A future typo in either location would silently break BYOK detection with no compiler or runtime warning.Export a constant from
byokErrors.ts(or a thin shared module) and import it inresilience.ts:♻️ Proposed refactor
+// Sentinel thrown by resilience.ts when the stream accumulator records +// finishReason "error" without a provider exception. +export const PROVIDER_RESPONSE_FAILED = "provider_response_failed"; + export function classifyByokError(error: unknown): ByokErrorCode | null { ... - if (lower.includes("provider_response_failed")) return "byok_unknown_error"; + if (lower.includes(PROVIDER_RESPONSE_FAILED)) return "byok_unknown_error";Then in
resilience.ts:+import { ..., PROVIDER_RESPONSE_FAILED } from "./byokErrors"; ... - if (accumulator.toRow().finishReason === "error") throw new Error("provider_response_failed"); + if (accumulator.toRow().finishReason === "error") throw new Error(PROVIDER_RESPONSE_FAILED);As per coding guidelines: "Extract magic values to named constants instead of using literals in code."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@convex/ai/byokErrors.ts` at line 53, Extract the literal "provider_response_failed" into a named exported constant (e.g. PROVIDER_RESPONSE_FAILED) from byokErrors.ts, replace the raw string in the predicate (the line with lower.includes("provider_response_failed") in byokErrors) with that constant, and update resilience.ts to import and use that same constant instead of the literal; ensure the constant is a named export and referenced by its symbol in both files to avoid duplicated magic strings.convex/ai/resilienceStreamFailure.test.ts (1)
60-114: ⚡ Quick winConsider adding a non-BYOK coverage case.
The single test verifies
isByok: truethoroughly, butisByok: falsetakes a different code path inrunAttempt:tryReportByokreturns early, and the error falls through toreportError(withAI_ERROR_MESSAGE). That branch is currently untested.A minimal addition:
it("surfaces provider finish errors as generic AI error for non-BYOK users", async () => { const streamText = vi.fn( async (options: { onStepFinish: (step: StepResult<ToolSet>) => void }) => { options.onStepFinish(responseFailedStep()); return { text: Promise.resolve("") }; }, ); const agent = { continueThread: vi.fn(async () => ({ thread: { streamText } })), } as unknown as Agent; const runQuery = vi.fn(async () => ({ page: [{ _id: "pending-message", status: "pending" }], })); const runMutation = vi.fn(async () => undefined); const runAction = vi.fn(async () => undefined); await streamWithRetry( { runQuery, runMutation, runAction } as unknown as ActionCtx, { primaryAgent: agent, fallbackAgent: agent, primaryModelName: "gpt-5.4", threadId: "thread-1", userId: "user-1", prompt: "hello", isByok: false, // <-- key difference provider: "openai", source: "chat", environment: "prod", }, ); expect(saveMessage).toHaveBeenCalledWith( expect.anything(), expect.anything(), expect.objectContaining({ message: expect.objectContaining({ content: expect.stringContaining("trouble") }), }), ); });As per coding guidelines: "Every function must have at least one error/edge case test in addition to happy path tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@convex/ai/resilienceStreamFailure.test.ts` around lines 60 - 114, Add a second test in resilienceStreamFailure.test.ts that mirrors the existing "surfaces non-thrown provider finish errors as BYOK messages" case but sets isByok: false to exercise the non-BYOK path in streamWithRetry/runAttempt; simulate the provider finish error via the same streamText + responseFailedStep(), call streamWithRetry, and assert that saveMessage was called with a message content containing the generic AI error wording (the branch handled by reportError/AI_ERROR_MESSAGE) and that the runMutation/runAction expectations align with the non-BYOK flow. Ensure the test references the same agent/runQuery/runMutation/runAction mocks used by the BYOK test so it exercises tryReportByok returning early and reportError being invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@convex/ai/byokErrors.ts`:
- Line 53: Extract the literal "provider_response_failed" into a named exported
constant (e.g. PROVIDER_RESPONSE_FAILED) from byokErrors.ts, replace the raw
string in the predicate (the line with
lower.includes("provider_response_failed") in byokErrors) with that constant,
and update resilience.ts to import and use that same constant instead of the
literal; ensure the constant is a named export and referenced by its symbol in
both files to avoid duplicated magic strings.
In `@convex/ai/resilienceStreamFailure.test.ts`:
- Around line 60-114: Add a second test in resilienceStreamFailure.test.ts that
mirrors the existing "surfaces non-thrown provider finish errors as BYOK
messages" case but sets isByok: false to exercise the non-BYOK path in
streamWithRetry/runAttempt; simulate the provider finish error via the same
streamText + responseFailedStep(), call streamWithRetry, and assert that
saveMessage was called with a message content containing the generic AI error
wording (the branch handled by reportError/AI_ERROR_MESSAGE) and that the
runMutation/runAction expectations align with the non-BYOK flow. Ensure the test
references the same agent/runQuery/runMutation/runAction mocks used by the BYOK
test so it exercises tryReportByok returning early and reportError being
invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 128e7fa2-6333-4467-a88e-26af6ab787b2
📒 Files selected for processing (3)
convex/ai/byokErrors.tsconvex/ai/resilience.tsconvex/ai/resilienceStreamFailure.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
errorbut do not throw provider exceptions.Changes
convex/ai/resilience.ts: throws a safeprovider_response_failedsentinel when the stream accumulator recordsfinishReason: "error"afterresult.textresolves.convex/ai/byokErrors.ts: classifies that sentinel asbyok_unknown_errorso the existing BYOK reporting path finalizes the pending message safely.convex/ai/resilienceStreamFailure.test.ts: exercises the non-thrown provider failure path throughstreamWithRetryand asserts the BYOK message path is used.Checklist
npx tsc --noEmitpassesnpm run lintpassesnpm testpasses (existing + new tests)anytypes (enforced by ESLint);ascasts only at deserialization boundariestype: description)Test Plan
npm run typechecknpm run lintnpx vitest run convex/ai/resilience.test.ts convex/ai/resilienceFinalize.test.ts convex/ai/resilienceStreamFailure.test.tsnpm testSummary by CodeRabbit