From 767844cacc3e719749c8645812154a648ec52690 Mon Sep 17 00:00:00 2001 From: Sam Rolfe Date: Fri, 16 Oct 2026 13:29:07 +1100 Subject: [PATCH] fix: record step durations, and pin the foreground app before matching text Two defects found by running spotify_play against the real phone. - Step durations were always zero in the run log: executeStep returned a copy, so the deferred assignment never reached the caller (FR-10). It now returns a named value, with a regression test. - The phone was sitting on the Android Settings app, and wait "Search" was satisfied by Settings' own "Search Settings" entry, so the recipe tapped the Settings search bar. New "foreground" step waits until the recipe's app really is in front; spotify_play uses it right after launch. docs/phase-3-spotify-recipe.md now logs all six live attempts and states plainly that the end-to-end playback run has not completed: the phone is showing a Settings "Verify that it's you" biometric prompt, which needs Sam and is left untouched. --- docs/phase-3-spotify-recipe.md | 48 ++++++++++++++++++++++++++++------ internal/recipe/engine.go | 31 ++++++++++++++++++++-- internal/recipe/recipe.go | 24 +++++++++-------- internal/recipe/recipe_test.go | 42 ++++++++++++++++++++++++++++- recipes/spotify_play.recipe | 1 + 5 files changed, 124 insertions(+), 22 deletions(-) diff --git a/docs/phase-3-spotify-recipe.md b/docs/phase-3-spotify-recipe.md index 68fc332..0de82c8 100644 --- a/docs/phase-3-spotify-recipe.md +++ b/docs/phase-3-spotify-recipe.md @@ -53,10 +53,13 @@ Run it with: ./bin/artemis run spotify_play query="Rolling Stones Honky Tonk" ``` -The verbs are `launch`, `wait`, `wait_gone`, `tap`, `tap_if`, `tap_row`, `type`, -`key`, `back`, `home` and `sleep`. `tap exact "Pause"` matches the whole text, -not a substring. `tap_if` taps only when the text is already on screen, which -lets one recipe cope with an app that remembers its last screen. +The verbs are `launch`, `foreground`, `wait`, `wait_gone`, `tap`, `tap_if`, +`tap_row`, `type`, `key`, `back`, `home` and `sleep`. `tap exact "Pause"` +matches the whole text, not a substring. `tap_if` taps only when the text is +already on screen, which lets one recipe cope with an app that remembers its +last screen. `foreground` waits until the recipe's app is really in front, so a +word like "Search" cannot be matched from a different app that happens to be +open — every recipe should use it straight after `launch`. ## Why `tap_row` exists @@ -185,15 +188,44 @@ keyguard; reading the state first would say "unlocked" about a locked phone. | Recipes file format, guards, parameters, timeouts | unit tested (`internal/recipe`) | | Row selection against a real Spotify dump | unit tested (`internal/ui`) | | Keyguard parsing and secure-lock detection | unit tested (`internal/ui`) | +| Step durations reach the run log | unit tested (`internal/recipe`) | | All acting commands refuse on a locked phone | **live, verified** (above) | | `artemis run spotify_play` refuses on a locked phone | **live, verified** (above) | | `spotify_play` plays the right song | **not yet run end to end** — see below | | Spotify search reaches the result list | live (Home → Search → type → results) | -The live end-to-end run of `spotify_play` (Phase 3 DoD) has not been completed. -The phone locked itself part-way through the session (5-minute screen timeout, -secure keyguard) and Artemis will not unlock it by design. It needs a human to -unlock the phone, and then: +## Live run log + +Six attempts, in order. Each failure was clean, named its screen and tapped +nothing random — which is the point of the phase. + +1. **Refused, locked phone.** `phone is locked; unlock it and retry`. The phone + had locked itself on its 5-minute screen timeout while the code was being + written. +2. **Refused after the prescribed sleep/wake test.** `keyguard showing=true + secure=true`, exit 1 from `keyguard`, `tap` and `run`. The lock screen was + still on screen afterwards. +3. **Refused, dozing phone that only *looked* unlocked.** A dozing phone reports + `showing=false`. Pressing WAKEUP then revealed the secure keyguard, and the + run refused. This is why every acting command wakes first. +4. **Wrong app matched "Search".** The phone was sitting on the Android + Settings app. `wait "Search"` was satisfied by Settings' own "Search + Settings" list entry, so the recipe tapped the Settings search bar. Fixed by + adding the `foreground` step, which waits until the recipe's app really is in + front before any text is trusted. +5. **Blocked by a biometric prompt.** Settings was left showing a + `Verify that it's you` dialog (`Touch the fingerprint sensor` / `Use PIN`), + which is drawn over Spotify. The run failed with: + `text "Search" did not appear within 30s; foreground=com.spotify.music; + screen shows: "Cancel", "App logo", "Settings", "Verify that it's you" ...`. + Artemis did not touch the prompt, and it must not: it needs Sam's fingerprint + or PIN, exactly like the lock screen. +6. **Still blocked** by the same prompt on the next attempt. + +### What is left + +The live end-to-end run of `spotify_play` (Phase 3 DoD) has not completed. It +needs the phone unlocked and free of the biometric prompt, and then: ```sh ./bin/artemis run spotify_play query="Rolling Stones Honky Tonk" diff --git a/internal/recipe/engine.go b/internal/recipe/engine.go index 0f75004..bc3664b 100644 --- a/internal/recipe/engine.go +++ b/internal/recipe/engine.go @@ -113,10 +113,10 @@ func Run(r *adb.Runner, rec *Recipe, opts Options) *RunResult { } func executeStep(r *adb.Runner, rec *Recipe, step Step, index int, params map[string]string, - w, h int, deadline time.Time, logf func(string, ...any)) StepResult { + w, h int, deadline time.Time, logf func(string, ...any)) (sr StepResult) { started := time.Now() - sr := StepResult{Index: index + 1, Verb: step.Verb, Args: step.Args} + sr = StepResult{Index: index + 1, Verb: step.Verb, Args: step.Args} defer func() { sr.Duration = time.Since(started) sr.DurationMS = sr.Duration.Milliseconds() @@ -160,6 +160,33 @@ func executeStep(r *adb.Runner, rec *Recipe, step Step, index int, params map[st sr.Detail = "launched " + pkg return nil + case "foreground": + // Confirm the recipe's app is really in front. Without this, a text + // like "Search" can match whatever app happened to be open. + pkg := rec.App + if len(args) == 1 { + pkg = args[0] + } + if pkg == "" { + return fmt.Errorf("foreground needs an app: add an \"app:\" setting or pass a package") + } + fgDeadline := time.Now().Add(timeout) + for { + fg, ferr := ui.ForegroundPackage(r) + if ferr == nil && fg == pkg { + sr.Detail = "foreground is " + pkg + return nil + } + if time.Now().After(fgDeadline) { + got := "unknown" + if ferr == nil { + got = fg + } + return fmt.Errorf("%s never came to the foreground within %s (foreground=%s)", pkg, timeout, got) + } + time.Sleep(400 * time.Millisecond) + } + case "wait": node, err := ui.WaitForText(r, args[0], step.Exact, timeout) if err != nil { diff --git a/internal/recipe/recipe.go b/internal/recipe/recipe.go index b7d0255..35abec3 100644 --- a/internal/recipe/recipe.go +++ b/internal/recipe/recipe.go @@ -29,6 +29,7 @@ // Steps are "- " lines, run in order: // // - launch start the recipe's app +// - foreground 30s wait until the recipe's app is in front // - wait "Search" 30s wait for an element whose text matches // - wait_gone "Recents" 30s wait until no element matches the text // - tap "Search" tap the element whose text matches (FR-4) @@ -105,17 +106,18 @@ var verbs = map[string]struct { durArg bool // the single argument is a duration allowExact bool }{ - "launch": {0, 1, false, false}, - "wait": {1, 2, false, true}, - "wait_gone": {1, 2, false, true}, - "tap": {1, 2, false, true}, - "tap_if": {1, 1, false, true}, - "tap_row": {1, 2, false, true}, - "type": {1, 1, false, false}, - "key": {1, 1, false, false}, - "back": {0, 0, false, false}, - "home": {0, 0, false, false}, - "sleep": {1, 1, true, false}, + "launch": {0, 1, false, false}, + "foreground": {0, 1, false, false}, + "wait": {1, 2, false, true}, + "wait_gone": {1, 2, false, true}, + "tap": {1, 2, false, true}, + "tap_if": {1, 1, false, true}, + "tap_row": {1, 2, false, true}, + "type": {1, 1, false, false}, + "key": {1, 1, false, false}, + "back": {0, 0, false, false}, + "home": {0, 0, false, false}, + "sleep": {1, 1, true, false}, } // keyNames are the key event names a recipe may use. diff --git a/internal/recipe/recipe_test.go b/internal/recipe/recipe_test.go index 41e4b1d..b821979 100644 --- a/internal/recipe/recipe_test.go +++ b/internal/recipe/recipe_test.go @@ -6,6 +6,8 @@ import ( "strings" "testing" "time" + + "gitea.lab.audasmedia.com.au/sam/artemis_control/internal/adb" ) const sample = `# a comment @@ -155,7 +157,17 @@ func TestSpotifyRecipeIsValid(t *testing.T) { t.Fatalf("step %d uses unknown verb %q", i+1, s.Verb) } } - // The recipe must wait for playback, not assume it. + // The recipe must confirm it is in Spotify before it trusts a text match, + // and it must wait for playback rather than assume it. + var sawForeground bool + for _, s := range rec.Steps { + if s.Verb == "foreground" { + sawForeground = true + } + } + if !sawForeground { + t.Fatal("spotify_play must pin the foreground app before matching text") + } last := rec.Steps[len(rec.Steps)-1] if last.Verb != "wait" || last.Args[0] != "Pause" { t.Fatalf("the last step must confirm playback, got %+v", last) @@ -192,3 +204,31 @@ func TestLoadAndList(t *testing.T) { t.Fatalf("Find unknown task: %v", err) } } + +func TestForegroundStep(t *testing.T) { + rec, err := Parse("app: com.spotify.music\nsteps:\n - launch\n - foreground 30s\n") + if err != nil { + t.Fatalf("Parse: %v", err) + } + fg := rec.Steps[1] + if fg.Verb != "foreground" || len(fg.Args) != 0 || fg.Timeout != 30*time.Second { + t.Fatalf("foreground step = %+v", fg) + } +} + +// TestStepDurationsAreRecorded guards the run log (FR-10): a step's duration +// must reach the caller, not be swallowed by the return value. +func TestStepDurationsAreRecorded(t *testing.T) { + rec := &Recipe{StepTimeout: time.Second} + step := Step{Verb: "sleep", Timeout: 50 * time.Millisecond} + sr := executeStep(adb.New(""), rec, step, 2, nil, 0, 0, time.Now().Add(time.Minute), func(string, ...any) {}) + if !sr.OK { + t.Fatalf("sleep step failed: %s", sr.Error) + } + if sr.Index != 3 { + t.Fatalf("index = %d, want 3", sr.Index) + } + if sr.Duration < 40*time.Millisecond || sr.DurationMS < 40 { + t.Fatalf("duration not recorded: %v (%d ms)", sr.Duration, sr.DurationMS) + } +} diff --git a/recipes/spotify_play.recipe b/recipes/spotify_play.recipe index 53eedf1..c8f979f 100644 --- a/recipes/spotify_play.recipe +++ b/recipes/spotify_play.recipe @@ -35,6 +35,7 @@ steps: # 1. Make sure Spotify is up. Launching an app that is already open is a # no-op, so this is safe to repeat. - launch + - foreground 30s - wait "Search" 30s # 2. Open the search tab. Spotify remembers the last tab, so a search screen