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. |
…l Android failures, cover setPermissions in fuzz
|
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. |
|
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
|
Done with the changes |
|
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.
Named multi-id targets now run a strict The set of servable names is declared four times: website/docs/docs/commands.md:732 still lists the old Android targets. The CLI now accepts The PR adds about 816 net production lines. Could the Maestro layer stay a thin name/value translator that emits one The live runs in the PR body are from 8d08026 and cover only microphone. Since then the Android 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
|
All green :) |
|
Reviewed at 9f187e6. Only a merge from main landed since e656ff1, so the review at e656ff1 still applies in full: the unreachable Android Smoke Tests fails on Live runs on this head are still needed. On Android: |
…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.
|
Reviewed at e4dd9a4, as a follow-up to the review at 9f187e6. Three earlier points are fixed: the unreachable Android Android can still receive values it rejects.
The file now reads The new intersection tests cover grant only. Could you add deny and reset cases against a READ_CONTACTS-only dumpsys, with a fake that fails The PR body still says Android location and calendar fail with The change is still about 926 net production lines, and the growth has not been explained. Would a thin Maestro translator that emits only the (target, mode) pairs the existing CI is green on e4dd9a4, and there are no conflicts. Live runs on this head are still needed: on Android, |
|
Reviewed at efe33f4, as a follow-up to the review at e4dd9a4. Four earlier points are fixed: the adapter now refuses iOS-only granular values on Android instead of emitting The iOS error and its hint now disagree. The new parser test can pass without checking anything (set-permissions-mapping.test.ts#L144). It wraps each mapping call in a bare Two new public exports look test-only. The size question from the earlier review has no answer yet. The change is about 968 net production lines, above the 700-line threshold, and this update added a shared reader and a platform guard without reducing scope. Would a thin Maestro translator that emits only the (target, mode) pairs the existing The PR has merge conflicts with main, and no CI checks ran on efe33f4. The PR body is still not refreshed for this head. Live runs on this head are still needed: (1) on Android, against an app that declares READ_CONTACTS but not WRITE_CONTACTS, Next: resolve the conflicts, fix the iOS hint and the parser test, answer the size question, then attach those runs. |
… skip role-managed ids Maestro iOS admission and hint derive from explicit MAESTRO_* lists in contracts; adapter validates values with MAESTRO_PERMISSION_VALUES. parseAndroidPermissionTarget leaves the mechanics surface so the mapping test pins backend-servable pairs against the contracts table. Android all skips role-managed ids such as WRITE_SETTINGS instead of aborting.
|
Addressed the review at efe33f4 in a3e35d2 (pushed):
Live on this head via test --maestro (dumpsys/TCC verified): lab-app mic allow / all-deny / clearing launchApp, camera2 camera allow + launch, contacts all-deny + contacts-allow, iOS all allow/unset, notifications-unset loud failure preserving mic. One live find fixed here: Android all now skips role-managed ids (WRITE_SETTINGS 'managed by role') with a unit test. What I need:
|
|
READ-only live evidence is in (closes item 2 of my previous comment — no test APK needed). Setup: added Run (via appId: com.callstack.agentdevicelab
---
- setPermissions:
permissions:
all: deny
contacts: allowResult 1/1 pass. No |
|
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 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 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 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
|
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}} Flow YAML (run2-android-launch-camera.yaml): appId: com.android.camera2
---
- launchApp:
appId: com.android.camera2
clearState: true
permissions:
camera: allowBefore (after Replay: After: Grant survives the clearing launch — permissions apply after clear, before open. Run (3) — iOS {all: allow} then {all: unset} Flows: # run3-ios-allow-all.yaml
appId: com.apple.mobilesafari
---
- setPermissions:
permissions:
all: allow
# run3-ios-unset-all.yaml — same with all: unsetReplay (both succeed, no silent skip): TCC ( After unset: Unset returns the bundle to prompt state via Apple's Notifications state after unset:
|
|
done, pls rereview, thanks |
|
Reviewed at aa9186d. Thanks for the live runs. Run (2) shows the CAMERA grant survives the One point from the a3e35d2 review is still open. On iOS, 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. |
Summary
Maestro
setPermissions+launchApp.permissionsparse and execute (allow|deny|unset, iOS-onlylocation: always|inuse|neverandphotos: limited, bare${VAR}; JS expressions rejected).allresolves in the backends (onesimctl privacy … allcall on iOS, declared-permission intersection on Android) with specifics overriding; launch permissions apply after state clearing but before launch; no silentall: allowdefault. Unservable names failUNSUPPORTED_OPERATIONwith hints.upstream/131_setPermissionsnow classifiesidentical.Latest head: Maestro admission sets and hint text derive from explicit
MAESTRO_*allowlists in contracts (native-onlycontacts-limited/location-alwayscan never leak into hints); the test-only Android parser export is removed; Androidallskips 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,allresolver) — a thin translator alone would emit targets the old backends reject.Validation
Tested at
a3e35d20f: 78 focused tests pass;tsc --noEmitclean;pnpm formatclean;fallow audit --base upstream/mainhas only the 2 inherited findings;check:affected --base upstream/main --runall runnable checks pass.Live via
test --maestroon this head (on-device state, not just replay): Android lab appmicrophone: allow(RECORD_AUDIO granted=true),all: deny(granted=false),launchApp{clearState, mic: allow}(grant survives); camera2camera: allowand clearing launch (CAMERA granted); system contacts-appall: deny + contacts: allow(contacts re-granted, all other runtimegranted=false); iOS Safariall: allow(TCC allowed rows) thenall: unset(prompt state);notifications: unsetfails 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_CONTACTSonly (scratchapp.config.jschange, reverted; original APK restored) —all: deny, contacts: allow→READ_CONTACTS granted=true,RECORD_AUDIO granted=false, noWRITE_CONTACTSin the dump;contacts: deny→contacts: allow2/2.