feat(installer): root backend + Dhizuku Android 14+ fix (#553)#569
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a Root-based silent install/uninstall backend plus domain/status models, DI wiring, UI/state plumbing, Dhizuku Android‑14 handling, localized strings, and release-note updates. RootServiceManager implements probing, permission requests, and su-based pm install/uninstall. ChangesRoot Installation Backend Integration
sequenceDiagram
participant UI as Tweaks UI
participant VM as TweaksViewModel
participant ISP as InstallerStatusProvider
participant DISP as SilentInstallerDispatcher
participant ROOT as RootServiceManager
participant DH as DhizukuInstallerServiceImpl
UI->>VM: OnRequestRootPermission
VM->>ISP: requestRootPermission()
ISP->>ROOT: requestPermission()
ROOT-->>ISP: status update (StateFlow)
VM-->>UI: update rootAvailability
UI->>VM: try silent install (ROOT selected)
VM->>DISP: trySilentInstall(apk, installerAttribution)
DISP->>ROOT: installPackage(apk, installerAttribution)
ROOT-->>DISP: return 0 / -1 / null
DISP-->>VM: install result
note right of DISP: For DHIZUKU, handle STATUS_PENDING_USER_ACTION_REQUIRED and retry once without attribution
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/installer/SilentInstallerDispatcher.kt (1)
118-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
resolveActiveBackend()can shell out on the caller thread from synchronousuninstall().
resolveActiveBackend()(Line 253) callsrootServiceManager.refreshStatus()when the cached status is notREADY, and the comment at Line 267 acknowledges the probe "shells out and is slow".uninstall(packageName)(Line 118) is a non-suspendoverride funand invokesresolveActiveBackend()directly at Line 121 — before thescope.launch(Dispatchers.IO)at Line 123. If a caller invokesuninstall()from the main thread while ROOT is the selected backend in a non-READY cached state (e.g., after a manager revoked the grant, on first launch before the eager init has run, or after process restart), the probe runs on that thread.The
!= READYgate keeps the steady-state hot path fast, but the cold path still violates the “no blocking calls on request threads” guarantee. Same concern applies to the synchronous portion ofinstall()at Line 107 andensurePermissionsOrThrowat Line 94, though those havesuspendcallers that can be dispatched.Consider routing the probe through
withContext(Dispatchers.IO)(the suspend paths already have this pattern, see Line 140), or making the synchronousuninstall()launch into IO first and resolving the backend inside the coroutine.Also applies to: 253-276
🤖 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 `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/installer/SilentInstallerDispatcher.kt` around lines 118 - 131, The synchronous uninstall(packageName) currently calls resolveActiveBackend() on the caller thread which may shell out via rootServiceManager.refreshStatus(); change uninstall to perform the backend probe inside the IO dispatcher by moving the resolveActiveBackend() call into the coroutine (i.e., call scope.launch(Dispatchers.IO) { val backend = resolveActiveBackend(); when (backend) { ... silentUninstall(...) or androidInstaller.uninstall(...) } }), so the blocking probe runs off the request thread; apply the same pattern to the synchronous paths in install() and any non-suspending callers that call ensurePermissionsOrThrow so all probes that may call rootServiceManager.refreshStatus() run under Dispatchers.IO rather than on the caller thread.
🧹 Nitpick comments (2)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/installer/SilentInstallerDispatcher.kt (1)
207-211: ⚡ Quick winMirror constant duplicates a value declared elsewhere — drift risk.
DHIZUKU_STATUS_PENDING_USER_ACTION = -2is a hand-mirrored copy ofDhizukuInstallerServiceImpl.STATUS_PENDING_USER_ACTION_REQUIRED. If either side is edited without the other, the Android 14+ retry path silently stops triggering and Dhizuku falls back to the system prompt again — i.e., the exact regression this PR is fixing reappears, with no compile-time signal.Consider hoisting the status codes into a single source of truth (e.g., an object in the Dhizuku service package, or the AIDL/domain layer) and importing it on both sides so a change to the wire value is visible to the dispatcher.
🤖 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 `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/installer/SilentInstallerDispatcher.kt` around lines 207 - 211, Replace the hard-coded mirror constant DHIZUKU_STATUS_PENDING_USER_ACTION in SilentInstallerDispatcher with a single source of truth shared with the Dhizuku service: create (or use) a shared constant in the Dhizuku service/AIDL/domain layer (e.g., export DhizukuInstallerServiceImpl.STATUS_PENDING_USER_ACTION_REQUIRED into a shared object or interface accessible from both modules) and import that shared symbol here instead of the literal -2 so both sides read the same value; update references in SilentInstallerDispatcher.kt to use the shared constant name and remove the local const val to avoid drift.feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/components/sections/Installation.kt (1)
496-503: 💤 Low valueSame icon for DHIZUKU and ROOT options reduces visual distinction.
Icons.Outlined.Securityis already used for the DHIZUKU option at Line 477 and is now reused here for ROOT. Two adjacent radio options sharing the same icon makes it harder to scan and pick the right backend, especially since the four installer choices already differ only by subtle text. Consider a distinct icon for ROOT (e.g.,Icons.Outlined.AdminPanelSettings,Icons.Outlined.Shield,Icons.Outlined.Terminal, or a key/lock variant) so each option has a unique glyph.🤖 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 `@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/components/sections/Installation.kt` around lines 496 - 503, The ROOT installer option currently reuses Icons.Outlined.Security (same as the DHIZUKU option) which reduces visual distinction; update the InstallerOption for InstallerType.ROOT (the call to InstallerOption where isSelected checks selectedType == InstallerType.ROOT and statusBadge = { RootStatusBadge(...) }) to use a different icon from Icons.Outlined (for example Icons.Outlined.AdminPanelSettings, Icons.Outlined.Shield, Icons.Outlined.Terminal, or a key/lock variant) so the four installer choices each have a unique glyph and are easier to scan.
🤖 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
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/root/RootServiceManager.kt`:
- Around line 138-140: The current code in RootServiceManager uses blocking
readText() on proc.inputStream and proc.errorStream (variables stdout and
stderr) before calling proc.waitFor(INSTALL_TIMEOUT_SECONDS, TimeUnit.SECONDS),
which can block forever and bypass the timeout; change the approach to drain the
streams asynchronously (spawn two background threads or coroutines to read
proc.inputStream and proc.errorStream into buffers) or redirect the process
streams to avoid blocking, then call proc.waitFor(INSTALL_TIMEOUT_SECONDS,
TimeUnit.SECONDS) and only after waitFor completes join the reader threads and
collect their buffers; ensure you still enforce INSTALL_TIMEOUT_SECONDS and
handle the case where waitFor returns false by destroying the process.
- Around line 96-110: The code interpolates untrusted
installerPackageName/packageName into a shell string before calling
Runtime.getRuntime().exec, enabling arbitrary root shell execution; fix by
validating and sanitizing those inputs (e.g. enforce a strict Android
package-name regex like ^[a-zA-Z0-9_.]+$ and reasonable length limits for
installerPackageName and packageName in installPackage()), and then avoid shell
interpolation by invoking the binary with an argument list (use ProcessBuilder
or Runtime.exec(String[]) instead of embedding values into buildString for su
-c), or reliably shell-quote inputs before passing them; update the call sites
around installerPackageName, packageName, buildString and the
Runtime.getRuntime().exec(...) usage to perform validation and to pass safe,
non-interpolated arguments.
In `@core/presentation/src/commonMain/composeResources/files/whatsnew/ja/17.json`:
- Line 12: Replace the phrase "導入できる" with the more precise "インストールできる" in the
JSON entry containing the string "Root による静音インストール — Magisk / KernelSU / APatch
ユーザーは Shizuku や Dhizuku なしで導入できるようになりました。" so the text reads
"...なしでインストールできるようになりました。"; locate this exact JSON value to update (unique
string shown) and keep surrounding punctuation and spacing unchanged.
In `@core/presentation/src/commonMain/composeResources/files/whatsnew/pl/17.json`:
- Line 11: Replace the ambiguous phrase "fałszywe powiadomienie o aktualizacji"
in the JSON value string with an explicit false-positive wording used in other
locales, e.g., "fałszywy alarm aktualizacji" (so the sentence reads: "Pomiń
konkretną aktualizację — odrzuć fałszywy alarm aktualizacji; gdy pojawi się
nowsza wersja, zostaniesz powiadomiony ponownie."), ensuring the change updates
only the translated text in this JSON entry.
In `@core/presentation/src/commonMain/composeResources/values-ja/strings-ja.xml`:
- Line 988: The Japanese term is incorrect in the string resource
installer_type_root_description; replace "静音インストール" with the standard term
"サイレントインストール" so the value reads "Magisk / KernelSU / APatch root
によるサイレントインストール" and keep the rest of the string unchanged.
---
Outside diff comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/installer/SilentInstallerDispatcher.kt`:
- Around line 118-131: The synchronous uninstall(packageName) currently calls
resolveActiveBackend() on the caller thread which may shell out via
rootServiceManager.refreshStatus(); change uninstall to perform the backend
probe inside the IO dispatcher by moving the resolveActiveBackend() call into
the coroutine (i.e., call scope.launch(Dispatchers.IO) { val backend =
resolveActiveBackend(); when (backend) { ... silentUninstall(...) or
androidInstaller.uninstall(...) } }), so the blocking probe runs off the request
thread; apply the same pattern to the synchronous paths in install() and any
non-suspending callers that call ensurePermissionsOrThrow so all probes that may
call rootServiceManager.refreshStatus() run under Dispatchers.IO rather than on
the caller thread.
---
Nitpick comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/installer/SilentInstallerDispatcher.kt`:
- Around line 207-211: Replace the hard-coded mirror constant
DHIZUKU_STATUS_PENDING_USER_ACTION in SilentInstallerDispatcher with a single
source of truth shared with the Dhizuku service: create (or use) a shared
constant in the Dhizuku service/AIDL/domain layer (e.g., export
DhizukuInstallerServiceImpl.STATUS_PENDING_USER_ACTION_REQUIRED into a shared
object or interface accessible from both modules) and import that shared symbol
here instead of the literal -2 so both sides read the same value; update
references in SilentInstallerDispatcher.kt to use the shared constant name and
remove the local const val to avoid drift.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/components/sections/Installation.kt`:
- Around line 496-503: The ROOT installer option currently reuses
Icons.Outlined.Security (same as the DHIZUKU option) which reduces visual
distinction; update the InstallerOption for InstallerType.ROOT (the call to
InstallerOption where isSelected checks selectedType == InstallerType.ROOT and
statusBadge = { RootStatusBadge(...) }) to use a different icon from
Icons.Outlined (for example Icons.Outlined.AdminPanelSettings,
Icons.Outlined.Shield, Icons.Outlined.Terminal, or a key/lock variant) so the
four installer choices each have a unique glyph and are easier to scan.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02065db0-b0d7-456d-b48c-b144eef8e8e6
📒 Files selected for processing (41)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/di/PlatformModule.android.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/dhizuku/DhizukuInstallerServiceImpl.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/dhizuku/DhizukuServiceManager.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/installer/AndroidInstallerStatusProvider.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/installer/SilentInstallerDispatcher.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/root/RootServiceManager.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/root/model/RootStatus.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopInstallerStatusProvider.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/InstallerType.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/RootAvailability.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/InstallerStatusProvider.ktcore/presentation/src/commonMain/composeResources/files/whatsnew/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ar/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/bn/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/es/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/fr/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/hi/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/it/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ja/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ko/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/pl/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ru/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/tr/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/zh-CN/17.jsoncore/presentation/src/commonMain/composeResources/values-ar/strings-ar.xmlcore/presentation/src/commonMain/composeResources/values-bn/strings-bn.xmlcore/presentation/src/commonMain/composeResources/values-es/strings-es.xmlcore/presentation/src/commonMain/composeResources/values-fr/strings-fr.xmlcore/presentation/src/commonMain/composeResources/values-hi/strings-hi.xmlcore/presentation/src/commonMain/composeResources/values-it/strings-it.xmlcore/presentation/src/commonMain/composeResources/values-ja/strings-ja.xmlcore/presentation/src/commonMain/composeResources/values-ko/strings-ko.xmlcore/presentation/src/commonMain/composeResources/values-pl/strings-pl.xmlcore/presentation/src/commonMain/composeResources/values-ru/strings-ru.xmlcore/presentation/src/commonMain/composeResources/values-tr/strings-tr.xmlcore/presentation/src/commonMain/composeResources/values-zh-rCN/strings-zh-rCN.xmlcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksAction.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksState.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksViewModel.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/components/sections/Installation.kt
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/root/RootServiceManager.kt (1)
132-189: 💤 Low valueOptional: replace the raw
Threadplumbing with coroutines for consistency.
installPackage/uninstallPackagealready run insidewithContext(Dispatchers.IO)and the class owns aCoroutineScope, but the stdout/stderr/stdin pumps are hand-rolledThread { … }+Thread.join(timeoutMs). Switching toasync(Dispatchers.IO) { … }(orcoroutineScope { launch { … } }) gives you structured cancellation on coroutine cancel, removes the explicit join-then-destroyForciblyladder, and matches the rest of the class. Not a correctness issue — purely an idiom/maintainability win.♻️ Sketch (install path)
- val stdoutBuf = StringBuilder() - val stderrBuf = StringBuilder() - val stdoutThread = Thread { - try { - BufferedReader(InputStreamReader(proc.inputStream)).use { reader -> - reader.forEachLine { stdoutBuf.append(it).append('\n') } - } - } catch (_: Exception) { - } - } - val stderrThread = Thread { - try { - BufferedReader(InputStreamReader(proc.errorStream)).use { reader -> - reader.forEachLine { stderrBuf.append(it).append('\n') } - } - } catch (_: Exception) { - } - } - stdoutThread.start() - stderrThread.start() + val stdoutDeferred = async(Dispatchers.IO) { + runCatching { proc.inputStream.bufferedReader().readText() }.getOrDefault("") + } + val stderrDeferred = async(Dispatchers.IO) { + runCatching { proc.errorStream.bufferedReader().readText() }.getOrDefault("") + }…with the corresponding
withTimeoutOrNull(INSTALL_TIMEOUT_SECONDS.seconds) { stdoutDeferred.await() }after the process finishes.Also applies to: 215-231
🤖 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 `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/root/RootServiceManager.kt` around lines 132 - 189, Replace the raw Thread plumbing used in installPackage (and similarly in uninstallPackage) — specifically the stdoutThread, stderrThread and pipeThread that read proc.inputStream/proc.errorStream and copy apkFile.inputStream to proc.outputStream — with coroutine-friendly constructs using the class CoroutineScope and Dispatchers.IO: launch or async coroutines to read streams and copy input, use structured concurrency (coroutineScope/withTimeoutOrNull) to await their completion with the existing INSTALL_TIMEOUT_SECONDS/READER_DRAIN_TIMEOUT_MS semantics, and ensure you cancel/close the coroutines on timeout instead of manually interrupting threads or relying on destroyForcibly; keep the existing logging (Logger.e(TAG)) and status returns (STATUS_FAILURE) behavior when timeouts/errors occur.
🤖 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
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/root/RootServiceManager.kt`:
- Around line 132-189: Replace the raw Thread plumbing used in installPackage
(and similarly in uninstallPackage) — specifically the stdoutThread,
stderrThread and pipeThread that read proc.inputStream/proc.errorStream and copy
apkFile.inputStream to proc.outputStream — with coroutine-friendly constructs
using the class CoroutineScope and Dispatchers.IO: launch or async coroutines to
read streams and copy input, use structured concurrency
(coroutineScope/withTimeoutOrNull) to await their completion with the existing
INSTALL_TIMEOUT_SECONDS/READER_DRAIN_TIMEOUT_MS semantics, and ensure you
cancel/close the coroutines on timeout instead of manually interrupting threads
or relying on destroyForcibly; keep the existing logging (Logger.e(TAG)) and
status returns (STATUS_FAILURE) behavior when timeouts/errors occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c078b16a-60e5-407b-ad50-f1227d58b955
📒 Files selected for processing (6)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/installer/SilentInstallerDispatcher.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/root/RootServiceManager.ktcore/presentation/src/commonMain/composeResources/files/whatsnew/ja/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/pl/17.jsoncore/presentation/src/commonMain/composeResources/values-ja/strings-ja.xmlfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/components/sections/Installation.kt
✅ Files skipped from review due to trivial changes (3)
- core/presentation/src/commonMain/composeResources/files/whatsnew/ja/17.json
- core/presentation/src/commonMain/composeResources/values-ja/strings-ja.xml
- core/presentation/src/commonMain/composeResources/files/whatsnew/pl/17.json
🚧 Files skipped from review as they are similar to previous changes (2)
- core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/installer/SilentInstallerDispatcher.kt
- feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/components/sections/Installation.kt
Adds a 4th silent-install backend (Magisk / KernelSU / APatch) alongside Shizuku and Dhizuku, and fixes Dhizuku silently falling back to the system prompt on Android 14+ (issue #553).
Dhizuku fix (#553): Android 14+ update-ownership enforcement returns
STATUS_PENDING_USER_ACTIONwhensetInstallerPackageNamedoesn't match the existing installer-of-record. We now surface that distinctly and the dispatcher retries the install once without attribution — silent install succeeds, only the "installed by Play Store/F-Droid" label is lost.Dhizuku.initis also warmed at app start to dodge a race against the first install.Root backend: raw
su -c "pm install -S <size> -i <pkg> -"with stdin streaming (sidesteps SELinux on app-private paths). Probes the standard Magisk/KernelSU/APatchsulocations, triggers the manager's grant dialog viasu -c true, full path/system/bin/pmto avoid PATH stripping. Tweaks gets a 4th radio + status badge. Strings + what's-new × 13 locales.Closes #553. No new deps.
Summary by CodeRabbit
New Features
Improvements