Skip to content

fix: make /guild code redeem consistent with /guild accept - #849

Closed
darbyjack wants to merge 4 commits into
masterfrom
fix/invite-code-membership-consistency
Closed

darbyjack wants to merge 4 commits into
masterfrom
fix/invite-code-membership-consistency

Conversation

@darbyjack

@darbyjack darbyjack commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

What was broken

/guild code redeem and /guild accept are the two ways into a guild, and they were not doing the same work. Redeeming a code added the player to the guild's member list and stopped there.

  • The join cooldown was never consulted. GuildHandler.handleInvite did not look at it, so a player who had just left a guild, or been kicked out of one, could redeem a code straight away. /guild accept and /guild create both refuse while the cooldown runs, and timers.cooldowns.join is documented as "how long should a user have to wait before joining a new guild after leaving one".
  • No join event was fired. /guild accept fires the cancellable GuildJoinEvent; a code-based join fired nothing, so a plugin listening for joins never heard about it and could not stop one.
  • No permissions were applied. Accept hands the new member the nodes their guild tier and their role grant. A code member got neither, so they held no guild permissions until their next login, when PlayerListener re-applied them.
  • The member was registered differently. Accept used Guild.addMember, which stamps the join date and clears a stale invite. A code used Guild.addMemberByCode, which did neither, so the members list showed a join date of zero for that player.

The fix

CommandCode.redeem checks the Join cooldown and refuses with the same message accept uses (accept.cooldown).

GuildHandler.handleInvite fires GuildJoinEvent before anything is consumed. A cancelled event stops the redeem: the use is still there, the player is not recorded as a redeemer and the guild does not gain a member. After the event, the member is registered through Guild.addMember, the member cache is updated, and the guild tier and role permissions are applied the way accept applies them.

Guild.addMember and Guild.addMemberByCode share one registration now. It finds a member by uuid, because GuildMember does not override equals, so the old members.contains(member) check never matched and a second entry for the same player was possible. It clears a stale invite and stamps the join date.

The use count, the redeemer list and the creator of a code are untouched. One redeem still spends one use and records one redeemer.

Tests

New unit tests:

  • GuildHandlerRedeemTest (6 tests): the join event is fired, a cancelled event leaves the use count, the redeemer list and the roster alone, the member is registered with a join date, the tier and role permissions are granted, a stale invite is cleared.
  • GuildMemberRegistrationTest (7 tests): addMember and addMemberByCode register a member the same way, and a player who is already in the guild is not added a second time.
  • GuildCodeTest (3 tests): what one use of a code means, including that the same player can spend a code twice and that it costs two uses.

New E2E specs in src/test/e2e/tests/codes.spec.ts:

  • a player who joins by code gets the permissions their guild grants them
  • a code cannot be redeemed while the join cooldown is running

The existing redeem spec now also checks the roster, so a code member shows up with the same role an invited member gets.

Results

Against the unmodified plugin the new tests fail the way the defect predicts. The E2E run of codes.spec.ts was 15 passed, 2 failed: the joiner was told "you do not have permission to perform this command" on every probe, and the second redeem was not refused with the cooldown message. GuildHandlerRedeemTest failed 5 of its 6 tests and GuildMemberRegistrationTest failed 4 of its 7.

With the fix:

  • ./gradlew test testJava11 spotlessCheck -> BUILD SUCCESSFUL, 84 tests, 0 failures. The Java 11 run included the 16 new tests.
  • ./gradlew check -> BUILD SUCCESSFUL, including the 1.8.8 Material probe.
  • ./gradlew plugwrightTest -> 146 passed, 0 failed, 0 skipped (7m 6s). Re-run on the current head after the CI failure described below: 146 passed, 0 failed, 0 skipped (7m 52s), with the homes test green.
  • Merged with fix(codes): refuse an invite code created with no uses to offer #844 in a scratch worktree: 84 unit tests, 0 failures, and the merged codes.spec.ts compiles with all 18 tests.
  • After the coverage document was set to the canonical totals: ./gradlew test testJava11 spotlessCheck -> 84 tests, 0 failures on both JVMs, and ./gradlew check -> BUILD SUCCESSFUL with the 1.8.8 Material probe.

The E2E check on this PR

The Plugwright check on this branch failed on "a second teleport home cannot happen until the cooldown expires" in homes.spec.ts, with the bot reported as "connection had already ended (socketClosed); /guild home would never have been sent". The bot's socket dropped, so the command never left the client. The assertion is not about this change, and this branch does not touch homes.spec.ts.

That test is flaky on the runner rather than on any one branch:

  • master, run 37211243837 (2026-10-04, a manual E2E run on master at 6a3ba8f): the same test failed with the same "Connection ended: socketClosed".
  • fix/blacklist-matching, run 39727120928: the same test failed the same way, and that branch went green on its next push, run 39729457184.
  • fix/invite-code-usage-validation, run 39728538651: the same test failed the same way, and that branch went green on its next push, run 39729585923.
  • this branch, run 39786062028: the same test failed the same way on the current head, on the first attempt and on the re-run, 145 of 146 passed each time.

Runs:
https://github.com/guilds-plugin/Guilds/actions/runs/37211243837
https://github.com/guilds-plugin/Guilds/actions/runs/39727120928
https://github.com/guilds-plugin/Guilds/actions/runs/39729457184
https://github.com/guilds-plugin/Guilds/actions/runs/39728538651
https://github.com/guilds-plugin/Guilds/actions/runs/39729585923
https://github.com/guilds-plugin/Guilds/actions/runs/39730173487
https://github.com/guilds-plugin/Guilds/actions/runs/39786062028

I left homes.spec.ts alone and did not weaken the assertion. The suite is green locally on this head. On GitHub the same homes test failed on the current head with the same socket close, on the first attempt and on the re-run, so the check is red for that reason and not for anything in this change.

The coverage doc and the other PRs

The coverage document counts every test, so its totals are contended between the open PRs. This branch carries the canonical values: 162 tests across 14 spec files on line 6, and 18 in the section H heading for codes.spec.ts. The arithmetic is master's 141 across 12 spec files, plus 2 from #848, 4 from #844, 2 from this PR, 4 from #850, 2 from #851 and 7 from #852, and #843 only renames two tests.

The headings in this copy add up to 162 on their own. Sections A (21), B (21), C (8 and 8) and H (18) carry their tables, as do the sections master owns. The two sections another PR owns are placeholders here, one line each with the count they will hold: ## L. Startup and the guild data — tests/startup-loading.spec.ts (4) and ## Runtime smoke coverage — tests/smoke.spec.ts (7). Their tables stay in the branches that own them, so the merged document picks them up without a second copy.

The section H table lists every invite-code test once, the two from this PR and the two from #844. The tests that no PR documents are listed as well: #848's two name-lookup tests and #851's two creation-confirmation tests, so the counts in those headings read 21 for the lifecycle spec, 21 for the membership spec and 8 for admin.spec.ts. Master's membership spec holds 20 test() calls while its heading said 19, which is where the 21 comes from.

I checked every open PR with git merge-tree. #843, #845, #846, #847, #848 and #851 merge into this branch without a conflict. #844, #850 and #852 conflict in this file, in two places. The table layouts: both sides list the same tests and group them into different rows, so the maintainer picks a layout, and the header total is one of those lines, where 162 across 14 is the value to take. The placeholders: my one-line placeholders sit exactly where #850 and #852 put their full sections, so those two regions resolve by keeping the section and dropping the bare heading. #844's copy of the header says 147 and #850's says 160, so those two need the same one-line change.

Merging #844 does not undo this fix

I merged #844's head into this branch in a scratch worktree and ran the suite there. #844 adds a guard to Guild.addCode, the method that creates a code, and it touches neither Guild.addMember, Guild.addMemberByCode nor GuildHandler.handleInvite. The uuid-based member registration and the redeem path's tier and role permissions are both still in the merged tree, my 16 unit tests pass next to #844's 6, and the merged codes.spec.ts compiles with all 18 tests, each name once.

#851 and #847 do not touch the redeem path either. #851 works inside CommandCreate and re-runs the creation checks when the pending action is confirmed. #847 changes the bank commands and the creation cost withdrawal, so the money is gone before the guild is created. Neither reads the join cooldown, and neither changes CommandCode, Guild.addMember or GuildHandler.handleInvite, so the join cooldown check this PR adds to the redeem path stands on its own.

What happens when the guild cannot be saved

The redeem path writes nothing to disk. It changes the guild in memory, the member cache, the permissions and the claim, and the record reaches the disk with the next autosave (storage.save-interval, one minute by default) or with the save on shutdown. /guild accept, a leave and a kick all have that shape, and this PR does not change it.

When a write fails, the JSON provider writes through a temporary file in the same folder and moves it over the record only after the write succeeded, so a failed write leaves the previous record exactly as it was. The failure is logged per guild and in a summary, nothing is corrupted, and nothing is rolled back.

What gets lost is the change itself. If the disk is full or the folder is read-only when the server stops, the membership is not on disk. After the restart the player is not in the guild, the guild's code still has its use because the use count lives in the same record, and the nodes the guild granted the player are still in the permissions plugin's own storage, since nothing revoked them. The player is told nothing at any point in that sequence. A crash between the temporary file and the move leaves a stray file in the data folder, which the current loader logs as a guild it cannot read and skips; #846 stops the loader from reading anything that is not a .json file.

I have left this alone. Making a membership durable at the moment of the join, and telling the player when it could not be stored, is a change to the persistence layer and belongs with the storage work.

Compatibility

No config key, default, stored format or public signature changed. GuildHandler.handleInvite, Guild.addMember and Guild.addMemberByCode keep their signatures, and addMemberByCode now behaves like addMember. The Java 11 bytecode target is unchanged, no Material constant was touched, and the claim and the announcement of a redeem are as they were. The permissions go through the same Vault calls accept makes, so a server with a permissions plugin sees the nodes a guild tier and a role grant, and one without it is unaffected (Guilds refuses to start without Vault).

Limitations

  • A player who leaves the guild they joined with a code can spend that code again once the join cooldown has run out. The second redeem costs a second use and names the player twice in the redeemers list. That is the existing usage semantics, so it is left as it is; the cooldown now delays it instead of being skipped.
  • The join event is fired from GuildHandler.handleInvite, so a third-party plugin calling that method gets the event as well. The other way a player can be put into a guild, /guild admin addplayer, still fires no join event.
  • The unit tests assert the event and the permission calls through mocks. The E2E specs cover what a player can see.

darbyjack and others added 2 commits October 22, 2026 05:48
Redeeming a code added the player to the guild's member list and stopped
there. It skipped the join cooldown, fired no join event, granted no
permissions and registered the member differently from an invite, so a
player who joined by code was a member in name only.

/guild code redeem now refuses while the join cooldown runs, and
GuildHandler.handleInvite fires the cancellable GuildJoinEvent before
anything is consumed, registers the member through Guild.addMember and
applies the guild tier and role permissions.

Guild.addMember and Guild.addMemberByCode share one registration, which
identifies a member by uuid and stamps the join date.

The use count, the redeemer list and the creator of a code are
unchanged.
darbyjack pushed a commit that referenced this pull request Oct 22, 2026
…land

#844 and #849 both add tests to tests/codes.spec.ts, so the counts in the
coverage inventory have to be the ones the merged file will have: 18 in the
invite codes section and 147 in the suite total. The table carries the rows
from both pull requests, with no row twice.

Nothing else in the inventory changes.
darbyjack pushed a commit that referenced this pull request Oct 22, 2026
The coverage inventory is contended between the open pull requests, and the
header and the per-section counts have to be the ones the merged file will
have whatever order they land in: 160 tests across 14 spec files, 18 in the
invite codes section, 21 in the guild lifecycle and membership sections and 8
in the admin commands section.

The rows for the tests that #848 and #851 add are in the tables here, since
neither of those pull requests touches this file, and the rows for #849's
tests were already here. Every test in the merged suite is listed once.

The membership section heading said 19 while the spec file holds 20 tests,
which is where the 21 comes from.
@darbyjack darbyjack closed this Oct 23, 2026
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