Skip to content

fix: read the lpVec3 continued scale as unsigned - #1542

Open
mc-zuri wants to merge 1 commit into
PrismarineJS:masterfrom
mc-zuri:lpvec3-unsigned-scale
Open

mc-zuri wants to merge 1 commit into
PrismarineJS:masterfrom
mc-zuri:lpvec3-unsigned-scale

Conversation

@mc-zuri

@mc-zuri mc-zuri commented Oct 3, 2026 •

Copy link
Copy Markdown

Split out of #1539.

lpVec3 read the continued scale as a signed int. A vector longer than 3 carries scale >> 2 in a trailing var-int that runs up to 2^32 - 1, and vanilla masks it back to unsigned:

scale |= ((long)VarInt.read(input) & 4294967295L) << 2

From 2^31 up (a component of 8589934592 or more) the scale came out negative: every component flipped sign, the magnitude collapsed, and the value no longer wrote back to the bytes it was read from. Writing was already correct.

Adds vanilla 26.3 bytes for scale >> 2 at each var-int length, either side of 2^31 and at the clamp. test/lpVec3Test.js passes with the fix (6 tests); with master's lpVec3.js, the new unsigned-scale test fails.

The test job runs mocha with -g <version>v, which never selects the version-independent lpVec3 suite, so a separate UnitTests job runs test/lpVec3Test.js without the filter.

🤖 Generated with Claude Code

A vector longer than 3 carries scale >> 2 in a trailing var-int. The
scale is clamped to 1.7179869183e10, so that var-int runs up to
2^32 - 1, and LpVec3.read masks it back to unsigned:

  scale |= ((long)VarInt.read(input) & 4294967295L) << 2

readVarInt returns an int, so from 2^31 up (a component of 8589934592
or more) the scale came out negative: every component flipped sign and
the magnitude collapsed, and the value no longer wrote back to the bytes
it was read from. The writer was already right.

Adds vanilla 26.3 bytes for scale >> 2 at each var-int length, either
side of 2^31 and at the clamp.

The test job runs mocha with -g <version>v, which never selects the
version-independent lpVec3 suite, so a separate UnitTests job runs
test/lpVec3Test.js without the filter.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@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.

Reviewed ee038fb: the unsigned continuation repair looks correct. Reinterpreting the decoded VarInt with >>> 0 before multiplying by four preserves the intended 34-bit scale without signed JavaScript coercion. Both interpreted and compiled NMP native-type registrations use this implementation. The new CI job also actually selects these version-independent tests.

Validation: the exact candidate's six lpVec3 tests pass locally with ProtoDef 1.19.0. A fix-removal control on f4ff7ffe00038080808008 returns x = -8589934592 before the repair and +8589934592 after it; the submitted fixtures also check decoded values, consumed size and write bytes through the continuation boundaries. I inspected the fixture claims but did not independently regenerate them from a vanilla server.

Code review looks good; CI needs resolution before calling the whole PR ready. UnitTests and the other version jobs pass, but the current test (26.1) run fails in packet_advancements with No value for type ItemStackTemplate from test/packetTest.js:466. That failure is in synthetic packet-value construction, outside the changed lpVec3 read path; it is not evidence against this unsigned-scale repair. Please resolve/rebase on the packet-fixture repair and rerun that job rather than weakening the assertion or hiding the failing version.

Skills used: prismarine-protocol-data-review checked signedness, read/write/size and production codec registration; prismarine-review checked real test subject, CI discovery, existing discussion and current-head failure evidence.

This branch has not been deployed

No deployments
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