feat: make /pkclear command configurable and available to regular players#822
feat: make /pkclear command configurable and available to regular players#822Rhefew wants to merge 1 commit into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request expands the functionality of the /pkclear chat command, enabling regular players to clear their own PK status for a configurable amount of Zen. It provides server administrators with granular control over the feature via a new configuration schema, while ensuring that Game Masters retain their ability to manage PK statuses for other players. The changes include robust validation to prevent unauthorized usage and ensure the economy remains balanced. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the /pkclear chat command to allow regular players to clear their PK status for a configurable Zen cost, and adds corresponding unit tests. The review feedback highlights several important issues: a potential integer overflow in the Zen cost calculation that could allow free PK clears, thread-safety and null-safety concerns in the command handler, a logical flaw where regular players specifying a target character silently clear their own status, and several hardcoded English messages that should be localized.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| var cost = targetCharacter.PlayerKillCount * configuration.ZenCostPerKill; | ||
| if (cost > 0 && !player.TryRemoveMoney(cost)) |
There was a problem hiding this comment.
An integer overflow can occur when calculating the Zen cost if a player has a high PK count and ZenCostPerKill is set to a high value (e.g., the default 10,000,000). For example, if a player has 215 kills, 215 * 10,000,000 equals 2,150,000,000, which exceeds int.MaxValue (2,147,483,647) and overflows to a negative number (-2,144,967,296). Since the negative cost is not greater than 0, the check cost > 0 evaluates to false, allowing the player to clear their PK status for free.
To prevent this, calculate the cost using long arithmetic and ensure it does not exceed int.MaxValue before calling TryRemoveMoney.
var cost = (long)targetCharacter.PlayerKillCount * configuration.ZenCostPerKill;
if (cost > 0)
{
if (cost > int.MaxValue || !player.TryRemoveMoney((int)cost))
{
await player.ShowLocalizedBlueMessageAsync(nameof(PlayerMessage.NotEnoughMoney)).ConfigureAwait(false);
return;
}
}| var configuration = this.Configuration ??= (PKClearConfiguration)this.CreateDefaultConfig(); | ||
| var isGameMaster = player.SelectedCharacter!.CharacterStatus >= CharacterStatus.GameMaster; |
There was a problem hiding this comment.
There are two issues here:
- Thread Safety: The plugin instance is shared and can be executed concurrently by multiple players. Mutating the instance property
this.Configurationusing??=during command execution is not thread-safe. Instead, use a local fallback variable. - Null Safety:
player.SelectedCharactercan be null (as seen in other parts of the codebase where it is guarded). Accessing it with!without a guard can lead to aNullReferenceException.
We can resolve both by guarding player.SelectedCharacter at the start of the method and using a local fallback for the configuration.
var selectedCharacter = player.SelectedCharacter;
if (selectedCharacter is null)
{
return;
}
var configuration = this.Configuration ?? (PKClearConfiguration)this.CreateDefaultConfig();
var isGameMaster = selectedCharacter.CharacterStatus >= CharacterStatus.GameMaster;| else | ||
| { | ||
| if (!configuration.AllowRegularPlayers) | ||
| { | ||
| await player.ShowBlueMessageAsync("This command is only available to Game Masters.").ConfigureAwait(false); | ||
| return; | ||
| } | ||
|
|
||
| targetPlayer = player; | ||
| } |
There was a problem hiding this comment.
If a regular player attempts to clear another player's PK status by typing /pkclear <other_character>, the command currently ignores the argument and silently clears the calling player's own PK status (deducting their Zen) instead of failing or showing an error. This can lead to accidental Zen loss and confusion.
Consider adding a check to ensure regular players cannot specify a different character name.
else
{
if (!configuration.AllowRegularPlayers)
{
await player.ShowBlueMessageAsync("This command is only available to Game Masters.").ConfigureAwait(false);
return;
}
if (!string.IsNullOrEmpty(arguments.CharacterName) && !arguments.CharacterName.Equals(player.Name, StringComparison.OrdinalIgnoreCase))
{
await player.ShowBlueMessageAsync("You can only clear your own PK status.").ConfigureAwait(false);
return;
}
targetPlayer = player;
}| { | ||
| if (!configuration.AllowRegularPlayers) | ||
| { | ||
| await player.ShowBlueMessageAsync("This command is only available to Game Masters.").ConfigureAwait(false); |
There was a problem hiding this comment.
| await player.ShowBlueMessageAsync(isGameMaster | ||
| ? $"{targetCharacter.Name} is not a Player Killer." | ||
| : "You are not a Player Killer.").ConfigureAwait(false); |
There was a problem hiding this comment.
The messages "{targetCharacter.Name} is not a Player Killer." and "You are not a Player Killer." are hardcoded in English. To support internationalization (i18n) and adhere to the project's standards, these should be localized using ShowLocalizedBlueMessageAsync with appropriate keys from PlayerMessage.
This PR enhances the
/pkclearchat command plugin to allow regular players (not just Game Masters) to clear their own PK status in exchange for Zen. It also introduces server configuration controls so administrators can adjust the Zen cost and toggle the feature's availability.Key Changes
/pkcleartoCharacterStatus.Normal.ISupportCustomConfigurationonPkClearChatCommandPlugInto expose a configuration schema:ZenCostPerKill(default:10,000,000Zen): Cost per PK count.AllowRegularPlayers(default:true): Toggles whether non-GMs can clear their own PK status./pkclear <character_name>.AllowRegularPlayersis enabled, the command calculatesPK Count * ZenCostPerKilland deducts the Zen. If they do not have enough Zen, the command fails with a localized"Not enough money"message.PkClearChatCommandArgsto make theCharacterNameargument optional. Regular players can simply type/pkclearto clear their own status, whereas GMs are still prompted to specify a name if they attempt to target another player.PKClearChatCommandPlugInTestcovering access rules, GM target clearing, regular player self-clearing, insufficient Zen failures, and targeting restrictions.Verification Results
Tested locally and verified: