feat(ruby): make JSON status protocol configurable and expose mismatch metadata#2
feat(ruby): make JSON status protocol configurable and expose mismatch metadata#2
Conversation
…h metadata - remove hardcoded JSON handshake protocol varint (760) - add status_protocol option (Integer, :auto, nil) - keep backward-compatible auto behavior via DEFAULT_JSON_STATUS_PROTOCOL (760) - expose requested_protocol, response_protocol, protocol_mismatch - record protocol mismatch as metadata without raising errors - document proxy rewrite caveat (Velocity/Bungee/ViaVersion) - add tests for explicit protocol injection, auto mode, and mismatch handling
Summary of ChangesHello @ryush00, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Ruby MineStat library by introducing a configurable JSON status handshake protocol, moving away from a hardcoded value. This change, along with the exposure of detailed protocol mismatch metadata, aims to provide more accurate and transparent server status observations, especially in complex proxy environments where protocol versions might be manipulated or differ between request and response. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
|
@codex review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces configurable JSON status protocol selection and exposes metadata about the requested and response protocols, which is a valuable enhancement for environments with proxy servers. The changes are well-documented in the ReadMe.md and include new tests to cover the added functionality. The code appears to be correct and follows good practices for Ruby development.
There was a problem hiding this comment.
Code Review
The pull request introduces configurable JSON status protocol selection and exposes mismatch metadata, which is a valuable enhancement for environments with proxy stacks. The changes are well-documented in the ReadMe.md and the new functionality is covered by comprehensive tests. The code is generally clean and follows good practices. I've provided some minor suggestions for clarity and consistency.
| ms = MineStat.new("minecraft.frag.land", 25565, 3, MineStat::Request::SLP, false, false) | ||
| ``` | ||
|
|
||
| ### JSON status protocol selection |
There was a problem hiding this comment.
The heading "JSON status protocol selection" is a bit verbose. Consider shortening it to "JSON Protocol Selection" or similar for conciseness.
| - `response_protocol`: protocol from `version.protocol` in response JSON (if present) | ||
| - `protocol_mismatch`: whether request/response protocol values differ | ||
|
|
||
| In proxy stacks (Velocity/Bungee) and ViaVersion setups, `version.protocol` may be rewritten or shaped by requester protocol. Do not assume `response_protocol` is always the backend server's real version. |
There was a problem hiding this comment.
This note is very important and is also present in the code comments. Consider making it more prominent, perhaps with a warning icon or a bolded "Important Note:" prefix, to draw more attention to it.
| # Default protocol version used for JSON status handshakes in auto mode. | ||
| # This preserves the current behavior while keeping future protocol bumps centralized. | ||
| DEFAULT_JSON_STATUS_PROTOCOL = 760 |
There was a problem hiding this comment.
The comment for DEFAULT_JSON_STATUS_PROTOCOL is good, but it could be slightly more concise. The phrase "This preserves the current behavior while keeping future protocol bumps centralized" is a bit long for a constant's comment.
# Default protocol version used for JSON status handshakes in auto mode.
# Centralizes protocol version for future updates while maintaining backward compatibility.| @requested_protocol # protocol used in JSON status handshake | ||
| @response_protocol # protocol returned by JSON status response | ||
| @protocol_mismatch # protocol mismatch between request and response? |
There was a problem hiding this comment.
The comments for @requested_protocol, @response_protocol, and @protocol_mismatch are good, but they could be aligned for better readability, similar to other instance variables above.
@protocol # protocol level
@requested_protocol # protocol used in JSON status handshake
@response_protocol # protocol returned by JSON status response
@protocol_mismatch # protocol mismatch between request and response?| # | ||
| # The response protocol is not always the real backend server protocol. | ||
| # Proxy stacks such as Velocity/Bungee may shape status responses from the | ||
| # requester protocol, and ViaVersion may rewrite `version.protocol`. |
There was a problem hiding this comment.
This comment block is very important for understanding the response_protocol behavior. It's good that it's included here and in the README. Consider using a consistent phrasing for this note across both locations.
| def normalize_status_protocol(protocol) | ||
| return :auto if protocol.nil? || protocol == :auto | ||
| return protocol if protocol.is_a?(Integer) |
There was a problem hiding this comment.
The normalize_status_protocol method's logic is clear. However, the return protocol if protocol.is_a?(Integer) line could be combined with the raise ArgumentError to make the intent of the if statement clearer: if it's not nil or :auto, it must be an Integer.
return :auto if protocol.nil? || protocol == :auto
raise ArgumentError, "status_protocol must be an Integer, :auto, or nil" unless protocol.is_a?(Integer)
protocol| # Protocol used in the JSON status handshake request | ||
| # @note `:auto`/`nil` mode uses {DEFAULT_JSON_STATUS_PROTOCOL} | ||
| attr_reader :requested_protocol | ||
|
|
||
| # Protocol observed in JSON status response (`version.protocol`) | ||
| # @note In proxy environments (Velocity/Bungee/ViaVersion), this may not match backend server protocol | ||
| attr_reader :response_protocol | ||
|
|
||
| # Whether requested and response protocols are different | ||
| attr_reader :protocol_mismatch |
There was a problem hiding this comment.
The attr_reader comments for requested_protocol, response_protocol, and protocol_mismatch are well-written and provide good context. Ensure that the spacing for these new attr_reader definitions is consistent with the existing ones for better readability.
attr_reader :protocol
# Protocol used in the JSON status handshake request
# @note `:auto`/`nil` mode uses {DEFAULT_JSON_STATUS_PROTOCOL}
attr_reader :requested_protocol
# Protocol observed in JSON status response (`version.protocol`)
# @note In proxy environments (Velocity/Bungee/ViaVersion), this may not match backend server protocol
attr_reader :response_protocol
# Whether requested and response protocols are different
attr_reader :protocol_mismatch| end | ||
|
|
||
| class MineStatStatusProtocolTest < Minitest::Test | ||
| def decode_varint(data, offset = 0) |
There was a problem hiding this comment.
The test class name MineStatStatusProtocolTest is descriptive. However, for consistency with Ruby's naming conventions, it's common to use TestMineStatStatusProtocol or MineStat::StatusProtocolTest.
| packet_id = handshake_packet.getbyte(packet_len_size) | ||
| raise "Unexpected packet id: #{packet_id}" unless packet_id == 0 |
There was a problem hiding this comment.
The raise statement here is good for ensuring the packet ID is as expected. However, in a test helper, it might be more robust to return nil or raise a more specific test-related error, or even assert the packet ID directly in the test method if it's critical for the test's premise.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b25d745f53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| # @return [Integer, Symbol] Integer protocol or :auto | ||
| def normalize_status_protocol(protocol) | ||
| return :auto if protocol.nil? || protocol == :auto | ||
| return protocol if protocol.is_a?(Integer) |
There was a problem hiding this comment.
normalize_status_protocol currently accepts any Integer, but pack_varint later truncates to 32 bits (value &= 0xFFFFFFFF), so large values are silently rewritten on the wire while requested_protocol still exposes the original input. In practice, passing something like 1 << 40 sends protocol 0, which can produce incorrect status responses and misleading mismatch metadata instead of honoring the caller’s explicit protocol request.
Useful? React with 👍 / 👎.
1 participant
Summary
This PR removes the hardcoded JSON status handshake protocol (
760) in Ruby MineStat and introduces configurable protocol selection while preserving backward-compatible default behavior.What changed
pack_varint(760)with dynamic handshake protocol resolution.status_protocoloption support:Integer: use explicit protocol:autoornil: use default auto moderequested_protocolresponse_protocolprotocol_mismatchresponse_protocolis not always backend server version in proxy stacks (Velocity/Bungee/ViaVersion).Why
In proxy/ViaVersion environments, status responses may be shaped by requester protocol and
version.protocolmay be rewritten. A fixed handshake protocol can produce misleading observations and false version assumptions.Validation
ruby -I Ruby/lib Ruby/test/test_status_protocol.rb