fix(android): warn when a permission revoke kills the session app - #1856
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
|
P1 state-truth blocker at exact |
0350ce6 to
148a6fe
Compare
|
Out of draft. CI is green on the actual head Second commit Re-verified end to end on the new text: red-then-green unit proof re-ran ( One |
|
Re-reviewed exact head |
|
Blocker accepted, and the "masked in practice" wave-through was wrong — the install-section scan is reachable, not just theoretically. Fixed in The new state model
The read itselfThe reader moved to its own module (
On the fixture package the install section really does carry eight RegressionsNew Planted red — restoring the pre-fix model (global Restored: Live (Pixel 7 CI, API 36,
|
|
P1 at exact |
|
P1 confirmed on hardware and fixed in The defect, proven on devicePixel 7 CI / API 36, secondary user created, foreground switched to it ( So FixThe foreground user is resolved once per permission command and passed explicitly to every mutation — The regression you asked for
plus Planted red — restore the bare calls ( Live multi-user evidence, post-fixSame device, foreground still 10, through the built CLI: The kill is the part the old code could never produce on this device: it was revoking a user whose app was not running. Secondary user removed, foreground restored to 0, session closed, daemon stopped, emulator shut down. Provider lane
CI on |
|
Re-reviewed exact head |
thymikee
added
the
ready-for-human
|
Size and design pass done in 1. The ratchetFixed before this message, and the pin went down, not up: 2. Size pass — measured
Net −211 lines across the touched set, and no file in it is over its tripwire any more: Deleted as dead or defensive:
Kept, and load-bearing despite looking defensive: Tests: higher coverage, fewer cases. The permission tests moved to 3. Every red-proof re-run after the consolidation
Nothing was weakened to save lines: the tri-state, the
One note on the previous head |
|
CI green on |
|
[P1] Do not mutate after losing the acting-user identity. When |
thymikee
removed
the
ready-for-human
settings permission deny|reset maps to pm revoke, and Android kills the app's process whenever a runtime permission it currently holds is revoked, so a grant -> deny/reset sequence silently left the session on the launcher and the next selector failed with no hint. The revoke path now reads the prior grant state from dumpsys package first and, when it was granted, returns wasGranted: true plus a warning naming open <app> --relaunch; the settings CLI output renders response warnings, and commands.md documents the behavior next to the pm revoke mapping. Closes #1796
Review finding: the warning asserted the app had been killed, inferred only from the prior grant state. dumpsys reports granted=true for any user profile while pm revoke acts on the current one, and the app need not have been running, so the claim could be false. State the platform rule and make the consequence conditional; the relaunch guidance is unchanged.
A failed or unparseable dumpsys read as "nothing granted", so the response asserted the app was untouched when the state was simply unknown, and the grant scan matched every granted=true line — install permissions and other users' blocks included — so another profile's grant could claim a kill that never happened. Both directions of the same defect. The read now resolves the acting user (am get-current-user) and walks the dump's nesting (Packages: > User <id>: > runtime permissions:), and reports priorGrantState: granted | not_granted | unknown. unknown carries the same relaunch guidance without claiming what the state was; only not_granted is silent.
The tri-state read scoped state to am get-current-user, but the mutations ran bare pm grant/revoke and clear-permission-flags. PackageManagerShellCommand defaults those to UserHandle.USER_SYSTEM, so on a device whose foreground user is nonzero the command read one user's state and edited user 0 — leaving the running app's permission untouched while reporting on a user it did not change. Proven on a Pixel 7 / API 36 emulator with the foreground user switched to 10: a bare pm revoke flipped User 0 to granted=false and left User 10 granted=true. The foreground user is now resolved once and passed as --user to pm grant/revoke, pm clear-permission-flags, and appops set, and the state read takes that same id. When it cannot be resolved the mutation keeps the platform default and the state is reported unknown rather than guessed.
…enario The scripted ADB provider answered only the unscoped pm grant/revoke form, and the Settings contract asserted the unscoped transcript entry, so the provider lane could not see which user a permission mutation addressed.
…olith The user-scoped argv assertions pushed android-lifecycle.test.ts past its size ratchet, whose instruction is to extract rather than grow a file over the tripwire. assertAndroidSettingsContract moves to a sibling module and the pin drops 1597 -> 1559.
Size/design pass on the #1796 change: - settings.ts was 505 lines (past the 500 extract-before-adding tripwire); the permission family moves to settings-permission.ts and the dispatcher drops to 265. - permission-grant-state.ts loses topLevelSection (a nestedBlock with an indent-0 header), its single-use line reader, and androidPriorGrantState (one map lookup at its only production call site). - the grants map narrows to 'granted' | 'not_granted': unknown was never a value, absence is what carries it, so the tests read the map directly. - the permission tests move to settings-permission.test.ts and consolidate into argv/tri-state/photos/rejection tables; the parser tests fold seven cases into two. Every red-proof re-run after the consolidation: dropping --user reds 7 argv/photos cases, and the pre-fix state model reds 13 across both files.
The fallback issued bare pm/appops commands when am get-current-user did not answer, which is the #1796 defect itself: those default to UserHandle.USER_SYSTEM, so a session running as user 10 had user 0 edited while the response reported only priorGrantState: unknown. It was also a fallback added without approval, and the docs' claim that every mutation names its user was false on that path. Resolving the acting user is now a prerequisite: setAndroidSetting permission fails with COMMAND_FAILED and a recovery hint, issuing no pm, appops or clear-permission-flags call at all. The test that locked the fallback in is replaced by one asserting the empty mutation call list for grant, deny and reset.
47f2bbe to
0f5d1cc
Compare
|
Accepted without reservation — the fallback was the defect on its own failure path, and it was a fallback I added unilaterally, which AGENTS.md forbids. Fixed in The refusalResolving the acting user is now a prerequisite of the whole permission family, not an input to it: async function requireAndroidPermissionUser(device: DeviceInfo): Promise<number> {
const userId = await readAndroidCurrentUserId(device);
if (userId !== undefined) return userId;
throw new AppError(
'COMMAND_FAILED',
'Could not determine which Android user the session runs as, so no permission was changed.',
{
deviceId: device.id,
hint: `Check adb -s ${device.id} shell am get-current-user — if the device is still booting, retry once it reports a user. agent-device refuses to change permissions it cannot scope, because pm would silently apply them to user 0.`,
},
);
}
Knock-on simplifications: The replacement testThe old test.each(['grant', 'deny', 'reset'] as const)(
'setAndroidSetting permission %s refuses to mutate when the acting user cannot be resolved',
...
await assertRejectsAppError(..., {
code: 'COMMAND_FAILED',
message: /Could not determine which Android user/,
hint: /am get-current-user/,
});
// The load-bearing assertion: the resolution attempt is the ONLY adb call. No pm, no
// appops, no clear-permission-flags — nothing that could edit user 0's state.
assert.deepEqual(calls, [['shell', 'am', 'get-current-user']]);(The Planted red — restore the reviewed-out fallback ( Restored: 20 passed. The other reverts still red as before ( Swept for any remaining unscoped path: all five DocsBoth surfaces corrected, since the old wording was false on this path. Corrected count14 files, not 8 — the body has been fixed and now enumerates them: 5 production, 8 test, 1 docs.
|
|
Re-reviewed exact head |
thymikee
added
the
ready-for-human
|
CI note on The smoke test itself was skipped, so nothing in this diff was exercised by the lane that went red. Everything test-bearing passed on the same head — CI, Android, iOS, macOS, Size, Test App Build Cache — and the Linux lane passed on both previous heads of this branch (
|
|
Follow-up on the Linux lane, now that the PR is merged — the rerun (attempt 2) was cancelled at the identical step, 30 minutes again: Two identical stalls is a pattern, so I checked whether it was specific to this branch. It is not — at the same moment, Linux runs on Root cause is in the lane itself, not in any PR: To be explicit about what this means for the merge: the Linux smoke test never executed on this change (the step was skipped both times), so it neither validated nor contradicted it. The lanes that did exercise the diff — CI, Android, iOS, macOS, Size, Test App Build Cache — all passed on |
1 participant
Summary
On Android,
settings permission deny|reset <perm>maps topm revoke, and Android kills the app process whenever a runtime permission the app currently holds is revoked. A grant → deny/reset sequence therefore silently left the session on the launcher; the next selector failed withSelector did not matchand nothing pointed at the cause (#1796, surfaced by #1793 / #1781 A1).The revoke path now reads the prior grant state (
dumpsys package <pkg>→runtime permissions: … granted=true) beforepm revokeand, when the permission was granted, says so on the response — issue option (a) + (c). No auto-relaunch: the issue's suggested surface is a hint, and relaunching implicitly would change whatsettingsdoes to the session (open --relaunchis a one-liner the agent chooses); noted here as the alternative, not taken.--json, SDK, MCP) fordeny/resetnow carriespermission(the resolved Android permission) andpriorGrantState: granted | not_granted | unknown— read for the userpm revokeacts on, from the dump'sPackages: > User <id>: > runtime permissions:nesting. Forgrantedandunknown,warnings: ["<perm> was granted before this revoke, and Android kills an app when a granted permission is revoked: if <pkg> was running it is no longer. Relaunch it with open <pkg> --relaunch before the next interaction."]. The consequence is phrased conditionally on purpose: this implements option (a), so process death is never observed, anddumpsysreportsgranted=truefor any user profile whilepm revokeacts on the current one — asserting the app was killed would be false on a multi-user grant or a not-running app. Warnings are appended as an array on the sharedwarningschannel (compose, never clobber).grantis unchanged (no state read).settingsgains the sharedmessageWithWarningsOutputformatter (message line + oneWarning:line per entry — same renderingopen/debuguse), so the hint is not--json-only. Output is byte-identical to before when there are no warnings.website/docs/docs/commands.md(next to thepm revokemapping) and thesettingsCLI help detail describe the kill +open <app> --relaunchrecovery.unknownis a first-class answer, not a synonym fornot_granted: a failed/unparseable dump or an unresolvable acting user gets the same relaunch guidance without claiming what the state was. The read adds oneam get-current-userand onedumpsys packagecall, on revokes only.Closes #1796.
Validation
Unit (
src/platforms/android/__tests__/settings.test.ts,permission-grant-state.test.ts,src/commands/capture/index.test.ts): 404 Android tests green. The load-bearing ones:deny microphone,reset camera,reset notifications, andgranteach assert the fullcallsarray, so a mutation that omits--userfails even though the response shape is unchanged.priorGrantState: granted.--user(platform default preserved) andpriorGrantState: unknown.granted/not_granted/ all threeunknowncauses; install-permission section, other users' blocks, and post-Packages:sections excluded.Planted red, each restored afterwards:
--userfrom the mutations (the pre-fix production behaviour) → 7 failures, including- '--user', - '10'in the argv diff and the call-order assertionshell pm revoke com.example.app …(no--user).granted=truescan, failures collapsing to "nothing granted") → 10 failures,+ 'not_granted' - 'unknown'.members.find-style first hit) → covered by the earlier evidence in this PR.Live, Pixel 7 CI / API 36 emulator, built CLI from this head. The defect and the fix, on the same device, with a secondary user created and the foreground user switched to it (
am switch-user 10):Single-user runs on the same device: reset while not granted →
not_granted, no warning, pid unchanged; grant then deny →granted+ warning, pid empty, launcher resumed;open --relaunchrestores the app. Secondary user removed, foreground user restored to 0, sessions closed, dev daemon stopped, emulator shut down (my own AVD on port 5562, not the shared devices).unknownis unit-covered only: a healthy device does not fail its owndumpsysoram get-current-useron demand, and faking one live would prove less than the three seeded causes do.pnpm check:affected --rungreen. Fallow flagged two intermediate drafts (the firstsetAndroidPermission, then the first parser at cyclomatic 19); both were split rather than waived.Touched files: 14 — production:
src/platforms/android/settings.ts,settings-permission.ts(new),permission-grant-state.ts(new),src/commands/capture/settings.ts,src/commands/output-common.ts; tests:android/__tests__/{settings,settings-permission,permission-grant-state}.test.ts,src/commands/capture/index.test.ts,src/__tests__/test-file-size-ratchet.test.ts,test/integration/provider-scenarios/{android-lifecycle.test.ts,android-settings-contract.ts,android-world.ts}; docs:website/docs/docs/commands.md. Skills untouched.Review follow-up (2026-08-19)
dumpsysgranted=truematches any user profile whilepm revokeacts on the current one, and the app need not have been running — this PR implements option (a) and never observes process death, so the message states the platform rule and leaves the consequence conditional. The pinned unit assertion, the docs line, and thesettingshelp detail were updated with it; the live emulator run above was re-executed against the new text on the final build (deny while granted → the quoted Warning line,pidofempty, launcher resumed; relaunch → pid 26573; session closed).parseAndroidGrantedRuntimePermissionsalso matches theinstall permissions:section of the dump. It is masked in practice —pm revokeon a non-changeable install permission throws, so no success response can carry a warning derived from that section — and narrowing the parse to theruntime permissions:block would add scanner state for a case the command cannot reach. Say so if you'd rather have it scoped anyway.Review follow-up 2 (2026-08-19) — state truth
Re-review found the prior-state read wrong in both directions: a failed or unparseable
dumpsyscollapsed towasGranted: false(asserting the app was untouched when the state was unknown), and thegranted=truescan matched theinstall permissions:section and every user's block (so another profile's grant could claim a kill that never happened). Fixed:wasGranted: boolean→priorGrantState: 'granted' | 'not_granted' | 'unknown';unknowncarries the relaunch guidance, onlynot_grantedis silent.src/platforms/android/permission-grant-state.ts, resolves the acting user witham get-current-user, and walks the dump's nesting instead of matchinggranted=anywhere.Packages:sections — proven red against the old model (10 failures,+ 'not_granted' - 'unknown').not_granted, no warning, app alive (pid 6990).Details and full evidence in this comment.
Review follow-up 3 (2026-08-19) — read and mutation addressed different users
Re-review found a product bug behind the reporting one: the state read scoped to
am get-current-user, but the mutations ran barepm grant/pm revoke/pm clear-permission-flags.PackageManagerShellCommanddefaults those toUserHandle.USER_SYSTEM, so on a device whose foreground user is nonzero the command read one user's state and edited user 0.This widens what #1796 was:
settings permission deny|resethas been editing user 0 regardless of the acting user all along. On a single-user device (the common case, and every previous run in this PR) the two coincide, which is why the original symptom looked purely like a missing hint.Fixed in
1a36c415's successor: the foreground user is resolved once and passed as--user <id>topm grant,pm revoke,pm clear-permission-flags, andappops set, and the state read uses that same id. If it cannot be resolved, the mutation keeps the platform default and the state is reportedunknownrather than guessed. Regressions pin the exact argv and call order for a nonzero foreground user, and the live before/after above was captured with the emulator's foreground user switched to 10.Review follow-up 4 (2026-08-19) — no unscoped mutation path remains
The previous revision still issued bare
pm/appopscommands whenam get-current-userdid not answer, which is the #1796 defect on the failure path: those default toUserHandle.USER_SYSTEM, so a session running as user 10 had user 0 edited while the response reported onlypriorGrantState: unknown. It was also a fallback added without approval (AGENTS.md Hard Rule), and the docs' "every permission mutation names the foreground user" was false there.Resolving the acting user is now a prerequisite:
settings permission grant|deny|resetfails withCOMMAND_FAILED("Could not determine which Android user the session runs as, so no permission was changed") plus a hint namingadb -s <serial> shell am get-current-user, and issues nopm,appops, orclear-permission-flagscall. The test that locked the fallback in is gone; its replacement asserts the empty mutation call list for all three actions.unknownnow has exactly two causes — a faileddumpsysand a dump with no runtime block for that user.