fix(settings-permissions): report Android permissions on evidence, not on an exit code

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).
 

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,

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<string>;
+  /** Runtime grant state for user 0. Undefined when no runtime block was found. */
+  runtime?: ReadonlyMap<string, boolean>;
+}
+
+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<string> | undefined {
+  return values.length > 0 ? new Set(values) : undefined;
+}
+
+function parseRuntimeRows(body: string[]): ReadonlyMap<string, boolean> | undefined {
+  const rows = new Map<string, boolean>();
+  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<PackagePermissionState> {
+  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" };
+}