Conversation
_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.
93043f8 to
1aca229
Compare
rom1504
left a comment
rom1504
left a comment
There was a problem hiding this comment.
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.
| } | ||
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good catch, thanks. Fixed in eb2e358. Reading the seen-message list no longer changes it; a new markAcknowledgementsSent() updates it after a packet that actually reports it (chat_message, chat_command_signed, or chat_command on versions before the split). Added a test for an unsigned /list followed by chat, checking the chat reports a count of 1, and one confirming the pre-split chat_command accounting is unchanged.
Once chat_command_signed is split out, chat_command carries only the command. Sending it no longer marks last-seen entries as acknowledged or resets the pending count, so the next chat_message reports the offset the server has not yet received. Versions where chat_command still carries the acknowledgements keep their accounting.
|
Checked eb2e358 on seven versions from 1.19.3 through 26.1 using the actual chat plugin and compiled packet codec. Unsigned commands preserve acknowledgements for the next chat; signed commands and older combined packets still clear them correctly. The 65-message acknowledgement threshold also holds. All five command tests pass with current master. These were synthetic checks, not a vanilla-server run. |
rom1504
left a comment
rom1504
left a comment
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
Looks ready to merge from this review. The acknowledgement mutation now happens only after a packet actually carries it. A focused run of the actual chat plugin plus compiled codecs passes on 1.19.3, 1.20.4, 1.20.5, 1.21.4, 1.21.8 and 26.1: unsigned commands preserve pending acknowledgements on split-packet versions, and the next chat reports them; older combined packets still consume them. The branch's four new tests pass. Its older command-tree fixture lacks rootIndex, already fixed on master by #1527: I fetched GitHub's actual merge result (1f1630f), verified chat.js is identical to this head, and all five command tests pass with that merged fixture. Current CI is green. No vanilla-server run.
Skills used: prismarine-lifecycle-action-review traced acknowledgement ownership through consecutive sends; prismarine-protocol-data-review checked actual compiled packet fields before and after the packet split. prismarine-review checked current discussions and avoided duplicate findings.
| }) | ||
|
|
||
| it('sends a command with no signable argument as the unsigned packet', () => { | ||
| const client = new EventEmitter() |
rom1504
left a comment
rom1504
left a comment
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
Correction to my earlier “Looks ready to merge” assessment: I withdraw that readiness recommendation for eb2e358. The focused checks supported the acknowledgement repair, but did not establish that the PR satisfies the subsequently requested integration coverage. The maintainer’s real-client request is still outstanding at this head. This corrects the earlier Astra verdict; it is not an additional defect report or a second test request.
Skills used: prismarine-review separated correctness evidence from merge readiness and checked the later maintainer feedback; prismarine-protocol-data-review distinguished stubbed client writes from coverage of the production client/server path. No new tests were run in this pass.
3 participants
Constraints on the serverbound command packet:
chat_command_signedis written only when the command produced at least one argument signature./login,/list, anything the server's command tree does not mark as a message argument) is written aschat_command, whether or not the client can sign.Vanilla reference:
ClientPacketListener.sendCommandsends the unsignedServerboundChatCommandPacketwhenSignableCommand.of(...).arguments()is empty.Tests:
/login hunter2and an argumentless/msgwritechat_commandwith 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.