fix(kotlin): verify native download checksums#508
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:
📝 WalkthroughWalkthroughAndroid release packaging now emits backend-specific ZIPs and checksum files, the release workflow requires them, and JNI download tasks use a shared helper to verify ZIP checksums before extraction. ChangesAndroid backend release packaging and publish checks
Shared checksum-verified download utility and JNI migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GradleTask as downloadJniLibs task
participant NativeLibraryDownload
participant ReleaseServer as release ZIP host
participant FileSystem as file system
GradleTask->>NativeLibraryDownload: downloadZipWithChecksum(zipUrl, tempZip)
NativeLibraryDownload->>ReleaseServer: GET zipUrl.sha256
ReleaseServer-->>NativeLibraryDownload: checksum text
NativeLibraryDownload->>ReleaseServer: GET zipUrl
ReleaseServer-->>NativeLibraryDownload: zip bytes
NativeLibraryDownload->>FileSystem: write tempZip
NativeLibraryDownload->>FileSystem: compute SHA-256
NativeLibraryDownload->>NativeLibraryDownload: compare checksums
alt mismatch or HTTP error
NativeLibraryDownload->>FileSystem: delete tempZip
NativeLibraryDownload-->>GradleTask: throw GradleException
else match
NativeLibraryDownload-->>GradleTask: return success
end
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 4
🤖 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/release.yml:
- Around line 789-790: The assertions for Android LlamaCPP and ONNX backend
artifacts only validate ZIP file presence but omit checking for required
`.sha256` sidecar files. Add additional `assert_present` calls after the
existing assertions on lines 789-790 to validate the corresponding `.sha256`
files for both the LlamaCPP and ONNX backend artifacts, using the same glob
patterns but with `.sha256` appended to match files like
RABackendLLAMACPP-android-*-v*.zip.sha256 and
RABackendONNX-android-*-v*.zip.sha256.
In `@sdk/runanywhere-commons/scripts/build-android.sh`:
- Around line 114-116: The conditional check at lines 114-116 silently skips the
operation when extra_file is set but the file does not exist at
${STAGING}/jni/${extra_file}. Make extra_file mandatory by modifying the logic
so that if extra_file is non-empty (requested), the script fails with an error
message if the file is not found at the expected location, rather than silently
continuing. Only proceed with the cp command if the file exists, but exit the
script with an appropriate error if extra_file is set but missing.
In `@sdk/runanywhere-kotlin/modules/runanywhere-core-llamacpp/build.gradle.kts`:
- Line 220: When the download and verification fails and throws a
GradleException at line 220, the already-copied .so files from previous
successful ABIs remain on disk. On subsequent task runs, the existence check at
line 183 will find these partial files and skip the download entirely, leaving
the build in an inconsistent state. Before throwing the GradleException in the
failure path, clean up any .so files that were already copied during this task
execution so that the next retry starts from a clean state and properly
re-downloads all required ABIs.
In `@sdk/runanywhere-kotlin/modules/runanywhere-core-onnx/build.gradle.kts`:
- Line 223: When a download fails for a specific ABI in the loop, the exception
at the GradleException throw statement leaves partial .so files in outputDir.
Before throwing the exception, clean up any incomplete files that were
downloaded for the current ABI iteration to prevent the early-exit check
(existingLibs > 0) from incorrectly skipping downloads on retry and using the
broken native library set. Ensure the cleanup happens in the catch block before
the GradleException is thrown.
🪄 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: e647cc76-5c66-43f9-86e2-3b994b3bd15c
📒 Files selected for processing (7)
.github/workflows/release.ymlsdk/runanywhere-commons/scripts/build-android.shsdk/runanywhere-kotlin/build.gradle.ktssdk/runanywhere-kotlin/buildSrc/build.gradle.ktssdk/runanywhere-kotlin/buildSrc/src/main/kotlin/com/runanywhere/gradle/NativeLibraryDownload.ktsdk/runanywhere-kotlin/modules/runanywhere-core-llamacpp/build.gradle.ktssdk/runanywhere-kotlin/modules/runanywhere-core-onnx/build.gradle.kts
b81c341 to
3d69032
Compare
|
Hi @shubhammalhotra28 - All 4 CodeRabbit review suggestions have been addressed in commit b81c341 (.sha256 sidecar validation in release.yml, mandatory extra_file check in build-android.sh, cleanup-on-failure for both backend build.gradle.kts modules). Please let me know if anything else is needed before this can be merged. |
|
Let us review your changes |
3d69032 to
8622e37
Compare
|
Hi @shubhammalhotra28 @Siddhesh2377 - just following up on this. CI is green now after the rebase. Please let me know if you want me to change anything when you get a chance. |
# Conflicts: # .github/workflows/release.yml # sdk/runanywhere-commons/scripts/build-android.sh # sdk/runanywhere-kotlin/build.gradle.kts # sdk/runanywhere-kotlin/modules/runanywhere-core-onnx/build.gradle.kts
# Conflicts: # .github/workflows/release.yml # sdk/runanywhere-commons/scripts/build-android.sh
8622e37 to
e23c66c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/release.yml (3)
284-288: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid direct template expansion of
versioninside the shell script.zizmor flags this as a template-injection risk:
${{ needs.validate.outputs.version }}is expanded directly into therun:block rather than passed through anenv:var, unlike theRAC_RELEASE_VERSIONpattern used innative_ios/native_android. Sinceversionis a workflow-controlled value, exploitability is low here, but aligning with the safer env-var pattern removes the flagged risk and improves consistency.🔒 Suggested fix
( cd dist - sha256sum "RACommons-windows-x64-v${{ needs.validate.outputs.version }}.zip" \ - > "RACommons-windows-x64-v${{ needs.validate.outputs.version }}.zip.sha256" + sha256sum "RACommons-windows-x64-v${VERSION}.zip" \ + > "RACommons-windows-x64-v${VERSION}.zip.sha256" )Add the corresponding
env:to the step (outside this range):- name: Package Windows outputs working-directory: sdk/runanywhere-commons shell: bash env: VERSION: ${{ needs.validate.outputs.version }} run: |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 284 - 288, Update the “Package Windows outputs” step to pass needs.validate.outputs.version through an env entry such as VERSION, then replace the direct GitHub template expansion in the shell commands with that environment variable when constructing the ZIP and checksum filenames.Source: Linters/SAST tools
342-350: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSame template-injection pattern as the Windows job — use an env var instead.
Same zizmor finding as the Windows checksum step:
${{ needs.validate.outputs.version }}is expanded directly intotar/shasumcommands here.🔒 Suggested fix
mkdir -p dist - tar czf "dist/RACommons-web-v${{ needs.validate.outputs.version }}.tar.gz" \ + tar czf "dist/RACommons-web-v${VERSION}.tar.gz" \ -C packages core/wasm llamacpp/wasm onnx/wasm ( cd dist - shasum -a 256 "RACommons-web-v${{ needs.validate.outputs.version }}.tar.gz" \ - > "RACommons-web-v${{ needs.validate.outputs.version }}.tar.gz.sha256" + shasum -a 256 "RACommons-web-v${VERSION}.tar.gz" \ + > "RACommons-web-v${VERSION}.tar.gz.sha256" )Add
env: VERSION: ${{ needs.validate.outputs.version }}to this step, mirroring theRAC_RELEASE_VERSIONpattern used elsewhere in the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 342 - 350, Add a step-level VERSION environment variable sourced from needs.validate.outputs.version, then update the tar archive and shasum filename references in the release step to use VERSION instead of directly interpolating the workflow expression, matching the existing RAC_RELEASE_VERSION pattern.Source: Linters/SAST tools
52-52: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHarden
actions/checkoutsteps withpersist-credentials: false.zizmor flags every
actions/checkoutinvocation in this workflow for not settingpersist-credentials: false. By default the checkout action writes the job'sGITHUB_TOKEN(or PAT) into local git config for the duration of the job; thepublishjob holdscontents: write, and several jobs clone third-party starter repos (swift/kotlin/react-native-starter-app). Disabling credential persistence on checkouts that don't need to push is a low-cost defense-in-depth improvement.🔒 Example fix (apply to each checkout step)
- uses: actions/checkout@v7 + with: + persist-credentials: falseAlso applies to: 98-98, 139-139, 170-170, 200-200, 226-226, 257-257, 306-306, 374-374, 404-404, 432-432, 523-524, 528-533, 631-631, 636-641, 934-934, 1053-1053, 1118-1118, 1125-1130, 1366-1366
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml at line 52, Update every actions/checkout step in the release workflow, including the listed checkout invocations, to set with persist-credentials disabled. Apply this consistently to checkout steps that clone the repository or third-party starter repos, without changing their existing checkout inputs or behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 284-288: Update the “Package Windows outputs” step to pass
needs.validate.outputs.version through an env entry such as VERSION, then
replace the direct GitHub template expansion in the shell commands with that
environment variable when constructing the ZIP and checksum filenames.
- Around line 342-350: Add a step-level VERSION environment variable sourced
from needs.validate.outputs.version, then update the tar archive and shasum
filename references in the release step to use VERSION instead of directly
interpolating the workflow expression, matching the existing RAC_RELEASE_VERSION
pattern.
- Line 52: Update every actions/checkout step in the release workflow, including
the listed checkout invocations, to set with persist-credentials disabled. Apply
this consistently to checkout steps that clone the repository or third-party
starter repos, without changing their existing checkout inputs or behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79139cdf-db89-4c18-af49-c9ed46ccb44e
📒 Files selected for processing (1)
.github/workflows/release.yml
|
Hey @shubhamsinnh there are few upgrades in ci-cd and kt releases so this pr has to wait |
|
Noted, @Siddhesh2377 thanks for the update. I have a few other PRs open for review. I would appreciate your feedback on them as well. happy to incorporate any changes or feedback you might have!! |
2 participants
Fixes #210
Summary
HttpURLConnectionwith a 30s connect timeout and 120s read timeout..sha256sidecar files and verify downloaded zips before extraction..sha256files from the release workflow for LlamaCPP and ONNX backend artifacts.Context
A previous PR, #416, attempted this issue but was closed unmerged. One blocker noted there was that backend
.sha256files did not exist on GitHub Releases, so checksum verification could not run end-to-end. This PR includes the release packaging changes needed for future releases to publish those backend checksum sidecars.Testing
Passed locally:
git diff --checkbash -n sdk/runanywhere-commons/scripts/build-android.sh./gradlew.bat buildSrc:build --no-daemon --console=plain./gradlew.bat help --no-daemon --stacktrace --console=plain./gradlew.bat test --no-daemon --console=plain -Prunanywhere.useLocalNatives=false -x downloadJniLibs -x :modules:runanywhere-core-llamacpp:downloadJniLibs -x :modules:runanywhere-core-onnx:downloadJniLibs./gradlew.bat lint --no-daemon --console=plain -Prunanywhere.useLocalNatives=false -x downloadJniLibs -x :modules:runanywhere-core-llamacpp:downloadJniLibs -x :modules:runanywhere-core-onnx:downloadJniLibs./gradlew.bat check --no-daemon --console=plain -Prunanywhere.useLocalNatives=false -x downloadJniLibs -x :modules:runanywhere-core-llamacpp:downloadJniLibs -x :modules:runanywhere-core-onnx:downloadJniLibs -x ktlintCheck -x ktlintMainSourceSetCheckKnown blockers observed locally:
checkcorrectly fails against the currentv0.19.13release becauseRABackendLLAMACPP-android-arm64-v8a-v0.19.13.zip.sha256returns HTTP 404. This PR fixes the release workflow for future releases, but cannot retroactively add sidecars to old releases.checkcurrently fails on unrelated ktlint violations inVoiceAgentMicDriver.ktandRunAnywhere.kt, which are outside this PR's scope.Unsupported host for Android NDK runtime lookup: Windows 10.Note
Medium Risk
Builds now hard-fail on bad or unverifiable native downloads, and release/Android packaging must produce new backend artifacts; supply-chain behavior changes but does not touch runtime auth or app logic.
Overview
Kotlin Gradle native fetch now verifies release zips before extraction: a shared
NativeLibraryDownloadhelper (in buildSrc) downloads each.zip.sha256sidecar, compares SHA-256, uses HTTP timeouts, and fails the build on HTTP/checksum errors instead of logging a warning. The root SDK and LlamaCPP/ONNX modules all use this path fordownloadJniLibs.Android release packaging now emits per-ABI
RABackendLLAMACPP-android-*andRABackendONNX-android-*archives (plus checksums) frombuild-android.sh, and the release workflow uploads them and requires them in the publish manifest—so future tags ship the sidecars backend downloads need. Older releases without those.sha256files will still break remote-native builds until a new release is cut.Reviewed by Cursor Bugbot for commit c89be6c. Configure here.
Summary by CodeRabbit
New Features
.sha256checksum files.Bug Fixes
Chores