feat: external storage on /sdcard/Oak/ with DocumentsProvider#16
Conversation
AI models and sandbox user files now live under /sdcard/Oak/, surviving app uninstall. The Alpine rootfs stays in app-private storage (ext4) because sdcardfs mounts with MS_NOEXEC, which is the same constraint Termux operates under. New files: ExternalStorage.kt — expect declarations ExternalStorage.android.kt — Android actual (MANAGE_EXTERNAL_STORAGE) ExternalStorage.desktop.kt — Desktop stub (no-op) Modified: AndroidManifest.xml — MANAGE_EXTERNAL_STORAGE permission OakApplication.kt — ensure directories at boot file_paths.xml — rename kai_home → sandbox_home InferencePlatform.android.kt — route models to /sdcard/Oak/models/ LinuxSandboxManager.kt — route home to /sdcard/Oak/sandbox-home/ On reinstall, the app rediscovers data at the well-known path via the .oak-location marker file — no SAF URI dependency. API 30+ only: MANAGE_EXTERNAL_STORAGE is required for /sdcard/ access with targetSdk 36. On API < 30 silently falls back to app-private storage (same as before). Legacy READ/WRITE_EXTERNAL_STORAGE permissions removed since they don't provide /sdcard/ access at this targetSdk level.
- writeOakLocationMarker: skip write if marker file already exists - resolveHome: check mkdirs() result in fallback branch, fall through to internal dir on failure - setup(): eagerly trigger homePath on IO so lazy init + legacy migration never blocks UI thread via transcriptFor - ExternalStorage.desktop: replace empty block with expression body
…vider config The PR routes sandbox-home to /sdcard/Oak/sandbox-home/ via MANAGE_EXTERNAL_STORAGE, but file_paths.xml only had an external-files-path entry (maps to Android/data/<pkg>/files/...). Any FileProvider.getUriForFile() call on the new path would throw IllegalArgumentException. Keep both entries: external-path for the MANAGE_EXTERNAL_STORAGE path, external-files-path for the fallback.
- Fix import ordering in HeartbeatNotifier.android.kt (spotless) - Fix import ordering in Platform.android.kt (spotless) - Implement missing DataRepository members in FakeDataRepository.kt - Rename PR title to conventional commits format
Implements OakDocumentsProvider extending DocumentsProvider to register the /sdcard/Oak/ directory (with models/ and sandbox-home/ children) as a root in Android's Storage Access Framework, making Oak visible as a storage entry in file managers. - queryRoots, queryChildDocuments, queryDocument, openDocument (read-only) - getDocumentMetadata, getDocumentType, isChildDocument - Registered in AndroidManifest.xml with DOCUMENTS_PROVIDER intent filter Re: PR #15
📝 WalkthroughWalkthroughAdds a multiplatform external-storage API, Android implementations that gate access and create Oak directories, an exported Oak DocumentsProvider registered in the manifest and file paths, and integrates external storage into app startup, model storage, and sandbox-home resolution. ChangesAndroid External Storage Access and DocumentsProvider Integration
Sequence Diagram(s)sequenceDiagram
participant Client as Android Picker/App
participant Provider as OakDocumentsProvider
participant Storage as External Oak Directory
Client->>Provider: queryRoots()
Provider->>Storage: count models, available bytes
Provider-->>Client: root cursor with metadata
Client->>Provider: queryChildDocuments(parentId)
Provider->>Storage: list files, filter non-hidden
Provider-->>Client: document cursor rows
Client->>Provider: openDocument(docId)
Provider->>Storage: verify path under Oak
Provider-->>Client: ParcelFileDescriptor (read-only)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@androidApp/src/main/AndroidManifest.xml`:
- Line 11: The manifest currently requests the high-risk
android.permission.MANAGE_EXTERNAL_STORAGE; remove or gate this permission from
release builds and add a policy justification. Update the AndroidManifest
handling so that android.permission.MANAGE_EXTERNAL_STORAGE is only present for
non-production flavors (e.g., add it to a debug/dev manifest or include it via a
productFlavor-specific manifest or Gradle config) and ensure the release
manifest does not include it (or explicitly remove it for release builds with
manifest merger rules/tools:node="remove"). Also add a short justification
document and the required Play Console permission declaration explaining why
all-files access is needed for debug/dev testing, and keep that justification
with the codebase/docs.
In `@composeApp/src/androidMain/kotlin/com/oak/app/ExternalStorage.android.kt`:
- Around line 16-17: The directory check currently treats any filesystem entry
as valid; change the validation in ensureExternalOakDirectories() so it verifies
both "models" and "sandbox-home" are directories (use File(...).isDirectory())
before calling writeOakLocationMarker(); update the ok variable logic to require
both isDirectory() to be true and keep the subsequent writeOakLocationMarker()
call guarded by that check.
In `@composeApp/src/androidMain/kotlin/com/oak/app/OakDocumentsProvider.kt`:
- Around line 35-44: The provider currently hardcodes user-visible text in
OakDocumentsProvider when building the root row (the title "Oak" and the summary
strings like "$totalItems folders" and "$modelCount models"); move these into
string resources and use Android resource APIs to fetch them. Add entries in
composeResources/values/strings.xml (e.g., oak_provider_title) and use plural
resources or formatted strings for folders/models (e.g., oak_folders_quantity,
oak_models_quantity), then update OakDocumentsProvider where c.addRow(...) is
called to build the summary with context.resources.getQuantityString(...) or
context.getString(...) instead of string literals so localization is supported.
Ensure the new resource keys match the names you add and replace the hardcoded
"Oak" title with the resource lookup for the provider title.
- Around line 131-134: The isUnderOak(File) function currently compares
file.absolutePath prefixes and is vulnerable to path-traversal; change it to
resolve canonical paths (use file.canonicalPath and baseDir.canonicalPath) and
then check equality or startsWith(baseCanonical + File.separator) to determine
containment, and wrap the canonicalization in a try/catch (or return false on
IOException) to safely handle failures; update references in isUnderOak and any
callers to rely on canonical containment checks rather than absolutePath string
prefixing.
In
`@composeApp/src/androidMain/kotlin/com/oak/app/sandbox/LinuxSandboxManager.kt`:
- Around line 65-68: The code in LinuxSandboxManager creating the internalTarget
File("home") ignores the boolean result of internalTarget.mkdirs(); update the
logic in the method that returns internalTarget.absolutePath to check the result
and/or existence of internalTarget after calling internalTarget.mkdirs() and
fail fast (throw an exception or return an error) if the directory could not be
created; reference the internalTarget variable and the migrateLegacyHome(...)
call so you ensure the check runs immediately after internalTarget.mkdirs() and
before returning internalTarget.absolutePath.
- Around line 52-55: In LinuxSandboxManager, change the existence check for the
sandbox home to ensure it is a directory (not just any file): replace the use of
dir.exists() with dir.isDirectory() (so the condition becomes something like
dir.isDirectory() || dir.mkdirs()), and if the path exists but is not a
directory handle it as a fallback/error (e.g., skip migrateLegacyHome/return and
let the subsequent fallback resolution run or log and fail). This ensures
migrateLegacyHome and the returned dir.absolutePath are only used for a valid
directory.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f58db52-63a2-40c8-a758-07baeff4c673
📒 Files selected for processing (12)
androidApp/src/main/AndroidManifest.xmlandroidApp/src/main/kotlin/com/oak/app/OakApplication.ktandroidApp/src/main/res/xml/file_paths.xmlcomposeApp/src/androidMain/kotlin/com/oak/app/ExternalStorage.android.ktcomposeApp/src/androidMain/kotlin/com/oak/app/HeartbeatNotifier.android.ktcomposeApp/src/androidMain/kotlin/com/oak/app/OakDocumentsProvider.ktcomposeApp/src/androidMain/kotlin/com/oak/app/Platform.android.ktcomposeApp/src/androidMain/kotlin/com/oak/app/inference/InferencePlatform.android.ktcomposeApp/src/androidMain/kotlin/com/oak/app/sandbox/LinuxSandboxManager.ktcomposeApp/src/commonMain/kotlin/com/oak/app/ExternalStorage.ktcomposeApp/src/commonTest/kotlin/com/oak/app/testutil/FakeDataRepository.ktcomposeApp/src/desktopMain/kotlin/com/oak/app/ExternalStorage.desktop.kt
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Unit Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Follow Kotlin best practices with consistent style, no wildcard/duplicate imports, and adhere to Kotlin idioms
No hardcoded strings — usecomposeResources/values/strings.xmlfor all user-facing text
Use standard M3 Material APIs directly such asModalDrawerSheetandNavigationDrawerItemwithout custom wrappers
Do not add gradients — they are being removed from the design system
UseOakUiNodeandOakUiRendererto render AI-generated interactive UI blocks using theoak-uifenced block syntax
Files:
composeApp/src/androidMain/kotlin/com/oak/app/HeartbeatNotifier.android.ktcomposeApp/src/desktopMain/kotlin/com/oak/app/ExternalStorage.desktop.ktcomposeApp/src/commonMain/kotlin/com/oak/app/ExternalStorage.ktcomposeApp/src/androidMain/kotlin/com/oak/app/inference/InferencePlatform.android.ktcomposeApp/src/commonTest/kotlin/com/oak/app/testutil/FakeDataRepository.ktcomposeApp/src/androidMain/kotlin/com/oak/app/ExternalStorage.android.ktcomposeApp/src/androidMain/kotlin/com/oak/app/OakDocumentsProvider.ktcomposeApp/src/androidMain/kotlin/com/oak/app/sandbox/LinuxSandboxManager.ktandroidApp/src/main/kotlin/com/oak/app/OakApplication.ktcomposeApp/src/androidMain/kotlin/com/oak/app/Platform.android.kt
**/{*.kt,*.xml}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
material-icons-extendedover custom drawables for icons
Files:
composeApp/src/androidMain/kotlin/com/oak/app/HeartbeatNotifier.android.ktcomposeApp/src/desktopMain/kotlin/com/oak/app/ExternalStorage.desktop.ktandroidApp/src/main/AndroidManifest.xmlandroidApp/src/main/res/xml/file_paths.xmlcomposeApp/src/commonMain/kotlin/com/oak/app/ExternalStorage.ktcomposeApp/src/androidMain/kotlin/com/oak/app/inference/InferencePlatform.android.ktcomposeApp/src/commonTest/kotlin/com/oak/app/testutil/FakeDataRepository.ktcomposeApp/src/androidMain/kotlin/com/oak/app/ExternalStorage.android.ktcomposeApp/src/androidMain/kotlin/com/oak/app/OakDocumentsProvider.ktcomposeApp/src/androidMain/kotlin/com/oak/app/sandbox/LinuxSandboxManager.ktandroidApp/src/main/kotlin/com/oak/app/OakApplication.ktcomposeApp/src/androidMain/kotlin/com/oak/app/Platform.android.kt
🪛 detekt (1.23.8)
composeApp/src/commonTest/kotlin/com/oak/app/testutil/FakeDataRepository.kt
[warning] 597-597: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
[warning] 599-599: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🔇 Additional comments (9)
composeApp/src/androidMain/kotlin/com/oak/app/HeartbeatNotifier.android.kt (1)
9-9: LGTM!composeApp/src/androidMain/kotlin/com/oak/app/Platform.android.kt (1)
59-59: LGTM!composeApp/src/commonTest/kotlin/com/oak/app/testutil/FakeDataRepository.kt (1)
593-599: LGTM!composeApp/src/commonMain/kotlin/com/oak/app/ExternalStorage.kt (1)
3-7: LGTM!composeApp/src/desktopMain/kotlin/com/oak/app/ExternalStorage.desktop.kt (1)
3-7: LGTM!androidApp/src/main/kotlin/com/oak/app/OakApplication.kt (1)
23-24: LGTM!androidApp/src/main/res/xml/file_paths.xml (1)
6-7: LGTM!composeApp/src/androidMain/kotlin/com/oak/app/inference/InferencePlatform.android.kt (1)
15-22: LGTM!composeApp/src/androidMain/kotlin/com/oak/app/sandbox/LinuxSandboxManager.kt (1)
9-9: LGTM!Also applies to: 41-51, 56-64, 71-83, 120-123
| <uses-permission android:name="com.android.alarm.permission.SET_ALARM"/> | ||
| <uses-permission android:name="android.permission.FOREGROUND_SERVICE"/> | ||
| <uses-permission android:name="android.permission.FOREGROUND_SERVICE_DATA_SYNC"/> | ||
| <uses-permission android:name="android.permission.MANAGE_EXTERNAL_STORAGE"/> |
There was a problem hiding this comment.
MANAGE_EXTERNAL_STORAGE needs explicit release gating and policy justification.
Line 11 adds an all-files access permission that is high-risk for Play compliance/privacy review. Without strict gating (build flavor/channel) and documented justification, release builds can be blocked.
🤖 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 `@androidApp/src/main/AndroidManifest.xml` at line 11, The manifest currently
requests the high-risk android.permission.MANAGE_EXTERNAL_STORAGE; remove or
gate this permission from release builds and add a policy justification. Update
the AndroidManifest handling so that android.permission.MANAGE_EXTERNAL_STORAGE
is only present for non-production flavors (e.g., add it to a debug/dev manifest
or include it via a productFlavor-specific manifest or Gradle config) and ensure
the release manifest does not include it (or explicitly remove it for release
builds with manifest merger rules/tools:node="remove"). Also add a short
justification document and the required Play Console permission declaration
explaining why all-files access is needed for debug/dev testing, and keep that
justification with the codebase/docs.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces external storage support for the Android application, allowing models and sandbox home directories to be stored in a shared 'Oak' directory. It includes the implementation of a DocumentsProvider to expose these files and migration logic for existing data. Feedback focused on the high risk of using the restricted MANAGE_EXTERNAL_STORAGE permission, which could lead to Google Play Store rejection. Additionally, technical improvements were suggested for the DocumentsProvider to correctly report usable space and use canonical paths for safer file comparisons.
| <uses-permission android:name="com.android.alarm.permission.SET_ALARM"/> | ||
| <uses-permission android:name="android.permission.FOREGROUND_SERVICE"/> | ||
| <uses-permission android:name="android.permission.FOREGROUND_SERVICE_DATA_SYNC"/> | ||
| <uses-permission android:name="android.permission.MANAGE_EXTERNAL_STORAGE"/> |
There was a problem hiding this comment.
The MANAGE_EXTERNAL_STORAGE permission is highly restricted by Google Play. Unless the app's core functionality is a file manager or backup tool, it is likely to be rejected during the review process. Consider if the app can achieve its goals using the Storage Access Framework (SAF) or getExternalFilesDir() without this broad permission.
| "Oak", | ||
| summary, | ||
| FLAG_ROOT, | ||
| root.totalSpace, |
There was a problem hiding this comment.
The COLUMN_AVAILABLE_BYTES should represent the remaining free space on the storage, not the total capacity. Using root.totalSpace provides the total size of the partition, which is misleading for this column.
| root.totalSpace, | |
| root.usableSpace, |
| return File(documentId).absolutePath.startsWith( | ||
| File(parentDocumentId).absolutePath + File.separator | ||
| ) |
There was a problem hiding this comment.
The isChildDocument implementation should also account for the case where documentId is exactly equal to parentDocumentId if the contract requires testing for descendants (including the root itself in some contexts). Additionally, using File.canonicalPath is safer for path comparisons to avoid issues with symbolic links or redundant separators.
| return File(documentId).absolutePath.startsWith( | |
| File(parentDocumentId).absolutePath + File.separator | |
| ) | |
| val parentPath = File(parentDocumentId).canonicalPath | |
| val childPath = File(documentId).canonicalPath | |
| return childPath == parentPath || childPath.startsWith(parentPath + File.separator) |
| "Oak", | ||
| summary, | ||
| FLAG_ROOT, | ||
| root.totalSpace, |
There was a problem hiding this comment.
This should be root.usableSpace (or freeSpace). totalSpace shows the whole partition size, not what's available — file managers display this as "free space" so it'd be lying.
| override fun isChildDocument(parentDocumentId: String, documentId: String): Boolean { | ||
| return File(documentId).absolutePath.startsWith( | ||
| File(parentDocumentId).absolutePath + File.separator | ||
| ) |
There was a problem hiding this comment.
Same canonical path concern as isUnderOak — if a malicious app crafts a document ID with ../, this'd say it's a child when it's not. Also doesn't handle the case where documentId == parentDocumentId (is a document a child of itself?). Throw a canonicalPath on both sides.
| private fun isUnderOak(file: File): Boolean { | ||
| return file.absolutePath == baseDir.absolutePath || | ||
| file.absolutePath.startsWith(baseDir.absolutePath + File.separator) | ||
| } |
There was a problem hiding this comment.
absolutePath won't resolve ../ segments — a crafted document ID could escape the Oak directory. Use canonicalPath for both sides to normalize path traversal attempts.
| val root = getExternalOakRoot() ?: return false | ||
| File(root, "models").mkdirs() | ||
| File(root, "sandbox-home").mkdirs() | ||
| val ok = File(root, "models").exists() && File(root, "sandbox-home").exists() |
There was a problem hiding this comment.
.exists() returns true for regular files too. If something creates a file named models instead of a directory, this returns success and everything downstream breaks. Use .isDirectory like you did in OakDocumentsProvider.queryRoots.
| import com.oak.app.getExternalOakRoot | ||
| import org.koin.java.KoinJavaComponent.inject | ||
| import java.io.File | ||
|
|
There was a problem hiding this comment.
Same .exists() issue as the other spots — if a file exists at litert_models instead of a directory, this returns the path and downstream file ops fail. Swap to .isDirectory for consistency.
| if (dir.exists() || dir.mkdirs()) { | ||
| migrateLegacyHome(dir) | ||
| return dir.absolutePath | ||
| } |
There was a problem hiding this comment.
.exists() → .isDirectory() same pattern as ExternalStorage.android.kt.
| internalTarget.mkdirs() | ||
| migrateLegacyHome(internalTarget) | ||
| return internalTarget.absolutePath | ||
| } |
There was a problem hiding this comment.
mkdirs() result is unchecked — if it fails, the path doesn't exist but you return it anyway and the sandbox blows up later in a confusing way. Either throw here or fall back.
- Use isDirectory instead of exists() so regular files don't fool the check - Fail fast with IllegalStateException when internal sandbox home cannot be created
- Replace absolutePath with canonicalPath in isUnderOak() to prevent path traversal via .. - Replace absolutePath with canonicalPath in isChildDocument() for consistency
- COLUMN_AVAILABLE_BYTES should report free space (usableSpace), not total capacity
- Move 'Oak', 'X folders', 'Y models' strings to strings.xml (project convention)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@composeApp/src/androidMain/kotlin/com/oak/app/OakDocumentsProvider.kt`:
- Around line 112-116: The isChildDocument function calls File.canonicalPath
which can throw IOException and should be wrapped in a try/catch like isUnderOak
to avoid crashing; update isChildDocument (and confirm isUnderOak) to perform
canonicalization inside a try block, compare the resulting canonical paths
(parent canonical + File.separator) and return false if any IOException (or
other File-related exception) occurs, ensuring the provider safely returns false
on failure instead of propagating the exception.
- Around line 134-137: The isUnderOak(File) method can throw when accessing
file.canonicalPath; wrap the canonicalPath accesses in a try/catch (catch
IOException and SecurityException) inside isUnderOak and return false if
resolving either path fails so provider operations don't crash; use the same
approach as the prior fix for canonical containment (compare resolved
canonicalPath of file against baseDir.canonicalPath and baseDir.canonicalPath +
File.separator) but guarded by the try/catch around the calls to
file.canonicalPath and baseDir.canonicalPath.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0c0dcd2-830f-41f2-bd6c-b3443c6ba6b7
📒 Files selected for processing (4)
composeApp/src/androidMain/kotlin/com/oak/app/ExternalStorage.android.ktcomposeApp/src/androidMain/kotlin/com/oak/app/OakDocumentsProvider.ktcomposeApp/src/androidMain/kotlin/com/oak/app/sandbox/LinuxSandboxManager.ktcomposeApp/src/androidMain/res/values/strings.xml
✅ Files skipped from review due to trivial changes (1)
- composeApp/src/androidMain/res/values/strings.xml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Run Unit Tests
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Follow Kotlin best practices with consistent style, no wildcard/duplicate imports, and adhere to Kotlin idioms
No hardcoded strings — usecomposeResources/values/strings.xmlfor all user-facing text
Use standard M3 Material APIs directly such asModalDrawerSheetandNavigationDrawerItemwithout custom wrappers
Do not add gradients — they are being removed from the design system
UseOakUiNodeandOakUiRendererto render AI-generated interactive UI blocks using theoak-uifenced block syntax
Files:
composeApp/src/androidMain/kotlin/com/oak/app/sandbox/LinuxSandboxManager.ktcomposeApp/src/androidMain/kotlin/com/oak/app/ExternalStorage.android.ktcomposeApp/src/androidMain/kotlin/com/oak/app/OakDocumentsProvider.kt
**/{*.kt,*.xml}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
material-icons-extendedover custom drawables for icons
Files:
composeApp/src/androidMain/kotlin/com/oak/app/sandbox/LinuxSandboxManager.ktcomposeApp/src/androidMain/kotlin/com/oak/app/ExternalStorage.android.ktcomposeApp/src/androidMain/kotlin/com/oak/app/OakDocumentsProvider.kt
🔇 Additional comments (3)
composeApp/src/androidMain/kotlin/com/oak/app/ExternalStorage.android.kt (1)
7-19: LGTM!composeApp/src/androidMain/kotlin/com/oak/app/OakDocumentsProvider.kt (1)
26-53: LGTM!composeApp/src/androidMain/kotlin/com/oak/app/sandbox/LinuxSandboxManager.kt (1)
48-71: LGTM!
| override fun isChildDocument(parentDocumentId: String, documentId: String): Boolean { | ||
| return File(documentId).canonicalPath.startsWith( | ||
| File(parentDocumentId).canonicalPath + File.separator | ||
| ) | ||
| } |
There was a problem hiding this comment.
Missing exception handling for canonicalPath resolution.
File.canonicalPath can throw IOException (e.g., symlink loops, permission issues). If this throws, the provider crashes instead of returning false safely. The same pattern applies to isUnderOak() below, though at least there the past review suggested wrapping in try/catch.
🛡️ Suggested defensive fix
override fun isChildDocument(parentDocumentId: String, documentId: String): Boolean {
+ return try {
File(documentId).canonicalPath.startsWith(
File(parentDocumentId).canonicalPath + File.separator
)
+ } catch (_: Exception) {
+ false
+ }
}🤖 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 `@composeApp/src/androidMain/kotlin/com/oak/app/OakDocumentsProvider.kt` around
lines 112 - 116, The isChildDocument function calls File.canonicalPath which can
throw IOException and should be wrapped in a try/catch like isUnderOak to avoid
crashing; update isChildDocument (and confirm isUnderOak) to perform
canonicalization inside a try block, compare the resulting canonical paths
(parent canonical + File.separator) and return false if any IOException (or
other File-related exception) occurs, ensuring the provider safely returns false
on failure instead of propagating the exception.
| private fun isUnderOak(file: File): Boolean { | ||
| return file.canonicalPath == baseDir.canonicalPath || | ||
| file.canonicalPath.startsWith(baseDir.canonicalPath + File.separator) | ||
| } |
There was a problem hiding this comment.
Same exception-safety gap in isUnderOak().
The original review suggestion for canonical path containment included a try/catch wrapper, but it wasn't applied here. If canonicalPath throws, this propagates up and breaks provider operations.
🛡️ Suggested fix matching the original review guidance
private fun isUnderOak(file: File): Boolean {
+ return try {
- return file.canonicalPath == baseDir.canonicalPath ||
+ file.canonicalPath == baseDir.canonicalPath ||
file.canonicalPath.startsWith(baseDir.canonicalPath + File.separator)
+ } catch (_: Exception) {
+ false
+ }
}📝 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.
| private fun isUnderOak(file: File): Boolean { | |
| return file.canonicalPath == baseDir.canonicalPath || | |
| file.canonicalPath.startsWith(baseDir.canonicalPath + File.separator) | |
| } | |
| private fun isUnderOak(file: File): Boolean { | |
| return try { | |
| file.canonicalPath == baseDir.canonicalPath || | |
| file.canonicalPath.startsWith(baseDir.canonicalPath + File.separator) | |
| } catch (_: Exception) { | |
| false | |
| } | |
| } |
🤖 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 `@composeApp/src/androidMain/kotlin/com/oak/app/OakDocumentsProvider.kt` around
lines 134 - 137, The isUnderOak(File) method can throw when accessing
file.canonicalPath; wrap the canonicalPath accesses in a try/catch (catch
IOException and SecurityException) inside isUnderOak and return false if
resolving either path fails so provider operations don't crash; use the same
approach as the prior fix for canonical containment (compare resolved
canonicalPath of file against baseDir.canonicalPath and baseDir.canonicalPath +
File.separator) but guarded by the try/catch around the calls to
file.canonicalPath and baseDir.canonicalPath.
2 participants
Completes the external storage feature properly.
Changes
/sdcard/Oak/(survives app uninstall)OakDocumentsProvider— registers a Storage Access Framework provider so Oak shows up as its own storage entry in Android file managers (like Termux does)Why
Without the DocumentsProvider,
/sdcard/Oak/was invisible in file manager UIs. Now it appears as "Oak" in the storage sidebar.What shows up
AI modelssandbox-home/Summary by CodeRabbit