fix(scripts): make mvnw invocation cross-platform for Windows support#950
fix(scripts): make mvnw invocation cross-platform for Windows support#950FelixTJDietrich merged 1 commit intomainfrom
Conversation
Replace Unix-only `./mvnw` calls in npm scripts with a cross-platform TypeScript runner that resolves `./mvnw` (Unix) or `mvnw.cmd` (Windows) based on `process.platform`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
bug
github-actions
bot
added
the
size:M
📝 WalkthroughWalkthroughTwo npm linting scripts ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a cross-platform wrapper for invoking the Maven Wrapper from npm scripts so Java linting works on Windows (where ./mvnw path syntax fails under cmd.exe).
Changes:
- Introduce
scripts/run-mvnw.tsto resolve and executemvnw/mvnw.cmddepending on platform. - Update
lint:javaandlint:java:reportnpm scripts to use the new runner instead ofcd ... && ./mvnw.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
scripts/run-mvnw.ts |
New Node/TS script that spawns Maven Wrapper in server/application-server, aiming to propagate exit codes and handle failures. |
package.json |
Switch Java lint scripts to use run-mvnw.ts for Windows compatibility. |
| const result = spawnSync( | ||
| isWindows ? "mvnw.cmd" : "./mvnw", | ||
| process.argv.slice(2), | ||
| { | ||
| stdio: "inherit", | ||
| cwd: mvnwDir, | ||
| shell: isWindows, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
On Windows shell: true means the spawned process is cmd.exe, so a missing mvnw.cmd typically won’t surface as result.error/ENOENT; instead you’ll get a non-zero status (often 9009) and a generic shell message. If you want the actionable “wrapper not found” error cross-platform, check for the wrapper file’s existence before spawning (or explicitly detect the Windows command-not-found exit code and print the same message).
|
|
||
| function main(): void { | ||
| // shell: true is required on Windows for .cmd files (CVE-2024-27980). | ||
| // Safe here because args come from hardcoded npm scripts, not user input. | ||
| const result = spawnSync( | ||
| isWindows ? "mvnw.cmd" : "./mvnw", | ||
| process.argv.slice(2), | ||
| { | ||
| stdio: "inherit", | ||
| cwd: mvnwDir, | ||
| shell: isWindows, |
There was a problem hiding this comment.
Using shell: true on Windows while forwarding process.argv.slice(2) allows shell metacharacters (e.g. &, |, >) to be interpreted by cmd.exe when someone runs npm run lint:java -- <args...> or invokes this script directly. To avoid this, consider spawning cmd.exe explicitly with a safely-quoted command line (or using a library like cross-spawn), and/or validating/escaping args so they’re never interpreted by the shell.
| function main(): void { | |
| // shell: true is required on Windows for .cmd files (CVE-2024-27980). | |
| // Safe here because args come from hardcoded npm scripts, not user input. | |
| const result = spawnSync( | |
| isWindows ? "mvnw.cmd" : "./mvnw", | |
| process.argv.slice(2), | |
| { | |
| stdio: "inherit", | |
| cwd: mvnwDir, | |
| shell: isWindows, | |
| const mvnwPath = path.join(mvnwDir, isWindows ? "mvnw.cmd" : "mvnw"); | |
| function main(): void { | |
| // Run the Maven wrapper directly to avoid shell interpretation of arguments. | |
| const result = spawnSync( | |
| mvnwPath, | |
| process.argv.slice(2), | |
| { | |
| stdio: "inherit", | |
| cwd: mvnwDir, |
| "lint:java": "cd server/application-server && ./mvnw pmd:check -q", | ||
| "lint:java:report": "cd server/application-server && ./mvnw pmd:pmd && echo 'Report: server/application-server/target/site/pmd.html'", | ||
| "lint:java": "node --import tsx scripts/run-mvnw.ts pmd:check -q", | ||
| "lint:java:report": "node --import tsx scripts/run-mvnw.ts pmd:pmd && echo 'Report: server/application-server/target/site/pmd.html'", |
There was a problem hiding this comment.
echo 'Report: ...' is Bash-friendly, but on Windows cmd.exe it prints the single quotes literally. Since this script is being made Windows-compatible, consider switching to echo Report: ... (no quotes) or printing via Node so the output is consistent across shells.
| "lint:java:report": "node --import tsx scripts/run-mvnw.ts pmd:pmd && echo 'Report: server/application-server/target/site/pmd.html'", | |
| "lint:java:report": "node --import tsx scripts/run-mvnw.ts pmd:pmd && echo Report: server/application-server/target/site/pmd.html", |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/run-mvnw.ts (1)
16-18: UseALL_CAPSfor file-level constants.
mvnwDirandisWindowsare constants; please rename toMVNW_DIRandIS_WINDOWS(and update references) to match the repository naming rule.As per coding guidelines, "Use ALL_CAPS for constants."♻️ Proposed rename
-const mvnwDir = path.resolve(__dirname, "..", "server", "application-server"); -const isWindows = process.platform === "win32"; +const MVNW_DIR = path.resolve(__dirname, "..", "server", "application-server"); +const IS_WINDOWS = process.platform === "win32"; - isWindows ? "mvnw.cmd" : "./mvnw", + IS_WINDOWS ? "mvnw.cmd" : "./mvnw", process.argv.slice(2), { stdio: "inherit", - cwd: mvnwDir, - shell: isWindows, + cwd: MVNW_DIR, + shell: IS_WINDOWS, },Also applies to: 23-24, 27-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run-mvnw.ts` around lines 16 - 18, Rename the file-level constants to ALL_CAPS: change mvnwDir -> MVNW_DIR and isWindows -> IS_WINDOWS (and update every usage) to follow repository naming rules; also update the other file-level consts on the same file referenced in the comment (the ones declared around lines referenced) to ALL_CAPS as well, ensuring any destructuring or references in functions (e.g., usages inside runMvnw or scripts invoking these variables) are updated to the new names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/run-mvnw.ts`:
- Around line 16-18: Rename the file-level constants to ALL_CAPS: change mvnwDir
-> MVNW_DIR and isWindows -> IS_WINDOWS (and update every usage) to follow
repository naming rules; also update the other file-level consts on the same
file referenced in the comment (the ones declared around lines referenced) to
ALL_CAPS as well, ensuring any destructuring or references in functions (e.g.,
usages inside runMvnw or scripts invoking these variables) are updated to the
new names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67407cdb-09b4-4dc8-bba2-005c8f6388a7
📒 Files selected for processing (2)
package.jsonscripts/run-mvnw.ts
📚 Documentation Preview
|
|
🎉 This PR is included in version 0.52.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
On Windows,
npm run lint:javafails with'.' is not recognized as an internal or external commandbecause the npm scripts invoke./mvnw— a Unix-only path syntax. Windowscmd.exe(npm's default script shell) does not understand./as a path prefix.This PR introduces
scripts/run-mvnw.ts, a cross-platform Maven wrapper runner that resolves./mvnw(Unix) ormvnw.cmd(Windows) based onprocess.platform, and updates thelint:javaandlint:java:reportnpm scripts to use it.What changed
scripts/run-mvnw.tsspawnSync. Handlesresult.error(ENOENT with actionable message),result.signal(re-raises), andresult.status(propagates exit code). Usesshell: trueonly on Windows per CVE-2024-27980. Path anchored via__dirname, notprocess.cwd().package.jsonlint:javaandlint:java:reportnow usenode --import tsx scripts/run-mvnw.tsinstead ofcd server/application-server && ./mvnw.Design decisions
run-quiet.tspattern — same invocation style (node --import tsx scripts/...), same imports (node:prefix, namespaceimport path), same__dirnameresolution viafileURLToPath, sameprocess.exitCode(notprocess.exit()).shell: truescoped to Windows only — required for.cmdfiles post-CVE-2024-27980, but unnecessary (and slightly less safe) on Unix. Args passed as array, never string-concatenated.main()—spawnSyncis inherently sync; wrapping inasyncwould be misleading.Out of scope
generate:api:application-server:specs(package.json line 11) also calls./mvnwbut is wrapped insh -cwith Docker detection logic that's inherently Unix-only. Fixing that requires a larger refactor and is tracked separately.How to test
npm run lint:java— runs PMD check, exits 0 on successnpm run lint:java:report— generates PMD report, prints report pathnpm run check— full check pipeline including lint:java'.' is not recognizederror is goneSummary by CodeRabbit