-
Notifications
You must be signed in to change notification settings - Fork 143
Sxian/clt 2533/update livekit models #868
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
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis pull request updates the LiveKit protocol definitions to introduce agent session management capabilities, client protocol versioning, pagination support for ingress listing, and custom serialization implementations. Key additions include structured agent session state tracking, participant permissions for agent session management, and backward-compatible serde serialization logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
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: 1
🤖 Fix all issues with AI agents
In `@livekit-protocol/protocol`:
- Line 1: The livekit-protocol/protocol git submodule is not initialized which
prevents verifying the referenced commit
cd064b4fb1c245416fa610264f4fbd205cc64554 and regenerating Rust artifacts; run
git submodule update --init livekit-protocol/protocol to populate the submodule,
check out the referenced commit, then re-run your proto/codegen step (e.g., the
project's build script or protoc/grpc/prost generation used by the repository)
to regenerate Rust code and commit any updated generated files so tests/builds
pass.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
livekit-protocol/protocollivekit-protocol/src/livekit.rslivekit-protocol/src/livekit.serde.rs
🔇 Additional comments (15)
livekit-protocol/src/livekit.serde.rs (8)
1196-1331: AgentSessionState serde implementation looks correct.The new
AgentSessionStatetype correctly handles:
versionserialized as a string (standard protobuf JSON for int64) and deserialized viaNumberDeserialize.oneof datawithSnapshotandDeltavariants encoded/decoded as base64 byte strings.- Duplicate field detection properly checks
data__(not individual variant names) for the oneof, ensuring mutual exclusivity betweensnapshotanddelta.This follows the standard pbjson-build generation patterns.
3145-3147: ClientInfo.client_protocol field addition looks correct.Standard pattern for a new numeric field: skip-if-zero on serialize,
NumberDeserializeon deserialize, dual camelCase/snake_case field name support.Also applies to: 3184-3186, 3212-3213, 3229-3229, 3263-3263, 3294-3294, 3365-3372, 3390-3390
18638-18650: ListIngressRequest.page_token pagination support looks correct.The field is correctly treated as
Option<String>— serialized only whenSome, deserialized withoutunwrap_or_default, and assigned directly as anOption.Also applies to: 18667-18668, 18677-18677, 18702-18702, 18724-18734, 18753-18753
18770-18779: ListIngressResponse.next_page_token follows the same correct pattern as page_token.Also applies to: 18793-18794, 18800-18800, 18824-18824, 18845-18854, 18867-18867
22058-22060: ParticipantInfo.client_protocol mirrors the ClientInfo.client_protocol pattern — looks correct.Also applies to: 22127-22129, 22163-22164, 22186-22186, 22226-22226, 22263-22263, 22376-22383, 22407-22407
22692-22694: ParticipantPermission.can_manage_agent_session — standard boolean field addition, looks correct.Also applies to: 22727-22729, 22755-22756, 22770-22770, 22802-22802, 22831-22831, 22888-22893, 22909-22909
40583-40584: TextMessageRequest.session_state — correctly handled asOption<AgentSessionState>.Serialized as a nested struct when
Some, deserialized directly intoOption, nounwrap_or_default— appropriate for an optional message field.Also applies to: 40602-40604, 40625-40626, 40636-40636, 40664-40664, 40689-40689, 40717-40721, 40739-40739
40758-40759: TextMessageResponse.session_state follows the same correct pattern as TextMessageRequest.Also applies to: 40768-40770, 40786-40787, 40794-40794, 40819-40819, 40841-40841, 40851-40855, 40870-40870
livekit-protocol/src/livekit.rs (7)
322-324: Permission flag addition is clear and additive.
No issues.
367-369: ParticipantInfo client protocol field looks good.
Additive field; no concerns.
1072-1074: ClientInfo client protocol field looks good.
Additive field; no concerns.
4434-4450: AgentSessionState schema is clear.
Version + snapshot/delta oneof is consistent.
5212-5213: Pagination request token addition looks good.
Additive optional field; no concerns.
5226-5228: Pagination response token addition looks good.
Additive optional field; no concerns.
4454-4464: The concern about wire-compatibility cannot be verified in this repository. The current code showssession_state(message type) at tag5, and no evidence exists of a previoussession_data(bytes) field at the same tag in either the current codebase or git history. If this is a new addition rather than a replacement, tag reuse is not a concern. Verify against the source.protofiles in the upstream protocol repository to confirm whether an actual field replacement occurred.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @@ -1 +1 @@ | |||
| Subproject commit 765a80e4298e376593859c3f11cf748c725f68f9 | |||
| Subproject commit cd064b4fb1c245416fa610264f4fbd205cc64554 | |||
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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify submodule changes and generated code status
# First, check the current state of the submodule
echo "=== Current submodule state ==="
cd livekit-protocol || exit 1
cat protocol
echo ""
echo "=== Submodule history (last 5 commits) ==="
cd protocol 2>/dev/null && git log --oneline -5 2>/dev/null || echo "Submodule not initialized"
echo ""
echo "=== Checking if commit exists ==="
cd protocol 2>/dev/null && git cat-file -t cd064b4fb1c245416fa610264f4fbd205cc64554 2>/dev/null && echo "Commit found" || echo "Commit verification failed"
echo ""
echo "=== Recent proto file changes ==="
cd protocol 2>/dev/null && git show --name-only cd064b4fb1c245416fa610264f4fbd205cc64554 2>/dev/null | head -20
echo ""
echo "=== Generated Rust files status ==="
cd ../..
git ls-files livekit-protocol/src/ | head -30
echo ""
echo "=== Check for uncommitted changes in livekit-protocol ==="
git status --short livekit-protocol/ | head -20Repository: livekit/rust-sdks
Length of output: 638
🏁 Script executed:
#!/bin/bash
# Investigate the actual submodule state and proto changes
echo "=== Actual current submodule HEAD ==="
cd livekit-protocol/protocol && git rev-parse HEAD && git log --oneline -1
echo ""
echo "=== Check if the review-mentioned commit exists anywhere ==="
git rev-list --all | grep cd064b4fb1c245416fa610264f4fbd205cc64554 && echo "Found" || echo "Not found in this repository"
echo ""
echo "=== Search for session_data/session_state in .proto files ==="
find . -name "*.proto" -type f | head -5
grep -r "session_data\|session_state" . --include="*.proto" 2>/dev/null | head -20
cd ../..
echo ""
echo "=== Check livekit.serde.rs for manual serde implementations ==="
head -50 livekit-protocol/src/livekit.serde.rs
echo ""
echo "=== Check for any recent generated code ==="
stat livekit-protocol/src/livekit.rs 2>/dev/null | grep Modify
stat livekit-protocol/src/livekit.serde.rs 2>/dev/null | grep Modify
echo ""
echo "=== Check git log for changes to the submodule pointer ==="
git log --oneline -- livekit-protocol/protocol | head -10Repository: livekit/rust-sdks
Length of output: 2007
🏁 Script executed:
#!/bin/bash
# Investigate submodule structure and content
echo "=== List submodule directory structure ==="
cd livekit-protocol/protocol
ls -la | head -20
echo ""
echo "=== Find all files in submodule ==="
find . -type f | head -30
echo ""
echo "=== Search for TextMessageRequest which was mentioned in AI summary ==="
grep -r "TextMessageRequest" . --include="*.proto" --include="*.rs" 2>/dev/null | head -10
cd ../..
echo ""
echo "=== Verify the submodule configuration ==="
cat .gitmodules | grep -A3 "protocol"
echo ""
echo "=== Check if submodule is properly initialized ==="
git submodule status livekit-protocol/protocolRepository: livekit/rust-sdks
Length of output: 701
Initialize the submodule before merging.
The submodule livekit-protocol/protocol is not initialized in the working directory (shown by the - prefix in git submodule status). This must be resolved with git submodule update --init livekit-protocol/protocol to populate the actual protocol definitions and verify that the referenced commit cd064b4fb1c245416fa610264f4fbd205cc64554 contains valid changes and that all generated Rust code is properly regenerated.
🤖 Prompt for AI Agents
In `@livekit-protocol/protocol` at line 1, The livekit-protocol/protocol git
submodule is not initialized which prevents verifying the referenced commit
cd064b4fb1c245416fa610264f4fbd205cc64554 and regenerating Rust artifacts; run
git submodule update --init livekit-protocol/protocol to populate the submodule,
check out the referenced commit, then re-run your proto/codegen step (e.g., the
project's build script or protoc/grpc/prost generation used by the repository)
to regenerate Rust code and commit any updated generated files so tests/builds
pass.
ladvoc
left a 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.
LGTM ✅
Update the livekit protos
Summary by CodeRabbit