refactor(contracts): one viewport-root predicate for the whole repo - #1613
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Two blockers on |
|
Both blockers addressed on (1) macOS vocabulary — correct, and worse than reported. I had omitted six fixed outputs, not five ( Completing the table also falsified the header's own numbers. It said "71 of 73 agree, 2 disagree"; over the real 75 names it is 71 agree and 4 disagree, because (2) Merge resolved preserving #1567. It added Thanks for the #1610 pointer — matches my read, so I'm not chasing the naive-pipe failure. The remaining local 🤖 Addressed by Claude Code |
78d9441 to
2a7ee8d
Compare
|
Re-review of |
|
Re-review of exact head One P2 blocker remains: please restore the 44 unrelated P3: the PR body still reports 73 emitted names / 71 agreements / 2 exceptions (24 macOS forms), while the corrected current measurement is 75 / 71 / 4 (26 macOS forms). Please update it to match the validated table. The iOS Smoke failure ( |
"Is this the Application/Window root" was written nine times: three spellings normalizing `type|role|subrole`, five lowercasing `type` alone, and one comparing the normalized type for EQUALITY. Two of the nine sat in `contracts/snapshot-visibility.ts` itself, disagreeing with each other. Measured before collapsing, using #1592's method — ground the comparison in what each backend ACTUALLY emits, not in fixture strings. Over the 31 names iOS's `elementTypeName` can return, the 18 fully-qualified class names Android emits, and the 24 mapped/raw forms the macOS helper produces, the nine agreed on 71 of 73. The two exceptions are macOS window subroles, and the only spelling that disagreed is maestro's `===`, whose platform union is `android | ios` — so it can never see them. The duplication was textual, not behavioral, which is what made the collapse safe. `isViewportRootNode` reads role and subrole because the macOS helper is the only backend populating them and the only one able to emit a window whose `type` does not say so: `normalizedSnapshotType` returns the raw subrole for a non-standard window, so an `AXWindow` with subrole `AXSystemDialog` or `AXUnknown` reads as neither from `type` alone. Those two shapes are the whole behavioral delta of this change, at the six call sites that were type-only, and they are windows by role. `snapshot-viewport-root.test.ts` pins the predicate over those three emitted vocabularies. Red evidence: reverting the canonical definition to the type-only spelling fails 2 of 5 cells, to the equality spelling 4 of 5. Also drops two kernel re-declarations this made visible: maestro's local `containsPoint` and `rectsOverlap` were character-identical to `@agent-device/kernel/rect`'s `containsPoint` and `isRectVisibleInViewport`, in a file that already imports from that module. And `resolveViewportRect` loses three `as Rect` casts that only existed because `.filter()` cannot narrow `node.rect` — one `flatMap` states the same thing honestly. Deliberately NOT in this change: the three viewport RESOLVERS still diverge, and on Android that is a live defect rather than duplication. Filed separately with the measurement.
Review found the table claimed to pin "the vocabulary each backend actually
emits" while omitting most of it. `normalizedSnapshotType` has three output
classes and only two were represented:
1. thirteen roles mapped to fixed short names — six were missing
(StaticText, TextField, TextArea, MenuBarItem, Menu, MenuItem);
2. AXWindow, whose output is the SUBROLE unless it is AXStandardWindow;
3. the `default:` arm, `subrole ?? role`, emitting the raw AX-prefixed
value for every unmapped role.
All three are now enumerated, and the table asserts its own completeness
against the emitter's fixed-output set — a role added to that switch without
being added here fails, which is the emitter-drift protection the docblock
was promising but not delivering.
Re-measuring over the complete tables also corrected the header's own
numbers. The claim was "71 of 73 agree, 2 disagree"; over 75 names it is 71
agree and FOUR disagree, because AXSystemDialog and AXUnknown were absent
from the old table. Those two are the behavioral delta of this PR — an
AXWindow whose subrole is emitted as the type, invisible to the six
type-only spellings and named exactly by `role` — so the incomplete table
had been hiding the very rows that justify reading role/subrole. The other
two (AXFloatingWindow, AXSystemFloatingWindow) remain inert: only the `===`
spelling misses them and its platform union is `android | ios`.
Two test-validity defects from review, both real.
The raw-fallback row `{ type: 'AXSearchField', role: 'AXTextField', subrole:
'AXSearchField' }` was unreachable: the `AXTextField` arm returns `TextField`
whatever the subrole, so no emitter run can produce it. Replaced with
`{ type: 'AXSortButton', role: 'AXCell', subrole: 'AXSortButton' }` — a
subrole on a genuinely unmapped role, which is what the `subrole ?? role`
default arm actually emits.
`MACOS_FIXED_OUTPUTS` was a hand-kept twin compared against a hand-kept
table, which is circular: a new mapped Swift role is absent from BOTH, so
they agree and the gate stays green. The "emitter-drift protection" the
docblock promised did not exist. The set is now parsed out of
`normalizedSnapshotType` in SnapshotTraversal.swift, so the comparison is
against the emitter rather than against a copy of the table's own
assumptions. `case "AXWindow"` returns a subrole expression rather than a
literal and is deliberately outside the literal-return set.
Red evidence: adding `case "AXDisclosureTriangle": return "DisclosureTriangle"`
to the Swift switch fails with `expected [ 'DisclosureTriangle' ] to deeply
equal []`; 6 pass once reverted. The parser throws rather than silently
matching nothing if the function is renamed or moved.
673713d to
84fbb8a
Compare
|
Rebased onto Smoke Tests red is not this change. Worth noting the truncation length differs per run — you saw That said, "typed text silently truncates on iOS" is worth its own issue independent of any of these PRs if it is not already tracked — it is a correctness symptom, not just CI noise. 🤖 Addressed by Claude Code |
|
Changed-head re-review of The requested cleanup is still outstanding: the same 44 unrelated Android Smoke failed during emulator startup/initial capture with |
|
Smoke red again, but a different failure from the last one, and this one is infrastructure. adb was unreachable 104 seconds before the wait failed — nothing was ever driven on the device, so the Cross-check: #1615 is on the same base, also touches daemon code, and has all five smoke checks green. So this is per-run, not per-branch. That is now three distinct Smoke failures on this PR across three runs — iOS typing truncation ( Re: the size report bot above — no action taken, it reports a decrease (JS raw −876 B, gzip −196 B), which is the nine deleted predicate copies. Nothing to address. 🤖 Addressed by Claude Code |
45 corpus YAMLs carried an unrelated quote-style churn ("Button" ->
'Button'). They were already modified in the worktree when this branch
started and a `git add -A` swept them into the predicate commit. Nothing
in this PR reads them. Restored verbatim to main.
|
Both cleanups landed on Corpus restored. You were right and the cause is worth naming: the 45 corpus YAMLs carried an unrelated quote-style churn ( Body corrected, and recounted from the table rather than retyped: 31 iOS + 18 Android + 26 macOS = 75, 71 agreements, 4 exceptions. The body now also splits the four the way the test docblock does — two inert ( On the CI lanes: agreed both were non-results — Android died in emulator startup with 🤖 Addressed by Claude Code |
thymikee
added
the
ready-for-human
|
1 participant
"Is this the Application/Window root" was written nine times: three
spellings normalizing
type|role|subrole, five lowercasingtypealone,and one comparing the normalized type for EQUALITY. Two of the nine sat
in
contracts/snapshot-visibility.tsitself, disagreeing with each other.Measured before collapsing, using #1592's method — ground the comparison
in what each backend ACTUALLY emits, not in fixture strings. Over the 31
names iOS's
elementTypeNamecan return, the 18 fully-qualified classnames Android emits, and the 26 mapped/raw forms the macOS helper
produces, the nine agreed on 71 of 75. All four exceptions are macOS
windows, splitting two ways:
AXFloatingWindow/AXSystemFloatingWindow— only maestro's===spelling misses them, and its platform union is
android | ios, so itcan never see a macOS node. Inert.
AXSystemDialog/AXUnknown— anAXWindowwhose subrole is emittedAS the type, invisible to the six type-only spellings and named exactly
by
role. These two are the entire behavioral delta of this PR, andthey are windows by role.
(Corrected from an earlier "73 names / 2 exceptions / 24 macOS forms":
that was measured against an incomplete macOS table, which omitted six
fixed outputs and most of the raw
subrole ?? rolefallback — includingthe two rows that ARE the behavioral delta. Completing the table per
review moved the count, and the completeness claim is now derived from
the Swift emitter rather than a hand-kept twin.)
isViewportRootNodereads role and subrole because the macOS helper isthe only backend populating them and the only one able to emit a window
whose
typedoes not say so:normalizedSnapshotTypereturns the rawsubrole for a non-standard window, so an
AXWindowwith subroleAXSystemDialogorAXUnknownreads as neither fromtypealone. Thosetwo shapes are the whole behavioral delta of this change, at the six
call sites that were type-only, and they are windows by role.
snapshot-viewport-root.test.tspins the predicate over those threeemitted vocabularies. Red evidence: reverting the canonical definition to
the type-only spelling fails 2 of 5 cells, to the equality spelling 4 of 5.
Also drops two kernel re-declarations this made visible: maestro's local
containsPointandrectsOverlapwere character-identical to@agent-device/kernel/rect'scontainsPointandisRectVisibleInViewport,in a file that already imports from that module. And
resolveViewportRectloses three
as Rectcasts that only existed because.filter()cannotnarrow
node.rect— oneflatMapstates the same thing honestly.Deliberately NOT in this change: the three viewport RESOLVERS still
diverge, and on Android that is a live defect rather than duplication.
Filed separately with the measurement.
Gates: typecheck / lint / format /
check:layering/check:production-exportsgreen.test:unit610 files / 5382 tests.check:affected --rungreen on this SHA: 438 files / 4055 tests, "all runnable checks passed".Follow-up: #1609 (the three viewport resolvers still diverge; on Android that is a live defect, filed with measurements rather than folded in here).
Local gate note.
pnpm check:affected --runis flaky on a loaded dev host. Across six runs on three branches — plus a reproduction on unmodifiedorigin/mainby a parallel agent — failures came only from the poolandroid-lifecycle/android-recording/doctor/input-actions/daemon-client, always as timeouts, with a different subset each run (1, 7, 0, 2, 1). All pass in isolation. Please treat the CI Integration Tests job on this head as authoritative forprovider-integration, per AGENTS.md ("GitHub remains authoritative for reported device/toolchain lanes").