[codex] Fix full disk access detection#4825
Conversation
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Ready to review this PR? Stage has broken it down into 3 individual chapters for you:
Chapters generated by Stage for commit 9b511bf on May 22, 2026 12:19am UTC. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughExtracts Full Disk Access probing into a utility, implements a native-permissions module that aggregates permission status checks and request helpers (including microphone askForMediaAccess behavior), adds tests for both utilities, and updates the permissions router to delegate status and request operations to the new helpers. ChangesPermissions refactor & native helpers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 replaces a single-file TCC probe (
Confidence Score: 4/5Safe to merge; the multi-probe approach is a clear improvement and the only theoretical concern is a narrow class of non-permission OS errors being misclassified. The change is well-structured and the test suite covers all four key code paths. The one concern — transient system errors like EMFILE or EIO being treated as "FDA denied" — requires an unlikely combination of conditions and would not cause data loss, only a misleading permissions prompt. apps/desktop/src/lib/trpc/routers/permissions/full-disk-access.ts — specifically the isFileNotFoundError guard and how non-ENOENT, non-permission errors are handled.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/lib/trpc/routers/permissions/full-disk-access.ts | New FDA detection module that probes multiple TCC-protected files; logic is sound but non-ENOENT/non-permission errors (EIO, EMFILE, ENOTDIR) are silently mapped to "FDA denied". |
| apps/desktop/src/lib/trpc/routers/permissions/full-disk-access.test.ts | Comprehensive unit tests covering the four main code paths (success, ENOENT fallthrough, permission error, all absent); no issues found. |
| apps/desktop/src/lib/trpc/routers/permissions.ts | Cleanly removes the inline single-probe implementation and delegates to the new module; no logic changes to the router itself. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[checkFullDiskAccess called] --> B[Build probe path list from homeDirectory]
B --> C{More probe paths?}
C -- Yes --> D[openForRead probePath]
D -- success --> E[return true FDA granted]
D -- throws ENOENT --> C
D -- throws other error e.g. EPERM EACCES --> F[return false FDA denied]
C -- No more paths --> G[return false no probe file found]
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/desktop/src/lib/trpc/routers/permissions/full-disk-access.ts:19-25
Non-filesystem errors such as `EMFILE` (too many open file descriptors), `EIO` (I/O error), or `ENOTDIR` (a path component is not a directory) would fall through to `return false` here, falsely reporting that FDA is unavailable. These are transient system-resource or path-resolution failures, not TCC denials. Treating them as permission errors can strand users in the permissions onboarding screen for no real reason.
```suggestion
function isSkippableError(error: unknown): boolean {
if (!(error instanceof Error) || !("code" in error)) {
return false;
}
const code = (error as NodeJS.ErrnoException).code;
// ENOENT – probe file genuinely absent, try the next one.
// ENOTDIR – a path component is missing (e.g. ~/Library/Safari/ never created).
return code === "ENOENT" || code === "ENOTDIR";
}
```
Reviews (1): Last reviewed commit: "Fix full disk access detection" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function isFileNotFoundError(error: unknown): boolean { | ||
| return ( | ||
| error instanceof Error && | ||
| "code" in error && | ||
| (error as NodeJS.ErrnoException).code === "ENOENT" | ||
| ); | ||
| } |
There was a problem hiding this comment.
EMFILE (too many open file descriptors), EIO (I/O error), or ENOTDIR (a path component is not a directory) would fall through to return false here, falsely reporting that FDA is unavailable. These are transient system-resource or path-resolution failures, not TCC denials. Treating them as permission errors can strand users in the permissions onboarding screen for no real reason.
| function isFileNotFoundError(error: unknown): boolean { | |
| return ( | |
| error instanceof Error && | |
| "code" in error && | |
| (error as NodeJS.ErrnoException).code === "ENOENT" | |
| ); | |
| } | |
| function isSkippableError(error: unknown): boolean { | |
| if (!(error instanceof Error) || !("code" in error)) { | |
| return false; | |
| } | |
| const code = (error as NodeJS.ErrnoException).code; | |
| // ENOENT – probe file genuinely absent, try the next one. | |
| // ENOTDIR – a path component is missing (e.g. ~/Library/Safari/ never created). | |
| return code === "ENOENT" || code === "ENOTDIR"; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/lib/trpc/routers/permissions/full-disk-access.ts
Line: 19-25
Comment:
Non-filesystem errors such as `EMFILE` (too many open file descriptors), `EIO` (I/O error), or `ENOTDIR` (a path component is not a directory) would fall through to `return false` here, falsely reporting that FDA is unavailable. These are transient system-resource or path-resolution failures, not TCC denials. Treating them as permission errors can strand users in the permissions onboarding screen for no real reason.
```suggestion
function isSkippableError(error: unknown): boolean {
if (!(error instanceof Error) || !("code" in error)) {
return false;
}
const code = (error as NodeJS.ErrnoException).code;
// ENOENT – probe file genuinely absent, try the next one.
// ENOTDIR – a path component is missing (e.g. ~/Library/Safari/ never created).
return code === "ENOENT" || code === "ENOTDIR";
}
```
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Kitenite
force-pushed
the
fix-disk-access-detection
branch
from
c1e0eeb to
690ddcc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/permissions/full-disk-access.test.ts (1)
102-115: ⚡ Quick winAdd an explicit
EACCESpermission-denied test.You already pin
EPERM; addingEACCEScloses the permission-denied regression gap for this helper’s contract.Proposed test addition
@@ it("returns false when an existing protected file cannot be opened", () => { const openedPaths: string[] = []; @@ expect(hasFullDiskAccess).toBe(false); expect(openedPaths).toEqual([tccDatabasePath]); }); + + it("returns false when an existing protected file is not readable (EACCES)", () => { + const openedPaths: string[] = []; + + const hasFullDiskAccess = checkFullDiskAccess({ + homeDirectory, + readProbe: (filePath) => { + openedPaths.push(filePath); + throw createFileSystemError("EACCES"); + }, + }); + + expect(hasFullDiskAccess).toBe(false); + expect(openedPaths).toEqual([tccDatabasePath]); + });🤖 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 `@apps/desktop/src/lib/trpc/routers/permissions/full-disk-access.test.ts` around lines 102 - 115, Add a new test alongside the existing "returns false when an existing protected file cannot be opened" case that verifies EACCES is treated the same as EPERM: call checkFullDiskAccess with the same homeDirectory and a readProbe that pushes filePath to openedPaths and throws createFileSystemError("EACCES"), then assert the returned value is false and openedPaths equals [tccDatabasePath]; this ensures checkFullDiskAccess (and its readProbe handling) explicitly covers the EACCES permission-denied variant.
🤖 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 `@apps/desktop/src/lib/trpc/routers/permissions/full-disk-access.test.ts`:
- Around line 102-115: Add a new test alongside the existing "returns false when
an existing protected file cannot be opened" case that verifies EACCES is
treated the same as EPERM: call checkFullDiskAccess with the same homeDirectory
and a readProbe that pushes filePath to openedPaths and throws
createFileSystemError("EACCES"), then assert the returned value is false and
openedPaths equals [tccDatabasePath]; this ensures checkFullDiskAccess (and its
readProbe handling) explicitly covers the EACCES permission-denied variant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c31131a9-0e30-470b-9b44-c9c4033cfec8
📒 Files selected for processing (3)
apps/desktop/src/lib/trpc/routers/permissions.tsapps/desktop/src/lib/trpc/routers/permissions/full-disk-access.test.tsapps/desktop/src/lib/trpc/routers/permissions/full-disk-access.ts
Kitenite
force-pushed
the
fix-disk-access-detection
branch
from
690ddcc to
8994883
Compare
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 `@apps/desktop/src/lib/trpc/routers/permissions/native-permissions.test.ts`:
- Around line 95-107: The test "returns granted when the native Microphone
prompt grants access" assumes macOS behavior but doesn't guard for platform;
update the test to run platform-aware by checking process.platform (or
skip/assert different expected behavior when not "darwin"). Specifically, in the
test that calls requestMicrophone with a stubbed
systemPreferencesApi.askForMediaAccess and inspects openedUrls and result,
conditionally assert { granted: true } and openedUrls === [] only when
process.platform === "darwin"; otherwise assert that
PERMISSION_SETTINGS_URLS.microphone was opened via shellApi (openedUrls contains
PERMISSION_SETTINGS_URLS.microphone) and result is { granted: false }. Reference
requestMicrophone, systemPreferencesApi.askForMediaAccess,
PERMISSION_SETTINGS_URLS.microphone, openedUrls, and shellApi when making the
conditional assertions.
🪄 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: 081c2f30-2396-4382-b1ad-468b53daf609
📒 Files selected for processing (5)
apps/desktop/src/lib/trpc/routers/permissions.tsapps/desktop/src/lib/trpc/routers/permissions/full-disk-access.test.tsapps/desktop/src/lib/trpc/routers/permissions/full-disk-access.tsapps/desktop/src/lib/trpc/routers/permissions/native-permissions.test.tsapps/desktop/src/lib/trpc/routers/permissions/native-permissions.ts
Kitenite
force-pushed
the
fix-disk-access-detection
branch
from
8994883 to
1915468
Compare
Kitenite
force-pushed
the
fix-disk-access-detection
branch
from
1915468 to
9b511bf
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
Fixes the macOS Full Disk Access detector used by the desktop permissions router.
The previous implementation probed only
~/Library/Safari/Bookmarks.plist. That file is TCC-protected, but it is also optional: users with no Safari bookmarks may not have it at all. In that case Superset reported Full Disk Access as missing even after the user granted it in System Settings.This PR moves FDA detection into a small helper that probes multiple protected user-data files. Missing probe files are skipped, while actual open failures such as
EPERMorEACCESstill report FDA as unavailable. It also extracts the other native permission checks/request entry points into a tested helper so the settings deep links and Microphone grant fallback behavior are covered.Impact
Users who have granted Full Disk Access should no longer get stuck in onboarding or permissions settings just because Safari has no bookmarks file.
Validation
bun test src/lib/trpc/routers/permissions/full-disk-access.test.ts src/lib/trpc/routers/permissions/native-permissions.test.ts(15 tests covering FDA probe behavior, realfs.openSyncfallback, Accessibility status, Microphone granted/denied/error branches, and all permission settings request URLs){"fullDiskAccess":true}{"name":"Electron","fullDiskAccess":true,"accessibility":true,"microphone":"granted"}{"fullDiskAccess":false,"accessibility":false,"microphone":"not-determined"}, confirming denied/not-yet-granted TCC pathsbun run --cwd apps/desktop typecheckbun run lint:fixbun run lintSummary by CodeRabbit
New Features
Refactor
Tests