Update project configuration and enhance documentation#16
Closed
SpyC0der77 wants to merge 12 commits into
Closed
Conversation
- Added new entries to `.gitignore` for Gradle build artifacts and node modules. - Expanded the `todo.md` file to include detailed implementation plans for the MCS Hot-Reload Mod, covering core functionality, project phases, and configuration management. - Introduced a context manager in `config_utils.py` for temporary configuration overrides. - Enhanced argument parsing in `shell_commands.py` to support new flags for the compile command, including `--mc-version` and `--force`. - Updated the `CompileInterpreter` to conditionally track built-in function usage based on logging style. - Minor adjustments to built-in log function templates for consistency. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
More reviews will be available in 9 minutes and 35 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThis PR implements a full Minecraft pack hot-reload runtime mod and extends the MCS compiler CLI with configurable flags. The mod watches a ChangesMCS Packs Mod Feature
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
- Introduced a new `gradlew` script to facilitate Gradle project execution in POSIX-compliant shells. - The script includes necessary configurations for Java command execution and environment variable handling. - Ensured compatibility with various operating systems, including Cygwin, Darwin, and MinGW. This addition streamlines the build process for users in POSIX environments.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
28 issues found across 59 files
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
- Introduced a conditional source set in `build.gradle` to switch between modern and legacy Java directories based on Minecraft version. - Updated `DatapackDeployer.java` to replace direct permission handling with a new command source method for improved clarity and maintainability.
- Added Bun dependency installation step in the GitHub Actions workflow for improved JavaScript package management. - Introduced a validation function in `shell_commands.py` to ensure valid datapack names, enhancing error handling. - Updated `build.gradle` to conditionally set Minecraft version and supported game versions based on CLI input. - Enhanced Gradle properties to support multiple platforms (fabric, forge, neoforge) for better compatibility. - Refactored `apply_version.py` to dynamically read enabled platforms from `gradle.properties`. - Improved error handling in `ModConfigManager` and `PackRegistry` for better resilience against invalid configurations. - Updated `mcs-spyglass-validate` script to streamline datapack validation and improve error reporting. - Removed deprecated `mcs-versions.json` and `mcs-spyglass-validate.mjs` files, consolidating functionality into updated scripts.oo
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
Owner
Author
|
@cubic-dev-ai review this |
@SpyC0der77 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
12 issues found across 61 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/common/src/main/resources/starter/pack.mcs">
<violation number="1" location="mod/common/src/main/resources/starter/pack.mcs:1">
P3: This file duplicates `examples/starter_datapack.mcs` verbatim, creating a maintainability risk from having two copies of the same starter script.</violation>
</file>
<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; add `distributionSha256Sum` to verify downloaded binaries.</violation>
</file>
<file name="mod/settings.gradle">
<violation number="1" location="mod/settings.gradle:5">
P2: Use the current Forge Maven repository URL in pluginManagement; the configured `files.minecraftforge.net` endpoint is legacy.</violation>
</file>
<file name=".github/workflows/mod.yml">
<violation number="1" location=".github/workflows/mod.yml:4">
P2: Workflow path filters exclude the workflow file itself, so workflow-only changes won't be validated by this CI job.</violation>
</file>
<file name="mod/versions/manifest.json">
<violation number="1" location="mod/versions/manifest.json:80">
P1: The `26.1` profile uses a Forge version (`60.0.0`) that targets Minecraft 1.21.10, so Forge builds for this profile will resolve the wrong artifact coordinate.</violation>
</file>
<file name="mod/neoforge/src/main/resources/META-INF/neoforge.mods.toml">
<violation number="1" location="mod/neoforge/src/main/resources/META-INF/neoforge.mods.toml:2">
P1: `loaderVersion` is set too high for NeoForge and can prevent the mod from loading on otherwise supported versions.</violation>
</file>
<file name="mod/common/src/main/resources/scripts/mcs-compile.cmd">
<violation number="1" location="mod/common/src/main/resources/scripts/mcs-compile.cmd:5">
P2: The wrapper prioritizes hardcoded Python paths before `mcs`, which can cause false failures when Python exists but `minecraft_script` is not installed.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der77/mcspacks/version/VersionMapper.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der77/mcspacks/version/VersionMapper.java:33">
P2: Unused parameter `gameVersion`: the method ignores it and calls `Platform.getMinecraftVersion()` directly. This creates a hidden static dependency that makes the method harder to test and the parameter misleading. Use the `gameVersion` parameter instead of the static call.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der77/mcspacks/util/SafePaths.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der77/mcspacks/util/SafePaths.java:14">
P1: `validateSafeName` permits `"."`, which resolves to the parent directory and can trigger recursive deletion of the root target directory.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/CompilerResolver.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/CompilerResolver.java:146">
P2: Batch command quoting does not escape `%`, so `cmd.exe` expands `%VAR%` inside quoted args and can corrupt compiler arguments.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/McsToolchain.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/McsToolchain.java:115">
P2: `relative()` assumes paths are always relativizable; with absolute `entryFile` overrides it can throw and break diagnostic reporting.</violation>
</file>
<file name="mod/common/build.gradle">
<violation number="1" location="mod/common/build.gradle:55">
P2: The build step invokes `bun` unconditionally, introducing an undocumented hard dependency that can break `processResources` on Node-only environments.</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
- Updated GitHub Actions workflow to pass the loader argument to the version application script, allowing for targeted platform builds. - Modified `settings.gradle` to dynamically set enabled platforms based on the provided loader or default to multiple platforms. - Enhanced `apply_version.py` to conditionally set enabled platforms based on the loader argument, improving flexibility in build configurations. - Refactored `build_all.py` to incorporate the loader argument in the build command, streamlining the build process for specific platforms.
There was a problem hiding this comment.
2 issues found across 15 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/common/src/main/resources/scripts/mcs-compile.cmd">
<violation number="1" location="mod/common/src/main/resources/scripts/mcs-compile.cmd:19">
P2: Interpreter resolution order now prefers arbitrary PATH Python over known compatible `C:\Python310+` installs, which can select an unsupported interpreter.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Code Review SummaryStatus: No New Issues Found | Recommendation: Review cubic comments before merge This PR has been partially reviewed by cubic. The existing cubic review comments cover the following key issues:
The new files added in this PR ( Please address the cubic review issues before merging. Files Reviewed (5 files with changes)
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (positional.length < 1) { | ||
| throw new Error('Usage: node mcs-spyglass-validate.js [--json] [--mc-version <version>] <datapack_dir>') |
There was a problem hiding this comment.
SUGGESTION: Usage message references wrong file extension. The usage message says mcs-spyglass-validate.js but the source file is mcs-spyglass-validate.mjs. Should reference the actual filename for consistency.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Carried Forward from Previous ReviewIssues from cubic on unchanged files:
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Reviewed by laguna-m.1-20260312:free · 765,433 tokens |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
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 | 🟠 Major | ⚡ Quick winNormalize this batch wrapper to CRLF line endings.
This file is a Windows
.batentrypoint; LF-only endings can cause parser/label issues in some Windows environments. Convert to CRLF to keep wrapper behavior reliable.🤖 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, Convert this batch wrapper to CRLF line endings (currently LF-only) so Windows cmd labels and parsing work reliably: open the gradlew.bat file and save/commit it with CRLF line endings, ensuring you do not alter any labels or logic (refer to labels like :execute, :fail, :mainEnd to verify content integrity), and add or update repository settings to enforce CRLF for .bat files (e.g., add a .gitattributes entry like "*.bat text eol=crlf" or set core.autocrlf appropriately) so future commits preserve CRLF.Source: Linters/SAST tools
mod/common/src/main/resources/scripts/mcs-compile.cmd (1)
1-62:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBatch file uses Unix line endings (LF) instead of Windows line endings (CRLF).
Windows batch files require CRLF line endings. LF-only line endings can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser bugs, especially at 512-byte boundaries.
Convert this file to Windows CRLF line endings using:
dos2unix(orunix2dos)- Git:
git config core.autocrlf trueand re-checkout- Or configure
.gitattributeswith*.cmd text eol=crlf🤖 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/resources/scripts/mcs-compile.cmd` around lines 1 - 62, The batch file uses LF-only line endings which can break Windows CMD label parsing (affecting sections like :run_mcs, :run_python, :run_py and the main search/exit flow), so convert this file to CRLF line endings (e.g., run unix2dos/dos2unix -w, set git core.autocrlf=true and re-checkout, or add a .gitattributes entry like "*.cmd text eol=crlf" and re-checkout) and re-run tests to ensure GOTO/CALL labels and %ERRORLEVEL% propagation behave correctly.Source: Linters/SAST tools
🟡 Minor comments (6)
minecraft_script/shell_commands.py-44-47 (1)
44-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject unknown
--flagsinstead of treating them as positional arguments.At Line 44-Line 47, unsupported flags are appended to
positional, which can mis-route argument parsing (e.g., source path becomes"--unknown"), producing confusing failures.Suggested fix
- if arg.startswith("--"): - if arg not in supported_flags: - positional.append(arg) - index += 1 - continue + if arg.startswith("--"): + print(f"Error: Unknown flag {arg}.") + exit(-1)🤖 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 `@minecraft_script/shell_commands.py` around lines 44 - 47, The code currently treats unsupported long flags as positional arguments in the argument-parsing loop (see the handling of arg, supported_flags, positional and index), causing misrouting; change this so that when arg.startswith("--") and arg is not in supported_flags you raise/return an explicit error (e.g., raise ValueError or call parser.error) instead of appending to positional, and ensure index is still advanced; update the branch in the parser loop that checks arg.startswith("--") to validate against supported_flags and reject unknown flags with a clear message referencing the flag.tests/test_compile_cli_flags.py-69-92 (1)
69-92:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
--mc-versionmutation test does not verify persisted config state.Line 91 checks in-process
COMMON_CONFIG, butsubprocess.run(...)executes in a separate process. A bad write toconfig.jsoncould still happen and this assertion would miss it.Suggested fix
def test_compile_mc_version_does_not_mutate_config(build_output): - original_version = COMMON_CONFIG["minecraft_version"] + config_path = Path(module_folder) / "config.json" + original_version = json.loads(config_path.read_text(encoding="utf-8"))["minecraft_version"] try: result = subprocess.run( @@ ) assert result.returncode == 0, result.stderr - assert COMMON_CONFIG["minecraft_version"] == original_version + current_version = json.loads(config_path.read_text(encoding="utf-8"))["minecraft_version"] + assert current_version == original_version🤖 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 `@tests/test_compile_cli_flags.py` around lines 69 - 92, The test test_compile_mc_version_does_not_mutate_config currently only checks the in-process COMMON_CONFIG after calling subprocess.run, which won't catch writes to the on-disk config; update the test to, after the subprocess.run completes, reload the persisted config file (e.g., read and parse the same config.json the application uses) and assert that the persisted "minecraft_version" equals original_version (in addition to the existing in-memory assertion). Use the same symbols from the diff (test_compile_mc_version_does_not_mutate_config, COMMON_CONFIG, and the subprocess.run invocation) to locate where to add the file-read/parse and assertion, and ensure cleanup still happens in the finally block.todo.md-69-69 (1)
69-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix markdownlint violations in docs blocks/tables.
Line 69 and Line 105 use fenced code blocks without language labels, and Line 290 table formatting needs blank lines around the table.
Also applies to: 105-105, 290-290
🤖 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 `@todo.md` at line 69, Update the markdown to satisfy markdownlint: add language identifiers to the two fenced code blocks (the triple-backtick blocks referenced) so they become e.g. ```bash or ```json as appropriate, and ensure the table near the noted section has a blank line before and after it (insert an empty line above the table header and one below the table) to fix table formatting violations.Source: Linters/SAST tools
mod/scripts/apply_version.py-22-24 (1)
22-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve explicit
enabled_platforms=fabricinstead of resetting to all loaders.Line 22 treats
"fabric"as invalid and falls back to default, so an explicit fabric-only config is lost on rewrite.Suggested fix
- if value and value != "fabric": + if value: return value🤖 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/scripts/apply_version.py` around lines 22 - 24, The code in apply_version.py wrongly treats the string "fabric" as invalid and falls back to DEFAULT_ENABLED_PLATFORMS when rewriting enabled_platforms; update the validation around the variable value (the enabled_platforms parsing branch that checks `if value and value != "fabric": return value`) to allow "fabric" as a legitimate value (or explicitly accept any known platform names including "fabric") so that an explicit enabled_platforms="fabric" is preserved instead of being reset to DEFAULT_ENABLED_PLATFORMS; ensure the logic that returns value uses the same variable name `value` and that DEFAULT_ENABLED_PLATFORMS remains only a fallback when value is empty or truly invalid.mod/common/build.gradle-71-73 (1)
71-73:⚠️ Potential issue | 🟡 MinorPin or localize the fallback
esbuildinvocation.
mod/common/build.gradlerunsnpx --yes esbuild ...whenbunisn’t available (lines 71-73), which can download an unpinnedesbuildand weaken build reproducibility/supply-chain safety. CI already installs Spyglass viabun install --frozen-lockfile, so this likely only affects local/other environments—still, prefer a pinnedesbuildversion (frommod/scriptslock/published config) or a local-only invocation (npx --no-install ...with a preinstallednode_modules/.bin/esbuild).🤖 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/build.gradle` around lines 71 - 73, The npx fallback invocation in mod/common/build.gradle currently uses 'npx --yes esbuild ...' which can pull an unpinned esbuild; change this to either (a) invoke a pinned version by replacing the call with 'npx --yes esbuild@<version>' using the esbuild version from the mod/scripts lock/published config, or (b) force a local-only invocation by switching to 'npx --no-install esbuild' and ensure esbuild is declared/installed in the project (node_modules/.bin/esbuild) so the local binary is used; locate the invocation by searching for the commandLine entry containing 'npx', 'esbuild', and update accordingly so builds are reproducible and do not fetch an arbitrary remote esbuild.mod/scripts/mcs-spyglass-validate.mjs-30-33 (1)
30-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
--mc-versionparsing should reject missing/flag values.Current parsing accepts the next token blindly, so
--mc-version --json <dir>treats--jsonas the version and changes behavior silently.Suggested fix
if (arg === '--mc-version') { - flags.mcVersion = argv[index + 1] + const value = argv[index + 1] + if (!value || value.startsWith('--')) { + throw new Error('Expected a value after --mc-version') + } + flags.mcVersion = value index += 1 continue }🤖 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/scripts/mcs-spyglass-validate.mjs` around lines 30 - 33, The parser currently assigns the next argv token to flags.mcVersion without validation; update the argument-handling logic around the '--mc-version' branch to ensure argv[index + 1] exists and does not start with '-' (i.e., is not another flag) before setting flags.mcVersion and incrementing index; if the value is missing or looks like a flag, emit a clear error message and exit (or return an error) instead of silently accepting the wrong token. Use the existing symbols flags.mcVersion and the code block handling if (arg === '--mc-version') to locate where to add this validation.
🧹 Nitpick comments (7)
mod/common/build.gradle (1)
49-50: ⚡ Quick winAvoid generating bundled artifacts directly into
src/main/resources.Lines 49 and 73 write build output into the source tree. Move this output under
build/generated/...and add that directory as a resources source to avoid dirty working trees and accidental committed generated diffs.Also applies to: 73-74
🤖 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/build.gradle` around lines 49 - 50, The build writes generated artifacts into src/main/resources (spyglassBundledScript), which pollutes the source tree; change the output path to under build/generated (e.g. def spyglassBundledScript = file("${buildDir}/generated/resources/scripts/mcs-spyglass-validate.js") and any other similar file outputs at 73-74), and then register that directory as a resources source by adding sourceSets.main.resources.srcDir "${buildDir}/generated/resources" (or call resources.srcDir). Update the tasks that produce those files to write into the new build/generated path instead of src/main/resources so generated files stay out of VCS and the working tree.mod/common/.gradle/architectury-cache/projectID (1)
1-1: ⚡ Quick winRemove generated Architectury cache state from source control.
This file appears machine/generated and will cause noisy churn across environments; it should not be 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/.gradle/architectury-cache/projectID` at line 1, Remove the generated Architectury cache file "projectID" from version control and stop tracking similar generated files: run git rm --cached for the tracked projectID file (and commit the removal), then add the architectury cache pattern (e.g., .gradle/architectury-cache/ or architectury-cache/projectID) to .gitignore so future generated cache files are not committed; commit the updated .gitignore and verify the file no longer appears in diffs.mod/neoforge/build.gradle (1)
7-7: Profile wiring already keeps MC↔NeoForge versions consistent; downgrade the Loom “guard” request to manifest sanity-check.
-Pmcs_profiledrives bothminecraft_versionandneoforge_versionfrom the same entry inmod/versions/manifest.json(viamod/scripts/apply_version.pyandmod/build.gradle), and CI passesmcs_profile=${{ matrix.profile }}—somod/neoforge/build.gradle’sversion = rootProject.neoforge_version(line 7) andneoForge ...:${rootProject.neoforge_version}(line 25) won’t see a cross-profile/mismatched pair. The “incompatible pair” risk only exists if the manifest mapping itself is wrong; if you still want protection, add a small validation step inapply_version.py/build logic to assert the selected NeoForge version matches the selected Minecraft version for each profile.🤖 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/neoforge/build.gradle` at line 7, The current guard in mod/neoforge/build.gradle that sets version = rootProject.neoforge_version and uses neoForge ...:${rootProject.neoforge_version} is redundant because -Pmcs_profile already wires minecraft_version and neoforge_version via mod/scripts/apply_version.py; change the Loom “guard” approach to a manifest sanity-check by adding a small assertion in apply_version.py (or equivalent build logic) that verifies the selected NeoForge version matches the selected Minecraft version for the given profile, and remove/relax the cross-profile incompatibility enforcement in build.gradle (keep version = rootProject.neoforge_version for normal wiring but rely on the manifest validation to catch mismatches).Source: Pipeline failures
mod/scripts/package.json (1)
8-11: 💤 Low valueConsider pinning Spyglass dependency versions for reproducible builds.
The Spyglass dependencies use caret ranges (
^), which automatically update to newer minor and patch versions. This can lead to non-reproducible builds and potential breakage from upstream changes.For critical tooling dependencies, consider using exact versions or tilde ranges (
~) that only allow patch updates.🤖 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/scripts/package.json` around lines 8 - 11, The dependencies in package.json currently use caret ranges which allow minor/patch upgrades; update the entries for "`@spyglassmc/core`", "`@spyglassmc/java-edition`", and "`@spyglassmc/mcdoc`" to pinned versions (remove the leading ^ to use exact versions like "0.4.47", "0.3.60", "0.3.51") or switch to tilde ranges if you only want patch updates (e.g. "~0.4.47") so builds become reproducible and less susceptible to upstream minor changes.mod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/McsToolchain.java (2)
47-47: ⚡ Quick winConsider catching more specific exception types.
The code catches generic
Exceptionin three locations (lint, compile, validateWithSpyglass). While this ensures all errors become diagnostics, catching more specific types likeIOExceptionorInterruptedExceptionwould make error handling more precise and avoid masking unexpected runtime exceptions.Also applies to: 78-78, 109-109
🤖 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/spyc0der77/mcspacks/toolchain/McsToolchain.java` at line 47, In McsToolchain, replace the generic catch (Exception error) blocks in the lint, compile, and validateWithSpyglass flows with specific exception types that the called APIs throw (e.g., IOException, InterruptedException, ExecutionException, etc.) by inspecting the methods invoked inside those try blocks and catching those concrete exceptions (for example catch (IOException | InterruptedException e) { ... }) while preserving the current diagnostic creation logic; if you need a safety net, add a final catch for RuntimeException that rethrows (or logs with explicit rethrow) to avoid silently masking unexpected runtime errors.
128-133: ⚖️ Poor tradeoffLoading entire directory into memory may cause issues with large directories.
At line 130,
stream.toList()loads all directory entries into memory before iterating. For directories with many files, this could cause unnecessary memory pressure.♻️ Proposed fix to stream children directly
if (Files.isDirectory(path)) { - try (var stream = Files.list(path)) { - for (Path child : stream.toList()) { - deleteRecursive(child); - } - } + try (var stream = Files.list(path)) { + stream.forEach(child -> { + try { + deleteRecursive(child); + } catch (IOException e) { + throw new java.io.UncheckedIOException(e); + } + }); + } catch (java.io.UncheckedIOException e) { + throw e.getCause(); + } }🤖 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/spyc0der77/mcspacks/toolchain/McsToolchain.java` around lines 128 - 133, The current code calls Files.list(path) and then stream.toList(), which materializes the entire directory into memory; replace that with streaming iteration to avoid loading all entries at once — inside the try (var stream = Files.list(path)) block, iterate the stream directly (e.g., stream.forEach(...) or stream.forEachOrdered(...)) and call deleteRecursive(child) for each Path instead of using stream.toList(); keep the try-with-resources and ensure deleteRecursive is invoked for every child.mod/common/src/main/resources/scripts/mcs-compile.cmd (1)
19-24: ⚖️ Poor tradeoffHardcoded Python search paths only check
C:\Python3xx.The script only searches for Python installations at
C:\Python313throughC:\Python310. This misses common installation locations like:
- Python installed via Windows Store (in
%LOCALAPPDATA%\Microsoft\WindowsApps\)- Python in
%LOCALAPPDATA%\Programs\Python\- Python in
%PROGRAMFILES%\Python3xxWhile not critical since the script already tries
pythonandpycommands first, adding these paths would improve robustness.🤖 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/resources/scripts/mcs-compile.cmd` around lines 19 - 24, The loop that searches for Python installs (for %%P in (313 312 311 310) do ...) only checks C:\Python3xx folders; update the search to also probe common install locations and call the existing :run_python label with any found executable: check %LOCALAPPDATA%\Microsoft\WindowsApps\python.exe, %LOCALAPPDATA%\Programs\Python\Python%P%\python.exe and %PROGRAMFILES%\Python%P%\python.exe (or equivalent variant patterns) in addition to the C:\Python%P\python.exe entries so the script will find Store, per-user, and Program Files installations before/alongside exiting with the same !ERRORLEVEL! behavior.
🤖 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.yml:
- Around line 24-29: Update the GitHub Actions step entries to pin the external
actions to immutable commit SHAs and disable credential persistence for
checkout: replace the mutable tags for actions/checkout, actions/setup-java, and
oven-sh/setup-bun with their corresponding full commit SHAs (instead of `@v4/`@v2)
and add with: persist-credentials: false to the actions/checkout step (keeping
the existing java setup keys distribution and java-version intact); ensure the
keyed names actions/checkout, actions/setup-java, and oven-sh/setup-bun are the
ones you edit so the workflow references the exact SHAs and checkout no longer
persists credentials.
In
`@mod/common/src/main/java/dev/spyc0der77/mcspacks/config/ModConfigManager.java`:
- Around line 35-37: The save method in ModConfigManager currently writes
directly to McsPaths.configFile() which can leave a truncated/corrupted file on
interruption; change ModConfigManager.save(ModConfig) to write the JSON to a
temporary file in the same directory (use GSON.toJson(config) output),
fsync/close it, then atomically replace the target using Files.move(tempPath,
McsPaths.configFile(), StandardCopyOption.ATOMIC_MOVE,
StandardCopyOption.REPLACE_EXISTING); ensure the parent directory exists
(Files.createDirectories(...)) and preserve the method's IOException behavior so
callers can handle failures.
In
`@mod/common/src/main/java/dev/spyc0der77/mcspacks/deploy/DatapackDeployer.java`:
- Around line 22-26: DatapackDeployer.syncPack currently deletes the existing
target before attempting to copy from compiledPack, risking data loss if
compiledPack is missing or not a directory; update syncPack to validate the
source first (e.g., Files.exists(compiledPack) &&
Files.isDirectory(compiledPack) and optionally Files.isReadable(compiledPack))
and only proceed to deleteRecursive(target) and call copyRecursive(compiledPack,
target) when validation passes; if validation fails, throw an IOException or
return an error so the old target is preserved.
In `@mod/common/src/main/java/dev/spyc0der77/mcspacks/pipeline/PackPipeline.java`:
- Around line 79-100: Resolve the PackDefinition and increment the generation
counter at enqueue time inside handlePackChange (before executor.submit) so
stale queued tasks can be dropped: call registry.resolvePack(packFolder)
synchronously, if null return, then compute long generation =
generations.merge(pack.id(), 1L, Long::sum), then submit a task that calls
processPack(server, pack, true, generation). Change processPack signature to
accept the passed generation (remove its internal generations.merge call) and
ensure runToolchain(pack, generation) uses that parameter; update any other
callers of processPack accordingly.
In `@mod/common/src/main/java/dev/spyc0der77/mcspacks/registry/PackRegistry.java`:
- Around line 57-63: The compiled output path is using displayName which can
collide; update the PackRegistry creation so the compiled folder uses the stable
pack id instead of displayName: change the SafePaths.resolveChild call in the
PackRegistry code that constructs the PackDefinition (the call currently passing
displayName) to pass the pack's stable id (the id variable/identifier used to
create the PackDefinition) so each pack's compiledRoot subfolder is unique.
In
`@mod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/CompilerResolver.java`:
- Around line 13-15: The static cache fields cachedKey and cachedCommand in
CompilerResolver are not thread-safe; mark them volatile and guard the
read-modify-write logic that updates them with synchronization (e.g., a private
static final Object lock or synchronizing on CompilerResolver.class) inside the
resolver method so concurrent threads cannot observe stale or partially-updated
state; alternatively replace the pair with atomic holders (AtomicReference) and
perform compare-and-set updates. Ensure all accesses (reads and writes) to
cachedKey and cachedCommand use the chosen synchronization/atomic mechanism so
the cache is consistent across threads.
- Around line 119-124: The canRun method swallows interrupts from
SubprocessRunner.run; change CompilerResolver.canRun to catch
InterruptedException separately, call Thread.currentThread().interrupt() to
restore the interrupt flag, and then return false (preserving the original
failure behavior), while keeping the existing catch for other Exceptions for the
normal false return; reference the canRun method and the call to
SubprocessRunner.run(...) so you locate where to add the specific
InterruptedException handler.
In
`@mod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/SubprocessRunner.java`:
- Around line 23-27: The current waitFor call in SubprocessRunner (method using
process.waitFor(timeoutSeconds, TimeUnit.SECONDS)) can be interrupted and return
early without terminating the child process; modify the method to catch
InterruptedException, call process.destroyForcibly() and then call
process.waitFor() to ensure the child is reaped, restore the thread interrupt
status (Thread.currentThread().interrupt()), and return an appropriate
ProcessResult indicating interruption; also ensure any normal timeout path still
destroys the process and reaps it before returning.
In `@mod/common/src/main/java/dev/spyc0der77/mcspacks/util/McsPaths.java`:
- Around line 50-53: The resource lookup in McsPaths hardcodes
"/scripts/mcs-spyglass-validate.js" while the shipped asset is
"mcs-spyglass-validate.mjs", causing getResourceAsStream to return null; update
the code in McsPaths so the script filename used for scriptsDir.resolve and the
getResourceAsStream call match the actual asset name (preferably
"mcs-spyglass-validate.mjs"), or implement a small fallback that first tries
"/scripts/mcs-spyglass-validate.mjs" and if null tries
"/scripts/mcs-spyglass-validate.js" before throwing the IOException so
validation can find the bundled script.
- Around line 34-45: McsPaths.compilerWrapper() currently always extracts
mcs-compile.cmd which will be executed directly on non-Windows and fail; change
the method to detect the OS (use the same isWindows() logic as CompilerResolver
or System.getProperty("os.name") check) and choose the appropriate bundled
script resource (e.g., "/scripts/mcs-compile.cmd" for Windows and
"/scripts/mcs-compile.sh" for Unix), extract the selected resource to the
scriptsDir and return that Path, and if the expected resource is missing throw
an IOException with a clear message; alternatively, if you must only support
Windows, gate extraction by returning null or throwing when not Windows so
CompilerResolver won't try to run a .cmd on Unix.
In `@mod/common/src/main/java/dev/spyc0der77/mcspacks/util/PlayerFeedback.java`:
- Around line 12-21: PlayerFeedback currently calls
server.getPlayerList().broadcastSystemMessage(...) directly from
broadcast/diagnostics which can run off the server thread; change both
broadcast(MinecraftServer, String) and diagnostics(...) to dispatch the actual
broadcast onto the server thread by using server.submit(...) (i.e., inside
broadcast, wrap the creation of Component and the call to
server.getPlayerList().broadcastSystemMessage(...) in a server.submit runnable
so all calls from diagnostics and messenger execute on the server thread).
In `@mod/common/src/main/java/dev/spyc0der77/mcspacks/version/VersionMapper.java`:
- Around line 25-27: VersionMapper.load() currently constructs a VersionMapper
from Index without validating that Index.supported and Index.profiles are
non-null and non-empty; add checks after deserializing the Index (the Index
class and its fields supported and profiles) to validate they are present and
the expected types/structures, and if either is null or invalid throw a clear,
specific exception (e.g., IllegalStateException or a custom ConfigException)
with a message referencing /mcs-versions.json so resolveMcsProfile() (which
expects profiles.containsKey(...) and supported.contains(...)) cannot later NPE
— update the load() method to perform these validations and fail fast with that
clear error.
In `@mod/common/src/main/java/dev/spyc0der77/mcspacks/watch/PackWatcher.java`:
- Around line 149-156: The watcher currently ignores deleted files because it
skips non-existent entries and path resolution can return null for
McsPaths.DEFAULT_ENTRY, so removals never trigger schedule(packFolder); update
the loop that inspects entries (the block using Files.exists(entry),
Files.getLastModifiedTime(entry), lastModified map and schedule(packFolder)) to
detect deletions by treating a previously-known path that now does not exist as
a change (i.e., if Files.exists(entry) is false and
lastModified.containsKey(entry) then remove its key and call
schedule(packFolder)); additionally, when resolving the default entry (where
McsPaths.DEFAULT_ENTRY may be missing and the resolver returns null), fall back
to scanning remaining entries or unconditionally schedule the packFolder when
resolution returns null so deletions of the default entry still trigger an
update.
In `@mod/fabric/src/main/resources/fabric.mod.json`:
- Around line 17-18: Replace the wildcard "*" in the depends entries for
architectury and fabric-api with explicit version ranges (e.g., a minimum or
ranged constraint such as "[0,)" or your project's supported minimum like
"[<min-version>,)"); update the depends.architectury and depends.fabric-api
values in fabric.mod.json accordingly so they no longer accept any version at
runtime.
In `@mod/forge/build.gradle`:
- Line 26: The Forge build fails because mod/forge/build.gradle always requests
dev.architectury:architectury-forge:${rootProject.architectury_api_version} even
for versions (13.0.8) where that artifact doesn't exist; update the Forge build
to choose the correct Architectury artifact for Forge (e.g., use
dev.architectury:architectury-neoforge:${rootProject.architectury_api_version}
or map 13.0.8 -> architectury-neoforge) instead of architectury-forge; modify
the statement referenced by modImplementation
"dev.architectury:architectury-forge:${rootProject.architectury_api_version}" to
conditionally select the appropriate artifact (or add a small mapping for
architectury_api_version == "13.0.8") so CI Forge matrix builds resolve
successfully.
In `@mod/forge/src/main/resources/META-INF/mods.toml`:
- Line 2: The loaderVersion and dependency versionRange entries are open-ended;
update the loaderVersion key and the dependency versionRange values (the strings
currently set to "[4,)" and "[${forge_version},)", "[${minecraft_version},)",
"[0,)") to include an exclusive upper bound that matches the next incompatible
version you’ve tested (e.g., change "[4,)" → "[4,X)" and "[${forge_version},)" →
"[${forge_version},${next_incompatible})", similarly for
"[${minecraft_version},)" and the Architectury "[0,)" entry) so the mods.toml
loaderVersion and versionRange fields enforce a concrete tested upper bound
rather than being unbounded.
In `@mod/neoforge/src/main/resources/META-INF/neoforge.mods.toml`:
- Around line 27-31: The Architectury dependency in neoforge.mods.toml uses an
open versionRange "[0,)" for modId "architectury" and the build step doesn't
expand an architectury_api_version property; tighten the versionRange to the
intended constrained range (replace "[0,)" with the specific range or property
placeholder like "${architectury_api_version}" you want to enforce) and add the
matching property expansion for architectury_api_version to the processResources
expand map in build.gradle so the TOML receives the concrete version at build
time; update the TOML entry for modId "architectury" and the processResources
expand configuration accordingly.
In `@mod/scripts/mcs-spyglass-validate.mjs`:
- Around line 167-171: The deduplication Map uses a key that omits severity so a
warning can overwrite an error and mask failures; include the severity field in
the dedupe key (e.g., use
`${diagnostic.file}:${diagnostic.line}:${diagnostic.column}:${diagnostic.severity
|| diagnostic.level}:${diagnostic.message}`) when building the Map in the loop
that creates `unique` and in the similar code at the later duplicate block
(lines around where `unique.set` is called again), so errors are not lost and
exit code logic sees the correct highest-severity diagnostic.
In `@mod/versions/manifest.json`:
- Line 11: Summary: The neoforge_version pin is for the wrong NeoForge series
(21.1.80 targets MC 1.21.1) causing CI to resolve incorrect mappings. Fix:
update the "neoforge_version" entry currently set to "21.1.80" to a NeoForge
21.2.x release that targets Minecraft 1.21.2 (for example change the value to a
21.2.x tag such as "21.2.0" or the latest 21.2.* available) so the profile
correctly maps to MC 1.21.2.
- Line 22: The manifest pins architectury_api_version to values (the
"architectury_api_version" key in mod/versions/manifest.json) that don't exist
in the Architectury Maven repo and the Forge build pulls that value via the
modImplementation line in mod/forge/build.gradle (modImplementation
"dev.architectury:architectury-forge:${rootProject.architectury_api_version}"),
causing 404s; fix by updating manifest.json to a valid Architectury release that
exists in https://maven.architectury.dev (or change the dependency coordinate in
mod/forge/build.gradle to the correct group/artifact if the project moved),
verify the chosen version/artifact resolves in CI, and run a gradle dependency
resolve to confirm no 404s before merging.
---
Outside diff comments:
In `@mod/common/src/main/resources/scripts/mcs-compile.cmd`:
- Around line 1-62: The batch file uses LF-only line endings which can break
Windows CMD label parsing (affecting sections like :run_mcs, :run_python,
:run_py and the main search/exit flow), so convert this file to CRLF line
endings (e.g., run unix2dos/dos2unix -w, set git core.autocrlf=true and
re-checkout, or add a .gitattributes entry like "*.cmd text eol=crlf" and
re-checkout) and re-run tests to ensure GOTO/CALL labels and %ERRORLEVEL%
propagation behave correctly.
In `@mod/gradlew.bat`:
- Around line 1-95: Convert this batch wrapper to CRLF line endings (currently
LF-only) so Windows cmd labels and parsing work reliably: open the gradlew.bat
file and save/commit it with CRLF line endings, ensuring you do not alter any
labels or logic (refer to labels like :execute, :fail, :mainEnd to verify
content integrity), and add or update repository settings to enforce CRLF for
.bat files (e.g., add a .gitattributes entry like "*.bat text eol=crlf" or set
core.autocrlf appropriately) so future commits preserve CRLF.
---
Minor comments:
In `@minecraft_script/shell_commands.py`:
- Around line 44-47: The code currently treats unsupported long flags as
positional arguments in the argument-parsing loop (see the handling of arg,
supported_flags, positional and index), causing misrouting; change this so that
when arg.startswith("--") and arg is not in supported_flags you raise/return an
explicit error (e.g., raise ValueError or call parser.error) instead of
appending to positional, and ensure index is still advanced; update the branch
in the parser loop that checks arg.startswith("--") to validate against
supported_flags and reject unknown flags with a clear message referencing the
flag.
In `@mod/common/build.gradle`:
- Around line 71-73: The npx fallback invocation in mod/common/build.gradle
currently uses 'npx --yes esbuild ...' which can pull an unpinned esbuild;
change this to either (a) invoke a pinned version by replacing the call with
'npx --yes esbuild@<version>' using the esbuild version from the mod/scripts
lock/published config, or (b) force a local-only invocation by switching to 'npx
--no-install esbuild' and ensure esbuild is declared/installed in the project
(node_modules/.bin/esbuild) so the local binary is used; locate the invocation
by searching for the commandLine entry containing 'npx', 'esbuild', and update
accordingly so builds are reproducible and do not fetch an arbitrary remote
esbuild.
In `@mod/scripts/apply_version.py`:
- Around line 22-24: The code in apply_version.py wrongly treats the string
"fabric" as invalid and falls back to DEFAULT_ENABLED_PLATFORMS when rewriting
enabled_platforms; update the validation around the variable value (the
enabled_platforms parsing branch that checks `if value and value != "fabric":
return value`) to allow "fabric" as a legitimate value (or explicitly accept any
known platform names including "fabric") so that an explicit
enabled_platforms="fabric" is preserved instead of being reset to
DEFAULT_ENABLED_PLATFORMS; ensure the logic that returns value uses the same
variable name `value` and that DEFAULT_ENABLED_PLATFORMS remains only a fallback
when value is empty or truly invalid.
In `@mod/scripts/mcs-spyglass-validate.mjs`:
- Around line 30-33: The parser currently assigns the next argv token to
flags.mcVersion without validation; update the argument-handling logic around
the '--mc-version' branch to ensure argv[index + 1] exists and does not start
with '-' (i.e., is not another flag) before setting flags.mcVersion and
incrementing index; if the value is missing or looks like a flag, emit a clear
error message and exit (or return an error) instead of silently accepting the
wrong token. Use the existing symbols flags.mcVersion and the code block
handling if (arg === '--mc-version') to locate where to add this validation.
In `@tests/test_compile_cli_flags.py`:
- Around line 69-92: The test test_compile_mc_version_does_not_mutate_config
currently only checks the in-process COMMON_CONFIG after calling subprocess.run,
which won't catch writes to the on-disk config; update the test to, after the
subprocess.run completes, reload the persisted config file (e.g., read and parse
the same config.json the application uses) and assert that the persisted
"minecraft_version" equals original_version (in addition to the existing
in-memory assertion). Use the same symbols from the diff
(test_compile_mc_version_does_not_mutate_config, COMMON_CONFIG, and the
subprocess.run invocation) to locate where to add the file-read/parse and
assertion, and ensure cleanup still happens in the finally block.
In `@todo.md`:
- Line 69: Update the markdown to satisfy markdownlint: add language identifiers
to the two fenced code blocks (the triple-backtick blocks referenced) so they
become e.g. ```bash or ```json as appropriate, and ensure the table near the
noted section has a blank line before and after it (insert an empty line above
the table header and one below the table) to fix table formatting violations.
---
Nitpick comments:
In `@mod/common/.gradle/architectury-cache/projectID`:
- Line 1: Remove the generated Architectury cache file "projectID" from version
control and stop tracking similar generated files: run git rm --cached for the
tracked projectID file (and commit the removal), then add the architectury cache
pattern (e.g., .gradle/architectury-cache/ or architectury-cache/projectID) to
.gitignore so future generated cache files are not committed; commit the updated
.gitignore and verify the file no longer appears in diffs.
In `@mod/common/build.gradle`:
- Around line 49-50: The build writes generated artifacts into
src/main/resources (spyglassBundledScript), which pollutes the source tree;
change the output path to under build/generated (e.g. def spyglassBundledScript
= file("${buildDir}/generated/resources/scripts/mcs-spyglass-validate.js") and
any other similar file outputs at 73-74), and then register that directory as a
resources source by adding sourceSets.main.resources.srcDir
"${buildDir}/generated/resources" (or call resources.srcDir). Update the tasks
that produce those files to write into the new build/generated path instead of
src/main/resources so generated files stay out of VCS and the working tree.
In
`@mod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/McsToolchain.java`:
- Line 47: In McsToolchain, replace the generic catch (Exception error) blocks
in the lint, compile, and validateWithSpyglass flows with specific exception
types that the called APIs throw (e.g., IOException, InterruptedException,
ExecutionException, etc.) by inspecting the methods invoked inside those try
blocks and catching those concrete exceptions (for example catch (IOException |
InterruptedException e) { ... }) while preserving the current diagnostic
creation logic; if you need a safety net, add a final catch for RuntimeException
that rethrows (or logs with explicit rethrow) to avoid silently masking
unexpected runtime errors.
- Around line 128-133: The current code calls Files.list(path) and then
stream.toList(), which materializes the entire directory into memory; replace
that with streaming iteration to avoid loading all entries at once — inside the
try (var stream = Files.list(path)) block, iterate the stream directly (e.g.,
stream.forEach(...) or stream.forEachOrdered(...)) and call
deleteRecursive(child) for each Path instead of using stream.toList(); keep the
try-with-resources and ensure deleteRecursive is invoked for every child.
In `@mod/common/src/main/resources/scripts/mcs-compile.cmd`:
- Around line 19-24: The loop that searches for Python installs (for %%P in (313
312 311 310) do ...) only checks C:\Python3xx folders; update the search to also
probe common install locations and call the existing :run_python label with any
found executable: check %LOCALAPPDATA%\Microsoft\WindowsApps\python.exe,
%LOCALAPPDATA%\Programs\Python\Python%P%\python.exe and
%PROGRAMFILES%\Python%P%\python.exe (or equivalent variant patterns) in addition
to the C:\Python%P\python.exe entries so the script will find Store, per-user,
and Program Files installations before/alongside exiting with the same
!ERRORLEVEL! behavior.
In `@mod/neoforge/build.gradle`:
- Line 7: The current guard in mod/neoforge/build.gradle that sets version =
rootProject.neoforge_version and uses neoForge
...:${rootProject.neoforge_version} is redundant because -Pmcs_profile already
wires minecraft_version and neoforge_version via mod/scripts/apply_version.py;
change the Loom “guard” approach to a manifest sanity-check by adding a small
assertion in apply_version.py (or equivalent build logic) that verifies the
selected NeoForge version matches the selected Minecraft version for the given
profile, and remove/relax the cross-profile incompatibility enforcement in
build.gradle (keep version = rootProject.neoforge_version for normal wiring but
rely on the manifest validation to catch mismatches).
In `@mod/scripts/package.json`:
- Around line 8-11: The dependencies in package.json currently use caret ranges
which allow minor/patch upgrades; update the entries for "`@spyglassmc/core`",
"`@spyglassmc/java-edition`", and "`@spyglassmc/mcdoc`" to pinned versions (remove
the leading ^ to use exact versions like "0.4.47", "0.3.60", "0.3.51") or switch
to tilde ranges if you only want patch updates (e.g. "~0.4.47") so builds become
reproducible and less susceptible to upstream minor changes.
🪄 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: 0a02c9e5-435e-4460-a595-2c034e241828
⛔ Files ignored due to path filters (4)
mod/common/.gradle/.architectury-transformer/debug.logis excluded by!**/*.logmod/dev/spyc0der77/mcspacks/toolchain/CompilerResolver.classis excluded by!**/*.classmod/gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jarmod/scripts/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (56)
.github/workflows/mod.yml.gitignoreminecraft_script/compiler/build_templates/builtins/1.21.2/log.mcfunctionminecraft_script/compiler/compile_interpreter.pyminecraft_script/config_utils.pyminecraft_script/shell_commands.pymod/README.mdmod/build.gradlemod/common/.gradle/architectury-cache/projectIDmod/common/build.gradlemod/common/src/legacy/java/dev/spyc0der77/mcspacks/deploy/CommandSources.javamod/common/src/main/java/dev/spyc0der77/mcspacks/McsPacks.javamod/common/src/main/java/dev/spyc0der77/mcspacks/config/ModConfig.javamod/common/src/main/java/dev/spyc0der77/mcspacks/config/ModConfigManager.javamod/common/src/main/java/dev/spyc0der77/mcspacks/deploy/DatapackDeployer.javamod/common/src/main/java/dev/spyc0der77/mcspacks/pipeline/PackPipeline.javamod/common/src/main/java/dev/spyc0der77/mcspacks/registry/PackDefinition.javamod/common/src/main/java/dev/spyc0der77/mcspacks/registry/PackRegistry.javamod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/CompilerResolver.javamod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/Diagnostic.javamod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/McsToolchain.javamod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/ProcessResult.javamod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/SubprocessRunner.javamod/common/src/main/java/dev/spyc0der77/mcspacks/util/McsPaths.javamod/common/src/main/java/dev/spyc0der77/mcspacks/util/PlayerFeedback.javamod/common/src/main/java/dev/spyc0der77/mcspacks/util/SafePaths.javamod/common/src/main/java/dev/spyc0der77/mcspacks/version/VersionMapper.javamod/common/src/main/java/dev/spyc0der77/mcspacks/watch/PackWatcher.javamod/common/src/main/resources/scripts/mcs-compile.cmdmod/common/src/main/resources/scripts/mcs-spyglass-validate.jsmod/common/src/modern/java/dev/spyc0der77/mcspacks/deploy/CommandSources.javamod/fabric/build.gradlemod/fabric/gradle.propertiesmod/fabric/src/main/java/dev/spyc0der77/mcspacks/fabric/McsPacksFabric.javamod/fabric/src/main/resources/fabric.mod.jsonmod/forge/build.gradlemod/forge/gradle.propertiesmod/forge/src/main/java/dev/spyc0der77/mcspacks/forge/McsPacksForge.javamod/forge/src/main/resources/META-INF/mods.tomlmod/forge/src/main/resources/mcs_packs.mixins.jsonmod/gradle.propertiesmod/gradle/wrapper/gradle-wrapper.propertiesmod/gradlewmod/gradlew.batmod/neoforge/build.gradlemod/neoforge/gradle.propertiesmod/neoforge/src/main/java/dev/spyc0der77/mcspacks/neoforge/McsPacksNeoForge.javamod/neoforge/src/main/resources/META-INF/neoforge.mods.tomlmod/scripts/apply_version.pymod/scripts/build_all.pymod/scripts/mcs-spyglass-validate.mjsmod/scripts/package.jsonmod/settings.gradlemod/versions/manifest.jsontests/test_compile_cli_flags.pytodo.md
There was a problem hiding this comment.
11 issues found across 31 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/common/src/main/resources/scripts/mcs-compile.cmd">
<violation number="1" location="mod/common/src/main/resources/scripts/mcs-compile.cmd:25">
P2: The new WindowsApps `python.exe` check can call the Microsoft Store alias and exit early, skipping working `py`/`python` fallbacks.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Comment on lines
+25
to
+28
| if exist "%LOCALAPPDATA%\Microsoft\WindowsApps\python.exe" ( | ||
| call :run_python "%LOCALAPPDATA%\Microsoft\WindowsApps\python.exe" %* | ||
| exit /b !ERRORLEVEL! | ||
| ) |
There was a problem hiding this comment.
P2: The new WindowsApps python.exe check can call the Microsoft Store alias and exit early, skipping working py/python fallbacks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mod/common/src/main/resources/scripts/mcs-compile.cmd, line 25:
<comment>The new WindowsApps `python.exe` check can call the Microsoft Store alias and exit early, skipping working `py`/`python` fallbacks.</comment>
<file context>
@@ -1,61 +1,73 @@
+ exit /b !ERRORLEVEL!
+ )
+)
+if exist "%LOCALAPPDATA%\Microsoft\WindowsApps\python.exe" (
+ call :run_python "%LOCALAPPDATA%\Microsoft\WindowsApps\python.exe" %*
+ exit /b !ERRORLEVEL!
</file context>
Suggested change
| if exist "%LOCALAPPDATA%\Microsoft\WindowsApps\python.exe" ( | |
| call :run_python "%LOCALAPPDATA%\Microsoft\WindowsApps\python.exe" %* | |
| exit /b !ERRORLEVEL! | |
| ) | |
| rem Skip direct WindowsApps python.exe invocation; rely on where python / py checks below. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mod/neoforge/build.gradle (1)
47-70: 💤 Low valueVersion parsing assumptions may not handle all formats.
The version increment logic assumes specific formats (modern:
2[6-9].x, release:x.y.z, NeoForge/Architectury:x.y.z) and silently falls back to returning the original version if the pattern doesn't match. This approach may not handle:
- Pre-release versions (e.g.,
1.21.2-rc1,21.1.0-alpha)- Snapshot versions
- Other non-standard version formats
While the current fallback behavior is reasonable, consider adding validation or logging to make mismatches visible during development, especially given the critical range limitation in the modern pattern (lines 49).
🤖 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/neoforge/build.gradle` around lines 47 - 70, The version increment closures nextMinecraftVersion, nextNeoForgeVersion and nextArchitecturyVersion currently assume strict regex formats and silently return the original version on mismatch; update them to validate inputs and surface mismatches (e.g., with logger.warn or throw during CI) and broaden parsing to recognize common pre-release/snapshot suffixes (like -rc, -alpha, -snapshot) by either stripping suffixes before applying the existing regex or adding alternative regex branches; ensure each closure logs the original version and the reason for fallback so non-standard formats are visible during development.
🤖 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/neoforge/build.gradle`:
- Line 49: The hardcoded regex for `modern` only matches majors 26–29; update
the pattern assigned to `modern` (the `def modern = ...` line) to accept major
versions >=26 (including 30+), e.g. replace `/^2[6-9]\.(\d+)$/` with a regex
like `/^(?:2[6-9]|[3-9]\d*)\.(\d+)$/` so the minor capture group remains the
same and future major versions (30, 31, ...) are treated as modern.
---
Nitpick comments:
In `@mod/neoforge/build.gradle`:
- Around line 47-70: The version increment closures nextMinecraftVersion,
nextNeoForgeVersion and nextArchitecturyVersion currently assume strict regex
formats and silently return the original version on mismatch; update them to
validate inputs and surface mismatches (e.g., with logger.warn or throw during
CI) and broaden parsing to recognize common pre-release/snapshot suffixes (like
-rc, -alpha, -snapshot) by either stripping suffixes before applying the
existing regex or adding alternative regex branches; ensure each closure logs
the original version and the reason for fallback so non-standard formats are
visible during development.
🪄 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: 73c3c437-db7a-4a4d-af20-b0f1c6924b76
📒 Files selected for processing (30)
.gitattributes.github/workflows/mod.yml.gitignoreminecraft_script/shell_commands.pymod/common/build.gradlemod/common/src/main/java/dev/spyc0der77/mcspacks/config/ModConfigManager.javamod/common/src/main/java/dev/spyc0der77/mcspacks/deploy/DatapackDeployer.javamod/common/src/main/java/dev/spyc0der77/mcspacks/pipeline/PackPipeline.javamod/common/src/main/java/dev/spyc0der77/mcspacks/registry/PackRegistry.javamod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/CompilerResolver.javamod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/McsToolchain.javamod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/SubprocessRunner.javamod/common/src/main/java/dev/spyc0der77/mcspacks/util/McsPaths.javamod/common/src/main/java/dev/spyc0der77/mcspacks/util/PlayerFeedback.javamod/common/src/main/java/dev/spyc0der77/mcspacks/version/VersionMapper.javamod/common/src/main/java/dev/spyc0der77/mcspacks/watch/PackWatcher.javamod/common/src/main/resources/scripts/mcs-compile.cmdmod/common/src/main/resources/scripts/mcs-compile.shmod/fabric/build.gradlemod/fabric/src/main/resources/fabric.mod.jsonmod/forge/build.gradlemod/forge/src/main/resources/META-INF/mods.tomlmod/gradle.propertiesmod/neoforge/build.gradlemod/neoforge/src/main/resources/META-INF/neoforge.mods.tomlmod/scripts/apply_version.pymod/scripts/mcs-spyglass-validate.mjsmod/scripts/package.jsonmod/versions/manifest.jsontests/test_compile_cli_flags.py
✅ Files skipped from review due to trivial changes (4)
- .gitattributes
- mod/forge/src/main/resources/META-INF/mods.toml
- mod/fabric/src/main/resources/fabric.mod.json
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (20)
- mod/gradle.properties
- mod/scripts/package.json
- mod/forge/build.gradle
- mod/common/src/main/java/dev/spyc0der77/mcspacks/config/ModConfigManager.java
- .github/workflows/mod.yml
- mod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/SubprocessRunner.java
- mod/neoforge/src/main/resources/META-INF/neoforge.mods.toml
- mod/common/src/main/java/dev/spyc0der77/mcspacks/util/PlayerFeedback.java
- mod/common/src/main/java/dev/spyc0der77/mcspacks/registry/PackRegistry.java
- tests/test_compile_cli_flags.py
- mod/fabric/build.gradle
- mod/common/src/main/java/dev/spyc0der77/mcspacks/util/McsPaths.java
- minecraft_script/shell_commands.py
- mod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/McsToolchain.java
- mod/common/src/main/java/dev/spyc0der77/mcspacks/deploy/DatapackDeployer.java
- mod/common/src/main/java/dev/spyc0der77/mcspacks/watch/PackWatcher.java
- mod/common/src/main/java/dev/spyc0der77/mcspacks/toolchain/CompilerResolver.java
- mod/scripts/mcs-spyglass-validate.mjs
- mod/common/build.gradle
- mod/versions/manifest.json
- Modified `gradle.properties` to enable support for multiple platforms: fabric, forge, and neoforge. - Updated `build.gradle` scripts to streamline version management for Forge and NeoForge. - Enhanced `ModConfigManager` to handle atomic file moves with fallback for unsupported operations. - Refactored `mcs-compile` scripts for improved Python version detection and execution. - Cleaned up unnecessary candidate paths in `CompilerResolver` and improved error handling in `McsPaths`. - Adjusted loader version constraints in `mods.toml` files for Forge and NeoForge.
- Introduced `stripPreReleaseSuffix` and `warnVersionFallback` functions to improve version handling. - Updated `nextMinecraftVersion` and `nextMajorVersion` functions to utilize the new helper methods for better version normalization and fallback warnings. - Enhanced version retrieval for `forge_version` and `neoforge_version` to include fallback handling for unrecognized formats.
There was a problem hiding this comment.
7 issues found across 31 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/common/src/main/resources/scripts/mcs-compile.cmd">
<violation number="1" location="mod/common/src/main/resources/scripts/mcs-compile.cmd:25">
P2: The new WindowsApps `python.exe` check can call the Microsoft Store alias and exit early, skipping working `py`/`python` fallbacks.</violation>
</file>
<file name="mod/common/src/main/java/dev/spyc0der77/mcspacks/registry/PackRegistry.java">
<violation number="1" location="mod/common/src/main/java/dev/spyc0der77/mcspacks/registry/PackRegistry.java:61">
P1: Compiled output folder tracking now uses pack ID, but compilation writes to display name, causing post-compile missing-folder failures.</violation>
</file>
<file name="mod/forge/build.gradle">
<violation number="1" location="mod/forge/build.gradle:26">
P1: Forge module now depends on the NeoForge Architectury artifact, which mismatches the Forge target and can break Forge runtime/build compatibility.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| id, | ||
| folder, | ||
| entry, | ||
| SafePaths.resolveChild(compiledRoot, id), |
There was a problem hiding this comment.
P1: Compiled output folder tracking now uses pack ID, but compilation writes to display name, causing post-compile missing-folder failures.
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/spyc0der77/mcspacks/registry/PackRegistry.java, line 61:
<comment>Compiled output folder tracking now uses pack ID, but compilation writes to display name, causing post-compile missing-folder failures.</comment>
<file context>
@@ -58,7 +58,7 @@ public PackDefinition resolvePack(Path folder) throws IOException {
folder,
entry,
- SafePaths.resolveChild(compiledRoot, displayName),
+ SafePaths.resolveChild(compiledRoot, id),
displayName
);
</file context>
Suggested change
| SafePaths.resolveChild(compiledRoot, id), | |
| SafePaths.resolveChild(compiledRoot, displayName), |
|
|
||
| dependencies { | ||
| forge "net.minecraftforge:forge:${rootProject.minecraft_version}-${rootProject.forge_version}" | ||
| modImplementation "dev.architectury:architectury-neoforge:${rootProject.architectury_api_version}" |
There was a problem hiding this comment.
P1: Forge module now depends on the NeoForge Architectury artifact, which mismatches the Forge target and can break Forge runtime/build compatibility.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mod/forge/build.gradle, line 26:
<comment>Forge module now depends on the NeoForge Architectury artifact, which mismatches the Forge target and can break Forge runtime/build compatibility.</comment>
<file context>
@@ -23,7 +23,7 @@ configurations {
dependencies {
forge "net.minecraftforge:forge:${rootProject.minecraft_version}-${rootProject.forge_version}"
- modImplementation "dev.architectury:architectury-forge:${rootProject.architectury_api_version}"
+ modImplementation "dev.architectury:architectury-neoforge:${rootProject.architectury_api_version}"
common(project(path: ':common', configuration: 'namedElements')) { transitive false }
</file context>
Suggested change
| modImplementation "dev.architectury:architectury-neoforge:${rootProject.architectury_api_version}" | |
| modImplementation "dev.architectury:architectury-forge:${rootProject.architectury_api_version}" |
- Modified GitHub Actions workflow to dynamically set the Java version based on the profile, allowing for better compatibility with different Minecraft versions. - Enhanced `build.gradle` files to conditionally set the required Java version based on the Minecraft version, improving version management. - Updated `McsToolchain` to use the pack ID instead of display name for better clarity. - Refactored Python version detection in `CompilerResolver` to handle version formatting more robustly. - Improved error handling in `mcs-spyglass-validate.mjs` to ensure required arguments are provided.
- Updated Gradle wrapper to version 8.14.5 for improved build performance and compatibility. - Deleted the `mcs-spyglass-validate.js` script as part of the ongoing cleanup and consolidation of validation functionality.
| } | ||
|
|
||
| if (positional.length < 1) { | ||
| throw new Error('Usage: node mcs-spyglass-validate.js [--json] [--mc-version <version>] <datapack_dir>') |
There was a problem hiding this comment.
SUGGESTION: Usage message references wrong file extension. The error message says mcs-spyglass-validate.js but the actual file is mcs-spyglass-validate.mjs. This could confuse users when they try to run the script.
… releases - Updated `forge_version` from 57.0.0 to 58.1.0 for game versions 1.21.8. - Updated `forge_version` from 58.0.0 to 60.1.0 for game versions 1.21.10, ensuring support for the latest features and improvements.
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.
.gitignorefor Gradle build artifacts and node modules.todo.mdfile to include detailed implementation plans for the MCS Hot-Reload Mod, covering core functionality, project phases, and configuration management.config_utils.pyfor temporary configuration overrides.shell_commands.pyto support new flags for the compile command, including--mc-versionand--force.CompileInterpreterto conditionally track built-in function usage based on logging style.Summary by CodeRabbit
New Features
compileCLI with stricter validation, safer path handling, and optional--mc-versionand--forceflags.Documentation
Tests