-
Notifications
You must be signed in to change notification settings - Fork 213
fix(openai): correct type assertions in livekitItemToOpenAIItem #981
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.
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
Conversation
🦋 Changeset detectedLatest commit: af47938 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughType assertions in the OpenAI realtime model were broadened to accept additional message content and item types (including output text and assistant/system items); a changeset file documenting the fix was added. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✏️ Tip: You can disable this entire section by setting Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/openai/src/realtime/realtime_model.ts (1)
1658-1663: Fix type mismatch:SystemItemexpects singleInputTextContent, not array.
SystemItem.contentis defined asInputTextContent(a single object), but the code always returnscontentListas an array for all roles including 'system'.UserItemandAssistantItemcorrectly expect arrays, butSystemItemdoes not.When
role === 'system'(converted from'developer'at line 1637), the returned object does not match theSystemItemstructure. Consider handling the system role separately by returning only the first content item, or adjust the return statement to reflect the actual type being returned.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/openai/src/realtime/realtime_model.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/agent-core.mdc)
Add SPDX-FileCopyrightText and SPDX-License-Identifier headers to all newly added files with '// SPDX-FileCopyrightText: 2025 LiveKit, Inc.' and '// SPDX-License-Identifier: Apache-2.0'
Files:
plugins/openai/src/realtime/realtime_model.ts
**/*.{ts,tsx}?(test|example|spec)
📄 CodeRabbit inference engine (.cursor/rules/agent-core.mdc)
When testing inference LLM, always use full model names from
agents/src/inference/models.ts(e.g., 'openai/gpt-4o-mini' instead of 'gpt-4o-mini')
Files:
plugins/openai/src/realtime/realtime_model.ts
**/*.{ts,tsx}?(test|example)
📄 CodeRabbit inference engine (.cursor/rules/agent-core.mdc)
Initialize logger before using any LLM functionality with
initializeLogger({ pretty: true })from '@livekit/agents'
Files:
plugins/openai/src/realtime/realtime_model.ts
🧠 Learnings (1)
📚 Learning: 2026-01-16T14:33:39.551Z
Learnt from: CR
Repo: livekit/agents-js PR: 0
File: .cursor/rules/agent-core.mdc:0-0
Timestamp: 2026-01-16T14:33:39.551Z
Learning: Applies to **/*.{ts,tsx}?(test|example|spec) : When testing inference LLM, always use full model names from `agents/src/inference/models.ts` (e.g., 'openai/gpt-4o-mini' instead of 'gpt-4o-mini')
Applied to files:
plugins/openai/src/realtime/realtime_model.ts
🧬 Code graph analysis (1)
plugins/openai/src/realtime/realtime_model.ts (1)
plugins/openai/src/realtime/api_proto.ts (5)
InputTextContent(155-158)OutputTextContent(170-173)UserItem(206-210)AssistantItem(212-216)SystemItem(200-204)
🔇 Additional comments (1)
plugins/openai/src/realtime/realtime_model.ts (1)
1641-1644: LGTM!The union type correctly reflects that the content type depends on the role:
output_textfor assistant messages (matchingOutputTextContent) andinput_textfor other roles (matchingInputTextContent).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Fixed type assertion for text content (line 1644)
InputTextContent, but whenrole === 'assistant', the type is'output_text'which corresponds toOutputTextContentInputTextContent | OutputTextContentto accurately reflect both casesFixed return type assertion for message items (line 1663)
UserItem, but this function can returnAssistantItemorSystemItemdepending on the roleUserItem | AssistantItem | SystemItemfor accuracySummary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.