Skip to content

Apply ZacSharp's review of the Java pathfinder - #23

Open
0Mattias wants to merge 30 commits into
mainfrom
claude/pathfinder-review-round-four
Open

0Mattias wants to merge 30 commits into
mainfrom
claude/pathfinder-review-round-four

Conversation

@0Mattias

@0Mattias 0Mattias commented Sep 16, 2026

Copy link
Copy Markdown
Owner

ZacSharp reviewed cabaletta/baritone#5117 on Sep 14 and 15 and babbaj answered on Sep 16. This brings the fixes back from the fork's java-nether-pathfinder branch (0Mattias/baritone 1a3c2113 to 63a5813b, one commit per comment), the way #20 to #22 did for the earlier rounds.

  • the x8 summary is exact on every clear and the exact flag is gone: no whole-chunk writer ever clears a block, so the scan the flag avoided never ran for them, and isEmptyX8 and isEmptyX16 can no longer answer "not empty" for an empty cube
  • the "x8 is marked before its block" comment is gone; the class comment now says what keeps readers and writers apart, NetherPathfinderContext's lock, which every reader holds on the read side and every writer on the write side
  • the summary is a byte[], one byte per section, as its comment already said
  • insertChunkData and setChunkState are deleted; only tests called them, and the Oracle test helper now generates its chunks itself
  • a chunk's origin is a final boolean fromCaller instead of a volatile int
  • NodePos holds three ints instead of a BlockPos
  • the set of chunks generated around is a fastutil LongOpenHashSet
  • the package comment names the Minecraft classes the package uses

The pathfinder package and its tests stay byte for byte the branch's, modulo the package name and the licence header; NetherPathfinderContext and package-info.java take the same change by hand.

Also: this fork's own bugs, found by a survey of the codebase

Six Opus agents read the whole codebase before this round. Of what they flagged, these are the fork's own and are fixed here, each verified against upstream's 26.2 branch first:

  • the pre-tick mixin's slice still named Yarn's MinecraftClient.itemUseCooldown; Mixin resolves an unmatched slice start as the start of the method, so the pre-tick event fired on the first gui read in tick(), before gui.tick() and the missTime write, instead of after it as upstream places it
  • CalculationContext hardcoded the Frost Walker and Depth Strider levels to 0 since the Mojang-names migration; both now come from the enchantment components, as in upstream
  • the cached-chunk obsidian rule compared a floor-relative y against -59 and could never fire; it fires for the bottom five layers of the Overworld and the Nether now
  • every cabaletta/baritone link in the comments had become cabaletta/cheesecake, and one javadoc link pointed at a cheesecake.leijurv.com that does not exist; the two links a player can see point at this repo
  • Fixer, a reflection scratch class from the migration, shipped in the jar; it is gone
  • SETUP.md claimed a "remapped to intermediary" jar check that does not exist

Three findings are real but identical in upstream and are filed instead: #24 (a CALC_FAILED can be lost before the process sees it), #25 (special-block lookup y mismatch in the Overworld cache), #26 (MovementDiagonal tests the first corner twice for magma).

All 101 unit tests pass locally, and the in-world test passes in full on this branch with the fixes above in place: all twenty stages, the four elytra flights included, in two runs.

Second round: verifying this branch, and what it turned up

A sweep of the whole codebase (38 Opus finders and verifiers, then checked by hand) confirmed the commits above and found that f9d70ee, taken from upstream as it is, regressed pathing. Two in-world stages added here print the pathfinder's own costs for a one-block step:

step before f9d70ee after it with the fixes
into water, no Depth Strider 9.091 4.633 9.091
onto a lava source, Frost Walker II COST_INF 3.564 COST_INF
onto waterlogged stairs, Frost Walker II 3.564 throws 3.564
Frost Walker level with the boots on 0 2 2

Fixed, one commit each:

  • the Depth Strider multiplier starts at 0: upstream's 1.0 is Depth Strider III and priced a step into water like a step on land for everyone without the enchantment (upstream 26.2 has the same value; recorded in the README)
  • both canUseFrostWalker overloads are upstream's, a water source only and the execution side reading the enchantment; the fork's accepted lava and threw on waterlogged blocks, dormant while the level was hardcoded to 0
  • four upstream fixes that had been missed: Litematica schema version 7 (e66fdea5, with a unit test on the gate), the try/catch around the elytra goal (e545294d), BetterBlockPos kept out of the block entity map (76d6a98d), #path without a goal (511a2ae8)
  • the tool data component instead of Item.class and instanceof Item, which every item passes (cfb7970e)
  • PlayerUpdateEvent PRE after the vanilla player tick, as upstream: at the head of the tick the bot's aim became the player's previous rotation, and the camera swept between the aim and the view on every frame under free look; the new stage saw the previous yaw a quarter turn from the camera's on 39 of 40 ticks
  • an agentApiPort above 65535 no longer crashes the client on every launch, and a status request over the socket outside a world answers instead of throwing
  • the cache's dimension rules key on the dimension id again, as upstream: the End's dimension type has skylight in 26.2, so the flag proxy read cached End blocks as stone and the bedrock rule from 59dcb00 fired in the End, contrary to that commit's message
  • #find searches around the player's z, not its y, and spawner avoidance asks the cache for spawner, not mob_spawner; both inherited from upstream and recorded in the README

The README's verification section and the passEvents comment are brought up to date, and the inventory-pause stage can run on its own. 103 unit tests pass, and the two new stages, which fail on cdf2238, pass with the fixes.

🤖 Generated with Claude Code

0Mattias and others added 14 commits September 16, 2026 19:04
Brings back 0Mattias/baritone 1a3c2113 from the fork's java-nether-pathfinder
branch, as #20 to #22 did for the earlier review rounds of
cabaletta/baritone#5117.

ZacSharp's review, on Chunk.setBlock and isEmptyX8. The exact flag
existed so that the callers that fill a whole chunk would not pay the
scan that keeping the summary exact costs on a clear, but none of them
ever clears a block: writeChunkData, insertChunkData, BaritoneRegion and
the generator only set, and the block update, the one caller that
clears, already asked for exact. So the flag decided nothing, and with
it gone the summary is exact by construction, which answers the question
of whether isEmptyX8 and isEmptyX16 may say no for an empty cube: they
may not, and a summary that disagrees with the blocks is a bug in
whatever wrote them, as babbaj put it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Brings back 0Mattias/baritone 9e6432c3 from the fork's java-nether-pathfinder
branch, as #20 to #22 did for the earlier review rounds of
cabaletta/baritone#5117.

ZacSharp asked whether the JIT or the CPU may reorder the two plain
writes in setBlock, and babbaj asked why a race was expected there at
all. The comment claimed an order the language does not promise:
without a fence, a reader on another thread may see the block before
the summary bit. It was written for a reader that does not exist. Chunk
was ported to be safe on its own, like the native code, but every
reader in Baritone takes NetherPathfinderContext's read lock, the
solver thread, the game thread's tick and a search that does not
generate alike, while a block update, a chunk pack, a cull and a
generating search hold its write lock, so no reader can watch a block
being set. The class comment now says so instead of describing a race,
and the comment on the write goes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Brings back 0Mattias/baritone 17e1d5e2 from the fork's java-nether-pathfinder
branch, as #20 to #22 did for the earlier review rounds of
cabaletta/baritone#5117.

ZacSharp's review. A section has eight x8 cubes, so its summary needs
eight bits, and the class comment already called it a byte; the int
array was an oversight. filled(y) still returns an int, the byte masked
to its eight bits, so the raytracer's tests of it are unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Brings back 0Mattias/baritone cb2a12b2 from the fork's java-nether-pathfinder
branch, as #20 to #22 did for the earlier review rounds of
cabaletta/baritone#5117.

ZacSharp found it unused and babbaj said to remove it. It was the
native library's entry point for a chunk as an array of booleans; the
game's chunks come in through allocateAndInsertChunk and are packed
straight into the Chunk, and only a test called this. dimensionHeight
served only its length check and goes with it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Brings back 0Mattias/baritone 41ae7b9e from the fork's java-nether-pathfinder
branch, as #20 to #22 did for the earlier review rounds of
cabaletta/baritone#5117.

ZacSharp asked whether the chunk state should be an enum or a boolean,
and babbaj noticed that setChunkState, its only writer, is never
called, so the field should be final and not volatile. It is now a
final boolean, fromCaller, in place of the two int constants, and
setChunkState goes; the test helper that marked generated chunks as the
game's now generates them itself and copies the blocks into chunks it
inserts as the game's.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Brings back 0Mattias/baritone b018f34f from the fork's java-nether-pathfinder
branch, as #20 to #22 did for the earlier review rounds of
cabaletta/baritone#5117.

ZacSharp's review. The BlockPos was private and only ever read back
through its three getters, so the node holds the coordinates itself and
allocates one object fewer per node the search creates.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Brings back 0Mattias/baritone 921e1bad from the fork's java-nether-pathfinder
branch, as #20 to #22 did for the earlier review rounds of
cabaletta/baritone#5117.

ZacSharp's review, babbaj agreed. The set of chunks whose neighbours
the search has generated was a HashSet of boxed Longs; fastutil's long
set holds the keys unboxed. The keys are what NetherPathfinder.key
gives, as before.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Brings back 0Mattias/baritone 63a5813b from the fork's java-nether-pathfinder
branch, as #20 to #22 did for the earlier review rounds of
cabaletta/baritone#5117.

ZacSharp's review, babbaj agreed the comment was outdated: the package
took Minecraft's BlockPos, Direction, Vec3 and Mth in the first review
round and the comment still said it used nothing of Minecraft's.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CachedChunk prices what may be bedrock: a cached solid block in the
world's bottom five layers reads as obsidian, so a path prefers to break
known stone above them. The port compared a y that CachedRegion has
already made relative to the world's floor against -59, which no such y
is, so the branch was never taken; upstream's `y < minY + 5` has the
same flaw in the Overworld and only works in the Nether. The bottom
five layers are y < 5 in that coordinate, and the rule applies where
there is a bedrock floor, the Overworld and the Nether, as upstream
intends.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The migration to Mojang's names left CalculationContext with a Frost
Walker level of 0 and a Depth Strider level of 0, so the pathfinder
priced water as if the player wore neither and never planned a Frost
Walker crossing. Upstream's 26.2 branch reads both through the
enchantment components: the Frost Walker level from the item
enchantments of each equipment slot, and the water speed from the
attribute effect that Depth Strider carries. Taken as it is.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A main method that printed LocalPlayer's methods by reflection, left
behind by the Yarn to Mojang migration and shipped in the jar since.
Nothing refers to it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
SETUP.md said the build job checks that the jar is remapped to
intermediary. No such check exists, and none could: since 26.1 the game
ships with Mojang's names and there is no intermediary. The list now
matches build.yml.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The package rename turned every github.com/cabaletta/baritone link into
cabaletta/cheesecake, which does not exist, and the javadoc link to
baritone.leijurv.com into a host that does not exist either. The issue,
pull request and javadoc references in the comments point at upstream
again. The two a player can see, the unhandled-exception message and
the readme.txt written into the save's cheesecake folder, point at this
fork.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The slice that places the pre-tick event kept Yarn's name for its
anchor, MinecraftClient.itemUseCooldown, which has not existed since
26.1 shipped Mojang's names. Mixin resolves a slice whose start matches
nothing as the start of the method, so the injection landed on the
first read of Minecraft.gui in tick(), before gui.tick() and before
missTime is written, rather than on the read after that write, where
upstream puts it. The anchor is Minecraft.missTime now, as upstream's
is. The smoke and in-world jobs could not catch this: to Mixin a slice
that matches nothing is not an error.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
0Mattias and others added 13 commits September 16, 2026 22:03
f9d70ee took the water walk speed from upstream's enchantment
components as it is, and with it upstream's starting value: the
multiplier starts at 1.0, which is Depth Strider III, and stays there
for a player without the enchantment, so a step into water was priced
like a step on land. The attribute the enchantment adds is a third per
level, so a player without it has 0, and before f9d70ee the fork's
hardcoded level of 0 gave the right cost by accident. The in-world
frost-walker stage prints 4.633 for a step into water on the previous
commit and 9.091 with this one. Upstream's 26.2 branch has the same
starting value; recorded in the README's status section.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The fork's canUseFrostWalker accepted any block with a fluid in it and
read LiquidBlock.LEVEL off it: a lava source passed, so a player in
Frost Walker boots was pathed onto lava, and a waterlogged block has no
LEVEL property, so any waterlogged stair, slab or kelp in the search
threw IllegalArgumentException, which AbstractNodeCostSearch reports as
a failed calculation. Both were dead code while the level was hardcoded
to 0 and have been live since f9d70ee. The execution-side overload was
still hardcoded to false, so the executor never agreed with a crossing
the planner had priced. Both overloads are upstream's now: the block has
to be what frosted ice melts into, a water source, and the execution
side reads the enchantment off the equipment. From cabaletta/baritone
63d0064d (Wagyourtail) and 74f46cd6 (rfresh2).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Litematica has written version 7 since 1.21, and the gate still accepted
6 and refused 7, so no current .litematic loaded: "Unsuported Version of
a Litematica Schematic". Upstream moved the gate in cabaletta/baritone
e66fdea5 (rfresh2), refusing 6 as too old; taken as it is, with a unit
test on the gate.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
cabaletta/baritone e545294d (Babbaj): ElytraProcess.pathTo throws
IllegalArgumentException for a goal it cannot fly to, and setGoal let it
escape when called while flying. It is reported in chat instead.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
cabaletta/baritone 76d6a98d (mankool0): calculateBlockCenter hands its
position to Level.getBlockEntity, and a BetterBlockPos, with its own
hashCode, must not end up as a key in the chunk's block entity map. The
position is copied to a plain BlockPos first.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
cabaletta/baritone 511a2ae8 (ZacSharp): #path with no goal set took
control and reported "Now pathing". It says "No goal set" now.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The migration replaced upstream's PickaxeItem class check, which no
longer exists, with Item.class and instanceof Item, which every item
passes: bestToolAgainst considered every stack a tool, and the throwaway
fallback that has to select something in the main hand that does not
right-click picked hotbar slot 0 whatever it held. Both read the tool
data component now, as upstream does since cabaletta/baritone cfb7970e
(rfresh2).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The port injected PlayerUpdateEvent PRE at the head of LocalPlayer.tick,
where upstream injects it after AbstractClientPlayer.tick() returns. The
difference matters with free look: LookBehavior puts the bot's aim on
the player at PRE and restores the camera's rotation at POST, and
LivingEntity.baseTick, which runs inside that vanilla tick, stores the
rotation it finds as the previous one, the value the renderer
interpolates the camera from. With the hook at the head it stored the
bot's aim, and the camera swept between the aim and the player's view on
every frame while pathing: the free-look-camera stage saw the previous
yaw a quarter turn from the camera's on 39 of 40 ticks. After the
vanilla tick the previous rotation is the camera's, and the aim still
reaches the rotation packet, which the method sends later.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ServerSocket throws IllegalArgumentException, not IOException, for a
port out of range, so the start attempt escaped the catch in onTick and
took Minecraft.tick with it, and since the value was already saved,
every later launch crashed on its first tick until settings.txt was
edited by hand. The port is remembered as failed, like one that is in
use.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
AgentStatus guarded the player fields behind inWorld but still asked the
pathing behavior for its two tick estimates, which start from the
player's position, so a status request over the control socket at the
title screen answered with a NullPointerException instead of the out-of-
world snapshot AI_AGENT_README.md promises. The estimates are null
outside a world.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The migration dropped the dimension id upstream threads from WorldData
down to CachedChunk and ChunkPacker, and stood in for it with the
dimension type's flags: skylight for the Overworld, a ceiling for the
Nether, neither for the End. In 26.2 the End's dimension type has
skylight, so a cached solid block in the End read back as stone rather
than end stone, and the bedrock rule 59dcb00 fixed fired in the End's
bottom layers, which that commit's message wrongly says it does not. The
id is threaded through again as upstream has it and both rules use it;
the bedrock rule keeps comparing the floor-relative y with 5, which is
the fork's fix of upstream's comparison and is recorded in the README's
status section now.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
getLocationsOf takes the centre's x and z, and #find passed the player's
y for the z, so it looked in the regions around (x, y). Upstream's 26.2
branch has the same line; recorded in the README's status section.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The cache records special blocks under the block's registry path, and
the spawner has been "spawner" since 1.13, but the mob spawner avoidance
asked the cache for "mob_spawner" and never found one, so
mobSpawnerAvoidanceCoefficient did nothing. Upstream's 26.2 branch has
the same string; recorded in the README's status section.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
0Mattias and others added 3 commits September 16, 2026 22:03
The README said building, farming and following were only covered by the
unit tests, which stopped being true when the in-world test got stages
for them; it lists the stages now, the two this branch adds included.
The passEvents comment in MixinMinecraft claimed that a wrong @slice is
a hard crash at startup; it is not, Mixin moves the injection quietly,
which is what cdf2238 had to fix.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
frost-walker builds four walled rigs above the platform and prices a
one-block walk into a water source, onto a lava source, onto a
waterlogged stair and onto stone with MovementTraverse.cost, first
barefoot and then in Frost Walker II boots: water has to cost
WALK_ONE_IN_WATER_COST without Depth Strider, lava must never be
walkable, a waterlogged block has to price like the block it is without
throwing, and the boots' level has to be read. free-look-camera walks
north under free look with the camera looking east and checks at the
start of every tick that the player's previous rotation is the camera's
and not the bot's aim. Both fail on cdf2238 and pass with the fixes
before this commit. Two things the harness taught along the way: a
/fill, /setblock or /clear that changes nothing is reported as an error
and fails the stage, and a failing stage ends the run, so a stage that
is expected to fail has to run alone.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Its opening /clear is reported as a failure, which fails the stage, when
the inventory is empty, and it is empty whenever the stage runs alone
through CHEESECAKE_AUTO_TEST_ONLY; in a full run the stages before it
leave items behind. It gives itself a stick to clear first. The clear
at the end has the fillers and the pickaxe to remove and is fine.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.

1 participant