From 93aebb5019877042fc1834fd1075761f0c9089fe Mon Sep 17 00:00:00 2001
From: saphid <4596216+saphid@users.noreply.github.com>
Date: Sun, 27 Sep 2026 17:22:20 +1000
Subject: [PATCH] Fix the review's findings in the test harness

Headset smoke test: drop the paired key from authorized_keys with a
same-mode copy swapped in, so a failed write can't truncate it; check the
throwaway key with no ssh_config or agent; clean up idempotently (tracked
and leftover titles, their json files, Steam's shortcuts via steamos-delete,
and ~/devkit-utils if it wasn't there before), with a failed cleanup a
failed step; count a launch only with fresh evidence (the process, Steam's
log, or the known missing-runtime line), matching with [d]evkit-game so
pgrep doesn't find its own shell. The test programs sleep 10 s.

Fake Frame: log a launch before its reaper can look for it. e2e: kill a
pairing client's process group when a test ends; accept an aarch64 program
running under QEMU on x86 hosts.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
---
 docs/testing.md                               |  14 +-
 tests/e2e/test_pairing.py                     |  14 +-
 tests/e2e/test_titles.py                      |  11 +-
 .../usr/local/lib/fakeframe/fakesteam.py      |  57 +++----
 tests/smoke/frame_smoke.py                    | 141 ++++++++++++++----
 tests/smoke/tiny_programs.py                  |   2 +-
 6 files changed, 173 insertions(+), 66 deletions(-)

diff --git a/docs/testing.md b/docs/testing.md
index e70a4a7..c95ee75 100644
--- a/docs/testing.md
+++ b/docs/testing.md
@@ -111,9 +111,17 @@ scripts/frame-smoke.sh --pair    # also pairs a throwaway key: approve it in the
 
 It checks `properties.json` and the status, then installs, launches and
 removes three tiny titles built from bytes by `tests/smoke/tiny_programs.py`
-(an ARM64 and an x86-64 static Linux program that sleep for five seconds, and
-an x86-64 `.exe` that exits at once), keeping what Steam logged about each.
-Everything it installs is removed again, also after a failure. Results go to
+(an ARM64 and an x86-64 static Linux program that sleep for ten seconds, and
+an x86-64 `.exe` that exits at once). A launch passes only with fresh evidence:
+the ARM64 program running, the `.exe` started (its process or Steam's log),
+and the x86-64 program running or Steam logging that its runtime isn't
+installed, which is what the Frame does today. Steam's log lines about each
+title are kept.
+
+Everything it installs is removed again, also after a failure: the titles and
+their Steam shortcuts, a paired key, and `~/devkit-utils` if it wasn't there
+before (if it was, it stays, synced to this checkout as Frame Control always
+does). A cleanup that fails counts as a failed step. Results go to
 `tests/smoke/results/<time>-<BUILD_ID>.json` (not committed) with a summary on
 screen; it exits 0 when every step passed, 1 if one failed, 2 if the headset
 isn't reachable.
diff --git a/tests/e2e/test_pairing.py b/tests/e2e/test_pairing.py
index aaf1ecf..8c6aae9 100644
--- a/tests/e2e/test_pairing.py
+++ b/tests/e2e/test_pairing.py
@@ -2,6 +2,7 @@
 steamos-devkit-service on the fake Frame, and its password fallback."""
 import json
 import os
+import signal
 import stat
 import subprocess
 import sys
@@ -28,9 +29,20 @@ class Pairing(harness.FrameTestCase):
                 f.write(f'#!/bin/sh\necho {askpass}\n')
             os.chmod(script, stat.S_IRWXU)
             env.update(SSH_ASKPASS=script, SSH_ASKPASS_REQUIRE='force')
-        return subprocess.Popen([sys.executable, str(ROOT / 'ui' / 'frame_connect.py'), FAKE_HOST],
+        proc = subprocess.Popen([sys.executable, str(ROOT / 'ui' / 'frame_connect.py'), FAKE_HOST],
                                 stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
                                 text=True, env=env, start_new_session=True)
+        self.addCleanup(self.stop, proc)    # a failed test mustn't leave it pairing into the next one
+        return proc
+
+    @staticmethod
+    def stop(proc):
+        if proc.poll() is None:
+            try:
+                os.killpg(proc.pid, signal.SIGKILL)     # frame_connect.py and its ssh children
+            except OSError:
+                pass
+        proc.communicate()
 
     def finish(self, proc, timeout=120):
         out, _ = proc.communicate(timeout=timeout)
diff --git a/tests/e2e/test_titles.py b/tests/e2e/test_titles.py
index c07bc0b..82d0afb 100644
--- a/tests/e2e/test_titles.py
+++ b/tests/e2e/test_titles.py
@@ -18,8 +18,9 @@ from harness import HOME, ROOT, ctl, exists, install_title, launches, ok, ssh, s
 harness.require()
 
 GAMES = f'{HOME}/devkit-game'
-# The host container shares the fake Frame's kernel, so a native program of this
-# machine's architecture really runs there; the other one fails to exec.
+# The host container shares the fake Frame's kernel, so a program of this machine's
+# architecture really runs there. The other one fails to exec, unless QEMU is
+# registered with binfmt_misc, which runs it emulated.
 NATIVE = {'aarch64': 'arm64', 'arm64': 'arm64', 'x86_64': 'x86_64'}.get(platform.machine())
 
 
@@ -74,10 +75,8 @@ class Titles(harness.FrameTestCase):
         if NATIVE == 'arm64':
             self.assertIsNotNone(run['pid'], run)
             done = wait_for(lambda: launches('devkit')[-1].get('exit') is not None and launches('devkit')[-1],
-                            20, 'the arm64 test program to exit')
+                            30, 'the arm64 test program to exit')
             self.assertEqual(done['exit'], 0)
-        else:
-            self.assertIn('Exec format error', run.get('exec_error', ''), run)
 
     def test_single_exe_upload_launch_and_remove(self):
         exe = tiny_programs.write(self.tmp, 'exe')
@@ -131,7 +130,7 @@ class Titles(harness.FrameTestCase):
         self.assertTrue(run['started'])
         if NATIVE == 'x86_64':
             done = wait_for(lambda: launches('devkit')[-1].get('exit') is not None and launches('devkit')[-1],
-                            20, 'the x86-64 test program to exit')
+                            30, 'the x86-64 test program to exit')
             self.assertEqual(done['exit'], 0)
 
     def test_reinstall_with_another_runtime_keeps_one_shortcut(self):
diff --git a/tests/fakeframe/rootfs/usr/local/lib/fakeframe/fakesteam.py b/tests/fakeframe/rootfs/usr/local/lib/fakeframe/fakesteam.py
index c76b586..c7adcc0 100755
--- a/tests/fakeframe/rootfs/usr/local/lib/fakeframe/fakesteam.py
+++ b/tests/fakeframe/rootfs/usr/local/lib/fakeframe/fakesteam.py
@@ -220,9 +220,8 @@ def cmd_run_game(q):
         # _v2-entry-point prefix, even though Steam recorded the mapping
         # (docs/sideloading.md, 2026-09-26). So does the fake, for real.
         record.update(started=True, command=full)
-        record.update(execute(full, game['directory'], {**game.get('env', {})}, f"devkit-{gameid}"))
-    with fs.update() as state:
-        state['launches'].append(record)
+        record['run'] = (full, game['directory'], {**game.get('env', {})}, f'devkit-{gameid}')
+    add_launch(record)
     console(record['message'] if not record['started'] else f'devkit run-game: started devkit game "{gameid}"')
     respond(q['response'], text='OK\n')   # guess: steam-devkit-rpc only checks that it arrives
 
@@ -232,26 +231,31 @@ DEVKIT = {'approve-ssh-key': cmd_approve_ssh_key, 'create-shortcut': cmd_create_
           'run-game': cmd_run_game}
 
 
-def execute(path, cwd, env, label):
-    """Start a program the way Steam would, and note its pid and exit status."""
-    os.makedirs(GAME_LOGS, exist_ok=True)
-    log = open(f'{GAME_LOGS}/{label}.log', 'ab')
-    try:
-        proc = subprocess.Popen([path], cwd=cwd if os.path.isdir(cwd) else HOME, env={**os.environ, **env},
-                                stdin=subprocess.DEVNULL, stdout=log, stderr=log, start_new_session=True)
-    except OSError as e:
-        return {'pid': None, 'exec_error': str(e)}
-    finally:
-        log.close()
-
-    def reap():
-        code = proc.wait()
-        with fs.update() as state:
-            for r in state['launches']:
-                if r.get('pid') == proc.pid and 'exit' not in r:
-                    r['exit'] = code
-    threading.Thread(target=reap, daemon=True).start()
-    return {'pid': proc.pid}
+def add_launch(record):
+    """Log a launch in the state. record['run'] = (path, cwd, env, log name) also starts the
+    program the way Steam would, and notes its pid and, once it ends, its exit status."""
+    run, proc = record.pop('run', None), None
+    if run:
+        path, cwd, env, label = run
+        os.makedirs(GAME_LOGS, exist_ok=True)
+        with open(f'{GAME_LOGS}/{label}.log', 'ab') as log:
+            try:
+                proc = subprocess.Popen([path], cwd=cwd if os.path.isdir(cwd) else HOME, env={**os.environ, **env},
+                                        stdin=subprocess.DEVNULL, stdout=log, stderr=log, start_new_session=True)
+                record['pid'] = proc.pid
+            except OSError as e:
+                record.update(pid=None, exec_error=str(e))
+    with fs.update() as state:              # before the reaper looks for it, however fast the program is
+        record['n'] = len(state['launches'])
+        state['launches'].append(record)
+    if proc:
+        def reap():
+            code = proc.wait()
+            with fs.update() as state:
+                mine = state['launches'][record['n']:record['n'] + 1]
+                if mine and mine[0].get('pid') == proc.pid:   # not a reset's fresh list
+                    mine[0]['exit'] = code
+        threading.Thread(target=reap, daemon=True).start()
 
 
 # ---- steam:// URLs from the `steam` wrapper ----------------------------------
@@ -271,15 +275,14 @@ def url_rungameid(gid):
             # STEAM_COMPAT_SHADER_PATH (docs/apks.md, 2026-09-25).
             env = {'SteamAppId': str(appid), 'SteamGameId': str(gid),
                    'STEAM_FOSSILIZE_DUMP_PATH': f'{fs.STEAM_ROOT}/steamapps/shadercache/{appid}/fozpipelinesv6'}
-            record.update(appid=appid, started=True, command=sc['exe'])
-            record.update(execute(sc['exe'], sc.get('start_dir') or HOME, env, f'shortcut-{appid}'))
+            record.update(appid=appid, started=True, command=sc['exe'],
+                          run=(sc['exe'], sc.get('start_dir') or HOME, env, f'shortcut-{appid}'))
     else:
         with fs.update() as state:
             app = next((a for a in state['steam']['apps'] if a['appid'] == gid), None)
         record.update(appid=gid, started=bool(app and app['installed']),
                       message=None if app and app['installed'] else 'not installed')
-    with fs.update() as state:
-        state['launches'].append(record)
+    add_launch(record)
 
 
 def url_install(appid):
diff --git a/tests/smoke/frame_smoke.py b/tests/smoke/frame_smoke.py
index 6cad662..3f16365 100644
--- a/tests/smoke/frame_smoke.py
+++ b/tests/smoke/frame_smoke.py
@@ -13,10 +13,14 @@ BUILD_ID in tests/smoke/results/<time>-<BUILD_ID>.json (git-ignored):
   (someone has to open Settings > Developer > Pair new host and approve it in
   the headset), checked, and taken out of authorized_keys again.
 
-Everything it installs is removed again, also when a step fails. Titles are
-named fc_smoke_*; leftovers from an interrupted run are removed first.
+Everything it installs is removed again, also when a step fails: the titles
+(named fc_smoke_*, and leftovers of an interrupted run first), their Steam
+shortcuts, the paired key, and ~/devkit-utils if it wasn't there before (if it
+was, it stays, synced to this checkout as Frame Control always does). A
+cleanup that fails is a failed step.
 
 Usage: python3 tests/smoke/frame_smoke.py [--pair]     (or scripts/frame-smoke.sh)
+Env:   FRAME_ALIAS (default frame), FRAME_SMOKE_RESULTS (default tests/smoke/results)
 Exit status: 0 all passed, 1 a step failed, 2 the headset isn't reachable.
 """
 import argparse
@@ -42,7 +46,7 @@ import frame_connect  # noqa: E402
 import frame_titles  # noqa: E402
 import tiny_programs  # noqa: E402
 
-RESULTS = HERE / 'results'
+RESULTS = Path(os.environ.get('FRAME_SMOKE_RESULTS') or HERE / 'results')
 PREFIX = 'fc_smoke_'
 # Earlier runs named titles fc-smoke*, which Steam refused to register but left on disk.
 OLD_PREFIX = 'fc-smoke'
@@ -110,12 +114,19 @@ def status(build):
     return {k: s.get(k) for k in ('os', 'battery', 'power', 'temp', 'services', 'volume')}
 
 
-def steam_log_lines(gid):
-    """What Steam logged about a title (log folder layout not verified; best effort)."""
-    out = ssh(f"grep -rsh -- {shlex.quote(gid)} ~/.local/share/Steam/logs/ 2>/dev/null | tail -n 12 || true")
+def steam_log_lines(*patterns):
+    """Steam log lines holding any of the fixed strings (which files: not verified, so all)."""
+    pats = ' '.join(f'-e {shlex.quote(p)}' for p in patterns)
+    out = ssh(f"grep -rshF {pats} ~/.local/share/Steam/logs/ 2>/dev/null || true")
     return out.strip().splitlines()
 
 
+# What a launch must show, per kind. The x86-64 runtime isn't installed on the
+# Frame, so Steam acknowledges that launch and logs "... is not installed"
+# instead (docs/sideloading.md, 2026-09-26): recorded, not a failure.
+EXPECTED = {'arm64': ('running',), 'x86_64': ('running', 'runtime missing'), 'exe': ('running', 'started')}
+
+
 def title_cycle(smoke, kind, folder):
     path = tiny_programs.write(folder, kind)
     gid = PREFIX + kind                 # named outright: the file names would share one id
@@ -129,10 +140,32 @@ def title_cycle(smoke, kind, folder):
         return {'id': gid, 'runtime': meta['runtime'], 'steam': meta.get('steam')}
 
     def launch():
+        mine = (f'devkit-game/{gid}', f'"{gid}"')
+        # The missing-runtime line names Steam's app id, which we don't know, so
+        # any new one counts; nothing else is launching during the test.
+        seen, seen_missing = len(steam_log_lines(*mine)), len(steam_log_lines('but is not installed'))
         frame_titles.launch(gid)       # raises unless Steam confirmed it
-        time.sleep(4)
-        procs = ssh(f"pgrep -af -- {shlex.quote('devkit-game/' + gid)} || true").strip()
-        return {'processes': procs.splitlines(), 'steam_log': steam_log_lines(gid)}
+        outcome, procs, new = 'no evidence', [], []
+        deadline = time.monotonic() + 12
+        while time.monotonic() < deadline:
+            # [d]: the pattern mustn't match the shell running pgrep, whose command line holds it.
+            procs = ssh(f"pgrep -af -- '[d]evkit-game/{gid}/' || true").strip().splitlines()
+            new = steam_log_lines(*mine)[seen:]
+            missing = steam_log_lines('but is not installed')[seen_missing:]
+            if procs:
+                outcome = 'running'
+            elif missing:
+                outcome, new = 'runtime missing', new + missing
+            elif any('started devkit game' in line or 'chdir' in line for line in new):
+                outcome = 'started'
+            if outcome != 'no evidence':
+                break
+            time.sleep(0.5)
+        detail = {'outcome': outcome, 'processes': procs, 'steam_log': new[-12:]}
+        if outcome not in EXPECTED[kind]:
+            raise AssertionError(f"Steam acknowledged the launch, but {outcome} (expected "
+                                 f"{' or '.join(EXPECTED[kind])}): {json.dumps(detail)[:400]}")
+        return detail
 
     def remove():
         frame_titles.remove(gid)
@@ -150,17 +183,60 @@ def title_cycle(smoke, kind, folder):
 
 
 def cleanup(smoke):
-    """Remove fc-smoke-* titles: this run's, and any an interrupted run left."""
+    """Remove this run's titles and any fc_smoke_* an interrupted run left: the folder,
+    its json files, and (through steamos-delete) Steam's shortcut. Raises if anything stays."""
+    problems = []
     try:
         ids = {t['id'] for t in frame_titles.list_titles() if t['id'].startswith((PREFIX, OLD_PREFIX))}
-    except frame_android.FrameError:
-        ids = set(smoke.installed)      # can't list: try what this run installed
+    except frame_android.FrameError as e:
+        ids = set()
+        problems.append(f'could not list titles: {e}')
+    ids |= smoke.installed
     for gid in sorted(ids):
         try:
-            frame_titles.remove(gid)
+            if ssh(f'test -d ~/devkit-game/{gid} && echo yes || true').strip() == 'yes':
+                frame_titles.remove(gid)
+            # Also when remove() stopped part-way, or only the json files are left.
+            ssh(f"rm -f {' '.join(f'~/devkit-game/{gid}-{k}.json' for k in ('argv', 'env', 'settings', 'framecontrol'))}")
+            if ssh(f'ls -d ~/devkit-game/{gid} ~/devkit-game/{gid}-*.json 2>/dev/null || true').strip():
+                problems.append(f'{gid} is still on the Frame')
+            else:
+                smoke.installed.discard(gid)
         except frame_android.FrameError as e:
-            print(f'  could not remove {gid}: {e}')
-    return sorted(ids)
+            problems.append(f'{gid}: {e}')
+    if ids:
+        # steamos-delete with no title only syncs Steam's shortcuts with ~/devkit-game.
+        try:
+            ssh('python3 ~/devkit-utils/steamos-delete 2>&1 || true', timeout=120)
+        except frame_android.FrameError as e:
+            problems.append(f'syncing Steam shortcuts: {e}')
+    if problems:
+        raise AssertionError('; '.join(problems))
+    return {'removed': sorted(ids)}
+
+
+# Runs on the Frame: drop the lines holding one key from authorized_keys, writing a
+# copy with the same mode and swapping it in, so a failure can't truncate the file.
+DROP_KEY = r"""
+import os, sys, tempfile
+path = os.path.expanduser('~/.ssh/authorized_keys')
+with open(path) as f:
+    lines = f.readlines()
+keep = [line for line in lines if sys.argv[1] not in line]
+if len(keep) < len(lines):
+    fd, tmp = tempfile.mkstemp(dir=os.path.dirname(path), prefix='.authorized_keys.')
+    try:
+        with os.fdopen(fd, 'w') as f:
+            f.writelines(keep)
+            f.flush()
+            os.fsync(f.fileno())
+        os.chmod(tmp, os.stat(path).st_mode & 0o777)
+        os.replace(tmp, path)
+    except BaseException:
+        os.unlink(tmp)
+        raise
+print(len(lines) - len(keep))
+"""
 
 
 def pair(folder):
@@ -169,8 +245,8 @@ def pair(folder):
     subprocess.run(['ssh-keygen', '-q', '-t', 'rsa', '-b', '3072', '-N', '', '-C', 'frame-control-smoke',
                     '-f', key], check=True)
     pub = Path(key + '.pub').read_text()
-    host = ssh_config('hostname') or ALIAS
-    user = ssh_config('user')
+    host, user, port = ssh_config('hostname') or ALIAS, ssh_config('user'), ssh_config('port') or '22'
+    known = ssh_config('userknownhostsfile') or '~/.ssh/known_hosts'
     comment = frame_connect.key_comment(platform.node()).replace('frame-control@', 'frame-control-smoke@')
     print('    In the headset: Steam Settings > Developer > Pair new host, then approve '
           f'"{comment}" (waits up to {frame_connect.PAIRING_MODE_WAIT} s)', flush=True)
@@ -179,16 +255,21 @@ def pair(folder):
         reason = frame_connect.devkit_pair(host, pub, comment)
         if reason:
             raise AssertionError(reason)
-        r = subprocess.run(['ssh', '-o', 'BatchMode=yes', '-o', 'IdentitiesOnly=yes', '-o', 'ConnectTimeout=8',
-                            '-o', 'ControlPath=none', '-i', key, f'{user}@{host}', 'true'], capture_output=True, text=True)
+        # Only this key: no ssh_config (whose IdentityFile lines IdentitiesOnly would
+        # still offer), no agent, no passwords.
+        r = subprocess.run(['ssh', '-F', '/dev/null', '-o', 'BatchMode=yes', '-o', 'IdentitiesOnly=yes',
+                            '-o', 'IdentityAgent=none', '-o', 'PasswordAuthentication=no',
+                            '-o', 'KbdInteractiveAuthentication=no', '-o', 'ConnectTimeout=8',
+                            '-o', 'StrictHostKeyChecking=yes', '-o', f'UserKnownHostsFile={known}',
+                            '-p', port, '-i', key, f'{user}@{host}', 'true'], capture_output=True, text=True)
         if r.returncode != 0:
             raise AssertionError(f'paired, but the key does not log in: {r.stderr.strip()}')
         return {'comment': comment}
     finally:
-        f = '~/.ssh/authorized_keys'
-        ssh(f'grep -vF {b64} {f} > {f}.smoke; cat {f}.smoke > {f}; rm -f {f}.smoke')
-        if b64 in ssh(f'cat {f}'):
+        dropped = frame_android.ssh(f'python3 - {shlex.quote(b64)}', input=DROP_KEY).strip()
+        if b64 in ssh('cat ~/.ssh/authorized_keys'):
             raise AssertionError('the smoke key is still in authorized_keys')
+        print(f'    removed the smoke key from authorized_keys ({dropped} line(s))', flush=True)
 
 
 def main():
@@ -207,9 +288,10 @@ def main():
     release = os_release()
     build = release['BUILD_ID'] or 'unknown'
     print(f'==> Frame smoke test on {ALIAS}: SteamOS {release["VERSION_ID"]} ({release["VARIANT_ID"]}), build {build}')
-    left = cleanup(smoke)
-    if left:
-        print(f'  removed leftovers from an earlier run: {", ".join(left)}')
+    # Frame Control copies Valve's devkit tools to ~/devkit-utils on the first install
+    # (as Valve's client does). If they weren't there before, they go again at the end.
+    had_utils = ssh('test -d ~/devkit-utils && echo yes || true').strip() == 'yes'
+    smoke.step('cleanup: leftovers from earlier runs', cleanup, smoke)
     folder = tempfile.mkdtemp(prefix='frame-smoke-')
     try:
         smoke.step('devkit service: properties.json', properties)
@@ -219,15 +301,18 @@ def main():
         if args.pair:
             smoke.step('devkit pairing (throwaway key)', pair, folder)
     finally:
-        removed = cleanup(smoke)
+        smoke.step('cleanup: everything this run installed', cleanup, smoke)
+        if not had_utils:
+            smoke.step('cleanup: ~/devkit-utils (not there before)', ssh,
+                       'rm -rf ~/devkit-utils ~/.devkit-utils.frame-control && echo removed')
         subprocess.run(['rm', '-rf', folder])
     passed = sum(s['ok'] for s in smoke.steps)
     RESULTS.mkdir(exist_ok=True)
     out = RESULTS / f"{started.replace(':', '')}-{build}.json"
     out.write_text(json.dumps({'started': started, 'alias': ALIAS, 'os_release': release, 'paired': args.pair,
                                'passed': passed, 'failed': len(smoke.steps) - passed,
-                               'cleanup_removed': removed, 'steps': smoke.steps}, indent=1))
-    print(f'==> {passed}/{len(smoke.steps)} steps passed on build {build}; results in {out.relative_to(ROOT)}')
+                               'devkit_utils_there_before': had_utils, 'steps': smoke.steps}, indent=1))
+    print(f'==> {passed}/{len(smoke.steps)} steps passed on build {build}; results in {out}')
     return 0 if passed == len(smoke.steps) else 1
 
 
diff --git a/tests/smoke/tiny_programs.py b/tests/smoke/tiny_programs.py
index c598b15..4323d32 100644
--- a/tests/smoke/tiny_programs.py
+++ b/tests/smoke/tiny_programs.py
@@ -11,7 +11,7 @@ smoke test and the fake Frame's e2e tests install. Python stdlib only.
 """
 import struct
 
-SLEEP_SECONDS = 5
+SLEEP_SECONDS = 10
 BASE = 0x400000