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.
|
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. |
|
Reviewed at a7131ca. Thanks for adding the note. The CHANGELOG, replay docs and support matrix now match the code: iOS One small non-blocking note: the grant/deny notifications error hint still suggests CI is green: 12 checks pass and 3 are skipped. There are no conflicts. |
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.
|
Reviewed at 12d00dd. The refactor in 68dac57 moves permission admission into the backends, and this lets a rejected map partly change an Android device. The smallest fix is to check the whole map against the platform's served targets and modes before One small question: on Android, 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. |
`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`.
|
Pushed Why no platform pre-check. On iOS the only capability oracle is So the verdict now comes from the command that would make the change. Partial application stands as specified behavior, not a gap:
Two follow-up issues cover the Android Gate note: |
|
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 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 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
|
Reviewed at 55466f9. |
|
The code review at 55466f9 still stands, but the branch now conflicts with main. The only conflict is in |
…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
|
Merged Nothing else needed resolving. Your own device-shell migration resolution was already correct: no Revalidated on the new head, with a live run of the changed path since the merge sits under it:
One caveat: |
|
Reviewed at c0df8ee. The merge of main resolves the Smoke Tests now fails in the iOS runner XCTest step on There are no conflicts now, and I applied |
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.