feat(maestro): support setPermissions and launchApp.permissions - #2363
feat(maestro): support setPermissions and launchApp.permissions#2363Rohit3523 wants to merge 5 commits into
Conversation
dd06012 to
8d08026
Compare
|
Three behavior gaps remain at 8d08026. launchApp applies permissions after open has already launched the app. Startup code can request access before the requested state is installed. Apply permissions after state clearing but before launch, and verify with an app that requests access immediately on startup. location: never maps to reset, which restores the prompt state rather than denying access. Map it to denial and add a regression distinguishing never from unset. all expands a fixed list that includes camera, despite the reported iOS run showing camera is unavailable. The sequential changes can therefore stop partway through. Derive the supported set from the existing backend capability information and validate before mutation; test all on the reported runtime. This head also has no CI checks yet. |
…tion never - launchApp.permissions now runs after state clearing but before open, so startup code observes the requested state; the map is validated before any mutation via a new clearAppState public operation. - location never maps to deny (unset keeps the reset prompt state). - ios all expansion skips the probe-unsupported camera/notifications so the sequential mutations cannot stop partway through.
|
The launch ordering and location: never fixes are addressed at 4de150a. The all case still needs the shared backend capability information: replacing the fixed list with fixed camera/notifications exclusions only reflects one host. It can skip a supported permission or still fail partway through on another runtime. Resolve and validate the runtime-supported set before clearing state or changing permissions, then test varying service sets and verify all on-device. The updated startup ordering also needs live verification; this head has no CI checks yet. |
thymikee
left a comment
There was a problem hiding this comment.
Reviewed at 4de150a. Parser, runtime-port, and daemon adapter suites plus tsc --noEmit pass locally. The parse/IR/projection layers are in good shape; the remaining problems are in how all and the fan-out meet the backends, plus some duplication.
1. all on Android fails partway on most apps. pm grant/pm revoke throw SecurityException: Package … has not requested permission … for anything the package does not declare, and grantAndroidPermission/revokeAndroidPermission run those without allowFailure. So all: deny expands to five pm calls and stops at the first undeclared one. The lab app in the description declares only RECORD_AUDIO, which is why all could not have been verified there. This is the same class of problem as the iOS exclusion list: the servable set is a property of the device and app, not the platform. Two concrete directions:
- Android: intersect the expansion with the package's
requested permissions:fromdumpsys packagebefore issuing anything (permission-grant-state.tsalready parses that dump). - iOS:
simctl privacyacceptsallas a service natively, and the backend already usesreset allas a fallback.all: Xcan be onesettings permission <action> allcall followed by the specific overrides. That removesALL_EXCLUDED_PERMISSIONSentirely and the "this host's simctl privacy help" reasoning with it.
Either way all belongs in the backends as a permission target, not as a list the daemon adapter maintains.
2. The fan-out is not atomic and does not say what landed. Name validation happens before the first mutation, but the backend can still reject mid-sequence (undeclared Android permission, missing iOS service). The flow then fails with a half-applied map and the error names only the entry that failed. If (1) resolves the servable set up front, this mostly goes away. Until then the error should at least list the mutations already applied.
3. launchApp duplicates the launch invoke. Both branches build the same operation. Collapse to:
if (input.permissions) {
const mutations = mapMaestroSetPermissions(input.permissions, platform);
if (clearState) await invokeMutation({ kind: 'clearAppState', ...(appId ? { appId } : {}) }, context);
await applyPermissionMutations(appId, mutations, context);
}
await invokeMutation(
{ kind: 'launchApp', ...(appId ? { appId } : {}), relaunch, clearState: clearState && !input.permissions, launchArgs },
context,
'deferred',
);The comment claiming the split "matches what open --clearAppState does" is only partly true on iOS: clearAppState also flips isDirectAppLaunch in platform-apple/src/lifecycle.ts, which changes how a runtime launch URL is folded into the open. Probably harmless for Maestro flows, but say that rather than claim equivalence.
4. Duplicate-key check in readSetPermissionsMap is dead and wrong. The YAML layer already rejects duplicate keys (Map keys must be unique), so the check never fires for real duplicates. It does fire for prototype keys: permissions: { constructor: allow } is rejected as "duplicate permission". Drop it.
5. Value validation is triplicated. MAESTRO_PERMISSION_VALUES (parser), RESOLVED_PERMISSION_VALUES (runtime port), and PLAIN_VALUE_STATES + GRANULAR_MUTATIONS (daemon) are the same set, with three copies of the "allow|deny|unset (plus always|inuse|never|limited …)" message. Export one constant from the maestro package. The runtime-port check is justified because ${VAR} resolves there, but it should reference the same set.
6. Smaller cleanups.
mapMaestroPermissiontakesexpandableas a parameter that is derivable fromplatform.applyPermissionsis a one-line wrapper with one caller; inline it intosetPermissions.- The empty-map check is repeated by both callers of
readSetPermissionsMapwith different messages; move it into the reader. isAgentTapCommand/isAgentAssertCommandrestate their kind lists by hand. Aconst TAP_KINDS = [...] as constwith.includeskeeps the guard and theExtractunion in sync.daemon-request.ts: thesettingsAppBundleIdcomment is good. Worth one line insnapshot-settings.tstoo, since the precedence over the session app is invisible from the CLI side.
(1) and (2) are the blockers; the rest is cleanup that should land in the same PR.
Generated by Claude Code
…mission divergences - iOS reset notifications bypasses the simctl probe gate into the existing reset-all fallback (verified live on iOS 26.3 where help omits the service); grant/deny stay loud rejections. - Support matrix and replay docs now declare the intentional gaps vs upstream: no silent all-allow launch default, backend-servable all expansion, loud rejections, true-reset unset, never denies.
|
At 7285bb1, resetting notifications can now fall through to simctl reset all when the runtime does not list notifications. A flow asking only for notifications: unset can therefore reset microphone, location and other permissions too. Keep the operation targeted; if that is unavailable, fail explicitly rather than clearing unrelated state. Add a regression that preserves another permission across notification reset. The earlier all-expansion finding also remains: the adapter still uses fixed lists, and the new docs describe them as runtime-supported even though no runtime preflight occurs. Please resolve capabilities before clearing state or applying permissions. CI and live all/startup validation are still missing. |
…e layers - settings permission all is now a backend target: iOS runs one simctl privacy call, Android intersects the package's declared permissions from dumpsys before mutating, skipping non-changeable ids with reasons instead of stopping partway. The adapter no longer keeps a fixed expansion list. - Android serves the full upstream name table (bluetooth, calendar, location, media-library, phone, sms, storage) through pm. - Fan-out failures report applied and failed mutations; launchApp collapses to one invoke; permission values share one maestro constant; duplicate-key and empty-map checks consolidated; TAP/ASSERT kind lists unified; settings app precedence noted.
…fallback A notifications-only unset must not clear microphone, location and other permissions through the reset-all sledgehammer. The probe gate rejects unlisted notifications again; the reset-all fallback stays for runtimes that list the service but block the direct reset. Regression proves a microphone grant survives the failed reset.
|
Moving all into the platform backends addresses the fixed-list problem at 682d43d. Two correctness gaps remain. On iOS, a listed notifications service that rejects reset still falls back to reset all. A notifications-only request can clear microphone and location grants. Fail the targeted operation instead and cover the listed-but-blocked case while preserving another grant. On Android, tryPmUnit treats every nonzero result as a skip, and tryPhotosUnit catches every error. An offline device or failed operation can therefore let launchApp continue with incomplete permissions. Skip only established non-changeable permissions; propagate operational failures and add regression coverage. The change adds roughly 812 net production lines, including broader Android permission support. Please account for that growth and explain why the existing permission paths cannot support a smaller design. Current live evidence predates these changes; all-permission and startup-order verification are still needed, and this head has no CI checks. |
|
The latest coverage run also fails because the Maestro fuzz inventory does not cover setPermissions (scripts/fuzz/validation-arbitraries-maestro.test.ts). Please add a meaningful generator case alongside the existing requested fixes and verify the coverage lane. The Android smoke failure at automation-press looks unrelated. |
Summary
Maestro manages permission state because OS prompts ("Allow Camera Access") typically appear only once, so a flow without explicit permission setup is order-dependent. Our engine rejected both halves of that: standalone
setPermissionswas a declaredwe-rejectdivergence (upstream/131_setPermissions), andlaunchApp.permissionsfailed as an unknown field. Both now parse and execute:setPermissions: { appId?, permissions }withallow|deny|unset, the iOS granularlocation: always|inuse|neverandphotos: limited, and bare ${VAR} values (JS expressions are rejected at parse time, same rule asassertTrue).allexpands at runtime to the platform's servable set with specific entries overriding regardless of authored order.launchApp.permissionsaccepts the same map and applies it after the (possibly state-clearing) launch, so grants land on the fresh install rather than being wiped by it. AlaunchAppwithoutpermissionstouches nothing, as before — there is intentionally no silentall: allowdefault.settings permissionplatform backends through a fan-out in the Maestro daemon adapter, so no native code changed. Names the backends cannot serve yet (Android location/calendar/custom IDs; iOS speech/homekit/etc.) failUNSUPPORTED_OPERATIONwith hints instead of being silently skipped; extending those backends needs device verification and is tracked as a follow-up.appIddiffering from the session app) rides a new daemon-internal request key, which never travels off the wire — thesettingspositionals carry no app slot.upstream/131_setPermissionsclassifiesidenticalagainst the pinned upstream parser capture, so the waiver is removed rather than amended.Incidental refactor the quality gate required: the two canonicalizer switches in
conformance-normalize.tswere already over the complexity threshold on main, and any new command kind touches them. Bare single-shape cases collapsed to lookup tables and the tap/assert groups extracted to helpers; agent-side cyclomatic 34→23 with theneverexhaustiveness check intact. No behavior change — the oracle suite below proves it.Validation
Tested at
8d0802621afterpnpm install --frozen-lockfile:Maestro package suite: 224 passed (26 files), including new parser tests (maps, ${VAR}, optional/label, empty-map and bad-value rejections) and a dispatch test proving ${VAR} resolution flows into the port input.
Daemon Maestro adapter suite: all pass, including new mapping tests (all-expansion order per platform, iOS granular values, alias, loud rejections), projection tests, and fan-out tests proving
settingscalls and the launch-before-permissions ordering.Conformance oracle: 14/14, with
131_setPermissionsexplicitly verifiedidentical; every other non-identical flow is a declared divergence.tsc --noEmitclean repo-wide;pnpm formatclean;fallow audit --base upstream/mainexits 0 (the two remaining CRITICAL complexity notes are inherited pre-existing debt, excluded by attribution).Live device evidence (all verified against on-device state, not just replay success):
iOS 17 Pro simulator (iOS 26.3), app under test com.apple.mobilesafari:
launchAppwithpermissions: { microphone: deny }thensetPermissions{ microphone: allow, location: unset }replayed 2/2. TCC.db confirmedkTCCServiceMicrophone = allowedand no location row (prompt state).setPermissions { microphone: deny }replayed 1/1; TCC.db confirmedauth_value = denied. A finalunsetrun removed the row, restoring prompt state, and the simulator was shut down.camera: allowon this runtime fails loudly withUNSUPPORTED_OPERATIONnaming the supported services — iOS 26.3 simctl privacy has no camera service. That is the designed loud-rejection path working live, and it means the Camera rows in the Maestro permission table need a runtime probe, not just a name map (follow-up).Android Pixel 9 (API 36), app under test com.callstack.agentdevicelab (declares RECORD_AUDIO). The already-running Pixel_9 (emulator-5554) was owned by another session and left untouched, so verification ran on a temporary copy of the Pixel_9 AVD (deleted afterward):
launchAppwithpermissions: { microphone: deny }replayed 1/1.setPermissions { microphone: allow }replayed 1/1;dumpsys packageconfirmedRECORD_AUDIO: granted=true.setPermissions { microphone: unset }replayed 1/1; confirmedgranted=falsewith permission flags cleared (true reset, not just revoke).CI on this head is the authority still to come.