fix(android): return meaningful occluding system surfaces instead of failing the snapshot#1301
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Review at exact head 23aa637 — not ready yet.
The branch is based on current main, and the local emulator evidence is useful for the demonstrated zero-app-root shade/QS shape. Mixed OEM window layouts, IME ordering semantics, and the requested Limrun smoke remain residual device risk. iOS Smoke is still running, so I did not apply ready-for-human. |
…s; gate the carve-out on non-chrome content (PR #1301 review) P1a — selector/find/wait routes no longer lose the disclosure. SnapshotState gains systemSurfaceOnly, stamped at the one seam where snapshot state and capture annotations meet (captureSnapshotAttempt), so every consumer — including session-stored snapshots — inherits it. The disclosure message moves to a shared module (snapshot/system-surface-disclosure.ts) used by the capture-runtime warning and a daemon response helper (handlers/system-surface-disclosure.ts) that appends it to ok-response warnings and error-response hints. Applied on both found and not-found outcomes across the public daemon selector routes: read-only find (exists/wait/get_text/get_attrs), mutating find (matched, unmatched, and ambiguous), wait (text/selector/ref/stable; pure sleep is exempt), get, and is. P1b — the >=3 meaningful-node floor for the system-surface carve-out now counts only NON-CHROME nodes. The status/nav-bar marker ids from the settle-chrome classifier (#1198/#1251) move to a shared contracts/android-system-chrome.ts (core/snapshot-chrome.ts keeps byte-identical behavior via the shared resource-id predicate), and classifyAndroidHelperContent excludes chrome-classified resource-ids from activeSystemSurfaceMeaningfulNodeCount: an active nav bar (Back + Home + Recents) or status chrome (clock/battery/wifi) is missing-app-content residue, not a usable shade. Shade/QS fixtures are unaffected (tile/notification ids are not chrome markers). P2 — regressions prove the production wiring, not just the classifier: snapshotAndroid stamps androidSnapshot.systemSurfaceOnly for a helper-backed shade capture; the capture runtime renders the disclosure warning from the annotation; daemon regressions pin the disclosure on mutating find (found), read-only find exists, and wait timeout against a shade capture; classifier fixtures pin nav-bar and status-chrome windows as unusable. Live-verified on emulator-5556 with the shade expanded: snapshot -i returns systemSurfaceOnly true plus the warning; find exists returns found:true plus the disclosure warning; wait timeout carries the disclosure in its hint.
|
Exact-head re-review (
All current CI is green; no |
|
All three findings addressed in 19a4313, CI green (22/22): P1 — disclosure threading. P1 — chrome-blind floor. The carve-out no longer counts raw meaningful nodes: P2 — production wiring proof. Helper-backed Live evidence: the snapshot route was verified on the emulator in the previous round (settled shade 5/5 and QS returning 🤖 Addressed by Claude Code |
…failing the snapshot (#1253) The notification shade and quick settings legitimately own the whole screen: the helper faithfully captures the active system window, but the content classifier treated the missing application window as a helper failure. Add a carve-out: an active/focused non-application window carrying meaningful content is returned as the snapshot, flagged systemSurfaceOnly, with an agent-facing warning explaining how to reach app content. Sparse or inactive system windows keep failing with the structured retriable error.
…s; gate the carve-out on non-chrome content (PR #1301 review) P1a — selector/find/wait routes no longer lose the disclosure. SnapshotState gains systemSurfaceOnly, stamped at the one seam where snapshot state and capture annotations meet (captureSnapshotAttempt), so every consumer — including session-stored snapshots — inherits it. The disclosure message moves to a shared module (snapshot/system-surface-disclosure.ts) used by the capture-runtime warning and a daemon response helper (handlers/system-surface-disclosure.ts) that appends it to ok-response warnings and error-response hints. Applied on both found and not-found outcomes across the public daemon selector routes: read-only find (exists/wait/get_text/get_attrs), mutating find (matched, unmatched, and ambiguous), wait (text/selector/ref/stable; pure sleep is exempt), get, and is. P1b — the >=3 meaningful-node floor for the system-surface carve-out now counts only NON-CHROME nodes. The status/nav-bar marker ids from the settle-chrome classifier (#1198/#1251) move to a shared contracts/android-system-chrome.ts (core/snapshot-chrome.ts keeps byte-identical behavior via the shared resource-id predicate), and classifyAndroidHelperContent excludes chrome-classified resource-ids from activeSystemSurfaceMeaningfulNodeCount: an active nav bar (Back + Home + Recents) or status chrome (clock/battery/wifi) is missing-app-content residue, not a usable shade. Shade/QS fixtures are unaffected (tile/notification ids are not chrome markers). P2 — regressions prove the production wiring, not just the classifier: snapshotAndroid stamps androidSnapshot.systemSurfaceOnly for a helper-backed shade capture; the capture runtime renders the disclosure warning from the annotation; daemon regressions pin the disclosure on mutating find (found), read-only find exists, and wait timeout against a shade capture; classifier fixtures pin nav-bar and status-chrome windows as unusable. Live-verified on emulator-5556 with the shade expanded: snapshot -i returns systemSurfaceOnly true plus the warning; find exists returns found:true plus the disclosure warning; wait timeout carries the disclosure in its hint.
|
Limrun smoke (the pending validation item) — run against this branch rebased onto current main (picks up #1278's direct Limrun runtime; rebase was conflict-free, branch force-pushed):
🤖 Addressed by Claude Code |
19a4313 to
bbbac6e
Compare
|
Exact-head re-review (
The Limrun smoke is credible for helper/provider routing but does not exercise |
… pin disclosure composition (PR #1301 review) Sessionless find/wait never store the consumed capture on a session record, so the disclosure read from the session store returned nothing. The selector capture runtime now reports every consumed snapshot through a shared slot on the runtime params, and disclosure reads prefer it; the session-store read remains only as a fallback for pre-captured snapshots. Regressions pin the sessionless route end-to-end and that the disclosure appends after existing success warnings and failure hints instead of replacing them.
|
Both remaining blockers addressed in 8c98dc0, on top of the rebased head: P1 — sessionless disclosure. The selector capture runtime now reports every consumed snapshot into a shared P2 — composition pinned. Two new direct regressions on CI-relevant: unit suite green (the handful of full-suite failures are the known process-spawn contention flakes — runtime-hints, apple runner-client/session, client-metro, daemon-client — all pass in isolation, 199/199). 🤖 Addressed by Claude Code |
|
Exact-head re-review ( The consumed-snapshot slot is initialized only in Initialize/pass the shared slot on the wait route and add a sessionless wait success/timeout regression. CI is still running and no |
…(PR #1301 review) dispatchWaitViaRuntime builds its selector runtime directly rather than via createSelectorRuntime, so sessionless waits had no slot for the capture runtime to report the consumed snapshot into and lost the system-surface disclosure. Regressions pin sessionless wait success and timeout, both asserting no session record exists and the disclosure is present.
|
Sessionless-wait blocker addressed in 76a180c: Daemon suite 1354/1354; typecheck/lint/format/fallow clean. 🤖 Addressed by Claude Code |
thymikee
added
the
ready-for-human
|
Exact-head re-review ( |
|
…nce screen.refs (#1318) * fix(replay): publish a full-cover system overlay's targets in divergence screen.refs A fully expanded quick-settings shade left the divergence `screen` available but empty: 95 captured systemui nodes, 0 refs, no suggestions. `selectDivergenceScreenRefNodes` drops `collectSettleChromeRefs` nodes before `isForeignOverlayDismissTarget` can rank them, and the run-level chrome rule condemns a whole contiguous same-package run when any node in it carries a status/nav marker. An expanded shade is a SINGLE systemui run: the 4 status-bar icons it hosts (clock, mobile_combo, mobile_signal, wifi_signal) condemn all 95 nodes, the 23 hittable qs_tile targets included. The run rule assumes the status bar is its own window — true collapsed, false expanded. Since #1301 a plain `snapshot` of that same surface returns the tiles (the `systemSurfaceOnly` carve-out), so the divergence had become strictly NARROWER than `snapshot` — the invariant `captureDivergenceObservation` documents and the ADR 0012 decision 4 amendment forbids: the chrome filter must stay a FILTER, never a narrower scoping. Fixed in the divergence layer, not in the shared run rule: settle and divergence genuinely disagree about these nodes and both are right. Settle must keep stripping the shade run — the markers are literally clock/signal/wifi, the churn settle exists to ignore, and sparing runs that hold actionable content would let a ticking clock hold settle awake. The divergence layer owns the violated contract. Chrome exclusion now falls back to hittable chrome nodes when it would otherwise empty the pool, mirroring the existing `covered` fallback. The gate is hittability, not label presence, and both real captures back that: the collapsed status-bar clock is labeled "7:03" but not hittable (so a chrome-only screen still publishes nothing), while the expanded shade's clock is hittable. Live-verified on emulator-5556 (Pixel 9 Pro XL API 37): same repro 0 -> 20 refs (cap, truncated) — the brightness slider plus 19 hittable tiles, matching the fixture-driven test exactly. `ANDROID_QS_SHADE_CAPTURE_RAW_NODES` is a real 138-node --raw capture, not hand-authored ids. * docs(replay): state the chrome-fallback boundary (PR #1318 review) The fallback fires only when chrome exclusion would empty the pool, mirroring the `covered` fallback right below it. A marker-bearing overlay over PARTIALLY visible app content keeps its tiles condemned — the pool is non-empty, so no fallback. That is deliberate (the full-cover case is the one that strands an agent), but it was implicit; stating it saves the next investigator a live session. * refactor(test): move Android capture fixtures into .json files The fixture module had grown to 1080 lines, of which ~990 were two inlined device-capture literals — the walkers and their docs were buried under the data they operate on, and any future capture would bury them further. Capture fixtures are archived device trees: DATA to be regenerated from a device, not code to be hand-edited. They now live in `.json` beside the module and load via `fs` + `import.meta.url` — the existing `test/output-economy` baseline pattern, which needs no `resolveJsonModule` or import-attributes support to typecheck, and keeps the trees out of the bundle. Mechanically extracted from the current exports and verified byte-identical (`JSON.stringify` before/after match for both fixtures), so this is a pure move: no fixture data changed. Module drops 1080 -> 109 lines. The leg-E regression test remains revert-sensitive. * refactor(test): typecheck the capture fixtures via resolveJsonModule Reading the fixtures with `fs` + `JSON.parse(...) as RawSnapshotNode[]` bought nothing over a static import and gave up the one thing that matters for a typed data fixture: the cast asserts the shape away unchecked, so a fixture that drifts from `RawSnapshotNode` compiles fine and only surfaces as a confusing test failure — or silently passes. `resolveJsonModule` + `import ... with { type: 'json' }` structurally checks each tree against `RawSnapshotNode[]` at typecheck time instead. Verified: corrupting a fixture (`hittable: "yes-please"`, a stray field) now fails `tsc` at the export, where the `fs` version accepted it. Also drops the reader helper and the runtime read. The `test/output-economy` precedent I first copied reads baselines through `fs` for a different reason — those are runtime-compared artifacts, not statically typed fixtures — so it did not apply here. tsconfig gains one option; `noEmit` is already set, so it has no output-layout effect. Fixture data unchanged (byte-identical), leg-E test still revert-sensitive, full tooling + 1931 tests green. * refactor(replay): inline the chrome fallback instead of explaining it `hittableChromeCandidates(nodes)` did not filter chrome — it returned every hittable node, and was only "chrome candidates" by virtue of its one call site. The name lied, and a chunk of the 15-line comment above it existed to cover the gap between the name and the behaviour: exactly the case where the comment is propping up the code. Inlined into the same `x.length > 0 ? x : y` idiom the `covered` fallback two lines below already uses, so the two narrowings now read in parallel. The boundary that comment spelled out — the fallback fires ONLY when exclusion empties the pool, so a partial-cover overlay keeps its tiles condemned — is now the ternary itself rather than a paragraph asserting it. What survives is the part the code genuinely cannot show: WHY a chrome-only screen must still publish (the never-narrower-than-`snapshot` contract) and why hittable is the discriminator. 6 lines, against the 8-line `covered` comment beside it. No behaviour change: same node set, fixtures untouched, leg-E test still revert-sensitive, 1931 tests + full tooling green.
1 participant
Closes #1253. Closes #1284 (decision record on the issue).
Scope note
Most of #1253 already landed on main between filing and now: the stock
uiautomator dumpfallback is gone from the snapshot path, artifact/install failures throw actionable errors, capture/parse failures propagate structured helper details, and content-poor output fails with a retriable hint — no production path shells out to stock UIAutomator, and every successful direct snapshot reportsbackend: "android-helper".This PR completes the issue's remaining half — "these should be treated as helper/product issues to verify and fix where possible" — for the one case live-probing proved was being misclassified.
The bug (live-reproduced on emulator)
With the notification shade or quick settings open over an app session, the helper faithfully captures the active systemui window (
helperWindowTypes: [3], dozens of meaningful nodes — the old stock dump returned 104–142 nodes on the same screens), butclassifyAndroidHelperContentsaw no application window and failed the snapshot ascontent-poor-app-window/system-window-only. The agent got an error and a screenshot hint on a screen that had a perfectly actionable accessibility tree (dismiss button, notification rows, QS toggles).Fix
A third carve-out in the content classifier, alongside the existing alert-surface and IME ones: when the capture has no application window but an active/focused non-application window carrying meaningful content (≥3 meaningful nodes), the capture is the truth and is returned:
classifyAndroidHelperContentRecovery→classifyAndroidHelperContent, returning{ outcome: 'ok' | 'system-surface-only' | 'unusable' }(decision unchanged for unusable).AndroidSnapshotBackendMetadatagainssystemSurfaceOnly?: boolean; the capture runtime emits an agent-facing warning: the snapshot shows the system surface; interact with it or dismiss it to reach app content.Live verification (emulator, Settings session)
content-poor-app-window)systemSurfaceOnly: true+ warningsystemSurfaceOnly: true+ warning — consistent across 5 consecutive capturesempty-helper-outputfailureVerification
system-surface-only; sparse active surface → unusable; content-rich but inactive window → unusable); snapshot + capture-runtime suites green; typecheck/lint/format/tooling/fallow clean (including the complexity gate — the meaningful-node counter was split into focused helpers).runtime-hints.test.tsprocess-spawn contention flake (passes in isolation).Pending from the issue's validation list
snapshotHelperArtifactprecedence + identity guard), but a short live Limrun session onsnapshot -i --jsonis still worth doing before closing the loop operationally.