refactor(platform-android): classify pm refusals on typed reasons - #3010
Conversation
) `settings permission all` decided skip-vs-abort by matching lowercase substrings of pm stderr, and the photos path re-ran the same sniff over another error's details.attempts. Classify once at the pm boundary into an AndroidPmSkipReason instead: tryPmUnit and setAndroidPhotoPermission both call classifyAndroidPmSkip, and the photos skip check reads the typed attempts it already recorded rather than re-matching stderr. Fold the repeated applied/warnings accounting across unit kinds into recordAppliedPermission/finishAllUnit.
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/platform-android/src/settings-permission.ts">
<violation number="1" location="packages/platform-android/src/settings-permission.ts:405">
P3: `isAndroidPhotosSkipAttempts` makes the whole skip decision on `attempt.reason !== undefined`, but `isAndroidPmSkipAttemptList` validates `permission`/`detail` as strings and never checks `reason`'s type. A truthy non-string `reason` (for example `false`, or a future shape change) would pass the guard and silently collapse an operational photos probe failure into the benign skip warning instead of aborting the fan-out — the exact misclassification this typed boundary is meant to prevent. Validate `reason` in the guard: `typeof reason === 'string' || reason === undefined` (or membership-check against `AndroidPmSkipReason`).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| return ( | ||
| isAndroidPmSkipAttemptList(attempts) && | ||
| attempts.length > 0 && | ||
| attempts.every((attempt) => attempt.reason !== undefined) |
There was a problem hiding this comment.
P3: isAndroidPhotosSkipAttempts makes the whole skip decision on attempt.reason !== undefined, but isAndroidPmSkipAttemptList validates permission/detail as strings and never checks reason's type. A truthy non-string reason (for example false, or a future shape change) would pass the guard and silently collapse an operational photos probe failure into the benign skip warning instead of aborting the fan-out — the exact misclassification this typed boundary is meant to prevent. Validate reason in the guard: typeof reason === 'string' || reason === undefined (or membership-check against AndroidPmSkipReason).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/platform-android/src/settings-permission.ts, line 405:
<comment>`isAndroidPhotosSkipAttempts` makes the whole skip decision on `attempt.reason !== undefined`, but `isAndroidPmSkipAttemptList` validates `permission`/`detail` as strings and never checks `reason`'s type. A truthy non-string `reason` (for example `false`, or a future shape change) would pass the guard and silently collapse an operational photos probe failure into the benign skip warning instead of aborting the fan-out — the exact misclassification this typed boundary is meant to prevent. Validate `reason` in the guard: `typeof reason === 'string' || reason === undefined` (or membership-check against `AndroidPmSkipReason`).</comment>
<file context>
@@ -355,20 +386,36 @@ async function tryPhotosUnit(
+ return (
+ isAndroidPmSkipAttemptList(attempts) &&
+ attempts.length > 0 &&
+ attempts.every((attempt) => attempt.reason !== undefined)
+ );
+}
</file context>
|
Reviewed at f78cc55. The pm refusals are now classified on typed reasons, and every refusal class keeps its old outcome. I found no blocking problem. One question: |
thymikee
added
the
ready-for-human
|
1 participant
Summary
settings permission alldecided skip-vs-abort by lowercase-substring matchingpmstderr in two independent places, and the photos path re-ran the same sniff over another error's recordedattempts. This refactors both call sites onto a single typed boundary:classifyAndroidPmSkipreadspmstderr once and returns anAndroidPmSkipReason(not-changeable,not-requested,not-runtime-permission,unknown-permission,role-managed);tryPmUnitandsetAndroidPhotoPermissionboth call it, and the photos skip check now reads the typedattemptsit already recorded instead of re-matching stderr. The repeated applied/warnings bookkeeping across unit kinds is folded intorecordAppliedPermission/finishAllUnit. No behavior change: the same five refusal patterns are still classified, and anything else still aborts the fan-out.Touched files: 2 (
packages/platform-android/src/settings-permission.ts, its test file).Closes #2700
Validation
Tested commit:
f78cc55332216634ee165f4292fbf66e65a12ac6pnpm format: no changespnpm check:affected --run: all runnable checks passed — 364 test files, 2380 tests passed; unit and provider-integration suites deduped as covered by related tests/CILive device check (private emulator
Pixel_9_Pro_XL_API_37, serialemulator-5584, never touching the reservedemulator-5554), against the pre-installedorg.telegram.messenger.web:grant allapplied 23 permissions and produced 52 skip warnings, exercising both thenot-changeablereason (e.g.FOREGROUND_SERVICE_LOCATION ... is not a changeable permission type) and theunknown-permissionreason (e.g.Unknown permission android.permission.READ_CLIPBOARD) throughclassifyAndroidPmSkip.deny allapplied 23 and produced 75 warnings mixing skip warnings with the pre-existing "was granted before this revoke" relaunch warnings, confirmingrecordAppliedPermission/finishAllUnitstill fold both warning kinds correctly. Emulator was booted and killed for this check only;adb devicesafterward showed onlyemulator-5554remaining.No unresolved risk: refusal-reason classification and applied/warning accounting are exercised end to end on-device, matching the added unit coverage for
classifyAndroidPmSkipand the photos skip path.