diff --git a/packages/skills/skills/argent-settings-permissions/SKILL.md b/packages/skills/skills/argent-settings-permissions/SKILL.md index 8f1ae1789..3638e7b83 100644 --- a/packages/skills/skills/argent-settings-permissions/SKILL.md +++ b/packages/skills/skills/argent-settings-permissions/SKILL.md @@ -74,13 +74,13 @@ One abstract permission can map to several concrete Android permissions; which o **Android emulator and physical device.** Changes the app's `android.permission.*` runtime permissions over adb (and, for `reset`, best-effort clears the user-set/user-fixed flags - the revoke is what decides success; flag-clearing needs Android 13 / API 33+). Requirements: - The app must be **installed** - the tool probes for the package first and errors clearly if it is missing (a transport/timeout failure surfaces adb's real cause, not a false "not installed"). -- The app must **declare** the permission in its manifest. The package manager rejects any mapped permission the manifest doesn't request; those come back in the result's `skipped` list. The action succeeds if **at least one** mapped permission sticks, and errors only if **all** of them were rejected. +- The app must **declare** the permission in its manifest. Any mapped permission the manifest doesn't request comes back in the result's `skipped` list. Recent Android accepts a request for an undeclared permission and silently does nothing, so the result is checked against the package manager's own state rather than the command's exit status. Granting succeeds if **at least one** mapped permission sticks and errors only if none did; denying a permission the app never declared is already satisfied and is reported as skipped. ## Gotchas - **Changing a permission can terminate a running app** (system behavior on both platforms). Prefer setting permissions **before** `launch-app`; if you change one while the app is running, `restart-app` afterward. - **Reset is per-app on both platforms** - pass `bundleId`; there is no reliable device-wide reset. -- **A partial Android result is normal.** `applied` lists what actually changed; `skipped` lists mapped permissions the package manager rejected (usually not in the manifest, or gated by API level). Both together tell you what happened. +- **A partial Android result is normal.** `applied` lists what actually changed, confirmed against the package manager's state; `skipped` lists mapped permissions that did not take effect (usually not in the manifest, or not runtime-changeable on this device). `unverified`, when present, lists applied entries that could not be confirmed — an older device or an unfamiliar layout — so you can tell a checked result from one taken on trust. - **A pre-launch `deny` suppresses the prompt on iOS only.** On iOS a TCC denial answers the app's request, so no dialog appears. On Android a `deny` clears the grant but sets no "user-fixed" flag, so the app's next request still shows the system dialog - a pre-launch `deny` there tests the revoked _state_, not a suppressed prompt. - **`camera` on iOS** may be rejected by a simulator **runtime** that doesn't model the service (it varies by simruntime, not by the installed Xcode - a runtime can accept `camera` even when the platform's own service list omits it). A rejection surfaces as a generic CoreSimulator error, so a `camera` failure (unless it's the shutdown-simulator case, which gets the boot hint instead) is reported with a hint about the runtime's supported services. - **`grant location` needs the app installed first (iOS).** Location authorization isn't stored in TCC and isn't applied to a bundle id until the app exists, so a pre-install `grant location` / `grant location-always` records nothing. On a **local** simulator the tool checks install state and errors clearly instead of reporting a false success; on a **remote** simulator it cannot probe install state, so a pre-install grant there reports success while recording nothing - make sure the app is installed before granting location remotely. (TCC-backed services like `camera`/`photos` _can_ be granted before install; they persist and apply on install.) @@ -90,7 +90,8 @@ One abstract permission can map to several concrete Android permissions; which o Returns `{ action, permission, bundleId, applied, skipped? }`: - `applied` - the platform-level services/permissions actually changed (the TCC service(s) on iOS; the `android.permission.*` names on Android). -- `skipped` - Android only, present when some mapped permissions were rejected but others succeeded. +- `skipped` - Android only, present when some mapped permissions did not take effect but others did. +- `unverified` - Android only, present when an applied entry could not be confirmed against the device's state. The call **fails** when nothing could be applied - read the error; it names the reason: an unsupported permission for the platform (`notifications` on iOS, `reminders` on Android), the app not installed (including a pre-install `grant location` on iOS), a shutdown simulator (iOS), or every mapped permission being rejected (usually a missing manifest entry). A non-shutdown `camera` failure additionally hints about the simulator runtime's supported services (a shutdown-simulator failure gets the boot hint instead). diff --git a/packages/tool-server/src/tools/settings-permissions/index.ts b/packages/tool-server/src/tools/settings-permissions/index.ts index b271c360f..9ea997a5c 100644 --- a/packages/tool-server/src/tools/settings-permissions/index.ts +++ b/packages/tool-server/src/tools/settings-permissions/index.ts @@ -69,7 +69,7 @@ Permissions: camera, microphone, photos, contacts, notifications, calendar, loca iOS simulator: edits the simulator's TCC store, always per-app. \`notifications\` is not supported (no iOS equivalent). \`reset\` is per-app — a device-wide reset is a no-op for existing grants on recent iOS, so it is not offered. \`grant location\`/\`location-always\` needs the app already installed (location auth isn't stored in TCC and isn't applied to a bundle id until the app exists) — enforced on local simulators; a remote simulator can't be probed for install state, so ensure the app is installed there first. Other services can be granted before install. Android: changes the mapped \`android.permission.*\` runtime permissions (reset also best-effort clears the user-set permission flags). The app must be installed and declare them in its manifest; \`reminders\` has no Android equivalent. Some permission changes terminate the app if it is running (system behavior on both platforms) — set permissions before launching, or relaunch after. -Returns { action, permission, bundleId, applied, skipped? }: \`applied\` lists the platform-level services/permissions actually changed; \`skipped\` (Android) lists mapped permissions the package manager rejected, e.g. ones the manifest doesn't declare. Fails if nothing could be applied.`, +Returns { action, permission, bundleId, applied, skipped?, unverified? }: \`applied\` lists the platform-level services/permissions actually changed; \`skipped\` (Android) lists mapped permissions that did not take effect, e.g. ones the manifest doesn't declare. On Android this is established by reading the package manager's state back, because recent Android accepts a request for an undeclared permission and silently does nothing; \`unverified\` lists entries that were applied but could not be confirmed on this device. Granting fails if nothing took effect; denying an undeclared permission is already satisfied and is reported in \`skipped\`.`, searchHint: "grant deny reset revoke app permissions privacy camera microphone location settings", zodSchema, capability, diff --git a/packages/tool-server/src/tools/settings-permissions/platforms/android-permission-state.ts b/packages/tool-server/src/tools/settings-permissions/platforms/android-permission-state.ts new file mode 100644 index 000000000..3ba3e57bc --- /dev/null +++ b/packages/tool-server/src/tools/settings-permissions/platforms/android-permission-state.ts @@ -0,0 +1,260 @@ +import { adbShell, shellQuote } from "../../../utils/adb"; + +/** + * Reads back what the package manager actually holds for a package, so a + * permission change can be reported on evidence rather than on an exit code. + * + * On Android 16 (API 36) granting a permission an app does not declare succeeds + * silently — the command exits 0, nothing is recorded, and the caller is told it + * was applied (#616). The exit code stopped being evidence, so the state has to + * be read. + * + * Every field here is tri-state on purpose: a section that was not found is + * `undefined`, never an empty collection. That distinction is load-bearing. + * "The package declares nothing" and "we could not read what it declares" lead + * to opposite conclusions, and conflating them would demote every permission on + * any package whose layout we failed to parse. + */ + +export interface PackagePermissionState { + /** Permissions the manifest declares. Undefined when the section was absent. */ + requested?: ReadonlySet; + /** Runtime grant state for user 0. Undefined when no runtime block was found. */ + runtime?: ReadonlyMap; +} + +export type PermissionVerdict = + /** Observed in the state the action asked for. */ + | { kind: "confirmed" } + /** Observed NOT to be in that state, with a reason worth showing the caller. */ + | { kind: "contradicted"; detail: string } + /** Nothing could be read. Callers must fall back to the command's own verdict. */ + | { kind: "unknown" }; + +function indentOf(line: string): number { + let i = 0; + while (i < line.length && line[i] === " ") i++; + return i; +} + +/** Blank lines carry no indent, so they must never terminate a section. */ +function isBlank(line: string): boolean { + return line.trim().length === 0; +} + +/** + * Lines belonging to a header: everything indented deeper, up to the next line + * at or above the header's own indent. Blank lines are skipped rather than + * treated as indent 0, which would truncate a section at its first gap. + */ +function sectionBody(lines: string[], headerIndex: number): string[] { + const headerIndent = indentOf(lines[headerIndex]!); + const body: string[] = []; + for (let i = headerIndex + 1; i < lines.length; i++) { + const line = lines[i]!; + if (isBlank(line)) continue; + if (indentOf(line) <= headerIndent) break; + body.push(line); + } + return body; +} + +/** Index of the first line whose trimmed text equals `header`, at any indent. */ +function findHeader(lines: string[], header: string, from = 0, until = Infinity): number { + for (let i = from; i < Math.min(lines.length, until); i++) { + if (lines[i]!.trim() === header) return i; + } + return -1; +} + +/** Index of the next line at indent 0 — the boundary of a top-level section. */ +function nextTopLevel(lines: string[], from: number): number { + for (let i = from; i < lines.length; i++) { + if (!isBlank(lines[i]!) && indentOf(lines[i]!) === 0) return i; + } + return lines.length; +} + +/** + * Entry names are the text before the first colon. Android 10-13 annotate + * restricted entries (`NAME: restricted=true`), so the colon cannot be assumed + * absent even in the declaration list. + */ +function entryName(line: string): string { + const trimmed = line.trim(); + const colon = trimmed.indexOf(":"); + return (colon === -1 ? trimmed : trimmed.slice(0, colon)).trim(); +} + +/** A section that parses to nothing is treated as unread, not as "declares nothing". */ +function setOrUndefined(values: string[]): ReadonlySet | undefined { + return values.length > 0 ? new Set(values) : undefined; +} + +function parseRuntimeRows(body: string[]): ReadonlyMap | undefined { + const rows = new Map(); + for (const line of body) { + const match = /^\s*([A-Za-z0-9_.]+):\s*granted=(true|false)/.exec(line); + if (match) rows.set(match[1]!, match[2] === "true"); + } + return rows.size > 0 ? rows : undefined; +} + +/** + * The runtime block for user 0. + * + * `User 0:` appears with a trailing payload inside a package block + * (`User 0: ceDataInode=…`) and bare inside a shared-user block, so it is matched + * as a prefix. Only user 0 is read: grant/revoke target the system user and the + * tool never selects another, so a row from a different user could only ever + * demote something wrongly. + */ +function runtimeForUserZero(lines: string[], from: number, until: number): string[] | null { + for (let i = from; i < Math.min(lines.length, until); i++) { + if (!lines[i]!.trim().startsWith("User 0:")) continue; + const userBody = sectionBody(lines, i); + const offset = i + 1; + const runtimeIdx = findHeader( + lines, + "runtime permissions:", + offset, + offset + userBody.length + 1 + ); + if (runtimeIdx !== -1) return sectionBody(lines, runtimeIdx); + } + return null; +} + +/** + * Parse the package-manager dump for one package. + * + * Anything unrecognised yields `undefined` fields rather than empty ones — see + * the note on the interface. + */ +export function parsePackagePermissionState( + dump: string, + bundleId: string +): PackagePermissionState { + // adb on Windows inserts CR; every match below is on trimmed text, but the + // split has to tolerate both endings. + const lines = dump.split(/\r?\n/); + + // Matched by equality: a per-package dump also contains top-level + // `Permissions:` sections, and a loose match would select the wrong one. + const packagesIdx = findHeader(lines, "Packages:"); + if (packagesIdx === -1) return {}; + + const packagesEnd = nextTopLevel(lines, packagesIdx + 1); + // Scoped to the first block under `Packages:`, which excludes the duplicate + // that `Hidden system packages:` prints for a system app. + const marker = `Package [${bundleId}] (`; + let blockIdx = -1; + for (let i = packagesIdx + 1; i < packagesEnd; i++) { + if (lines[i]!.trim().startsWith(marker)) { + blockIdx = i; + break; + } + } + if (blockIdx === -1) return {}; + + const blockBody = sectionBody(lines, blockIdx); + const blockEnd = blockIdx + 1 + blockBody.length; + + const requestedIdx = findHeader(lines, "requested permissions:", blockIdx + 1, blockEnd); + const requested = + requestedIdx === -1 + ? undefined + : setOrUndefined(sectionBody(lines, requestedIdx).map(entryName)); + + let runtimeBody = runtimeForUserZero(lines, blockIdx + 1, blockEnd); + + // A package with `android:sharedUserId` keeps its runtime state in a separate + // top-level `Shared users:` section, keyed by the shared-user name rather than + // the package name. Without this, ~1 in 6 packages on a stock image — Maps, + // Calendar, Settings among them — would read as "no runtime state" and fall + // back to trusting the exit code, leaving #616 unfixed exactly where it is + // most likely to be hit. + if (!runtimeBody) { + const sharedName = /sharedUser=SharedUserSetting\{\S+\s+(\S+?)\/\d+\}/.exec( + lines.slice(blockIdx, blockEnd).join("\n") + )?.[1]; + if (sharedName) { + const sharedIdx = findHeader(lines, "Shared users:"); + if (sharedIdx !== -1) { + const sharedEnd = nextTopLevel(lines, sharedIdx + 1); + const sharedMarker = `SharedUser [${sharedName}] (`; + for (let i = sharedIdx + 1; i < sharedEnd; i++) { + if (!lines[i]!.trim().startsWith(sharedMarker)) continue; + const sharedBody = sectionBody(lines, i); + runtimeBody = runtimeForUserZero(lines, i + 1, i + 1 + sharedBody.length); + break; + } + } + } + } + + return { + ...(requested && { requested }), + ...(runtimeBody && { runtime: parseRuntimeRows(runtimeBody) }), + }; +} + +/** + * Read the package's permission state. Never throws: a failed read leaves the + * command's own verdict in place, which is the behaviour every caller had before + * verification existed. + */ +export async function readPackagePermissionState( + udid: string, + bundleId: string +): Promise { + try { + const out = await adbShell(udid, `dumpsys package ${shellQuote(bundleId)}`); + return parsePackagePermissionState(out, bundleId); + } catch { + return {}; + } +} + +/** + * Does the observed state agree that this permission was changed as asked? + * + * `grant` targets granted; `deny` and `reset` both target not-granted. The + * question is whether the permission is now in the requested state — not whether + * anything changed — because denying an already-denied permission is a perfectly + * good outcome for the caller who asked for it. + */ +export function verifyPermission( + state: PackagePermissionState, + permission: string, + action: "grant" | "deny" | "reset" +): PermissionVerdict { + const target = action === "grant"; + + // A runtime row is the strongest evidence and outranks the declaration list: + // a permission split into the app by the platform can hold real state while + // reading as undeclared. + const granted = state.runtime?.get(permission); + if (granted !== undefined) { + return granted === target + ? { kind: "confirmed" } + : { + kind: "contradicted", + detail: `the package manager still reports it as ${granted ? "granted" : "not granted"}`, + }; + } + + if (state.requested) { + if (!state.requested.has(permission)) { + return { kind: "contradicted", detail: "the app's manifest does not declare it" }; + } + if (state.runtime) { + return { + kind: "contradicted", + detail: "it is declared but is not a runtime-changeable permission on this device", + }; + } + } + + return { kind: "unknown" }; +} diff --git a/packages/tool-server/src/tools/settings-permissions/platforms/android.ts b/packages/tool-server/src/tools/settings-permissions/platforms/android.ts index 80dfadbf1..5bfe6cca9 100644 --- a/packages/tool-server/src/tools/settings-permissions/platforms/android.ts +++ b/packages/tool-server/src/tools/settings-permissions/platforms/android.ts @@ -1,6 +1,7 @@ import { FAILURE_CODES, FailureError, getFailureSignal } from "@argent/registry"; import type { PlatformImpl } from "../../../utils/cross-platform-tool"; import { adbShell, isTerminalAdbError, shellQuote } from "../../../utils/adb"; +import { readPackagePermissionState, verifyPermission } from "./android-permission-state"; import type { PermissionAction, PermissionName, @@ -200,7 +201,12 @@ export const androidImpl: PlatformImpl< } const applied: string[] = []; - const failures: Array<{ permission: string; detail: string }> = []; + const pending: string[] = []; + const unverified: string[] = []; + // `rejected` distinguishes the package manager refusing outright from a + // change that was accepted and then found not to have happened — they read + // very differently to a caller, and only the former is a hard failure. + const failures: Array<{ permission: string; detail: string; rejected: boolean }> = []; for (const perm of permissions) { let result: PmResult; @@ -238,9 +244,32 @@ export const androidImpl: PlatformImpl< } } if (result.ok) { - applied.push(perm); + // Not yet `applied`: the command reporting success is no longer evidence + // that anything happened, so hold it until the state has been read. + pending.push(perm); } else { - failures.push({ permission: perm, detail: result.detail }); + failures.push({ permission: perm, detail: result.detail, rejected: true }); + } + } + + // One read for the whole fan-out, and only when something claims to have + // landed — a call the package manager rejected outright has nothing to + // verify and should not pay for a round trip. + if (pending.length > 0) { + const state = await readPackagePermissionState(udid, bundleId); + for (const perm of pending) { + const verdict = verifyPermission(state, perm, action); + if (verdict.kind === "contradicted") { + failures.push({ permission: perm, detail: verdict.detail, rejected: false }); + } else { + applied.push(perm); + // Fail open, but say so. The command succeeded and we could not read + // the state to check it — on an older device, an unfamiliar dump, or a + // failed read. Refusing to believe it would break every such device; + // staying silent would leave the caller exactly where this bug left + // them, unable to tell a confirmed change from an assumed one. + if (verdict.kind === "unknown") unverified.push(perm); + } } } @@ -250,16 +279,28 @@ export const androidImpl: PlatformImpl< // don't reassert that here — surface pm's own per-permission reasons, which // also carry any transport/timeout cause verbatim, so the caller sees the // real problem rather than a fixed manifest guess. - if (applied.length === 0) { + // Zero successes means the action did nothing — but only for `grant`. Asking + // to DENY a permission the app never declared is already satisfied: the app + // cannot hold it, which is exactly what the caller wanted. Erroring there + // would break the most natural setup call there is ("make sure this app has + // no camera access") on any app that simply doesn't use the camera. + const undeclaredOnly = + action !== "grant" && failures.length > 0 && failures.every((f) => !f.rejected); + + if (applied.length === 0 && !undeclaredOnly) { const details = failures.map((f) => `${f.permission}: ${f.detail}`).join("; "); throw new FailureError( - `Failed to ${action} '${permission}' for ${bundleId} on ${udid} — every mapped runtime permission was rejected. ` + - `Usually the manifest doesn't declare it, or it isn't a runtime-changeable permission; see the per-permission detail for the exact cause. (${details})`, + `Failed to ${action} '${permission}' for ${bundleId} on ${udid} — no mapped runtime permission took effect. ` + + `Usually the app's manifest doesn't declare it, or it isn't a runtime-changeable permission; see the per-permission detail for the exact cause. (${details})`, { error_code: FAILURE_CODES.ANDROID_SETTINGS_PERMISSION_FAILED, - failure_stage: "android_settings_permission_pm", + failure_stage: failures.some((f) => f.rejected) + ? "android_settings_permission_pm" + : "android_settings_permission_unverified", failure_area: "tool_server", - error_kind: "subprocess", + // Nothing crashed when the change was silently dropped — the app just + // cannot take this permission. + error_kind: failures.some((f) => f.rejected) ? "subprocess" : "unsupported", } ); } @@ -270,6 +311,7 @@ export const androidImpl: PlatformImpl< bundleId, applied, ...(failures.length > 0 ? { skipped: failures.map((f) => f.permission) } : {}), + ...(unverified.length > 0 ? { unverified } : {}), }; }, }; diff --git a/packages/tool-server/src/tools/settings-permissions/types.ts b/packages/tool-server/src/tools/settings-permissions/types.ts index b26acc5fd..db6d6374e 100644 --- a/packages/tool-server/src/tools/settings-permissions/types.ts +++ b/packages/tool-server/src/tools/settings-permissions/types.ts @@ -40,12 +40,29 @@ export interface SettingsPermissionsResult { */ applied: string[]; /** - * Android only: mapped `android.permission.*` entries the package manager - * rejected (typically not declared in the app's manifest, or gated by the - * device's API level). Present only when at least one other mapped - * permission succeeded — if all of them fail, the tool errors instead. + * Android only: mapped `android.permission.*` entries that did not take + * effect — typically not declared in the app's manifest, or not a + * runtime-changeable permission on this device. Present only when at least + * one other mapped permission succeeded. + * + * On Android these are established by reading the package manager's own state + * back, not by trusting the command's exit status: recent Android accepts a + * request for a permission an app never declared and does nothing, so an exit + * code alone would report it as applied. */ skipped?: string[]; + /** + * Android only: entries reported in `applied` that could NOT be confirmed + * against the package manager's state — an older device, an unfamiliar dump + * layout, or a read that failed. They are still listed in `applied`, because + * the command itself reported success and refusing to believe it would break + * every device whose state we cannot read; this field exists so the caller can + * tell "confirmed" from "taken on trust". + * + * iOS never sets this: its permission commands fail loudly, so there is no + * equivalent silent no-op to guard against. + */ + unverified?: string[]; } export type SettingsPermissionsServices = Record; diff --git a/packages/tool-server/test/settings-permissions.test.ts b/packages/tool-server/test/settings-permissions.test.ts index 3c5b4868d..1c90bc19d 100644 --- a/packages/tool-server/test/settings-permissions.test.ts +++ b/packages/tool-server/test/settings-permissions.test.ts @@ -75,6 +75,10 @@ function execFileFails(message: string): void { // Default adb behavior: the `pm list packages` existence preflight finds the // package (it prints a `package:` line and exits 0), and every mutating pm // command succeeds silently (pm's real success shape). +// +// `dumpsys package` also falls through to "", which parses to "nothing could be +// read" — so verification stays neutral and every pre-#616 assertion here keeps +// its original meaning. Tests that care about verification must supply a dump. function adbDefaults(overrides?: (cmd: string) => string | Promise | undefined): void { mockAdbShell.mockImplementation(async (_serial, cmd) => { const overridden = overrides?.(cmd); @@ -566,6 +570,9 @@ describe("settings-permissions Android branch", () => { "pm list packages 'com.example.app'", "pm revoke 'com.example.app' android.permission.CAMERA", "pm clear-permission-flags 'com.example.app' android.permission.CAMERA user-set user-fixed", + // One read back for the whole call: the command exiting 0 no longer proves + // anything happened, so the result is checked against real state (#616). + "dumpsys package 'com.example.app'", ]); }); @@ -890,6 +897,9 @@ describe("settings-permissions Android branch", () => { "pm list packages 'com.example.app'", "pm revoke 'com.example.app' android.permission.ACCESS_BACKGROUND_LOCATION", "pm clear-permission-flags 'com.example.app' android.permission.ACCESS_BACKGROUND_LOCATION user-set user-fixed", + // One read back for the whole call: the command exiting 0 no longer proves + // anything happened, so the result is checked against real state (#616). + "dumpsys package 'com.example.app'", ]); }); }); @@ -985,3 +995,206 @@ describe("settings-permissions dispatch wiring (through tool.execute)", () => { expect((args as string[]).slice(0, 2)).toEqual(["simctl", "privacy"]); }); }); + +/** + * Verification of Android permission changes (#616). + * + * Recent Android accepts a request for a permission an app never declared and + * does nothing — exit 0, no state written — so the exit code stopped being + * evidence and the result is now checked against the package manager's state. + */ +describe("settings-permissions Android verification (#616)", () => { + function params(overrides: Partial): SettingsPermissionsParams { + return { + udid: ANDROID_SERIAL, + action: "grant", + permission: "camera", + bundleId: "com.example.app", + ...overrides, + } as SettingsPermissionsParams; + } + + /** + * Renders the dump the verifier reads. Models the two facts that decide a + * verdict: what the manifest declares, and the runtime grant state. + */ + function dumpFor(opts: { declared?: string[]; granted?: Record }): string { + const declared = opts.declared ?? []; + const granted = opts.granted ?? {}; + const rows = Object.entries(granted).map( + ([perm, value]) => ` ${perm}: granted=${value}, flags=[ USER_SET]` + ); + return [ + "Packages:", + " Package [com.example.app] (abc1234):", + " userId=10123", + " requested permissions:", + ...declared.map((perm) => ` ${perm}`), + " User 0: ceDataInode=1 installed=true", + ...(rows.length > 0 ? [" runtime permissions:", ...rows] : []), + ].join("\n"); + } + + const withDump = (dump: string) => (cmd: string) => + cmd.startsWith("dumpsys package") ? dump : undefined; + + it("reports undeclared permissions as skipped rather than applied", async () => { + // `photos` fans out to four permissions; this app declares one of them. The + // other three exit 0 and do nothing, and used to be reported as applied. + adbDefaults( + withDump( + dumpFor({ + declared: ["android.permission.READ_EXTERNAL_STORAGE"], + granted: { "android.permission.READ_EXTERNAL_STORAGE": true }, + }) + ) + ); + + const result = await androidImpl.handler( + {}, + params({ action: "grant", permission: "photos" }), + androidDevice + ); + + expect(result.applied).toEqual(["android.permission.READ_EXTERNAL_STORAGE"]); + expect(result.skipped).toEqual([ + "android.permission.READ_MEDIA_IMAGES", + "android.permission.READ_MEDIA_VIDEO", + "android.permission.READ_MEDIA_VISUAL_USER_SELECTED", + ]); + }); + + it("keeps a mixed result mixed — the real change applied, the phantom one skipped", async () => { + // The reported case: one call where READ_EXTERNAL_STORAGE genuinely flipped + // and READ_MEDIA_AUDIO does not exist on the package. Reporting either + // wholesale — all applied, or a hard failure — would be wrong. + adbDefaults( + withDump( + dumpFor({ + declared: ["android.permission.READ_EXTERNAL_STORAGE"], + granted: { "android.permission.READ_EXTERNAL_STORAGE": false }, + }) + ) + ); + + const result = await androidImpl.handler( + {}, + params({ action: "deny", permission: "media-library" }), + androidDevice + ); + + expect(result.applied).toEqual(["android.permission.READ_EXTERNAL_STORAGE"]); + expect(result.skipped).toEqual(["android.permission.READ_MEDIA_AUDIO"]); + }); + + it("still reports a genuine change as applied, reading state once for the whole fan-out", async () => { + const declared = [ + "android.permission.READ_MEDIA_IMAGES", + "android.permission.READ_MEDIA_VIDEO", + "android.permission.READ_MEDIA_VISUAL_USER_SELECTED", + "android.permission.READ_EXTERNAL_STORAGE", + ]; + adbDefaults( + withDump(dumpFor({ declared, granted: Object.fromEntries(declared.map((x) => [x, true])) })) + ); + + const result = await androidImpl.handler( + {}, + params({ action: "grant", permission: "photos" }), + androidDevice + ); + + expect(result.applied).toEqual(declared); + expect(result.skipped).toBeUndefined(); + // One read per call, not per permission. + const reads = mockAdbShell.mock.calls.filter((c) => String(c[1]).startsWith("dumpsys package")); + expect(reads).toHaveLength(1); + }); + + it("trusts the command when the state cannot be read, and says so", async () => { + // Older devices, unfamiliar layouts and failed reads must not turn into a + // refusal — but the caller has to be able to tell this from a confirmed + // change, which is the complaint that motivated the issue. + adbDefaults(withDump("Unable to find package: com.example.app")); + + const result = await androidImpl.handler( + {}, + params({ action: "grant", permission: "camera" }), + androidDevice + ); + + expect(result.applied).toEqual(["android.permission.CAMERA"]); + expect(result.unverified).toEqual(["android.permission.CAMERA"]); + expect(result.skipped).toBeUndefined(); + }); + + it("does not fail a landed change when the verification read itself dies", async () => { + adbDefaults((cmd) => { + if (cmd.startsWith("dumpsys package")) throw new Error("adb: device 'x' not found"); + return undefined; + }); + + const result = await androidImpl.handler( + {}, + params({ action: "grant", permission: "camera" }), + androidDevice + ); + + expect(result.applied).toEqual(["android.permission.CAMERA"]); + expect(result.unverified).toEqual(["android.permission.CAMERA"]); + }); + + it("errors when a grant took effect nowhere", async () => { + adbDefaults(withDump(dumpFor({ declared: ["android.permission.INTERNET"] }))); + + const err = await androidImpl + .handler({}, params({ action: "grant", permission: "camera" }), androidDevice) + .then(() => null) + .catch((e: Error) => e); + + expect(getFailureSignal(err!)?.error_code).toBe("ANDROID_SETTINGS_PERMISSION_FAILED"); + expect(err!.message).toMatch(/manifest/); + // Nothing was rejected — the change was silently dropped. + expect(err!.message).not.toMatch(/was rejected/); + }); + + it("treats denying an undeclared permission as already satisfied", async () => { + // The app cannot hold a permission it does not declare, so "make sure this + // app has no camera access" is met. Erroring would break the most natural + // setup call there is on any app that simply doesn't use the camera. + adbDefaults(withDump(dumpFor({ declared: ["android.permission.INTERNET"] }))); + + const result = await androidImpl.handler( + {}, + params({ action: "deny", permission: "camera" }), + androidDevice + ); + + expect(result.applied).toEqual([]); + expect(result.skipped).toEqual(["android.permission.CAMERA"]); + }); + + it("keeps the package manager's own reason when it rejected the change outright", async () => { + // Below the API level where the silent no-op appears, the package manager + // still refuses — and its wording is the more actionable one, so + // verification must neither overwrite it nor promote the permission back. + adbDefaults((cmd) => { + if (cmd.startsWith("pm grant")) throw new Error("Security exception: not requested"); + if (cmd.startsWith("dumpsys package")) { + return dumpFor({ + declared: ["android.permission.CAMERA"], + granted: { "android.permission.CAMERA": true }, + }); + } + return undefined; + }); + + const err = await androidImpl + .handler({}, params({ action: "grant", permission: "camera" }), androidDevice) + .then(() => null) + .catch((e: Error) => e); + + expect(err).toBeInstanceOf(Error); + expect(err!.message).toMatch(/not requested/); + }); +});