refactor: migrate get to the request-bound device runtime - #1877
refactor: migrate get to the request-bound device runtime#1877thymikee wants to merge 6 commits into
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Reviewed exact head
The branch’s own reconciliation and size blockers also remain: it must reconcile with |
…ad outcome Review blockers on #1877. 1. `dispatchGetViaRuntime` could complete the direct-iOS selector query before `resolveBoundGetRuntime`. Once `get` declares `device-runtime`, ADR 0019 requires resolve -> admit -> bind before anything in the request path operates, so admission now runs first for every target shape and the fast path is a fast path *within* an admitted request. Regression: an eligible direct selector cannot operate when facts refuse admission. 2. `readTextAtPoint` returned `Promise<string>` and `readTextForNode` caught any throw and fell back, assigning a typed diagnostic after an untyped failure. It now returns a closed `ElementTextReadOutcome`; fallback happens only for the contract's classified reasons; unexpected errors propagate. The reason union is derived from its runtime list so the two cannot drift, and an unhandled reason is a compile error at the consumer. This retires the generic catch the start record promised.
Both code blockers resolvedNew head: Blocker 1 — direct-iOS selector path completed before admissionFixed by ordering, not by reverting the descriptor. To be explicit about what this does and does not claim: Red before green. New regression The fast path answered Blocker 2 — untyped fallback
| Readonly<{ status: 'read'; text: string }>
| Readonly<{ status: 'unreadable'; reason: ElementTextUnreadableReason }>with
This is the generic-catch retirement my start record promised, so it is a deletion I already owed. Red before green. New regression Planted violation for the exhaustiveness gate. Adding a third reason to the single source of Both the compiler and the contract test go red; both are green with the planting removed. Contract coverage: On the ADR 0011 path classificationI did not add a row to Behavior change, declaredAn unexpected live-read failure (runner transport, helper crash) during Validation
One correction worth recording, because it cost a cycle and generalizes. An earlier attempt That failure was itself a real finding: only I resolved it by satisfying neither: the runtime reason list is deleted outright. Exhaustiveness The planted Live re-verificationRe-run on the assigned iOS Simulator
Still open, per the coordinator's sequencingThe The scratch-dir wart is fixed — it now lives outside the worktree, so |
|
Re-reviewed exact head |
…ad outcome Review blockers on #1877. 1. `dispatchGetViaRuntime` could complete the direct-iOS selector query before `resolveBoundGetRuntime`. Once `get` declares `device-runtime`, ADR 0019 requires resolve -> admit -> bind before anything in the request path operates, so admission now runs first for every target shape and the fast path is a fast path *within* an admitted request. Regression: an eligible direct selector cannot operate when facts refuse admission. 2. `readTextAtPoint` returned `Promise<string>` and `readTextForNode` caught any throw and fell back, assigning a typed diagnostic after an untyped failure. It now returns a closed `ElementTextReadOutcome`; fallback happens only for the contract's classified reasons; unexpected errors propagate. The reason union is derived from its runtime list so the two cannot drift, and an unhandled reason is a compile error at the consumer. This retires the generic catch the start record promised.
c666150 to
4778a27
Compare
The selector capture seam lands here, with
|
Live re-verification of the shipped seam pathThe seam swap changed both how
Session closed; no sessions left open. |
|
Blocking: #1739’s batch decision requires the shared read path to retire jointly, even while |
|
Out of draft at Stack: #1875 → #1877 → #1876 → Both blockers from your review at
This PR also carries the selector capture seam, because Three things worth the hardest look:
Live-verified at this exact head on |
…ad outcome Review blockers on #1877. 1. `dispatchGetViaRuntime` could complete the direct-iOS selector query before `resolveBoundGetRuntime`. Once `get` declares `device-runtime`, ADR 0019 requires resolve -> admit -> bind before anything in the request path operates, so admission now runs first for every target shape and the fast path is a fast path *within* an admitted request. Regression: an eligible direct selector cannot operate when facts refuse admission. 2. `readTextAtPoint` returned `Promise<string>` and `readTextForNode` caught any throw and fell back, assigning a typed diagnostic after an untyped failure. It now returns a closed `ElementTextReadOutcome`; fallback happens only for the contract's classified reasons; unexpected errors propagate. The reason union is derived from its runtime list so the two cannot drift, and an unhandled reason is a compile error at the consumer. This retires the generic catch the start record promised.
4778a27 to
8aed44f
Compare
`get` declares `elementReadRuntimeUse` (required `captureSnapshot`, preferred `readTextAtPoint`), admits once from exact owner facts, refuses before binding, and binds exactly once. Its capability bucket, the static HarmonyOS/Web command sets that augmented it, and `requireCommandSupported` admission for `get` are gone; `'get'` leaves the `createSelectorRuntime` capability union. The neutral `readTextAtPoint` operation replaces the branch-per-family legacy `read` dispatch on the `get` path. Every local family and both providers now classify it exhaustively — Web, HarmonyOS, Vega and every provider row report it unavailable, which is behaviour-preserving because the legacy dispatch had no arm for them and threw on every call before falling back. R36 is the new parametrized cutover row.
…ad outcome Review blockers on #1877. 1. `dispatchGetViaRuntime` could complete the direct-iOS selector query before `resolveBoundGetRuntime`. Once `get` declares `device-runtime`, ADR 0019 requires resolve -> admit -> bind before anything in the request path operates, so admission now runs first for every target shape and the fast path is a fast path *within* an admitted request. Regression: an eligible direct selector cannot operate when facts refuse admission. 2. `readTextAtPoint` returned `Promise<string>` and `readTextForNode` caught any throw and fell back, assigning a typed diagnostic after an untyped failure. It now returns a closed `ElementTextReadOutcome`; fallback happens only for the contract's classified reasons; unexpected errors propagate. The reason union is derived from its runtime list so the two cannot drift, and an unhandled reason is a compile error at the consumer. This retires the generic catch the start record promised.
…nsumer Takes ownership of the request-bound selector capture seam from #1876, which cannot ship standalone: with find's cutover deferred it had no consuming command (ADR 0019 §10) and was not dead-code clean (check:production-exports 19 -> 20). `get` is its first consumer, so it lands here. Adopts find's handoff as given. The one shape change, approved by the coordinator: the selector family gets its own capture uses carrying a PREFERRED `readTextAtPoint`, declared ALONGSIDE the snapshot uses so `snapshot`/`diff` keep binding exactly what they bind today. The read is surfaced through the existing arms of `bindSnapshotCaptureRuntime`, reusing the same selectActiveAppSnapshot / selectSnapshotWithoutActiveApp selectors — no second plan-to-operation dispatch. `get` now runs through `createBoundSelectorRuntime`; `resolveBoundGetRuntime` and its test are deleted as superseded, and `'get'` leaves the `createSelectorRuntime` capability union. The legacy read adapter survives for `find <q> get text` and is selected by which command constructed the runtime — never by failure, family, environment, or flag — so `get` cannot reach it. It retires in find's cutover, where the last consumer moves.
Read-only `find` now constructs a BOUND selector backend, so `get text` and
`find <q> get text` execute the same bound `readTextAtPoint` instead of one
binding it and the other dispatching the legacy `read`. This moves find's READ
LEG only: find's descriptor stays LEGACY_PLATFORM_EXECUTION and it claims no
cutover row.
With no consumer left, the whole chain goes: the `read` registry entry and its
`dispatch: {}` projection, `DISPATCH_HANDLERS.read`, `handleReadCommand`,
`interaction-read-legacy-dispatch.ts`, and the duplicate platform reader
branches it carried. `read` was the only `dispatch-alias` descriptor, so that
catalog group goes too.
Deleting the registry entry drops 'read' from DescriptorDispatchCommandName,
which makes a surviving DISPATCH_HANDLERS.read a compile error rather than
something R36 has to police. R36 now claims the retirement it can prove.
`find.test.ts` is over the size tripwire, so its handler invocation is
extracted to find-handler-fixture.ts and the pin lowered 1237 -> 1221.
Two edits, per find's ADDENDUM.md: 1. `includeRects` returns to `buildRuntimeCaptureInput`. It was removed from #1876 as unconsumed; the selector capture path is genuinely its first consumer (a Web rect capture requests bounds explicitly), so it lands here under the same rule that moved the seam. `snapshot`/`diff` pass nothing. 2. The per-capture `signal` is dropped, not restored. `CaptureSnapshotInput` has no such field on this stack — it moved to `wait` (#1875) with the regression that proves per-poll abort and quiescence. `get` captures once per resolution and never polls, so nothing here needs it. The seam test and fixture coverage for it moves with the contract rather than being kept against a field that no longer exists.
8aed44f to
52a1c9d
Compare
`get` declares device-runtime, so its request path must reach the platform only through operations R36 declares. `dispatchDirectIosSelectorGet` reached `runAppleRunnerCommand` through a path the row declares no operation for; admitting before a bypass is not executing through the seam, so the bypass is removed rather than ordered after admission. Every target shape — including the simple iOS `id=` selector — now resolves through the bound capture. `queryDirectIosSelector` itself stays: `offscreen-target-probe.ts` still consumes it and it remains single-copy. `dispatchDirectIosSelectorIs` belongs to `is` (#1883). Two get-only helpers (`readDirectIosGetSelector`, `buildDirectIosGetResult`) became unreachable and are deleted with the caller. Declaring `querySelector` as a fact-admitted preferred operation was rejected on duplication, not correctness: the offscreen probe takes a plain session and cannot consume a bound operation, so it would ship the query twice until Wave 5 moves the probe — the deferred-duplication shape this PR was already overruled for on the `read` alias. It returns as a declared, §9-measured operation in a later unit that also moves the probe. Cost, stated plainly: `get text id=…` loses its tree-capture skip on iOS. No fallback was added and the latency is not recovered elsewhere. R36's singularExecution claim is now what the code does rather than aspirational.
|
Blocking on exact head |
|
That review is against Verified on the pushed head:
Per the tracker ruling, the shortcut is retired in Two helpers went unreachable with the caller and are deleted: On the R36's A cost stated rather than buried: this is a real latency regression. The body refresh you asked for is in progress along with the CI Size figure and |
|
Re-reviewed exact head Not ready yet: the branch conflicts with current |
Summary
Migrates the
getdescriptor offplatformExecution: LEGACY_PLATFORM_EXECUTIONonto arequest-bound device runtime (ADR 0019 §6), Wave 4 unit 2 of issue #1739. Cutover rule id R36.
Start record: #1739 (comment)
Read-alias classification correction: #1739 (comment)
getnow declareselementReadRuntimeUse— requiredcaptureSnapshot, preferredreadTextAtPoint— resolves its plan, performs one side-effect-free facts inspection, refusesbefore binding when the required capture is unavailable, and binds exactly once on the admitted
device through the
admitRuntimePlantoken.The new neutral
readTextAtPointoperation replaces the branch-per-family legacyreaddispatchon the
getpath. Every local family and both providers classify it exhaustively; thetypechecker forced that completeness (the required fact is a missing-property error until each
owner declares it).
Read before reviewing: two things this PR does not claim
main(6984a1e09), not onfind's seam branch.findpublishedagent/wave4-findafter this work was already green, and its shape is better than mine: itadds
createBoundSelectorRuntime(params, { command })plusresolveBoundSelectorCapture, andbuilds the per-capture
CaptureSnapshotInputinselector-capture-runtime.ts. This branchmust be rebased onto
findbefore it is anything but a draft. The exact reconciliation is in"Rebasing onto find" below — it deletes code from this PR rather than adding any.
readdispatch alias, and that leaves a real duplication this PRshould not be merged with. See "Known gaps", which is the most important section here.
Cutover and retirement
Retired from the live path:
getcapability bucket on the descriptor;requireCommandSupportedadmission reached withcapability: 'get'—'get'leaves thecreateSelectorRuntime(..., { capability })union, which now reads'find' | 'is'. The optionand the call itself stay for the
isunit to delete last, per the batch plan;'get'fromHARMONYOS_SUPPORTED_COMMANDSandWEB_QUERY_COMMANDSinsrc/core/capabilities.ts(
addWebCommandCapabilitiesthrows for a web-listed command with no matrix row, so the web entrycould not have been left behind);
get's stale capability assertions in the descriptor parity and plugin-routing parity suites;vi.mock('.../core/dispatch.ts')binding inget's own tests — moved to theinspectFacts/bindDeviceseam in this PR.readTextForNodeis now platform-free: it owns only the policy of when to consult a live read,and the read itself is injected.
Declared parity delta
The two Apple watchOS cells are legacy-capability-supported for
getbut have no snapshotbackend, so they move from "admitted, then fails at the runner" to a typed unavailable admission
refusal. This is the same watchOS sentinel
snapshot(#1779) anddiff(#1847) already landed,not a new behavior decision, and it is the only cell where the fact denominator differs from the
19-cell capability denominator computed in the start record.
Preferred-operation measurement (ADR 0019 §9)
Recorded live on iOS Simulator
iPhone 17 Pro(F7D6F9A4-4FCC-4DD7-AC0B-3280C9319CB9), Settingssearch field with a 1,127-character value:
get attrsvalue)[truncated])readTextAtPointThe preferred read recovers 615 characters (2.2x) the required path truncates. Cost, measured
over 3 runs each:
get texton an editable node (live read fires) 0.46–0.48 s versus anon-editable node (tree only) 0.07–0.09 s, so the read costs ~0.38 s and is gated on iOS to
editable/expandable nodes only — the pre-existing gate, preserved verbatim.
Validation
Red before green
R36 was added to the cutover table and run against a clean pre-cutover tree (base
src/andpackages/, new modules removed) — 5 violations:After the cutover:
pnpm check:layeringgreen, andgetjoins the migrated-command list —"each migrated command (… snapshot, diff, get, viewport) keeps exactly one platform-execution path".
My first red capture was contaminated and I am recording that rather than quietly re-running
it:
git checkout <base> -- src packagesleaves newly-added files on disk, soget-runtime.tswas still present and the two operation-count violations were masked (4 violations, then 3). The
5-violation result above is from a tree with the new modules actually removed.
Descriptor/capability red, captured against pre-change code:
Note the union narrowing is not covered by the gate's admission column:
requireCommandSupported(options.capability, device)passes a variable, not a literal, so thegate cannot see it. The type-level narrowing of the
capabilityunion is the proof instead, andthe
isunit deletes the call.Gates
pnpm check:layering: PASS (1,324 source files; R36 green).vitest run --project unit-corebehind the shared wave-4 gate lock: 921 files / 6,964tests, all passing.
tsc -bover all 17 workspace packages, then root): PASS.pnpm format: applied.pnpm check:affected --run && git push: PASS — "all runnable checkspassed", pushed at
28b327ff0f36128f1024d5af752b3a2267c80979. Two earlier attempts failed thegate (format, then fallow) and correctly did not push; both causes are fixed in this head.
fallow audit: clean on 62 changed files. The audit initially flagged two unused fixture exportsand one high-complexity function in
platform-runtime-element-text-host.ts— both fixed by thetightening pass (mocks made module-private; the four-branch dispatcher split into named
per-family readers).
New coverage
src/daemon/__tests__/get-runtime.test.ts(9 tests): no-session refusal before any inspection orbind; exactly one inspection and one bind; the admitted device and exact use reaching
bindDevice;required-capture-unavailable refusing before binding; preferred-unavailable still admitting
and binding; provider fail-closed; provider-with-capture-but-no-read; missing-gateway refusal.
src/daemon/handlers/__tests__/interaction-read.test.ts— rewritten onto the runtime seam, plusnew rows for preferred-absent, typed-reason failure fallback, and blank-read fallback.
src/daemon/handlers/__tests__/interaction-get-runtime-fixture.ts— the sharedinspectFacts/bindDevicefake for interaction tests.readTextAtPointfact assertions in all six platform packages and both providers.get text/get attrsover@refand selector, both not-found failurepaths, 3
readTextrunner commands proving the bound preferred operation reaches the platform,and zero
platform_command_preparediagnostics withcommand: "read"proving the legacydispatch never fires for
get. Session opened and closed.Test fixture change that is not incidental churn
makeSessioninsrc/daemon/handlers/__tests__/interaction-touch-fixtures.tsnow returnsmakeIosAppSessioninstead ofmakeIosSession. This is a behavior surface, not tidying.On an iOS-family leaf the without-active-app capture row is unavailable. A selector command run
against a session with no tracked app therefore resolves the without-active-app plan, is refused
at admission before it ever captures, and returns the shared
SESSION_NOT_FOUND"requires anactive app session … run open first" — the same normalization the merged
snapshotanddiffunits already took on this exact operation. It cost three test failures before I applied it.
Reviewers should connect this to the admission-refusal question currently being grilled for
waitrather than reading it as test churn: every selector command inherits this surface.
Known gaps
1. The
readalias retirement is deferred, and it costs a duplicationMy start record classified the
readdispatch alias as exclusivelyget's. That was wrong,and I corrected it on the issue before writing the cutover row:
readText()inselector-read-shared.tshas three call sites across two commands —getCommand(get text @ref,get text <selector>) andfindCommand(read-onlyfind … get text). The alias is theshared element-text mechanic of the selector read backend.
So this PR leaves the alias alive for
find, behindsrc/daemon/handlers/interaction-read-legacy-dispatch.ts, and the four platform read branchesnow exist twice: once in
handleReadCommand(servingfind) and once insrc/platform-runtime-element-text-host.ts(servingget).fallow dupesdoes not flag it — 5groups / 207 lines, unchanged — because the two differ structurally, but it is the same four
branches and the wave's deletion discipline should not accept it.
Recommendation, and why it is now cheap: on
find's branch,createBoundSelectorRuntimeisthe single construction point for both
findandget, andis/waitdo not usebackend.readTextat all. Widening the selector use withpreferred: ['readTextAtPoint']theretherefore binds the read for both consumers at once and lets the whole chain retire together — the
readregistry entry and itsdispatch: {}projection,DISPATCH_HANDLERS.read,handleReadCommand,interaction-read-legacy-dispatch.ts, and the duplicated host branches.That is a change to the seam
findowns, so it needsfind's sign-off rather than my unilateraledit — flagged on the issue.
2. Size budget breached
Against the posted budget and the measured stack base
6984a1e09:Root production TypeScript also grew (+300 / −57 lines) where I budgeted a decrease.
Workspace-package production TypeScript: +212 / −1 lines.
I am not asking for a revised budget. The overage is the duplication in gap 1 — the read
mechanics landed in the host without the legacy copy leaving — so the correct response is to
complete the joint retirement, not to re-baseline. Expect this table to go negative once
handleReadCommand, thereaddescriptor, and the legacy adapter are deleted.3. Not claimed
queryDirectIosSelector) still runs ahead of the bind indispatchGetViaRuntimeand reachesrunAppleRunnerCommandfromsrc/daemon/. It is co-owned byis,wait, and the Wave 5 offscreen-target probe (src/daemon/offscreen-target-probe.ts), sounder ADR 0019 §6 it stays physically in place until its last consumer can move. Recorded as open
shared debt; this PR does not claim it as migrated.
captureSnapshotinsrc/daemon/handlers/snapshot-capture.tskeeps its optionalcaptureData?seam and the legacycaptureSnapshotWithInteractorfallback stays in place.readTextForNodeis retained under ADR 0019 §2'styped-reason carve-out (
interaction_read_fallbackwithbackend_read_failed/empty_backend_text— structured reasons, never message sniffing). The ADR 0011 pathclassification for it is not added here and belongs to the joint retirement above.
fact/parity evidence only, per the device allocation.
Rebasing onto find
Mechanical, and net-negative for this PR:
find'sselector-capture-runtime.tsandselector-runtime-backend.tswholesale;captureDatato(input: CaptureSnapshotInput) => …insnapshot-capture.ts—findkeeps the zero-arg seam and builds the input insnapshot-runtime-capture-input.ts, which is the better split;captureDataparam onselector-capture-runtime.tsand theoperations?.captureSnapshotthreading;resolveBoundGetRuntime's capture half withcreateBoundSelectorRuntime(params, { requireSession: true, command: 'get' });find's composition point per gap 1.Trivial conflicts already identified: the
WEB_QUERY_COMMANDSlist (both units remove an entry),the contracts and registry import lists, and the Linux runtime import list.
Scope and docs
Touched files: 50 (1,070 insertions, 89 deletions). Scope did not expand beyond the
getdescriptor: no other Wave 4 descriptor, no Wave 5 interaction descriptor, no capability or legacy
fallback added.
Docs and skills are unchanged: CLI grammar, help, and user-visible
getbehavior are identical.The coordination artifact on #1739 owns the exact denominator.