Adding play packet#1
Draft
AxenoDev wants to merge 18 commits into
Draft
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughReplaces the ChangesBinary I/O Infrastructure (yoki_binutils)
yoki_identifier crate
Packet API migration and new play packets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
Owner
Author
|
tu fous quoi ici @CodeRabbit |
|
Tip For best results, initiate chat on the files or code changes.
Si tu ne veux pas de mes retours ici, dis-le et je m’efface — ou tu peux me demander de mettre les reviews en pause. (／_·)／🐇 |
Owner
Author
|
@coderabbitai retire toi de mes PRs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (2)
crates/yoki_identifier/src/identifier.rs (1)
56-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
as_stror change its contract.
as_strreturns an ownedString, so the name reads like a borrowed&strAPI. Rename it to something likeas_string/to_string, or change it to return a borrowed view if that’s the intended contract.🤖 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 `@crates/yoki_identifier/src/identifier.rs` around lines 56 - 58, The Identifier::as_str method returns an owned String, which conflicts with the naming expectation of a borrowed string view. Update the API in identifier.rs by either renaming as_str to something like as_string or to_string, or change its signature and implementation so it returns a borrowed &str if that is the intended contract. Make sure any callers of Identifier::as_str are updated to match the revised method name or return type.crates/yoki_binutils/src/binary_writer.rs (1)
33-35: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning every byte-array payload before writing.
write_byte_arraycurrently copies the whole slice into a newVecand then serializes it byte-by-byte.crates/yoki_macros/src/attrs.rs:103-136sends everyVec<u8>field through this helper, so larger payloads pay an unnecessary extra allocation on each encode. Prefer writing the length prefix directly and then appending the slice in one shot.🤖 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 `@crates/yoki_binutils/src/binary_writer.rs` around lines 33 - 35, Avoid cloning the byte slice in BinaryWriter::write_byte_array: it currently allocates a new Vec<u8> and then writes it back out, which adds unnecessary copying for every Vec<u8> field encoded through the attrs macros path. Update write_byte_array to write the length prefix directly and then write the provided slice in one shot, keeping the BinaryError flow unchanged and preserving the existing callers that route through write_byte_array.
🤖 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.
Inline comments:
In `@crates/minecraft_packet/src/connection.rs`:
- Around line 31-37: The `Connection::send` API currently accepts a free-form
`i32` packet id, which breaks the packet-type-to-id association and can let
callers encode mismatched frames. Update `send` to derive the packet id from the
packet type’s metadata instead of taking it as a parameter, and keep that
association enforced through the `OutgoingPacket`/`PacketMeta` types so only the
correct id can be used for each packet. Adjust any call sites to pass only the
packet value and use the existing packet metadata path for encoding.
In `@crates/minecraft_packet/src/packet/raw.rs`:
- Around line 12-14: RawPacket::decode is now decoding payloads without
verifying the packet ID, which lets callers misinterpret unrelated packet bytes
as type P. Restore the safety check inside RawPacket::decode by validating
self.id against P before calling P::decode_payload, or make the helper
internal/private and only use the unchecked path from
PacketReport::decode_serverbound. Keep the existing decode flow in RawPacket and
the IncomingPacket bound, but ensure public callers still get UnknownPacket
protection instead of silent mis-decodes.
In `@crates/minecraft_packet/src/packets/configuration/plugin_message.rs`:
- Line 7: The plugin-message payload field is being decoded with the wrong
framing because `PluginMessage`’s `data: Vec<u8>` is missing remaining-bytes
semantics. Update the `PluginMessage` packet definition to restore the
`#[protocol(remaining)]` behavior on `data` so the derive reads the rest of the
packet as the payload instead of treating it as a length-prefixed byte array.
In `@crates/minecraft_packet/src/packets/login/login_success.rs`:
- Around line 20-23: The LoginSuccess packet encoding in write is missing the
optional-signature presence flag before the signature payload, so the
Some(signature) branch currently writes the value directly and misframes signed
properties. Update the LoginSuccess::write logic to emit the expected
boolean/prefix flag for self.signature first, then write the signature only when
present, and keep the None case consistent with the protocol’s optional-field
encoding.
In `@crates/minecraft_packet/src/packets/play/data/death_location.rs`:
- Around line 1-4: The import block in death_location.rs is not
rustfmt-compliant, so update the existing use statement to match standard
rustfmt ordering and formatting. Keep the same symbols imported from
yoki_binutils, but let rustfmt-style grouping/order be applied to the `use` list
so CI passes consistently.
- Around line 10-29: Make DeathLocation atomic by removing the per-field Omitted
wrappers from the DeathLocation struct and having ReadBytes/WriteBytes serialize
both dimension and location as required fields. Update DeathLocation::read and
DeathLocation::write so truncated payloads fail with BinaryError::UnexpectedEof
instead of producing a partially filled value, and let
LoginPacket.death_location’s outer Option be the only presence guard.
In `@crates/minecraft_packet/src/packets/play/data/position.rs`:
- Around line 1-4: The import block in the position module is not
rustfmt-compliant. Reformat the existing yoki_binutils use statement so it
matches standard rustfmt ordering and wrapping, keeping the same imported
symbols in the same module.
- Around line 7-35: The Position type is currently serialized and deserialized
as three f64 values, which does not match the packed block-position wire format
used by DeathLocation.location. Update Position’s ReadBytes and WriteBytes
implementations to use the packed block-position encoding/decoding instead of
individual floating-point fields, and make sure the DeathLocation path that uses
Position continues to read/write the remaining optional fields in the correct
order.
In `@crates/minecraft_packet/src/packets/play/login.rs`:
- Around line 7-29: `LoginPacket` is currently impossible to construct outside
its module because all fields are private and `PacketOut` only adds encoding
metadata, not a constructor. Make `LoginPacket` buildable by either exposing a
public constructor on `LoginPacket` or making the fields public, and ensure any
new constructor covers all required fields such as `entity_id`,
`dimension_names`, and `death_location`.
In `@crates/yoki_binutils/src/data_types/f64.rs`:
- Around line 2-3: The import ordering in the f64 module is not formatted
consistently, causing CI to fail. Run cargo fmt on this module so the use
statements in f64.rs are reordered and normalized automatically; the affected
symbols are the top-level imports from binary_reader and binary_writer.
In `@crates/yoki_binutils/src/data_types/i32.rs`:
- Around line 1-3: The import block in i32.rs needs to be rustfmt-compliant
because CI is failing on reordered use lists. Run cargo fmt, or manually reorder
the imports in the i32 module so the BinaryError, BinaryReader/ReadBytes, and
BinaryWriter/WriteBytes uses match rustfmt’s expected order.
In `@crates/yoki_binutils/src/data_types/i64.rs`:
- Around line 1-3: The import block in i64.rs needs to be rustfmt-compliant
because the current use list ordering is causing CI failures. Run cargo fmt, or
reorder the imports in the module’s top-level use statements so the grouped
symbols from binary_reader and binary_writer match rustfmt’s expected ordering.
In `@crates/yoki_binutils/src/data_types/string.rs`:
- Around line 8-10: The string decode path in the read logic for String should
reject negative lengths before converting the VarInt value to usize. Update the
String reader to inspect the signed value returned by VarInt::inner() first,
return an invalid-length-style BinaryError for any negative value, and only cast
to usize after that check so malformed input is not misreported as
UnexpectedEof. Apply the same validation pattern anywhere this length-decoding
flow is reused for identifier parsing.
In `@crates/yoki_binutils/src/data_types/u16.rs`:
- Around line 1-3: The import block in u16.rs needs to be reformatted to match
rustfmt’s ordering so CI passes. Update the grouped use statements for
BinaryReader/BinaryWriter and their helpers by running cargo fmt or otherwise
reordering the imports in place, keeping the symbols BinaryError, read_be,
BinaryReader, ReadBytes, write_be, BinaryWriter, and WriteBytes intact.
In `@crates/yoki_binutils/src/data_types/uuid.rs`:
- Around line 3-5: The import block in uuid.rs needs to be rustfmt-compliant
because CI is failing on the `use` ordering. Re-run `cargo fmt` or reorder the
imports in the `uuid` module so the `BinaryReader`, `ReadBytes`, `BinaryWriter`,
and `WriteBytes` items are grouped and sorted the way rustfmt expects, keeping
the existing symbols intact.
In `@crates/yoki_binutils/src/data_types/var_int.rs`:
- Around line 80-89: The VarInt conversions from u32 and &u32 are lossy because
they cast into i32 and can truncate values above i32::MAX. Replace the existing
From<u32> and From<&u32> implementations on VarInt with fallible TryFrom
versions, and make the conversion return an error for out-of-range values
instead of silently narrowing. Update any related VarInt conversion helpers or
call sites in this module to use the new TryFrom-based API.
In `@crates/yoki_binutils/src/data_types/vec.rs`:
- Around line 8-10: The Vec<T> decoder currently casts VarInt::inner() directly
to usize and preallocates before validating the decoded length, which can turn
malformed input into an oversized allocation. Update the ProtocolRead
implementation for Vec<T> in vec.rs to validate the decoded VarInt length first,
reject negative or unreasonably large values, and only then create the vector
with capacity and enter the read loop. Keep the fix localized around
VarInt::read and the Vec::with_capacity path.
In `@crates/yoki_binutils/src/error.rs`:
- Around line 44-52: The BinaryError to ProtocolError conversion is collapsing
distinct varint and varlong failures into the same ProtocolError::VarIntTooBig
case. Update the From<BinaryError> for ProtocolError implementation in error.rs
so BinaryError::VarLongTooBig maps to its own corresponding ProtocolError
variant instead of sharing the varint one, while keeping the existing mappings
for the other BinaryError cases unchanged.
In `@crates/yoki_identifier/src/protocol.rs`:
- Around line 1-4: The `yoki_binutils` import block in `protocol.rs` needs to be
reformatted to match rustfmt output so CI stops failing. Update the grouped `use
yoki_binutils::{...}` statement to the canonical rustfmt ordering and wrapping,
keeping the same imported symbols (`BinaryError`, `BinaryReader`,
`BinaryWriter`, `ProtocolError`, `ProtocolRead`, `ProtocolWrite`, `ReadBytes`,
`WriteBytes`) but letting rustfmt control the layout.
In `@crates/yoki_macros/src/attrs.rs`:
- Around line 84-87: The generated code for #[present_if(...)] is using a bare
identifier that is not in scope in either derive path. Update
generate_field_decode and generate_field_encode in attrs.rs so
field_present_if(field) resolves against an available binding: use the packet
struct/instance field access in PacketOut and the decoded-value binding in
PacketIn, rather than emitting `#present_field` directly. Ensure the generated
quote! references a symbol that exists in each context so derives with
present_if compile cleanly.
---
Nitpick comments:
In `@crates/yoki_binutils/src/binary_writer.rs`:
- Around line 33-35: Avoid cloning the byte slice in
BinaryWriter::write_byte_array: it currently allocates a new Vec<u8> and then
writes it back out, which adds unnecessary copying for every Vec<u8> field
encoded through the attrs macros path. Update write_byte_array to write the
length prefix directly and then write the provided slice in one shot, keeping
the BinaryError flow unchanged and preserving the existing callers that route
through write_byte_array.
In `@crates/yoki_identifier/src/identifier.rs`:
- Around line 56-58: The Identifier::as_str method returns an owned String,
which conflicts with the naming expectation of a borrowed string view. Update
the API in identifier.rs by either renaming as_str to something like as_string
or to_string, or change its signature and implementation so it returns a
borrowed &str if that is the intended contract. Make sure any callers of
Identifier::as_str are updated to match the revised method name or return type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f277019b-bb8a-4512-ac86-8b32b336dd2f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (60)
Cargo.tomlcrates/minecraft_packet/Cargo.tomlcrates/minecraft_packet/src/connection.rscrates/minecraft_packet/src/packet/mod.rscrates/minecraft_packet/src/packet/raw.rscrates/minecraft_packet/src/packets/configuration/acknowledge_finish_configuration.rscrates/minecraft_packet/src/packets/configuration/client_information.rscrates/minecraft_packet/src/packets/configuration/finish_configuration.rscrates/minecraft_packet/src/packets/configuration/plugin_message.rscrates/minecraft_packet/src/packets/handshaking/handshake.rscrates/minecraft_packet/src/packets/login/cookie_response_login.rscrates/minecraft_packet/src/packets/login/encryption_response.rscrates/minecraft_packet/src/packets/login/login_acknowledged.rscrates/minecraft_packet/src/packets/login/login_plugin_response.rscrates/minecraft_packet/src/packets/login/login_start.rscrates/minecraft_packet/src/packets/login/login_success.rscrates/minecraft_packet/src/packets/play/data/death_location.rscrates/minecraft_packet/src/packets/play/data/mod.rscrates/minecraft_packet/src/packets/play/data/position.rscrates/minecraft_packet/src/packets/play/login.rscrates/minecraft_packet/src/packets/play/mod.rscrates/minecraft_packet/src/packets/status/ping_request.rscrates/minecraft_packet/src/packets/status/ping_response.rscrates/minecraft_packet/src/packets/status/status_request.rscrates/minecraft_packet/src/packets/status/status_response.rscrates/yoki_binutils/Cargo.tomlcrates/yoki_binutils/src/binary_error.rscrates/yoki_binutils/src/binary_reader.rscrates/yoki_binutils/src/binary_writer.rscrates/yoki_binutils/src/data_types/bool.rscrates/yoki_binutils/src/data_types/f64.rscrates/yoki_binutils/src/data_types/i32.rscrates/yoki_binutils/src/data_types/i64.rscrates/yoki_binutils/src/data_types/i8.rscrates/yoki_binutils/src/data_types/mod.rscrates/yoki_binutils/src/data_types/option.rscrates/yoki_binutils/src/data_types/optional.rscrates/yoki_binutils/src/data_types/string.rscrates/yoki_binutils/src/data_types/u16.rscrates/yoki_binutils/src/data_types/u8.rscrates/yoki_binutils/src/data_types/uuid.rscrates/yoki_binutils/src/data_types/var_int.rscrates/yoki_binutils/src/data_types/vec.rscrates/yoki_binutils/src/error.rscrates/yoki_binutils/src/lib.rscrates/yoki_binutils/src/protocol.rscrates/yoki_binutils/src/reader.rscrates/yoki_binutils/src/writer.rscrates/yoki_identifier/Cargo.tomlcrates/yoki_identifier/src/error.rscrates/yoki_identifier/src/identifier.rscrates/yoki_identifier/src/lib.rscrates/yoki_identifier/src/protocol.rscrates/yoki_macros/Cargo.tomlcrates/yoki_macros/src/attrs.rscrates/yoki_macros/src/lib.rscrates/yoki_macros/src/packet_in.rscrates/yoki_macros/src/packet_out.rscrates/yoki_macros/src/packet_report.rsyoki/src/handlers/handshake.rs
💤 Files with no reviewable changes (12)
- crates/minecraft_packet/src/packets/login/login_acknowledged.rs
- crates/minecraft_packet/src/packets/status/status_request.rs
- crates/minecraft_packet/src/packets/login/login_start.rs
- crates/minecraft_packet/src/packets/status/status_response.rs
- crates/minecraft_packet/src/packets/configuration/acknowledge_finish_configuration.rs
- crates/minecraft_packet/src/packets/login/cookie_response_login.rs
- crates/minecraft_packet/src/packets/status/ping_response.rs
- crates/yoki_binutils/src/writer.rs
- crates/minecraft_packet/src/packets/status/ping_request.rs
- crates/minecraft_packet/src/packets/login/encryption_response.rs
- crates/yoki_binutils/src/reader.rs
- crates/minecraft_packet/src/packets/configuration/finish_configuration.rs
Comment on lines
+31
to
37
| pub async fn send<P: OutgoingPacket>( | ||
| &mut self, | ||
| id: i32, | ||
| packet: &P, | ||
| ) -> Result<(), ProtocolError> { | ||
| let payload = packet.encode_with_id(id)?; | ||
| self.send_framed(&payload).await |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
send can now emit mismatched packet IDs and payloads.
With the per-type ID removed from PacketMeta, this signature accepts any id for any OutgoingPacket. A wrong call site still produces a valid frame, but the peer will decode it as a different packet type. Please keep the (packet type, packet id) association in the type system instead of taking a free i32 here.
🤖 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 `@crates/minecraft_packet/src/connection.rs` around lines 31 - 37, The
`Connection::send` API currently accepts a free-form `i32` packet id, which
breaks the packet-type-to-id association and can let callers encode mismatched
frames. Update `send` to derive the packet id from the packet type’s metadata
instead of taking it as a parameter, and keep that association enforced through
the `OutgoingPacket`/`PacketMeta` types so only the correct id can be used for
each packet. Adjust any call sites to pass only the packet value and use the
existing packet metadata path for encoding.
|
(╥﹏╥) ✅ Action performedReviews paused. |
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 by CodeRabbit
New Features
Bug Fixes