Skip to content

Android settings permission all: classify pm refusals on typed reasons, not stderr substrings #2700

Description

@thymikee

Purpose

settings permission with target all on Android decides whether a failing pm call is a skip or an abort by matching lowercase substrings of adb stderr, and the photos path re-derives the same decision by re-running that sniff over another error's details.attempts. AGENTS.md requires behavior to key on typed reasons and details, never error text. This issue also folds in the per-unit accounting that is currently re-spelled in four functions.

Current shape

packages/platform-android/src/settings-permission.ts:

  • isSkippablePmStderr (~L338) — five text.includes(...) branches over not a changeable permission, has not requested permission, is not a runtime permission, unknown permission, managed by role.
  • tryPmUnit (~L307) — skippable stderr becomes { ok: false, reason: firstStderrLine(stderr) }, i.e. the reason is the raw first stderr line, and the skip decision is already lost by the time a caller sees it.
  • isSkippablePhotosError (~L364) — re-applies the same text sniff to error.details.attempts.
  • AllUnitContext / applyAllPermissionUnit / applyAllNotificationsUnit / applyAllPhotosUnit / applyAllPmUnit / finishAllUnit (~L197-L267) — every unit path destructures the same mutable context and repeats the applied.push + warnIfRevoked accounting, with skip warnings spelled separately in two of them.

Required behavior

pm exposes no machine-readable status code, so this is not a request to stop reading adb output. The requirement is one classification point:

  1. A single owning function turns an adb result into a typed outcome. Suggested shape:

    type AndroidPmUnitOutcome =
      | { kind: 'applied' }
      | { kind: 'skipped'; reason: AndroidPmSkipReason; detail: string }
      | { kind: 'aborted'; error: AppError };
    
    type AndroidPmSkipReason =
      | 'not-changeable' | 'not-requested' | 'not-runtime-permission'
      | 'unknown-permission' | 'role-managed';
  2. warnings strings are rendered from the typed reason plus the detail line, so operator-facing text is unchanged while no consumer re-reads stderr.

  3. The photos path consumes the same typed outcome. isSkippablePhotosError and the second isSkippablePmStderr call go away; a photos skip carries the per-candidate reasons.

  4. Operational failures (offline device, dropped transport, denied op) must keep aborting the fan-out — that is what stops launchApp continuing half-applied. Keep it that way.

  5. Each declared unit owns its own mutation and its own applied / warnings accounting, or the code states why it cannot. allPermissionUnits (~L275) keeps the collapse it performs today: one photos probe, one appops path, direct pm otherwise.

Observable completion

  • No .includes( over stderr anywhere in settings-permission.ts.
  • pnpm vitest run packages/platform-android/src/__tests__/settings-permission.test.ts green, including the existing pins that all skips install permissions, special ids such as MANAGE_EXTERNAL_STORAGE, custom ids, and role-managed ids like WRITE_SETTINGS on API 36.
  • New tests assert the abort side per reason: an operational pm failure surfaces as COMMAND_FAILED and stops the sequence, with the earlier applied ids named.
  • pnpm check:quick and pnpm format clean.

Dependencies

No blocker. Independent of #2363, which deliberately left this out to avoid widening a maestro/ios change into the Android backend.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions