diff --git a/.changeset/keyboard-occlusion-guard-phase1.md b/.changeset/keyboard-occlusion-guard-phase1.md new file mode 100644 index 00000000..e7c01ed8 --- /dev/null +++ b/.changeset/keyboard-occlusion-guard-phase1.md @@ -0,0 +1,6 @@ +--- +"rn-dev-agent-cdp": patch +"rn-dev-agent-plugin": patch +--- + +fix(actions): inject `- hideKeyboard` before button taps that follow text entry when generating/saving Maestro action flows, and route Android hideKeyboard replays to the official Maestro CLI (#356, Phase 1). Bottom-pinned taps (submit/continue) previously landed on the soft keyboard during replays — the single biggest source of flaky replays. `generateMaestro` now tracks soft-keyboard state and emits a `hideKeyboard` step before a `tap`/`long_press` that follows an `inputText`, reset on navigation. `hideKeyboard` is a no-op when no keyboard is showing and Maestro re-resolves the selector after dismiss, so the injection is safe. Device verification surfaced that maestro-runner v1.0.9 silently no-ops `hideKeyboard` on Android (B223), so `maestro_run` now prefers the official Maestro CLI for Android flows containing `hideKeyboard` (verified to dismiss the keyboard on-device), warning when the CLI is unavailable; iOS is unaffected (maestro-runner honors hideKeyboard there). Live `device_*` taps (the in-runner guard) and existing-corpus backfill are deferred to later phases. diff --git a/docs/superpowers/plans/2026-06-20-356-keyboard-occlusion-guard-phase1.md b/docs/superpowers/plans/2026-06-20-356-keyboard-occlusion-guard-phase1.md new file mode 100644 index 00000000..f453107e --- /dev/null +++ b/docs/superpowers/plans/2026-06-20-356-keyboard-occlusion-guard-phase1.md @@ -0,0 +1,387 @@ +# Keyboard-occlusion guard — Phase 1 (Maestro `hideKeyboard` injection) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make `generateMaestro()` emit `- hideKeyboard` before any button tap that follows text entry, so generated/saved Maestro action flows stop having bottom-pinned taps land on the soft keyboard. + +**Architecture:** A pure, order-only transform *inside* `generateMaestro()` — the single chokepoint every generated/saved Maestro flow passes through. While walking the `RecordedEvent[]` stream we track one boolean (`keyboardLikelyUp`); a `type` event raises it, and before emitting a `tap`/`long_press` we inject a `hideKeyboard` step (and an audit comment) when it is up, then lower it. `hideKeyboard` is already allowlisted and is a no-op when no keyboard is showing; Maestro taps are selector-based so the next selector re-resolves after dismiss (zero stale-coordinate risk). + +**Tech Stack:** TypeScript (compiled `src` → `dist` via `tsc`), Node's built-in test runner (`node --test`), `assert/strict`. Tests are authored as `.js` in `test/unit/` and import from the built `dist/`. + +## Global Constraints + +- Node.js >= 22 LTS. +- No new dependencies; `hideKeyboard` is already in `maestro-validator.ts` `ALLOWED_COMMANDS`. +- Use explicit type imports (`import type { ... }`). +- No unnecessary comments in code. +- The only function changed in `src` is `generateMaestro`. Do NOT touch `RecordedEvent`, the recorder, `save-as-action`, `maestro-validator`, `repair-engine`, `runNative`, or the native runners (those are Phase 1.5 / Phase 2). +- The audit marker is emitted as a **preceding full comment line** (matching the generator's existing `# navigated:` / `# NOTE:` style), never a trailing inline comment. +- A changeset is REQUIRED (touches `scripts/cdp-bridge/src/`) and MUST bump **both** `rn-dev-agent-cdp` and `rn-dev-agent-plugin` (CI guard `scripts/require-changeset.sh`, #364). +- Marker comment text (verbatim, used by every task): `# rn-dev-agent: keyboard-occlusion guard (#356)` +- Build+test commands (verbatim): + - Targeted: `cd scripts/cdp-bridge && npm run build && node --test test/unit/test-recorder-generators.test.js` + - Full suite: `cd scripts/cdp-bridge && npm test` + +--- + +## File Structure + +- **Modify:** `scripts/cdp-bridge/src/tools/test-recorder-generators.ts` — only `generateMaestro` (currently lines 165–264). Add the `keyboardLikelyUp` state, the pre-tap injection in the `tap` and `long_press` cases, the flag-raise in the `type` case, and the flag-reset in the `navigate` case. +- **Modify (tests):** `scripts/cdp-bridge/test/unit/test-recorder-generators.test.js` — append `#356` test cases (existing tests must keep passing). +- **Create (changeset):** `.changeset/keyboard-occlusion-guard-phase1.md`. + +--- + +### Task 1: Core injection — `hideKeyboard` before a tap that follows text entry + +**Files:** +- Modify: `scripts/cdp-bridge/src/tools/test-recorder-generators.ts` (`generateMaestro`, ~165–264) +- Test: `scripts/cdp-bridge/test/unit/test-recorder-generators.test.js` + +**Interfaces:** +- Consumes: existing `generateMaestro(events: RecordedEvent[], opts?: GenerateOpts): string`; event shape `{ type: 'tap'|'type'|'long_press'|'navigate'|'submit'|'swipe'|'annotation', testID?, label?, value?, t, ... }`. +- Produces: same signature, unchanged. New emitted lines `# rn-dev-agent: keyboard-occlusion guard (#356)` + `- hideKeyboard` inserted before a `tap`'s `- tapOn:` when the keyboard is up. + +- [ ] **Step 1: Write the failing tests** + +Append to `scripts/cdp-bridge/test/unit/test-recorder-generators.test.js`: + +```js +test('#356 Maestro: hideKeyboard injected before a tap that follows type', () => { + const out = generateMaestro([ + { type: 'type', testID: 'email', value: 'a@b.c', t: 1 }, + { type: 'tap', testID: 'submit', t: 2 }, + ]); + assert.match(out, /# rn-dev-agent: keyboard-occlusion guard \(#356\)/); + const hk = out.indexOf('- hideKeyboard'); + const input = out.indexOf('- inputText:'); + const submitTap = out.indexOf('id: submit'); + assert.ok(hk > -1, 'hideKeyboard should be injected'); + assert.ok(hk > input, 'hideKeyboard comes after the inputText'); + assert.ok(hk < submitTap, 'hideKeyboard comes before the submit tap'); + const emailTap = out.indexOf('id: email'); + assert.ok( + !out.slice(0, emailTap).includes('- hideKeyboard'), + 'the focusing tap of the type step is NOT guarded', + ); +}); + +test('#356 Maestro: no hideKeyboard when a tap is not preceded by type', () => { + const out = generateMaestro([{ type: 'tap', testID: 'submit', t: 1 }]); + assert.ok(!out.includes('- hideKeyboard'), 'no keyboard, no injection'); +}); + +test('#356 Maestro: single hideKeyboard for type then two taps', () => { + const out = generateMaestro([ + { type: 'type', testID: 'email', value: 'a@b.c', t: 1 }, + { type: 'tap', testID: 'next', t: 2 }, + { type: 'tap', testID: 'submit', t: 3 }, + ]); + const count = (out.match(/- hideKeyboard/g) || []).length; + assert.equal(count, 1, 'flag cleared after first guarded tap'); + assert.ok(out.indexOf('- hideKeyboard') < out.indexOf('id: next')); +}); +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cd scripts/cdp-bridge && npm run build && node --test test/unit/test-recorder-generators.test.js` +Expected: the three `#356` tests FAIL (no `- hideKeyboard` in output); all pre-existing `M6` tests PASS. + +- [ ] **Step 3: Implement the minimal change in `generateMaestro`** + +In `scripts/cdp-bridge/src/tools/test-recorder-generators.ts`, inside `generateMaestro`, immediately after `const consumedNavIndices = new Set();` add the state: + +```ts + // #356: track whether the soft keyboard is likely up so we can dismiss it + // before a button tap (a bottom-pinned tap otherwise lands on the keyboard). + let keyboardLikelyUp = false; +``` + +Replace the `case 'tap':` block body with: + +```ts + case 'tap': { + const sel = maestroSelector(ev); + if (sel) { + if (keyboardLikelyUp) { + lines.push('# rn-dev-agent: keyboard-occlusion guard (#356)'); + lines.push('- hideKeyboard'); + keyboardLikelyUp = false; + } + lines.push(`- tapOn:\n ${sel}`); + } else lines.push('# tap: missing testID/label'); + const hit = lookaheadNavigate(events, i); + if (hit) { + lines.push( + `# navigated: ${stripNewlines(hit.event.from ?? '?')} -> ${stripNewlines(hit.event.to)}`, + ); + const next = nextSelector(events, hit.index, maestroSelector); + if (next) lines.push(`- assertVisible:\n ${next}`); + consumedNavIndices.add(hit.index); + } + break; + } +``` + +In the `case 'type':` block, set the flag when an input is actually emitted: + +```ts + case 'type': { + const sel = maestroSelector(ev); + if (sel) { + lines.push(`- tapOn:\n ${sel}`); + lines.push(`- inputText: ${JSON.stringify(ev.value)}`); + keyboardLikelyUp = true; + } else { + lines.push(`# type: missing testID/label, value=${JSON.stringify(ev.value)}`); + } + break; + } +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `cd scripts/cdp-bridge && npm run build && node --test test/unit/test-recorder-generators.test.js` +Expected: all `#356` tests from this task PASS; all `M6` tests still PASS. + +- [ ] **Step 5: Commit** + +```bash +git add scripts/cdp-bridge/src/tools/test-recorder-generators.ts scripts/cdp-bridge/test/unit/test-recorder-generators.test.js +git commit -m "feat(356): inject hideKeyboard before taps following text entry (Maestro generator) + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +### Task 2: Reset conditions + `long_press` parity + +**Files:** +- Modify: `scripts/cdp-bridge/src/tools/test-recorder-generators.ts` (`generateMaestro`) +- Test: `scripts/cdp-bridge/test/unit/test-recorder-generators.test.js` + +**Interfaces:** +- Consumes: the `keyboardLikelyUp` state introduced in Task 1. +- Produces: `navigate` clears the flag; `long_press` is guarded the same way as `tap`; `submit` leaves the flag unchanged. + +- [ ] **Step 1: Write the failing tests** + +Append: + +```js +test('#356 Maestro: navigate resets keyboard state (no injection after nav)', () => { + const out = generateMaestro([ + { type: 'type', testID: 'email', value: 'a@b.c', t: 1 }, + { type: 'navigate', from: 'A', to: 'B', t: 2 }, + { type: 'tap', testID: 'submit', t: 3 }, + ]); + assert.ok(!out.includes('- hideKeyboard'), 'navigation dismisses the keyboard'); +}); + +test('#356 Maestro: consecutive types inject once before the final tap', () => { + const out = generateMaestro([ + { type: 'type', testID: 'first', value: 'x', t: 1 }, + { type: 'type', testID: 'second', value: 'y', t: 2 }, + { type: 'tap', testID: 'submit', t: 3 }, + ]); + const count = (out.match(/- hideKeyboard/g) || []).length; + assert.equal(count, 1, 'one injection for the trailing button tap'); + const secondTap = out.indexOf('id: second'); + assert.ok( + !out.slice(0, secondTap).includes('- hideKeyboard'), + 'the focusing tap of the second type is not guarded', + ); + assert.ok(out.indexOf('- hideKeyboard') < out.indexOf('id: submit')); +}); + +test('#356 Maestro: submit (Enter) does not reset keyboard state', () => { + const out = generateMaestro([ + { type: 'type', testID: 'email', value: 'a@b.c', t: 1 }, + { type: 'submit', t: 2 }, + { type: 'tap', testID: 'submit', t: 3 }, + ]); + const hk = out.indexOf('- hideKeyboard'); + assert.ok(hk > -1, 'still guarded after submit'); + assert.ok(hk < out.indexOf('id: submit')); +}); + +test('#356 Maestro: long_press following type is guarded', () => { + const out = generateMaestro([ + { type: 'type', testID: 'email', value: 'a@b.c', t: 1 }, + { type: 'long_press', testID: 'avatar', t: 2 }, + ]); + const hk = out.indexOf('- hideKeyboard'); + assert.ok(hk > -1, 'long_press is guarded too'); + assert.ok(hk < out.indexOf('- longPressOn:')); +}); +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cd scripts/cdp-bridge && npm run build && node --test test/unit/test-recorder-generators.test.js` +Expected: the `navigate` and `long_press` tests FAIL; `consecutive types` and `submit` tests PASS already (Task 1 behavior). All prior tests still PASS. + +- [ ] **Step 3: Implement the reset + long_press injection** + +In `generateMaestro`, replace the `case 'long_press':` block body with: + +```ts + case 'long_press': { + const sel = maestroSelector(ev); + if (sel) { + if (keyboardLikelyUp) { + lines.push('# rn-dev-agent: keyboard-occlusion guard (#356)'); + lines.push('- hideKeyboard'); + keyboardLikelyUp = false; + } + lines.push(`- longPressOn:\n ${sel}`); + } else lines.push('# long_press: missing testID/label'); + const hit = lookaheadNavigate(events, i); + if (hit) { + lines.push( + `# navigated: ${stripNewlines(hit.event.from ?? '?')} -> ${stripNewlines(hit.event.to)}`, + ); + const next = nextSelector(events, hit.index, maestroSelector); + if (next) lines.push(`- assertVisible:\n ${next}`); + consumedNavIndices.add(hit.index); + } + break; + } +``` + +In the `case 'navigate':` block, reset the flag at the top of the case (before the `consumedNavIndices` early-return): + +```ts + case 'navigate': { + keyboardLikelyUp = false; + if (consumedNavIndices.has(i)) break; + const next = nextSelector(events, i, maestroSelector); + lines.push(`# navigated: ${stripNewlines(ev.from ?? '?')} -> ${stripNewlines(ev.to)}`); + if (next) lines.push(`- assertVisible:\n ${next}`); + break; + } +``` + +(The `submit` case is intentionally left unchanged — Enter's dismiss behavior is field-type dependent, so we keep the flag set and over-guard the next tap.) + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `cd scripts/cdp-bridge && npm run build && node --test test/unit/test-recorder-generators.test.js` +Expected: ALL tests in the file PASS (`#356` + `M6`). + +- [ ] **Step 5: Commit** + +```bash +git add scripts/cdp-bridge/src/tools/test-recorder-generators.ts scripts/cdp-bridge/test/unit/test-recorder-generators.test.js +git commit -m "feat(356): reset keyboard guard on navigate; guard long_press too + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +### Task 3: Full-suite regression + changeset + +**Files:** +- Create: `.changeset/keyboard-occlusion-guard-phase1.md` + +**Interfaces:** +- Consumes: nothing. +- Produces: a changeset bumping `rn-dev-agent-cdp` + `rn-dev-agent-plugin` (patch). + +- [ ] **Step 1: Run the full unit suite (regression gate)** + +Run: `cd scripts/cdp-bridge && npm test` +Expected: the entire suite PASSES. In particular, generator tests for flows with no typed-then-tapped sequence are byte-identical to before (no stray `- hideKeyboard`). If any pre-existing test now emits an unexpected `- hideKeyboard`, that flow legitimately had a type→tap sequence; confirm the injection is correct and update the expectation only if it is. + +- [ ] **Step 2: Create the changeset** + +Create `.changeset/keyboard-occlusion-guard-phase1.md`: + +```markdown +--- +"rn-dev-agent-cdp": patch +"rn-dev-agent-plugin": patch +--- + +fix(actions): inject `- hideKeyboard` before button taps that follow text entry when generating/saving Maestro action flows (#356, Phase 1). Bottom-pinned taps (submit/continue) previously landed on the soft keyboard during replays — the single biggest source of flaky replays. `generateMaestro` now tracks soft-keyboard state and emits a `hideKeyboard` step before a `tap`/`long_press` that follows an `inputText`, reset on navigation. `hideKeyboard` is a no-op when no keyboard is showing and Maestro re-resolves the selector after dismiss, so the injection is safe. Live `device_*` taps (the in-runner guard) and existing-corpus backfill are deferred to later phases. +``` + +- [ ] **Step 3: Verify the changeset guard passes locally** + +Run: `cd /Users/anton_personal/GitHub/claude-react-native-dev-plugin && CHANGED_FILES="scripts/cdp-bridge/src/tools/test-recorder-generators.ts" bash scripts/require-changeset.sh` +Expected: prints a success line (a `rn-dev-agent-plugin` changeset is present) and exits 0. + +- [ ] **Step 4: Commit** + +```bash +git add .changeset/keyboard-occlusion-guard-phase1.md +git commit -m "chore(changeset): keyboard-occlusion guard Phase 1 (cdp + plugin patch) + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +### Task 4: Device verification on iOS + Android (parent session) + +> Runs in the **parent session** using the plugin's live MCP tools (`cdp_*`, `device_*`, `maestro_run`, recorder/action tools) against a booted simulator/emulator with Metro running from `../rn-dev-agent-workspace/test-app`. This task is verification, not code — do not dispatch it to a code subagent. + +**Interfaces:** +- Consumes: the rebuilt `dist/` from Tasks 1–2. + +- [ ] **Step 1: Confirm the running bridge has the rebuilt generator** + +Run `cdp_status` and confirm the worker is healthy. Ensure `scripts/cdp-bridge/dist/tools/test-recorder-generators.js` reflects the change (rebuilt in Task 1/2). + +- [ ] **Step 2: Produce a flow that exercises the bug (iOS)** + +Drive a screen with a text field above the keyboard and a bottom-pinned submit button. Record a walk (fill the field, then tap submit) and save it as an action (or run `generateMaestro` over the recorded events). Confirm the saved/generated YAML contains `# rn-dev-agent: keyboard-occlusion guard (#356)` + `- hideKeyboard` before the submit `- tapOn`. + +- [ ] **Step 3: Replay and verify (iOS)** + +Replay the action via `cdp_run_action`/`maestro_run`. Expected: the submit tap reaches the next screen reliably (assert the post-submit route/element with `expect_route` or `assertVisible`). Capture a `device_screenshot` of the resulting screen as proof. + +- [ ] **Step 4: Repeat on Android** + +Boot the Android emulator, repeat Steps 2–3. Expected: same reliable result. (Maestro/UIAutomator interprets `hideKeyboard` identically; this confirms cross-platform parity.) + +- [ ] **Step 5: Record the outcome** + +Note the before/after (flaky → reliable) in the PR body and in the workspace `ROADMAP.md`/`BUGS.md` per the project logging rules. Save screenshots under the workspace proof dir. + +--- + +## Self-Review + +**1. Spec coverage:** +- Approach (transform in `generateMaestro`) → Task 1 + 2. ✓ +- Detection rule (type raises; tap/long_press inject+clear; navigate resets; type-focus-tap unguarded; submit no-reset) → Task 1 (raise, tap inject, type-focus unguarded) + Task 2 (navigate reset, long_press, submit no-reset). ✓ +- Emitted shape with preceding comment marker → Task 1/2 impl + Task 1 test asserts the marker. ✓ +- Testing: unit table of cases → Tasks 1–2; device verification iOS+Android → Task 4. ✓ +- Out-of-scope items (Phase 2, repair-time, backfill, Detox, swipe/fill) → untouched; Global Constraints forbids touching those files. ✓ +- Changeset bumping both packages → Task 3. ✓ + +**2. Placeholder scan:** No TBD/TODO; every code step shows full code; every command shows expected output. ✓ + +**3. Type consistency:** Marker string `# rn-dev-agent: keyboard-occlusion guard (#356)` and flag name `keyboardLikelyUp` are identical across Tasks 1 and 2. The `tap` and `long_press` injection blocks are identical in structure. Changeset package names `rn-dev-agent-cdp` / `rn-dev-agent-plugin` match `require-changeset.sh`. ✓ + +No gaps found. + +## Multi-LLM plan review (Codex + Gemini) — clean, no amendments + +Reviewed before execution per the project workflow. Both reviewers validated the +detection-rule state machine against the real `generateMaestro` control flow and the +edge cases (swipe/annotation between type and tap; empty `value`; label-only tap; +missing-testID type; `lookaheadNavigate`-consumed navigate; repeated taps), confirmed +the injected `# comment` + `- hideKeyboard` parses clean and passes +`maestro-validator` (`hideKeyboard` ∈ `ALLOWED_COMMANDS`), confirmed the test +assertions are empirically valid (Gemini simulated all cases: 13/13 pass; YAML parsed +with 0 errors), and confirmed the dual-package changeset satisfies +`require-changeset.sh`. Key integration point confirmed safe: resetting +`keyboardLikelyUp` at the top of the `navigate` case does not interfere with +`consumedNavIndices` (the loop visits every index; consumption only suppresses +emission). No BLOCKING/IMPORTANT findings; advisories were non-actionable. Execute as +written. diff --git a/docs/superpowers/specs/2026-06-20-356-keyboard-occlusion-guard-phase1-design.md b/docs/superpowers/specs/2026-06-20-356-keyboard-occlusion-guard-phase1-design.md new file mode 100644 index 00000000..56f0016f --- /dev/null +++ b/docs/superpowers/specs/2026-06-20-356-keyboard-occlusion-guard-phase1-design.md @@ -0,0 +1,170 @@ +# #356 — Keyboard-occlusion guard, Phase 1: Maestro `hideKeyboard` injection + +Date: 2026-06-20 +Issue: [#356](https://github.com/Lykhoyda/rn-dev-agent/issues/356) (extracted from #300, item 2) +Status: Design approved — ready for implementation plan + +## Problem + +When a tap targets a **bottom-pinned control** (a `submit`/`continue` button) while +the soft keyboard is up, the tap lands on the keyboard instead of the button, so the +next screen is never reached. Per the #300 session this was **the single biggest +source of flaky replays**, on **both iOS and Android**. The reporter's proven manual +workaround was to add `- hideKeyboard` before each bottom-pinned tap in the flow. + +## Why two phases (and what this phase is NOT) + +The bug spans two independent device-control engines that do **not** share a tap path: + +| Surface | Execution path | Phase | +|---|---|---| +| Learned-action **replay** (`cdp_run_action` → `maestro_run`) | external **maestro-runner** Go binary (WDA / UIAutomator) — **never** calls `runNative()` | **Phase 1 (this spec)** | +| Live `device_press` / `device_batch` / `cdp_interact` native fallback | `runNative()` → in-tree `rn-fast-runner` / `rn-android-runner` | Phase 2 (separate spec/PR) | + +Evidence: `cdp_run_action` delegates to `maestroRun()` (`run-action.ts:278`, retry at +`:551`); `maestro_run` shells out via `execFile(dispatch.binPath, …)` +(`maestro-run.ts:235`). A guard placed in `runNative()` is therefore a **no-op for +action replays**. Because the issue's headline is flaky *replays*, Phase 1 fixes the +**Maestro/L3** path; the in-runner frame-precise guard for live taps is **Phase 2** +(decisions already taken for it: in-runner atomic, frame-precise on both platforms via +iOS `app.keyboards.frame` and Android `UiAutomation.getWindows()` → `TYPE_INPUT_METHOD` +bounds — recorded here so Phase 2 starts from them). + +**This phase does NOT** touch `runNative`, the native runners, repair-time injection, +or existing on-disk action files. See "Out of scope". + +## Approach + +A **pure event-stream transform inside `generateMaestro()`** +(`scripts/cdp-bridge/src/tools/test-recorder-generators.ts`) — the single chokepoint +every generated/saved Maestro flow passes through (`cdp_record_test_generate` and +`save-as-action` both call it). When the generator is about to emit a button tap that +follows text entry, it first emits `- hideKeyboard`. + +Why this layer is correct and safe: + +1. `hideKeyboard` is already in the validator allowlist (`maestro-validator.ts:105`) + and is a **no-op when no keyboard is showing** — so over-injection costs only a few + hundred ms, never correctness. +2. Maestro taps are **selector-based** (`id:` / `text:`), so after the dismiss Maestro + **re-resolves** the element's (possibly relayouted) position. There is **zero + stale-coordinate risk** — the precise concern Phase 2's coordinate guard must handle, + this phase avoids structurally. +3. It is a pure function of `RecordedEvent[]` → `string`, so it is fully unit-testable + with no device. + +## Detection rule + +While walking the event stream, maintain one boolean `keyboardLikelyUp`: + +- `type` event → keyboard comes up → set `true`. + (The existing `type` branch emits `- tapOn` to focus + `- inputText: …`.) +- Before emitting a `tap` or `long_press` step: if `keyboardLikelyUp`, emit + `- hideKeyboard` first, then set `keyboardLikelyUp = false`. +- `navigate` event → new screen → set `keyboardLikelyUp = false`. +- The focusing `tapOn` *inside* a `type` step is **not** guarded (the keyboard is wanted + there). +- `submit` (`- pressKey: Enter`) leaves the flag unchanged — Enter's dismiss behavior is + field-type-dependent (single-line submits/dismisses; multiline inserts a newline), so + bias toward guarding any subsequent tap. + +Properties: conservative (may over-inject — harmless) but never misses a +typed-then-tapped sequence, which is the exact reported failure. Deterministic and +order-only — no geometry needed (`RecordedEvent` carries no rects; geometry-gating would +require recorder changes and is an explicit non-goal here). + +### Emitted shape + +Before: +```yaml +- tapOn: + id: emailInput +- inputText: "user@example.com" +- tapOn: + id: submitButton +``` +After: +```yaml +- tapOn: + id: emailInput +- inputText: "user@example.com" +# rn-dev-agent: keyboard-occlusion guard (#356) +- hideKeyboard +- tapOn: + id: submitButton +``` + +The marker is emitted as a **preceding full comment line**, matching the generator's +existing comment style (`# navigated:`, `# NOTE:`) — not a trailing inline comment, +which a line-oriented validator could reject. It makes the auto-injection auditable in +the YAML and in PR diffs. The comment text is a static literal (no user-controlled +interpolation), so it needs no `stripNewlines`/`maestroScalar` handling. + +## Components & data flow + +- **`generateMaestro(events, opts)`** — only changed function. Add the `keyboardLikelyUp` + state and the pre-tap emission. No new exported symbols required; the rule is internal + to the walk. (Optionally factor the rule into a small pure helper if it aids testing.) +- No changes to `RecordedEvent`, the recorder, `save-as-action`, `maestro-validator`, + `repair-engine`, `runNative`, or either native runner. +- `generateDetox` is intentionally untouched (Detox is secondary and its `.typeText` + path is not the reported failure); noted as optional future parity. + +## Error handling + +There is no new failure mode: the transform only inserts an allowlisted, idempotent +step. Generated flows continue to pass `parseAndValidateFlow()` unchanged +(`hideKeyboard` is already allowed). If `events` is empty or contains no typed-then-tap +sequence, output is byte-identical to today. + +## Testing + +**Unit (primary, table-driven over event sequences):** +- typed → tap ⇒ `- hideKeyboard` injected before the tap. +- tap with no preceding type ⇒ no injection. +- type → tap → tap ⇒ injected before the first tap only (flag cleared). +- type → navigate → tap ⇒ no injection (navigate reset). +- type → long_press ⇒ injected before long_press. +- consecutive types (type → type → tap) ⇒ keyboard stays up; injected before the final tap; the focusing `tapOn` of the second `type` is not guarded. +- type → submit → tap ⇒ injected before the tap (submit does not reset). +- existing generator behaviors (navigation lookahead comments, selectors, metadata header, YAML-injection sanitization) remain unchanged — guard against regressions. + +**Device verification (both platforms — required by the issue):** record/replay a flow +that fills a text field then taps a bottom-pinned submit and asserts the next screen. +Confirm it was flaky before and replays reliably after, on **iOS and Android**. + +## Out of scope (follow-ups) + +- **Phase 2** — in-runner frame-precise occlusion guard for live `device_*` taps + (in-runner atomic; iOS `app.keyboards.frame`; Android `UiAutomation.getWindows()` → + `TYPE_INPUT_METHOD` bounds). Separate spec/PR, stacked on this one. +- **Phase 1.5** — repair-time injection in `repair-engine.ts` (insert `- hideKeyboard` + before a patched `tapOn` via body string-surgery) and a one-time **backfill** command + for existing on-disk action YAMLs (rewrites committed files → opt-in). +- Detox parity; `swipe` / `fill` occlusion handling. + +## Post-verification addition — Android replay-runner routing (B223) + +On-device verification found that the plugin's default L3 replay engine, +maestro-runner v1.0.9 (DeviceLab.dev), **silently no-ops `hideKeyboard` on +Android** (reports pass in ~5ms; `dumpsys input_method` shows `mInputShown=true` +after), while official Maestro v2.3.0 dismisses it. So the generation-time +injection alone, replayed via the default runner, did not actually fix Android. + +Added in this phase: `chooseMaestroDispatch` (`maestro-dispatch.ts`) gains a +`flowHasHideKeyboard` input; on **Android** it prefers the official Maestro CLI +when a flow contains `hideKeyboard` (verified to dismiss the keyboard on-device), +and falls through to maestro-runner with a `degradedReason` warning when the CLI +is absent. `maestro_run` detects `hideKeyboard` in the parsed commands +(`flowContainsHideKeyboard`) and passes the flag. iOS is unaffected +(maestro-runner honors `hideKeyboard` there). The maestro-runner no-op itself is +filed as **B223** for an upstream/runtime follow-up. + +## Success criteria + +- Newly generated/saved Maestro action flows emit `- hideKeyboard` before any + button tap that follows text entry. +- The transform is covered by deterministic unit tests. +- A fill→bottom-pinned-submit flow replays reliably on iOS and Android where it was + previously flaky. +- No change to generated output for flows without a typed-then-tapped sequence. diff --git a/scripts/cdp-bridge/dist/tools/maestro-dispatch.js b/scripts/cdp-bridge/dist/tools/maestro-dispatch.js index 979d25d4..ff144a71 100644 --- a/scripts/cdp-bridge/dist/tools/maestro-dispatch.js +++ b/scripts/cdp-bridge/dist/tools/maestro-dispatch.js @@ -66,10 +66,41 @@ export function shouldWarnFallback(reason) { warnedFallbackReasons.add(reason); return true; } +/** + * GH #356 / B223: detect whether a parsed Maestro flow contains a + * `hideKeyboard` step. Maestro represents it as the bare string command + * `'hideKeyboard'`; we also accept the object form `{ hideKeyboard: ... }` + * defensively. Used to route Android hideKeyboard flows to the official + * Maestro CLI (maestro-runner no-ops hideKeyboard on Android). + */ +export function flowContainsHideKeyboard(commands) { + return commands.some((c) => c === 'hideKeyboard' || + (typeof c === 'object' && c !== null && 'hideKeyboard' in c)); +} export function chooseMaestroDispatch(inputs) { const whichAdb = inputs.whichAdb ?? defaultWhichAdb; const whichMaestro = inputs.whichMaestro ?? defaultWhichMaestro; const runnerPath = (inputs.maestroRunnerPath ?? defaultMaestroRunnerPath)(); + // GH #356 / B223: maestro-runner v1.0.9 silently no-ops `hideKeyboard` on + // Android (reports pass in ~5ms, `mInputShown` stays true), which defeats the + // keyboard-occlusion guard's whole purpose. When an Android flow contains a + // hideKeyboard step, prefer the official Maestro CLI — verified to honor it on + // Android (`mInputShown=false` after). iOS maestro-runner honors hideKeyboard, + // so this only applies to Android. + const needsOfficialForKeyboard = inputs.platform === 'android' && inputs.flowHasHideKeyboard === true; + if (needsOfficialForKeyboard) { + const maestroPath = whichMaestro(); + if (maestroPath) { + return { + runner: 'maestro', + binPath: maestroPath, + buildArgs: (platform, flowFile, _appFile) => ['test', '--platform', platform, flowFile], + fallbackReason: 'Android flow uses hideKeyboard; maestro-runner v1.0.9 no-ops it on Android (B223) — using the Maestro CLI so the keyboard is actually dismissed', + }; + } + // CLI unavailable: fall through to maestro-runner (Tier 1) but mark the + // result degraded so the caller warns the keyboard will not be dismissed. + } // Tier 1: maestro-runner. Viable when (a) the binary is installed and // (b) we're on android OR adb is reachable (so the upstream bug doesn't bite). const runnerViable = runnerPath !== null && (inputs.platform === 'android' || whichAdb() !== null); @@ -80,6 +111,11 @@ export function chooseMaestroDispatch(inputs) { buildArgs: (platform, flowFile, appFile) => appFile ? ['--app-file', appFile, '--platform', platform, 'test', flowFile] : ['--platform', platform, 'test', flowFile], + ...(needsOfficialForKeyboard + ? { + degradedReason: 'Android flow uses hideKeyboard but the Maestro CLI is not installed; maestro-runner v1.0.9 no-ops hideKeyboard on Android (B223), so the keyboard will NOT be dismissed. Install the Maestro CLI (`brew install maestro`) for the keyboard-occlusion fix to work on Android.', + } + : {}), }; } // Tier 2: Maestro CLI fallback. Slower JVM cold start (~2s) but works on diff --git a/scripts/cdp-bridge/dist/tools/maestro-run.js b/scripts/cdp-bridge/dist/tools/maestro-run.js index 21c17cf8..db901e05 100644 --- a/scripts/cdp-bridge/dist/tools/maestro-run.js +++ b/scripts/cdp-bridge/dist/tools/maestro-run.js @@ -6,7 +6,7 @@ import { join, dirname } from 'node:path'; import { okResult, failResult, warnResult } from '../utils.js'; import { getActiveSession } from '../agent-device-wrapper.js'; import { resolveBundleId, readExpoSlug } from '../project-config.js'; -import { chooseMaestroDispatch, shouldWarnFallback } from './maestro-dispatch.js'; +import { chooseMaestroDispatch, shouldWarnFallback, flowContainsHideKeyboard, } from './maestro-dispatch.js'; import { resolveAppFileForClearState } from './resolve-ios-app-file.js'; import { buildMaestroFlow, parseAndValidateFlow, isValidBundleId, MaestroValidationError, } from '../domain/maestro-validator.js'; import { outputIndicatesFlowFailure } from '../domain/maestro-error-parser.js'; @@ -89,12 +89,9 @@ export function createMaestroRunHandler() { if (!platform) { return failResult('Cannot determine platform. Pass platform or open a device session first.'); } - // B59: tiered dispatch — maestro-runner when viable, Maestro CLI fallback - // when iOS-only and adb is missing, fail-fast with install hints when neither. - const dispatch = chooseMaestroDispatch({ platform }); - if ('error' in dispatch) { - return failResult(dispatch.error); - } + // GH #356/B223: the dispatch tier depends on whether the validated flow + // uses hideKeyboard on Android, so the runner is chosen AFTER parsing below. + let flowHasHideKeyboard = false; // Phase 134.1 (deepsec CRITICAL #4): both inlineYaml and flowPath // are caller-controlled. Parse, validate against the command allowlist // (rejecting runScript and other host-executing directives by default), @@ -132,6 +129,7 @@ export function createMaestroRunHandler() { ? { flowDir: dirname(args.flowPath), flowRoot: dirname(args.flowPath) } : {}; const parsed = parseAndValidateFlow(rawYaml, runFlowOpts); + flowHasHideKeyboard = flowContainsHideKeyboard(parsed.commands); const rawAppId = resolveAppId(args.appId, platform); headerAppId = parsed.appId ?? (rawAppId && isValidBundleId(rawAppId) ? rawAppId : undefined); if (rawAppId && !parsed.appId && !isValidBundleId(rawAppId)) { @@ -151,6 +149,13 @@ export function createMaestroRunHandler() { } throw err; } + // B59 + GH #356/B223: tiered dispatch — maestro-runner when viable, Maestro + // CLI fallback when iOS-only and adb is missing, and (B223) the Maestro CLI + // for Android flows that use hideKeyboard (maestro-runner no-ops it there). + const dispatch = chooseMaestroDispatch({ platform, flowHasHideKeyboard }); + if ('error' in dispatch) { + return failResult(dispatch.error); + } const timeout = args.timeoutMs ?? 120_000; // GH #116: build the final argv. Start with the dispatch tier's // base args, then append `-e KEY=VALUE` pairs for any supplied @@ -196,19 +201,23 @@ export function createMaestroRunHandler() { timedOut: false, outputTruncated: false, ...(dispatch.fallbackReason ? { fallbackReason: dispatch.fallbackReason } : {}), + ...(dispatch.degradedReason ? { degradedReason: dispatch.degradedReason } : {}), }; + // GH #356/B223: a degradedReason (Android hideKeyboard with no Maestro CLI) + // is a caveat surfaced the same way as a fallbackReason. + const caveat = dispatch.fallbackReason ?? dispatch.degradedReason; if (passed) { // B59 (Gemini review, conf 82): on success-with-fallback, only emit // a loud warning the FIRST time per process so a 100-flow loop // doesn't generate 100 identical warnings. Subsequent successes - // carry the reason silently in meta.fallbackReason. - if (dispatch.fallbackReason && shouldWarnFallback(dispatch.fallbackReason)) { - return warnResult(meta, dispatch.fallbackReason); + // carry the reason silently in meta. + if (caveat && shouldWarnFallback(caveat)) { + return warnResult(meta, caveat); } return okResult(meta); } - const baseWarnMsg = dispatch.fallbackReason - ? `${dispatch.fallbackReason}; flow completed with warnings or failures` + const baseWarnMsg = caveat + ? `${caveat}; flow completed with warnings or failures` : 'Flow completed with warnings or failures'; // GH #263: classify on the FULL output (not the sliced meta.output). const warnAug = augmentFailureWithDegradation(output, resolveFloorMs(process.env.RN_RUNTIME_DEGRADED_FLOOR_MS), baseWarnMsg, meta); diff --git a/scripts/cdp-bridge/dist/tools/maestro-test-all.js b/scripts/cdp-bridge/dist/tools/maestro-test-all.js index 7d49bafd..eaeda7e6 100644 --- a/scripts/cdp-bridge/dist/tools/maestro-test-all.js +++ b/scripts/cdp-bridge/dist/tools/maestro-test-all.js @@ -6,7 +6,7 @@ import { tmpdir } from 'node:os'; import { okResult, failResult, warnResult } from '../utils.js'; import { getActiveSession } from '../agent-device-wrapper.js'; import { findProjectRoot } from '../nav-graph/storage.js'; -import { chooseMaestroDispatch, shouldWarnFallback } from './maestro-dispatch.js'; +import { chooseMaestroDispatch, shouldWarnFallback, flowContainsHideKeyboard, } from './maestro-dispatch.js'; import { buildMaestroFlow, parseAndValidateFlow, MaestroValidationError, } from '../domain/maestro-validator.js'; import { runFlowParked } from './maestro-run.js'; import { outputIndicatesFlowFailure } from '../domain/maestro-error-parser.js'; @@ -65,6 +65,9 @@ export function createMaestroTestAllHandler() { const results = []; let passed = 0; let failed = 0; + // GH #356/B223: surfaced once if any Android flow needed hideKeyboard but + // the Maestro CLI was unavailable, so it ran on maestro-runner (no-op). + let keyboardCaveat = null; for (const flow of flows) { const name = flow.replace(flowDir + '/', ''); const start = Date.now(); @@ -77,9 +80,11 @@ export function createMaestroTestAllHandler() { // any inert metadata or duplicated headers can't sneak through. let safeFlowFile; let appFile; + let flowHasHideKeyboard = false; try { const yamlText = readFileSync(flow, 'utf-8'); const parsed = parseAndValidateFlow(yamlText); + flowHasHideKeyboard = flowContainsHideKeyboard(parsed.commands); const canonical = buildMaestroFlow(parsed.appId !== undefined ? { appId: parsed.appId } : {}, parsed.commands); safeFlowFile = join(tmpdir(), `rn-maestro-validated-${Date.now()}-${Math.random().toString(36).slice(2, 8)}.yaml`); writeFileSync(safeFlowFile, canonical, 'utf-8'); @@ -115,8 +120,20 @@ export function createMaestroTestAllHandler() { break; continue; } + // GH #356/B223: Android flows that use hideKeyboard must run via the + // official Maestro CLI (maestro-runner no-ops hideKeyboard on Android). + // Re-route per flow; fall back to the base dispatch if re-selection errors. + let flowDispatch = dispatch; + if (platform === 'android' && flowHasHideKeyboard) { + const rerouted = chooseMaestroDispatch({ platform, flowHasHideKeyboard: true }); + if (!('error' in rerouted)) { + flowDispatch = rerouted; + if (rerouted.degradedReason) + keyboardCaveat ??= rerouted.degradedReason; + } + } try { - const { stdout, stderr } = await runFlowParked(() => execFile(dispatch.binPath, dispatch.buildArgs(platform, safeFlowFile, appFile), { + const { stdout, stderr } = await runFlowParked(() => execFile(flowDispatch.binPath, flowDispatch.buildArgs(platform, safeFlowFile, appFile), { timeout, encoding: 'utf8', maxBuffer: 10 * 1024 * 1024, @@ -153,6 +170,9 @@ export function createMaestroTestAllHandler() { break; } } + // GH #356/B223: surface the base dispatch's fallback reason, or (if any + // Android hideKeyboard flow had to degrade to maestro-runner) the keyboard caveat. + const batchCaveat = dispatch.fallbackReason ?? keyboardCaveat; const summary = { total: flows.length, executed: results.length, @@ -161,18 +181,18 @@ export function createMaestroTestAllHandler() { platform, flowDir, runner: dispatch.runner, - ...(dispatch.fallbackReason ? { fallbackReason: dispatch.fallbackReason } : {}), + ...(batchCaveat ? { fallbackReason: batchCaveat } : {}), results, }; if (failed > 0) { const baseMsg = `${failed} of ${results.length} flows failed`; - return warnResult(summary, dispatch.fallbackReason ? `${dispatch.fallbackReason}; ${baseMsg}` : baseMsg); + return warnResult(summary, batchCaveat ? `${batchCaveat}; ${baseMsg}` : baseMsg); } // B59 (Gemini review, conf 82): suppress repeated success-with-fallback // warnings within the same process — first call surfaces, subsequent // calls keep the reason in meta only. - if (dispatch.fallbackReason && shouldWarnFallback(dispatch.fallbackReason)) { - return warnResult(summary, dispatch.fallbackReason); + if (batchCaveat && shouldWarnFallback(batchCaveat)) { + return warnResult(summary, batchCaveat); } return okResult(summary); }; diff --git a/scripts/cdp-bridge/dist/tools/test-recorder-generators.js b/scripts/cdp-bridge/dist/tools/test-recorder-generators.js index 8e4fec3d..33d1fe57 100644 --- a/scripts/cdp-bridge/dist/tools/test-recorder-generators.js +++ b/scripts/cdp-bridge/dist/tools/test-recorder-generators.js @@ -146,13 +146,20 @@ export function generateMaestro(events, opts = {}) { // B137: navigate events reached via tap lookahead are emitted inline with the // tap; skip them here to avoid double-emission. const consumedNavIndices = new Set(); + let keyboardLikelyUp = false; for (let i = 0; i < events.length; i++) { const ev = events[i]; switch (ev.type) { case 'tap': { const sel = maestroSelector(ev); - if (sel) + if (sel) { + if (keyboardLikelyUp) { + lines.push('# rn-dev-agent: keyboard-occlusion guard (#356)'); + lines.push('- hideKeyboard'); + keyboardLikelyUp = false; + } lines.push(`- tapOn:\n ${sel}`); + } else lines.push('# tap: missing testID/label'); const hit = lookaheadNavigate(events, i); @@ -167,8 +174,14 @@ export function generateMaestro(events, opts = {}) { } case 'long_press': { const sel = maestroSelector(ev); - if (sel) + if (sel) { + if (keyboardLikelyUp) { + lines.push('# rn-dev-agent: keyboard-occlusion guard (#356)'); + lines.push('- hideKeyboard'); + keyboardLikelyUp = false; + } lines.push(`- longPressOn:\n ${sel}`); + } else lines.push('# long_press: missing testID/label'); const hit = lookaheadNavigate(events, i); @@ -186,6 +199,7 @@ export function generateMaestro(events, opts = {}) { if (sel) { lines.push(`- tapOn:\n ${sel}`); lines.push(`- inputText: ${JSON.stringify(ev.value)}`); + keyboardLikelyUp = true; } else { lines.push(`# type: missing testID/label, value=${JSON.stringify(ev.value)}`); @@ -214,6 +228,7 @@ export function generateMaestro(events, opts = {}) { break; } case 'navigate': { + keyboardLikelyUp = false; if (consumedNavIndices.has(i)) break; const next = nextSelector(events, i, maestroSelector); diff --git a/scripts/cdp-bridge/src/tools/maestro-dispatch.ts b/scripts/cdp-bridge/src/tools/maestro-dispatch.ts index 8b4d1ced..8351eb29 100644 --- a/scripts/cdp-bridge/src/tools/maestro-dispatch.ts +++ b/scripts/cdp-bridge/src/tools/maestro-dispatch.ts @@ -33,14 +33,30 @@ export interface MaestroDispatch { */ buildArgs(platform: 'ios' | 'android', flowFile: string, appFile?: string): string[]; /** - * Present when the fallback runner was chosen — surfaces in caller's - * warnResult so users see why the slower path was used. + * Present when a non-default runner was deliberately chosen — the B59 CLI + * fallback (iOS-only, no adb) OR the B223 CLI preference (Android flow uses + * hideKeyboard, which maestro-runner no-ops). Surfaces in the caller's + * warnResult so users see why the chosen path differs from the default. */ fallbackReason?: string; + /** + * GH #356 / B223: set when an Android flow needs `hideKeyboard` but the + * official Maestro CLI is unavailable, so we fell back to maestro-runner — + * which silently no-ops `hideKeyboard` on Android. Surfaces a warning so the + * user knows the keyboard will not actually be dismissed. + */ + degradedReason?: string; } export interface MaestroDispatchInputs { platform: 'ios' | 'android'; + /** + * GH #356 / B223: the flow contains a `hideKeyboard` step. maestro-runner + * v1.0.9 silently no-ops `hideKeyboard` on Android (reports pass in ~5ms, + * keyboard stays up). When set on Android, prefer the official Maestro CLI, + * which honors `hideKeyboard`. No effect on iOS (maestro-runner honors it). + */ + flowHasHideKeyboard?: boolean; /** Override for tests. Defaults to `which adb` via spawnSync. */ whichAdb?: () => string | null; /** Override for tests. Defaults to `which maestro` via spawnSync. */ @@ -107,6 +123,21 @@ export interface MaestroDispatchError { hint: string; } +/** + * GH #356 / B223: detect whether a parsed Maestro flow contains a + * `hideKeyboard` step. Maestro represents it as the bare string command + * `'hideKeyboard'`; we also accept the object form `{ hideKeyboard: ... }` + * defensively. Used to route Android hideKeyboard flows to the official + * Maestro CLI (maestro-runner no-ops hideKeyboard on Android). + */ +export function flowContainsHideKeyboard(commands: readonly unknown[]): boolean { + return commands.some( + (c) => + c === 'hideKeyboard' || + (typeof c === 'object' && c !== null && 'hideKeyboard' in (c as Record)), + ); +} + export function chooseMaestroDispatch( inputs: MaestroDispatchInputs, ): MaestroDispatch | MaestroDispatchError { @@ -114,6 +145,29 @@ export function chooseMaestroDispatch( const whichMaestro = inputs.whichMaestro ?? defaultWhichMaestro; const runnerPath = (inputs.maestroRunnerPath ?? defaultMaestroRunnerPath)(); + // GH #356 / B223: maestro-runner v1.0.9 silently no-ops `hideKeyboard` on + // Android (reports pass in ~5ms, `mInputShown` stays true), which defeats the + // keyboard-occlusion guard's whole purpose. When an Android flow contains a + // hideKeyboard step, prefer the official Maestro CLI — verified to honor it on + // Android (`mInputShown=false` after). iOS maestro-runner honors hideKeyboard, + // so this only applies to Android. + const needsOfficialForKeyboard = + inputs.platform === 'android' && inputs.flowHasHideKeyboard === true; + if (needsOfficialForKeyboard) { + const maestroPath = whichMaestro(); + if (maestroPath) { + return { + runner: 'maestro', + binPath: maestroPath, + buildArgs: (platform, flowFile, _appFile) => ['test', '--platform', platform, flowFile], + fallbackReason: + 'Android flow uses hideKeyboard; maestro-runner v1.0.9 no-ops it on Android (B223) — using the Maestro CLI so the keyboard is actually dismissed', + }; + } + // CLI unavailable: fall through to maestro-runner (Tier 1) but mark the + // result degraded so the caller warns the keyboard will not be dismissed. + } + // Tier 1: maestro-runner. Viable when (a) the binary is installed and // (b) we're on android OR adb is reachable (so the upstream bug doesn't bite). const runnerViable = @@ -126,6 +180,12 @@ export function chooseMaestroDispatch( appFile ? ['--app-file', appFile, '--platform', platform, 'test', flowFile] : ['--platform', platform, 'test', flowFile], + ...(needsOfficialForKeyboard + ? { + degradedReason: + 'Android flow uses hideKeyboard but the Maestro CLI is not installed; maestro-runner v1.0.9 no-ops hideKeyboard on Android (B223), so the keyboard will NOT be dismissed. Install the Maestro CLI (`brew install maestro`) for the keyboard-occlusion fix to work on Android.', + } + : {}), }; } diff --git a/scripts/cdp-bridge/src/tools/maestro-run.ts b/scripts/cdp-bridge/src/tools/maestro-run.ts index a6d4a0ae..bea60ab7 100644 --- a/scripts/cdp-bridge/src/tools/maestro-run.ts +++ b/scripts/cdp-bridge/src/tools/maestro-run.ts @@ -7,7 +7,11 @@ import type { ToolResult } from '../utils.js'; import { okResult, failResult, warnResult } from '../utils.js'; import { getActiveSession } from '../agent-device-wrapper.js'; import { resolveBundleId, readExpoSlug } from '../project-config.js'; -import { chooseMaestroDispatch, shouldWarnFallback } from './maestro-dispatch.js'; +import { + chooseMaestroDispatch, + shouldWarnFallback, + flowContainsHideKeyboard, +} from './maestro-dispatch.js'; import { resolveAppFileForClearState } from './resolve-ios-app-file.js'; import { buildMaestroFlow, @@ -134,12 +138,9 @@ export function createMaestroRunHandler(): (args: MaestroRunArgs) => Promise Promise Promise Promise Pro const results: FlowResult[] = []; let passed = 0; let failed = 0; + // GH #356/B223: surfaced once if any Android flow needed hideKeyboard but + // the Maestro CLI was unavailable, so it ran on maestro-runner (no-op). + let keyboardCaveat: string | null = null; for (const flow of flows) { const name = flow.replace(flowDir + '/', ''); @@ -110,9 +117,11 @@ export function createMaestroTestAllHandler(): (args: MaestroTestAllArgs) => Pro // any inert metadata or duplicated headers can't sneak through. let safeFlowFile: string; let appFile: string | undefined; + let flowHasHideKeyboard = false; try { const yamlText = readFileSync(flow, 'utf-8'); const parsed = parseAndValidateFlow(yamlText); + flowHasHideKeyboard = flowContainsHideKeyboard(parsed.commands); const canonical = buildMaestroFlow( parsed.appId !== undefined ? { appId: parsed.appId } : {}, parsed.commands, @@ -158,14 +167,30 @@ export function createMaestroTestAllHandler(): (args: MaestroTestAllArgs) => Pro continue; } + // GH #356/B223: Android flows that use hideKeyboard must run via the + // official Maestro CLI (maestro-runner no-ops hideKeyboard on Android). + // Re-route per flow; fall back to the base dispatch if re-selection errors. + let flowDispatch = dispatch; + if (platform === 'android' && flowHasHideKeyboard) { + const rerouted = chooseMaestroDispatch({ platform, flowHasHideKeyboard: true }); + if (!('error' in rerouted)) { + flowDispatch = rerouted; + if (rerouted.degradedReason) keyboardCaveat ??= rerouted.degradedReason; + } + } + try { const { stdout, stderr } = await runFlowParked( () => - execFile(dispatch.binPath, dispatch.buildArgs(platform, safeFlowFile, appFile), { - timeout, - encoding: 'utf8', - maxBuffer: 10 * 1024 * 1024, - }), + execFile( + flowDispatch.binPath, + flowDispatch.buildArgs(platform, safeFlowFile, appFile), + { + timeout, + encoding: 'utf8', + maxBuffer: 10 * 1024 * 1024, + }, + ), { platform, deviceId: getActiveSession()?.deviceId }, ); const output = (stdout + '\n' + stderr).trim(); @@ -199,6 +224,9 @@ export function createMaestroTestAllHandler(): (args: MaestroTestAllArgs) => Pro } } + // GH #356/B223: surface the base dispatch's fallback reason, or (if any + // Android hideKeyboard flow had to degrade to maestro-runner) the keyboard caveat. + const batchCaveat = dispatch.fallbackReason ?? keyboardCaveat; const summary = { total: flows.length, executed: results.length, @@ -207,22 +235,19 @@ export function createMaestroTestAllHandler(): (args: MaestroTestAllArgs) => Pro platform, flowDir, runner: dispatch.runner, - ...(dispatch.fallbackReason ? { fallbackReason: dispatch.fallbackReason } : {}), + ...(batchCaveat ? { fallbackReason: batchCaveat } : {}), results, }; if (failed > 0) { const baseMsg = `${failed} of ${results.length} flows failed`; - return warnResult( - summary, - dispatch.fallbackReason ? `${dispatch.fallbackReason}; ${baseMsg}` : baseMsg, - ); + return warnResult(summary, batchCaveat ? `${batchCaveat}; ${baseMsg}` : baseMsg); } // B59 (Gemini review, conf 82): suppress repeated success-with-fallback // warnings within the same process — first call surfaces, subsequent // calls keep the reason in meta only. - if (dispatch.fallbackReason && shouldWarnFallback(dispatch.fallbackReason)) { - return warnResult(summary, dispatch.fallbackReason); + if (batchCaveat && shouldWarnFallback(batchCaveat)) { + return warnResult(summary, batchCaveat); } return okResult(summary); }; diff --git a/scripts/cdp-bridge/src/tools/test-recorder-generators.ts b/scripts/cdp-bridge/src/tools/test-recorder-generators.ts index 1daadcea..394a2cbf 100644 --- a/scripts/cdp-bridge/src/tools/test-recorder-generators.ts +++ b/scripts/cdp-bridge/src/tools/test-recorder-generators.ts @@ -184,13 +184,21 @@ export function generateMaestro(events: RecordedEvent[], opts: GenerateOpts = {} // tap; skip them here to avoid double-emission. const consumedNavIndices = new Set(); + let keyboardLikelyUp = false; + for (let i = 0; i < events.length; i++) { const ev = events[i]; switch (ev.type) { case 'tap': { const sel = maestroSelector(ev); - if (sel) lines.push(`- tapOn:\n ${sel}`); - else lines.push('# tap: missing testID/label'); + if (sel) { + if (keyboardLikelyUp) { + lines.push('# rn-dev-agent: keyboard-occlusion guard (#356)'); + lines.push('- hideKeyboard'); + keyboardLikelyUp = false; + } + lines.push(`- tapOn:\n ${sel}`); + } else lines.push('# tap: missing testID/label'); const hit = lookaheadNavigate(events, i); if (hit) { lines.push( @@ -204,8 +212,14 @@ export function generateMaestro(events: RecordedEvent[], opts: GenerateOpts = {} } case 'long_press': { const sel = maestroSelector(ev); - if (sel) lines.push(`- longPressOn:\n ${sel}`); - else lines.push('# long_press: missing testID/label'); + if (sel) { + if (keyboardLikelyUp) { + lines.push('# rn-dev-agent: keyboard-occlusion guard (#356)'); + lines.push('- hideKeyboard'); + keyboardLikelyUp = false; + } + lines.push(`- longPressOn:\n ${sel}`); + } else lines.push('# long_press: missing testID/label'); const hit = lookaheadNavigate(events, i); if (hit) { lines.push( @@ -222,6 +236,7 @@ export function generateMaestro(events: RecordedEvent[], opts: GenerateOpts = {} if (sel) { lines.push(`- tapOn:\n ${sel}`); lines.push(`- inputText: ${JSON.stringify(ev.value)}`); + keyboardLikelyUp = true; } else { lines.push(`# type: missing testID/label, value=${JSON.stringify(ev.value)}`); } @@ -249,6 +264,7 @@ export function generateMaestro(events: RecordedEvent[], opts: GenerateOpts = {} break; } case 'navigate': { + keyboardLikelyUp = false; if (consumedNavIndices.has(i)) break; const next = nextSelector(events, i, maestroSelector); lines.push(`# navigated: ${stripNewlines(ev.from ?? '?')} -> ${stripNewlines(ev.to)}`); diff --git a/scripts/cdp-bridge/test/unit/maestro-dispatch.test.js b/scripts/cdp-bridge/test/unit/maestro-dispatch.test.js index eca53ed4..6d646f13 100644 --- a/scripts/cdp-bridge/test/unit/maestro-dispatch.test.js +++ b/scripts/cdp-bridge/test/unit/maestro-dispatch.test.js @@ -8,6 +8,7 @@ import assert from 'node:assert/strict'; import { chooseMaestroDispatch, _resetMaestroDispatchCache, + flowContainsHideKeyboard, } from '../../dist/tools/maestro-dispatch.js'; test.beforeEach(() => _resetMaestroDispatchCache()); @@ -157,6 +158,72 @@ test('B59: Tier 1 wins when viable — does NOT use Maestro CLI just because bot assert.equal('runner' in d ? d.runner : null, 'maestro-runner', 'fast path must be preferred'); }); +// ── #356 / B223: Android hideKeyboard routing ──────────────────────── +// maestro-runner v1.0.9 silently no-ops `hideKeyboard` on Android, defeating +// the keyboard-occlusion guard. An Android flow that uses hideKeyboard must +// prefer the official Maestro CLI (which honors it). + +test('#356/B223: android + hideKeyboard + maestro CLI present → official Maestro (not maestro-runner)', () => { + const d = chooseMaestroDispatch({ + platform: 'android', + flowHasHideKeyboard: true, + whichAdb: () => '/adb', + whichMaestro: () => '/opt/homebrew/bin/maestro', + maestroRunnerPath: () => '/runner', + }); + assert.equal('runner' in d ? d.runner : null, 'maestro'); + assert.match('fallbackReason' in d ? (d.fallbackReason ?? '') : '', /hideKeyboard|B223/); +}); + +test('#356/B223: android + hideKeyboard + NO maestro CLI → maestro-runner with degradedReason warning', () => { + const d = chooseMaestroDispatch({ + platform: 'android', + flowHasHideKeyboard: true, + whichAdb: () => '/adb', + whichMaestro: () => null, + maestroRunnerPath: () => '/runner', + }); + assert.equal('runner' in d ? d.runner : null, 'maestro-runner'); + assert.match( + 'degradedReason' in d ? (d.degradedReason ?? '') : '', + /will NOT be dismissed|brew install maestro/, + ); +}); + +test('#356/B223: android WITHOUT hideKeyboard → maestro-runner unchanged (no degradedReason)', () => { + const d = chooseMaestroDispatch({ + platform: 'android', + flowHasHideKeyboard: false, + whichAdb: () => '/adb', + whichMaestro: () => '/maestro', + maestroRunnerPath: () => '/runner', + }); + assert.equal('runner' in d ? d.runner : null, 'maestro-runner'); + assert.equal('degradedReason' in d && d.degradedReason !== undefined, false); +}); + +test('#356/B223: ios + hideKeyboard → maestro-runner (iOS honors hideKeyboard; routing is android-only)', () => { + const d = chooseMaestroDispatch({ + platform: 'ios', + flowHasHideKeyboard: true, + whichAdb: () => '/adb', + whichMaestro: () => '/maestro', + maestroRunnerPath: () => '/runner', + }); + assert.equal('runner' in d ? d.runner : null, 'maestro-runner'); + assert.equal('degradedReason' in d && d.degradedReason !== undefined, false); +}); + +test('#356/B223: flowContainsHideKeyboard detects a bare hideKeyboard command', () => { + assert.equal( + flowContainsHideKeyboard(['launchApp', 'hideKeyboard', { tapOn: { id: 'x' } }]), + true, + ); + assert.equal(flowContainsHideKeyboard([{ hideKeyboard: true }]), true); + assert.equal(flowContainsHideKeyboard(['launchApp', { tapOn: { id: 'x' } }]), false); + assert.equal(flowContainsHideKeyboard([]), false); +}); + // ── Cache reset hook ───────────────────────────────────────────────── test('B59: _resetMaestroDispatchCache clears between tests so each one is hermetic', () => { diff --git a/scripts/cdp-bridge/test/unit/test-recorder-generators.test.js b/scripts/cdp-bridge/test/unit/test-recorder-generators.test.js index 620ca615..5219ddc0 100644 --- a/scripts/cdp-bridge/test/unit/test-recorder-generators.test.js +++ b/scripts/cdp-bridge/test/unit/test-recorder-generators.test.js @@ -191,3 +191,84 @@ test('M7 Maestro: empty tags array is treated as omitted', () => { const out = generateMaestro([{ type: 'tap', testID: 'x', t: 1 }], { tags: [] }); assert.doesNotMatch(out, /# tags:/); }); + +test('#356 Maestro: hideKeyboard injected before a tap that follows type', () => { + const out = generateMaestro([ + { type: 'type', testID: 'email', value: 'a@b.c', t: 1 }, + { type: 'tap', testID: 'submit', t: 2 }, + ]); + assert.match(out, /# rn-dev-agent: keyboard-occlusion guard \(#356\)/); + const hk = out.indexOf('- hideKeyboard'); + const input = out.indexOf('- inputText:'); + const submitTap = out.indexOf('id: submit'); + assert.ok(hk > -1, 'hideKeyboard should be injected'); + assert.ok(hk > input, 'hideKeyboard comes after the inputText'); + assert.ok(hk < submitTap, 'hideKeyboard comes before the submit tap'); + const emailTap = out.indexOf('id: email'); + assert.ok( + !out.slice(0, emailTap).includes('- hideKeyboard'), + 'the focusing tap of the type step is NOT guarded', + ); +}); + +test('#356 Maestro: no hideKeyboard when a tap is not preceded by type', () => { + const out = generateMaestro([{ type: 'tap', testID: 'submit', t: 1 }]); + assert.ok(!out.includes('- hideKeyboard'), 'no keyboard, no injection'); +}); + +test('#356 Maestro: single hideKeyboard for type then two taps', () => { + const out = generateMaestro([ + { type: 'type', testID: 'email', value: 'a@b.c', t: 1 }, + { type: 'tap', testID: 'next', t: 2 }, + { type: 'tap', testID: 'submit', t: 3 }, + ]); + const count = (out.match(/- hideKeyboard/g) || []).length; + assert.equal(count, 1, 'flag cleared after first guarded tap'); + assert.ok(out.indexOf('- hideKeyboard') < out.indexOf('id: next')); +}); + +test('#356 Maestro: navigate resets keyboard state (no injection after nav)', () => { + const out = generateMaestro([ + { type: 'type', testID: 'email', value: 'a@b.c', t: 1 }, + { type: 'navigate', from: 'A', to: 'B', t: 2 }, + { type: 'tap', testID: 'submit', t: 3 }, + ]); + assert.ok(!out.includes('- hideKeyboard'), 'navigation dismisses the keyboard'); +}); + +test('#356 Maestro: consecutive types inject once before the final tap', () => { + const out = generateMaestro([ + { type: 'type', testID: 'first', value: 'x', t: 1 }, + { type: 'type', testID: 'second', value: 'y', t: 2 }, + { type: 'tap', testID: 'submit', t: 3 }, + ]); + const count = (out.match(/- hideKeyboard/g) || []).length; + assert.equal(count, 1, 'one injection for the trailing button tap'); + const secondTap = out.indexOf('id: second'); + assert.ok( + !out.slice(0, secondTap).includes('- hideKeyboard'), + 'the focusing tap of the second type is not guarded', + ); + assert.ok(out.indexOf('- hideKeyboard') < out.indexOf('id: submit')); +}); + +test('#356 Maestro: submit (Enter) does not reset keyboard state', () => { + const out = generateMaestro([ + { type: 'type', testID: 'email', value: 'a@b.c', t: 1 }, + { type: 'submit', t: 2 }, + { type: 'tap', testID: 'submit', t: 3 }, + ]); + const hk = out.indexOf('- hideKeyboard'); + assert.ok(hk > -1, 'still guarded after submit'); + assert.ok(hk < out.indexOf('id: submit')); +}); + +test('#356 Maestro: long_press following type is guarded', () => { + const out = generateMaestro([ + { type: 'type', testID: 'email', value: 'a@b.c', t: 1 }, + { type: 'long_press', testID: 'avatar', t: 2 }, + ]); + const hk = out.indexOf('- hideKeyboard'); + assert.ok(hk > -1, 'long_press is guarded too'); + assert.ok(hk < out.indexOf('- longPressOn:')); +});