fix(root): detect granted root on KernelSU/APatch#687
Conversation
Walkthrough
ChangesRoot Status Computation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Greptile SummaryThis PR revamps
Confidence Score: 3/5The cached-shell path is sound, but the fallback null→PERMISSION_NEEDED mapping reintroduces a known UX regression for non-rooted first-run users and risks a silent install-abort when computeStatus() returns READY via shell.isRoot but isRootGranted() disagrees. On first launch on a non-rooted device, getCachedShell() is null and isAppGrantedRoot() is null, so the fallback now resolves to PERMISSION_NEEDED — showing a Grant button that can never be granted. This is the exact regression PR #673 fixed. Additionally, isRootGranted() (which gates installPackage and uninstallPackage) still reads isAppGrantedRoot() == true, which may not agree with the new READY state derived from shell.isRoot on KernelSU, causing installs to silently abort even when the UI reports root is ready. RootServiceManager.kt — specifically the null branch of the isAppGrantedRoot() fallback (line 132) and the isRootGranted() helper (line 136). Important Files Changed
Sequence DiagramsequenceDiagram
participant App
participant RSM as RootServiceManager
participant Shell as libsu Shell
Note over App,Shell: initialize() / refreshStatus()
App->>RSM: initialize()
RSM->>RSM: configureDefaultShell()
RSM->>RSM: refreshStatusBlocking()
RSM->>RSM: computeStatus()
RSM->>Shell: getCachedShell()
alt Cached shell exists
Shell-->>RSM: Shell object
RSM->>Shell: shell.isRoot?
alt "shell.isRoot == true"
RSM-->>RSM: READY
else "shell.isRoot == false"
RSM-->>RSM: NOT_AVAILABLE
end
else No cached shell
Shell-->>RSM: null
RSM->>Shell: isAppGrantedRoot()
alt returns true
Shell-->>RSM: true
RSM-->>RSM: READY
else returns false or null
Shell-->>RSM: false / null
RSM-->>RSM: PERMISSION_NEEDED
end
end
Note over App,Shell: requestPermission() — user taps Grant
App->>RSM: requestPermission()
RSM->>Shell: Shell.getShell()
Shell-->>RSM: shell (root or non-root)
RSM->>RSM: refreshStatusBlocking()
RSM->>Shell: getCachedShell()
Shell-->>RSM: Shell object
RSM->>Shell: shell.isRoot?
alt KernelSU granted
Shell-->>RSM: true → READY
else No root
Shell-->>RSM: false → NOT_AVAILABLE
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return when (Shell.isAppGrantedRoot()) { | ||
| true -> RootStatus.READY | ||
| false -> RootStatus.PERMISSION_NEEDED | ||
| null -> RootStatus.NOT_AVAILABLE | ||
| false, null -> RootStatus.PERMISSION_NEEDED | ||
| } |
There was a problem hiding this comment.
On any device where no shell has ever been built (first launch, cache cleared, etc.), getCachedShell() returns null and isAppGrantedRoot() also returns null — because libsu hasn't probed su yet. With this change, that path now resolves to PERMISSION_NEEDED, surfacing a "Grant" button to users on completely non-rooted devices. PR #673 fixed exactly this by mapping null → NOT_AVAILABLE. The cached-shell guard introduced here only protects the case where a non-root shell has already been built and cached; it doesn't cover the pre-shell-creation state.
The ambiguity is fundamental: libsu's null can mean either "su present, not yet asked" (KernelSU) or "su absent, no probe attempted" (no root). Without attempting a shell, these two cases cannot be distinguished from isAppGrantedRoot() alone.
There was a problem hiding this comment.
Per libsu 6.0.0 Utils.isAppGrantedRoot(), a non-rooted device returns false, not null: the method eagerly walks System.getenv("PATH") and returns false the moment no su is found. null is returned only when an su binary is present in PATH but unconfirmed (the KernelSU case in #680). CodeRabbit's web-query in this same review agrees: "false: ... inability to find an executable su binary in the system's PATH."
So the "first-run non-rooted → null → Grant button" path doesn't occur — those devices hit false. And the false -> PERMISSION_NEEDED mapping is unchanged from main (#673 already ships it); this PR only changes null. No new behavior for non-rooted or Magisk-hidden-su devices.
There was a problem hiding this comment.
You're right, and I apologize for the incorrect analysis. Having now read the actual Utils.java source, the implementation confirms your point exactly:
for (String path : System.getenv("PATH").split(":")) {
File su = new File(path, "su");
if (su.canExecute()) {
currentRootState = 1;
return null; // su found, but grant unconfirmed
}
}
currentRootState = 0;
return false; // no su binary in PATH at allnull is only returned when an executable su binary is present in PATH but the grant state is unconfirmed (KernelSU/APatch). Non-rooted devices have no su in PATH and always get false. My regression concern was wrong — the null → PERMISSION_NEEDED mapping in this PR cannot surface a spurious Grant button on non-rooted devices.
The fix is correct. Withdrawing the concern.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
![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
🤖 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 130-133: The when-expression in RootServiceManager.kt currently
treats Shell.isAppGrantedRoot()'s false and null the same; change the mapping so
that Shell.isAppGrantedRoot() returns true -> RootStatus.READY, false ->
RootStatus.NOT_AVAILABLE, and null -> RootStatus.PERMISSION_NEEDED (i.e.
pattern-match the nullable Boolean explicitly rather than grouping false and
null together in the when inside the method that returns RootStatus).
🪄 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: ebeaf861-f894-4197-bb16-3de3fce096af
📒 Files selected for processing (1)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/root/RootServiceManager.kt
| return when (Shell.isAppGrantedRoot()) { | ||
| true -> RootStatus.READY | ||
| false -> RootStatus.PERMISSION_NEEDED | ||
| null -> RootStatus.NOT_AVAILABLE | ||
| false, null -> RootStatus.PERMISSION_NEEDED | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
libsu Shell.isAppGrantedRoot() return value false meaning root access impossible
💡 Result:
In the context of the libsu library for Android, the Shell.isAppGrantedRoot method returns a nullable Boolean value to indicate the application's root access state [1][2]. When this method returns false, it explicitly means that the library has determined it is impossible for the application to obtain root access [1]. According to the official libsu documentation and implementation details, the significance of the return values is as follows: false: The library has determined that root access is impossible to obtain [1]. This is a definitive state, often resulting from the library's inability to find an executable su binary in the system's PATH [3][4]. Because this result is considered 100% accurate regarding the impossibility of access, it will not change in future invocations [1][2]. true: The application has successfully created at least one root shell [1][2]. null: The library has not yet determined, or cannot currently determine, the root grant state [1][2]. This value is returned when the state is still pending or undetermined, such as when an su binary exists but a root shell has not been constructed or verified [1][2][4]. Future attempts or further shell operations may cause this to transition to true or false [1]. Developers are encouraged to use Shell.isAppGrantedRoot instead of the deprecated Shell.rootAccess method, as the former provides a more accurate and nuanced representation of the root state [1][2].
Citations:
- 1: https://github.com/topjohnwu/libsu/blob/master/CHANGELOG.md
- 2: https://github.com/topjohnwu/libsu/blob/master/core/src/main/java/com/topjohnwu/superuser/Shell.java
- 3: topjohnwu/libsu@8efbac3
- 4: [Bug] KernelSU fails root check with ksu Custom App Profile NeoApplications/Neo-Backup#796
Fix Shell.isAppGrantedRoot() null/false mapping to avoid showing a pointless “Grant” path
In libsu, Shell.isAppGrantedRoot() returns Boolean?: false means root access is impossible (definitive), while null means the state is unknown/pending. The current code maps both false and null to RootStatus.PERMISSION_NEEDED; false should map to RootStatus.NOT_AVAILABLE.
🐛 Proposed fix to split false (root impossible) from null (undetermined)
return when (Shell.isAppGrantedRoot()) {
true -> RootStatus.READY
- false, null -> RootStatus.PERMISSION_NEEDED
+ null -> RootStatus.PERMISSION_NEEDED
+ false -> RootStatus.NOT_AVAILABLE
}🤖 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 130 - 133, The when-expression in RootServiceManager.kt currently
treats Shell.isAppGrantedRoot()'s false and null the same; change the mapping so
that Shell.isAppGrantedRoot() returns true -> RootStatus.READY, false ->
RootStatus.NOT_AVAILABLE, and null -> RootStatus.PERMISSION_NEEDED (i.e.
pattern-match the nullable Boolean explicitly rather than grouping false and
null together in the when inside the method that returns RootStatus).
There was a problem hiding this comment.
false -> NOT_AVAILABLE would reintroduce #651. On Magisk ≥27 / Android 14+, su is hidden from non-granted callers, so the PATH probe finds nothing and isAppGrantedRoot() returns false even though root is present. #673 fixed exactly that by mapping false -> PERMISSION_NEEDED, so the Grant button still surfaces and Shell.getShell() can trigger the daemon prompt.
false is therefore ambiguous (Magisk hiding su vs. truly no root) and indistinguishable from the cheap check alone, so grouping false, null -> PERMISSION_NEEDED is intentional. The cached-shell guard downgrades to NOT_AVAILABLE once a built shell proves non-root — so a genuinely non-rooted device self-corrects to "No root" after one grant attempt, instead of dead-ending Magisk ≥27 users.
There was a problem hiding this comment.
@rainxchzed, that's a well-reasoned explanation — thank you for the detailed context. You're right and my suggestion was incorrect.
The key insight I missed: on Magisk ≥27 / Android 14+ the PATH-based probe (isAppGrantedRoot()) returns false even when root is present because the su binary is intentionally hidden from non-granted callers. Splitting false -> NOT_AVAILABLE would silently dead-end those users by never surfacing the Grant button, reintroducing #651. Since false and null are both "grant not yet confirmed" from the app's perspective — and only a live shell can prove non-root — the false, null -> PERMISSION_NEEDED grouping is correct, with the cached-shell guard serving as the reliable fallback to NOT_AVAILABLE for genuinely non-rooted devices.
I'm withdrawing this suggestion.
(╯°□°)╯ 🐇 ← caught mid-hop, humbly standing down
✏️ Learnings added
Learnt from: rainxchzed
Repo: OpenHub-Store/GitHub-Store PR: 687
File: core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/root/RootServiceManager.kt:130-133
Timestamp: 2026-05-29T05:47:57.395Z
Learning: In `RootServiceManager.computeStatus()` (file: `core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/root/RootServiceManager.kt`), grouping `false, null -> PERMISSION_NEEDED` for `Shell.isAppGrantedRoot()` is intentional. On Magisk ≥27 / Android 14+, `su` is hidden from non-granted callers, so the PATH probe returns nothing and `isAppGrantedRoot()` yields `false` even when root IS present. Mapping `false -> NOT_AVAILABLE` would dead-end Magisk ≥27 users by suppressing the Grant button (reintroduces issue `#651`). The correct approach (introduced in PR `#673`) is `false, null -> PERMISSION_NEEDED` so that `Shell.getShell()` can still trigger the daemon grant prompt. The cached-shell guard (`Shell.getCachedShell()`) then correctly downgrades to `NOT_AVAILABLE` once a built shell proves the device is truly non-rooted.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
Closes #680.
Root install showed "No root" on KernelSU even with root granted.
PR #673 mapped libsu's
isAppGrantedRoot()asnull -> NOT_AVAILABLE. Butnullmeans "su present, grant not yet confirmed" (KernelSU), andtrueis impossible until a shell is built — whichcomputeStatus()never did. So it could never reach READY.Fix:
null/false -> PERMISSION_NEEDED(surfaces the Grant button), and a cached-shell check returns READY once a confirmed root shell exists, else clean "No root". Keeps #673's #651 fix intact.Compile-verified. Untested on-device; community confirmation welcome.
Summary by CodeRabbit