droid-ash
diff --git a/‎.github/workflows/ci.yml‎
Lines changed: 8 additions & 6 deletions b/‎.github/workflows/ci.yml‎
Lines changed: 8 additions & 6 deletions
diff --git a/‎docs/memory/ci/index.md‎
Lines changed: 1 addition & 1 deletion b/‎docs/memory/ci/index.md‎
Lines changed: 1 addition & 1 deletion
diff --git a/‎docs/memory/ci/pr-quality-gate.md‎
Lines changed: 3 additions & 3 deletions b/‎docs/memory/ci/pr-quality-gate.md‎
Lines changed: 3 additions & 3 deletions
diff --git a/‎docs/memory/cli/index.md‎
Lines changed: 1 addition & 1 deletion b/‎docs/memory/cli/index.md‎
Lines changed: 1 addition & 1 deletion
diff --git a/‎docs/memory/cli/session-runner.md‎
Lines changed: 15 additions & 1 deletion b/‎docs/memory/cli/session-runner.md‎
Lines changed: 15 additions & 1 deletion
@@ -44,12 +44,14 @@ jobs:
         run: npm run build --workspaces
 
       # Typecheck every workspace explicitly (tsc --noEmit -p tsconfig.json
-      # per package, fanned out WITHOUT --if-present). This is the one stage
-      # that typechecks report-web at all (its tsup/vite build never runs
-      # tsc); for the five tsc-building packages it is redundant with their
-      # build, deliberately — so a future build-tool switch cannot silently
-      # drop the check. It runs after build because dependent
-      # packages resolve @finalrun/* types from built dist/ declarations.
+      # per TypeScript package, fanned out WITHOUT --if-present;
+      # local-runtime's typecheck script is an explicit no-op — it has no
+      # TypeScript to check). This is the one stage that typechecks
+      # report-web at all (its tsup/vite build never runs tsc); for the five
+      # tsc-building packages it is redundant with their build, deliberately
+      # — so a future build-tool switch cannot silently drop the check. It
+      # runs after build because dependent packages resolve @finalrun/*
+      # types from built dist/ declarations.
       - name: Typecheck all workspaces
         run: npm run typecheck
 
 
@@ -7,4 +7,4 @@ description: "PR quality gate (`npm ci` → build → typecheck → test → lin
 
 | File | Description |
 |------|-------------|
-| [pr-quality-gate](pr-quality-gate.md) | PR CI gate runs `npm ci` → build → typecheck → test → lint via .github/workflows/ci.yml — every workspace typechecks via `tsc --noEmit`, no workspace step uses `--if-present` (a lost script fails loudly; local-runtime holds explicit no-ops); code-quality rules are ESLint warnings; tests use strict explicit-discovery runners (Node 20.19 has no `node --test` glob); refactors batch a package's tested oversized functions, characterize untested ones first, and prove equivalence differentially. |
+| [pr-quality-gate](pr-quality-gate.md) | PR CI gate runs `npm ci` → build → typecheck → test → lint via .github/workflows/ci.yml — every TypeScript workspace typechecks via `tsc --noEmit`, no fan-out uses `--if-present` (a lost script fails loudly; local-runtime holds explicit no-ops); code-quality rules are ESLint warnings; tests use strict explicit-discovery runners (Node 20.19 has no `node --test` glob); refactors batch a package's tested oversized functions, characterize untested ones first, and prove equivalence differentially. |
@@ -1,6 +1,6 @@
 ---
 type: memory
-description: "PR CI gate runs `npm ci` → build → typecheck → test → lint via .github/workflows/ci.yml — every workspace typechecks via `tsc --noEmit`, no workspace step uses `--if-present` (a lost script fails loudly; local-runtime holds explicit no-ops); code-quality rules are ESLint warnings; tests use strict explicit-discovery runners (Node 20.19 has no `node --test` glob); refactors batch a package's tested oversized functions, characterize untested ones first, and prove equivalence differentially."
+description: "PR CI gate runs `npm ci` → build → typecheck → test → lint via .github/workflows/ci.yml — every TypeScript workspace typechecks via `tsc --noEmit`, no fan-out uses `--if-present` (a lost script fails loudly; local-runtime holds explicit no-ops); code-quality rules are ESLint warnings; tests use strict explicit-discovery runners (Node 20.19 has no `node --test` glob); refactors batch a package's tested oversized functions, characterize untested ones first, and prove equivalence differentially."
 ---
 # PR Quality Gate (ci)
 
@@ -120,9 +120,9 @@ This costs nothing at the tooling layer and requires no runner change: every run
 *Introduced by*: 260726-vzi3-split-testexecutor-runtests
 
 ### `finally` scope follows the acquisition, not the phase split
-**Decision**: A resource is released by a `finally` whose `try` opens immediately after the acquisition, and any guard that can throw between acquisition and use sits *inside* that `try`, never above it. The rule binds **every** acquisition in a function independently, not only the most recent: N resources means N `try`/`finally` scopes nested in acquisition order. A single `finally` placed at the latest acquisition cannot release the earlier ones — it is reachable only from its own `try`, so every throw between an earlier acquisition and that `try` orphans what the earlier one took. Registrations on process-global state — `Logger` sinks, signal listeners — are torn down in the function's outermost `finally`. `testRunner.runTests` and `submitRun` (`packages/cloud-core/src/submit.ts`) are the worked examples: `runTests` releases the device session in an inner `finally` that the post-preparation abort check sits inside, while sink removal and SIGINT-listener removal sit in the outer `finally` covering every exit path; `submitRun` opens its outer `try` right after `resolveAppMode` for the temp `.app.zip` and its inner `try` right after `writeSpecZip` for the spec zip, which also makes release order structural — innermost first — instead of a property of statement order (260726-rpx7-fix-submit-appzip-cleanup-leak). **Where the releasing `finally` reads a state ledger rather than a lexical scope, the rule binds the *registration* too: the write that records the acquisition comes before any fallible statement that follows it — including a log line.** `executeTestOnSession`'s recording and log-capture phases are the worked case ([/cli/session-runner.md](/cli/session-runner.md)).
+**Decision**: A resource is released by a scope that **encloses its acquisition point**, and any guard that can throw between acquisition and use sits *inside* that scope, never above it. Where the acquisition is a statement, the `try` opens immediately after it. Where the acquisition happens *inside* a call, the scope opens **before** the call: `zip.writeZip(path)` creates the file mid-call — adm-zip's `writeFileTo` runs `openSync(path,'w')` → `writeSync` → `closeSync` → `chmodSync` — so a scope opening after it can never see a file the call itself created and abandoned ([/cloud-core/submit-pipeline.md](/cloud-core/submit-pipeline.md)). The scope's *form* follows ownership: `finally` where the function releases on every exit, `catch`-and-rethrow where the success path hands the resource to the caller. The rule binds **every** acquisition in a function independently, not only the most recent: N resources means N `try`/`finally` scopes nested in acquisition order. A single `finally` placed at the latest acquisition cannot release the earlier ones — it is reachable only from its own `try`, so every throw between an earlier acquisition and that `try` orphans what the earlier one took. Registrations on process-global state — `Logger` sinks, signal listeners — are torn down in the function's outermost `finally`. `testRunner.runTests` and `submitRun` (`packages/cloud-core/src/submit.ts`) are the worked examples: `runTests` releases the device session in an inner `finally` that the post-preparation abort check sits inside, while sink removal and SIGINT-listener removal sit in the outer `finally` covering every exit path; `submitRun` opens its outer `try` right after `resolveAppMode` for the temp `.app.zip` and its inner `try` right after `writeSpecZip` for the spec zip, which also makes release order structural — innermost first — instead of a property of statement order (260726-rpx7-fix-submit-appzip-cleanup-leak). **Where the releasing `finally` reads a state ledger rather than a lexical scope, the rule binds the *registration* too: the write that records the acquisition comes before any fallible statement that follows it — including a log line.** `executeTestOnSession`'s recording and log-capture phases are the worked case ([/cli/session-runner.md](/cli/session-runner.md)).
 **Why**: A guard above the releasing `try` strands whatever was already acquired — for `runTests`, a prepared device session, meaning emulator/simulator state, driver processes and ports, on the SIGINT-during-preparation path users actually hit. A leaked global registration is worse than a per-call leak because it accumulates across calls in one process: a sink left on the module-level `Logger` by an early exit keeps receiving every later run's entries, including across tests in a single suite run. The multi-acquisition case is stated per-acquisition because it is the variant that reads as correct: a lone `finally` enumerating every release names all the right resources, so a reader checks *what* it releases and never *from where it is reachable*. The registration-ordering facet reads as correct for the same reason and is the one the rule keeps losing: a phase that starts a capture, logs it, then records it looks like three ordered steps rather than a window, yet the log call is fallible — the CLI's runner-log sink is an unguarded `fs.appendFileSync` — and a throw between the two leaves the resource acquired on the device and invisible to the `finally`, so the run reports success while a `logcat` process is orphaned. Neither leak is visible to a green suite, so a phase split MUST come with explicit error- and abort-path tests — happy-path coverage alone lets a stranded resource survive a full-file restructuring unnoticed.
-**Rejected**: (a) a single `finally` covering several acquisitions — whether it inlines the releases or delegates to one combined cleanup helper — every resource then shares that one scope's reachability, so anything acquired before the scope opens goes unreleased on every exit that precedes it; (b) repeating the release in a `catch`-and-rethrow at each guard — restates cleanup at every throw site and drifts as throw sites are added; (c) guarding the individual fallible statement that sits between an acquisition and its registration — it closes one window and leaves the next inserted statement equally lethal, whereas ordering the registration first removes the window itself.
+**Rejected**: (a) a single `finally` covering several acquisitions — whether it inlines the releases or delegates to one combined cleanup helper — every resource then shares that one scope's reachability, so anything acquired before the scope opens goes unreleased on every exit that precedes it; (b) repeating the release in a `catch`-and-rethrow at each guard — restates cleanup at every throw site and drifts as throw sites are added; (c) guarding the individual fallible statement that sits between an acquisition and its registration — it closes one window and leaves the next inserted statement equally lethal, whereas ordering the registration first removes the window itself; (d) reading "opens immediately after the acquisition" as "after the acquiring *statement*" — a call that acquires internally has its acquisition point before its own return, so the literal reading leaves open exactly the window the rule exists to close, and the reading is self-confirming because the resource is invisible from where the scope was placed.
 *Introduced by*: 260726-gohy-fix-runtests-session-cleanup-leak
 
 ### Characterization tests pin an untested function before it is restructured
 
@@ -8,4 +8,4 @@ description: "CLI orchestration, report writing, HTML generation"
 | File | Description |
 |------|-------------|
 | [report-writer](report-writer.md) | ReportWriter (packages/cli/src/reportWriter.ts) — the run directory it emits (runner.log, input/, tests/<id>/, summary.json, run.json), the secret-redaction contract every write path crosses, device-log copy-then-redact, first-failure precedence, and the emitted-JSON key-omission contract that makes an absent optional field part of the schema |
-| [session-runner](session-runner.md) | sessionRunner.ts — prepareTestSession/executeTestOnSession/runGoal: the per-call ExecutionSessionState every phase records its acquisition into, the single finally that releases whatever is still held, the recording and device-log capture lifecycles, and the CLI's two sanctioned dependency seams (TestSessionDeps and testRunnerDependencies) |
+| [session-runner](session-runner.md) | sessionRunner.ts — prepareTestSession/executeTestOnSession/runGoal: the per-call ExecutionSessionState every phase records its acquisition into, the single finally that releases whatever is still held, the recording and device-log capture lifecycles, the guarded (never `!`-asserted) install-override preconditions and model-owned platform derivation, and the CLI's two sanctioned dependency seams (TestSessionDeps and testRunnerDependencies) |
@@ -1,6 +1,6 @@
 ---
 type: memory
-description: "sessionRunner.ts — prepareTestSession/executeTestOnSession/runGoal: the per-call ExecutionSessionState every phase records its acquisition into, the single finally that releases whatever is still held, the recording and device-log capture lifecycles, and the CLI's two sanctioned dependency seams (TestSessionDeps and testRunnerDependencies)"
+description: "sessionRunner.ts — prepareTestSession/executeTestOnSession/runGoal: the per-call ExecutionSessionState every phase records its acquisition into, the single finally that releases whatever is still held, the recording and device-log capture lifecycles, the guarded (never `!`-asserted) install-override preconditions and model-owned platform derivation, and the CLI's two sanctioned dependency seams (TestSessionDeps and testRunnerDependencies)"
 ---
 # Session Runner (cli)
 
@@ -13,6 +13,8 @@ description: "sessionRunner.ts — prepareTestSession/executeTestOnSession/runGo
 - `cleanup` is a closure over a `cleanedUp` flag, so calling it twice runs `deviceNode.cleanup()` once.
 - The whole body sits in a `try`/`catch` that runs `cleanup()` — itself guarded, its own failure only warns — and rethrows, so a failure at any phase after `deviceNode.init` never leaks the device.
 - A `startTarget` diagnostic, an entry that never becomes runnable, and an inventory with nothing usable all surface as `DevicePreparationError`, which carries the `DeviceInventoryDiagnostic[]` for the caller to print. The no-usable-target message is scoped to the requested platform when one was given.
+- `establishDeviceSession` takes the session `platform` from `deviceInfo.getPlatform()`. The `DeviceInfo` model owns the `isAndroid → PLATFORM_ANDROID | PLATFORM_IOS` mapping and is the single place it is written; `sessionRunner` still imports `PLATFORM_ANDROID` for its own platform comparisons elsewhere in the file.
+- `installAppOverride`'s Android branch checks both of its preconditions before calling `installAndroidApp`, each with a descriptive `Error` in the same voice: `Android device serial is required to install an app override.` and `adb path is required to install an Android app override.` `AdbPath` is `Awaited<ReturnType<CliFilePathUtil['getADBPath']>>` and therefore nullable; it is passed unasserted everywhere in the file, including to `detectInventory`, which accepts the nullable type.
 
 ## executeTestOnSession
 
@@ -39,3 +41,15 @@ description: "sessionRunner.ts — prepareTestSession/executeTestOnSession/runGo
 **Why**: The two seams look nothing alike at a call site, so grepping for one shape finds only that one. `goalRunner.test.ts` exercises `prepareTestSession`/`executeTestOnSession`/`runGoal` entirely through `TestSessionDeps`, yet a search for the `testRunnerDependencies` pattern in `sessionRunner.ts` returns nothing and reads as "no seam exists" — a false negative that has already produced a scoping decision (a proposal to abandon characterizing `sessionRunner`) based on a seam that was there all along. Naming both is what makes the check cheap enough to actually perform.
 **Rejected**: (a) unifying the two onto one shape — a refactor of working, tested infrastructure whose only benefit is making a grep easier, and the per-call parameter and the module-level object genuinely suit different call graphs (`runTests` is a top-level entry point with no caller to thread a parameter from); (b) treating "I found no seam" as sufficient evidence — the constitution forbids *adding* a seam for tests, so a false negative silently converts a tractable change into a deferred one.
 *Introduced by*: 260727-18tg-characterize-refactor-cli-giants
+
+### A nullable precondition is guarded, never asserted away
+**Decision**: Every nullable input the Android install path requires is checked with an explicit `if (!x) throw new Error(...)` naming the missing precondition. A non-null assertion (`!`) is not used to satisfy a callee's non-null parameter. The guards sit together, immediately before the call, and read in one voice.
+**Why**: `adbPath` is genuinely nullable — `getADBPath()` resolves to `null` when adb is not on the machine — so an assertion there silences a reachable case rather than an impossible one, and `null` then travels into `installAndroidApp`, where the resulting failure describes a broken adb invocation instead of a missing tool. A guard converts that into a message naming what is missing, at the point the requirement exists, and it makes the two preconditions of the same operation legible as a pair rather than one checked and one asserted.
+**Rejected**: (a) skipping the install silently when `adbPath` is null — preparation then reports success for an override that was never installed, which is the failure mode hardest to diagnose from the run output; (b) widening `installAndroidApp` to accept a nullable path — moves the check into a package with less context about why the path is missing and leaves every other caller to re-handle it; (c) leaving the `!` — every other use of `adbPath` in the file passes it unasserted, so the assertion is the file's only claim that the value is non-null, contradicted by the type it is asserting away.
+*Introduced by*: 260728-o3me-fix-deferred-error-path-defects
+
+### A fix's test kind follows from whether it changes behaviour, and the kind is proved by reverting
+**Decision**: Each fix carries a test whose *kind* is determined by whether the fix changes observable behaviour. A behaviour-changing fix takes a **regression** test that fails on the pre-fix source and passes after; a behaviour-preserving fix takes a **characterization** test that passes on the unmodified source and still passes after. The two sessionRunner preconditions are the worked pair: the platform derivation is characterization — `PLATFORM_IOS` is the literal `'ios'` (`packages/common/src/constants.ts`), so `deviceInfo.getPlatform()` and the local ternary form are the same expression — while the `adbPath` guard beside it is regression. Either claim is established the same way and per fix, not per suite: restore the pre-fix source with the new test kept, rebuild, and observe which tests fail. With both sessionRunner fixes reverted, the platform test passes and only the `adbPath` test fails.
+**Why**: The two kinds prove opposite things, so conflating them produces a suite that is worthless in a specific way. Demanding a fails-before test on a behaviour-preserving fix forces the author to fabricate a difference, and the resulting test pins the new code's shape rather than the contract. Accepting a passes-before test on a behaviour-changing fix leaves nothing establishing that the bug existed, which is the entire content of a regression test. The revert is the only evidence for either claim — "the test passes now" is equally true of a test that constrains nothing — and it stays cheap because it is scoped to one fix at a time. This is the bug-fix counterpart of the characterize-before-refactor sequence in [/ci/pr-quality-gate.md](/ci/pr-quality-gate.md).
+**Rejected**: (a) a blanket "every fix needs a test that fails before it" — false for behaviour-preserving fixes, and the only way to satisfy it there is a fake; (b) writing no test at all for a provably-equivalent change — the equivalence holds because `PLATFORM_IOS` is `'ios'` today, and a test over both branches is what keeps the delegation honest if that ever changes; (c) asserting the kind from a reading of the diff without running the revert — that is precisely the reasoning a latent behaviour change survives.
+*Introduced by*: 260728-o3me-fix-deferred-error-path-defects