Skip to content

feat: Add doctor check to verify full disk access permissions#385

Merged
mykola-mokhnach merged 4 commits into
appium:masterfrom
mykola-mokhnach:doctor-check
May 15, 2026
Merged

feat: Add doctor check to verify full disk access permissions#385
mykola-mokhnach merged 4 commits into
appium:masterfrom
mykola-mokhnach:doctor-check

Conversation

@mykola-mokhnach
Copy link
Copy Markdown
Contributor

@mykola-mokhnach mykola-mokhnach requested a review from KazuCocoa May 14, 2026 09:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds diagnostics and runtime handling around macOS Full Disk Access requirements for native screen recording retrieval (XCTest daemon attachments under ~/Library/Daemon Containers/...), and expands attachment discovery to support both legacy and newer attachment directory layouts.

Changes:

  • Added an optional doctor check to verify the Appium server process can access ~/Library/Daemon Containers and enumerate XCTest attachment files.
  • Updated native screen recording attachment enumeration to search both Data/Attachments and Data/tmp/Attachments, and to handle “no access” scenarios explicitly.
  • Made UUID-to-path matching case-insensitive when locating the recorded attachment for retrieval/BiDi publishing.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
lib/doctor/optional-checks.ts Adds an optional doctor check to detect whether Full Disk Access is needed for reading XCTest daemon attachment locations.
lib/commands/native-record-screen.ts Updates attachment discovery/matching and adds explicit handling and messaging when daemon container access is blocked.
Comments suppressed due to low confidence (1)

lib/commands/native-record-screen.ts:480

  • macosStopNativeScreenRecording now reports a Full Disk Access problem whenever listAttachments() returns hasAccess=false. Since listAttachments sets hasAccess=false for any fs.access failure (including ENOENT), this can misdiagnose a missing ~/Library/Daemon Containers directory as a permissions issue. Consider distinguishing ENOENT from permission errors in listAttachments and surfacing a more accurate message.
async function listAttachments(): Promise<{hasAccess: boolean; paths: string[]}> {
  // e.g. .../Daemon Containers/<container-id>/Data/Attachments/<uuid>
  // or .../Daemon Containers/<container-id>/Data/tmp/Attachments/<uuid> (Xcode 26.5+)
  const daemonContainersRoot = path.resolve(os.homedir(), 'Library', 'Daemon Containers');
  try {
    await fs.access(daemonContainersRoot);
  } catch {
    return {hasAccess: false, paths: []};
  }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +149 to +150
return doctor.nokOptional(
`Cannot access "${this.DAEMON_CONTAINERS_ROOT}": ${(e as Error).message}`,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this folder should always be there, it's a system one

Comment on lines 139 to 165
@@ -145,7 +159,7 @@ export class NativeVideoChunksBroadcaster {
);
} catch {
this._log.warn(
`The video recording identified by ${uuid} did not ` +
`The video recording BiDi broadcast identified by ${uuid} did not ` +
`start within ${RECORDING_STARTUP_TIMEOUT_MS}ms timeout`,
);
return;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's ok

Comment on lines 436 to 442
`The screen recording identified by ${uuid} cannot be retrieved. ` +
`Make sure the Appium Server process or its parent process (e.g. Terminal) ` +
`has Full Disk Access permission enabled in 'System Preferences' -> 'Privacy & Security' tab. ` +
`You may verify the presence of the recorded video manually by running the ` +
`'find "$HOME/Library/Daemon Containers/" -type f -name "${uuid}"' command from Terminal ` +
`You may verify the presence of the recorded video manually (e.g. under ` +
`*/Data/Attachments/ or */Data/tmp/Attachments/ within Daemon Containers) by running ` +
`'find "$HOME/Library/Daemon Containers" -type f -name "${uuid}"' from Terminal ` +
`if the latter has been granted the above access permission.`,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +278 to 281
const tasks: Promise<void>[] = paths
.map((attachmentPath) => [path.basename(attachmentPath), attachmentPath])
.filter(([name]) => uuidSet.has(name))
.map(([, attachmentPath]) => fs.rimraf(attachmentPath));
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@mykola-mokhnach mykola-mokhnach merged commit e58943b into appium:master May 15, 2026
13 checks passed
@mykola-mokhnach mykola-mokhnach deleted the doctor-check branch May 15, 2026 06:58
github-actions Bot pushed a commit that referenced this pull request May 15, 2026
## [3.5.0](v3.4.3...v3.5.0) (2026-05-15)

### Features

* Add doctor check to verify full disk access permissions ([#385](#385)) ([e58943b](e58943b))
@github-actions
Copy link
Copy Markdown

🎉 This PR is included in version 3.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants