| Age | Commit message (Collapse) | Author |
|---|
|
It was the one file store without a test, so the round trip it exists for - and
its two fallbacks, an absent file and an unparseable one - were only asserted
through PlayerSettingsManager. The write failure matters most: loading a torn
file discards every player's settings, so a failed save must leave the previous
file untouched.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Making a conversion timeout an ordinary exception put it in the same arm as a
hard API failure, where caching the hiragana fallback is deliberate - so one slow
reply recorded the unconverted form and that word rendered as hiragana for the
life of the cache. A timeout says the request was slow, not that the word has no
conversion, so it now returns the fallback without caching it and the next
message asks again.
The retry test builds its own remembering cache: the shared fixture's get()
always returns null, so against it the API is called every time and the test
would have passed even with the timeout cached.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
- getDirectMessageSpyPlayers had no callers left; handing out a copy of the spy
map is the shape notifySpies was introduced to replace.
- pluginScope was widened to public for "commands that must not block the tick
thread", but commands take the delivery queue and only LunaticChat reads it.
- getPlayerChannels returned a Result that cannot fail, so three callers carried
unreachable error paths and channel.status.error could never be shown.
- The cross-server managers were gated on velocityIntegration.enabled as well as
on a manager that is non-null only when it is enabled, letting the two
conditions disagree.
- LenientBoolean's non-YamlInput fallback was observationally identical to the
cast failing, since both land in ConfigManager's catch-all.
- sendCrossServerMessage caught its own failures underneath the delivery queue,
which already reports them without stopping the sender's later messages. The
second boundary is what forced a CancellationException clause here.
- handleOutgoingCrossServerMessage still recorded the reply target after the
commands took that over, so remote targets were recorded twice - re-inserting
entries clearPlayer had swept, which is the bug the local path was fixed for.
- The reason delivery is queued was written out in both command constructors and
twice more in KDoc; it now lives where the queueing happens.
Spy notification also defers its notice lookup and member set until a spy is
actually online, which is not the normal case.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Atomicity was opt-in per write site, so a file added later was safe only if its
author noticed the convention. Worse, DebouncedSaver drops a request while one is
pending and so serves exactly one file - a rule held up only by the wiring
happening to construct a separate saver per file, and written down nowhere. A
FileStore now owns its file, its atomic write and its own saver, so neither can
be got wrong by wiring; writeTextAtomically is internal to the package.
Taking the Bukkit plugin out of DebouncedSaver in favour of an AsyncScheduler
makes the debounce testable at all: ChannelStorage's "the snapshot is taken when
the write runs" now runs against the real thing rather than a mocked saver.
Teardown gets the same treatment. The five services with shutdown work spelled it
saveToDisk() three times and shutdown() twice, and the list of them was
hand-maintained against a fourteen-field container - so a new service was not
stopped unless someone remembered a second place. They now implement
StoppableService and register as they are built, and shutdown iterates that list.
stop() delegates rather than renames, because the conversion cache's periodic
flush is a different caller from shutdown.
Also here, on files this commit already touches: player settings carry a dirty
flag, since updateSettings is the only writer and queues its own save, so every
quit was re-serializing every stored player to write identical bytes; and
setPlayerChannel returns early when nothing moved, because the quit path clears
the active channel for every player whether or not they had one, and a mass
disconnect paid a full snapshot per player in one tick.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
An item takes up to the conversion timeout to drain, far slower than a player
can send, so an unbounded queue grew for as long as a macro ran - each item
holding its sender and recipient alive and arriving minutes after it was typed.
Refusing the overflow is at least visible to the player.
Releasing a player now cancels their worker instead of letting the backlog run.
Finishing it would spend a round trip per item writing to somebody who has left,
and keep both players reachable until it drained.
Chosen over DROP_LATEST on the channel: that reports success to trySend and
drops silently, which would make the warning about discarded work unreachable in
the case it was written for.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
withTimeout reports a timeout as a CancellationException, which callers must
rethrow rather than degrade. With api.timeout below convertWithRomaji's budget
the client's timeout therefore travelled through convertWord, through
withTimeoutOrNull - which rethrows a timeout belonging to another coroutine -
and into the queue worker, which read it as shutdown and ended its loop. The
channel stayed registered with nothing reading it, so every later message from
that player was buffered and never delivered: exactly the failure the worker's
own guard exists to prevent. A timeout is now an ordinary exception, so
cancellation once again means only cancellation.
The concurrency limiter also no longer covers the cache lookup. Its four
permits are shared by the whole server and held for a full round trip, so
cached words queued behind in-flight requests for a permit they did not need
and a message whose every word was cached could still exhaust the caller's
budget and go out unconverted.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Catching YamlException at the parse step covered kaml but not the scanner
underneath it: a file saved as UTF-16, or one truncated with NUL padding after an
unclean shutdown, fails inside snakeyaml-engine's reader with an exception that
is not a YamlException. It escaped onEnable and Bukkit disabled the plugin over a
config file - the very failure reading per setting was meant to prevent, and one
the catch-all this replaced had handled. The same gap swallowed the serializer's
own non-YamlException fallback.
A UTF-8 BOM is also stripped before parsing. It otherwise stays on the first key,
which strictMode = false drops as an unknown setting without logging anything, so
the operator sees exactly one setting ignored and no reason why.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
kaml rejects a document as a whole, so one unreadable value lost every other
setting the operator had written: velocityIntegration off, serverName "Unknown"
and channel chat disabled, behind one vague SEVERE line. The hand-written mapper
this replaced defaulted per key, so reading the file directly had quietly made
config.yml far more brittle than it was.
The offending setting is now dropped by the path its parse error carries and the
rest of the document is decoded, so the operator loses the one value they got
wrong and is told which. Only a document that is not YAML at all still costs
them everything.
Booleans also accept the YAML 1.1 spellings again. Bukkit read config.yml as
YAML 1.1, where `yes`, `no`, `on` and `off` are booleans; under kaml's YAML 1.2
they became strings, so `checkForUpdates: no` would have reset to its default -
which is the opposite of what the file says.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
readText sat outside ConfigManager, so the documented "fall back to defaults"
never covered the read itself. saveDefaultConfig only logs when it fails to
write the file, so the read can still find nothing there - and the IOException
then escaped onEnable and Paper disabled the plugin outright.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
CommandResult carried Adventure Components, which was engine's last
Minecraft dependency and the reason CLAUDE.md's "engine has no Minecraft
platform dependencies" was not quite true. It also meant a command could
not report a result without having already decided how it looks: every
site had to pick formatError versus format before it could return.
Results now carry text, and LunaticCommandBase.handleResult is the single
place that styles it - error red for Failure, normal for
SuccessWithMessage. The fail()/ok() helpers from #260 already funnelled
every call site through two functions, so this is a change to those two
plus the one command that composes its own success text.
engine's dependency list is down to kotlinx-serialization, and nothing
under engine/src references net.kyori, org.bukkit, com.velocitypowered or
io.papermc.
Not done: the review also proposed collapsing the per-command
`when (error)` blocks into one exception-to-key table. Those blocks pick
wording, not just a key - "only owners can delete this channel" reads
differently in the ban command than the delete command - so a shared table
would hand every caller the same sentence and need per-command overrides
on top. Left alone deliberately.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Every setting's default was written three times - in config.yml, in the
ConfigManager getter call, and on the data class - and copying the file
key by key is what made that necessary. They had already drifted:
checkForUpdates defaulted to false in ConfigManager while both config.yml
and the data class said true.
Worse, features.channelChat.messageLogging was documented in config.yml
with three settings and never parsed at all. ConfigManager did not build
it, so ChannelMessageLoggingConfig() always won and an operator editing
retentionDays or maxFileSizeMB changed nothing. Those settings now take
effect - the documented behaviour, but a real change for anyone whose file
disagrees with the defaults.
KAML deserializes the file straight into the tree, the same way player
settings and channel data are already read, so a default now lives only on
the data class. Two consequences worth stating:
- japaneseConversion.cache and .api are nested classes now, because the
data has to match the file rather than the file being flattened by hand
on the way in. The YAML is unchanged.
- api.retryAttempts is gone from config.yml. It was parsed and stored, but
never reached GoogleIMEClient or RomanjiConverter, so it documented a
knob that did nothing.
Unknown keys are ignored and a malformed file falls back to defaults with
a log line, so neither an old config nor a typo stops the server booting.
The tests parse real YAML instead of a mocked FileConfiguration, which
lets them cover what the mock could not: a partial file, a retired key, a
malformed document, and - the one that would have caught the drift above -
that the bundled config.yml equals the declared defaults.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
DirectMessageError.reason was a String backed by two constants, so the
receiving side matched one case and let everything else fall through to
"the target is offline". Adding a third reason on the proxy would have
shipped it to Paper servers that silently reported the wrong thing - the
one string-keyed dispatch sitting next to a protocol layer whose messages
are otherwise a sealed hierarchy with exhaustiveness checking.
As an enum, the reader must decide what to show for each case, and
CrossServerDirectMessageManager's when no longer needs an else.
The wire format is unchanged: kotlinx serializes an enum as its name, so
the existing snapshots still decode. What did need care is the reverse
direction - a reason from a newer proxy would now fail to parse, where the
String version degraded. The property has a default and the codec enables
coerceInputValues, so an unknown reason lands on TARGET_OFFLINE, exactly
the old else branch. There is a compatibility test for that case.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Closes #259.
engine exposed ktor through api(), so both platforms inherited the client
and its CIO engine. The only thing in engine that used ktor was
GoogleIMEClient, and the only module that used GoogleIMEClient was
platform-paper - Velocity was shipping roughly six megabytes of HTTP
client to support a Paper-only feature. Same story for
kotlinx-coroutines-core, which Velocity does not use at all.
Romaji conversion is a Paper feature, so the converter package now lives
in platform-paper alongside the ConversionCache and RomanjiConverter that
were already there. engine keeps kotlinx-serialization on api(), which is
genuine shared surface: the plugin messaging protocol is built on it.
The velocity shadow jar goes from 7,618,405 to 2,769,395 bytes, and no
longer contains io/ktor at all.
CacheData's tests were sitting inside engine's SettingsDataClassesTest,
which is unrelated to settings; they move with the class.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Neither ChannelStorage.saveToDisk nor ConversionCache had a test, so the
round-trip they exist for - and the reasons they skip work, discard a file, or
leave the previous one intact - were only asserted through their callers.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Both commands already record the conversation on the command thread before
queueing the delivery, for the same reason recordRemoteRecipient exists: /reply
reads the target there. Recording it again inside the queued work was not only
redundant but late, re-inserting entries that clearPlayer had already swept - so
lastMessager grew by one dead UUID every time a recipient quit mid-delivery.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Cancelling the scope kills the worker coroutines but does not close their
channels, so trySend kept reporting success for work nothing would ever read.
The warning that exists precisely to avoid dropping a message in silence was
therefore unreachable in the case it was written for.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
The catch-all around the Google IME call also caught the CancellationException
from the caller's timeout, and then cached the unconverted hiragana. Since the
words of a message are now converted concurrently, one timeout pinned every
word of that message to its hiragana form for the life of the cache instead of
just the word that timed out.
The same swallowing let a cancelled delivery carry on past the plugin scope
being cancelled, and reported the shutdown of an in-flight cross-server
message as a SEVERE delivery failure.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
The steps ran as one statement each, so the first exception escaped
onDisable and took the rest with it - leaving the channel message logger
unflushed and the Velocity connection to be torn down by the server rather
than by us. Each step is independent, so a failure is now reported and the
remaining ones still run.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Writing to a fixed sibling only moved the interleaving from the destination
to the temporary file: two saves racing there published mixed content, and
the losing move then failed with the temporary file already gone. Each write
now gets a unique temporary file, and falls back to a non-atomic replace on
the network mounts that refuse an atomic rename.
settings.yml and the conversion cache were still written in place. Both are
discarded wholesale when they do not parse, so a torn file silently costs
every player's settings or the whole accumulated cache.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Findings from reviewing the branch, in descending order of how much they
could hurt.
A message that threw ended the consumer loop while its channel stayed
registered in the map. Every later message from that player was then
buffered with nothing reading it, so their /tell and /reply stopped working
silently until they reconnected - one bad message format was enough. The
CoroutineExceptionHandler added earlier reported the throw but could not
bring the worker back. Each item now runs inside its own guard, so a failure
costs one message instead of the player's whole session.
Reply targets were recorded inside the queued delivery, but /reply reads
them on the command thread. /tell followed straight by /r therefore failed
with "nobody to reply to" for as long as the conversion took - up to the
full timeout. They are recorded before the work is queued now, which is
also where the sender expects the effect to happen.
Three narrower ones:
- A refused trySend was discarded without a word. It can happen once the
scope is cancelled at shutdown, and dropping a player's message in
silence is the worst way to handle it.
- Parallel conversion opened one API request per word with no ceiling, and
a repeated word in one line no longer shared the cache - the sequential
version got that for free by caching before the next lookup. Rate-limited
replies land in convertWord's catch and quietly degrade to hiragana, so
it is better not to ask that hard: distinct words only, four at a time.
- coerceAtLeast(1) on the cache interval turned a configured 0 into a whole
file rewrite every second, since any chat re-arms the dirty flag. A
non-positive value now falls back to the documented 300 with a warning.
ChannelStorage writes through a temporary file and an atomic move. Bukkit
runs onDisable before cancelling scheduler tasks, so the shutdown save can
overlap a still-pending debounced save, and two truncating writes to
channels.json would interleave into something unparseable. Debouncing made
that window much wider than the old runNow did.
TestLogger now captures log(level, msg, thrown); it only overrode
severe/warning/info, which do not route through each other, so anything
logged with a throwable attached was invisible to every assertion.
The two queue tests fail against the unguarded loop.
Left as review comments: delivery in flight can re-add a reply entry for a
player who just quit. resolveValidTarget filters unreachable targets on
read and the entry is cleared on their next quit, so the cost is a lingering
map entry, not wrong behaviour - and removing it would change what
sendDirectMessage promises, which a test pins.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Passing ::snapshot to the debounced write meant the three caches were read
from the writer thread while the server thread mutated them. They are
separate maps, so a snapshot could catch a channel already removed from
channelsCache while its membersCache entry still existed - persisting an
orphaned member list - or the reverse, persisting a channel with no members
and therefore no owner. Neither crashes anything, since the readers filter
on channelsCache, but they are wrong state written to channels.json and
carried across restarts.
The snapshot is taken on the mutating thread again and handed over through
a volatile field, so the write still reads the newest state when it
eventually runs rather than the state at queue time. That keeps both
properties: consistent halves, and a batched write that reflects every
change made during the delay.
This does not give back the per-change cost the earlier commit was avoiding,
because copying three maps is not what made saving expensive - the debounce
already coalesces the file write, which is.
Also floors the conversion cache interval at one second. It is now the only
writer besides shutdown, so a non-positive value would both be rejected by
runAtFixedRate and leave the cache unsaved until the server stopped.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Two regressions from taking the direct message path off the tick thread in
this branch.
A coroutine that threw reported to the JVM default handler, so nothing
reached the plugin log - and since the command had already returned
Success, a failed delivery was invisible to the player and the operator
alike. Before the change the exception propagated out of execute() into
Brigadier. PluginCoroutineScope now carries a CoroutineExceptionHandler,
which every caller that dispatches and returns depends on.
Launching a coroutine per message also dropped the ordering the synchronous
version had. Dispatchers.Default is a pool, so a cached romaji conversion
finishing in microseconds could overtake an uncached one sent before it:
the sender sees their second message first, and on the cross-server path
the relay to Velocity inverts too, so the recipient does as well. With
Japanese conversion on, mixing cached and uncached words is ordinary, not
an edge case.
A mutex would not have fixed it - two launched coroutines reach the lock in
whatever order the pool starts them. PerPlayerWorkQueue instead gives each
player a channel with a single consumer, so submission order is decided on
the calling thread and preserved, while different players stay independent.
The queue is dropped when a player quits; work already queued still runs.
The ordering test fails against the previous launch-per-message code.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
/tell and /reply ran romaji conversion inline. Brigadier executors run on
the main thread, and convertWithRomaji wrapped a Google IME call in
runBlocking with a one-second timeout, so a single direct message could
hold the tick thread for up to a second - twenty ticks - whenever the
words were not already cached. Conversion defaults to on for players once
the feature is enabled, so this was the ordinary path, not an edge case.
The conversion chain is suspending now, and the two commands dispatch
delivery to the plugin scope instead of running it inline. Nothing in that
chain touches world state: it sends chat components and plays client-side
sounds, both of which Paper already accepts off-thread, and which this
plugin already does from AsyncChatEvent.
AsyncChatEvent keeps a blocking bridge, renamed convertWithRomajiBlocking
so the choice is visible at the call site. That handler has to decide
whether to cancel the event and what body to set before it returns, and it
is already off the tick thread.
The commands take their scope as a constructor parameter so tests can
choose one. The new test uses StandardTestDispatcher to pin the property
that matters: execute() returns before delivery has run at all.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Every channel mutation built a full snapshot - copying the channel map,
every member list, and stringifying a UUID per active player - and then
queued a task that pretty-printed and rewrote all of channels.json. There
was no debounce, unlike the settings storage, so a fifty-player join wave
meant fifty snapshots and fifty whole-file writes. Joins and quits both go
through it via setPlayerChannel, and so does the self-healing branch of
getPlayerChannelContext, which sits on the channel-chat message path.
ChannelStorage now debounces like the other two storages, and takes the
snapshot as a supplier so it is built once when the write runs rather than
once per queued change. saveToStorage and saveToDisk had the same six-line
snapshot construction; that is now one private function.
The trade-off is the same one the settings storage already makes: a crash
within the debounce window loses the last few seconds of channel state.
Shutdown still writes synchronously.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
isMember went through getChannelMembers, which copies a channel's whole
member list defensively - to then run any{} over it and throw the copy
away. getPlayerChannels did that once per channel, so asking "which
channels is this player in" allocated an ArrayList per channel and scanned
every one. It runs on join, and on every /lc channel join when a
membership limit is configured.
ChannelManager now answers both directly against the live lists: isMember
scans in place, and channelIdsOf walks the membership map once. No copies,
and getPlayerChannels no longer enumerates all channels separately to
cross-reference them.
Not done: the reviewed suggestion was a playerId -> channelIds reverse
index for O(1) lookups. That means a second record of who is in what,
updated by hand at five mutation sites, where drift shows up as a wrong
membership answer rather than a crash. The allocation was the real cost
here, and removing it does not introduce a fact stored twice.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Spy notification ran per recipient what it could run once: both handlers
looked up general.spyMessage and rebuilt the hover component inside the
forEach, and the direct message path also allocated a Set inside the
filter predicate, once per spy per message. It formatted the spy copy of
the message before discovering there were no spies to send it to - and
spies are rare, so that was the normal case.
SpyPermissionManager.notifySpies now owns the whole shape. It takes the
body as a lambda so nothing is built for an empty audience, attaches the
hover once, and reads the roster directly rather than through
getDirectMessageSpyPlayers()'s defensive copy. It also puts "who must not
see this" in one place; the two handlers had drifted to expressing it by
name in one and by UUID in the other.
ConversionCache tracked no dirtiness, so the periodic task re-serialized
and rewrote the entire cache file on its fixed interval whether or not
anyone had chatted, while every single put scheduled another full rewrite
five seconds out. It now records that it changed and the periodic task is
the only writer, returning immediately when there is nothing to write. A
failed write marks the cache dirty again so the next tick retries.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
PlayerQuitEvent called saveToDisk(), which KAML-serializes every player
ever recorded and writes the file inline. That runs on the tick thread,
costs time proportional to the total stored players rather than the online
ones, and so grows for the life of the server. A logout wave stacks the
writes into a visible stall. The comment above the call already said
"async save"; the storage KDoc already said the synchronous path was for
shutdown.
Quitting changes no setting, so there is nothing new to persist - the
value of saving here is flushing what an earlier toggle left pending.
queueSave() does exactly that through the existing debounce, and
saveToDisk() is now documented as the shutdown path and called from
nowhere else.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
RomanjiConverter awaited the Google IME call for one word before starting
the next, though the words are independent. Callers convert under a single
one-second timeout covering the whole message, so an N-word message needed
N round trips inside a budget sized for roughly one: past the first word or
two the remaining calls were cancelled, and because cache.put is only
reached after a call returns, the discarded work was not even remembered.
The next identical message repeated it.
The words now run under one coroutineScope, so the message costs one round
trip rather than N.
The new test pins the concurrency itself rather than the timing: the fake
API client records how many calls are in flight at once, which fails
against the sequential version.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Every channel message wrote a fully interpolated INFO line, duplicating
what the dedicated async ChannelMessageLogger already persists. Every
cross-server message produced one INFO on the sending Paper server, two on
Velocity, and one on each receiving Paper server - so a single global chat
message cost up to four synchronous console and latest.log writes across
the network, on the message path, with JUL and Logback appenders being
synchronous.
These are now debug, and phrased so the string is not built unless debug
is on: the JUL sites take a supplier, the slf4j sites take a format plus
arguments.
Handshake, registration and delivery-failure lines stay at info - they
fire once per server or once per failed message, and they explain
something an operator needs to see.
Two relay tests asserted on the text of a log line. One of them was
verifying "0 servers" where the same test already verifies zero sends;
the other was really checking that the payload reaches the target, so it
now decodes the relayed bytes and compares them to the original message.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Both HTTP consumers - Japanese conversion and the update check - default
to disabled, yet onEnable built an HttpClient(CIO) unconditionally,
starting a selector and dispatcher pool that a stock install never used
and nothing ever closed. It is now created on first use and closed on
disable, which also fixes the leak across /reload.
LanguageManager parsed and flattened every bundled language file, though
getMessage only ever reads the selected one and the English fallback. It
now loads exactly those two.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
ChannelInviteCommand used to pre-check isPlayerBanned itself. That check
moved into ChannelMembershipManager.inviteToChannel in 288d1e4, but the
stub stayed behind, implying the command still consults the channel
manager for ban state when it no longer does.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
DebouncedSaver keeps only the first callback of a burst, and
queueAsyncSave closed over the snapshot taken when it was called. So if
one player toggled a setting and a second toggled two seconds later, the
write that fired at five seconds persisted the first snapshot and dropped
the second player's change - it survived in memory until some later toggle
happened to trigger another write, and was lost on a crash.
Passing a supplier instead means the snapshot is taken when the write
runs, which is what "batched into a single save" was always meant to mean.
ConversionCache already had this shape by passing ::saveToDisk.
The staleness predates the refactor, but bc8010b claimed to have closed
this window; it only moved where the snapshot was built, not when.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
- MessageFormatter built the same prefix component in three functions.
- LanguageManager copied kaml's YamlNode into a private YamlValue tree
before flattening it, so the map case was written twice and the
list-of-maps case rendered a Kotlin data class toString into a player
facing string. It now folds YamlNode directly.
- StatusCommand inlined `if (enabled) "toggle.on" else "toggle.off"`,
which is the body of LanguageManager.getToggleText.
- The three chat formats each spelled out their own chain of
String.replace, with the valid placeholder names documented only in a
config.yml comment.
- ChannelContext carried a channelId that both construction sites filled
with channel.id; it is now derived, so the two cannot disagree.
- ChannelInfo and ChannelStatus each declared MAX_MEMBERS_DISPLAY = 10 and
built the same truncated member line, differing only in indent. A
divergence between the two constants would have been invisible.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
ServiceInitializer held ten nullable fields alongside the ServiceContainer
it returns. Six of them were written and never read; the remaining four
made shutdown() ambiguous, reading conversionCache and channelMessageLogger
from its own fields but everything else from the container it was handed.
LunaticChat then mirrored seven more into public vars, one of which
(channelMessageHandler) nothing read at all.
Now the container is the single place a service lives: the initializer
builds and returns, shutdown and the periodic task read from what they are
given, and the plugin's public properties delegate rather than copy. A new
service is one field instead of three, and no copy can go stale.
PlayerSettingsManager had the same shape at a smaller scale: three UUID
maps plus a PlayerSettingsData kept in sync by hand, where the data object
was a pure derivation rebuilt - three full map copies - on every toggle. It
now keeps one map and derives the snapshot at save time, which also closes
the window where queueAsyncSave captured state that changes before the
debounce fires.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
YamlPlayerSettingsStorage and ConversionCache each carried a line-for-line
identical AtomicBoolean-plus-runDelayed debounce, five-second constant
included, so changing the save cadence meant changing it twice and
noticing that it was written twice.
Extracting it also removed the only reason those two classes held a
JavaPlugin: they took the whole plugin to reach the scheduler. They now
take the collaborator they actually use, which is both narrower and
testable without a running server.
ChannelStorage is deliberately left alone - it saves through runNow with
no debounce at all, and giving it one would change when writes happen.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
The rule "owners and moderators may kick and ban" was written out six
times, as `senderRole == null || senderRole == ChannelRole.MEMBER`, while
ChannelMembershipManager.hasRole - which encodes the OWNER > MODERATOR >
MEMBER hierarchy in one place and has tests - had no production caller at
all. Letting moderators ban meant editing six files and hoping none was
missed; a miss is a silent privilege change.
The bypass and self-invite rules had drifted further still: the engine
defines ChannelPlayerBypassBanException, ChannelPlayerBypassKickException
and ChannelCannotInviteSelfException, but nothing threw them. The checks
lived inline in the commands, and ChannelBanCommand carried a catch arm
for an exception that could never arrive. Those three rules now live on
ChannelMembershipManager and throw what the engine already declared, so
every caller is held to them rather than only the command path.
ChannelInviteCommand also pre-checked for a banned target, which
joinChannel checks again a moment later and reports through the same
message; the pre-check is gone.
What stays in the commands is what belongs there: parsing an argument and
choosing which message to show. ChannelSubCommand names the steps they
share and derives message keys from the subcommand's own literal.
The tests followed the rules: bypass and self-invite are now asserted
against ChannelMembershipManager, with a bypass predicate injected so the
manager stays testable without a running server.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
The channel Paper and Velocity talk over was declared in seven places, in
two spellings ("lunaticchat:main" and the namespace/name pair), one of
them an inline literal in CrossServerChatManager that bypassed even its
own file's constant. Renaming it meant finding all seven; missing one
leaves both sides compiling and starting, just not talking. It now lives
next to the codec that defines the wire format.
The echo-suppression cache was likewise written twice, and the copies had
already drifted in style - one hand-rolled the expiry sweep, the other used
filter/map - while staying semantically identical. Any future change to
eviction would have had to land in both, and CrossServerChatManager's copy
carried a comment claiming ConcurrentHashMap iterators cannot remove(),
which they can.
MessageDeduplicationCache documents the one property that surprised the
tests written against it: eviction orders by millisecond timestamp, so a
burst inside a single millisecond evicts arbitrarily among its members.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
The three setting handlers were the same 53-line class three times over,
differing in one copy() field, one read, and two message keys. Adding
persistence, auditing or a permission check to settings meant writing it
three times, and a fourth setting meant a fourth copy.
A setting is its key, the messages that report it, and how it is read from
and written to PlayerChatSettings - so SettingKey now carries all of that,
and SettingHandler is the single mechanism that applies it. The registry
keeps its role as the seam where features decide which settings exist,
which is why registration is still conditional in LunaticChat.
SettingHandlerTest now asserts over every SettingKey rather than repeating
five near-identical tests per handler, so a new setting is covered the
moment it is declared. It also pins down the property that made the old
duplication dangerous: writing one setting must not disturb the others.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Eighty-nine call sites spelled out the same four-line nest to say "fail
with localized message X":
CommandResult.Failure(
MessageFormatter.formatError(
languageManager.getMessage("channel.ban.noPermission"),
),
)
The pairing of formatError with getMessage was a convention held together
only by copy-paste, so changing how command errors are presented meant
touching all eighty-nine. fail() and ok() on LunaticCommandBase own that
pairing now, and the call sites read as the intent: fail("channel.ban.noPermission").
Three when(error) blocks turned out to map every arm to the same message,
and ChannelCreateCommand ran two parallel whens over one error to pick a
key and its parameters separately. Both were only visible once the noise
around them was gone.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Subcommands were LunaticCommands that could not be commands. Each of the
17 carried the same three-method preamble plus an override of
buildCommand() that only threw, because the contract it inherited -
@Command-driven name, aliases and description - has no meaning for a node
attached under a parent literal. Accessing name on any of them would have
thrown too.
LunaticSubCommand now models that node directly, so the preamble and the
throwing override are gone. Two knock-on wins:
- The permission gate is a declared property instead of
applyMethodPermission("build", ...), which looked the method up by name
through reflection and returned the builder ungated whenever the lookup
or the annotation was missing. A rename or a dropped annotation silently
opened the command up; now omitting the gate fails to compile.
- Subcommands expose the literal they register under, so ChannelCommand
drives both its Brigadier tree and its help output from one list. The
two 14-entry lists it maintained by hand could disagree without any
compiler or test noticing.
ApplyMethodPermissionTest covered the reflection helper that no longer
exists; LunaticSubCommandTest covers the gate that replaced it.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
These were all scaffolding that drifted out of use, and each one costs
a reader time before they discover it does nothing:
- UUIDASStringSerializer duplicated UUIDSerializer byte for byte; the
differing descriptor name never reaches the JSON/YAML wire format, so
the choice between them was a coin flip for contributors.
- Velocity's BuildInfo was never referenced (the plugin reads its version
from PluginContainer) and read a "commit" property the build never
wrote, so it would have reported "unknown" had anyone called it.
- KanaConverter.TrieNode.Leaf is never constructed: buildTrie starts from
a Branch and insert only ever returns Branch. Six branches guarded
against a state the type system allowed but the code could not produce.
With those gone, isValidRomaji and toHiragana were visibly the same
trie walk, so they now share one longestMatch.
- @Deprecated command handling had no annotated command to act on.
- The settings backup restore looked for *.backup.* files that nothing in
the repository writes, so it always fell through to empty settings.
Also drops CommandContext.replyWithEvent/replyPlain, PluginCoroutineScope's
unused plugin parameter, GitHubRelease fields no caller reads, and four
language keys with no lookup site.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Paper 26.2 bundles Adventure 5.2.0, a major bump from the 4.26.1 shipped by 26.1.
Adventure 5 makes ClickEvent generic, so the raw type in
CommandContext.replyWithEvent no longer compiled. Auditing the rest of the
Adventure 5 removals found no other affected usage.
api-version is raised to 26.2, dropping 26.1 support: the two Adventure majors
are not binary compatible, so claiming 26.1 compatibility would be a lie.
Also fixes two problems found while verifying on a real server: the Velocity
healthcheck invoked mc-health, which does not exist in the mc-proxy image, so the
container had never once passed its check; and the debug environments pinned no
build at all, which meant the proxy came up as 4.1.0-SNAPSHOT while the plugin is
compiled against velocity-api 3.5.1. Server versions are now derived by x from
the Gradle coordinates so they cannot drift from what the plugin targets.
|
|
Add unit tests for CrossServerDirectMessageManager (send/receive/error/dedup),
DirectMessageHandler cross-server display and ReplyTarget resolution, and
ServiceContainer wiring to raise patch coverage.
Extend codecov ignore list to the plugin bootstrap classes (LunaticChat,
ServiceInitializer), consistent with the existing exclusions for runtime-coupled
classes (VelocityConnectionManager, command/impl) that are covered by
end-to-end server tests.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Allow /tell and /reply to reach players on other Paper servers behind a
Velocity proxy using the "<player>@<server>" target syntax.
Engine (protocol bumped 1.0.0 -> 1.0.1, optional sub-channels):
- Add DirectMessageRelay, DirectMessageError, PresenceSnapshot/PresenceEntry
and PresenceRequest messages plus codec branches.
Velocity:
- CrossServerDirectMessageRelay routes a DM to the target server (or returns
a delivery error to the source).
- PresenceTracker broadcasts proxy-wide presence snapshots on join/quit/switch
and on request.
Paper:
- RemotePlayerRegistry caches proxy presence for completion and remote target
resolution.
- CrossServerDirectMessageManager handles send/receive/error and dedup.
- DirectMessageHandler reply state generalized to ReplyTarget (Local/Remote)
so /reply works across servers.
- TellCommand parses "name@server", completes local names and remote
name@server targets, and uses exact local name matching.
- New crossServerDirectMessage config flag and i18n keys (en/ja).
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
applyMethodPermission tests
Exclude command implementation classes (platform-paper/command/impl/**) from
codecov coverage metrics since they are tightly coupled to the Paper/Brigadier
runtime and exercised via end-to-end server tests. This consolidates the
previous per-file exclusion for VelocityStatusCommand.
Also add unit tests for the new applyMethodPermission method in LunaticCommand
to ensure the command/core package maintains coverage.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Cover the new withAliases helper in LunaticCommand to satisfy
codecov/patch coverage threshold (80%).
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Closes #173
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
|
|
|
|
|
