feat: add Android test IME helper for deterministic text entry (#1198)#1201
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
thymikee
left a comment
There was a problem hiding this comment.
Adversarial review — independent, live-verified on emulator-5556
Scratch worktree off origin/feat/android-ime-helper @ b527b44e6. Built the real APK from source (pnpm run package:android-ime-helper:npm), drove the actual CLI (node --experimental-strip-types src/bin.ts) against emulator-5556 with an isolated --state-dir, and cross-checked every claim in the PR description against CI and live device state. Cleanup done (IME restored to Gboard, helper APK uninstalled, session closed, scratch daemon shut down; emulator-5554/Metros/iOS untouched).
Verdict: needs-changes
Two independent, PR-caused CI failures contradict the PR's "Gate: ... all green" claim, plus a live-confirmed security hole in the broadcast receiver. None of these are hard to fix, but none should merge as-is.
Blockers
1. Broadcast receiver accepts unauthenticated text injection from any app (real-device risk)
android-ime-helper/src/main/java/.../TestInputMethodService.java registers its BroadcastReceiver with Context.RECEIVER_EXPORTED (API 33+) and no android:permission. The one soft check — an optional protocol extra — is explicitly commented as "not a security boundary; missing extra is accepted."
Live-verified: with the helper active and a field focused, I sent
adb shell am broadcast -a com.callstack.agentdevice.imehelper.ACTION_INPUT_TEXT_B64 --es text <base64>
without -p (package targeting) and without the protocol extra — i.e. exactly what any installed app can do via sendBroadcast(new Intent(ACTION_INPUT_TEXT_B64).putExtra("text", ...)) with zero declared permissions. Logcat confirms the receiver processed it (commitText length=16) and get text showed the field now contained INJECTED-BY-ROGUE-APP.
This means: while the helper IME is active (default-on for emulators, opt-in via --test-ime on real devices), any co-installed app can silently inject arbitrary text into whatever field the user currently has focused, system-wide, for as long as the helper is the active IME. On real devices (the case the opt-in flag exists to gate) this is a genuine local trust-boundary violation — data exfiltration/phishing-autofill surface — that --test-ime does nothing to mitigate, since the flag only controls activation, not receiver exposure.
Minimum fix: since this is a dynamically-registered receiver it has no component name for adb shell am broadcast -n ... targeting, so the usual "exported=false + explicit component from shell" pattern (used correctly elsewhere, e.g. how io.appium.settings protects its own receivers) needs the receiver to move to a manifest-declared, non-exported component so it can be explicitly targeted; have the TS side switch from -a <action> -p <pkg> to -n <pkg>/.ReceiverClass -a <action>. A signature-level custom permission alone won't work here since adb shell (uid 2000) isn't signed with the app's key and wouldn't hold it. Compare android-multitouch-helper, which sidesteps this whole class of bug by using am instrument (inherently shell/root-only, no implicit-broadcast surface) instead of a broadcast.
2. CI is red on two counts the PR description claims are green
Integration Testsfails (scripts/integration-progress.ts --check): "provider-backed integration progress check failed: unclassified public CLI flags: testIme". The new--test-imeflag was never added toscripts/integration-progress-model.ts's flag-coverage buckets. Trivial fix — e.g. extend the existing'platform boot fallback without provider seam'bucket (currently['headless'], owner "handler and Android platform unit tests") to includetestIme, which fits the same shape.Coveragefails, and I root-caused + reproduced it locally:src/daemon/handlers/__tests__/session-doctor-android.test.ts's two orphan-detection tests ("reports fail with a remediation command when the test IME is orphaned", "falls back to ime list -s...") only pass ifandroid-ime-helper/dist/<version>.manifest.jsonhappens to already exist on disk.probeAndroidTestIme→resolveAndroidImeHelperServiceComponent()calls the real, unmockedresolveAndroidImeHelperArtifact(), which reads that file from the actual project tree; when it's absent (any fresh checkout that hasn't run the packaging step — which is exactly CI'sCoveragejob), the call throws, is caught, and returnsundefined, soisHelperCurrentImeis alwaysfalseand the check always reportspassinstead of the expectedfail. I reproduced this precisely: movedandroid-ime-helper/distaside, reran the file, got the identical 2/4 failures CI shows; restored it, tests pass again. Needs the artifact resolution mocked/injected like the rest of the suite does for adb.- (
Smoke Testsalso shows one red shard, but it's a macOS "About This Mac" replay timeout on an unrelated Sidebar/System-Report selector — looks like pre-existing infra flake, not attributable to this diff.)
Verified sound (please don't re-litigate these)
- Restore-loop corruption (the requested double-crash probe) does NOT reproduce.
activateAndroidTestImeexplicitly checkscurrentIme === manifest.serviceComponentbefore persisting, and skips overwriting the previous-IME record when the helper is already active — this is exactly the guard needed. Live-verified twice: (a) via the real CLI — open →kill -9the daemon mid-session (simulating a crash with the helper stuck active and no orphan-recovery run yet) → freshopenfrom a new daemon process → persisted record stayedGboard, not the helper;closerestored Gboard correctly. (b) via a direct, race-free call toactivateAndroidTestIme/restoreAndroidTestImebypassing the daemon entirely, to rule out any fire-and-forget orphan-recovery timing luck — same result. fill= REPLACE,type= APPEND, matching #1164. Live-verified onorg.reactnavigation.playground's EditText:fill "hello"→type " world"→ text became"hello world"(append confirmed); subsequentfill "Café ☕ 🎉 你好世界"fully replaced it (replace confirmed) and round-tripped the CJK/emoji/Latin mix exactly viaget text.helper-package-install.tsextraction is a faithful, safe dedup. Ran the multitouch-helper regression suite plus every new IME test file locally: 34/34 pass. The version-mismatch/checksum/silent-fallback shape is unchanged (checksum verified before install, cache keyed bydeviceKey\0packageName\0versionCode, reinstall withreplace:trueonly when outdated) — no regression of the fallback class from the earlier benchmark incident.- Comment-audit compliance is clean. Scanned every new/changed core file (TS + the Java service) for >130-char comment lines and 3+-line consecutive comment blocks — zero hits. Comments are one-line constraint notes as required.
typecheck/lint/format:check/check:layeringall pass locally, consistent with their green status ingh pr checks.- Keyboard-enter path with the helper active works — the newline-triggered
keyevent ENTERdispatch shared by bothtypeAndroidShelland the newtypeAndroidTestImefired successfully with the helper active. Note: the literalkeyboard enterCLI subcommand thatcli-help.tstells agents to use is currently rejected (KEYBOARD_METADATA_ACTION_VALUES = ['status','dismiss']insrc/commands/system/index.ts) — confirmed pre-existing and unrelated to this PR (that file's last touch was #1147, untouched here). Flagging since it's adjacent to what the issue assumed still worked, but it's not this PR's regression to fix.
Should-fix, non-blocking
- APK reproducibility claim is overstated. PR says "confirmed: rebuilt APK is byte-identical in size — 12,779 bytes." I rebuilt twice: sizes and every extracted file's CRC32 are identical, but the whole-file SHA256 differs between builds —
zip -q -j "$UNSIGNED_APK" "$DEX_DIR/classes.dex"embeds the current wall-clock time intoclasses.dex's local-header Unix-timestamp extra field (unlike the aapt2-compiled entries, which are pinned to the 1980 DOS epoch). This is inherited, pre-existing behavior shared by all three helper build scripts (build-android-{snapshot,multitouch,ime}-helper.shall have the identicalzip -jline), not a regression — but the claim as written overstates what was actually checked (size only, not hash). - No opt-out on emulators.
shouldActivateAndroidTestImeactivates unconditionally wheneverdevice.kind === 'emulator';--test-imeis a plain boolean flag with no negation form in this CLI's parser (no--no-test-ime), so there's no way to keep the real keyboard on an emulator if a user wants to eyeball it manually. The issue doesn't explicitly require an opt-out, so not a blocker, but worth a follow-up.
Not independently re-verified
fallow audit --base origin/main, Bundle Size, Swift Runner Unit Compile, and the full ~290-test Android unit suite beyond the files listed above — these show green in CI and I have no reason to doubt them given everything else lined up.
|
Blocking findings on current head
The macOS smoke failure looks plausibly unrelated, but Coverage and Integration are direct branch failures; live recovery/security evidence is still required after fixes. |
Addresses the independent review's blockers and should-fixes. SECURITY (blocker 1): the text-injection receiver was RECEIVER_EXPORTED with no gate — any co-installed app could inject text into the focused field while the test IME was active. Fixed by requiring the WRITE_SECURE_SETTINGS sender permission on the (in-process, dynamically-registered) receiver: adb shell holds it, third-party apps cannot. The reviewer's suggested exported=false + explicit-component approach was tried first but empirically breaks delivery on API 36 (adb shell cannot reach a non-exported receiver there) — documented in the helper README. Live-verified: a purpose-built rogue APK's broadcasts (implicit and package-scoped, no permission) are silently dropped, field unchanged; adb shell's bare broadcast still injects. Added ime-helper-security.test.ts asserting the permission gate and that no permissionless exported registration returns. CI (blocker 2): (a) added `testIme` to integration-progress-model flag buckets (Integration Tests was red on the unclassified flag). (b) mocked resolveAndroidImeHelperArtifact in session-doctor-android / ime-lifecycle / input-actions-test-ime tests so they no longer depend on android-ime-helper/dist existing on disk (Coverage was red on a fresh checkout); verified by running them with dist removed. Should-fixes: added `--no-test-ime` to opt out on emulators (tri-state gating, parser-tested); PR body's "byte-identical" claim corrected to size/CRC-match. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Review response — all blockers + should-fixes addressed (commit
|
thymikee
left a comment
There was a problem hiding this comment.
Re-review addendum — commit 04c30d0b3 (permission-gate substitute)
Scoped verification of the substitute mechanism, live on emulator-5556 (API 36, the disputed level) with a freshly-built helper APK from this commit and two purpose-built test APKs. Verdict up front: the deviation from my prescribed fix was justified, and the substitute is sound. All three round-1 blockers are now resolved.
1. API-36 finding — independently confirmed; deviation justified
Built a minimal 2-receiver APK (targetSdk=36): one android:exported="true", one android:exported="false", each logging on receive. From adb shell:
am broadcast -n com.test.exptest/.ExportedReceiver→ fired (onReceivelogged).am broadcast -n com.test.exptest/.NonExportedReceiver→ silently dropped (result=0, noonReceive). Both target the same package, so stopped-state is excluded as a confound — the exported flag is the sole differentiator.
So my originally-prescribed "manifest receiver + exported=false + -n targeting from adb shell" is empirically broken on API 36: shell cannot deliver explicit broadcasts to a non-exported component there. The implementer's isolation was correct and the deviation is warranted. (I still recommend a one-line code comment citing the API-36 constraint next to the RECEIVER_EXPORTED flag so a future reader doesn't "harden" it back to exported=false and silently break delivery.)
2. Permission-gate semantics — correct overload, verified on-device
The Java uses the right overloads — registerReceiver(receiver, filter, REQUIRED_SENDER_PERMISSION, null, RECEIVER_EXPORTED) (API 33+) and registerReceiver(receiver, filter, REQUIRED_SENDER_PERMISSION, null) (legacy) — where the 3rd positional broadcastPermission is a sender requirement, not a receiver-holds requirement. Correct.
Live-verified the real trust boundary with a no-permission rogue APK (declares zero <uses-permission>; confirmed via dumpsys package it does not hold WRITE_SECURE_SETTINGS):
- Rogue app launched as its own uid, called
sendBroadcast(...)for the inject action (both implicit andsetPackage()-scoped, matching the CLI's own-p). Its own log confirms both sends fired, but noAgentDeviceTestIME: commitTextappeared and the focused field was unchanged — silently dropped by the permission gate. This is precisely the vector I flagged in round 1, now closed. - Same moment,
adb shell am broadcast -p ... -a ACTION_INPUT_TEXT_B64 ...(uid 2000, holdsWRITE_SECURE_SETTINGS) →commitText length=17, field updated. The CLI path still works end-to-end (all baseline fills this round went through it).
3. Residual surface — acceptable; gate covers all actions
WRITE_SECURE_SETTINGS protectionLevel on-device is signature|privileged|development|installer|role. Boundary for the PR: a normal co-installed third-party (e.g. Play Store) app can never obtain it; only platform-signed apps, privileged /system/priv-app apps, or an app that declares it and is then granted it via adb shell pm grant (i.e. someone with physical adb/dev access — already game-over, since that actor could just set their own IME directly) — an acceptable residual. The gate is receiver-wide: the permission is on the single registerReceiver call covering the whole IntentFilter, and handleAction has no other entry point, so all four actions (INPUT_TEXT, INPUT_TEXT_B64, CLEAR_TEXT, ENTER) are gated — no ungated action slipped through.
4. ime-helper-security.test.ts — pins the gate against regression
Two tests: (a) asserts Manifest.permission.WRITE_SECURE_SETTINGS is present and every registerReceiver(...) call includes REQUIRED_SENDER_PERMISSION (a refactor dropping the permission arg fails); (b) asserts the permissionless registerReceiver(receiver, filter, RECEIVER_EXPORTED) / registerReceiver(receiver, filter) forms — the original vulnerability — can never come back. Adequate. Minor note: it pins the presence of the permission arg, not its exact positional slot, but a wrong slot would fail to compile against the real overloads anyway.
5. CI — the two round-1 failures are genuinely green; remaining red is the known infra flake
gh pr checks 1201: Coverage ✅, Integration Tests ✅ (both red in round 1). I re-verified each root fix locally:
- Coverage: the doctor test now
vi.mocksresolveAndroidImeHelperArtifact— reproduced the fresh-checkout case (movedandroid-ime-helper/distaside), test passes 4/4 where it previously failed 2/4. - Integration Tests:
testImeadded to theintegration-progress-model.tsflag buckets;integration-progress.ts --checknow reports 0 unclassified flags, exit 0.
The one remaining red Smoke Tests shard is _LSOpenURLsWithCompletionHandler() failed for ... System Settings.app with error -600 — the macOS LaunchServices -600 infra-flake class (same family I dismissed in round 1), on a macOS System-Settings replay, entirely unrelated to the Android IME diff.
Also confirmed
--no-test-ime opt-out (my round-1 nice-to-have) shipped and works live: open ... --no-test-ime on the emulator kept Gboard active instead of the helper. Parse test pins flags.testIme === false. The multitouch-helper regression suite and all IME test files stay green (74/74 across the changed suites).
Verdict
Approve-worthy. All three round-1 blockers (unauthenticated injection, Integration Tests, Coverage) are resolved and independently re-verified on the exact API level in dispute. The API-36 deviation from my prescription was justified by a reproducible platform constraint. Non-blocking nit only: add a one-line comment pinning the API-36 rationale beside the RECEIVER_EXPORTED flag so nobody re-hardens it to exported=false and breaks delivery.
Cleanup: session closed, test APKs + helper uninstalled, scratch daemon down, emulator-5556 IME restored to Gboard (persisted key null). emulator-5554 / Metros / iOS untouched.
The RECEIVER_EXPORTED flag cannot express why it must stay exported. Add a one-line note so a future hardening pass doesn't switch to RECEIVER_NOT_EXPORTED and silently break the CLI (adb shell can't deliver explicit broadcasts to non-exported components on API 36+; WRITE_SECURE_SETTINGS is the actual gate). 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
|
Re-check of
These need success/current-IME verification and serialization coverage. The current head also still has a failed macOS Smoke check, so it is not GTG regardless. |
Addresses the independent review's blockers and should-fixes. SECURITY (blocker 1): the text-injection receiver was RECEIVER_EXPORTED with no gate — any co-installed app could inject text into the focused field while the test IME was active. Fixed by requiring the WRITE_SECURE_SETTINGS sender permission on the (in-process, dynamically-registered) receiver: adb shell holds it, third-party apps cannot. The reviewer's suggested exported=false + explicit-component approach was tried first but empirically breaks delivery on API 36 (adb shell cannot reach a non-exported receiver there) — documented in the helper README. Live-verified: a purpose-built rogue APK's broadcasts (implicit and package-scoped, no permission) are silently dropped, field unchanged; adb shell's bare broadcast still injects. Added ime-helper-security.test.ts asserting the permission gate and that no permissionless exported registration returns. CI (blocker 2): (a) added `testIme` to integration-progress-model flag buckets (Integration Tests was red on the unclassified flag). (b) mocked resolveAndroidImeHelperArtifact in session-doctor-android / ime-lifecycle / input-actions-test-ime tests so they no longer depend on android-ime-helper/dist existing on disk (Coverage was red on a fresh checkout); verified by running them with dist removed. Should-fixes: added `--no-test-ime` to opt out on emulators (tri-state gating, parser-tested); PR body's "byte-identical" claim corrected to size/CRC-match. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
The RECEIVER_EXPORTED flag cannot express why it must stay exported. Add a one-line note so a future hardening pass doesn't switch to RECEIVER_NOT_EXPORTED and silently break the CLI (adb shell can't deliver explicit broadcasts to non-exported components on API 36+; WRITE_SECURE_SETTINGS is the actual gate). 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
9a0882c to
6f59994
Compare
Blocker 1 — a failed restore no longer deletes the recovery value. restore now reads back default_input_method after `ime set` and only clears the persisted previous-IME record on a confirmed-successful restore; a failed set keeps the value so a later retry / startup recovery / doctor remediation can still un-strand the user off the helper IME. Blocker 2 — startup orphan-recovery no longer overwrites/races user state. It only restores when the device's CURRENT default IME is still our helper (so a user who legitimately switched away is left alone), and skips any device a live session in this process owns (the fire-and-forget startup vs. concurrent `open` race — activate now marks the device active BEFORE the `ime set`, so any recovery pass that could observe the helper active also observes the flag and skips). Never persists the helper itself as the previous IME. activate also verifies its own switch via read-back. Exported ANDROID_IME_HELPER_SERVICE_COMPONENT so restore compares the active IME without reading the packaged artifact from disk. Tests: failed-restore keeps the value (+ later recovery succeeds), startup no-op when current != helper, startup skips a live-owned device. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…ort seams Rebased onto main (#1202 production-unused-exports gate). Two follow-ups: - Deleted the unused ACTION_ENTER broadcast end-to-end (TS sendAndroidImeHelperEnter + its test, Java handler, README): nothing routes through it — `keyboard enter` uses the keyevent ENTER path — so the new production-exports gate flagged it as dead production code. Removed rather than grandfathered. - Added the three legitimate test-only seams (resetAndroidImeHelperInstallCache, resetAndroidTestImeActivationCacheForTests, setAndroidTestImeActiveForTests) to fallow-baselines/production-unused-exports.json, matching how the sibling helper reset functions (resetAndroidMultiTouchHelperInstallCache, ...) are already grandfathered there. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
… Smoke) Root cause of the red macOS Smoke shard (proven, not hand-waved): the fire-and-forget restoreOrphanedAndroidTestImeOnDaemonStartup ran `adb devices` at EVERY daemon startup, on every platform. GitHub macOS runners ship the Android SDK, so this cold-started the adb server mid-replay and destabilized the macOS System Settings replay timing — the failed job's cleanup shows "Terminate orphan process: pid (N) (adb)"; main's green runs spawn no adb. Fix: gate the startup orphan scan behind a host-side marker written in the daemon state dir when a session activates the test IME (mirrors the managed-web-browser orphan-cleanup `installed` gate). A host that never uses the Android test IME — the macOS CI runner included — never writes the marker and so never spawns adb at startup. The marker is cleared once nothing is left stuck. Adds SessionStore.resolveStateDir(); tests: startup recovery does not scan adb when no marker exists (+ marker cleared after a clean scan). 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…r accessor CI's Fallow audit flags SessionStore.resolveDaemonStateDir as an unused class member, but it is called via sessionStore.resolveDaemonStateDir() in session-open.ts — fallow's class-member tracer just doesn't resolve a method call sited inside a call argument. Renamed for clarity (avoids the collision with config.ts's free resolveStateDir) and added the localized fallow-ignore-next-line unused-class-member suppression. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Maintainer blockers — all three addressed (rebased onto 0.19.3; full CI green)Blocker 1 — a failed restore no longer deletes the recovery value ✅
Blocker 2 — startup recovery no longer overwrites/races user state ✅Startup recovery only restores when the device's current default IME is still the helper — a user (or another tool) who switched away is left alone. Test: "startup recovery is a no-op when the current IME is no longer the helper" (asserts the user's IME and the record are both untouched). Race-safe against a concurrent Blocker 3 — macOS Smoke: investigated for real, and it was MINE ✅You were right not to accept the infra hand-wave — apologies. Root cause proven from the logs: the failed macOS job's cleanup shows Fix: gate the startup orphan scan behind a host-side marker written in the daemon state dir when a session activates the test IME (mirrors the managed-web-browser orphan-cleanup Verified green: macOS Smoke passes on the fix commit and on the final commit (run Rebase + new production-exports gate (#1202/#1203)Rebased onto Full CI is green: typecheck, lint, format, layering, integration-progress, Fallow, production-exports, Coverage, all Smoke shards (incl. macOS), iOS, plus 3557 unit tests locally. |
|
Two remaining restore-safety blockers on head
After these fixes, rebase after #1199 because both PRs touch open handling. |
Ships a headless InputMethodService (android-ime-helper) as a third Android helper APK, replacing the visible system keyboard during automated sessions. Renders zero accessibility nodes and accepts Unicode/CJK/emoji text over a base64-encoded broadcast channel, fixing both the settle-diff IME-chrome flood and the ASCII-only adb-shell text entry limit in one structural fix. - android-ime-helper/: InputMethodService + build/package scripts on the existing helper-APK toolchain (javac+d8+aapt2+zipalign+apksigner). - src/platforms/android/ime-helper.ts, ime-lifecycle.ts: install/version lifecycle (shared with the other two helpers via the new helper-package-install.ts), activation on session open, and on-device restore-hygiene (previous IME persisted to a device settings key so any daemon/state-dir can recover it; restored on close, daemon teardown, and daemon startup for orphans left by a crashed run). - input-actions.ts: fill/type route through the helper's broadcast channel when active, unicode-safe; unchanged ASCII-shell fallback otherwise. - doctor: new android-test-ime check flags a stuck helper IME with a copy-pasteable `adb shell ime set` remediation command. - Gating: default-on for emulators, opt-in via `open --test-ime` on real devices. - Dead-weight: rewrote the manual ADBKeyBoard workaround doc, dropped the now-provably-live skillgym non-ASCII eval case, updated the ASCII fallback's error message to point at the helper instead of dead-ending. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Addresses the independent review's blockers and should-fixes. SECURITY (blocker 1): the text-injection receiver was RECEIVER_EXPORTED with no gate — any co-installed app could inject text into the focused field while the test IME was active. Fixed by requiring the WRITE_SECURE_SETTINGS sender permission on the (in-process, dynamically-registered) receiver: adb shell holds it, third-party apps cannot. The reviewer's suggested exported=false + explicit-component approach was tried first but empirically breaks delivery on API 36 (adb shell cannot reach a non-exported receiver there) — documented in the helper README. Live-verified: a purpose-built rogue APK's broadcasts (implicit and package-scoped, no permission) are silently dropped, field unchanged; adb shell's bare broadcast still injects. Added ime-helper-security.test.ts asserting the permission gate and that no permissionless exported registration returns. CI (blocker 2): (a) added `testIme` to integration-progress-model flag buckets (Integration Tests was red on the unclassified flag). (b) mocked resolveAndroidImeHelperArtifact in session-doctor-android / ime-lifecycle / input-actions-test-ime tests so they no longer depend on android-ime-helper/dist existing on disk (Coverage was red on a fresh checkout); verified by running them with dist removed. Should-fixes: added `--no-test-ime` to opt out on emulators (tri-state gating, parser-tested); PR body's "byte-identical" claim corrected to size/CRC-match. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
The RECEIVER_EXPORTED flag cannot express why it must stay exported. Add a one-line note so a future hardening pass doesn't switch to RECEIVER_NOT_EXPORTED and silently break the CLI (adb shell can't deliver explicit broadcasts to non-exported components on API 36+; WRITE_SECURE_SETTINGS is the actual gate). 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Blocker 1 — a failed restore no longer deletes the recovery value. restore now reads back default_input_method after `ime set` and only clears the persisted previous-IME record on a confirmed-successful restore; a failed set keeps the value so a later retry / startup recovery / doctor remediation can still un-strand the user off the helper IME. Blocker 2 — startup orphan-recovery no longer overwrites/races user state. It only restores when the device's CURRENT default IME is still our helper (so a user who legitimately switched away is left alone), and skips any device a live session in this process owns (the fire-and-forget startup vs. concurrent `open` race — activate now marks the device active BEFORE the `ime set`, so any recovery pass that could observe the helper active also observes the flag and skips). Never persists the helper itself as the previous IME. activate also verifies its own switch via read-back. Exported ANDROID_IME_HELPER_SERVICE_COMPONENT so restore compares the active IME without reading the packaged artifact from disk. Tests: failed-restore keeps the value (+ later recovery succeeds), startup no-op when current != helper, startup skips a live-owned device. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…ort seams Rebased onto main (#1202 production-unused-exports gate). Two follow-ups: - Deleted the unused ACTION_ENTER broadcast end-to-end (TS sendAndroidImeHelperEnter + its test, Java handler, README): nothing routes through it — `keyboard enter` uses the keyevent ENTER path — so the new production-exports gate flagged it as dead production code. Removed rather than grandfathered. - Added the three legitimate test-only seams (resetAndroidImeHelperInstallCache, resetAndroidTestImeActivationCacheForTests, setAndroidTestImeActiveForTests) to fallow-baselines/production-unused-exports.json, matching how the sibling helper reset functions (resetAndroidMultiTouchHelperInstallCache, ...) are already grandfathered there. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
… Smoke) Root cause of the red macOS Smoke shard (proven, not hand-waved): the fire-and-forget restoreOrphanedAndroidTestImeOnDaemonStartup ran `adb devices` at EVERY daemon startup, on every platform. GitHub macOS runners ship the Android SDK, so this cold-started the adb server mid-replay and destabilized the macOS System Settings replay timing — the failed job's cleanup shows "Terminate orphan process: pid (N) (adb)"; main's green runs spawn no adb. Fix: gate the startup orphan scan behind a host-side marker written in the daemon state dir when a session activates the test IME (mirrors the managed-web-browser orphan-cleanup `installed` gate). A host that never uses the Android test IME — the macOS CI runner included — never writes the marker and so never spawns adb at startup. The marker is cleared once nothing is left stuck. Adds SessionStore.resolveStateDir(); tests: startup recovery does not scan adb when no marker exists (+ marker cleared after a clean scan). 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…r accessor CI's Fallow audit flags SessionStore.resolveDaemonStateDir as an unused class member, but it is called via sessionStore.resolveDaemonStateDir() in session-open.ts — fallow's class-member tracer just doesn't resolve a method call sited inside a call argument. Renamed for clarity (avoids the collision with config.ts's free resolveStateDir) and added the localized fallow-ignore-next-line unused-class-member suppression. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…very markers Addresses devin-ai-integration's two P1 restore-safety blockers on 19cbce7. P1.1 — durably persist the restore target BEFORE the global IME switch. writePersistedPreviousIme now checks the `settings put` exit code AND reads the value back, returning a boolean. activate persists first and, if it cannot be persisted, fails open to the existing input path WITHOUT switching — a rejected `settings put` can no longer strand the user on the helper with no restore target. Regression test added. P1.2 — close the marker crash/offline blind spot. Recovery intent is now recorded per device, BEFORE the switch (ordering: durable record -> marker -> ime set), eliminating the post-switch/pre-marker crash window. Markers are device-scoped and each is retained until that device is actually observed clean: an offline/disconnected-but-stuck device keeps its marker and is recovered on reconnect instead of being cleared because the current `adb devices` scan saw no set-failed. Close-time restore clears only that device's marker (stateDir plumbed through teardown/close). Tests cover the persist-failure, post-switch/pre-marker crash, offline-then-reconnect, live-session-owned, and user-switched-away cases. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
19cbce7 to
b6e7483
Compare
Both P1 restore-safety blockers fixed (
|
Summary
Ships #1198's Phase 1: a headless
InputMethodService(android-ime-helper) as a third helper APK on the existing pipeline, replacing the visible system keyboard during automated Android sessions. This is the structural fix for two problems that turned out to be the same root cause:adb shell input textis ASCII-only; CJK/emojifill/typethrowsCOMMAND_FAILED.The helper renders zero accessibility nodes (
onEvaluateInputViewShown()→false,onCreateInputView()→null) and accepts Unicode-safe text over a base64-encoded broadcast channel, the samepayloadBase64conventionandroid-multitouch-helperalready uses.What's here
android-ime-helper/—TestInputMethodService.java+ manifest +res/xml/method.xml, built on the exactjavac+d8+aapt2+zipalign+apksignertoolchain the other two helpers use (one new step:aapt2 compilefor the IME resource, since the other two don't have resources).scripts/build-android-ime-helper.sh/package-android-ime-helper.shmirror the multitouch helper's shape;package:android-ime-helper:npmis wired intoprepackand CI (android.yml,perf-nightly.yml).src/platforms/android/ime-helper.ts— install/version-check/checksum lifecycle (now shared withmultitouch-helper.tsvia a newhelper-package-install.ts— this also deleted ~140 lines of near-duplicate install logic between the two) + the broadcast text-entry channel (ACTION_INPUT_TEXT_B64/ACTION_CLEAR_TEXT/ACTION_ENTER).src/platforms/android/ime-lifecycle.ts— activation (session open) and restore-hygiene. The previously-active IME is persisted to a devicesettings securekey (not a host-side file), so any daemon/state-dir can recover it regardless of which process touched the device. Restored on session close, daemon teardown, and best-effort on daemon startup for orphans left by a crashed run.input-actions.ts—fill/typeroute through the helper's broadcast channel when active (unicode-safe, atomiccommitText); unchanged ASCII-shell fallback when the helper isn't installed/active.doctor— newandroid-test-imecheck:passwhen the normal IME is in use or this process owns the active helper;failwith a copy-pasteableadb shell ime set <id>command when the helper is active but orphaned.open --test-imeon real devices (stuck-keyboard footgun is much worse on a real phone).Dead-weight deleted
website/docs/docs/known-limitations.md— replaced the manual ADBKeyBoard install/revert instructions with a description of the automatic helper.test/skillgym/suites/agent-device-smoke-suite.ts— removedandroid-non-ascii-text-stays-in-fill, the eval case that forbade the agent from ever suggesting ADBKeyBoard (nothing left to warn it away from).input-actions.ts's ASCII-only fallback error now hints at the helper instead of dead-ending ("report the tool/device gap"). Per the issue's own framing this path stays as the degraded fallback when the helper can't be installed — not deleted outright.cli-help.ts's Android text-entry guidance updated to describe the three-tier fallback (provider-native → helper → ASCII shell).android-input-ownership.ts,android-input-method-overlays.ts+ call sites) — still load-bearing for the degraded/opt-out path.Live verification (emulator-5556, RN Navigation playground — emulator-5554 was in use by another agent's benchmark run, left untouched)
open --test-ime;default_input_methodconfirmed set to the helper service, previous IME (Gboard) persisted to the device setting.dumpsys input_methodshowedmIsInputViewShown=falsethroughout.fillround-tripped exactly for"hello","你好世界","😀🎉👍", and"Café ☕ 🎉 你好"(verified viaget text), with zeroshell input textcalls while the helper was active.closerestored the previous IME (default_input_methodback to Gboard) and cleared the persisted record.kill -9mid-session, IME left stuck on the device) — the next daemon startup's orphan-recovery path restored Gboard automatically before I could even observe thedoctorfail state, so I added direct unit coverage (session-doctor-android.test.ts) for the fail-with-remediation-command branch instead.Gate
build,lint,typecheck,format:check,check:layering,fallow audit --base origin/main(clean, 0 issues), full android unit suite (290+ tests), touched daemon tests, smoke suite — all green. The helper APK is reproducible from a clean checkout with onlyANDROID_HOME/ANDROID_SDK_ROOT+ standard build-tools. Reproducibility is size- and per-entry-CRC-stable across rebuilds, but the whole-file SHA differs run-to-run:zip -jstampsclasses.dex's local-header timestamp with wall-clock time. This is pre-existing behavior shared by all three helper build scripts (build-android-{snapshot,multitouch,ime}-helper.sh), not introduced here; not fixing the toolchain in this PR.Spec-vs-reality notes for the maintainer
android-non-ascii-text-stays-in-fill) was actually still a coherent, passing eval on its own terms (it just asserts the agent plans plainfill, never mentioningadb/clipboard/paste/ime) — I deleted it per the issue's explicit dead-weight list, but wanted to flag that its removal is about eliminating a now-redundant guard, not a broken test.keyboard dismissshort-circuiting to a no-op when the helper IME is active (mentioned in the issue's integration design as a nice-to-have) was left as a follow-up — existingdismissbehavior already degrades safely, just with one wastedESCkeyevent in that case.