refactor: migrate screenshot to request-bound runtime - #1878
Conversation
|
Reviewed exact head This is not readiness-label eligible yet. The branch currently conflicts with One explicit human decision is also required: the posted unit budget required root |
Retires the last dispatchCommand edges for screen capture: the generic-route command, the sparse-snapshot fallback, and the Android snapshot-timeout evidence capture all admit exact owner facts and bind once (ADR 0019, cutover rule R39). --overlay-refs becomes part of the declared use, so a target that can capture pixels but not a tree is refused before anything is written to disk.
5fc8b7e to
6e6abed
Compare
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Re-reviewed exact head |
|
Out of draft at
A correction on the posted budget itself: this unit's ≤ −1,500 B root The one genuine reduction this unit found was taken rather than argued: withdrawing the Unchanged from your review: the Android overlay debt is declined with evidence, not claimed — |
|
Readiness update at unchanged exact head |
|
…sult Review P1 on #1865: routing blocking-dialog recovery through buildSnapshotState made the occlusion result available but nothing consumed it. containsBlockingDialog scanned every node and findCloseAppButton returned the first text match with a rect, so a stale ANR surface left under the foreground one could still trigger recovery, and a covered "Close app" could be tapped ahead of the visible top button — the disagreement the routing was supposed to remove. Both decisions now filter through isSnapshotNodeInteractionBlocked, the shared predicate over the annotator's structured result. Two regressions cover it, both proven red against an unfiltered selection: a covered Close app preceding a visible one (asserts the visible center is tapped) and a fully covered dialog signal (asserts recovery does not trigger, no tap dispatched). Rebase reconciliation: screenshot-runtime.ts arrived on main (#1878) importing buildSnapshotState from its old home; repointed to src/daemon/snapshot-state.ts with the other importers.
…log recovery; residues declared (#1832 C3) (#1865) * refactor(android): raw is the acquired tree; one presentation for dialog recovery; residues declared (#1832 C3) - C3: the three regular-projection pruners (invisible subtrees, stale application windows, covered same-window surfaces) move out of parseUiHierarchyTree into the projection as a non-mutating classification (collectAndroidHiddenNodes in ui-hierarchy-visibility.ts). --raw presents the acquired tree; interactive ⊆ regular ⊆ raw by construction. Hidden-content hints and the scope root are derived per projection over retained children, which is what the mutating pruners implied. Property-checked identical to main for regular/-i/depth/scope over 12,000 random tree × projection pairs; raw grew on 2,514/3,000 and never shrank. - Android blocking-dialog recovery routes through buildSnapshotState (the one presentation), which moves to src/daemon/snapshot-state.ts below the daemon-server type cycle; importers repointed, its tests mirror the module. - Freshness route signature drops role/selected (Android never carries them). - Residues declared at their sites and in CONTEXT.md; docs + CHANGELOG. - ui-hierarchy.ts split by question: node predicates (ui-hierarchy-node.ts), regular-projection visibility (ui-hierarchy-visibility.ts), scope (ui-hierarchy-scope.ts); 974 → 634 LOC. * test: lower the snapshot.test.ts size pin to its new length * fix(android): dialog recovery acts on the presentation's occlusion result Review P1 on #1865: routing blocking-dialog recovery through buildSnapshotState made the occlusion result available but nothing consumed it. containsBlockingDialog scanned every node and findCloseAppButton returned the first text match with a rect, so a stale ANR surface left under the foreground one could still trigger recovery, and a covered "Close app" could be tapped ahead of the visible top button — the disagreement the routing was supposed to remove. Both decisions now filter through isSnapshotNodeInteractionBlocked, the shared predicate over the annotator's structured result. Two regressions cover it, both proven red against an unfiltered selection: a covered Close app preceding a visible one (asserts the visible center is tapped) and a fully covered dialog signal (asserts recovery does not trigger, no tap dispatched). Rebase reconciliation: screenshot-runtime.ts arrived on main (#1878) importing buildSnapshotState from its old home; repointed to src/daemon/snapshot-state.ts with the other importers.
Summary
screenshotno longer reaches a platform through the legacy dispatcher. It resolves a plan, admitsits exact owner facts once, binds once, and captures through the bound device runtime — the ADR 0019
request-bound path, enforced by cutover rule R39. Part of #1739 (Wave 4).
Three things change for callers:
--overlay-refsfails closed up front. The annotation needs a snapshot of the same screen, sothe snapshot is now part of the declared use. A target that can capture pixels but not a tree is
refused before a PNG is written, instead of producing a file and then failing — and the refusal
names the flag to drop:
Unsupported targets are refused by their owner, not by a hand-maintained bucket. The message is
unchanged (
screenshot is not supported on this device); the hint now comes from the owningruntime's fact and the payload carries
details.reason. watchOS is the one cell that tightens: thecapability bucket used to admit it and the Apple interactor then threw
UNSUPPORTED_PLATFORMwhilebeing constructed. It is now an unavailable fact with a hint, refused before any binding.
capabilitiesderives thescreenshotrow fromcaptureScreenshot, likeapps,appstate,open, andviewportalready do.CLI grammar, flags, help text, output shape, and recorded session actions are unchanged.
Retiring the command leaf required retiring every
dispatchCommand(device, 'screenshot', …)edge, sotwo observation paths that captured a screenshot as evidence move onto the same admitted seam: the
sparse-snapshot fallback (
sparse-fallback-screenshot.ts) and the Android snapshot-timeout evidencecapture (
android-snapshot-timeout-evidence.ts). That is cutover scope, not scope creep — thedispatch handler cannot be deleted while any caller survives.
Retirement list
DISPATCH_HANDLERS.screenshotandhandleScreenshotCommand(src/core/dispatch.ts).dispatch: {}projection andcapability: ALL_DEVICE_COMMAND_CAPABILITY, plus'screenshot'inWEB_QUERY_COMMANDSandHARMONYOS_SUPPORTED_COMMANDS.requireCommandSupportedfor screenshot. Rather than adding a second hardcodedcommand name beside viewport's, the exemption is registry-derived
(
commandUsesDeviceRuntimeExecution): adevice-runtimedescriptor never consults a capabilitybucket. Viewport's hardcoded name goes with it, and the next unit's descriptor flip is the whole
change.
dispatchCommand-backedAgentDeviceBackend, theScreenshotOutputPlacementtype, and theoutputPlacementparameter with both of its dispatch shapes — they were already behaviorallyidentical, because
handleScreenshotCommandcollapsedpositionals[0] ?? outPath.request-generic-dispatch.ts:executeScreenshotPlatformCommand,resolveScreenshotOutputPlacement,applyScreenshotOverlay, and the four positional/--outresolvers. The dispatcher went 425 → 317 LOC and no longer branches on a command name at all.
resolveCapturedScreenshotPath/hasStringPathin the Android evidence path (the operationreturns
void; the destination is the one the caller reserved).request-router.ts, folded into one resolver so the router staysorchestration-only.
WebDriverPlatformRuntimeOptionsreplaces the same inline options object repeated at three callsites, and repeated
x ? available : unavailableconjunctions infreezeUnavailableFacts,webRuntimeFacts,webDriverFacts, and two fixtures each collapse to one evaluation. Thescreenshot binder itself is union-free: each entry point supplies its own owner resolution, so
there is no
ownershipdiscriminator to share and nothing to clone from the snapshot facet.dispatchGenericCommandsplit (22 cyclomatic / 111 lines, and the only remainingover-threshold function in the changed set): its post-execution tail is now
finalizeGenericCommand, and the three-clause recovered-dialog condition iswithRecoveredDialogWarning. Both are below threshold.captureScreenshotUsefacade re-export, which had no production consumer once the descriptortook the plan array — the use is module-private again.
screenshot-overlay.tswas 635 LOC — past the extract-before-adding-behaviortripwire. Its PNG raster/font renderer is now
screenshot-overlay-draw.ts(153 LOC), leaving 486LOC that answer only "which node earns a ref, and where does its rect land".
clampwas duplicatedacross both halves and now has one owner in
screenshot-overlay-rects.ts.Shared Wave 4 capture debt:
screenshot-overlay-android.ts— declined, with evidenceEnumerated consumers:
screenshot-overlay-android.tshas exactly one importer(
screenshot-overlay.ts:17), andscreenshot-overlay.tshas two production importers — thescreenshot route (mine) and
android-snapshot-timeout-evidence.ts:10, whose annotation path thisunit does not migrate (only its capture leg moves). The Android source-rect resolver is therefore not
reachable from screenshot alone, so retiring or relocating it here would either break that consumer
or claim a file whose remaining consumer I did not migrate. It stays assigned-but-open Wave 4
debt. Its natural owner is whoever migrates that annotation path.
One correction for the tracker:
resolveAndroidOverlaySourceRectis not platform-gated —screenshot-overlay.tscalls it unconditionally and it self-selects on node shape. It importsnothing from
src/platforms/**, so it is not a daemon→platform dependency edge; it is Wave 6branch/tag-floor material, not terminal-gate material.
The overlay source-rect resolution facet named in #1739's Wave 4 facet list is deliberately not
landed here: its only non-screenshot consumer is that same annotation path, so a contract facet now
would be a registration API with one contributor — the rule this tracker already applies to iOS
presentation.
Validation
pnpm check:unit(928 files, 7,010 tests green) andpnpm check:affected --rungreen locally, serialized againstthe other Wave 4 worktrees under the shared lock.
pnpm check:layeringgreen with R39 in the migrated-command list; thelargest type-level import cycle dropped from 46 files to 26 because screenshot stopped threading
src/commands/**through generic dispatch, and the R9/R10 zone ceilings are lowered to match in thischange (
commandsandclientleave the cycle entirely).Red before green. Every new gate was seen failing first:
The R39 row went red twice before it went green —
expected one narrowed captureScreenshot call, found 0while the new modules were untracked, thenfound 2once both plan branches called theoperation directly. Funnelling both through
selectScreenshotCaptureis what makes the row'ssingular-execution claim true.
src/core/__tests__/dispatch-screenshot.test.tsasserts the legacy dispatcher answersINVALID_ARGS: Unknown command: screenshotand never reaches an interactor. Reverting onlysrc/core/dispatch.tstof3d5b3d92and rerunning it goes red exactly as it should — legacydispatch really did capture:
The exhaustive fact catalog rejected the change until all eight runtime owners classified
captureScreenshot:Property 'captureScreenshot' is missing in type … but required in type …across every owner and fixture.
Tests moved off the dispatch mock.
request-router-screenshot.test.tsno longer fakes screenshotthrough
vi.mock('core/dispatch.ts'); it binds a fake runtime atinspectFacts/bindDevicevia anew
screenshotRuntimeFixture, and the binder-level cases split intosrc/daemon/__tests__/screenshot-runtime.test.ts(907 → 639 + 203 lines, so neither approaches the1,000-line ratchet).
sparse-fallback-screenshot.test.tsand the Android-timeout leg ofsnapshot-handler.test.tsmoved to the same seam; the latter shrank, and its ratchet pin is loweredfrom 2654 to 2640. The file still mocks dispatch for
click/scroll, which are not migrated.Two overlay expectations changed, and the new values are the truer ones: the fake now supplies the
backendthe production contract requires, so Android's "rects are already device pixels, do notproject" rule applies exactly as it does on a real emulator. The old expectation was an artifact of a
fixture that omitted
backend.Rebased onto
8c06965d2and re-verified end to end. Conflicts were withmain's own commits,not with siblings — none of the five wave-4 units has merged, so
WEB_QUERY_COMMANDS/HARMONYOS_SUPPORTED_COMMANDSmerged clean and the predicted five-way conflict falls to whoeverlands second. Two resolutions worth naming:
#1873madeviewporta required cell onUnavailablePlatformRuntimeFactsso an ownercannot inherit the transport gap silently.
screenshotfollows it: also required. That surfacedevery gap site — linux, vega, limrun, webdriver ×2, the gateway ×2, and three fixtures — each of
which now states its own capture reason instead of falling back to
network.find(refactor(daemon): one capture-input builder and one admit-then-bind step #1876) threads a per-captureAbortSignalthrough exactly thebindSnapshotInteractorbody I had extracted, so thatextraction was a competing shape on a seam
findowns.packages/contracts/src/snapshot-runtime.tsis now byte-identical to
mainand this branch adds no diff to it; the screenshot facet dedupeswithin itself instead.
Live evidence — iOS Simulator (
iPhone 17, iOS 26.2, UDID1604B975-…), packaged daemon afterpnpm build && pnpm clean:daemon. Captured atf3d5b3d92before the change and re-run on thisbranch; results are identical:
screenshot positional.pngscreenshot --out out-flag.pngscreenshot(default)agent-device-screenshot-*/screenshot.pngscreenshot --overlay-refse6rect 16,168 370×90.33--pixel-density 2--scale 0.5capabilitiesscreenshotscreenshot, now from the factTwo consecutive plain captures of a static screen are byte-identical (
md5equal) while the--overlay-refscapture differs — which is what proves the annotation is drawn into the file ratherthan only reported in the response. Session
w4shot2closed;session listis empty.Tradeoffs and known gaps
+3 kB budget; raw +5.1 kB, tarball +1.9 kB, unpacked +5.1 kB) and
packages/**production is+10,629 B of a +14,000 B budget. But root
src/**production is +12,495 B against aposted budget of ≤ −1,500 B, so the move-dominated claim does not hold for this unit. Itemized:
screenshot-runtime-binding.ts+4,607 (the plan→admit→bind spine, which has no legacycounterpart to delete — legacy execution selection was one
dispatchCommandline); +4,082 netacross
screenshot-runtime.ts/request-generic-dispatch.ts/dispatch.tsfor behavior thatmoved out of the generic dispatcher plus its new finalizer;
generic-runtime-execution.ts+1,274for the runtime-owned-leaf resolver; +475 net for the overlay module split; the remainder is the
registry predicate, the two observation call sites, and the required-cell fills. I did not trim
doc comments to buy the number back. Flagged for the wave-level decision rather than
re-baselined.
src/daemon/screenshot-runtime.tsis 314 LOC, marginally over the 300 target and well under the500 extract line. Splitting it would add bytes to an already-missed budget for a 4% overshoot.
ScreenshotCommandResult: R2 keepsdaemon/below the command surface, type-only importsincluded. Removing two unreachable
typeof data.path !== 'string'guards is what surfaced it —the layering gate caught the import the same run.
captureSnapshot's optionalcaptureDataseam insnapshot-capture.tsstays optional andcaptureSnapshotWithInteractorstays: this is not the last selector unit. The unit that landslast makes it required and deletes the fallback.
cells are fixture-backed only. The assigned family for this unit is the iOS Simulator, and
siblings own the other devices.
find/get/is/waitunits insrc/core/capabilities.ts:all five delete one entry each from
WEB_QUERY_COMMANDSandHARMONYOS_SUPPORTED_COMMANDS. Theresolution is always "keep every sibling's deletion".
Docs/skills: not updated, and deliberately. No CLI grammar, flag, help, or output change; the
unsupported-message text is unchanged;
website/docs/docs/commands.md's only screenshot supportclaim (Vega reports unsupported) stays true under the new facts.
Scope: 64 files (33 production, 31 test). Scope expanded beyond the single generic-route leaf
only where the cutover forced it — the two observation capture sites named above.