fix(replay): publish a full-cover system overlay's targets in divergence screen.refs - #1318
Merged
Conversation
…nce 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.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
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.
Member
Author
|
Code review is clean at |
thymikee
added
the
ready-for-human
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.
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.
`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.
thymikee
removed
the
ready-for-human
Member
Author
|
Re-review is clean at |
thymikee
added
the
ready-for-human
|
thymikee
added a commit
that referenced
this pull request
Jul 21, 2026
…g is ruled out on device Follow-up to the #1319 investigation, acting on review feedback that a paragraph justifying fragile behavior means the code is wrong. Two candidate fixes for the capture-shape-dependent chrome classification were tested, and BOTH are dead. Recording that here so the next person does not re-walk them: 1. The window-type approach this file's own TODO proposed. Measured against the live helper XML (emulator-5554, Pixel 9 Pro XL API 37): systemui reports `window-type=3` (TYPE_SYSTEM) both collapsed and expanded, `TYPE_STATUS_BAR` (2000) never appears, the helper stamps window metadata on window ROOTS only (1 of 169 nodes in an expanded-shade capture), and an expanded shade is ONE window hosting the status icons AND the quick-settings tiles. No window-level signal separates them. The TODO promised a fix the device data rules out. 2. Replacing run-condemnation with "condemn each marked node's subtree plus fully-condemned ancestors". Shape-independent as intended, and it keeps the tiles in both walks — but on the COLLAPSED status bar it leaks the unmarked chrome the walk re-parents next to the markers ("Battery 100 percent.", the notification-icon summary, neither carrying any resource-id). That is the ticking-clock regression #1319 explicitly warned against. So run-condemnation is not a lazy workaround: it reconstructs identity the walk already discarded when it drops the `status_bar*` wrappers and re-parents chrome leaves next to content. That upstream information loss is the actual root cause, and a provenance-preserving walk is the real fix — larger than this PR, and it would let #1318's divergence fallback be revisited. Also trims the #1319 test's doc comment from a long justification of the current behavior down to the finding, the defect, and what the test holds in place.
thymikee
added a commit
that referenced
this pull request
Jul 21, 2026
…nstructing it downstream Chrome classification gave opposite answers about the same screen depending on capture shape: an expanded quick-settings shade was 100% chrome under the `--raw`/non-raw walk (#1318 — every tile condemned, needing a divergence-local fallback) and ~17% chrome under the interactive-only walk (#1319). Two layers were papering over one broken classifier. Root cause: `shouldIncludeStructuralAndroidNode` drops the `status_bar*` / `navigation_bar*` containers — the only nodes that identify the region — and re-parents their leaves next to real content. Everything downstream was reconstructing identity the walk had already discarded, and reconstruction is what depended on shape. Fix: record it while it still exists. `walkUiHierarchyNode` threads `ancestorSystemChrome` exactly like the `ancestorHittable` it already carries, and stamps `systemChrome` on the emitted node; `androidUiNodes` tracks the same subtree for the streaming content-recovery pass. Classification is then per node and intrinsic, so `--raw` and non-raw agree by construction. This deletes what was compensating for the loss: - the 18 hand-picked marker leaf ids and their justification comment (what enumerates them is "descendant of a status/nav-bar container" — now stamped); - `collectAndroidSystemChromeRunIndexes`, the run-condemnation rule that let one `clock` node condemn 95 unrelated ones; - the leaf-id/prefix split, replaced by one container predicate that matches `status_bar`/`navigation_bar` as an id SEGMENT so the shade's own `split_shade_status_bar` counts. Net −53 lines across 8 files, almost all of it classifier and prose. Two alternatives were measured and rejected before this one (both recorded in #1319 so they are not re-attempted): - keying off AOSP window types, which this file's own TODO proposed: impossible. On a live device systemui reports `window-type=3` (TYPE_SYSTEM) collapsed AND expanded, `TYPE_STATUS_BAR` never appears, metadata is stamped on window roots only (1 of 169 nodes), and an expanded shade is ONE window holding the status icons and the tiles. - condemning marked subtrees plus fully-condemned ancestors: leaks the unmarked chrome the walk re-parents beside the markers ("Battery 100 percent.", the notification-icon summary) — the ticking-clock regression #1319 warned about. Test fixtures that modelled the old mechanism now model the device instead: the synthetic status bar gets the `status_bar_launch_animation_container` root the real capture has, and the content-recovery XML nests its chrome leaves inside their container. Assertions were not weakened — settle.test.ts stamps via the production predicate, and the #1319 test now asserts both walks classify IDENTICALLY, which is the property that was missing. Verified live on emulator-5554 (Pixel 9 Pro XL API 37, deskclock): - shade opening mid-settle: settled after 7655ms: +28 -25, tiles present, zero status-bar chrome (unchanged from before — settle was already right) - ordinary action, no shade: +1 -1, no chrome leak - real captures: collapsed status bar 9/9 chrome; expanded shade keeps every tile under BOTH walks, which is the behavior that changed
thymikee
added a commit
that referenced
this pull request
Jul 22, 2026
…nstructing it downstream (#1319) (#1359) * test(settle): pin that an expanded quick-settings shade stays visible to --settle (#1319) #1319 asked whether the systemui run-condemnation rule leaves `--settle` blind to a fully expanded quick-settings shade, the way it blinded the replay divergence in #1318. It does not, and this pins the reason. The settle loop captures `interactiveOnly: true` (`stable-capture.ts`). That walk drops the structural systemui window spine (`legacy_window_root`, `notification_panel`, `qs_frame`, the quick-settings ComposeView chain) — and those are exactly the nodes that merge the shade into ONE contiguous run in the `--raw` / non-raw shape #1318 measured. Under settle's shape the same capture arrives as five runs, only `split_shade_status_bar` carries a marker, and the 29 quick-settings nodes survive into both diff sides. So settle and divergence do not read one tree differently, as #1318 framed it: they consume different capture shapes, and settle already gets the outcome that layer wants — shade content diffs, status-bar churn does not. Separately, the quiet-detection loop digests the UNFILTERED capture (`digestSnapshotNodes` in `stable-capture.ts`), so the shade would reset the quiet window even in the hypothetical where chrome stripping had emptied the diff. Both halves of the question come out clean. No product change. The behavior is correct but rested on an untested structural coincidence: retaining the systemui spine in the interactive walk would re-merge the runs and make `--settle` report a full-cover shade as bare removals with no added content and no hint. The test fails if that happens (verified by flipping the helper to `interactiveOnly: false`). Verified live on emulator-5554 (Pixel 9 Pro XL API 37, deskclock), both directions #1319 asked about and both halves the test asserts: - shade OPENING mid-settle: settled after 6917ms: +28 -25 (added: brightness seekbar, Wi-Fi/Bluetooth/Mobile data/Quick Share/ Modes/Wallet tiles) - shade CLOSING mid-settle: settled after 6919ms: +25 -28 (mirror image) - the shade's own status bar ("Tue, Jul 21, Wifi signal full., T-Mobile") is absent from both settle diffs while `snapshot -i` of the identical screen still lists it — the run rule filters the churn rather than sitting inert. The archived #1318 capture run through the real interactive-only walk reproduces the live tree node-for-node (35 nodes, 29 kept, 6 stripped). * docs(android): correct the chrome-classifier TODO — window-type keying is ruled out on device Follow-up to the #1319 investigation, acting on review feedback that a paragraph justifying fragile behavior means the code is wrong. Two candidate fixes for the capture-shape-dependent chrome classification were tested, and BOTH are dead. Recording that here so the next person does not re-walk them: 1. The window-type approach this file's own TODO proposed. Measured against the live helper XML (emulator-5554, Pixel 9 Pro XL API 37): systemui reports `window-type=3` (TYPE_SYSTEM) both collapsed and expanded, `TYPE_STATUS_BAR` (2000) never appears, the helper stamps window metadata on window ROOTS only (1 of 169 nodes in an expanded-shade capture), and an expanded shade is ONE window hosting the status icons AND the quick-settings tiles. No window-level signal separates them. The TODO promised a fix the device data rules out. 2. Replacing run-condemnation with "condemn each marked node's subtree plus fully-condemned ancestors". Shape-independent as intended, and it keeps the tiles in both walks — but on the COLLAPSED status bar it leaks the unmarked chrome the walk re-parents next to the markers ("Battery 100 percent.", the notification-icon summary, neither carrying any resource-id). That is the ticking-clock regression #1319 explicitly warned against. So run-condemnation is not a lazy workaround: it reconstructs identity the walk already discarded when it drops the `status_bar*` wrappers and re-parents chrome leaves next to content. That upstream information loss is the actual root cause, and a provenance-preserving walk is the real fix — larger than this PR, and it would let #1318's divergence fallback be revisited. Also trims the #1319 test's doc comment from a long justification of the current behavior down to the finding, the defect, and what the test holds in place. * fix(android): stamp status-bar chrome during the walk instead of reconstructing it downstream Chrome classification gave opposite answers about the same screen depending on capture shape: an expanded quick-settings shade was 100% chrome under the `--raw`/non-raw walk (#1318 — every tile condemned, needing a divergence-local fallback) and ~17% chrome under the interactive-only walk (#1319). Two layers were papering over one broken classifier. Root cause: `shouldIncludeStructuralAndroidNode` drops the `status_bar*` / `navigation_bar*` containers — the only nodes that identify the region — and re-parents their leaves next to real content. Everything downstream was reconstructing identity the walk had already discarded, and reconstruction is what depended on shape. Fix: record it while it still exists. `walkUiHierarchyNode` threads `ancestorSystemChrome` exactly like the `ancestorHittable` it already carries, and stamps `systemChrome` on the emitted node; `androidUiNodes` tracks the same subtree for the streaming content-recovery pass. Classification is then per node and intrinsic, so `--raw` and non-raw agree by construction. This deletes what was compensating for the loss: - the 18 hand-picked marker leaf ids and their justification comment (what enumerates them is "descendant of a status/nav-bar container" — now stamped); - `collectAndroidSystemChromeRunIndexes`, the run-condemnation rule that let one `clock` node condemn 95 unrelated ones; - the leaf-id/prefix split, replaced by one container predicate that matches `status_bar`/`navigation_bar` as an id SEGMENT so the shade's own `split_shade_status_bar` counts. Net −53 lines across 8 files, almost all of it classifier and prose. Two alternatives were measured and rejected before this one (both recorded in #1319 so they are not re-attempted): - keying off AOSP window types, which this file's own TODO proposed: impossible. On a live device systemui reports `window-type=3` (TYPE_SYSTEM) collapsed AND expanded, `TYPE_STATUS_BAR` never appears, metadata is stamped on window roots only (1 of 169 nodes), and an expanded shade is ONE window holding the status icons and the tiles. - condemning marked subtrees plus fully-condemned ancestors: leaks the unmarked chrome the walk re-parents beside the markers ("Battery 100 percent.", the notification-icon summary) — the ticking-clock regression #1319 warned about. Test fixtures that modelled the old mechanism now model the device instead: the synthetic status bar gets the `status_bar_launch_animation_container` root the real capture has, and the content-recovery XML nests its chrome leaves inside their container. Assertions were not weakened — settle.test.ts stamps via the production predicate, and the #1319 test now asserts both walks classify IDENTICALLY, which is the property that was missing. Verified live on emulator-5554 (Pixel 9 Pro XL API 37, deskclock): - shade opening mid-settle: settled after 7655ms: +28 -25, tiles present, zero status-bar chrome (unchanged from before — settle was already right) - ordinary action, no shade: +1 -1, no chrome leak - real captures: collapsed status bar 9/9 chrome; expanded shade keeps every tile under BOTH walks, which is the behavior that changed * fix(android): keep systemChrome provenance out of published nodes; simplify androidUiNodes Review blockers on 8615771: 1. `systemChrome` reached CLI/SDK/MCP payloads: `serializeSnapshotResult` passed `result.nodes` straight through, so an internal classification became a de-facto public contract and per-node bytes. Added `publicSnapshotNodes` next to the field it strips, mirroring the existing `publicSnapshotCaptureAnnotations` projection, and applied it at that boundary (the single serializer both the CLI and capture output route through). The regression test asserts the published nodes deep-equal their input minus the field AND that the serialized JSON never contains the string; it fails if the projection is removed. 2. Fallow complexity on `androidUiNodes` (10 cyclomatic / 19 cognitive): extracted `createAndroidChromeSubtreeTracker`, which owns the depth and chrome-subtree state, leaving the generator a token loop. Not baselined. Note the tracker keeps open/close as distinct operations on purpose: a self-closing tag leaves at the depth it entered, a container descends and is left by its `</node>`, so collapsing them silently breaks nesting. * refactor(android): let the chrome classifier read the tree, deleting the test-side stamping helper Review: the remaining long comments read as workarounds. Three were, and each pointed at code worth changing rather than explaining. 1. `collectAndroidSystemChromeIndexes` required the walk-stamped `systemChrome` even when the tree still held the container that defines it. Hand-built trees (every settle unit test) therefore classified as "no chrome at all", which is why they needed `withAndroidSystemChrome` — a test helper re-implementing the production descent, with a comment explaining why it had to exist. The classifier now reads the container from whichever source still has it: the parent chain when the capture kept it, the stamped flag when the walk dropped it. The helper, its type and its comment are deleted; no test re-states production logic, and any caller holding an intact tree is now classified correctly regardless of how it was produced. 2. `createAndroidChromeSubtreeTracker` tracked depth with arithmetic subtle enough to need a comment about self-closing tags — and subtle enough that an earlier revision of it was wrong. Replaced with a stack of open elements: push on descent, pop on `</node>`, nothing to explain. 3. `walkInteractiveOnlyAndroidFixture`'s 16-line comment still described the run-classification this branch deleted — stale, not merely long. Cut to what the helper does. Also trimmed the contract, kernel, classifier and serialization-test comments to the fact each states. Comment lines in the branch drop from 116 to 51. No behavior change beyond (1), which strictly widens correct classification. 4318/4318 unit tests, lint, typecheck, build and smoke pass. * fix(android): keep chrome provenance internal * refactor(android): tighten chrome provenance seam * fix(android): close chrome provenance response leaks * test(provider): enforce chrome invariant across clients
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #1312 (closed as completed by #1301 — that close was incomplete: #1301 fixed the capture, but this issue's headline symptom, 0 refs for a full-cover overlay, survived it for a different reason; corrected diagnosis posted there).
Splits out #1319 (
--settlemay be blind to an expanded shade for the same run-rule reason — untested, plausibly desirable).The bug
With a foreground app open, raise a persistent full-cover quick-settings shade (
adb shell cmd statusbar expand-settings) and trigger a replay divergence. The divergencescreencomes backavailablewith 0 refs and no suggestions — while the capture plainly holds 95 systemui nodes, 23 of them hittable quick-settings tiles.Root cause
selectDivergenceScreenRefNodesdropscollectSettleChromeRefsnodes beforeisForeignOverlayDismissTargetcan rank them, so the overlay ranking never runs.collectAndroidSystemChromeRunIndexescondemns an entire contiguous same-package run when any node in it carries a status/nav marker. An expanded shade is a single systemui run — proven against a live capture:legacy_window_root, 95 nodes, one runclock,mobile_combo,mobile_signal,wifi_signal— status-bar icons the expanded shade hosts inside its own windowqs_tile_*targets with themThe run rule assumes the status bar is its own window. True when the shade is collapsed; false when it is expanded.
Why this is an invariant violation, not just a missed goal
Since #1301, a plain
snapshotof that same surface returns the tiles (thesystemSurfaceOnlycarve-out). So the divergence had become strictly narrower thansnapshot— precisely whatcaptureDivergenceObservationdocuments and the ADR 0012 decision 4 amendment forbids: the chrome filter must stay a FILTER, "never as a narrower scoping".Why the fix is divergence-local, not in the shared run rule
Settle and divergence genuinely disagree about these 95 nodes, and both are right. Settle must keep stripping the shade run: the 4 markers are literally clock/signal/wifi — the canonical 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, so that is the layer fixed. The shared classifier in
snapshot-chrome.tsis untouched, so--settlebehavior is unchanged by construction.Chrome exclusion now falls back to hittable chrome nodes when it would otherwise empty the pool — mirroring the
coveredfallback already in the same function ("a report broken by construction").The gate is hittability, not label presence, and both real captures back that:
7:03)9:56, 339×350)Without that gate, a bare
"7:03"would be published as the whole screen whenever an app tree yielded no meaningful targets.Verification
Live on emulator-5556 (Pixel 9 Pro XL API 37, deskclock):
screen.refs: 0screen.refs: 20(cap,truncated: true) —e14:seekbar "Display brightness"+ 19 hittable tilesThe live refs match the fixture-driven unit test exactly (same count, order, labels).
ANDROID_QS_SHADE_CAPTURE_RAW_NODESis a real 138-node--rawcapture generated mechanically from the device, not hand-authored ids — the walk reproduces the live tree node-for-node (95 nodes / 23 hittable). The regression test fails if the fix is reverted.225 test files / 1931 tests, lint and typecheck all pass.
Notes for reviewers
snapshotfails identically"). Post-fix(android): return meaningful occluding system surfaces instead of failing the snapshot #1301 that symmetry broke —snapshotreturns 95 refs, the divergence returned 0 — so it is divergence-specific, and it is a documented-invariant violation rather than a missed aspiration.ad3cf045= main1fdbf80c3, a git ancestor of fix(android): return meaningful occluding system surfaces instead of failing the snapshot #1301 — i.e. a pre-fix binary. That guard is no longer implicated; the shade capture succeeds today. The benchSUMMARY.mdstill records the stale conclusion.press @e14) reports "needs a complete snapshot" — identical on the ordinary app-content divergence path (@e20 "World Clock", no shade), so it is by design; the hint directs the agent to re-snapshot or use a selector.--settlemay be blind to an expanded shade for the same run-rule reason. Untested, and plausibly desirable — worth a separate look.