Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the command configuration system by introducing dynamic default generation, robust stale command cleanup, and null-safe handling for aliases and permissions. While the integration of ConfigurationManager, Scheduler, and Logger via dependency injection is a solid architectural choice, the implementation introduces significant race conditions. The most critical issue is a hardcoded 1-second timer for stale command cleanup, which can lead to the deletion and reset of custom security configurations (permissions, enabled status) on servers where command registration takes longer than one second. Additionally, non-atomic updates to the shared command configuration map from multiple threads can lead to data inconsistency and loss.
...re-core/src/main/java/com/eternalcode/core/litecommand/configurator/CommandConfigurator.java
Show resolved
Hide resolved
| } | ||
|
|
||
| private void cleanupStaleCommands() { | ||
| this.commandConfiguration.commands = mutableMap(this.commandConfiguration.commands); |
There was a problem hiding this comment.
The CommandConfigurator class performs non-atomic updates to the commandConfiguration.commands map from both the main thread (during command registration) and a scheduled task (during stale command cleanup). The use of ConcurrentHashMap for other fields and the volatile keyword suggests that concurrent access is expected. However, the commands map is replaced with a new copy in both synchronizeDefaults and cleanupStaleCommands, leading to a race condition where one thread's updates can be overwritten by another. This can result in configuration data loss or inconsistency, potentially reverting security-sensitive settings like command permissions or enabled status.
To remediate this, use a thread-safe map (like ConcurrentHashMap) for commandConfiguration.commands and perform atomic updates, or synchronize access to the map.
| if (this.commandConfiguration.commands.remove("") != null) { | ||
| this.logger.warning("[CommandConfigurator] Removed invalid empty command key ('') from commands.yml"); | ||
| changed = true; | ||
| } |
There was a problem hiding this comment.
The logic for removing an invalid empty command key is duplicated here and in cleanupStaleCommands. Extracting this into a private helper method would improve code reusability and reduce redundancy.
changed |= this.removeEmptyCommandKey();![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b54dbedcf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| List<String> staleKeys = this.commandConfiguration.commands.entrySet().stream() | ||
| .filter(entry -> entry.getValue() == null || !runtimeNames.contains(entry.getKey())) | ||
| .map(Map.Entry::getKey) |
There was a problem hiding this comment.
cleanupStaleCommands deletes every config key that is not in discoveredRuntimeCommands, but command discovery is already filtered by compatibility (EternalCore scans only CompatibilityService.isCompatible types), so version-gated commands can be missing at runtime by design (for example ElderGuardianCommand and EndScreenCommand have @Compatibility(from=...)). On older server versions this logic will remove those valid entries from commands.yml, permanently discarding user overrides that should still exist when the server is upgraded later.
Useful? React with 👍 / 👎.
1 participant
No description provided.