Add mod#17
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces a complete Minecraft mod build matrix infrastructure and runtime pack compilation/watching system. It extends the compiler with version-override support, implements cross-loader mod entrypoints (Fabric/Forge/NeoForge), provides CI automation via GitHub Actions, and wires server lifecycle events to pack discovery, compilation, and hot-reload. ChangesMod Matrix Build and Runtime Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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 57 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="mod/gradle.properties">
<violation number="1" location="mod/gradle.properties:12">
P2: Using SNAPSHOT plugin versions makes the Gradle build non-reproducible and prone to unexpected breakage.</violation>
</file>
<file name="mod/platform/neoforge/src/main/resources/META-INF/neoforge.mods.toml">
<violation number="1" location="mod/platform/neoforge/src/main/resources/META-INF/neoforge.mods.toml:11">
P2: Missing explicit `neoforge` dependency/version range allows loading on incompatible NeoForge versions.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der/minecraftscript/ProcessMcsCompiler.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der/minecraftscript/ProcessMcsCompiler.java:38">
P1: Interrupted/timeout compile paths do not wait for subprocess termination, which can leave the compiler process running after this method returns.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsCompilerConfig.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsCompilerConfig.java:38">
P2: Malformed properties content can crash config loading because `IllegalArgumentException` from `Properties.load` is not caught.</violation>
</file>
<file name="mod/platform/forge/src/main/resources/META-INF/mods.toml">
<violation number="1" location="mod/platform/forge/src/main/resources/META-INF/mods.toml:2">
P2: `loaderVersion` is too broad (`[1,)`), so this metadata does not meaningfully gate compatible Forge/FML versions.</violation>
<violation number="2" location="mod/platform/forge/src/main/resources/META-INF/mods.toml:12">
P1: Missing explicit `forge` dependency allows unsupported Forge versions to pass dependency checks.</violation>
<violation number="3" location="mod/platform/forge/src/main/resources/META-INF/mods.toml:13">
P1: Forge dependency metadata uses `type` instead of required `mandatory`, which can break mod loading.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsModRuntime.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsModRuntime.java:23">
P2: Watcher construction can leak `WatchService` resources if initialization fails partway through.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsCommandLine.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der/minecraftscript/McsCommandLine.java:24">
P1: Backslashes are always consumed as escapes, which corrupts Windows-style paths in quoted commands.</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:45">
P1: Generated datapack names are not unique across distinct pack folder names, so one pack can overwrite another when names sanitize to the same value.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Partial review: This PR has more than 50 files, so cubic reviewed the highest-priority files first. During the trial, paid plans get a higher file limit.
You can try an ultrareview to bypass the file limit, comment @cubic-dev-ai ultrareview. Learn more.
Re-trigger cubic
| escaping = false; | ||
| continue; | ||
| } | ||
| if (character == '\\') { |
There was a problem hiding this comment.
P1: Backslashes are always consumed as escapes, which corrupts Windows-style paths in quoted commands.
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/McsCommandLine.java, line 24:
<comment>Backslashes are always consumed as escapes, which corrupts Windows-style paths in quoted commands.</comment>
<file context>
@@ -0,0 +1,54 @@
+ escaping = false;
+ continue;
+ }
+ if (character == '\\') {
+ escaping = true;
+ continue;
</file context>
| .redirectOutput(logFile.toFile()) | ||
| .start(); | ||
|
|
||
| boolean exited = process.waitFor(COMPILE_TIMEOUT_SECONDS, TimeUnit.SECONDS); |
There was a problem hiding this comment.
P1: Interrupted/timeout compile paths do not wait for subprocess termination, which can leave the compiler process running after this method returns.
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/ProcessMcsCompiler.java, line 38:
<comment>Interrupted/timeout compile paths do not wait for subprocess termination, which can leave the compiler process running after this method returns.</comment>
<file context>
@@ -0,0 +1,50 @@
+ .redirectOutput(logFile.toFile())
+ .start();
+
+ boolean exited = process.waitFor(COMPILE_TIMEOUT_SECONDS, TimeUnit.SECONDS);
+ String output = Files.readString(logFile, StandardCharsets.UTF_8);
+ if (!exited) {
</file context>
|
|
||
| [[dependencies.${mod_id}]] | ||
| modId = "minecraft" | ||
| type = "required" |
There was a problem hiding this comment.
P1: Forge dependency metadata uses type instead of required mandatory, which can break mod loading.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mod/platform/forge/src/main/resources/META-INF/mods.toml, line 13:
<comment>Forge dependency metadata uses `type` instead of required `mandatory`, which can break mod loading.</comment>
<file context>
@@ -0,0 +1,16 @@
+
+[[dependencies.${mod_id}]]
+modId = "minecraft"
+type = "required"
+versionRange = "[${minecraft_version}]"
+ordering = "NONE"
</file context>
| } | ||
|
|
||
| public static String generatedDatapackName(Path packFolder) { | ||
| return GENERATED_PREFIX + sanitizePackFolderName(packFolder.getFileName().toString()); |
There was a problem hiding this comment.
P1: Generated datapack names are not unique across distinct pack folder names, so one pack can overwrite another when names sanitize to the same value.
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 45:
<comment>Generated datapack names are not unique across distinct pack folder names, so one pack can overwrite another when names sanitize to the same value.</comment>
<file context>
@@ -0,0 +1,47 @@
+ }
+
+ public static String generatedDatapackName(Path packFolder) {
+ return GENERATED_PREFIX + sanitizePackFolderName(packFolder.getFileName().toString());
+ }
+}
</file context>
| mod_license=MIT | ||
| mod_description=Compiles Minecraft-Script packs from world mcs_packs folders into datapacks. | ||
|
|
||
| architectury_loom_version=1.14-SNAPSHOT |
There was a problem hiding this comment.
P2: Using SNAPSHOT plugin versions makes the Gradle build non-reproducible and prone to unexpected breakage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mod/gradle.properties, line 12:
<comment>Using SNAPSHOT plugin versions makes the Gradle build non-reproducible and prone to unexpected breakage.</comment>
<file context>
@@ -0,0 +1,14 @@
+mod_license=MIT
+mod_description=Compiles Minecraft-Script packs from world mcs_packs folders into datapacks.
+
+architectury_loom_version=1.14-SNAPSHOT
+architectury_plugin_version=3.4-SNAPSHOT
+fabric_loader_version=0.19.3
</file context>
| displayName = "${mod_name}" | ||
| description = '''${mod_description}''' | ||
|
|
||
| [[dependencies.${mod_id}]] |
There was a problem hiding this comment.
P2: Missing explicit neoforge dependency/version range allows loading on incompatible NeoForge versions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mod/platform/neoforge/src/main/resources/META-INF/neoforge.mods.toml, line 11:
<comment>Missing explicit `neoforge` dependency/version range allows loading on incompatible NeoForge versions.</comment>
<file context>
@@ -0,0 +1,16 @@
+displayName = "${mod_name}"
+description = '''${mod_description}'''
+
+[[dependencies.${mod_id}]]
+modId = "minecraft"
+type = "required"
</file context>
| Properties properties = new Properties(); | ||
| try (Reader reader = Files.newBufferedReader(configFile)) { | ||
| properties.load(reader); | ||
| } catch (IOException ignored) { |
There was a problem hiding this comment.
P2: Malformed properties content can crash config loading because IllegalArgumentException from Properties.load is not caught.
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/McsCompilerConfig.java, line 38:
<comment>Malformed properties content can crash config loading because `IllegalArgumentException` from `Properties.load` is not caught.</comment>
<file context>
@@ -0,0 +1,43 @@
+ Properties properties = new Properties();
+ try (Reader reader = Files.newBufferedReader(configFile)) {
+ properties.load(reader);
+ } catch (IOException ignored) {
+ return null;
+ }
</file context>
| @@ -0,0 +1,16 @@ | |||
| modLoader = "javafml" | |||
| loaderVersion = "[1,)" | |||
There was a problem hiding this comment.
P2: loaderVersion is too broad ([1,)), so this metadata does not meaningfully gate compatible Forge/FML versions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mod/platform/forge/src/main/resources/META-INF/mods.toml, line 2:
<comment>`loaderVersion` is too broad (`[1,)`), so this metadata does not meaningfully gate compatible Forge/FML versions.</comment>
<file context>
@@ -0,0 +1,16 @@
+modLoader = "javafml"
+loaderVersion = "[1,)"
+license = "MIT"
+
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
mod/common/src/main/java/dev/spyc0der/minecraftscript/McsPackWatcher.java (1)
84-86: ⚡ Quick winUse a logger instead of
System.out.println.Direct console output bypasses standard logging infrastructure. Use a proper logger (e.g.,
org.slf4j.Loggeror platform-specific logger) to allow operators to control log levels and formatting.📝 Example using SLF4J
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + public final class McsPackWatcher implements Closeable { + private static final Logger LOGGER = LoggerFactory.getLogger(McsPackWatcher.class); private static final long COMPILE_DEBOUNCE_MILLIS = 750; ... try { registerRecursively(changed); } catch (IOException error) { - System.out.println("[Minecraft Script] Could not watch " + changed + ": " + error.getMessage()); + LOGGER.warn("Could not watch {}: {}", changed, error.getMessage()); }🤖 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/McsPackWatcher.java` around lines 84 - 86, Replace the System.out.println in the McsPackWatcher catch block with a proper SLF4J logger: add a private static final org.slf4j.Logger (e.g., LoggerFactory.getLogger(McsPackWatcher.class)) to the McsPackWatcher class and change the catch for IOException to call logger.error or logger.warn with a descriptive message including the changed variable and the exception (pass the exception as the throwable argument so the stacktrace is logged). Ensure imports for org.slf4j.Logger and org.slf4j.LoggerFactory are added.mod/build.gradle (1)
201-223: Address cross-repo path coupling and document the"26.1"exclusion invalidateModMatrix
mod/build.gradleusesrootProject.file("../minecraft_script/versions/index.json")forvalidateModMatrix, which assumesminecraft_script/is a sibling directory tomod/(so buildingmod/standalone will fail with a missing-file error).- The
"26.1"filter is consistent with the repo:minecraft_script/versions/index.jsonincludes"26.1", whileext.modSupportedMinecraftVersions(andmod/settings.gradle’s version list) only cover"1.21.2"–"1.21.11". Add an explicit comment explaining why"26.1"is excluded (it’s currently unexplained in the code).🤖 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/build.gradle` around lines 201 - 223, The validateModMatrix task currently hard-codes a sibling-repo path to rootProject.file("../minecraft_script/versions/index.json") which breaks standalone builds; change it to read the index via a configurable project property or ext value (e.g., a new property like minecraftVersionsIndexFile or ext.minecraftScriptDir) with a documented fallback only when that file exists, and fail with a clear message suggesting how to supply the property if not present; keep using validateModMatrix, modSupportedMinecraftVersions, modLoaders and index.json names to locate the code to change. Also add an inline comment next to the expectedVersions filter explaining why "26.1" is excluded (e.g., "exclude 26.1 because ext.modSupportedMinecraftVersions targets 1.21.x series only") so the special-case is documented.mod/platform/forge/src/main/java/dev/spyc0der/minecraftscript/forge/ForgeServerAccess.java (1)
30-38: ⚡ Quick winConsider adding error handling for command execution.
The
executeReload()method schedules a reload command asynchronously but provides no error handling or logging. If the command fails, the failure will be silent (or only visible in server console).💪 Proposed enhancement with error logging
`@Override` public void executeReload() { server.execute(() -> { - CommandSourceStack source = server.createCommandSourceStack() - .withPermission(4) - .withSuppressedOutput(); - server.getCommands().performPrefixedCommand(source, "reload"); + try { + CommandSourceStack source = server.createCommandSourceStack() + .withPermission(4) + .withSuppressedOutput(); + server.getCommands().performPrefixedCommand(source, "reload"); + } catch (Exception e) { + // Log error or notify operators + e.printStackTrace(); + } }); }🤖 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/main/java/dev/spyc0der/minecraftscript/forge/ForgeServerAccess.java` around lines 30 - 38, The executeReload() method currently schedules server.execute(...) and runs server.getCommands().performPrefixedCommand(...) with no error handling; wrap the runnable body in a try-catch that catches Exception (or CommandException) around the CommandSourceStack creation and performPrefixedCommand call (inside the lambda passed to server.execute), and on catch log the error with a clear message including the exception (use the class logger or server.getLogger() if available) so failures during executeReload() are recorded; also consider logging a success message after the command completes..github/workflows/mod-ci.yml (1)
20-20: ⚡ Quick winDisable persisted Git credentials in checkout steps.
Set
persist-credentials: falsein both checkout steps (Line 20 and Line 78) to reduce token exposure risk in workflow execution context.Suggested hardening diff
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + persist-credentials: false ... - - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + persist-credentials: falseAlso applies to: 78-78
🤖 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 @.github/workflows/mod-ci.yml at line 20, The checkout steps currently use actions/checkout@v4 without disabling persisted credentials; update both checkout usages (the actions/checkout@v4 steps referenced around the first occurrence and the second occurrence later in the workflow) to include persist-credentials: false so the runner does not persist the GITHUB_TOKEN into the workspace or subsequent steps.Source: Linters/SAST tools
🤖 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 @.github/workflows/mod-ci.yml:
- Line 20: The workflow uses floating action tags like "uses:
actions/checkout@v4" (and similar lines at "uses: actions/setup-node@v4", "uses:
actions/cache@v4", etc.) which must be pinned to immutable full-length commit
SHAs; update each "uses: <owner>/<repo>@<tag>" occurrence referenced in the diff
(e.g., actions/checkout@v4, actions/setup-node@v4, actions/cache@v4 and the
other uses lines noted) to the corresponding full commit SHA for the exact
action version, replacing the tag with the full-length SHA string so every
action is referenced by a specific immutable commit.
In `@mod/build.gradle`:
- Around line 76-82: Remove the project-level repositories block that redeclares
Maven repos (the repositories { ... } block containing maven { url =
"https://maven.fabricmc.net/" }, maven { url =
"https://maven.minecraftforge.net/" }, maven { url =
"https://maven.neoforged.net/releases/" }, maven { url =
"https://maven.architectury.dev/" }, and mavenCentral()), since settings.gradle
enforces RepositoriesMode.FAIL_ON_PROJECT_REPOS and already declares these
repos; delete that entire repositories block from mod/build.gradle so the
project relies on the repositories provided by settings.gradle.
In `@mod/common/src/main/java/dev/spyc0der/minecraftscript/McsCommandLine.java`:
- Around line 28-35: The command parser in McsCommandLine currently toggles
inSingleQuote/inDoubleQuote but doesn’t reject unterminated quotes; after the
tokenization loop(s) in the splitting method (the parsing routine inside class
McsCommandLine) add a post-loop check: if inSingleQuote or inDoubleQuote is true
then throw an IllegalArgumentException (or appropriate parse exception) with a
clear message about an unterminated quote so malformed input fails fast; apply
the same post-loop check to the other parsing branch referenced by the second
loop (the code around the logic at the other quote-handling site) so both paths
validate terminated quotes.
In
`@mod/common/src/main/java/dev/spyc0der/minecraftscript/McsCompilerConfig.java`:
- Around line 38-39: The catch block in McsCompilerConfig that currently does
"catch (IOException ignored) { return null; }" should not swallow the exception;
instead surface the failure by either rethrowing a wrapped exception (e.g., new
RuntimeException("Failed to read compiler config", e)) or logging the
IOException with context before returning; locate the IOException catch in the
McsCompilerConfig method that reads the config (the block returning null) and
replace the silent ignore with a proper log/error throw including the original
exception and a clear message about the config path/permission failure.
In `@mod/common/src/main/java/dev/spyc0der/minecraftscript/McsModRuntime.java`:
- Around line 23-29: The code currently assigns activeWatcher = new
McsPackWatcher(packManager, serverAccess) before calling activeWatcher.start(),
which can leave a partially initialized watcher on failure; change this to
create a local variable (e.g., McsPackWatcher watcher = new
McsPackWatcher(packManager, serverAccess)), call watcher.start(), and only
assign activeWatcher = watcher after start() completes successfully (and if
start() can throw or partially initialize, ensure to close/cleanup watcher in
the catch before rethrowing or logging). This references McsPackWatcher,
activeWatcher, packManager, serverAccess, and the start() call.
In
`@mod/common/src/main/java/dev/spyc0der/minecraftscript/ProcessMcsCompiler.java`:
- Around line 38-45: The code reads the logFile before guaranteeing the process
has stopped, causing a race; in ProcessMcsCompiler change the order so you only
call Files.readString(logFile, StandardCharsets.UTF_8) after the process is
confirmed terminated: keep the initial boolean exited =
process.waitFor(COMPILE_TIMEOUT_SECONDS, TimeUnit.SECONDS), and if !exited first
call process.destroyForcibly() and then wait for termination (e.g.,
process.waitFor() with no timeout or re-wait) before reading logFile, then
construct the McsCompileResult with exit code 124 and the read output; similarly
when exited is true read the log and return new
McsCompileResult(process.exitValue(), output).
In
`@mod/common/src/test/java/dev/spyc0der/minecraftscript/McsWorldPackManagerTest.java`:
- Around line 29-33: Test is flaky because it asserts a specific order for
manager.discoverPackFolders() results; change McsWorldPackManagerTest to perform
an order-independent check by either sorting the discovered names before
comparison or comparing as unordered collections (e.g., convert packNames and
expected list to Sets or use an assert that ignores order) so the assertion no
longer depends on filesystem traversal order from discoverPackFolders().
In `@mod/platform/forge/src/main/resources/META-INF/mods.toml`:
- Line 2: The mods.toml currently sets loaderVersion = "[1,)" which is too
permissive; update the loaderVersion range to the minimum supported Forge/FML
version from your ext.forgeVersions map and add an upper bound that matches the
top of your tested build matrix (e.g. use a range like
"[MIN_SUPPORTED,MAX_TESTED)"); change the loaderVersion value in mods.toml
accordingly and keep it in sync with the versions indexed by minecraftVersion in
build.gradle (ext.forgeVersions).
---
Nitpick comments:
In @.github/workflows/mod-ci.yml:
- Line 20: The checkout steps currently use actions/checkout@v4 without
disabling persisted credentials; update both checkout usages (the
actions/checkout@v4 steps referenced around the first occurrence and the second
occurrence later in the workflow) to include persist-credentials: false so the
runner does not persist the GITHUB_TOKEN into the workspace or subsequent steps.
In `@mod/build.gradle`:
- Around line 201-223: The validateModMatrix task currently hard-codes a
sibling-repo path to rootProject.file("../minecraft_script/versions/index.json")
which breaks standalone builds; change it to read the index via a configurable
project property or ext value (e.g., a new property like
minecraftVersionsIndexFile or ext.minecraftScriptDir) with a documented fallback
only when that file exists, and fail with a clear message suggesting how to
supply the property if not present; keep using validateModMatrix,
modSupportedMinecraftVersions, modLoaders and index.json names to locate the
code to change. Also add an inline comment next to the expectedVersions filter
explaining why "26.1" is excluded (e.g., "exclude 26.1 because
ext.modSupportedMinecraftVersions targets 1.21.x series only") so the
special-case is documented.
In `@mod/common/src/main/java/dev/spyc0der/minecraftscript/McsPackWatcher.java`:
- Around line 84-86: Replace the System.out.println in the McsPackWatcher catch
block with a proper SLF4J logger: add a private static final org.slf4j.Logger
(e.g., LoggerFactory.getLogger(McsPackWatcher.class)) to the McsPackWatcher
class and change the catch for IOException to call logger.error or logger.warn
with a descriptive message including the changed variable and the exception
(pass the exception as the throwable argument so the stacktrace is logged).
Ensure imports for org.slf4j.Logger and org.slf4j.LoggerFactory are added.
In
`@mod/platform/forge/src/main/java/dev/spyc0der/minecraftscript/forge/ForgeServerAccess.java`:
- Around line 30-38: The executeReload() method currently schedules
server.execute(...) and runs server.getCommands().performPrefixedCommand(...)
with no error handling; wrap the runnable body in a try-catch that catches
Exception (or CommandException) around the CommandSourceStack creation and
performPrefixedCommand call (inside the lambda passed to server.execute), and on
catch log the error with a clear message including the exception (use the class
logger or server.getLogger() if available) so failures during executeReload()
are recorded; also consider logging a success message after the command
completes.
🪄 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: a6e8fada-6415-466e-8e5e-f30751e6a641
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (56)
.github/workflows/mod-ci.yml.gitignoreminecraft_script/shell_commands.pymod/build.gradlemod/common/src/main/java/dev/spyc0der/minecraftscript/McsCommandLine.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsCompileResult.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsCompiler.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsCompilerConfig.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsModRuntime.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/McsCommandLineTest.javamod/common/src/test/java/dev/spyc0der/minecraftscript/McsWorldPackManagerTest.javamod/gradle.propertiesmod/platform/fabric/src/main/java/dev/spyc0der/minecraftscript/fabric/FabricServerAccess.javamod/platform/fabric/src/main/java/dev/spyc0der/minecraftscript/fabric/McsFabricMod.javamod/platform/fabric/src/main/resources/fabric.mod.jsonmod/platform/forge/src/main/java/dev/spyc0der/minecraftscript/forge/ForgeServerAccess.javamod/platform/forge/src/main/java/dev/spyc0der/minecraftscript/forge/McsForgeMod.javamod/platform/forge/src/main/resources/META-INF/mods.tomlmod/platform/neoforge/src/main/java/dev/spyc0der/minecraftscript/neoforge/McsNeoForgeMod.javamod/platform/neoforge/src/main/java/dev/spyc0der/minecraftscript/neoforge/NeoForgeServerAccess.javamod/platform/neoforge/src/main/resources/META-INF/neoforge.mods.tomlmod/settings.gradlemod/versions/mc_1_21_10/fabric/build.gradlemod/versions/mc_1_21_10/forge/build.gradlemod/versions/mc_1_21_10/neoforge/build.gradlemod/versions/mc_1_21_11/fabric/build.gradlemod/versions/mc_1_21_11/forge/build.gradlemod/versions/mc_1_21_11/neoforge/build.gradlemod/versions/mc_1_21_2/fabric/build.gradlemod/versions/mc_1_21_2/forge/build.gradlemod/versions/mc_1_21_2/neoforge/build.gradlemod/versions/mc_1_21_4/fabric/build.gradlemod/versions/mc_1_21_4/forge/build.gradlemod/versions/mc_1_21_4/neoforge/build.gradlemod/versions/mc_1_21_5/fabric/build.gradlemod/versions/mc_1_21_5/forge/build.gradlemod/versions/mc_1_21_5/neoforge/build.gradlemod/versions/mc_1_21_6/fabric/build.gradlemod/versions/mc_1_21_6/forge/build.gradlemod/versions/mc_1_21_6/neoforge/build.gradlemod/versions/mc_1_21_7/fabric/build.gradlemod/versions/mc_1_21_7/forge/build.gradlemod/versions/mc_1_21_7/neoforge/build.gradlemod/versions/mc_1_21_8/fabric/build.gradlemod/versions/mc_1_21_8/forge/build.gradlemod/versions/mc_1_21_8/neoforge/build.gradlemod/versions/mc_1_21_9/fabric/build.gradlemod/versions/mc_1_21_9/forge/build.gradlemod/versions/mc_1_21_9/neoforge/build.gradletests/test_mod_compile_cli.py
| working-directory: mod | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
Pin all GitHub Actions to commit SHAs.
Using floating tags (@v4) leaves CI vulnerable to upstream action supply-chain drift and violates the repository’s stated policy. Pin each action to an immutable full-length commit SHA.
Suggested hardening diff
- - uses: actions/checkout@v4
+ - uses: actions/checkout@<full-length-commit-sha>
...
- uses: actions/setup-java@v4
+ uses: actions/setup-java@<full-length-commit-sha>
...
- uses: gradle/actions/setup-gradle@v4
+ uses: gradle/actions/setup-gradle@<full-length-commit-sha>
...
- uses: actions/upload-artifact@v4
+ uses: actions/upload-artifact@<full-length-commit-sha>Also applies to: 23-23, 29-29, 38-38, 78-78, 81-81, 87-87, 103-103
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 20-20: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 20-20: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/mod-ci.yml at line 20, The workflow uses floating action
tags like "uses: actions/checkout@v4" (and similar lines at "uses:
actions/setup-node@v4", "uses: actions/cache@v4", etc.) which must be pinned to
immutable full-length commit SHAs; update each "uses: <owner>/<repo>@<tag>"
occurrence referenced in the diff (e.g., actions/checkout@v4,
actions/setup-node@v4, actions/cache@v4 and the other uses lines noted) to the
corresponding full commit SHA for the exact action version, replacing the tag
with the full-length SHA string so every action is referenced by a specific
immutable commit.
Source: Linters/SAST tools
Comment on lines
+76
to
+82
| repositories { | ||
| maven { url = "https://maven.fabricmc.net/" } | ||
| maven { url = "https://maven.minecraftforge.net/" } | ||
| maven { url = "https://maven.neoforged.net/releases/" } | ||
| maven { url = "https://maven.architectury.dev/" } | ||
| mavenCentral() | ||
| } |
There was a problem hiding this comment.
Remove repository declarations; they violate FAIL_ON_PROJECT_REPOS.
The CI pipeline fails because settings.gradle enforces RepositoriesMode.FAIL_ON_PROJECT_REPOS (line 13), but this block redeclares repositories in the project build file. All repositories are already declared in settings.gradle lines 14-20 and are automatically available to all subprojects.
🔧 Proposed fix
}
-
- repositories {
- maven { url = "https://maven.fabricmc.net/" }
- maven { url = "https://maven.minecraftforge.net/" }
- maven { url = "https://maven.neoforged.net/releases/" }
- maven { url = "https://maven.architectury.dev/" }
- mavenCentral()
- }
}
project(":common") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| repositories { | |
| maven { url = "https://maven.fabricmc.net/" } | |
| maven { url = "https://maven.minecraftforge.net/" } | |
| maven { url = "https://maven.neoforged.net/releases/" } | |
| maven { url = "https://maven.architectury.dev/" } | |
| mavenCentral() | |
| } |
🧰 Tools
🪛 GitHub Actions: Mod CI / 1_validate.txt
[error] 77-77: Gradle build failed during project evaluation. Build configured to prefer settings repositories over project repositories, but repository 'maven' was added by build.gradle. (org.gradle.api.InvalidUserCodeException)
🪛 GitHub Actions: Mod CI / validate
[error] 77-77: Gradle configuration failed while evaluating root project 'minecraft-script-mod'. Build was configured to prefer settings repositories over project repositories, but repository 'maven' was added by build file 'build.gradle' (repository mutation disallowed on project).
🤖 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/build.gradle` around lines 76 - 82, Remove the project-level repositories
block that redeclares Maven repos (the repositories { ... } block containing
maven { url = "https://maven.fabricmc.net/" }, maven { url =
"https://maven.minecraftforge.net/" }, maven { url =
"https://maven.neoforged.net/releases/" }, maven { url =
"https://maven.architectury.dev/" }, and mavenCentral()), since settings.gradle
enforces RepositoriesMode.FAIL_ON_PROJECT_REPOS and already declares these
repos; delete that entire repositories block from mod/build.gradle so the
project relies on the repositories provided by settings.gradle.
Comment on lines
+28
to
+35
| if (character == '\'' && !inDoubleQuote) { | ||
| inSingleQuote = !inSingleQuote; | ||
| continue; | ||
| } | ||
| if (character == '"' && !inSingleQuote) { | ||
| inDoubleQuote = !inDoubleQuote; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Fail fast on unterminated quotes in command parsing.
split silently accepts unclosed '/" and returns tokens anyway. That turns malformed command strings into unintended argv values instead of rejecting invalid input.
Suggested fix
if (escaping) {
current.append('\\');
}
+ if (inSingleQuote || inDoubleQuote) {
+ throw new IllegalArgumentException("Unterminated quoted string in command");
+ }
if (!current.isEmpty()) {
parts.add(current.toString());
}Also applies to: 49-53
🤖 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/McsCommandLine.java`
around lines 28 - 35, The command parser in McsCommandLine currently toggles
inSingleQuote/inDoubleQuote but doesn’t reject unterminated quotes; after the
tokenization loop(s) in the splitting method (the parsing routine inside class
McsCommandLine) add a post-loop check: if inSingleQuote or inDoubleQuote is true
then throw an IllegalArgumentException (or appropriate parse exception) with a
clear message about an unterminated quote so malformed input fails fast; apply
the same post-loop check to the other parsing branch referenced by the second
loop (the code around the logic at the other quote-handling site) so both paths
validate terminated quotes.
Comment on lines
+38
to
+39
| } catch (IOException ignored) { | ||
| return null; |
There was a problem hiding this comment.
Don’t silently swallow config-read failures.
On Line 38, swallowing IOException hides broken config/permissions and makes later compiler failures harder to diagnose. Surface this failure (log or throw with context) instead of silently defaulting.
Suggested patch
- } catch (IOException ignored) {
- return null;
+ } catch (IOException error) {
+ System.err.println("[Minecraft Script] Failed to read " + configFile + ": " + error.getMessage());
+ return null;
}🤖 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/McsCompilerConfig.java`
around lines 38 - 39, The catch block in McsCompilerConfig that currently does
"catch (IOException ignored) { return null; }" should not swallow the exception;
instead surface the failure by either rethrowing a wrapped exception (e.g., new
RuntimeException("Failed to read compiler config", e)) or logging the
IOException with context before returning; locate the IOException catch in the
McsCompilerConfig method that reads the config (the block returning null) and
replace the silent ignore with a proper log/error throw including the original
exception and a clear message about the config path/permission failure.
Comment on lines
+23
to
+29
| activeWatcher = new McsPackWatcher(packManager, serverAccess); | ||
| activeWatcher.start(); | ||
| } catch (IOException error) { | ||
| String message = "Could not initialize mcs_packs: " + error.getMessage(); | ||
| System.out.println("[Minecraft Script] " + message); | ||
| serverAccess.notifyOperators(message); | ||
| } |
There was a problem hiding this comment.
Avoid leaking/stashing a failed watcher instance.
On Line 23, activeWatcher is assigned before Line 24 start(). If start() throws, the catch path leaves a possibly partially initialized watcher hanging. Start with a local variable and only publish to activeWatcher after successful start().
Suggested patch
- activeWatcher = new McsPackWatcher(packManager, serverAccess);
- activeWatcher.start();
+ McsPackWatcher watcher = new McsPackWatcher(packManager, serverAccess);
+ watcher.start();
+ activeWatcher = watcher;
} catch (IOException error) {
String message = "Could not initialize mcs_packs: " + error.getMessage();
System.out.println("[Minecraft Script] " + message);
serverAccess.notifyOperators(message);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| activeWatcher = new McsPackWatcher(packManager, serverAccess); | |
| activeWatcher.start(); | |
| } catch (IOException error) { | |
| String message = "Could not initialize mcs_packs: " + error.getMessage(); | |
| System.out.println("[Minecraft Script] " + message); | |
| serverAccess.notifyOperators(message); | |
| } | |
| McsPackWatcher watcher = new McsPackWatcher(packManager, serverAccess); | |
| watcher.start(); | |
| activeWatcher = watcher; | |
| } catch (IOException error) { | |
| String message = "Could not initialize mcs_packs: " + error.getMessage(); | |
| System.out.println("[Minecraft Script] " + message); | |
| serverAccess.notifyOperators(message); | |
| } |
🤖 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/McsModRuntime.java`
around lines 23 - 29, The code currently assigns activeWatcher = new
McsPackWatcher(packManager, serverAccess) before calling activeWatcher.start(),
which can leave a partially initialized watcher on failure; change this to
create a local variable (e.g., McsPackWatcher watcher = new
McsPackWatcher(packManager, serverAccess)), call watcher.start(), and only
assign activeWatcher = watcher after start() completes successfully (and if
start() can throw or partially initialize, ensure to close/cleanup watcher in
the catch before rethrowing or logging). This references McsPackWatcher,
activeWatcher, packManager, serverAccess, and the start() call.
Comment on lines
+38
to
+45
| boolean exited = process.waitFor(COMPILE_TIMEOUT_SECONDS, TimeUnit.SECONDS); | ||
| String output = Files.readString(logFile, StandardCharsets.UTF_8); | ||
| if (!exited) { | ||
| process.destroyForcibly(); | ||
| return new McsCompileResult(124, "Minecraft-Script compile timed out after " | ||
| + COMPILE_TIMEOUT_SECONDS + " seconds.\n" + output); | ||
| } | ||
| return new McsCompileResult(process.exitValue(), output); |
There was a problem hiding this comment.
Race condition: Log file read before process termination.
The output is read from the log file (line 39) before checking whether the process timed out. If !exited is true, the process is still running when Files.readString is called, and it may still be writing to the log file. destroyForcibly() is only called afterward (line 41), creating a race condition that can capture incomplete or corrupted output in timeout scenarios.
Move the log file read to occur after the process is guaranteed to have stopped.
🔒 Proposed fix to eliminate the race
boolean exited = process.waitFor(COMPILE_TIMEOUT_SECONDS, TimeUnit.SECONDS);
- String output = Files.readString(logFile, StandardCharsets.UTF_8);
if (!exited) {
process.destroyForcibly();
+ String output = Files.readString(logFile, StandardCharsets.UTF_8);
return new McsCompileResult(124, "Minecraft-Script compile timed out after "
+ COMPILE_TIMEOUT_SECONDS + " seconds.\n" + output);
}
+ String output = Files.readString(logFile, StandardCharsets.UTF_8);
return new McsCompileResult(process.exitValue(), output);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| boolean exited = process.waitFor(COMPILE_TIMEOUT_SECONDS, TimeUnit.SECONDS); | |
| String output = Files.readString(logFile, StandardCharsets.UTF_8); | |
| if (!exited) { | |
| process.destroyForcibly(); | |
| return new McsCompileResult(124, "Minecraft-Script compile timed out after " | |
| + COMPILE_TIMEOUT_SECONDS + " seconds.\n" + output); | |
| } | |
| return new McsCompileResult(process.exitValue(), output); | |
| boolean exited = process.waitFor(COMPILE_TIMEOUT_SECONDS, TimeUnit.SECONDS); | |
| if (!exited) { | |
| process.destroyForcibly(); | |
| String output = Files.readString(logFile, StandardCharsets.UTF_8); | |
| return new McsCompileResult(124, "Minecraft-Script compile timed out after " | |
| COMPILE_TIMEOUT_SECONDS + " seconds.\n" + output); | |
| } | |
| String output = Files.readString(logFile, StandardCharsets.UTF_8); | |
| return new McsCompileResult(process.exitValue(), output); |
🤖 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/ProcessMcsCompiler.java`
around lines 38 - 45, The code reads the logFile before guaranteeing the process
has stopped, causing a race; in ProcessMcsCompiler change the order so you only
call Files.readString(logFile, StandardCharsets.UTF_8) after the process is
confirmed terminated: keep the initial boolean exited =
process.waitFor(COMPILE_TIMEOUT_SECONDS, TimeUnit.SECONDS), and if !exited first
call process.destroyForcibly() and then wait for termination (e.g.,
process.waitFor() with no timeout or re-wait) before reading logFile, then
construct the McsCompileResult with exit code 124 and the read output; similarly
when exited is true read the log and return new
McsCompileResult(process.exitValue(), output).
Comment on lines
+29
to
+33
| List<String> packNames = manager.discoverPackFolders().stream() | ||
| .map(path -> path.getFileName().toString()) | ||
| .toList(); | ||
|
|
||
| assertEquals(List.of("custom", "starter"), packNames); |
There was a problem hiding this comment.
Avoid order-dependent assertion for discovered pack folders.
This test assumes deterministic traversal order (["custom", "starter"]). Discovery order can vary by filesystem/runtime, causing flaky CI.
Suggested fix
- assertEquals(List.of("custom", "starter"), packNames);
+ assertEquals(List.of("custom", "starter"), packNames.stream().sorted().toList());🤖 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/test/java/dev/spyc0der/minecraftscript/McsWorldPackManagerTest.java`
around lines 29 - 33, Test is flaky because it asserts a specific order for
manager.discoverPackFolders() results; change McsWorldPackManagerTest to perform
an order-independent check by either sorting the discovered names before
comparison or comparing as unordered collections (e.g., convert packNames and
expected list to Sets or use an assert that ignores order) so the assertion no
longer depends on filesystem traversal order from discoverPackFolders().
| @@ -0,0 +1,16 @@ | |||
| modLoader = "javafml" | |||
| loaderVersion = "[1,)" | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check Forge and NeoForge loader versions declared in the root build.gradle
rg -n "forge.*Version|loaderVersion" mod/build.gradle mod/gradle.propertiesRepository: SpyC0der77/Minecraft-Script
Length of output: 445
Constrain Forge loaderVersion range to the supported Forge/FML versions used in the build matrix.
mod/platform/forge/src/main/resources/META-INF/mods.toml sets loaderVersion = "[1,)", which allows any FML version ≥ 1 with no upper bound; since mod/build.gradle pins Forge dependency versions via ext.forgeVersions (and indexes it by minecraftVersion), mods.toml should be updated to start at the minimum supported Forge/FML version from that map and ideally add an upper bound aligned with what you test.
🤖 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/main/resources/META-INF/mods.toml` at line 2, The
mods.toml currently sets loaderVersion = "[1,)" which is too permissive; update
the loaderVersion range to the minimum supported Forge/FML version from your
ext.forgeVersions map and add an upper bound that matches the top of your tested
build matrix (e.g. use a range like "[MIN_SUPPORTED,MAX_TESTED)"); change the
loaderVersion value in mods.toml accordingly and keep it in sync with the
versions indexed by minecraftVersion in build.gradle (ext.forgeVersions).
- Enhanced the GitHub Actions CI workflow to include detailed logging and failure summaries for build matrix jobs. - Updated the `.gitignore` to exclude build directories. - Refactored project settings to dynamically manage supported Minecraft versions and loaders, removing unsupported versions. - Added version-specific build.gradle files for Minecraft versions 1.21.2 to 1.21.11, ensuring proper loader configuration. - Removed obsolete build.gradle files for unsupported versions.
- Updated the McsPackWatcher class to enhance the handling of file changes with new methods for polling and remembering pack modification times. - Introduced a scheduler for periodic polling of pack modification times and adjusted compile and reload debounce timings. - Added a notification method in McsWorldPackManager to report pending pack changes. - Improved thread management by separating the scheduler and compile executor for better performance and clarity.
There was a problem hiding this comment.
1 issue found across 63 files (changes from recent commits).
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="mod/gradle/wrapper/gradle-wrapper.properties">
<violation number="1" location="mod/gradle/wrapper/gradle-wrapper.properties:3">
P2: Gradle wrapper distribution is not checksum-pinned (`distributionSha256Sum` missing), reducing protection against tampered distribution downloads.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Partial review: This PR has more than 50 files, so cubic reviewed the highest-priority files first. During the trial, paid plans get a higher file limit.
You can try an ultrareview to bypass the file limit, comment @cubic-dev-ai ultrareview. Learn more.
Fix all with cubic | Re-trigger cubic
| @@ -0,0 +1,7 @@ | |||
| distributionBase=GRADLE_USER_HOME | |||
| distributionPath=wrapper/dists | |||
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.10.2-bin.zip | |||
There was a problem hiding this comment.
P2: Gradle wrapper distribution is not checksum-pinned (distributionSha256Sum missing), reducing protection against tampered distribution downloads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mod/gradle/wrapper/gradle-wrapper.properties, line 3:
<comment>Gradle wrapper distribution is not checksum-pinned (`distributionSha256Sum` missing), reducing protection against tampered distribution downloads.</comment>
<file context>
@@ -0,0 +1,7 @@
+distributionBase=GRADLE_USER_HOME
+distributionPath=wrapper/dists
+distributionUrl=https\://services.gradle.org/distributions/gradle-8.10.2-bin.zip
+networkTimeout=10000
+validateDistributionUrl=true
</file context>
- Updated loader.gradle to dynamically set platformJavaSuffix based on Minecraft version, including support for versions 1.21.9 and 1.21.10. - Removed the McsForgeMod class, which was responsible for handling server events in the Forge environment, as part of a cleanup effort.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mod/gradlew.bat (1)
1-95:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winConvert to Windows (CRLF) line endings.
This
.batfile uses Unix (LF) line endings, which can cause Windows batch parser failures, especially with GOTO and CALL statements at 512-byte boundaries. Windows batch scripts require CRLF line endings to function reliably.🔧 Fix options
Option 1: Convert file locally
dos2unix -n mod/gradlew.bat mod/gradlew.bat # or use your editor's "Convert to CRLF" functionOption 2: Configure Git to handle line endings automatically
Add to.gitattributes:*.bat text eol=crlfThen re-checkout the file:
git rm --cached mod/gradlew.bat git reset mod/gradlew.bat🤖 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/gradlew.bat` around lines 1 - 95, The gradlew.bat script is currently using LF line endings which can break Windows batch parsing (affecting labels like :findJavaFromJavaHome, :execute, :fail, :mainEnd and commands such as GOTO/exit); convert mod/gradlew.bat to CRLF line endings (e.g., via your editor's "Convert to CRLF" or a dos2unix/dos2unix -n reversal) and ensure Git preserves .bat CRLFs by adding a .gitattributes entry for *.bat text eol=crlf and re-checking the file into the branch so the labels and control flow (setlocal/endlocal, GOTO targets) parse correctly on Windows.mod/common/src/main/java/dev/spyc0der/minecraftscript/McsPackWatcher.java (1)
243-250:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShutdown race can throw uncaught RejectedExecutionException.
The
close()method shuts downschedulerandcompileExecutorbefore closing thewatchService. IfwatchLoop(line 82–127) orpollPackMtimes(line 180–204) is mid-execution, it may callschedulePackRefreshorscheduleFullScanafter the executors are shut down, throwingRejectedExecutionException(aRuntimeExceptionnot covered by theIOExceptionsignature).Impact: Unhandled exception propagates up the
watcherThreador scheduler thread, leading to error logs or thread termination during shutdown.Recommended fix: reorder shutdown sequence
Close the
watchServicefirst to interrupt thewatchLoop, then ensure no tasks are in flight before shutting down executors:`@Override` public void close() throws IOException { running = false; + watchService.close(); + try { + watcherThread.join(1000); + } catch (InterruptedException ignored) { + Thread.currentThread().interrupt(); + } for (ScheduledFuture<?> future : new HashMap<>(pendingCompiles).values()) { future.cancel(false); } scheduler.shutdownNow(); compileExecutor.shutdownNow(); - watchService.close(); }This stops the watch loop before shutting down executors, eliminating the race window.
🤖 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/McsPackWatcher.java` around lines 243 - 250, The shutdown sequence in close() can race with watchLoop and pollPackMtimes which may call schedulePackRefresh/scheduleFullScan after executors are shut down; reorder the teardown in McsPackWatcher.close(): close watchService first to interrupt the watchLoop, set running = false, cancel pendingCompiles futures, then shutdown scheduler and compileExecutor (and optionally await termination or call shutdownNow), and ensure schedulePackRefresh/scheduleFullScan either catch RejectedExecutionException or check running before submitting to scheduler/compileExecutor to avoid uncaught runtime exceptions.
♻️ Duplicate comments (1)
.github/workflows/mod-ci.yml (1)
20-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin all
uses:actions to immutable SHAs.These steps still use floating tags (
@v4), so workflow behavior can change upstream unexpectedly. Replace each with a full commit SHA.#!/bin/bash # Verify unpinned GitHub Actions references in workflow files. rg -nP '^\s*uses:\s*[^@]+@v[0-9]+(\s|$)' .github/workflowsAlso applies to: 23-23, 29-29, 40-40, 112-112, 115-115, 121-121, 170-170
🤖 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 @.github/workflows/mod-ci.yml at line 20, Replace the floating GitHub Actions tags with immutable commit SHAs: find each occurrence of "uses: actions/checkout@v4" and other "uses: ...@vN" entries in the workflow (e.g., the occurrences flagged near the top and repeated entries) and update them to the corresponding full commit SHA for that action release; ensure you pin every "uses:" reference (not just checkout) to a specific SHA, verify the SHAs point to the intended commit on the action's repository, and run the provided rg pattern to confirm no unpinned `@vN` usages remain.
🧹 Nitpick comments (3)
mod/build.gradle (1)
108-110: ⚡ Quick winConsolidate Forge exclusion logic to avoid duplication.
The Forge version exclusion is defined in two places:
- Here:
forgeVersions[mcVersion] == null(checking the map defined at line 34)settings.gradleline 23:unsupportedForgeVersions.contains(mcVersion)(hardcoded list at line 19)When a future Minecraft version lacks Forge support, both locations must be updated. Consider deriving
unsupportedForgeVersionsin settings.gradle from the parsed forgeVersions map, or vice versa, to maintain a single source of truth.🤖 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/build.gradle` around lines 108 - 110, Currently Forge exclusion is duplicated: mod/build.gradle checks forgeVersions[mcVersion] == null (symbols: loader, forgeVersions, mcVersion) while settings.gradle uses unsupportedForgeVersions.contains(mcVersion). Fix by consolidating to a single source of truth — parse/produce the forgeVersions map once (or generate unsupportedForgeVersions from it) and have the other build script reference that derived set; update the logic in mod/build.gradle to use the unified check (e.g., consult the derived unsupportedForgeVersions or the canonical forgeVersions) and remove the hardcoded list so future MC versions require only one update.mod/platform/fabric/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/fabric/FabricServerAccess.java (1)
42-49: 💤 Low valueDuplicate
getPlayerList()inefficiency spans all platform adapters.All five
ServerAccessimplementations (Fabric, Forge, NeoForge across version variants) repeat theserver.getPlayerList()call for each player innotifyOperators(). The shared root cause is duplicated implementation across platform adapters. Consider extracting this logic to a shared helper or base class to eliminate the redundancy and centralize the optimization.Refactor: extract to shared utility
Add a common helper in the
commonmodule:// mod/common/.../McsServerAccess.java default void notifyOperatorsImpl(MinecraftServer server, String message) { server.execute(() -> { var playerList = server.getPlayerList(); playerList.getPlayers().forEach(player -> { if (playerList.isOp(new NameAndId(player.getGameProfile()))) { player.sendSystemMessage(Component.literal("[MCS] " + message).withStyle(ChatFormatting.YELLOW)); } }); }); }Then delegate from each platform adapter:
`@Override` public void notifyOperators(String message) { notifyOperatorsImpl(server, message); }🤖 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/fabric/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/fabric/FabricServerAccess.java` around lines 42 - 49, The notifyOperators implementation in FabricServerAccess (and the other ServerAccess adapters) repeatedly calls server.getPlayerList() for each player and duplicates logic across platforms; extract the loop into a shared helper (e.g., a default method notifyOperatorsImpl(MinecraftServer server, String message) placed in the common module) that captures playerList once, iterates players, checks playerList.isOp(new NameAndId(player.getGameProfile())), and sends the formatted Component message, then change FabricServerAccess.notifyOperators to delegate to notifyOperatorsImpl(server, message); ensure the helper uses server.execute(...) same as the adapters so behavior remains identical.mod/common/src/main/java/dev/spyc0der/minecraftscript/McsPackWatcher.java (1)
137-158: ⚖️ Poor tradeoffRace condition in schedulePackRefresh allows duplicate futures.
When
schedulePackRefreshis called concurrently for the same normalized pack folder, the remove-schedule-put sequence is not atomic. Two threads can each remove the existing future (both seenull), schedule separate futures, and put them intopendingCompiles, leaving only the last one tracked. The earlier future remains scheduled but uncancellable.Impact: Duplicate pack compilations (wasteful but safe due to single-threaded
compileExecutor) and a dangling future that won't be cancelled on subsequent changes or duringclose().Recommended fix using atomic compute
Replace the remove-schedule-put pattern with an atomic compute operation:
private void schedulePackRefresh(Path packFolder) { Path normalized = packFolder.toAbsolutePath().normalize(); long now = System.currentTimeMillis(); if (firstChangeAt.putIfAbsent(normalized, now) == null) { packManager.notifyPackChangePending(packFolder); } - ScheduledFuture<?> existing = pendingCompiles.remove(normalized); - if (existing != null) { - existing.cancel(false); - } - long delay = computeCompileDelayMillis(firstChangeAt.get(normalized), now); - ScheduledFuture<?> future = scheduler.schedule(() -> compileExecutor.execute(() -> { + ScheduledFuture<?> newFuture = scheduler.schedule(() -> compileExecutor.execute(() -> { pendingCompiles.remove(normalized); firstChangeAt.remove(normalized); if (packManager.refreshPack(normalized)) { scheduleReload(); } }), delay, TimeUnit.MILLISECONDS); - pendingCompiles.put(normalized, future); + + ScheduledFuture<?> previous = pendingCompiles.put(normalized, newFuture); + if (previous != null) { + previous.cancel(false); + } }This ensures the new future is tracked before the old one is cancelled, preventing a window where no future is tracked.
🤖 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/McsPackWatcher.java` around lines 137 - 158, schedulePackRefresh has a race between removing, scheduling and putting futures into pendingCompiles causing duplicate uncancellable futures; fix it by replacing the remove/schedule/put sequence with an atomic pendingCompiles.compute(normalized, ...) call that schedules the new ScheduledFuture inside the compute lambda (calling scheduler.schedule(... -> compileExecutor.execute(...))) and returns that future, and then cancels the existing future (existing.cancel(false)) inside the same lambda if non-null; keep the existing compileExecutor.execute task body (pendingCompiles.remove(normalized), firstChangeAt.remove(normalized), packManager.refreshPack(...) and scheduleReload()) unchanged so the new future is installed atomically and the old one is cancelled safely.
🤖 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 @.github/workflows/mod-ci.yml:
- Around line 137-138: The pipeline currently exits on a failing Gradle command
before the echo "log_file=${LOG_FILE}" runs; change the step around the gradlew
pipeline to capture the Gradle exit code from the pipeline (e.g., use bash's
PIPESTATUS[0] after ./gradlew ... | tee "${LOG_FILE}" to get the gradle exit
code), always emit echo "log_file=${LOG_FILE}" to GITHUB_OUTPUT, then exit the
step with the captured code (exit $EXIT_CODE) so steps.build.outputs.log_file is
set even when ./gradlew fails; ensure you reference the existing LOG_FILE
variable and preserve the original exit status.
---
Outside diff comments:
In `@mod/common/src/main/java/dev/spyc0der/minecraftscript/McsPackWatcher.java`:
- Around line 243-250: The shutdown sequence in close() can race with watchLoop
and pollPackMtimes which may call schedulePackRefresh/scheduleFullScan after
executors are shut down; reorder the teardown in McsPackWatcher.close(): close
watchService first to interrupt the watchLoop, set running = false, cancel
pendingCompiles futures, then shutdown scheduler and compileExecutor (and
optionally await termination or call shutdownNow), and ensure
schedulePackRefresh/scheduleFullScan either catch RejectedExecutionException or
check running before submitting to scheduler/compileExecutor to avoid uncaught
runtime exceptions.
In `@mod/gradlew.bat`:
- Around line 1-95: The gradlew.bat script is currently using LF line endings
which can break Windows batch parsing (affecting labels like
:findJavaFromJavaHome, :execute, :fail, :mainEnd and commands such as
GOTO/exit); convert mod/gradlew.bat to CRLF line endings (e.g., via your
editor's "Convert to CRLF" or a dos2unix/dos2unix -n reversal) and ensure Git
preserves .bat CRLFs by adding a .gitattributes entry for *.bat text eol=crlf
and re-checking the file into the branch so the labels and control flow
(setlocal/endlocal, GOTO targets) parse correctly on Windows.
---
Duplicate comments:
In @.github/workflows/mod-ci.yml:
- Line 20: Replace the floating GitHub Actions tags with immutable commit SHAs:
find each occurrence of "uses: actions/checkout@v4" and other "uses: ...@vN"
entries in the workflow (e.g., the occurrences flagged near the top and repeated
entries) and update them to the corresponding full commit SHA for that action
release; ensure you pin every "uses:" reference (not just checkout) to a
specific SHA, verify the SHAs point to the intended commit on the action's
repository, and run the provided rg pattern to confirm no unpinned `@vN` usages
remain.
---
Nitpick comments:
In `@mod/build.gradle`:
- Around line 108-110: Currently Forge exclusion is duplicated: mod/build.gradle
checks forgeVersions[mcVersion] == null (symbols: loader, forgeVersions,
mcVersion) while settings.gradle uses
unsupportedForgeVersions.contains(mcVersion). Fix by consolidating to a single
source of truth — parse/produce the forgeVersions map once (or generate
unsupportedForgeVersions from it) and have the other build script reference that
derived set; update the logic in mod/build.gradle to use the unified check
(e.g., consult the derived unsupportedForgeVersions or the canonical
forgeVersions) and remove the hardcoded list so future MC versions require only
one update.
In `@mod/common/src/main/java/dev/spyc0der/minecraftscript/McsPackWatcher.java`:
- Around line 137-158: schedulePackRefresh has a race between removing,
scheduling and putting futures into pendingCompiles causing duplicate
uncancellable futures; fix it by replacing the remove/schedule/put sequence with
an atomic pendingCompiles.compute(normalized, ...) call that schedules the new
ScheduledFuture inside the compute lambda (calling scheduler.schedule(... ->
compileExecutor.execute(...))) and returns that future, and then cancels the
existing future (existing.cancel(false)) inside the same lambda if non-null;
keep the existing compileExecutor.execute task body
(pendingCompiles.remove(normalized), firstChangeAt.remove(normalized),
packManager.refreshPack(...) and scheduleReload()) unchanged so the new future
is installed atomically and the old one is cancelled safely.
In
`@mod/platform/fabric/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/fabric/FabricServerAccess.java`:
- Around line 42-49: The notifyOperators implementation in FabricServerAccess
(and the other ServerAccess adapters) repeatedly calls server.getPlayerList()
for each player and duplicates logic across platforms; extract the loop into a
shared helper (e.g., a default method notifyOperatorsImpl(MinecraftServer
server, String message) placed in the common module) that captures playerList
once, iterates players, checks playerList.isOp(new
NameAndId(player.getGameProfile())), and sends the formatted Component message,
then change FabricServerAccess.notifyOperators to delegate to
notifyOperatorsImpl(server, message); ensure the helper uses server.execute(...)
same as the adapters so behavior remains identical.
🪄 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: 1fc59a12-a0d2-4d8b-90a7-85228b66f6cb
⛔ Files ignored due to path filters (1)
mod/gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (66)
.github/workflows/mod-ci.yml.gitignoremod/build.gradlemod/common/src/main/java/dev/spyc0der/minecraftscript/McsPackWatcher.javamod/common/src/main/java/dev/spyc0der/minecraftscript/McsWorldPackManager.javamod/common/src/test/java/dev/spyc0der/minecraftscript/McsPackWatcherTest.javamod/gradle/wrapper/gradle-wrapper.propertiesmod/gradlewmod/gradlew.batmod/loader.gradlemod/matrix-project.gradlemod/platform/fabric/src/mc_1_21_11/java/dev/spyc0der/minecraftscript/fabric/FabricServerAccess.javamod/platform/fabric/src/mc_1_21_9/java/dev/spyc0der/minecraftscript/fabric/FabricServerAccess.javamod/platform/fabric/src/mc_legacy/java/dev/spyc0der/minecraftscript/fabric/FabricServerAccess.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_9/java/dev/spyc0der/minecraftscript/forge/ForgeServerAccess.javamod/platform/forge/src/mc_legacy/java/dev/spyc0der/minecraftscript/forge/ForgeServerAccess.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/NeoForgeServerAccess.javamod/platform/neoforge/src/mc_legacy/java/dev/spyc0der/minecraftscript/neoforge/NeoForgeServerAccess.javamod/settings.gradlemod/versions/mc_1_21_10/fabric/build.gradlemod/versions/mc_1_21_10/forge/build.gradlemod/versions/mc_1_21_10/forge/gradle.propertiesmod/versions/mc_1_21_10/neoforge/build.gradlemod/versions/mc_1_21_10/neoforge/gradle.propertiesmod/versions/mc_1_21_11/fabric/build.gradlemod/versions/mc_1_21_11/forge/build.gradlemod/versions/mc_1_21_11/forge/gradle.propertiesmod/versions/mc_1_21_11/neoforge/build.gradlemod/versions/mc_1_21_11/neoforge/gradle.propertiesmod/versions/mc_1_21_2/fabric/build.gradlemod/versions/mc_1_21_2/neoforge/build.gradlemod/versions/mc_1_21_2/neoforge/gradle.propertiesmod/versions/mc_1_21_4/fabric/build.gradlemod/versions/mc_1_21_4/forge/build.gradlemod/versions/mc_1_21_4/forge/gradle.propertiesmod/versions/mc_1_21_4/neoforge/build.gradlemod/versions/mc_1_21_4/neoforge/gradle.propertiesmod/versions/mc_1_21_5/fabric/build.gradlemod/versions/mc_1_21_5/forge/build.gradlemod/versions/mc_1_21_5/forge/gradle.propertiesmod/versions/mc_1_21_5/neoforge/build.gradlemod/versions/mc_1_21_5/neoforge/gradle.propertiesmod/versions/mc_1_21_6/fabric/build.gradlemod/versions/mc_1_21_6/forge/build.gradlemod/versions/mc_1_21_6/forge/gradle.propertiesmod/versions/mc_1_21_6/neoforge/build.gradlemod/versions/mc_1_21_6/neoforge/gradle.propertiesmod/versions/mc_1_21_7/fabric/build.gradlemod/versions/mc_1_21_7/forge/build.gradlemod/versions/mc_1_21_7/forge/gradle.propertiesmod/versions/mc_1_21_7/neoforge/build.gradlemod/versions/mc_1_21_7/neoforge/gradle.propertiesmod/versions/mc_1_21_8/fabric/build.gradlemod/versions/mc_1_21_8/forge/build.gradlemod/versions/mc_1_21_8/forge/gradle.propertiesmod/versions/mc_1_21_8/neoforge/build.gradlemod/versions/mc_1_21_8/neoforge/gradle.propertiesmod/versions/mc_1_21_9/fabric/build.gradlemod/versions/mc_1_21_9/forge/build.gradlemod/versions/mc_1_21_9/forge/gradle.propertiesmod/versions/mc_1_21_9/neoforge/build.gradlemod/versions/mc_1_21_9/neoforge/gradle.properties
💤 Files with no reviewable changes (4)
- mod/platform/fabric/src/mc_legacy/java/dev/spyc0der/minecraftscript/fabric/FabricServerAccess.java
- mod/platform/forge/src/forge_legacy/java/dev/spyc0der/minecraftscript/forge/McsForgeMod.java
- mod/platform/forge/src/mc_legacy/java/dev/spyc0der/minecraftscript/forge/ForgeServerAccess.java
- mod/platform/neoforge/src/mc_legacy/java/dev/spyc0der/minecraftscript/neoforge/NeoForgeServerAccess.java
✅ Files skipped from review due to trivial changes (14)
- mod/versions/mc_1_21_4/forge/gradle.properties
- mod/matrix-project.gradle
- mod/versions/mc_1_21_10/neoforge/gradle.properties
- mod/versions/mc_1_21_9/neoforge/gradle.properties
- mod/versions/mc_1_21_11/forge/gradle.properties
- mod/gradle/wrapper/gradle-wrapper.properties
- mod/versions/mc_1_21_4/neoforge/gradle.properties
- mod/versions/mc_1_21_8/forge/gradle.properties
- mod/versions/mc_1_21_5/neoforge/gradle.properties
- .gitignore
- mod/versions/mc_1_21_10/forge/gradle.properties
- mod/versions/mc_1_21_6/neoforge/gradle.properties
- mod/versions/mc_1_21_8/neoforge/gradle.properties
- mod/gradlew
🚧 Files skipped from review as they are similar to previous changes (3)
- mod/versions/mc_1_21_4/fabric/build.gradle
- mod/versions/mc_1_21_8/neoforge/build.gradle
- mod/common/src/main/java/dev/spyc0der/minecraftscript/McsWorldPackManager.java
Comment on lines
+137
to
+138
| ./gradlew "${TASK}" --stacktrace --info 2>&1 | tee "${LOG_FILE}" | ||
| echo "log_file=${LOG_FILE}" >> "${GITHUB_OUTPUT}" |
There was a problem hiding this comment.
Preserve log_file output even when Gradle fails.
With pipefail, a failing Gradle command exits before Line 138, so steps.build.outputs.log_file is empty in the failure-summary step and you lose inline log excerpts.
Suggested fix
- name: Build matrix project
id: build
@@
run: |
chmod +x ./gradlew
set -o pipefail
VERSION_KEY="${MINECRAFT_VERSION//./_}"
TASK=":versions:mc_${VERSION_KEY}:${LOADER}:build"
LOG_FILE="build-${MINECRAFT_VERSION}-${LOADER}.log"
+ echo "log_file=${LOG_FILE}" >> "${GITHUB_OUTPUT}"
echo "Building ${TASK}"
- ./gradlew "${TASK}" --stacktrace --info 2>&1 | tee "${LOG_FILE}"
- echo "log_file=${LOG_FILE}" >> "${GITHUB_OUTPUT}"
+ set +e
+ ./gradlew "${TASK}" --stacktrace --info 2>&1 | tee "${LOG_FILE}"
+ status=$?
+ set -e
+ exit "${status}"Also applies to: 146-146
🤖 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 @.github/workflows/mod-ci.yml around lines 137 - 138, The pipeline currently
exits on a failing Gradle command before the echo "log_file=${LOG_FILE}" runs;
change the step around the gradlew pipeline to capture the Gradle exit code from
the pipeline (e.g., use bash's PIPESTATUS[0] after ./gradlew ... | tee
"${LOG_FILE}" to get the gradle exit code), always emit echo
"log_file=${LOG_FILE}" to GITHUB_OUTPUT, then exit the step with the captured
code (exit $EXIT_CODE) so steps.build.outputs.log_file is set even when
./gradlew fails; ensure you reference the existing LOG_FILE variable and
preserve the original exit status.
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.
Summary by CodeRabbit
New Features
Tests
Chores