Skip to content

fix: send a command with no signable argument as chat_command - #1525

Open
u9g wants to merge 1 commit into
PrismarineJS:masterfrom
u9g:fix/unsigned-command-without-signable-args
Open

u9g wants to merge 1 commit into
PrismarineJS:masterfrom
u9g:fix/unsigned-command-without-signable-args

Conversation

@u9g

@u9g u9g commented Sep 7, 2026

Copy link
Copy Markdown
Member

Constraints on the serverbound command packet:

  • chat_command_signed is written only when the command produced at least one argument signature.
  • A command with no signable argument (/login, /list, anything the server's command tree does not mark as a message argument) is written as chat_command, whether or not the client can sign.
  • The signature list carried by the packet is the one the command produced.

Vanilla reference: ClientPacketListener.sendCommand sends the unsigned ServerboundChatCommandPacket when SignableCommand.of(...).arguments() is empty.

Tests: /login hunter2 and an argumentless /msg write chat_command with an empty signature list. Both fail on master and pass with the change. They do not run in CI yet; #1526 makes this file run and #1527 fixes the pre-existing test in it.

_signedChat chose chat_command_signed whenever the client had profile
keys and a chat session, regardless of the command. The vanilla client
picks the packet per command: ClientPacketListener.sendCommand sends the
unsigned ServerboundChatCommandPacket when SignableCommand.of(...)
.arguments() is empty, and only sends the signed packet when there is an
argument to sign. Every argumentless command (/login, /list, and any
command the server's command tree does not mark as a message argument)
went out on the signed packet with an empty signature list.

The packet now follows the signatures actually produced.
@u9g
u9g force-pushed the fix/unsigned-command-without-signable-args branch from 93043f8 to 1aca229 Compare September 7, 2026 01:03

@rom1504 rom1504 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Astra agent review — AI-generated, not manually written by the maintainer.

The Astra agent reviewed this change at the maintainer's request. I inspected the current diff and packet schema and ran a local reproduction using the current chat plugin with installed dependencies; an unsigned command consumes acknowledgement state without sending it. I did not run a live vanilla server or the full test suite.

Comment thread src/client/chat.js
}
client.write((mcData.supportFeature('seperateSignedChatCommandPacket') && canSign) ? 'chat_command_signed' : 'chat_command', chatPacket)
// A command with nothing to sign goes as the unsigned chat_command whether or not the client can sign.
client.write((mcData.supportFeature('seperateSignedChatCommandPacket') && argumentSignatures.length > 0) ? 'chat_command_signed' : 'chat_command', chatPacket)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Astra agent review — AI-generated, not manually written by the maintainer.

On versions with seperateSignedChatCommandPacket (for example 1.21.8), the newly selected chat_command contains only command, so the acknowledgement fields in chatPacket never reach the server. However, this branch still calls getAcknowledgements() (which marks entries as acknowledged) and then resets _lastSeenMessages.pending. I reproduced receiving one signed message, sending /list with signing enabled, and then sending chat: the command clears pending from 1 to 0 and the following chat_message sends offset 0, although no packet reported that acknowledgement offset. This desynchronizes the last-seen window used for chat validation. Could the unsigned-command branch leave both the pending count and entry flags untouched, with a regression covering an unsigned command followed by signed chat? Older versions whose chat_command carries acknowledgements should retain their current accounting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants