Skip to content

feat(maestro): support setPermissions and launchApp.permissions - #2363

Open
Rohit3523 wants to merge 23 commits into
callstack:mainfrom
Rohit3523:feat/maestro-setPermissions
Open

Rohit3523 wants to merge 23 commits into
callstack:mainfrom
Rohit3523:feat/maestro-setPermissions

Conversation

@Rohit3523

@Rohit3523 Rohit3523 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Maestro setPermissions + launchApp.permissions parse and execute (allow|deny|unset, iOS-only location: always|inuse|never and photos: limited, bare ${VAR}; JS expressions rejected). all resolves in the backends (one simctl privacy … all call on iOS, declared-permission intersection on Android) with specifics overriding; launch permissions apply after state clearing but before launch; no silent all: allow default. Unservable names fail UNSUPPORTED_OPERATION with hints. upstream/131_setPermissions now classifies identical.

Latest head: Maestro admission sets and hint text derive from explicit MAESTRO_* allowlists in contracts (native-only contacts-limited/location-always can never leak into hints); the test-only Android parser export is removed; Android all skips role-managed ids (WRITE_SETTINGS) instead of aborting. Scope is ~36 files; the growth is backend widening the Maestro surface requires (calendar/location/media-library, multi-id intersection, all resolver) — a thin translator alone would emit targets the old backends reject.

- launchApp:
    clearState: true
    permissions:
      all: deny
      camera: allow
- setPermissions:
    permissions:
      notifications: unset

Validation

Tested at a3e35d20f: 78 focused tests pass; tsc --noEmit clean; pnpm format clean; fallow audit --base upstream/main has only the 2 inherited findings; check:affected --base upstream/main --run all runnable checks pass.

Live via test --maestro on this head (on-device state, not just replay): Android lab app microphone: allow (RECORD_AUDIO granted=true), all: deny (granted=false), launchApp{clearState, mic: allow} (grant survives); camera2 camera: allow and clearing launch (CAMERA granted); system contacts-app all: deny + contacts: allow (contacts re-granted, all other runtime granted=false); iOS Safari all: allow (TCC allowed rows) then all: unset (prompt state); notifications: unset fails loud with supported-services hint, microphone grant preserved.

CI on the pushed head is pending. READ-only gap closed live after the above: lab app rebuilt with READ_CONTACTS only (scratch app.config.js change, reverted; original APK restored) — all: deny, contacts: allowREAD_CONTACTS granted=true, RECORD_AUDIO granted=false, no WRITE_CONTACTS in the dump; contacts: denycontacts: allow 2/2.

@Rohit3523
Rohit3523 force-pushed the feat/maestro-setPermissions branch from dd06012 to 8d08026 Compare September 6, 2026 15:18
@Rohit3523 Rohit3523 changed the title feat: support Maestro setPermissions feat(maestro): support setPermissions and launchApp.permissions Sep 6, 2026
@Rohit3523
Rohit3523 marked this pull request as ready for review September 6, 2026 17:04
@thymikee

thymikee commented Sep 6, 2026

Copy link
Copy Markdown
Member

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.
@thymikee

thymikee commented Sep 6, 2026

Copy link
Copy Markdown
Member

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 thymikee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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: from dumpsys package before issuing anything (permission-grant-state.ts already parses that dump).
  • iOS: simctl privacy accepts all as a service natively, and the backend already uses reset all as a fallback. all: X can be one settings permission <action> all call followed by the specific overrides. That removes ALL_EXCLUDED_PERMISSIONS entirely 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.

  • mapMaestroPermission takes expandable as a parameter that is derivable from platform.
  • applyPermissions is a one-line wrapper with one caller; inline it into setPermissions.
  • The empty-map check is repeated by both callers of readSetPermissionsMap with different messages; move it into the reader.
  • isAgentTapCommand/isAgentAssertCommand restate their kind lists by hand. A const TAP_KINDS = [...] as const with .includes keeps the guard and the Extract union in sync.
  • daemon-request.ts: the settingsAppBundleId comment is good. Worth one line in snapshot-settings.ts too, 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.
@thymikee

thymikee commented Sep 6, 2026

Copy link
Copy Markdown
Member

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.
@thymikee

thymikee commented Sep 6, 2026

Copy link
Copy Markdown
Member

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.

@thymikee

thymikee commented Sep 7, 2026

Copy link
Copy Markdown
Member

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.

…l Android failures, cover setPermissions in fuzz
@thymikee

thymikee commented Sep 7, 2026

Copy link
Copy Markdown
Member

At 752b88d, the targeted iOS reset and Android operational-error fixes address the previous failures, and setPermissions is now in the fuzz inventory. One design gap remains: the all-permission path decides whether to continue by matching raw pm stderr, including nested photos attempts. Classify those outcomes once at the Android permission boundary and let the fan-out consume typed reasons, with unknown failures still aborting.

Please also update the live evidence for all-permission handling and permission-before-startup ordering. The reported device runs predate those changes. The roughly 823 net production lines still need a short growth breakdown and an explanation of why a smaller design was rejected. This head has no CI results yet.

@thymikee

thymikee commented Sep 8, 2026

Copy link
Copy Markdown
Member

The branch now also conflicts with main at 752b88d. The previously reported permission-classification and validation gaps remain unresolved; resolve those together with the conflict before rerunning the affected checks.

…ssions

# Conflicts:
#	packages/maestro/src/internal/__tests__/program-ir-parser.test.ts
#	packages/maestro/src/internal/__tests__/runtime-port-fixtures.ts
#	packages/maestro/src/internal/conformance-normalize.ts
#	packages/maestro/src/internal/program-ir-command-parser.ts
#	packages/maestro/src/internal/program-ir.ts
#	packages/maestro/src/internal/runtime-port-commands.ts
#	packages/maestro/src/internal/runtime-port-types.ts
#	packages/maestro/test/conformance/expected-divergence.ts
#	scripts/fuzz/validation-arbitraries-maestro.ts
#	src/daemon/adapters/maestro/__tests__/daemon-runtime-public-operation.test.ts
#	src/daemon/adapters/maestro/daemon-runtime-port.ts
#	src/daemon/adapters/maestro/daemon-runtime-public-operation.ts
@Rohit3523

Copy link
Copy Markdown
Contributor Author

Done with the changes

@thymikee

Copy link
Copy Markdown
Member

Reviewed at e656ff1, as a follow-up to the review at 752b88d. Two backend problems remain, and the shared permission list is still declared in several places.

parseAndroidPermissionTarget (packages/platform-android/src/settings-permission.ts:451) still runs the contracts parsePermissionTarget first, and that parser does not accept bluetooth, phone, sms or storage. So those rows in ANDROID_PERMISSION_TABLE can never be reached, even though EXPANDABLE_PERMISSIONS.android and UNSUPPORTED_HINTS (set-permissions-mapping.ts:19) advertise them. A Maestro setPermissions: {bluetooth: allow} passes adapter validation and then fails in the backend with "permission setting requires a target", after earlier entries in the map already landed. Please either add the four names to the contracts PermissionTarget list and the CLI copy in src/commands/capture/settings.ts, with a backend test per name, or drop them from the table and the adapter list.

Named multi-id targets now run a strict pm grant|revoke for every id in the table (settings-permission.ts:395): contacts is READ+WRITE_CONTACTS, location is FINE+COARSE, calendar is READ+WRITE. Doesn't pm fail when the app does not declare one of those ids? If so, agent-device settings permission grant contacts now fails on an app that declares only READ_CONTACTS, which worked before this PR, and Maestro location: allow fails the same way on coarse-only apps. The all path already intersects with the declared permissions from dumpsys; could the named path use the same resolver? The fake-adb test at settings-permission.test.ts:326 accepts every id, so a case where the second id is rejected would catch this.

The set of servable names is declared four times: EXPANDABLE_PERMISSIONS, the UNSUPPORTED_HINTS text, ANDROID_PERMISSION_TABLE with its error string (settings-permission.ts:479), and contracts PERMISSION_TARGETS plus the CLI copy. That drift is what causes the first problem. Could per-platform targets live once in contracts, with adapter validation, hint text and backend parsing derived from them?

website/docs/docs/commands.md:732 still lists the old Android targets. The CLI now accepts all on both platforms and calendar/location/media-library on Android, contacts also changes WRITE_CONTACTS, and the deny/reset permission result is now a comma-joined list. Please update that page, add a CHANGELOG line for the CLI change, and refresh the PR body, which still says no native code changed. Two small leftovers from the merge can also go: the StopAppCommand row in BARE_UPSTREAM_CANONICAL (conformance-normalize.ts:138) is now dead, and mapMaestroAll repeats checks the parser already owns.

The PR adds about 816 net production lines. Could the Maestro layer stay a thin name/value translator that emits one settings permission call per entry, with Android using one resolver for both named targets and all? That would remove EXPANDABLE_PERMISSIONS, the hint text and the strict pm loop, and fix the first two problems by construction. The Android table expansion (bluetooth/phone/sms/storage and multi-id fan-out) also goes beyond the Maestro mapping in the issue and could be its own PR with device evidence. What would need to land first is a set of per-platform target constants in packages/contracts. If the larger shape is needed, can you say why the smaller one does not work?

The live runs in the PR body are from 8d08026 and cover only microphone. Since then the Android all and multi-id paths and the iOS notifications and all paths changed. A live Android run of setPermissions {all: deny, contacts: allow} on an app that declares only some of the ids, and a live iOS simulator run of all: allow and all: unset, both on this head, would cover the changed routes.

All 15 checks pass on e656ff1, and there are no conflicts. The next step is to fix the unreachable Android targets and the strict multi-id fan-out, then add the live Android and iOS evidence on the new head.

…ssions

# Conflicts:
#	packages/contracts/src/client-settings.ts
#	packages/contracts/src/settings.ts
#	src/commands/capture/settings.ts
@Rohit3523

Copy link
Copy Markdown
Contributor Author

All green :)

@thymikee

Copy link
Copy Markdown
Member

Reviewed at 9f187e6. Only a merge from main landed since e656ff1, so the review at e656ff1 still applies in full: the unreachable Android bluetooth/phone/sms/storage targets (settings-permission.ts#L451), the strict pm grant/pm revoke loop over multi-id targets such as contacts and location (L395), the permission names declared in several places, the stale commands.md and missing CHANGELOG entry, and the question whether a thin Maestro translator would do instead of about 810 net production lines.

Smoke Tests fails on RunnerTests.testAlertDismissDoesNotActivateAReplacementWithTheSameTitle. This diff touches no runner Swift or alert code, so that failure looks unrelated.

Live runs on this head are still needed. On Android: setPermissions {all: deny, contacts: allow} against an app that declares READ_CONTACTS but not WRITE_CONTACTS, with dumpsys package output, and launchApp {clearState: true, permissions: {...}} showing the grant after the clearing launch. On an iOS simulator: {all: allow} then {all: unset}, with a notifications result. The existing evidence is from 8d08026 and covers only microphone. Next: fix the two backend problems, then add those runs.

…rgets, single-source permission sets

Drop unreachable bluetooth/phone/sms/storage from Android table and Maestro
adapter; declare ANDROID/IOS_PERMISSION_TARGETS once in contracts and derive
adapter lists, hints, and backend error strings from them.

Intersect named multi-id pm targets (contacts/location/calendar/media-library)
with dumpsys requested permissions like all does: grant via resolveNamedPmIds,
revoke via single-read revokeNamedPmTarget; fail loudly when none declared,
fall back to strict table when dump unreadable.

Cleanups: drop dead StopAppCommand bare row, lifecycle kinds const,
simplify mapMaestroAll. Update commands.md targets and CHANGELOG.
@Rohit3523

Rohit3523 commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

READ-only live evidence is in (closes item 2 of my previous comment — no test APK needed).

Setup: added permissions: ['android.permission.READ_CONTACTS'] to expo.android in examples/test-app/app.config.js, rebuilt (expo prebuild + ./gradlew assembleDebug — note: expo run:android served the disk-cached build and skipped prebuild, so the Gradle step ran directly), installed on Pixel 9 (emulator-5554). aapt2 dump badging confirmed READ_CONTACTS present, WRITE_CONTACTS absent. Scratch config reverted afterward and the original APK reinstalled; git status clean.

Run (via test --maestro, head a3e35d2):

appId: com.callstack.agentdevicelab
---
- setPermissions:
    permissions:
      all: deny
      contacts: allow

Result 1/1 pass. adb shell dumpsys package com.callstack.agentdevicelab after:

requested permissions:
  ...
  android.permission.RECORD_AUDIO
  android.permission.READ_CONTACTS
runtime permissions:
  android.permission.RECORD_AUDIO: granted=false, flags=[ USER_SENSITIVE_WHEN_GRANTED|USER_SENSITIVE_WHEN_DENIED]
  android.permission.READ_CONTACTS: granted=true, flags=[ USER_SENSITIVE_WHEN_GRANTED|USER_SENSITIVE_WHEN_DENIED]

No WRITE_CONTACTS anywhere in the dump, and the flow passed — the named intersection attempted only the declared READ id (the pre-fix strict fan-out fails this exact flow on WRITE_CONTACTS has not requested permission). Follow-up contacts: denycontacts: allow also 2/2, ending READ_CONTACTS granted=true.

@thymikee

Copy link
Copy Markdown
Member

Reviewed at a3e35d2, as a follow-up to the review at efe33f4. The three code points are fixed: the iOS admission list and its hint now both come from MAESTRO_IOS_PERMISSION_TARGETS, the parser test accepts only the two typed refusals and checks every Android target with allow, deny and unset, and parseAndroidPermissionTarget is no longer exported. The size answer settles the earlier question: the old backends could not serve the targets Maestro needs, and the per-platform table now lives in contracts. The merge from main only touches CHANGELOG.md, and both sides are kept.

Run (1), Android READ_CONTACTS-only, is now covered. The other two required live runs are described in your comment and the PR body, but no output is attached. Run (2), Android launchApp {clearState: true, permissions: {camera: allow}}, has no flow YAML and no post-launch dumpsys, so nothing shows whether a clearState launch keeps the camera grant. Run (3), iOS setPermissions {all: allow} then {all: unset}, has no captured TCC rows and no notifications state after unset; the only notifications evidence on file is for the named notifications: unset route, which now refuses loudly, not for all: unset, so it's unclear whether all: unset returns notifications to not-determined on the shipped head. Can you attach, for a3e35d2: for (2) the flow YAML for that launch step plus adb shell dumpsys package <pkg> showing android.permission.CAMERA: granted=true; and for (3) the flow output for {all: allow} then {all: unset}, the simulator TCC rows for the bundle after each step, and the notifications authorization state after unset? If all: unset can't reset notifications on this runtime, the run should show a loud failure or warning rather than a silent pass.

Not blocking: the MAESTRO_PERMISSION_VALUES guard in set-permissions-mapping.ts:134 seems to duplicate the fall-through throw since runtime-port-commands.ts:196 already rejects unknown values earlier, the 'backend-servable' set in the test at set-permissions-mapping.test.ts:220 looks like a circular check against the same constant the adapter admits from, and MAESTRO_PERMISSION_ALIASES (set-permissions-mapping.ts:41) is exported only for the test to read — happy to leave all three as-is if you'd rather not touch them.

CI is green: the packet reports 15 checks with 0 not passing at a3e35d2.

I ran no tests or devices; these notes come from reading the code at a3e35d2 and the author's comments. I couldn't confirm whether simctl privacy reset all <bundle> resets notifications authorization on the runtime you used, so I can't judge whether a silent pass of all: unset is correct, and I haven't verified the 'managed by role' stderr match is stable across Android API levels other than the API 36 case you reported.

The Android clearState+camera dumpsys output and the iOS all:allow/unset TCC-and-notifications output are what's needed before this is ready to merge.

…ssions

# Conflicts:
#	packages/maestro/src/daemon-port/__tests__/daemon-runtime-port-set-permissions.test.ts
#	packages/maestro/src/daemon-port/__tests__/set-permissions-mapping.test.ts
#	packages/maestro/src/daemon-port/set-permissions-mapping.ts
@Rohit3523

Copy link
Copy Markdown
Contributor Author

Live evidence for a3e35d2 follow-up (run on 1ffc404, local build 0.21.6, 2026-09-18T03:41Z — includes a3e35d2 + main merge, no code change to permission paths since a3e35d2).

Run (2) — Android launchApp {clearState: true, permissions: {camera: allow}}
Device: Pixel 9 emulator-5554, API 36. Pkg com.android.camera2 (declares CAMERA).

Flow YAML (run2-android-launch-camera.yaml):

appId: com.android.camera2
---
- launchApp:
    appId: com.android.camera2
    clearState: true
    permissions:
      camera: allow

Before (after adb shell pm revoke com.android.camera2 android.permission.CAMERA):

android.permission.CAMERA: granted=false, flags=[ GRANTED_BY_DEFAULT|USER_SENSITIVE_WHEN_GRANTED|USER_SENSITIVE_WHEN_DENIED]

Replay:

./bin/agent-device.mjs replay run2-android-launch-camera.yaml --maestro --platform android
Replayed 1 step in 0.7s

After:

adb shell dumpsys package com.android.camera2 | grep CAMERA
android.permission.CAMERA: granted=true, flags=[ GRANTED_BY_DEFAULT|USER_SENSITIVE_WHEN_GRANTED|USER_SENSITIVE_WHEN_DENIED]

Grant survives the clearing launch — permissions apply after clear, before open.

Run (3) — iOS {all: allow} then {all: unset}
Device: iPhone 17 Pro Max 16562605-4CCF-48F6-BCD1-19369DF43263, iOS 26.5. Bundle com.apple.mobilesafari.

Flows:

# run3-ios-allow-all.yaml
appId: com.apple.mobilesafari
---
- setPermissions:
    permissions:
      all: allow
# run3-ios-unset-all.yaml — same with all: unset

Replay (both succeed, no silent skip):

replay run3-ios-allow-all.yaml --maestro --platform ios
Replayed 1 step in 4.1s
replay run3-ios-unset-all.yaml --maestro --platform ios
Replayed 1 step in 4.1s

TCC (TCC.db access WHERE client='com.apple.mobilesafari'):
After allow:

kTCCServiceAll|2
kTCCServiceLiverpool|2

After unset:

kTCCServiceLiverpool|2

Unset returns the bundle to prompt state via Apple's reset all (rows deleted = not-determined; Liverpool is Apple's leftover, not ours). Earlier on this head an allow showed 16 allowed rows (All + Calendar/Camera/Photos/Microphone/etc. all auth 2); the variance is Apple's grant all/reset all semantics, not adapter expansion — the adapter emits one simctl privacy <action> all <bundle> call.

Notifications state after unset:

  • Targeted route fails loudly, no silent pass:
replay run3-ios-notif-unset.yaml (notifications: unset) --maestro --platform ios
Error (REPLAY_DIVERGENCE): iOS simctl privacy does not support service "notifications" on this runtime.
Hint: Supported services: all, calendar, contacts, contacts-limited, location, location-always, media-library, microphone, motion, photos, photos-add, reminders, siri
  • xcrun simctl privacy <udid> help on this runtime lists no notifications or camera service (only all/calendar/contacts/location/photos-add/photos/media-library/microphone/motion/reminders/siri). So named notifications: unset correctly refuses with UNSUPPORTED_OPERATION; all: unset goes through Apple's reset all, which succeeds. There is no separate notifications auth store I could query on the simulator beyond TCC — notifications does not appear in TCC.db services.

@Rohit3523

Copy link
Copy Markdown
Contributor Author

done, pls rereview, thanks

@thymikee

Copy link
Copy Markdown
Member

Reviewed at aa9186d. Thanks for the live runs. Run (2) shows the CAMERA grant survives the clearState launch, and the TCC rows for {all: allow} then {all: unset} look right. The lazy-load commit keeps the same call order and errors, so the runs on 1ffc404 still cover it.

One point from the a3e35d2 review is still open. On iOS, all is one simctl privacy grant|reset all <bundle> call, and your run shows simctl privacy has no notifications service on this runtime. So setPermissions {all: allow} does not grant notifications and {all: unset} does not reset them, but the step still reports success. The replay docs say unset fully resets and that unservable names fail loudly. A flow that relies on all for notifications will pass that step and fail later, far from the cause. I believe Maestro's iOS all does cover notifications through applesimutils; can you confirm? The smallest fix is to say in the replay docs and the CHANGELOG entry that iOS all does not cover notifications. A warning from the iOS all path when notifications is left unchanged would be better.

I did not run a device or tests; this comes from the code and your posted output. CI is green: 15 checks, 0 failing at aa9186d. With that note or warning in place, this is ready for human review.

@thymikee

Copy link
Copy Markdown
Member

Reviewed at a7131ca. Thanks for adding the note. The CHANGELOG, replay docs and support matrix now match the code: iOS all is one simctl call that leaves notifications unchanged, and a targeted notifications entry fails with an error instead of falling back to reset all. This is ready for human review.

One small non-blocking note: the grant/deny notifications error hint still suggests reset notifications, which now fails the same way on these runtimes, so it could reuse the reset branch's wording at L303.

CI is green: 12 checks pass and 3 are skipped. There are no conflicts.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 19, 2026
The Maestro adapter held a second copy of what each backend serves (per-platform
admission lists plus hints), which is what let the hint and admission disagree.

- Map Maestro names and values to `settings permission` mutations in the
  projection module, with a Maestro-only name table. Each backend refuses the
  targets it does not serve with its own hint. No lazy import, no contracts
  edge, no MAESTRO_PERMISSION_VALUES export from the package index.
- Drop IOS_/ANDROID_/MAESTRO_ANDROID_/MAESTRO_IOS_PERMISSION_TARGETS from
  contracts. Android keeps its target list next to its table; iOS maps every
  target through one exhaustive simctl service table.
- The runtime port only lowercases values; the parser checks literal values and
  the adapter checks each value against its permission after ${VAR} lookups.
- One `dumpsys package` reader for every Android permission path.
- Android refuses any permission mode with an accurate message.
@thymikee

Copy link
Copy Markdown
Member

Reviewed at 12d00dd. The refactor in 68dac57 moves permission admission into the backends, and this lets a rejected map partly change an Android device. mapMaestroSetPermissions (here) no longer takes the platform. So motion, reminders, siri, location: always and photos: limited now map on every platform, and only the Android backend refuses them. applyPermissionMutations (here) applies the mutations one by one. On Android, setPermissions {camera: allow, motion: allow} grants camera and then fails. launchApp {clearState: true, permissions: {location: always}} clears app data and then fails. Before this delta the adapter refused the whole map before any change. The error was also a Maestro-level refusal, not INVALID_ARGS for location-always. The "launchApp with rejected permissions launches nothing" test uses health, which every platform still refuses, so it does not catch this.

The smallest fix is to check the whole map against the platform's served targets and modes before clearState or the first mutation, for both setPermissions and launchApp. The platform is already in scope in createDaemonMaestroRuntimeParts. Could you add an Android port test with {camera: allow, location: always} and clearState: true that asserts no requests were sent? It should fail on 12d00dd and pass after the fix.

One small question: on Android, location: inuse and location: never now grant and deny location instead of failing as iOS-only (here). Is that intended? If yes, a note in the CHANGELOG and replay docs would help. If not, the same platform check can refuse them.

CI was still running at review time. Typecheck & Package, Repo Guards, Coverage and Integration Tests all cover the changed code, so a failure there is likely related. The admission fix is the next blocker before this is ready for human review again.

@thymikee thymikee removed the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 19, 2026
`simctl privacy` has no capability query, and its help text is not one. Xcode 26
(17C52) omits `camera` from the service list while granting it: the probe gate
refused `settings permission grant camera` and a Maestro `camera: allow` as
UNSUPPORTED_OPERATION on every flow that asked for the camera, while the raw
command worked (TCC `kTCCServiceCamera` 0 -> 2). The same omission was reported
for notifications on the runtimes in callstack#2363.

Delegate the verdict to the command that would make the change. A service the
runtime cannot change answers EPERM — for a real service it withheld and for a
name it does not know alike — so generalize the notifications classifier to
every target and read the refusal from that call. Xcode 26 words grant/revoke
failures as "Failed to set access", which the old matcher did not look for, so
the generalization is required and not merely a widening.

Deletes the cached help probe, its parser, and the `privacy help` spawn before
the first permission change; a grant is now one simctl call.

Records why a Maestro `location: inuse` or `location: never` reaching the plain
Android target is correct rather than an unguarded iOS leak: `pm grant` leaves a
location appop of `foreground`, which is Android's only allow level. The
fan-out's ordered, stop-at-first-refusal shape is now stated in the versioned
Maestro compatibility reference, the replay docs, and the changelog, and the
"iOS all does not cover notifications" line is corrected: notifications has no
service on current runtimes, named or under `all`.
@thymikee

Copy link
Copy Markdown
Member

Pushed ab84e0da onto this branch. It resolves the admission question, and please do not add the "no requests were sent" test I asked for — I measured it and the pre-check I requested would have made things worse. My earlier comment was wrong on the mechanism.

Why no platform pre-check. On iOS the only capability oracle is simctl privacy itself, tried. There is no list to check against: simctl privacy help on Xcode 26.2 (17C52) omits camera while simctl privacy grant <udid> camera <bundle> succeeds (TCC kTCCServiceCamera 0 → 2). That gate is live on 12d00dd: settings permission grant camera and any Maestro camera: allow returned UNSUPPORTED_OPERATION. A preflight built from that list would refuse {all: deny, camera: allow} outright — turning a partial application into a total failure, for a false reason.

So the verdict now comes from the command that would make the change. runIosPrivacyCommand attempts it and classifies the EPERM; the cached privacy help probe, its parser and its cache are gone (net −108 lines, one simctl spawn per grant instead of two). Xcode 26 words grant/revoke refusals as Failed to set access, which the old notifications matcher did not look for, so generalizing the matcher is required and not merely a widening. notifications still fails loudly, as it should.

Partial application stands as specified behavior, not a gap: applyPermissionMutations is documented and tested to stop at the first refusal and name what landed (a mid-sequence backend rejection names what already landed), upstream Maestro is non-atomic the same way, and after clearState: true the app state is gone because the flow asked for it. Stated now in the versioned capability reference and the replay docs.

location: inuse / never on Android is intended, which answers your question — and it is not an over-grant. pm grant ACCESS_FINE_LOCATION leaves the appop at foreground, which is Android's only allow level, so inuse/never map onto the only two states the platform has. location: always and photos: limited have no Android equivalent and the backend still refuses them. Recorded as a comment on MAESTRO_GRANULAR_PERMISSIONS plus the docs, so nobody "fixes" it next round.

Two follow-up issues cover the Android all fan-out debt I chose not to widen this PR into.

Gate note: check:replay-compat and check:daemon-wire-compat refuse to run on a shallow clone (--is-shallow-repository=true) and need git fetch --unshallow --tags; they pass on CI. Everything else runs green locally: check:quick, maestro:conformance, and check:affected --base origin/main (472 files / 3458 tests).

@thymikee

Copy link
Copy Markdown
Member

Reviewed at ab84e0d. The code reads clean and nothing new blocks it. Not blocking: the daemon-runtime-port comment still describes the old all-or-nothing validation and should say the map's names are validated up front while a backend refusal stops the sequence with earlier grants applied, the all refusal hint on iOS should stop pointing at a target that still leaves notifications unchanged, the widened privacy-refusal regex has no test for a privacy failure that isn't "Operation not permitted" staying COMMAND_FAILED, and it would help to see the actual xcrun simctl privacy ... revoke notifications stderr and the resulting agent-device settings permission deny notifications error code on the same Xcode 26.2 runtime, since the classification currently rests on wording described in prose rather than pasted output — all of these can be taken or left.

This is a follow-up on the 12d00dd review; the earlier findings are addressed, and nothing new blocks the change.

The branch has merge conflicts with main and can't be merged as-is.

No checks have reported on ab84e0d yet. The maintainer's local runs of check:quick, maestro:conformance and check:affected are green, and check:replay-compat and check:daemon-wire-compat still need an unshallow clone to run; since this change touches packages/platform-apple and the maestro support matrix, a failure in Typecheck & Package, Coverage, or Integration Tests once CI runs would be relevant here.

I have no simulator to run this against, so the Xcode 26 behavior (help text omitting camera while the grant still works, refusals worded "Failed to set access") is the maintainer's own measurement, and I only checked it against the fake tool; I read the changed tests without running them, and the "red on 12d00dd" read comes from tracing the removed code path rather than executing it. The Android parser export and the Android all fan-out gap stay out of scope for this delta, as covered in earlier rounds.

Before this can be merged, the maestro daemon-port files need to move from packages/maestro/src/daemon-port (the location on the merge base) to packages/daemon/adapters/maestro, since main no longer has the old directory; that relocation covers the two production files and both new port test files and should clear the conflict so the checks can run.

# Conflicts:
#	CHANGELOG.md
#	packages/platform-android/src/permission-grant-state.ts
#	packages/platform-android/src/settings-permission.ts
#	src/daemon/daemon-request.ts
#	src/daemon/replay/internal/__tests__/session-replay-maestro-request.test.ts
@thymikee

Copy link
Copy Markdown
Member

Reviewed at 55466f9. git range-diff shows all sixteen commits byte-identical to ab84e0d, so the rebase onto c442cf6 is clean and the review there still stands. The rebase also cleared the conflict on its own, so the file relocation I described last time is not needed — packages/maestro/src/daemon-port is still present on main, and my note that it had moved was wrong. Smoke Tests fails on the live simulator fixture step at id="automation-longpress" did not become visible after scrolling, but PR #2715 hit the same assertion in the same job with no code in common, and the iOS workflow passed on main at this merge base, so I read that failure as unrelated to your change. I applied ready-for-human.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 20, 2026
@thymikee

Copy link
Copy Markdown
Member

The code review at 55466f9 still stands, but the branch now conflicts with main. The only conflict is in CHANGELOG.md, after recent entries landed on main. Could you rebase and resolve it? I removed ready-for-human until the branch merges cleanly again. A rebase with only the changelog resolved needs no new review.

@thymikee thymikee removed the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 21, 2026
…missions

* origin/main:
  feat(diff): accept JPEG inputs for screenshot comparison (callstack#2718)
  refactor(system-button): declare home, app-switcher and action-button as one button family (callstack#2715)

# Conflicts:
#	CHANGELOG.md
@thymikee

Copy link
Copy Markdown
Member

Merged main into this branch (c0df8ee3, fast-forward). Your 55466f9 had taken main to 0.21.7, which was two commits short — 4376b9c (system-button family) and be0902e (JPEG diff) — and that gap is what GitHub was calling CONFLICTING. The only conflict was CHANGELOG.md; mergeable is now MERGEABLE.

Nothing else needed resolving. Your own device-shell migration resolution was already correct: no runAndroidAdb(device, ['shell', …]) survives on this branch, so d688926's refusal of a shell argv has nothing to trip, and settingsAppBundleId already sits in ReplayDispatchOptions rather than being re-declared on the daemon's request-private half — one owner, which is what the Pick<ReplayDispatchOptions, keyof MaestroDaemonDispatchOptions> lock in session-replay-maestro-request.ts is for.

Revalidated on the new head, with a live run of the changed path since the merge sits under it:

  • settings permission deny microphone and deny all on an emulator — RECORD_AUDIO and CAMERA really flipped, and all still skipped WRITE_SETTINGS ("managed by role") and the non-changeable ids rather than aborting.
  • setPermissions {camera: allow, motion: allow}CAMERA went granted=falsetrue before the step failed on motion, which is the ordered stop-at-first-refusal shape working as specified.
  • Cross-app targeting: session app com.android.settings, flow appId: com.android.camera2CAMERA flipped, so the dispatch key still reaches the settings handler through the replay envelope.
  • settings permission grant camera on an iOS simulator — kTCCServiceCamera absent → 2.
  • pnpm check:affected --base origin/main --run: all runnable checks passed, 513 files / 3870 tests, typecheck and layering clean.

One caveat: check:affected printed check:affected: lint failed. once partway through, with no oxlint diagnostics in the log, and lint passed standalone and in every later stage. It looked like a stage flake, not a finding — flagging it in case your CI says otherwise.

@thymikee

Copy link
Copy Markdown
Member

Reviewed at c0df8ee. The merge of main resolves the CHANGELOG.md conflict, and the PR's own changes are byte-identical to 55466f9, so the code review there still stands.

Smoke Tests now fails in the iOS runner XCTest step on testAlertAcceptDoesNotActivateAReplacementWithASharedButton (ALERT_DEADLINE_EXCEEDED). This PR does not touch the runner, the same test passed on this PR's earlier runs, and the iOS workflow passed on main at be0902e, so I read it as unrelated. A rerun would confirm it.

There are no conflicts now, and I applied ready-for-human again.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants