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
|
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.