ADFA-2498 Plumbing for plugin-artifacts.zip#792
Conversation
📝 WalkthroughRelease Notes - Plugin Artifacts InfrastructureChanges Overview
Risks & Best Practice Concerns
Recommended Action Items for Review
WalkthroughAdds bundling and distribution of plugin-artifact release assets: new Gradle task to produce compressed plugin-artifacts, GitHub Actions steps to create and optionally SCP-upload release assets, removes SSH setup/cleanup in release workflow, and app code to extract the new plugin-artifacts ZIP during installation. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Gradle as Gradle (runner)
participant Repo as Repository (assets/)
participant Server as Remote Server (SCP)
participant App as App Installer (runtime)
GH->>Gradle: run bundlePluginArtifactsForRelease (and V7/V8 bundles)
Gradle-->>Repo: write assets/release/.../plugin-artifacts.zip.br
alt SCP_HOST set and assets exist
GH->>Server: scp upload assets/release/* -> /remote/path
Server-->>GH: upload OK
else no SCP or missing assets
GH-->>GH: emit warning, skip upload
end
Note right of App: At runtime
App->>Repo: read bundled asset PLUGIN_ARTIFACTS_ZIP
App->>App: brotli-decompress -> ZipInputStream -> extract files (with path checks)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In @app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt:
- Around line 146-148: The code creates pluginDir from
Environment.PLUGIN_API_JAR.parentFile and calls pluginDir.mkdirs() without
checking the return value; update the BundledAssetsInstaller (where pluginDir is
computed from Environment.PLUGIN_API_JAR) to capture the boolean result of
pluginDir.mkdirs() and throw or log a clear IllegalStateException if it returns
false (and the directory does not already exist/is not a directory), so failures
to create the directory are detected and handled.
- Around line 157-161: The code writing ZIP entries uses pluginEntry.name
directly which allows path traversal; change the extraction to sanitize and
validate names: for each ZipEntry (pluginEntry) obtain a normalized relative
path (e.g., Paths.get(pluginEntry.name).normalize()), reject entries that are
absolute or that contain ".." or that resolve outside the intended pluginDir by
resolving pluginDir.resolve(normalizedRelative) and verifying the resulting
targetPath.toRealPath().startsWith(pluginDir.toRealPath()) (or compare
normalized paths), create parent directories as needed, handle directory entries
separately, and only open output streams for regular files; replace the direct
pluginDir.resolve(pluginEntry.name) and pluginZip.copyTo(output) usage with this
safe resolution + check before writing.
🧹 Nitpick comments (1)
.github/workflows/generate_assets.yml (1)
162-168: Task invocation is correct but slightly redundant.The step calls
bundleV7LlamaAssetsandbundleV8LlamaAssets, which depend onassembleV7AssetsandassembleV8Assetsrespectively. Since these were already invoked in the "Assemble Assets" step (line 156), Gradle will mark them as UP-TO-DATE and skip re-execution. This redundancy is safe but results in a slightly longer task resolution phase.The critical part—invoking
bundlePluginArtifactsForRelease—is correct and necessary.🔎 Optional optimization
If you want to eliminate the redundancy, you could call only the new task:
- name: Create Release Assets run: | - flox activate -d flox/base -- ./gradlew :app:bundleV7LlamaAssets :app:bundleV8LlamaAssets :app:bundlePluginArtifactsForRelease --no-daemon \ + flox activate -d flox/base -- ./gradlew :app:bundlePluginArtifactsForRelease --no-daemon \ -Dorg.gradle.jvmargs="-Xmx10g -XX:MaxMetaspaceSize=2g -XX:+HeapDumpOnOutOfMemoryError --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.io=ALL-UNNAMED" \ -Dandroid.aapt2.daemonHeapSize=4096M \ -Dorg.gradle.workers.max=1 \ -Dorg.gradle.parallel=falseHowever, explicitly listing all three tasks makes the workflow's intent clearer and documents what assets are being bundled, which may be preferable from a maintainability perspective.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/generate_assets.yml.github/workflows/release.yml.gitignoreapp/build.gradle.ktsapp/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt
💤 Files with no reviewable changes (2)
- .gitignore
- .github/workflows/release.yml
🔇 Additional comments (7)
app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt (2)
24-24: LGTM!The import is necessary for handling ZIP stream extraction in the new plugin artifacts logic.
181-181: The constantAssetsInstallationHelper.PLUGIN_ARTIFACTS_ZIPis properly defined in AssetsInstallationHelper.kt at line 60 with value"plugin-artifacts.zip"and follows the established naming convention. The asset size mapping at line 181 is correct.app/build.gradle.kts (4)
679-735: LGTM! Task follows established patterns.The
bundlePluginArtifactsForReleasetask implementation correctly mirrors the existingbundleLlamaAssetspattern:
- Proper dependency on
createPluginArtifactsZip- Robust Brotli availability check with graceful fallback
- Appropriate error handling for missing source files
- Clean file management (ensures only compressed or uncompressed version exists)
780-780: Dependency correctly added.The addition of
bundlePluginArtifactsForReleaseas a dependency ensures plugin artifacts are properly bundled for V8 release builds.
797-797: Dependency correctly added.The addition of
bundlePluginArtifactsForReleaseas a dependency ensures plugin artifacts are properly bundled for V7 release builds.
994-995: Asset entry correctly configured.The plugin-artifacts.zip.br asset is properly added to the release assets list with correct paths and URL. The entry follows the established pattern for release assets.
Note: Ensure that the corresponding
.md5checksum file is generated and uploaded to the server alongside the asset, consistent with how other release assets are handled (referenced inassetsFileChecksumfunction at lines 1074-1103)..github/workflows/generate_assets.yml (1)
170-186: LGTM! Proper error handling and guards.The upload step correctly handles missing configuration and missing directories:
- Checks for
SCP_HOSTbefore attempting upload- Verifies
assets/releasedirectory exists- Uses
exit 0to allow the workflow to continue gracefully if conditions aren't met- SCP command properly uploads the release assets to the target server
app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt (1)
116-118: Consider checkingmkdirs()result.While unlikely to fail, verifying the result of
mkdirs()would catch directory creation failures explicitly.🔎 Proposed improvement
val pluginDir = Environment.PLUGIN_API_JAR.parentFile ?: throw IllegalStateException("Plugin API parent directory is null") - pluginDir.mkdirs() + if (!pluginDir.exists() && !pluginDir.mkdirs()) { + throw IllegalStateException("Failed to create plugin directory: ${pluginDir.absolutePath}") + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.ktapp/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Universal APK
🔇 Additional comments (4)
app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt (1)
119-131: Excellent path traversal protection.The security implementation correctly prevents path traversal attacks by:
- Normalizing the base directory path
- Resolving each entry against the base
- Validating the resolved path stays within bounds
This approach effectively mitigates the risk of malicious ZIP entries escaping the target directory.
app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt (3)
24-24: LGTM!Import addition is appropriate for the new ZIP extraction functionality.
158-164: Path traversal protection correctly implemented.The security check properly prevents malicious ZIP entries from escaping the target directory by normalizing paths and validating with
startsWith(). This addresses the critical vulnerability from the previous review.
190-190: Expected size value added for plugin artifacts.The expected size
86442Lhas been added forAssetsInstallationHelper.PLUGIN_ARTIFACTS_ZIPin theexpectedSize()function at line 190. This value (~86 KB) represents the expected compressed size of the plugin artifacts asset and is used for progress tracking during installation. The value appears reasonable compared to other bundled assets, and the addition properly integrates with the existing asset size mapping in the when-expression.
hal-eisen-adfa
deleted the
ADFA-2498-Update-BundledAssetsInstaller-for-plugin-artifacts
branch
1 participant
Add creation and upload of plugins-artifacts.zip to generate_assets.yml
Finish removing SSH from release.yml
Tweak .gitignore to fix overly broad handling of "assets"
Add bundlePluginArtifactsForRelease to Gradle
Set up download URL for plugin-artifacts.zip.br
Add how to handle plugin-artifacts.zip to BundledAssetsInstaller.kt