Conversation
…1-13)
P0-3 — null-guard VariableManagerImpl.shutdown() in onDisable so a
mid-enable failure no longer NPEs and masks the real exception.
P1-4 — cache AddonManager.availableNotLoaded() with 2s TTL. Tab
completion was opening N jars + parsing HOCON synchronously per
keystroke. Cache invalidates on loadOne/rescan so successful loads
appear immediately.
P1-5 + P1-7 — closing the cross-addon sabotage hole:
* moved AbstractMenusApiImpl, TypeRegistryImpl, ProviderRegistryImpl
out of ru.abstractmenus.api.* (where they leaked package-private
visibility into the public API package) into ru.abstractmenus.impl.*
* removed unregisterAll(MenuExtension) from public TypeRegistry and
ProviderRegistry interfaces — addons can no longer call it via the
documented API surface to wipe another extension's registrations.
* AddonManager.rollbackRegistrations now casts to *Impl. Also
extended to call providers().unregisterAll, which was missing.
* Tests + ProviderRegistryImplTest moved alongside.
P1-6 — Logger.set is now set-once (throws IllegalStateException on
re-install). Previously any addon could replace the global logger to
silence or capture core's output.
P1-13 — TypeRegistryImpl now actually drops Class entries from
NodeSerializers' backing map on unregisterAll (via reflection, since
hocon 1.0.6 has no public unregister). Without this, a Class held by
NodeSerializers kept the addon's closed AddonClassLoader alive, leaking
native FDs and all loaded classes.
…-1, P1-2, P1-3)
Previously several entity-touching paths routed through BukkitTasks.runTask
or Bukkit.getServer().getGlobalRegionScheduler(). On Folia, the global
scheduler is forbidden from touching entity state; doing so causes a
threading violation.
BukkitTasks now exposes runForEntity / runForEntityLater / runForEntityTimer
which dispatch via FoliaLib's entity scheduler on Folia and fall through
to runTask* on Spigot/Paper. All scheduling methods also gracefully
degrade to synchronous execution when BukkitTasks isn't initialised
(pure-unit test contexts).
Sites migrated:
* MenuManager.UpdateTask - menu.update(player) now dispatched per-player
via runForEntity. The outer iteration still runs on the global
scheduler; each tick of inventory work happens on the player's region.
* InventoryListener.onClick - player.updateInventory now via
runForEntityLater.
* InventoryListener.onInventoryClose - MenuManager.closeMenu (which
calls player.closeInventory) now via runForEntityLater.
* ChatListener.onChat - action.input dispatch now via runForEntity
(the input callbacks open menus and run actions on the player).
* ActionCommand.activate - player.performCommand wrapped in
runForEntity; console branch now uses BukkitTasks.runTask instead of
direct Bukkit.getServer().getGlobalRegionScheduler() (consistent
abstraction).
* ActionPlayerChat.activate - messages.forEach(player::chat) wrapped
in runForEntity.
* ActionTeleport.activate - switched from sync teleport (throws on
Folia cross-region) to teleportAsync.
TaskHandle gains a NOOP sentinel returned by runTaskTimer/runTaskTimerAsync
when BukkitTasks is uninitialised.
Cumulative breaking changes since 2.0.0-alpha: - Logger.set is now set-once (an addon cannot replace the global logger to silence or capture core's output). - TypeRegistry.unregisterAll(MenuExtension) and ProviderRegistry.unregisterAll(MenuExtension) removed from the public API surface. Cleanup is internal to AbstractMenus' addon manager. Addons cannot wipe each other's registrations through the documented API. - AbstractMenusApiImpl, TypeRegistryImpl, ProviderRegistryImpl moved from ru.abstractmenus.api.* to ru.abstractmenus.impl.* so the impl classes no longer leak package-private visibility into the public API package. Plus Folia correctness fixes (entity-aware BukkitTasks variants, sync-teleport replaced with teleportAsync, etc.) and the TypeRegistry NodeSerializers cleanup that was leaking closed classloaders. PlayerPointsAddon needs a rebuild against this version to pick up the relocated impl classes (it doesn't reference them directly, but recompilation refreshes its bundled api jar). README badge bumped from 1.18.0 (very stale) to 2.0.0-alpha.2 and the broken 'AbstractMenus/plugin' org/repo links pointed at the live 'minecraft-plugin' repo. Build output path updated to reflect the multi-module + dist aggregator layout.
P1-9 - pluginSoftDependencies in addon.conf is now actually consulted. loadAll/enableSingle log a warning per missing soft-dep at addon enable time, then proceed (soft means soft - no markFailed). The field had been parsed and stored since Phase B.2 but no consumer read it. P1-10 - extracted checkPluginDeps(LoadedAddon) private helper. loadAll and enableSingle previously duplicated the iteration with slightly different formulations (different log message wording, different early exit shapes). Now both delegate to one helper. Side benefit: the test mode (plugin == null) skip lives in one place. P1-11 - ProviderRegistryImpl.resolveWithConfig is now atomic. Previously it called configDefaults.apply(kind), then section.byId(), then section.auto() as separate operations, each section call acquiring its own monitor. On Folia (or any context where addon reload is concurrent with provider lookup) there was a window where the configured id read referred to a provider the unregister had just dropped. Section now exposes resolveWithDefault(String) which holds one lock across both the byId-by-configured-id and the auto-resolve fallback. P1-12 - ActionLuckPermsMetaSet/Remove now log a warning when the active permissions provider is not LuckPerms (instead of silently no-op'ing). The operator's menu config did request meta operations, so quietly dropping them is the wrong default - now they get a one-line server-log hint pointing at config.conf providers.permissions = "luckperms".
ProviderRegistry had 25 parallel methods (5 per section × economy /
permissions / levels / placeholders / skins) - each section's
register / get / get(id) / all / has lived directly on the registry
interface. Adding a sixth provider type would have meant adding 5+
methods on the public interface and threading a new field through
every consumer that wanted to enumerate sections.
Refactored to ProviderSection<T> with one shape per section:
void register(String id, T handler, int priority, MenuExtension owner);
T resolve();
T resolve(String id);
Collection<T> all();
boolean has(String id);
ProviderRegistry now exposes 5 typed-section getters:
ProviderSection<EconomyHandler> economy()
ProviderSection<PermissionsHandler> permissions()
ProviderSection<LevelHandler> levels()
ProviderSection<PlaceholderHandler> placeholders()
ProviderSection<SkinHandler> skins()
Migration:
api.providers().registerEconomy(id, h, p, o)
-> api.providers().economy().register(id, h, p, o)
api.providers().economy() (returns EconomyHandler)
-> api.providers().economy().resolve()
api.providers().economy("vault") (returns EconomyHandler by id)
-> api.providers().economy().resolve("vault")
api.providers().allEconomy()
-> api.providers().economy().all()
api.providers().hasEconomy(id)
-> api.providers().economy().has(id)
(Same shape for permissions / levels / placeholders / skins.)
Adopted across 128 in-tree call sites. The atomic resolve-with-default
that landed in P1-11 is now baked into ProviderSection.resolve() - the
single critical section guarantees that the configured-id read and the
priority-fallback selection cannot race with a concurrent register /
unregister.
Breaking for downstream consumers. PlayerPointsAddon (separate repo)
needs a one-line update on the call site:
api.providers().registerEconomy(...) -> api.providers().economy().register(...)
AddonManager had fully-qualified java.util.jar.JarFile, java.nio.file.Files,
java.io.IOException, java.nio.charset.StandardCharsets, java.net.URL, and
ru.abstractmenus.api.MenuExtension peppered through the body. Hoist them
into the import block so the code reads like Java instead of like a textbook
example.
Drop column-aligned padding in:
- AddonConf.parse() variable declarations
- ProviderRegistryImpl section field initializers + getters
Replace stale {@link ProviderRegistry#registerEconomy} (and friends) javadoc
references with {@link ProviderSection#register} since the parametric
refactor moved register() onto the section type. Five handler-interface
javadocs touched.
P2-7 - dedup the open-jar-read-addon.conf-parse triple. findJarByName and availableNotLoaded both did the same scan; extracted scanJarConfs() that returns Path -> AddonConf for every parseable jar. The two callers now filter the same map. discover() stays separate because it ALSO builds the AddonClassLoader, which we do not want for the tab-completion path. P2-8 - replaced the nullable plugin field with a PluginDepChecker functional interface plus a parentClassLoader field. The production constructor wires both up from the AbstractMenus instance; the test constructor uses ALL_PRESENT (every name reports loaded) and the addon module's own classloader. Result: zero 'if (plugin != null)' guards in the body, every method works the same way regardless of whether tests or the real plugin is calling. P2-9 - Stage 2 of loadAll now also skips entries whose extension is null. In practice the FAILED check covers this (instantiate() throws inside the catch which calls markFailed), but the local-invariant guard makes the body NPE-proof if Stage 1 ever evolves to leave a non-FAILED-but-null state. P2-17 - reload(name) now goes through enableSingle(fresh) instead of inlining instantiate + onLoad + onEnable. enableSingle re-runs both plugin-dep and addon-dep checks, so a dependency that disappeared between the original load and this reload causes a clean FAILED state with the expected error message instead of a confusing trace deep inside onEnable. Bonus: the cached availableNotLoaded list is invalidated as part of reload.
…4 / P2-5) P2-1 - cap addon.conf reads at 64 KB (a real addon.conf is well under 1 KB). readBoundedAddonConf checks both the declared uncompressed size in the zip header AND the actual byte count during read, so a crafted jar can't lie about either to OOM the server. Applied at both readAddonJar (production load path) and scanJarConfs (tab completion + findJarByName paths). P2-2 - add net.minecraft.* and com.mojang.* to AddonClassLoader PARENT_FIRST_PREFIXES. Paperweight-userdev exposes remapped NMS internals to the plugin classloader; without parent-first an addon could ship its own copy of, say, net.minecraft.nbt.CompoundTag and have it hide the real one for any code that crosses the addon boundary, causing ClassCastException-on-cross-loader-cast or, worse, silent class substitution. P2-3 - Colors.init is now set-once. Subsequent calls (whether from a malicious addon trying to flip the RGB mode, or from /reload running onEnable a second time) are silently skipped. The skip-rather-than- throw keeps /reload working; trade-off is that flipping useMiniMessage in config requires a server restart. P2-3 (cont) - same skip-if-set treatment applied to Logger.set, which the previous round had thrown IllegalStateException on second call. The throw broke /reload because Bukkit calls onEnable again without re-creating the static field. P2-4 + P2-5 - CommandAddons now strips legacy &x, section-sign §x, and hex <#RRGGBB> tokens from every addon-supplied string before rendering it through Colors.of into operator chat. Applied to: addon names, versions, descriptions, authors, dep lists, target API versions, exception messages, and the operator-typed name argument echoed back in error responses. Threat: a malicious addon's addon.conf name 'CRITICAL ERROR' formatted in red, or an exception message reading 'OK, password=XYZ' in green, could social-engineer the operator. Operators install third-party addons (semi-trusted) - the bar is not full sandbox but they should not be able to forge legitimate-looking server messages through normal /am addons output.
Paper bundles net.kyori.adventure (api / text-minimessage / serializer-legacy / examination / option) directly via paper-api. Our shadow jar was duplicating ~2.7 MB of those same classes for no benefit; on Paper, the classloader picks the plugin's bundled copy first which is wasteful and in some cases collides with internal Paper utilities. Changes: - Removed adventure-platform-bukkit:4.4.1 entirely. Verified zero references in plugin/api source (no BukkitAudiences, no adventure-platform imports anywhere). - Switched adventure-text-minimessage:4.26.1 to compileOnly. Compile resolution still works because paper-api's BOM exposes it; runtime uses Paper's bundled copy. Result: AbstractMenus-2.0.0-alpha.2.jar shrinks from 2.2 MB to 1.1 MB (-50%). No code changes needed, all 248 tests still pass.
P2-6 - extracted MoneyAmountSpec.parse(ConfigNode) shared helper. The scalar-or-map deserialization plus provider validation block was duplicated verbatim across ActionMoneyTake.Serializer, ActionMoneyGive.Serializer, and RuleMoney.Serializer. ~18 lines × 3. Each serializer is now a 2-line delegation. P2-13 - ProviderSection.resolve() and resolve(id) javadocs already state @return null on empty / missing. Verified the contract is documented; no doc change needed beyond existing. P2-16 - provider error message now lists registered IDs ("vault", "playerpoints") instead of impl class names ("EconomyVaultHandler"). Added ProviderSection.ids() returning a Set<String> snapshot of registered ids; the money parse error builds its 'Registered: [...]' list from that. Operators can now copy from the error message straight into their config.conf without a mental class-name-to-id translation step.
…4 / P2-19) P2-10 - drop the dead try/catch around CoreActionsBundle's customSound registration. The 'SoundCategory missing on legacy Bukkit' comment referred to pre-1.9 servers; we target Paper 1.21.11. The catch was also broad (Throwable) and would have hidden any real registration error in ActionSoundCustom.Serializer construction. P2-11 - CoreExtension.version() previously returned null with a 'populated at runtime via api.apiVersion()' comment but no code actually populated it. /am addons list rendered the core entry as 'AbstractMenus-Core vnull'. Now captured in onEnable from api.apiVersion(). P2-12 - filterByPrefix moved from CommandAddons-private to a protected static helper on the Command base class. Same five-line filter could appear in any future tab-complete override; now they share one definition. P2-14 - swap java.util.Stack (extends Vector, synchronises every op) for java.util.ArrayDeque in Command.onCommand. Single-threaded dispatch path - no benefit from the lock, just GC noise. P2-19 - the WatchService 'main thread' comment in MenuManager was misleading. On Folia, BukkitTasks.runTask routes to the global region scheduler, not 'main thread'. Updated the comment to spell out what the dispatch actually does (and what it's actually for - ordering against /am reload, not thread-safety, since menus is already a ConcurrentHashMap).
P2-15 - addon dependency graph no longer poisons the whole batch when a single addon declares an unknown dep. AddonDependencyGraph.unsatisfied() returns the set of addons whose deps are missing; AddonManager.loadAll now pre-filters those (marking each FAILED individually with a clear 'missing addon dependency: X' message) BEFORE topoSort. topoSort itself is now strict-but-naive (cycle detection only) since all dangling refs are filtered upstream. The most common trigger was an addon depending on a Path 1 plugin-as-addon: that dep is real but lives in plugins/ rather than addons/, so AddonManager never sees it in the graph. Old behaviour: 'Addon X depends on unknown addon Y' kills every other addon in addons/. New behaviour: only X is marked FAILED; the rest enable normally. Test infra - AddonDependencyGraphTest's unknownDependency_throws() was asserting the now-removed throw. Replaced with two focused tests for unsatisfied(): one for missing-dep detection, one for the empty-on-clean case. P2-18 - left disablePlugin() inside onEnable's catch as is, but added a comment explaining the trade-off. The re-entrant disable was the agent finding's concern; P0-3 (null-guard around VariableManagerImpl.shutdown) already mitigated the actual NPE risk. Rethrowing instead would skip ALL our cleanup paths because Bukkit does not call onDisable for plugins whose onEnable threw - resource leaks worse than the cosmetic re-entrancy. P2-20 - similar accept-and-document for determineProxy()'s YAML reads. The files are <10 KB and the call fires once during onEnable before any gameplay can be affected. Comment notes 'acceptable startup-only IO'.
P3-1 - CommandAddons.tabComplete now passes the loaded-names sequence to filterByPrefix as a Supplier-style Iterable instead of materialising a fresh List on every TAB keystroke. Filter still allocates a List for the result, but the source pass is zero-copy. P3-2 - MenuExtension.name() and version() defaults no longer return null. name() falls back to getClass().getSimpleName(), version() to 'unknown'. A Path 1 plugin-as-addon that forgets to override these now shows up as 'PlayerPointsAddon vunknown' in /am addons output instead of 'null vnull'. P3-5 - VariablesDao.init dropped the bogus 'root', '' credentials from its SQLite getConnection call. SQLite ignored them but they read like a MySQL hangover. The two-arg overload is the canonical SQLite shape. P3-6 - AddonConf.parse now regex-validates the 'main' field against a loose Java class-name pattern (letter|underscore|dollar followed by identifier chars and dots). A typo like 'main = my-addon.Main' now fails with a clear 'not a legal Java class name' error during parse, instead of bubbling up as ClassNotFoundException several stages later when AddonManager tries to instantiate.
P0-1: 4 actions still routed entity work through the global region scheduler (ActionDelay, ActionMenuClose/Refresh/ItemRefresh on the ticks branch). Switched to runForEntityLater so the deferred close / refresh / chained activate runs on the player's region thread on Folia. P0-2: AddonDependencyGraph.unsatisfied() did a single pass, so a chain like A -> B -> missing C only flagged B and let A poison topoSort or onEnable. Now runs to fixed point and flags every node in the bad-closure. P1: TypeRegistryImpl.register() on overwrite kept the old owner's entry in keysByOwner, so the old owner's later unregisterAll wiped the new owner's registration. Strip k from every owner-set on overwrite. P1: Stage 1 onLoad failure now calls rollbackRegistrations - if a misbehaving addon registered a type before throwing, that type shouldn't bleed into the next addon's enable. P1: PlayerQuitEvent now removes the tracked menu directly. The 3-tick deferred close in InventoryListener is silently dropped on Folia if the player quits inside the window, which leaked entries in openedMenus. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
P2-S2: CommandAddons.safe() now strips MiniMessage tags too (<click:run_command:...>, <hover:...>, <red>, etc.). Without this a malicious addon could put a click-to-run-command tag in its name and turn an operator's /am addons list click into privilege escalation. P2-S3: NodeSerializers reflection failure now logs a clearer message that points at the bundled hocon lib (not AbstractMenus) as the source, and explicitly says the side effect is a memory leak until full server restart that compounds across reloads. P2-S4: AddonClassLoader.PARENT_FIRST_PREFIXES extended with org.spigotmc, ca.spottedleaf, io.netty, it.unimi.dsi.fastutil, gson, guava. An addon shipping its own copy of any of these would otherwise hit ClassCastException at the first hand-off back to server code (same FQN, different classloader = different class). P3-N3: CoreExtension.version() now returns "unknown" until onEnable populates it, matching the MenuExtension default and avoiding a "vnull" render in /am addons list during the brief onLoad-to-onEnable window. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Before: /am addons list only surfaced Path 2 (AM-loaded jars in
addons/). A plugin-as-addon's contributions were invisible — operators
had to cross-reference /plugins to figure out who registered what.
Now the list renders three groups:
- Path 2 addons unchanged
- Path 1 plugin-as-addons with [as-plugin] suffix
- The built-in CoreExtension with [built-in] suffix
Plumbing:
- TypeRegistryImpl + ProviderRegistryImpl expose seenOwners() — the
union of every MenuExtension that registered a type/provider.
- AddonManager.knownExtensions() unions all six registries.
- AbstractMenus keeps a reference to the CoreExtension instance so the
list command can identify and label it.
- /am addons info works for all three kinds. Path 1 surfaces Bukkit
PluginDescriptionFile (authors / depend / softDepend / status).
- tab-complete for info covers all three; reload still only completes
Path 2 (no jar to re-read for the others).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up after the API merge (#22). Multi-round code review fixes plus a few user-visible features.
Highlights
Folia correctness. Every entity-touching scheduler call moved off the global region scheduler onto
runForEntity*. CoversInventoryListenerclose/refresh,ChatListenerinput,MenuManagerper-player tick, and 4 actions (ActionDelay,ActionMenu{Close,Refresh},ActionItemRefresh) that Round 4 caught still routing through the global path. Also:player.teleport→teleportAsync,player.chatpinned to entity region, command dispatch split into player/console paths.Addon lifecycle hardening.
AddonDependencyGraph.unsatisfied()is now a fixed-point pass — A → B → missing C now flags both A and B instead of letting A poisontopoSort.TypeRegistryImpl.register()overwrite strips the old owner's tracking entry, so its laterunregisterAlldoesn't wipe the new owner's registration.PlayerQuitEventremoves the tracked menu directly — Folia silently drops the 3-tick deferred close if the player quits inside the window.NodeSerializersreflection unregister bumped to a clearer warning that names hocon as the source.Security.
CommandAddons.safe()strips MiniMessage tags too. Without this a malicious addon could put<click:run_command:/op X>in its name and turn an operator's/am addons listclick into privilege escalation.AddonClassLoader.PARENT_FIRST_PREFIXESextended (org.spigotmc,ca.spottedleaf,io.netty,it.unimi.dsi.fastutil, gson, guava) to prevent same-class-different-classloader CCE on hand-off.addon.confreads now capped at 64 KB.addon.conf mainvalidated against a Java-class-name regex before classloader sees it.API surface.
ProviderRegistryreshaped: 25 flat methods → 5 typed sections (economy(),permissions(),levels(),placeholders(),skins()) returningProviderSection<T>withregister / resolve / resolve(id) / all / ids / has.AbstractMenusApiImpl,TypeRegistryImpl,ProviderRegistryImpl) moved out of the api package intoru.abstractmenus.impl.*so addons can't reach them without a cast.MoneyAmountSpec.parse()shared byActionMoneyTake/GiveandRuleMoney.Build / packaging.
AbstractMenus-2.0.0-alpha.2.jar,AbstractMenus-api-2.0.0-alpha.2.jar.disttask aggregates shipping artifacts tobuild/dist/./am addonsUX.load <name>andrescansubcommands.listnow shows Path 1 plugin-as-addons with[as-plugin]and the built-inCoreExtensionwith[built-in].infoworks for all three kinds. Path 1 surfaces BukkitPluginDescriptionFile.reload(Path 2 only) andinfo(everything).ServerLoadEventinstead of the end ofonEnable, so Path 1 plugin-as-addons (which Bukkit only enables AFTER us due todepend: AbstractMenus) have a chance to register their providers before HOCON parse-time validation runs.Misc.
Logger.setandColors.initare set-once-skip instead of throwing on second call (so/reloaddoesn't crash).CoreExtension.version()falls back to"unknown"between construction and onEnable."root", ""credentials from the SQLite connection string (no-op but read confusingly).Known limitation
Cross-addon sabotage via
setAccessible(true)is still possible. Documented as a known limitation; not fixable without JPMS modules or removed/deprecated SecurityManager. In scope only if AbstractMenus ever ships an addon marketplace.