Add npm#12
Conversation
|
Warning Review limit reached
More reviews will be available in 51 minutes and 51 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 selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds an npm package wrapper that distributes the minecraft-script Python CLI: a package manifest, a Node.js CLI shim delegating to the Python module, a postinstall script to pip-install the matching Python package, pathlib-based path handling updates in Python code, and .gitignore additions. ChangesNPM Package Wrapper for Python CLI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }, | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "git+https://github.com/Bard-Gaming/Minecraft-Script.git", |
There was a problem hiding this comment.
WARNING: Repository URL points to Bard-Gaming instead of SpyC0der77
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file in incremental diff)
Reviewed by step-3.7-flash-20260528 · 185,325 tokens |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 4 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="npm/package.json">
<violation number="1" location="npm/package.json:3">
P2: Missing `"os"` field: The readme explicitly states this package does not work on Linux/macOS, but the package.json does not enforce this restriction with `"os": ["win32"]`. Without it, users on unsupported platforms can install the npm package and encounter confusing failures instead of a clear install-time error.</violation>
<violation number="2" location="npm/package.json:23">
P2: The repository, homepage, and bugs URLs all point to `Bard-Gaming/Minecraft-Script`. Verify these point to the correct GitHub owner/organization for this project. If this repository is under a different account (e.g., `SpyC0der77`), these URLs will misdirect users looking for source code, filing issues, etc.</violation>
</file>
<file name="npm/scripts/postinstall.js">
<violation number="1" location="npm/scripts/postinstall.js:25">
P2: The loop exits after the first non-ENOENT failure, so fallback Python commands are never tried.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| @@ -0,0 +1,36 @@ | |||
| { | |||
| "name": "minecraft-script", | |||
| "version": "0.2.2", | |||
There was a problem hiding this comment.
P2: Missing "os" field: The readme explicitly states this package does not work on Linux/macOS, but the package.json does not enforce this restriction with "os": ["win32"]. Without it, users on unsupported platforms can install the npm package and encounter confusing failures instead of a clear install-time error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At npm/package.json, line 3:
<comment>Missing `"os"` field: The readme explicitly states this package does not work on Linux/macOS, but the package.json does not enforce this restriction with `"os": ["win32"]`. Without it, users on unsupported platforms can install the npm package and encounter confusing failures instead of a clear install-time error.</comment>
<file context>
@@ -0,0 +1,36 @@
+{
+ "name": "minecraft-script",
+ "version": "0.2.2",
+ "description": "Compile and debug Minecraft Script (.mcs) files into Minecraft datapacks",
+ "license": "MIT",
</file context>
|
@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: 2
♻️ Duplicate comments (1)
npm/package.json (1)
21-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWARNING: Repository metadata points to
Bard-Gaminginstead ofSpyC0der77The repository URLs reference
https://github.com/Bard-Gaming/Minecraft-Scriptbut the PR is opened from theSpyC0der77account. Verify that the repository ownership is correct for publishing this npm package.🤖 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 `@npm/package.json` around lines 21 - 29, The package metadata in npm/package.json currently points to the Bard-Gaming repo; update the "repository" object (type/url/directory) and the "homepage" and "bugs" fields to reference the correct GitHub account (SpyC0der77) so URLs reflect the PR owner; ensure the "repository.url" uses the git+https URL with SpyC0der77, "homepage" points to the SpyC0der77 repo readme, and "bugs.url" points to the SpyC0der77 issues page to keep package publishing metadata consistent.
🤖 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 `@npm/bin/mcs.js`:
- Around line 17-21: The loop currently exits on non-ENOENT spawn errors; change
the logic in npm/bin/mcs.js so that you only call process.exit when the command
actually executed (i.e., when result.status is defined) and not when spawn
produced an error. Concretely, keep checking result.error?.code === "ENOENT" to
continue, but ALSO treat any result.error (non-ENOENT) as a reason to continue
to the next candidate (do not call process.exit there); only invoke
process.exit(result.status ?? 1) when result.status is present (meaning the
spawn succeeded and returned an exit code).
In `@npm/scripts/postinstall.js`:
- Around line 20-33: The postinstall logic currently returns on non-zero pip
exit or spawn errors, preventing trying other candidates; modify the flow around
the result/error handling so that any spawn error (result.error is truthy)
causes the loop to continue (except you may still continue on ENOENT as already
done) and only treat a completed process with result.status !== 0 as the
condition to warn and return. Concretely, in the block using result.error?.code
and result.status, move or add the check so that if result.error exists you
continue to the next candidate (preserving the existing ENOENT special-case),
and only call console.warn and return when result.status is defined and
non-zero.
---
Duplicate comments:
In `@npm/package.json`:
- Around line 21-29: The package metadata in npm/package.json currently points
to the Bard-Gaming repo; update the "repository" object (type/url/directory) and
the "homepage" and "bugs" fields to reference the correct GitHub account
(SpyC0der77) so URLs reflect the PR owner; ensure the "repository.url" uses the
git+https URL with SpyC0der77, "homepage" points to the SpyC0der77 repo readme,
and "bugs.url" points to the SpyC0der77 issues page to keep package publishing
metadata consistent.
🪄 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: f9f93f28-2516-43ae-bbdf-cc85e1f4e119
📒 Files selected for processing (4)
.gitignorenpm/bin/mcs.jsnpm/package.jsonnpm/scripts/postinstall.js
…tilities - Updated README to clarify that the Python package is compatible with Windows, macOS, and Linux. - Refactored path handling in `config_utils.py` and `shell_commands.py` to use `pathlib` for improved cross-platform compatibility and readability. - Adjusted error handling in shell commands to provide clearer messages for file and directory validation. - Updated npm package repository URLs to reflect the new maintainer.
There was a problem hiding this comment.
1 issue found across 6 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="npm/scripts/postinstall.js">
<violation number="1" location="npm/scripts/postinstall.js:20">
P2: Non-ENOENT process spawn errors are silently ignored, which can misreport failures as "Python was not found".</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| if (result.error) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
P2: Non-ENOENT process spawn errors are silently ignored, which can misreport failures as "Python was not found".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At npm/scripts/postinstall.js, line 20:
<comment>Non-ENOENT process spawn errors are silently ignored, which can misreport failures as "Python was not found".</comment>
<file context>
@@ -17,20 +17,22 @@ const candidates = [
const result = spawnSync(command, args, { stdio: "inherit" });
- if (result.error?.code === "ENOENT") {
+ if (result.error) {
continue;
}
</file context>
| if (result.error) { | |
| continue; | |
| } | |
| if (result.error?.code === "ENOENT") { | |
| continue; | |
| } | |
| if (result.error) { | |
| console.warn( | |
| "\n[minecraft-script] Failed to install the Python package via pip.\n" + | |
| "Install it manually with:\n" + | |
| ` pip install minecraft-script==${version}\n` | |
| ); | |
| return; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
npm/scripts/postinstall.js (1)
28-35: ⚡ Quick winConsider trying all candidates before stopping on non-zero exit.
The script now correctly continues on spawn errors (line 20), but if a candidate runs and exits with a non-zero status, it warns and returns immediately without trying the remaining candidates. While this is appropriate for network errors or missing packages (where all candidates would fail the same way), it may prevent success when pip is unavailable for the first Python but installed for another.
For example, if
py -3 -m pipexits 1 because pip isn't installed for that Python, the script stops—even thoughpython -m piporpython3 -m pipmight succeed.♻️ Proposed fix to try all candidates on any failure
if (result.status === 0) { return; } - - if (result.status != null) { - console.warn( - "\n[minecraft-script] Failed to install the Python package via pip.\n" + - "Install it manually with:\n" + - ` pip install minecraft-script==${version}\n` - ); - return; - } } +console.warn( + "\n[minecraft-script] Failed to install the Python package via pip.\n" + + "Install it manually with:\n" + + ` pip install minecraft-script==${version}\n` +); +return; + -console.warn( - "\n[minecraft-script] Python was not found, so the PyPI package was not installed.\n" + - "Install Python 3 from https://www.python.org/downloads/ and run:\n" + - ` pip install minecraft-script==${version}\n` -);This moves the pip-failure warning outside the loop so all candidates are tried, merging the two warnings into one. If finer-grained messaging is desired (distinguishing "Python not found" from "pip failed"), you can track whether any candidate actually ran.
🤖 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 `@npm/scripts/postinstall.js` around lines 28 - 35, The current loop over Python candidates returns immediately when a candidate runs but exits with a non-zero status (the check on result.status), preventing later candidates from being tried; modify the logic in the code that iterates pythonCandidates so it does not return on a non-zero exit from a spawned pip process but instead records that a candidate ran and failed, continues to the next candidate, and after the loop emits a single consolidated warning if none succeeded (merge the pip-failure and spawn-failure messages). Specifically, change the early-return in the block that inspects result.status to continue/collect failure state, ensure the existing spawn error path still continues, and emit the final warning once after the loop if no successful install occurred.
🤖 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 `@minecraft_script/config_utils.py`:
- Around line 49-54: The README/docs need a short note documenting that
config_path_check() now resolves and persists default_output_path as an absolute
path when update_config() writes to config.json, and that the CLI requires the
target directory to already exist (config_path_check will raise/exit if it
doesn't); also mention that runtime code (sh_compile) still resolves
COMMON_CONFIG["default_output_path"] before use so existing relative values
continue to work until updated.
---
Nitpick comments:
In `@npm/scripts/postinstall.js`:
- Around line 28-35: The current loop over Python candidates returns immediately
when a candidate runs but exits with a non-zero status (the check on
result.status), preventing later candidates from being tried; modify the logic
in the code that iterates pythonCandidates so it does not return on a non-zero
exit from a spawned pip process but instead records that a candidate ran and
failed, continues to the next candidate, and after the loop emits a single
consolidated warning if none succeeded (merge the pip-failure and spawn-failure
messages). Specifically, change the early-return in the block that inspects
result.status to continue/collect failure state, ensure the existing spawn error
path still continues, and emit the final warning once after the loop if no
successful install occurred.
🪄 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: 48bc49c2-7d82-41d0-99cf-ab10a2b2f49d
📒 Files selected for processing (6)
minecraft_script/config_utils.pyminecraft_script/shell_commands.pynpm/bin/mcs.jsnpm/package.jsonnpm/scripts/postinstall.jsreadme.md
🚧 Files skipped from review as they are similar to previous changes (2)
- npm/bin/mcs.js
- npm/package.json
… error handling - Added a new section in README to explain how to set the default output path for compilation. - Updated the post-install script to improve error handling and provide clearer instructions for manual installation of the Python package if automatic installation fails.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
npm/scripts/postinstall.js (1)
32-34: 💤 Low valueConsider simplifying the status check.
At this point in the code,
result.erroris falsy andresult.status !== 0, soresult.statusis either a non-zero number (pip failed) ornull(killed by signal). In both cases, you want to try the next candidate. The explicit!= nullcheck works but could be replaced with an unconditionalcontinuefor clarity:if (result.status === 0) { return; } -if (result.status != null) { - continue; -} +// Process completed with non-zero status or was killed; try next candidate +continue;This simplifies the logic and makes the intent clearer. However, the current code is functionally correct, so this is optional.
🤖 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 `@npm/scripts/postinstall.js` around lines 32 - 34, The conditional check "if (result.status != null) { continue; }" can be simplified: inside the loop that evaluates each candidate result, replace that conditional with an unconditional "continue;" since at this point result.error is falsy and any non-zero status or null (killed by signal) should both skip to the next candidate; update the loop body where the "result" variable is used to just "continue;" to make the intent clearer.
🤖 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.
Nitpick comments:
In `@npm/scripts/postinstall.js`:
- Around line 32-34: The conditional check "if (result.status != null) {
continue; }" can be simplified: inside the loop that evaluates each candidate
result, replace that conditional with an unconditional "continue;" since at this
point result.error is falsy and any non-zero status or null (killed by signal)
should both skip to the next candidate; update the loop body where the "result"
variable is used to just "continue;" to make the intent clearer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 786a4e73-782b-42c6-9f7f-dc7f697a4389
📒 Files selected for processing (2)
npm/scripts/postinstall.jsreadme.md
- Removed unnecessary status check in the post-install script to simplify command processing. - Maintained clarity in manual installation instructions for the Python package.
1 participant
Summary by CodeRabbit
New Features
mcs/minecraft-scriptCLI entrypointsChores
Documentation