Update datapack initialization to use scheduled function calls#18
Closed
SpyC0der77 wants to merge 1 commit into
Closed
Update datapack initialization to use scheduled function calls#18SpyC0der77 wants to merge 1 commit into
SpyC0der77 wants to merge 1 commit into
Conversation
- Modified the `datapack.init` template in multiple version files to replace direct function calls with scheduled function calls, enhancing the initialization process for better timing control. - This change applies to versions 1.21.2 through 1.21.11 and 26.1, ensuring consistency across all supported versions.
📝 WalkthroughWalkthroughThis PR adds support for global (server-wide) packs alongside world-specific packs, refactors configuration management into a persistent system, and introduces a comprehensive command infrastructure across all supported platforms (Fabric, Forge, NeoForge). ChangesGlobal and World Pack Scoping with Configuration Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
13 issues found across 46 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="minecraft_script/versions/1.21.4.json">
<violation number="1" location="minecraft_script/versions/1.21.4.json:6">
P2: Delaying `user_functions/init` by 20 ticks allows `main` to run before initialization, changing load/tick ordering and risking uninitialized runtime state.</violation>
</file>
<file name="minecraft_script/versions/1.21.11.json">
<violation number="1" location="minecraft_script/versions/1.21.11.json:6">
P2: Scheduling `user_functions/init` 20 ticks later allows `main` to run before initialization completes, introducing an init/main ordering race.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsModConfig.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsModConfig.java:120">
P2: Record fields are backed by mutable `Set` instances, allowing external mutation of config state after construction.</violation>
</file>
<file name="minecraft_script/versions/26.1.json">
<violation number="1" location="minecraft_script/versions/26.1.json:6">
P1: Delaying `user_functions/init` by 20 ticks allows `main` to run before initialization, causing startup-order regressions.</violation>
</file>
<file name="minecraft_script/versions/1.21.9-10.json">
<violation number="1" location="minecraft_script/versions/1.21.9-10.json:6">
P2: Delaying `user_functions/init` by 20 ticks can run `main` before initialization is complete, breaking prior load-before-tick ordering.</violation>
</file>
<file name="minecraft_script/versions/1.21.6.json">
<violation number="1" location="minecraft_script/versions/1.21.6.json:6">
P2: Deferring `user_functions/init` by 20 ticks can make `main` run before initialization, causing early-tick logic to execute with uninitialized state.</violation>
</file>
<file name="minecraft_script/versions/1.21.5.json">
<violation number="1" location="minecraft_script/versions/1.21.5.json:6">
P1: Delaying `user_functions/init` by 20 ticks allows `main` to run before initialization, creating a startup ordering bug.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsPackWatcher.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsPackWatcher.java:145">
P1: Early-returning when `findPackEntry` is empty prevents cleanup on pack deletion, leaving stale generated datapacks.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsCommandService.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsCommandService.java:45">
P2: `setPackEnabled` reloads the server even for successful no-op results (already enabled/disabled), causing unnecessary reloads.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsPaths.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsPaths.java:74">
P2: Global datapack naming can collide with world pack names (e.g., world `global_x` vs global `x`), causing overwrite/delete of the wrong generated datapack.</violation>
</file>
<file name="mod/platform/fabric/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/fabric/McsFabricCommands.java">
<violation number="1" location="mod/platform/fabric/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/fabric/McsFabricCommands.java:28">
P1: Permission requirement was lowered from owner-level (4) to admin-level (3), exposing mcs management commands to lower-privileged operators.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsWorldPackManager.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsWorldPackManager.java:160">
P2: `refreshPack(Path)` ignores the provided folder path and looks up by pack name only, which can refresh the wrong pack when world/global names collide.</violation>
</file>
<file name="minecraft_script/versions/1.21.2.json">
<violation number="1" location="minecraft_script/versions/1.21.2.json:6">
P1: Delaying `user_functions/init` by 20 ticks allows `main` to run before initialization, creating an init-order race after reload.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| "pack.mcmeta": "{\n \"pack\": {\n \"min_format\": [{{formatMinMajor}}, {{formatMinMinor}}],\n \"max_format\": [{{formatMaxMajor}}, {{formatMaxMinor}}],\n \"description\": \"{{packDescription}}\"\n }\n}", | ||
| "datapack.init.header": "################################################################\n# #\n# default init.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n", | ||
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init", | ||
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t", |
There was a problem hiding this comment.
P1: Delaying user_functions/init by 20 ticks allows main to run before initialization, causing startup-order regressions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At minecraft_script/versions/26.1.json, line 6:
<comment>Delaying `user_functions/init` by 20 ticks allows `main` to run before initialization, causing startup-order regressions.</comment>
<file context>
@@ -3,7 +3,7 @@
"pack.mcmeta": "{\n \"pack\": {\n \"min_format\": [{{formatMinMajor}}, {{formatMinMinor}}],\n \"max_format\": [{{formatMaxMajor}}, {{formatMaxMinor}}],\n \"description\": \"{{packDescription}}\"\n }\n}",
"datapack.init.header": "################################################################\n# #\n# default init.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n",
- "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init",
+ "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t",
"datapack.main.header": "################################################################\n# #\n# default main.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n",
"datapack.main": "function {{datapack_id}}:user_functions/main",
</file context>
Suggested change
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t", | |
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init", |
| "pack.mcmeta": "{\n \"pack\": {\n \"pack_format\": {{pack_format}},\n \"supported_formats\": [71, 71],\n \"description\": \"{{packDescription}}\"\n }\n}", | ||
| "datapack.init.header": "################################################################\n# #\n# default init.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n", | ||
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init", | ||
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t", |
There was a problem hiding this comment.
P1: Delaying user_functions/init by 20 ticks allows main to run before initialization, creating a startup ordering bug.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At minecraft_script/versions/1.21.5.json, line 6:
<comment>Delaying `user_functions/init` by 20 ticks allows `main` to run before initialization, creating a startup ordering bug.</comment>
<file context>
@@ -3,7 +3,7 @@
"pack.mcmeta": "{\n \"pack\": {\n \"pack_format\": {{pack_format}},\n \"supported_formats\": [71, 71],\n \"description\": \"{{packDescription}}\"\n }\n}",
"datapack.init.header": "################################################################\n# #\n# default init.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n",
- "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init",
+ "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t",
"datapack.main.header": "################################################################\n# #\n# default main.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n",
"datapack.main": "function {{datapack_id}}:user_functions/main",
</file context>
Suggested change
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t", | |
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init", |
Comment on lines
+145
to
+149
| if (entry.isEmpty() | ||
| || !packManager.isPackEnabled(entry.get()) | ||
| || !packManager.isHotReloadEnabledForPack(entry.get())) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
P1: Early-returning when findPackEntry is empty prevents cleanup on pack deletion, leaving stale generated datapacks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mod/common/src/main/java/dev/spyc0der/minecraftscript/McsPackWatcher.java, line 145:
<comment>Early-returning when `findPackEntry` is empty prevents cleanup on pack deletion, leaving stale generated datapacks.</comment>
<file context>
@@ -136,6 +140,18 @@ private void scheduleFullScan() {
Path normalized = packFolder.toAbsolutePath().normalize();
+ try {
+ Optional<McsPackEntry> entry = packManager.findPackEntry(packFolder.getFileName().toString());
+ if (entry.isEmpty()
+ || !packManager.isPackEnabled(entry.get())
+ || !packManager.isHotReloadEnabledForPack(entry.get())) {
</file context>
Suggested change
| if (entry.isEmpty() | |
| || !packManager.isPackEnabled(entry.get()) | |
| || !packManager.isHotReloadEnabledForPack(entry.get())) { | |
| return; | |
| } | |
| if (entry.isPresent() | |
| && (!packManager.isPackEnabled(entry.get()) | |
| || !packManager.isHotReloadEnabledForPack(entry.get()))) { | |
| return; | |
| } |
|
|
||
| public static void register(CommandDispatcher<CommandSourceStack> dispatcher) { | ||
| dispatcher.register(Commands.literal("mcs") | ||
| .requires(Commands.hasPermission(Commands.LEVEL_ADMINS)) |
There was a problem hiding this comment.
P1: Permission requirement was lowered from owner-level (4) to admin-level (3), exposing mcs management commands to lower-privileged operators.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mod/platform/fabric/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/fabric/McsFabricCommands.java, line 28:
<comment>Permission requirement was lowered from owner-level (4) to admin-level (3), exposing mcs management commands to lower-privileged operators.</comment>
<file context>
@@ -0,0 +1,113 @@
+
+ public static void register(CommandDispatcher<CommandSourceStack> dispatcher) {
+ dispatcher.register(Commands.literal("mcs")
+ .requires(Commands.hasPermission(Commands.LEVEL_ADMINS))
+ .then(Commands.literal("hotreloading")
+ .then(Commands.literal("enable")
</file context>
Suggested change
| .requires(Commands.hasPermission(Commands.LEVEL_ADMINS)) | |
| .requires(Commands.hasPermission(Commands.LEVEL_OWNERS)) |
| "pack.mcmeta": "{\n \"pack\": {\n \"pack_format\": {{pack_format}},\n \"supported_formats\": [48, 57],\n \"description\": \"{{packDescription}}\"\n }\n}", | ||
| "datapack.init.header": "################################################################\n# #\n# default init.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n", | ||
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init", | ||
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t", |
There was a problem hiding this comment.
P1: Delaying user_functions/init by 20 ticks allows main to run before initialization, creating an init-order race after reload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At minecraft_script/versions/1.21.2.json, line 6:
<comment>Delaying `user_functions/init` by 20 ticks allows `main` to run before initialization, creating an init-order race after reload.</comment>
<file context>
@@ -3,7 +3,7 @@
"pack.mcmeta": "{\n \"pack\": {\n \"pack_format\": {{pack_format}},\n \"supported_formats\": [48, 57],\n \"description\": \"{{packDescription}}\"\n }\n}",
"datapack.init.header": "################################################################\n# #\n# default init.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n",
- "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init",
+ "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t",
"datapack.main.header": "################################################################\n# #\n# default main.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n",
"datapack.main": "function {{datapack_id}}:user_functions/main",
</file context>
Suggested change
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t", | |
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init", |
| "pack.mcmeta": "{\n \"pack\": {\n \"min_format\": [{{formatMinMajor}}, {{formatMinMinor}}],\n \"max_format\": [{{formatMaxMajor}}, {{formatMaxMinor}}],\n \"description\": \"{{packDescription}}\"\n }\n}", | ||
| "datapack.init.header": "################################################################\n# #\n# default init.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n", | ||
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init", | ||
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t", |
There was a problem hiding this comment.
P2: Delaying user_functions/init by 20 ticks can run main before initialization is complete, breaking prior load-before-tick ordering.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At minecraft_script/versions/1.21.9-10.json, line 6:
<comment>Delaying `user_functions/init` by 20 ticks can run `main` before initialization is complete, breaking prior load-before-tick ordering.</comment>
<file context>
@@ -3,7 +3,7 @@
"pack.mcmeta": "{\n \"pack\": {\n \"min_format\": [{{formatMinMajor}}, {{formatMinMinor}}],\n \"max_format\": [{{formatMaxMajor}}, {{formatMaxMinor}}],\n \"description\": \"{{packDescription}}\"\n }\n}",
"datapack.init.header": "################################################################\n# #\n# default init.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n",
- "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init",
+ "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t",
"datapack.main.header": "################################################################\n# #\n# default main.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n",
"datapack.main": "function {{datapack_id}}:user_functions/main",
</file context>
Suggested change
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t", | |
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init", |
| "pack.mcmeta": "{\n \"pack\": {\n \"pack_format\": {{pack_format}},\n \"supported_formats\": [80, 80],\n \"description\": \"{{packDescription}}\"\n }\n}", | ||
| "datapack.init.header": "################################################################\n# #\n# default init.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n", | ||
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init", | ||
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t", |
There was a problem hiding this comment.
P2: Deferring user_functions/init by 20 ticks can make main run before initialization, causing early-tick logic to execute with uninitialized state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At minecraft_script/versions/1.21.6.json, line 6:
<comment>Deferring `user_functions/init` by 20 ticks can make `main` run before initialization, causing early-tick logic to execute with uninitialized state.</comment>
<file context>
@@ -3,7 +3,7 @@
"pack.mcmeta": "{\n \"pack\": {\n \"pack_format\": {{pack_format}},\n \"supported_formats\": [80, 80],\n \"description\": \"{{packDescription}}\"\n }\n}",
"datapack.init.header": "################################################################\n# #\n# default init.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n",
- "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init",
+ "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t",
"datapack.main.header": "################################################################\n# #\n# default main.mcfunction file generated by Minecraft-Script #\n# #\n################################################################\n",
"datapack.main": "function {{datapack_id}}:user_functions/main",
</file context>
Suggested change
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nschedule function {{datapack_id}}:user_functions/init 20t", | |
| "datapack.init": "scoreboard objectives add mcs_math dummy \"Minecraft-Script Math\"\nscoreboard objectives add mcs_click {{clickScoreboardCriterion}} \"Minecraft-Script Click\"\n\nfunction {{datapack_id}}:user_functions/init", |
| return McsCommandResult.error("Minecraft Script is not active on this server."); | ||
| } | ||
| try { | ||
| McsCommandResult result = packManager.setPackEnabled(packName, enabled); |
There was a problem hiding this comment.
P2: setPackEnabled reloads the server even for successful no-op results (already enabled/disabled), causing unnecessary reloads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mod/common/src/main/java/dev/spyc0der/minecraftscript/McsCommandService.java, line 45:
<comment>`setPackEnabled` reloads the server even for successful no-op results (already enabled/disabled), causing unnecessary reloads.</comment>
<file context>
@@ -0,0 +1,115 @@
+ return McsCommandResult.error("Minecraft Script is not active on this server.");
+ }
+ try {
+ McsCommandResult result = packManager.setPackEnabled(packName, enabled);
+ if (result.success()) {
+ serverAccess.executeReload();
</file context>
| public static String generatedDatapackName(Path packFolder, McsPackSource source) { | ||
| String sanitized = sanitizePackFolderName(packFolder.getFileName().toString()); | ||
| if (source == McsPackSource.GLOBAL) { | ||
| return GENERATED_PREFIX + "global_" + sanitized; |
There was a problem hiding this comment.
P2: Global datapack naming can collide with world pack names (e.g., world global_x vs global x), causing overwrite/delete of the wrong generated datapack.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mod/common/src/main/java/dev/spyc0der/minecraftscript/McsPaths.java, line 74:
<comment>Global datapack naming can collide with world pack names (e.g., world `global_x` vs global `x`), causing overwrite/delete of the wrong generated datapack.</comment>
<file context>
@@ -42,6 +65,18 @@ public static String sanitizePackFolderName(String folderName) {
+ public static String generatedDatapackName(Path packFolder, McsPackSource source) {
+ String sanitized = sanitizePackFolderName(packFolder.getFileName().toString());
+ if (source == McsPackSource.GLOBAL) {
+ return GENERATED_PREFIX + "global_" + sanitized;
+ }
+ return GENERATED_PREFIX + sanitized;
</file context>
Suggested change
| return GENERATED_PREFIX + "global_" + sanitized; | |
| return GENERATED_PREFIX + "global__" + sanitized; |
| public boolean refreshPack(Path packFolder) { | ||
| Path packFile = packFolder.resolve(McsPaths.PACK_FILE); | ||
| try { | ||
| Optional<McsPackEntry> entry = findPackEntry(packFolder.getFileName().toString()); |
There was a problem hiding this comment.
P2: refreshPack(Path) ignores the provided folder path and looks up by pack name only, which can refresh the wrong pack when world/global names collide.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mod/common/src/main/java/dev/spyc0der/minecraftscript/McsWorldPackManager.java, line 160:
<comment>`refreshPack(Path)` ignores the provided folder path and looks up by pack name only, which can refresh the wrong pack when world/global names collide.</comment>
<file context>
@@ -75,15 +156,45 @@ public void notifyPackChangePending(Path packFolder) {
public boolean refreshPack(Path packFolder) {
- Path packFile = packFolder.resolve(McsPaths.PACK_FILE);
+ try {
+ Optional<McsPackEntry> entry = findPackEntry(packFolder.getFileName().toString());
+ if (entry.isEmpty()) {
+ return deleteGeneratedPack(packFolder, resolvePackSource(packFolder));
</file context>
Suggested change
| Optional<McsPackEntry> entry = findPackEntry(packFolder.getFileName().toString()); | |
| Optional<McsPackEntry> entry = discoverPackEntries().stream() | |
| .filter(candidate -> candidate.packFolder().toAbsolutePath().normalize() | |
| .equals(packFolder.toAbsolutePath().normalize())) | |
| .findFirst(); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
mod/common/src/main/java/dev/spyc0der/minecraftscript/McsStarterPack.java (2)
29-31: 💤 Low valueConsider adding a comment explaining the escape order.
The method correctly escapes backslashes before quotes, which is essential to avoid double-escaping. While the implementation is correct, a brief comment would help future maintainers understand why order matters.
📝 Suggested comment
public static String escapeForTextLiteral(String value) { + // Backslashes must be escaped first to avoid double-escaping quotes return value.replace("\\", "\\\\").replace("\"", "\\\""); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mod/common/src/main/java/dev/spyc0der/minecraftscript/McsStarterPack.java` around lines 29 - 31, Add a brief comment above the McsStarterPack.escapeForTextLiteral method explaining that backslashes are escaped first to prevent newly introduced backslashes from being re-escaped when quotes are processed (i.e., ".replace("\\", "\\\\") before .replace("\"", "\\\"") ensures correct escaping), so future maintainers understand the required order and rationale.
33-49: 💤 Low valueConsider adding filename length validation.
The method validates forbidden characters and reserved names, but doesn't check for filename length limits. Most filesystems support up to 255 bytes, but very long pack names could theoretically fail during file creation. While unlikely in practice, adding a length check would make the validation more robust.
🛡️ Suggested length check
public static boolean isValidPackName(String packName) { if (packName == null || packName.isBlank()) { return false; } if (packName.equals(".") || packName.equals("..")) { return false; } + if (packName.length() > 200) { // Leave margin below filesystem limit + return false; + } for (int index = 0; index < packName.length(); index++) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mod/common/src/main/java/dev/spyc0der/minecraftscript/McsStarterPack.java` around lines 33 - 49, The isValidPackName method currently checks null/blank, reserved names, and forbidden characters but lacks a filename-length guard; update isValidPackName to reject names exceeding typical filesystem limits by returning false when the UTF-8 byte length of packName (use packName.getBytes(StandardCharsets.UTF_8).length) is greater than 255 (or choose a configurable MAX_NAME_BYTES constant), and ensure to import java.nio.charset.StandardCharsets and introduce a MAX_NAME_BYTES constant used in the check so very long names are rejected early.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mod/common/src/main/java/dev/spyc0der/minecraftscript/McsModConfig.java`:
- Around line 32-33: The record McsModConfig currently assigns mutable
LinkedHashSet instances returned by normalizePackSet to its fields
hotReloadDisabledPacks and disabledPacks (and similar assignments later in the
class), exposing them via accessors; instead, wrap the normalized sets with an
unmodifiable wrapper before storing to prevent external mutation—i.e., replace
direct assignments like hotReloadDisabledPacks = normalizePackSet(...) and
disabledPacks = normalizePackSet(...) (and the other similar assignments around
the other constructors/initializers) with assignments that create and store
Collections.unmodifiableSet(new LinkedHashSet<>(normalizePackSet(...))) (or
equivalent) so callers receive immutable views while preserving iteration order.
- Around line 39-43: The config file read in McsModConfig currently swallows
IOException when loading properties (the try-with-resources around
properties.load(reader)), which hides corrupted/unreadable configs; change this
to either rethrow the IOException (or wrap in a RuntimeException) or at minimum
log the error via the module's logger before continuing so startup can surface
failures—locate the Files.isRegularFile(configFile) block in McsModConfig and
replace the empty catch of IOException with code that logs the exception
(including message and stack trace) and/or rethrows it to trigger the startup
error path.
In
`@mod/platform/forge/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/forge/McsForgeCommands.java`:
- Line 28: The Forge command registration in McsForgeCommands uses
.requires(Commands.hasPermission(Commands.LEVEL_ADMINS)) which diverges from
other variants; change the requirement to the same predicate used in mc_1_21_9
and mc_legacy by replacing it with a lambda that checks the numeric permission
(source -> source.hasPermission(4)) on the command builder used when registering
the /mcs admin command tree (locate the CommandDispatcher/
LiteralArgumentBuilder creation in McsForgeCommands and update the .requires
call).
---
Nitpick comments:
In `@mod/common/src/main/java/dev/spyc0der/minecraftscript/McsStarterPack.java`:
- Around line 29-31: Add a brief comment above the
McsStarterPack.escapeForTextLiteral method explaining that backslashes are
escaped first to prevent newly introduced backslashes from being re-escaped when
quotes are processed (i.e., ".replace("\\", "\\\\") before .replace("\"",
"\\\"") ensures correct escaping), so future maintainers understand the required
order and rationale.
- Around line 33-49: The isValidPackName method currently checks null/blank,
reserved names, and forbidden characters but lacks a filename-length guard;
update isValidPackName to reject names exceeding typical filesystem limits by
returning false when the UTF-8 byte length of packName (use
packName.getBytes(StandardCharsets.UTF_8).length) is greater than 255 (or choose
a configurable MAX_NAME_BYTES constant), and ensure to import
java.nio.charset.StandardCharsets and introduce a MAX_NAME_BYTES constant used
in the check so very long names are rejected early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6e83708-20bd-4914-a098-5fb67ad11ebc
📒 Files selected for processing (46)
minecraft_script/versions/1.21.11.jsonminecraft_script/versions/1.21.2.jsonminecraft_script/versions/1.21.4.jsonminecraft_script/versions/1.21.5.jsonminecraft_script/versions/1.21.6.jsonminecraft_script/versions/1.21.7-8.jsonminecraft_script/versions/1.21.9-10.jsonminecraft_script/versions/26.1.jsonmod/common/src/main/java/dev/spyc0der/minecraftscript/McsCommandResult.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsCommandService.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsCompilerConfig.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsModConfig.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsModRuntime.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsPackEntry.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsPackSource.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsPackWatcher.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsPaths.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsServerAccess.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsStarterPack.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsWorldPackManager.javamod/common/src/main/java/dev/spyc0der/minecraftscript/ProcessMcsCompiler.javamod/common/src/test/java/dev/spyc0der/minecraftscript/McsModConfigTest.javamod/common/src/test/java/dev/spyc0der/minecraftscript/McsStarterPackTest.javamod/common/src/test/java/dev/spyc0der/minecraftscript/McsWorldPackManagerTest.javamod/platform/fabric/src/main/java/dev/spyc0der/minecraftscript/fabric/McsFabricMod.javamod/platform/fabric/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/fabric/FabricServerAccess.javamod/platform/fabric/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/fabric/McsFabricCommands.javamod/platform/fabric/src/mc_1_21_9/java/dev/spyc0der/minecraftscript/fabric/FabricServerAccess.javamod/platform/fabric/src/mc_1_21_9/java/dev/spyc0der/minecraftscript/fabric/McsFabricCommands.javamod/platform/fabric/src/mc_legacy/java/dev/spyc0der/minecraftscript/fabric/FabricServerAccess.javamod/platform/fabric/src/mc_legacy/java/dev/spyc0der/minecraftscript/fabric/McsFabricCommands.javamod/platform/forge/src/forge_legacy/java/dev/spyc0der/minecraftscript/forge/McsForgeMod.javamod/platform/forge/src/forge_modern/java/dev/spyc0der/minecraftscript/forge/McsForgeMod.javamod/platform/forge/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/forge/ForgeServerAccess.javamod/platform/forge/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/forge/McsForgeCommands.javamod/platform/forge/src/mc_1_21_9/java/dev/spyc0der/minecraftscript/forge/ForgeServerAccess.javamod/platform/forge/src/mc_1_21_9/java/dev/spyc0der/minecraftscript/forge/McsForgeCommands.javamod/platform/forge/src/mc_legacy/java/dev/spyc0der/minecraftscript/forge/ForgeServerAccess.javamod/platform/forge/src/mc_legacy/java/dev/spyc0der/minecraftscript/forge/McsForgeCommands.javamod/platform/neoforge/src/main/java/dev/spyc0der/minecraftscript/neoforge/McsNeoForgeMod.javamod/platform/neoforge/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/neoforge/McsNeoForgeCommands.javamod/platform/neoforge/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/neoforge/NeoForgeServerAccess.javamod/platform/neoforge/src/mc_1_21_9/java/dev/spyc0der/minecraftscript/neoforge/McsNeoForgeCommands.javamod/platform/neoforge/src/mc_1_21_9/java/dev/spyc0der/minecraftscript/neoforge/NeoForgeServerAccess.javamod/platform/neoforge/src/mc_legacy/java/dev/spyc0der/minecraftscript/neoforge/McsNeoForgeCommands.javamod/platform/neoforge/src/mc_legacy/java/dev/spyc0der/minecraftscript/neoforge/NeoForgeServerAccess.java
💤 Files with no reviewable changes (1)
- mod/common/src/main/java/dev/spyc0der/minecraftscript/McsCompilerConfig.java
Comment on lines
+32
to
+33
| hotReloadDisabledPacks = normalizePackSet(hotReloadDisabledPacks); | ||
| disabledPacks = normalizePackSet(disabledPacks); |
There was a problem hiding this comment.
Seal Set fields against external mutation.
hotReloadDisabledPacks/disabledPacks are stored as mutable LinkedHashSets and exposed via record accessors, so callers can mutate config state after creation. Wrap normalized/parsed sets with unmodifiable copies before storing.
Proposed fix
import java.util.Arrays;
+import java.util.Collections;
import java.util.LinkedHashSet;
@@
private static Set<String> normalizePackSet(Set<String> packNames) {
if (packNames == null || packNames.isEmpty()) {
return Set.of();
}
- return packNames.stream()
+ Set<String> normalized = packNames.stream()
.map(McsModConfig::normalizePackName)
.filter(name -> !name.isBlank())
.collect(Collectors.toCollection(LinkedHashSet::new));
+ return Collections.unmodifiableSet(normalized);
}
@@
private static Set<String> parsePackSet(String value) {
if (value == null || value.isBlank()) {
return Set.of();
}
- return Arrays.stream(value.split(","))
+ Set<String> parsed = Arrays.stream(value.split(","))
.map(McsModConfig::normalizePackName)
.filter(name -> !name.isBlank())
.collect(Collectors.toCollection(LinkedHashSet::new));
+ return Collections.unmodifiableSet(parsed);
}Also applies to: 113-121, 130-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mod/common/src/main/java/dev/spyc0der/minecraftscript/McsModConfig.java`
around lines 32 - 33, The record McsModConfig currently assigns mutable
LinkedHashSet instances returned by normalizePackSet to its fields
hotReloadDisabledPacks and disabledPacks (and similar assignments later in the
class), exposing them via accessors; instead, wrap the normalized sets with an
unmodifiable wrapper before storing to prevent external mutation—i.e., replace
direct assignments like hotReloadDisabledPacks = normalizePackSet(...) and
disabledPacks = normalizePackSet(...) (and the other similar assignments around
the other constructors/initializers) with assignments that create and store
Collections.unmodifiableSet(new LinkedHashSet<>(normalizePackSet(...))) (or
equivalent) so callers receive immutable views while preserving iteration order.
Comment on lines
+39
to
+43
| if (Files.isRegularFile(configFile)) { | ||
| try (Reader reader = Files.newBufferedReader(configFile)) { | ||
| properties.load(reader); | ||
| } catch (IOException ignored) { | ||
| } |
There was a problem hiding this comment.
Do not silently ignore config read failures.
On Line 42, IOException is swallowed; that hides corrupted/unreadable config and prevents the startup error path from surfacing the issue. Propagate (or at least log) the exception so operator-facing diagnostics work.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mod/common/src/main/java/dev/spyc0der/minecraftscript/McsModConfig.java`
around lines 39 - 43, The config file read in McsModConfig currently swallows
IOException when loading properties (the try-with-resources around
properties.load(reader)), which hides corrupted/unreadable configs; change this
to either rethrow the IOException (or wrap in a RuntimeException) or at minimum
log the error via the module's logger before continuing so startup can surface
failures—locate the Files.isRegularFile(configFile) block in McsModConfig and
replace the empty catch of IOException with code that logs the exception
(including message and stack trace) and/or rethrows it to trigger the startup
error path.
|
|
||
| public static void register(CommandDispatcher<CommandSourceStack> dispatcher) { | ||
| dispatcher.register(Commands.literal("mcs") | ||
| .requires(Commands.hasPermission(Commands.LEVEL_ADMINS)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Forge command permission gates are consistent across versioned command registrars.
fd McsForgeCommands.java mod/platform/forge/src -tf | sort | while read -r file; do
echo "== $file =="
rg -n '\.requires\(' "$file"
doneRepository: SpyC0der77/Minecraft-Script
Length of output: 579
Align Forge command authorization across variants.
In McsForgeCommands.java line 28, mc_1_21_11 uses .requires(Commands.hasPermission(Commands.LEVEL_ADMINS)), while mc_1_21_9 and mc_legacy use .requires(source -> source.hasPermission(4)), creating version-dependent admin access control for the same /mcs admin command tree.
Suggested minimal alignment (match the other variants)
- .requires(Commands.hasPermission(Commands.LEVEL_ADMINS))
+ .requires(source -> source.hasPermission(4))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@mod/platform/forge/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/forge/McsForgeCommands.java`
at line 28, The Forge command registration in McsForgeCommands uses
.requires(Commands.hasPermission(Commands.LEVEL_ADMINS)) which diverges from
other variants; change the requirement to the same predicate used in mc_1_21_9
and mc_legacy by replacing it with a lambda that checks the numeric permission
(source -> source.hasPermission(4)) on the command builder used when registering
the /mcs admin command tree (locate the CommandDispatcher/
LiteralArgumentBuilder creation in McsForgeCommands and update the .requires
call).
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.
datapack.inittemplate in multiple version files to replace direct function calls with scheduled function calls, enhancing the initialization process for better timing control.Summary by CodeRabbit
Release Notes
New Features
/mcscommand suite for pack management: enable/disable packs and hot-reload, create packs (global or local scopes), reload packs, and list pack status.Bug Fixes
Improvements