refactor: migrate focus to the request-bound device runtime - #1925
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
The migration shape is sound, but three items still block readiness:
|
Wave 5's first unit (#1739, ADR 0019). `focus x y` and `find <q> focus` now reach the device through one admitted, request-bound `focusPoint` operation instead of the `handleFocusCommand` interactor leaf and its dispatch-table arm. - New `FocusRuntimeOperations` contract with local and provider interactor binders, mirroring the screenshot/element-text seam rather than inventing a second way for one operation class to reach its mechanics. - Exact-owner facts replace the capability bucket: apple simulator/device, android emulator/device/unknown, harmonyos emulator/device, linux device, web device, vega none, providers wherever their interactor is reachable. That is the retired bucket's cell table, restated as facts. - `focus` leaves BASE_COMMAND_CAPABILITY_MATRIX and both hand-maintained overlays (HARMONYOS_SUPPORTED_COMMANDS, WEB_INTERACTION_COMMANDS). - R40 is the new parametrized cutover row; `focusPoint` has exactly one owner. - The `x y` positional parse moves to utils and is shared with the still-legacy touch siblings, so a migrated command cannot drift from them. `find` stays legacy: this unit owns its focus leg only, its `type` leg still dispatches, and R35 waits on the Wave 5 `type` unit.
Review follow-ups on #1925. P1: focus-runtime.test.ts bound a fake focusPoint, so deleting the interactor call inside bindLocalFocusInteractor left focus a successful no-op with every test green. Adds packages/contracts/src/focus-runtime.test.ts, which executes both binders and asserts resolver context, positional (x, y) forwarding, the structured missing-provider failure, and that an already-cancelled request never resolves an interactor at all. Two planted mutants confirm it bites: removing `await interactor.focus(input.point.x, input.point.y)` and transposing its two arguments each fail exactly the two forwarding tests, while the daemon-level focus and find suites stay green — which is the gap the reviewer named. Coverage: find.test.ts shrank to 1204 lines when its focus assertion moved off the dispatch mock; the ratchet pin follows it down.
56e3238 to
46ccce3
Compare
|
Rebased onto 1 — [P1] The binder is now exercised, and the mutant proves itYou were right, and the failure mode was exactly as described: the daemon tests bound a fake Added Two planted mutants, both killed:
The right-hand columns are the point: the no-op mutant left every pre-existing test green, which is 2 — Coverage
3 — Live evidence per claimed cellRan real targets rather than fixtures. Both execution paths where reachable: the generic
All sessions closed and the emulator shut down after. Cells not exercised, and why
If you would rather I narrow the declared denominator to the five cells with real evidence and |
CI green, and the size number needs itemizingAll lanes pass at Flagging this myself rather than letting it pass: the authoritative Size run moved once rebased,
Itemization
The two big lines are one rebalance, not growth. They net +1.1 kB and appeared identically in The focus payload itself is small per bundle. Minified, each site is the use declaration
For calibration, #1875 pre-tightening was +3.1 kB raw / +950 B gzip / +1.1 kB tarball / +3.8 kB If you read the +4.5 kB as not justified by the above, the lever I would reach for first is |
|
Re-reviewed exact head 46ccce3. The owning-binder and find-ratchet findings are fixed, CI is green, and the +4.5 kB unpacked growth is adequately itemized. One readiness blocker remains: the unit record claims all six local families plus WebDriver and Limrun, but live changed-path evidence still omits Linux, Vega's unavailable refusal, and both provider owners. ADR 0019 requires a real-target result for every claimed family; fixture parity and same-binder reasoning do not close that denominator. Record those results, or formally narrow/correct the unit record with the corresponding scope decision, before calling this merge-ready. |
Provider cells: run live, both claims now have real-target evidenceRan the two cells I had left resting on the binder test. The denominator no longer needs narrowing —
Both exercise BillingBaseline checked before starting (no The hazard I was avoiding no longer existsI held this run back citing a 90s Allocation gets its own 300s budget, the client envelope derives from it so they cannot drift, and Full live matrix, consolidated
Nothing in the unit record needs correcting. |
The Linux `focus` claim rested on the provider scenario at command-contract level. The desktop replay runs on real Linux hardware in the Smoke lane, so it now runs a coordinate focus and re-asserts the session survived it. Coordinate, not selector: the step exists to prove the migrated `focusPoint` path executes on real hardware, so it must not be able to fail on match ambiguity or CI layout drift. Reclassifies focus contract -> live in the Linux coverage manifest and updates the two pinned counts. The manifest gate is two-sided — a live claim must name a command the replay actually invokes — so the claim cannot drift from the file.
Both remaining cells closed — Vega live on a real VVD, Linux promoted to live in CIHead is now Vega — run live, refusal confirmed on a real VVDThe Vega toolchain was installed here (SDK 0.23.9221), so this did not need a denominator revision. The hint is the Vega owner's own Linux — promoted from command-contract to liveRather than argue the bar, I closed it. Two findings first:
So Coordinate, not selector, deliberately: this step's job is to prove the migrated Manifest updated Full matrix, final
One thing I found that is NOT mine to fix, but you should knowWhile getting the Vega session I ran
This is the behavior #1739 already anticipates — Flagging it because the inaccuracy grows by one command per unit and is user-visible. If you want it |
|
1 participant
Wave 5's first unit for #1739 (ADR 0019). Migrates
focusfrom the legacy interactor leaf onto arequest-bound device runtime.
Unit record
focus), one operation (focusPoint), six platformfamilies plus two provider runtimes.
handleFocusCommandinsrc/core/dispatch-interactions.tsand thefocusarm of the
dispatchKnownCommandtable, at06d27de4d.packages/contracts/src/focus-runtime.ts— one contract owner for theoperation, its facts, and both interactor binders.
focusbinds no durable resource; its binding lives exactlyas long as the request.
handleFocusCommand, the dispatch-table arm, thefocuscapabilitybucket, and
focus's membership in the two hand-maintained capability overlays.What changed
One execution path.
focus x y(generic route) andfind <q> focusboth resolveresolveBoundFocusRuntime, admit the exact owner'sfocusPointfact, and bind once. Previouslythe leaf dispatched through
core/dispatch.tsand find dispatched thefocuscommand a secondtime, independently.
Facts replace the capability bucket. The retired bucket was
{ apple: sim+device, android: all, linux: device }, widened at runtime by two hand-maintainedoverlays in
core/capabilities.tsthat addedharmonyosandweb. Those five families plus vegaare now stated as facts by their owners:
focusPointsimulator|devicesimulatorrowemulator|devicedevicedevicecellThat is the retired bucket's exact cell table, so admission is unchanged; what changed is who
states it.
focusleavesBASE_COMMAND_CAPABILITY_MATRIX,HARMONYOS_SUPPORTED_COMMANDSandWEB_INTERACTION_COMMANDS.No second mechanism.
bindLocalFocusInteractor/bindProviderFocusInteractorride the sameInteractorseambindLocalScreenshotInteractorandbindElementTextRuntimealready use, ratherthan a bespoke host port for one more operation of the same class.
Deduplicated the positional parse.
readPointmoves fromdispatch-interactions.tstoutils/validation.tsasreadPointPositionals, shared byfocusand by its still-legacy touchsiblings (
press,longpress,hover). A migrated command and an unmigrated one cannot drift onwhat "requires x y" means.
focusaccepts fractional coordinates exactly as the leaf did — arequireInthere would have been a silent behavior change, and there is a test pinning it.Scope this unit does NOT claim
findstaysLEGACY_PLATFORM_EXECUTIONand claims no cutover row. This unit moves its focus legonly; the
typeleg still dispatches, so R35 remains reserved for the Wave 5typeunit. Sameshape as #1877, which moved find's read leg while leaving the descriptor legacy.
dispatchFocusForFindMatchis deliberately not in the retirement claim: the helper survives, whatchanged is what it calls. R40's
operationOwnersprovesfocusPointhas exactly one owner.Gate
pnpm check:affected --run: all runnable checks passed.pnpm check:layering: OK — 24 migrated commands now keep exactly one platform-execution path,including
focus. R40 is the new parametrized cutover row.focusPointtoPlatformRuntimeOperationsbroke every factsconstructor that had not classified the cell, which is how the six platform packages, two
provider runtimes and the composition root were found rather than guessed.
Size
Authoritative CI Size (#1842) at
46ccce3d8: JS raw +4.5 kB, gzip +1.3 kB, npm tarball+1.0 kB, npm unpacked +4.5 kB. Unpacked exceeds #1842's ~3 kB escalation bar, so it is
itemized in this comment. Source movement:
root
src/production +131 / −49, workspace packages +260 / −0.Closes nothing on its own — #1739 stays open until the terminal gate.