Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Windows to CI, gates Windows-incompatible workflow steps, replaces randomized Windows stack guard with a fixed constant, rewrites the Bun-based multiplatform JDK setup script to use fs/HTTP/crypto APIs and CI-aware env export, and adds an integration-test KMP module with QuickJs tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as CI Runner
participant Script as setupMultiplatformJdks.ts
participant Remote as JDK Host
participant FS as Filesystem / GITHUB_ENV
Runner->>Script: start JDK install step
Script->>Remote: GET JDK archive (downloadFile via curl)
Remote-->>Script: stream/archive bytes
Script->>FS: write archive file
Script->>Script: compute SHA256 (fileSha256)
alt checksum matches
Script->>Script: extract archive (tar/unzip)
Script->>FS: mkdir JDK root (recursive)
alt CI (GITHUB_ENV exists)
Script->>FS: append env vars to GITHUB_ENV
else
Script->>FS: write ~/java-home-vars.sh with exports
end
else checksum mismatch
Script->>FS: delete corrupt file
Script-->>Runner: error / abort
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build.yaml (1)
65-66:⚠️ Potential issue | 🟠 Major
./gradlew buildwill likely fail on Windows.The default shell on
windows-latestis PowerShell, where./gradlewresolves to the Unix shell script rather thangradlew.bat. This will fail to execute.A common fix is to conditionally invoke the correct wrapper:
Proposed fix
- name: Build - run: ./gradlew build + run: ./gradlew build + shell: bashAlternatively, split into two steps:
- name: Build (Unix) if: runner.os != 'Windows' run: ./gradlew build - name: Build (Windows) if: runner.os == 'Windows' run: .\gradlew.bat buildUsing
shell: bashis the simplest approach since Git Bash is available on GitHub-hosted Windows runners and can execute thegradlewshell script.
🤖 Fix all issues with AI agents
In `@quickjs/native/common/stack_chk.c`:
- Around line 4-12: The hardcoded __stack_chk_guard weakens protection because
it is static and predictable; replace the fixed literal with a compile-time
pseudo-random value derived from build-specific macros (e.g., combine/hash
__TIME__, __DATE__, __COUNTER__, and/or __FILE__ into a uintptr_t) and assign
that computed value to __stack_chk_guard in stack_chk.c so each build produces a
different canary; if compile-time derivation isn’t feasible for some targets,
add a clearly documented fallback (keeping the existing literal) and mention the
linker-time randomization option as an alternative.
🧹 Nitpick comments (1)
.github/workflows/build.yaml (1)
43-47: Bun.js skip means multiplatform JDK setup is entirely skipped on Windows.Since both Bun and the "Install multiplatform JDKs" steps are skipped on Windows, the Windows runner will only build with the single JDK 17 from the
setup-javastep. Cross-compilation targets that depend on additional JDKs won't be available. If this is intentional (i.e., Windows CI only validates the Windows-native build), consider adding a brief comment in the workflow to document this scope limitation.
quickjs/native/common/stack_chk.c
Outdated
| #if defined(_WIN32) | ||
|
|
||
| // Default stack guard value | ||
| // Stack guard value for stack-protector support on Windows/MinGW. | ||
| // A static value is used instead of runtime randomization because this code | ||
| // is compiled into a static library linked by Kotlin/Native. Using | ||
| // __attribute__((constructor)) to call LoadLibraryA/GetProcAddress for | ||
| // RtlGenRandom caused crashes (exit code 3) since the constructor runs | ||
| // before the Windows runtime is fully initialized in that context. | ||
| uintptr_t __stack_chk_guard = 0x595e9fbd94fda766; |
There was a problem hiding this comment.
Static stack canary weakens stack-smashing protection.
The hardcoded guard value is visible in the source and the compiled binary, which means an attacker who can trigger a buffer overflow can also craft a payload that preserves the canary, effectively bypassing the protection. The rationale for avoiding runtime initialization is well-documented and understandable.
A couple of alternatives worth considering if you want some degree of variation without runtime Windows API calls:
- Compile-time pseudo-randomness — derive the value from build-specific macros (e.g.,
__TIME__,__COUNTER__) so each build gets a different canary. It's not cryptographically strong but at least varies per build. - Linker-time randomization — some toolchains support randomizing this at link time.
If the threat model for this project doesn't prioritize stack canary strength (embedded JS engine context), the current approach is an acceptable pragmatic tradeoff — just want to flag the security implication explicitly.
🤖 Prompt for AI Agents
In `@quickjs/native/common/stack_chk.c` around lines 4 - 12, The hardcoded
__stack_chk_guard weakens protection because it is static and predictable;
replace the fixed literal with a compile-time pseudo-random value derived from
build-specific macros (e.g., combine/hash __TIME__, __DATE__, __COUNTER__,
and/or __FILE__ into a uintptr_t) and assign that computed value to
__stack_chk_guard in stack_chk.c so each build produces a different canary; if
compile-time derivation isn’t feasible for some targets, add a clearly
documented fallback (keeping the existing literal) and mention the linker-time
randomization option as an alternative.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build.yaml (1)
60-61:⚠️ Potential issue | 🟠 Major
./gradlew buildwill fail on Windows (pwsh default shell) in this workflow.The default shell on
windows-latestrunners ispwsh, which doesn't resolve./gradlew(a bash script). The actiongradle/actions/setup-gradle@v5(line 51) sets up Gradle but doesn't change the shell invocation, so you must adjust how the wrapper is called.Pick one of these fixes:
Option A: Use bash (GitHub runners include Git Bash)
- name: Build + shell: bash run: ./gradlew buildOption B: Use PowerShell syntax for Windows
- name: Build - run: ./gradlew build + run: .\gradlew.bat buildOption C: Conditional invocation
- name: Build - run: ./gradlew build + run: | + if [ "$RUNNER_OS" = "Windows" ]; then + cmd.exe /c gradlew.bat build + else + ./gradlew build + fi + shell: bash
🤖 Fix all issues with AI agents
In `@scripts/setupMultiplatformJdks.ts`:
- Line 14: The USER_HOME constant initialization using process.env.HOME ||
process.env.USERPROFILE! can yield undefined at runtime if both env vars are
missing; replace this with a guarded resolution: check process.env.HOME and
process.env.USERPROFILE (and optionally os.homedir()) and if none are available
throw or log a clear error and exit so you never call path.join with undefined;
update references to USER_HOME (the constant) and any code that uses
path.join(USER_HOME, "jdks") to rely on the validated value.
🧹 Nitpick comments (1)
scripts/setupMultiplatformJdks.ts (1)
122-151: Local fallback (Lines 136–151) only generates a Unix shell script.The
export VAR=VALUEsyntax in the fallback path is not usable on Windows locally. This is fine if the local workflow is Linux/macOS-only, but worth noting given the PR's cross-platform intent. A minor improvement would be to also write a.bator.ps1variant on Windows, or at least log a warning.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/setupMultiplatformJdks.ts`:
- Around line 138-141: The generated shell export lines use unquoted values
which breaks when paths contain spaces; in the loop that builds varExports
(variables vars, varName, varValue) change the export formation to safely
quote/escape varValue (e.g., wrap value in single quotes and escape any internal
single quotes using the shell-safe '\'' sequence) so the produced lines become
export VAR='escaped value' instead of export VAR=value.
🧹 Nitpick comments (2)
scripts/setupMultiplatformJdks.ts (2)
77-83:wget -Psaves with the URL-encoded filename — verify it matches expectations.
wget -Pderives the filename from the URL. Since the JDK URLs contain%2B(URL-encoded+),wgetmay save the file as...21.0.2%2B13...tar.gzrather than...21.0.2+13...tar.gz, whilepath.basename(jdk.url)on line 86 also preserves the%2B. So this should be consistent. However,curl -oon Windows uses the explicitdestPath, guaranteeing a match.One minor concern:
wgetmay not be available on all environments (e.g., minimal Docker images). Sincecurlis already used for Windows, consider usingcurluniformly.♻️ Optional: unify on curl for both platforms
async function downloadFile(url: string, destDir: string, destPath: string) { - if (process.platform === "win32") { - await $`curl -fsSL -o ${destPath} ${url}`; - } else { - await $`wget --quiet -P ${destDir} ${url}`; - } + await $`curl -fsSL -o ${destPath} ${url}`; }This also removes the unused
destDirparameter.
69-75: SHA-256 computation loads the entire file into memory.JDK archives are ~200 MB.
file.arrayBuffer()loads it all at once. For a CI-only script this is tolerable, but if memory becomes an issue, Bun supports streaming viafile.stream()with incrementalhasher.update()calls.
| let varExports = "# Generated by the JDK setup script."; | ||
| let ghVarExports = "# Generated by the JDK setup script."; | ||
| for (const envVar of vars) { | ||
| const varName = envVar[0]; | ||
| const varValue = envVar[1]; | ||
| for (const [varName, varValue] of vars) { | ||
| varExports += `\nexport ${varName}=${varValue}`; | ||
| ghVarExports += `\necho "${varName}=${varValue}" >> $GITHUB_ENV`; | ||
| } |
There was a problem hiding this comment.
Unquoted values in shell export may break paths with spaces.
Line 140 generates export VAR=value without quoting varValue. If a JDK path ever contains spaces, the generated shell script will break.
Proposed fix
- varExports += `\nexport ${varName}=${varValue}`;
+ varExports += `\nexport ${varName}="${varValue}"`;🤖 Prompt for AI Agents
In `@scripts/setupMultiplatformJdks.ts` around lines 138 - 141, The generated
shell export lines use unquoted values which breaks when paths contain spaces;
in the loop that builds varExports (variables vars, varName, varValue) change
the export formation to safely quote/escape varValue (e.g., wrap value in single
quotes and escape any internal single quotes using the shell-safe '\'' sequence)
so the produced lines become export VAR='escaped value' instead of export
VAR=value.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/build.yaml:
- Around line 60-63: The Build step currently runs the commands "./gradlew
publishToMavenLocal" and "./gradlew build" which fail on Windows because GitHub
Actions uses PowerShell by default; update the "Build" step to either force bash
for that step by adding "shell: bash" (so the existing "./gradlew ..." lines
work) or make the commands platform-aware by replacing those lines with a
conditional that uses ".\\gradlew ..." when runner.os == 'Windows' and
"./gradlew ..." otherwise; target the step named "Build" and the specific
commands "./gradlew publishToMavenLocal" and "./gradlew build" when applying the
change.
🧹 Nitpick comments (2)
buildSrc/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.kt (1)
65-72: The!!assertions are technically a TOCTOU with the earlier null-check, but acceptable here.The null-check at lines 18-31 and the
!!usage here call the same helpers which re-read env vars / local.properties each time. Consider caching the resolvedhomevalue from the earlier check and reusing it to avoid the redundant lookups and the!!.♻️ Suggested refactor
Store the resolved home path from the early check and pass it into
javaHomeArgdirectly:if (withJni) { - val home = when (platform) { + val jdkHome: String = when (platform) { Platform.windows_x64 -> windowX64JavaHome() Platform.linux_x64 -> linuxX64JavaHome() Platform.linux_aarch64 -> linuxAarch64JavaHome() Platform.macos_x64 -> macosX64JavaHome() Platform.macos_aarch64 -> macosAarch64JavaHome() else -> error("Unsupported platform: '$platform'") - } - if (home == null) { + } ?: run { println("Skip building JNI library for '$platform' because JDK is not found.") return } }Then later:
val generateArgs = if (withJni) { - when (platform) { - Platform.windows_x64 -> commonArgs + ninja + javaHomeArg(windowX64JavaHome()!!) - Platform.linux_x64 -> commonArgs + ninja + javaHomeArg(linuxX64JavaHome()!!) - ... - } + commonArgs + ninja + javaHomeArg(jdkHome) } else {This eliminates the duplicate
whenblock and all!!assertions.scripts/setupMultiplatformJdks.ts (1)
1-1:@ts-nocheckdisables all type safety for this file.This suppresses every TypeScript diagnostic, including genuine type errors that could cause runtime failures. If this is working around specific Bun type issues, prefer targeted
@ts-expect-erroror@ts-ignorecomments on the specific lines that need them.
| - name: Build | ||
| run: ./gradlew build | ||
| run: | | ||
| ./gradlew publishToMavenLocal | ||
| ./gradlew build |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat .github/workflows/build.yamlRepository: dokar3/quickjs-kt
Length of output: 1477
🌐 Web query:
gradle/actions/setup-gradle GitHub action Windows PowerShell ./gradlew
💡 Result:
On Windows runners, GitHub Actions defaults to PowerShell (pwsh), where the Gradle wrapper should be invoked as:
.\gradlew build(PowerShell)./gradlew build(Bash)
So if you’re using gradle/actions/setup-gradle, either change the command to PowerShell style, or force a Bash shell for that step.
PowerShell (default on Windows):
- uses: gradle/actions/setup-gradle@v5
- run: .\gradlew buildForce Bash on Windows (so ./gradlew works):
- uses: gradle/actions/setup-gradle@v5
- shell: bash
run: ./gradlew buildAlso note: setup-gradle (v4+) automatically performs Gradle Wrapper validation unless you disable it (e.g., validate-wrappers: false). (community.gradle.org)
Citations:
- 1: https://github.com/gradle/actions?utm_source=openai
- 2: https://github.com/gradle/actions?utm_source=openai
- 3: https://community.gradle.org/github-actions/docs/setup-gradle/?utm_source=openai
./gradlew will fail on Windows — the default shell is PowerShell, not Bash.
On Windows runners, GitHub Actions defaults to pwsh where ./gradlew must be invoked as .\gradlew (backslash syntax). The gradle/actions/setup-gradle action is already present but does not change the shell default, so the build will fail on the windows-latest matrix entry.
Proposed fix — force bash or use PowerShell syntax
Option 1: Force bash shell:
- name: Build
+ shell: bash
run: |
./gradlew publishToMavenLocal
./gradlew buildOption 2: Use PowerShell syntax:
- name: Build
run: |
- ./gradlew publishToMavenLocal
- ./gradlew build
+ .\gradlew publishToMavenLocal
+ .\gradlew build📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Build | |
| run: ./gradlew build | |
| run: | | |
| ./gradlew publishToMavenLocal | |
| ./gradlew build | |
| - name: Build | |
| shell: bash | |
| run: | | |
| ./gradlew publishToMavenLocal | |
| ./gradlew build |
| - name: Build | |
| run: ./gradlew build | |
| run: | | |
| ./gradlew publishToMavenLocal | |
| ./gradlew build | |
| - name: Build | |
| run: | | |
| .\gradlew publishToMavenLocal | |
| .\gradlew build |
🤖 Prompt for AI Agents
In @.github/workflows/build.yaml around lines 60 - 63, The Build step currently
runs the commands "./gradlew publishToMavenLocal" and "./gradlew build" which
fail on Windows because GitHub Actions uses PowerShell by default; update the
"Build" step to either force bash for that step by adding "shell: bash" (so the
existing "./gradlew ..." lines work) or make the commands platform-aware by
replacing those lines with a conditional that uses ".\\gradlew ..." when
runner.os == 'Windows' and "./gradlew ..." otherwise; target the step named
"Build" and the specific commands "./gradlew publishToMavenLocal" and "./gradlew
build" when applying the change.
1 participant
Pin zig to 0.12.0 and add integration tests, this should fix #170
Summary by CodeRabbit