Skip to content

feat(anvil)!: make the version guard and the unknown-entry fallback replaceable services - #47

Merged
TheMeinerLP merged 18 commits into
mainfrom
feat/anvil-extension-points
Aug 4, 2026
Merged

feat(anvil)!: make the version guard and the unknown-entry fallback replaceable services#47
TheMeinerLP merged 18 commits into
mainfrom
feat/anvil-extension-points

Conversation

@TheMeinerLP

Copy link
Copy Markdown
Contributor

Proposed changes

falco-anvil had two decisions fixed in code that are really policy:

  1. Which worlds are readable. #45 refuses
    anything below the version floor or carrying a Level compound. Right for a server that only
    wants worlds it can read, wrong for a tool that wants to inspect one it cannot.
  2. What an unknown palette entry becomes. BlockPaletteResolver substitutes air,
    BiomePaletteResolver substitutes plains, both counting it. A reasonable last resort for a server
    keeping a world loadable; the wrong reaction for anything that converts or audits one, because a
    substitution that is counted is still a substitution written back on the next save.

Neither is wrong. Both are policy, and policy only one consumer can choose is not policy.

Both become services, discoverable through the platform ServiceLoader or settable through a builder
slot. PaletteEntryResolver already proved the shape; what it lacked was discovery.

FalcoAnvilLoader.builder()
    .versionPolicy(myPolicy)          // explicit
    .discoverUnknownEntryPolicy()     // or from the classpath
    .build(worldRoot, OVERWORLD);

UnknownEntryPolicy returns a substitute name, not an id"minecraft:air",
"minecraft:plains", or whatever the caller wants — or throws to fail the chunk. The resolver
resolves the name, because it holds the registry anyway. That shape was not the first draft; see
below.

Resolution rules, uniform across both services

  • The shipped default steps aside for a foreign provider. Without this, falco-anvil registering
    its own implementations would mean a third party taking the documented route always produces two
    providers and always gets the refusal — discovery could never return anything but the default, and
    the extension point would be decoration.
  • Two foreign providers throw, naming both. The default is not among the names.
  • Builder slot and discovery are exclusive; the slot short-circuits before the class path is read.
  • Discovery loads with the service's own class loader, not the thread context loader. Under
    CloudNet or extension class loaders the context loader may not see the jar; discovery would find
    nothing, resolve to no policy, and silently put the pre-21w43a air chunk back.

Resolution happens once, when the loader is built.

One deliberate behaviour change, decided with its cost stated

The version guard may be absent entirely: no registered provider means no check. That is a
deliberate decision, not an oversight, and its consequence is that a world older than snapshot 21w43a
loads as air again — no error, no log line. Three things keep it from being something anyone stumbles
into:

  • falco-anvil registers its own guard, so losing it takes an exclusion somebody writes rather than
    an empty class path.
  • The startup line that already reports the chosen region layout now reports the resolved policy, or
    none.
  • testWithoutAnyPolicyALegacyChunkIsNotChecked asserts the chunk decodes to air. The cost is in
    executable form, so nobody has to take a paragraph's word for it.

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Bugfix (non-breaking change which fixes an issue)
  • Performance change (behaviour unchanged, cost changed)
  • Documentation Update (if none of the other choices apply)

The break is behavioural and narrow: build(...) now refuses a caller who configures an
UnknownEntryPolicy and supplies a custom resolver, because the custom resolver would silently
swallow the policy. Every signature change is additive; checkApiCompatibility reports "No changes."

Checklist

  • I have read the CONTRIBUTING.md
  • The title of this pull request is a Conventional Commit
  • I have added tests that prove my fix is effective, written before the implementation and watched fail for the right reason
  • Tests are package-private, named test<What><Expectation>, and use plain JUnit assertions
  • Every new or changed class and method carries Javadoc with @param / @return / @throws
  • I have not written @NotNull
  • ./gradlew build is green locally
  • I have added necessary documentation — README plus the existing Anvil-Chunk-Loader wiki page (wiki commit held back locally until this lands)

Counts from the JUnit XML, all six modules, --rerun-tasks: anvil 258, light 223, instance 259,
demo 167, benchmarks 42 (1 skipped), archunit 47. No count fell.

Concurrency

Both services are resolved once and then called from every parallel chunk load.

  • I have stated below which state the change adds or shares, and what guards it
  • Any new mutable state is either confined to one call or explicitly documented as thread-safe
  • I have added or extended a *ConcurrencyTest — see below

What is shared, and how is it guarded? No shared mutable state is introduced. Both shipped
defaults are stateless — they hold only static final constants and no instance fields.
ServiceLoader creates a fresh instance per loader build, so discovery shares nothing. The one route
to sharing is an explicitly passed policy on a reused builder, which every loader built from it then
shares; both policy slots document that, the way the diagnostics slot already did. Both interfaces
now state that implementations are called concurrently and must be thread-safe.

No new *ConcurrencyTest: the change adds no state for one to exercise, and a test over stateless
defaults would assert nothing.

Measurements

No performance claim is made anywhere in this branch, and none may be quoted from it.

Further comments

The interface was rebuilt mid-flight, and the reason is worth recording. UnknownEntryPolicy
first returned an int id, which meant the default implementation resolved Block.AIR and the biome
registry itself. That tripped three ArchUnit rules — among them
dynamicRegistryOnlyInBiomeResolver, whose stated reason is that the registry lives "in exactly one
adapter"
. Two review rounds had waved the resulting duplication through as acceptable; the
architecture rules were sharper than the reviews. Returning a name instead put the registry back in
one place and removed the duplication, its double-checked locking included.

The rules only fired in the acceptance run, because no task had run :falco-archunit:test — a gap in
the plan, not in the work.

A fourth rule, byteLayerKnowsNoNbt, was widened rather than worked around: its allow list is
hand-maintained, and its own Javadoc wrongly promised it needed no maintenance, which is exactly why
nobody extended it. The promise is corrected. That rule is a layering statement about the byte
level, not the cardinality statement the registry rule makes — the policies sit a layer above and
carry NBT in their contracts by design.

Two findings are recorded as deliberate rather than fixed: checkVersion labels every policy refusal
as a version refusal (reachable only with a foreign policy throwing a non-version Reason;
diagnostics, not control flow), and it dereferences a @Nullable field under the caller's guard,
documented at both ends.

🤖 Generated with Claude Code

https://claude.ai/code/session_016jtJ4GUtmyCSHkiGY1CvgR

TheMeinerLP and others added 18 commits August 4, 2026 14:36
Stacked on feat/anvil-version-guard rather than main, because the plan moves the body of
requireReadableVersion and that only exists on #45. Rebase onto main once #45 lands.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Moves the decision logic of requireReadableVersion into ChunkVersionPolicy /
DefaultChunkVersionPolicy, unchanged, and resolves it through
ServiceResolution.choose the way Task 1 built it. The loader keeps counting
and logging a refusal itself; the policy only decides and throws. A builder
that never touches versionPolicy()/discoverVersionPolicy() keeps discovering
the default, so a plain constructor call refuses the same chunks it always
did. versionPolicy(null) is the only way to skip the check entirely.
Review of the version-guard service extraction found the discovery path
was unusable for any third party: falco-anvil registers its own
DefaultChunkVersionPolicy, so a caller who registered a policy the
documented way always hit "two providers" and got refused. Teaches
ServiceResolution.discover/choose an optional shippedDefault class that
steps aside for any single foreign provider; two foreign providers still
refuse each other, so "no silent choice between undocumented opinions"
still holds.

Also: ServiceLoader.load now pins the service's own classloader instead
of trusting the thread-context one, which is not reliable across a
CloudNet/extension/plugin boundary; documents the new IllegalStateException
on the three public entry points that can now throw it; adds the
pass-through tests the two new builder setters were missing; and corrects
a wrong test count in the task report.
…oration

The resolution rules as written made discovery useless: falco-anvil registers its own provider, so a
third party taking the documented route would always produce two and always get the refusal.
Discovery could never return anything but the default. Found by the review of the first
implementation, fixed there, and now corrected here so the document does not keep teaching the
version that does not work.

Two rules gained along the way. Only foreign providers are counted for the ambiguity check, so the
default never appears in the error message. And discovery loads with the service's own class loader
rather than the thread context one - under CloudNet or extension class loaders the context loader may
not see the jar, and discovery would then silently find nothing and put the air chunk back.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- PaletteEntryResolver.toId's Javadoc now describes the contract this
  task's own change created: an implementation is allowed to fail,
  unchecked, instead of unconditionally promising a substitute.
- Add the biome mirror of every UnknownEntryPolicyTest case
  (BiomePaletteResolver had none before this).
- Narrow BiomePaletteResolver's three-argument constructor to
  package-private; it has no production caller and testability alone
  does not justify public API.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016jtJ4GUtmyCSHkiGY1CvgR
…and in

#45 merged with a breaking-change marker, so release-please raised the next version to 2.0.0 (PR
#46). Everything on this branch is additive on top of that, which makes it 2.1.0 - not the 1.2.0 the
tags carried from when this branch was written against a 1.x line. Nineteen tags across ten files.

@Version tags are untouched: those count a class's own revisions, not the artefact's.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ion it found

Six modules re-run with --rerun-tasks, both gate attacks re-executed and reverted, javadoc/japicmp
re-verified. falco-anvil grew from 230 to 253 cases and every other module held; falco-archunit's
ForeignCouplingTest now fails 4 cases because its allow-list regex was never extended for the new
policy classes — reported here rather than fixed, per the acceptance's own instruction not to
silently repair a defect found during acceptance.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016jtJ4GUtmyCSHkiGY1CvgR
… resolving one

DefaultUnknownEntryPolicy resolved Block.AIR and the biome registry itself, which archunit's
ForeignCouplingTest rightly refuses (registry access belongs to exactly one adapter per kind).
UnknownEntryPolicy now returns a palette name instead of an id; the resolver, which already holds
the registry from the original lookup, resolves the substitute itself and fails the chunk - without
asking the policy twice - if that name is unknown too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016jtJ4GUtmyCSHkiGY1CvgR
ChunkVersionPolicy, DefaultChunkVersionPolicy, UnknownEntryPolicy and
DefaultUnknownEntryPolicy carry NBT in their contract by design, one
layer above RegionFile's pure-byte guarantee that this rule actually
protects. Add all four to ANVIL_NBT_LAYER and correct the rule's
javadoc, which wrongly claimed the exemption was a self-maintaining
complement rather than the hand-maintained name list it always was -
the reason nobody extended it when these classes were added.
…ed the gap

Three commits landed since the last measurement (fe569f5, 4985431, a7f7b57): UnknownEntryPolicy now
names a substitute instead of resolving one, ANVIL_NBT_LAYER was widened and its Javadoc corrected, and
README gained the replaceable-policy paragraph. Re-ran all six modules and the build/javadoc/japicmp
check against the new tip: falco-archunit is back to 47/47, falco-anvil grew to 255 cases, no other
module moved. Replaces the previous Result section rather than appending a second one, and keeps the
archunit defect's discovery and actual fix in the history instead of quietly re-measuring it away.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016jtJ4GUtmyCSHkiGY1CvgR
…ownEntryPolicy

FalcoAnvilLoader.Builder.build(...) silently dropped the configured
UnknownEntryPolicy whenever a caller also supplied their own
blockResolver or biomeResolver: the resolver is used exactly as
given, so the policy was never reached even though
loader.unknownEntryPolicy() kept reporting it as active. A caller
who set both, e.g. unknownEntryPolicy(strict).blockResolver(mine),
got a loader that silently substituted air for an unknown block
instead of enforcing the configured policy.

The constructor now refuses that combination with an
IllegalStateException naming the slot to use instead, applying the
same standard ServiceResolution.choose already holds for explicit
configuration versus discovery. A resolver configured without
touching either unknownEntryPolicy slot is unaffected: a new
unknownEntryPolicyConfigured flag on the builder tracks whether
unknownEntryPolicy(...) or discoverUnknownEntryPolicy() was actually
called, separate from the default value both already carry.

Verified with a temporary mutation: disabling the new guard turned
the two new tests red, confirming they actually exercise it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016jtJ4GUtmyCSHkiGY1CvgR
…can be shared

ChunkVersionPolicy and UnknownEntryPolicy are resolved once when a
loader is built and then consulted by every parallel load
afterward, since FalcoAnvilLoader.supportsParallelLoading() is
true. Neither interface said so, leaving a foreign implementer to
discover the requirement by reading the loader rather than the
contract. Both javadocs now state the requirement explicitly and
note that the shipped defaults are stateless.

The two builder slots that accept an explicit instance
(versionPolicy(...), unknownEntryPolicy(...)) now carry the same
note the diagnostics slot already implies: an instance passed there
is shared by every loader built from that builder afterward, so it
has to tolerate exactly the concurrent, shared use the interface
javadoc now documents.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016jtJ4GUtmyCSHkiGY1CvgR
testWithoutAnyPolicyALegacyChunkIsNotChecked only asserted
assertNotNull on the loaded chunk, even though its own javadoc
claims "it loads ... as a chunk of air" -- nothing verified that.
The test now reads a block back with the existing blockAt(...)
helper and asserts it is Block.AIR, so the assertion matches what
the test documents.

Verified with a temporary mutation: changing the expected block to
Block.STONE turned the test red, confirming the assertion is
actually exercised.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016jtJ4GUtmyCSHkiGY1CvgR
…usal message

testTwoForeignProvidersAreStillRefusedEvenWithAShippedDefaultRegistered
only checked that the two foreign provider names appear in the
refusal message, never that the shipped default's name does not --
the other half of the rule the spec states: a default that stepped
aside for a foreign provider is not one of the competing candidates
and should not be named as one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016jtJ4GUtmyCSHkiGY1CvgR
…o what shipped

The design section still declared int onUnknownBlock(...) /
int onUnknownBiome(...) and said "returning an id substitutes it,"
but the interface that actually shipped returns String, not int.
The registry lookup that turns a name into an id belongs in the one
adapter that already owns it -- the resolver -- not duplicated into
every UnknownEntryPolicy implementation, which is why the signature
changed during implementation. The plan's Result section already
documents this as superseded; only the design section still showed
the old shape.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016jtJ4GUtmyCSHkiGY1CvgR
@TheMeinerLP
TheMeinerLP requested a review from a team as a code owner August 4, 2026 14:46
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Test results

  294 files    294 suites   8m 22s ⏱️
  973 tests   972 ✅ 1 💤 0 ❌
2 946 runs  2 944 ✅ 2 💤 0 ❌

Results for commit f41530b.

@TheMeinerLP
TheMeinerLP merged commit 94dc761 into main Aug 4, 2026
8 checks passed
@TheMeinerLP
TheMeinerLP deleted the feat/anvil-extension-points branch August 4, 2026 14:54
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