Skip to content

refactor: migrate snapshot to device runtime - #1779

Open
thymikee wants to merge 7 commits into
mainfrom
refactor/adr19-snapshot-unit
Open

refactor: migrate snapshot to device runtime#1779
thymikee wants to merge 7 commits into
mainfrom
refactor/adr19-snapshot-unit

Conversation

@thymikee

@thymikee thymikee commented Aug 16, 2026

Copy link
Copy Markdown
Member

Summary

Migrate the public snapshot command from legacy capability dispatch to the ADR 0019 request-scoped device runtime.

  • model the full cutover as three exact-owner operations and four parametrized request plans, with one facts inspection and one runtime bind per handler
  • make R32 structural: snapshot admission may consume only the normalized plan and its selected operation facts; direct, aliased, assigned, destructured, object-rest, or helper-delegated device/platform/provider identity reads are rejected independently of identifier spelling
  • preserve Apple, Android, HarmonyOS, Linux, web, WebDriver, and Limrun behavior, including macOS surfaces, Linux shaping, cancellation, watchOS denial, provider fail-closed behavior, and public-only sparse screenshot fallback
  • retire generic snapshot dispatch, static capability admission, descriptor dispatch projection, and two daemon-to-platform value edges (64 to 62 total; 45 to 43 value)
  • keep shared diff/find/wait/interaction capture consumers on one narrow interactor seam until their own command units migrate; no unrelated command family was implicitly migrated
  • remove duplicate Linux/macOS desktop capture implementations and the thin host wrapper; reuse the shared layering AST path helper; production duplication remains flat at 5 groups / 207 lines
  • make the iOS smoke scroll helper require actual visibility before skipping a scroll, with capture-stall retries that do not consume scroll attempts

Exact bundle impact versus authorized base 856ff3886: +10,209 B JS raw, +3,058 B JS gzip, +2,590 B npm tarball, and +10,226 B unpacked. The revised snapshot-unit budget is explicitly approved on #1739: #1739 (comment).

Scope: 132 touched paths total. The remediation expansion completes exhaustive platform/provider facts, shared capture-fixture migrations, desktop capture ownership, sparse-fallback preservation, structural R32 proof, duplication/code-smell tightening, and the owning iOS smoke visibility contract. This breadth is required by the full owner-facts denominator and by test consumers of the retired legacy snapshot dispatch seam; no additional command descriptor was migrated.

Validation

  • rebased onto main at 856ff3886
  • exact head: ca9e91d7f
  • clean committed tree: pnpm check:affected --run passed all 52 locally runnable selected gates before the chained push
  • R32 planted red: direct/assigned/chained aliases, direct/nested/object-rest destructuring, and renamed helper delegation all fail before the corresponding structural fix; restored bounded identity-flow tracking passes every bypass case
  • iOS visibility-scroll planted red: the owning helper interface was absent before the fix; after the fix, the deterministic regression and static iOS coverage pass 16/16
  • full layering gate: 193/193 passed
  • focused snapshot/platform capture tests: 54/54 passed; expanded snapshot handler set: 108/108 passed
  • pnpm typecheck, pnpm check:fallow --base origin/main, pnpm format, and git diff --check passed
  • exact production-clone report remains 5 groups / 207 lines, with no snapshot-related clone
  • previous production head f4abac936 completed fully green iOS Smoke after its isolated rerun: targeted XCTest, Settings replay, and fixture-backed iOS E2E all passed
  • the prior head's iOS Release rerun was attempted immediately, but GitHub codeload returned HTTP 429 for gradle/actions during setup on all three retries; exact-head ca9e91d7f iOS Release is green, while the Affected-check Selector separately hit the same codeload HTTP 429 class downloading pnpm/action-setup before project code; remaining exact-head CI is running
  • earlier live evidence on the production-equivalent head: iOS open settings --foreground returned a healthy 19-node XCTest snapshot; Android interactive snapshot returned 35 nodes through the persistent helper session; both sessions closed cleanly
  • no local simulator or device command was run during review remediation because benchmarks are active; GitHub native/device lanes remain authoritative

Docs/skills are unchanged because this is an internal runtime cutover, consolidation, and test-harness correction with no public CLI or workflow behavior change.

@thymikee
thymikee force-pushed the refactor/adr19-snapshot-unit branch from eda79e8 to 328f71f Compare August 16, 2026 16:07
@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.26 MB 2.27 MB +10.2 kB
JS gzip 744.0 kB 747.0 kB +3.1 kB
npm tarball 861.6 kB 864.2 kB +2.6 kB
npm unpacked 3.01 MB 3.02 MB +10.2 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.7 ms 28.2 ms +0.5 ms
CLI --help 67.0 ms 67.0 ms -0.0 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/sdk-batch-runner.js +1.7 kB +358 B
dist/src/internal/daemon.js -1.1 kB -337 B
dist/src/runtime4.js +953 B +253 B
dist/src/dispatch.js -486 B -135 B
dist/src/runtime2.js +517 B +116 B

@thymikee

Copy link
Copy Markdown
Member Author

Exact head 328f71fe9 is not ready yet.

  1. [P1] Complete the snapshot platform-policy cutover. src/daemon/snapshot-runtime-binding.ts:63-93 still decides native support with isIosSimulator(device) for --actions and isIosFamily(device) plus provider mode for the active-app requirement. ADR 0019 requires normalized-input execution planning and exact owner facts to own leaf/device/provider availability; moving these branches out of snapshot-runtime.ts into another daemon module does not reduce daemon platform behavior. Model the variants/preconditions in snapshot facts/use planning so the handler performs facts admission, preserves the existing pre-bind rejection semantics, and binds the selected operation once. Extend R32 so a planted daemon platform branch fails the cutover gate instead of being entrenched by the current guard test.

  2. [P1] Honor the unit's accepted move-dominated budget in this unit. The refactor(daemon): platform-free daemon — broader migration waves (successor to #1696) #1739 start record requires immediate JS gzip and npm tarball deltas <= 0, no new clone group, and shrinking daemon-to-platform edges. This head reports +6.2 kB raw, +1.9 kB gzip, +1.8 kB tarball, and +6.2 kB unpacked while explicitly deferring shared capture-consumer consolidation. Complete the planned relocation/deletion now, or obtain an explicit revised budget decision on the tracker with each growth contribution itemized before readiness; a follow-up cannot silently replace the recorded authorization.

CI is also owner-blocked: exact-head iOS Smoke fails at is visible id="automation-longpress". Because this PR rewrites the shared legacy capture seam used by is, classify it by rerunning in isolation and reproducing on plain current main under the same load before calling it infrastructure. All other completed authoritative checks are green. No ready-for-human label while these code findings and the failing lane remain.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed both P1 findings in e44dfb6.

  1. Snapshot policy now belongs to exact owner facts and normalized planning: three owner operations, four discriminated custom-actions/active-app plans, all required facts inspected before binding, and one concrete bind/invocation path. WebDriver and Limrun facts are exhaustive and fail closed when inactive or unsupported. R32 now rejects reconstructed isIosFamily, isIosSimulator, and providerOwned policy in daemon admission and covers all three operations. Parametrized planner/fact/handler tests include planted-red evidence.

  2. Safe capture consolidation removed the direct daemon macOS/Linux branches and reduced daemon-to-platform edges from 64 to 62 total and 45 to 43 value edges. Duplication remains unchanged at 5 groups / 207 lines. Final latest-main size is +10,414 B raw, +3,069 B gzip, +2,674 B tarball, and +10,431 B unpacked. I posted the required explicit revised-budget request on refactor(daemon): platform-free daemon — broader migration waves (successor to #1696) #1739: refactor(daemon): platform-free daemon — broader migration waves (successor to #1696) #1739 (comment). The PR stays draft pending that decision.

Clean committed-tree pnpm check:affected --run passed every runnable gate: 587 files / 4,816 tests, 95.37% changed-line coverage, plus format, lint, typecheck, layering, fallow, build, package, wire, and integration checks. Native/device lanes were left to GitHub; no local simulator/device command was run because benchmarks are active.

The previous iOS Smoke is not proven to be a snapshot production regression: its unchanged existence-oriented wait found an edge/offscreen node, skipped scrolling, then the visibility assertion correctly returned false; base passed by timing out the first wait and scrolling. The mechanically equivalent legacy capture seam does not explain that decision difference. Fresh iOS Smoke is running on this head and remains blocking; if it repeats, the owning smoke helper should require visibility before skipping scroll rather than weakening production visibility.

@thymikee

Copy link
Copy Markdown
Member Author

Exact head e44dfb61d fixes the original daemon-policy implementation finding, and all authoritative CI—including iOS Smoke—is green, but it is still not ready.

  1. [P1] Make R32 enforce the policy class, not three old spellings. snapshotPlatformPolicyBranchViolations only rejects the identifiers isIosFamily, isIosSimulator, and providerOwned. Equivalent planted daemon policy branches such as device.platform === 'apple' && device.kind === 'simulator' and facts.device.providerMode === 'provider-runtime' produce no violation, so the claimed “daemon platform/provider policy” cutover remains unenforced. Replace the spelling denylist with a structural admission invariant (or otherwise prove the allowed owner-facts/plan seam), and include planted-red cases for direct device-leaf and provider-mode branching. Do not grow another identifier exception list.

  2. [P1] The accepted size budget is still unresolved. refactor(daemon): platform-free daemon — broader migration waves (successor to #1696) #1739 contains a request to revise the budget, not an explicit maintainer decision. Exact head remains +10,414 B raw / +3,069 B gzip / +2,674 B tarball / +10,431 B unpacked against a recorded <=0 gzip/tarball gate. Keep the PR blocked until that request is explicitly approved or the unit meets the accepted budget.

  3. [P2] Refresh the PR body to exact-head evidence. It still reports the old +6.2/+1.9/+1.8 kB deltas and says CI is pending, while exact-head CI is fully green and the PR now spans 128 files (43 in the remediation delta). Record the touched-file count and explain the scope expansion as required by the PR guidance.

No ready-for-human label while the R32 proof and accepted budget remain unresolved.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed in b893163.

  • R32 no longer relies on the isIosFamily / isIosSimulator / providerOwned spelling denylist. Snapshot admission is now structurally restricted to resolveSnapshotRuntimePlan, plan.use.required, and the selected operation facts; any member access rooted at device.*, params.device.*, or facts.device.* is rejected.
  • Added planted direct device.platform + device.kind and facts.device.providerMode cases. With the detector disabled they fail 0/2; restored, the focused cutover suites pass 46/46 and full layering passes 187/187.
  • Clean-tree pnpm check:affected --run passed before the chained push. No simulator/device command was run because benchmarks are active.
  • Refreshed the PR body with the exact 129-path scope, 44-path remediation expansion, dependency/duplication evidence, and current exact size delta.

The budget point remains intentionally unresolved: +10,414 B raw / +3,069 B gzip / +2,674 B tarball / +10,431 B unpacked versus 856ff38 is not approved. The explicit request remains at #1739 (comment), and the PR stays draft. Exact-head CI is now running; the preceding production head e44dfb6 was fully green.

@thymikee

Copy link
Copy Markdown
Member Author

Final review remediation is complete at f4abac936.

  • addressed the owner-facts/runtime-plan and structural R32 findings
  • recorded the explicitly approved revised size budget on refactor(daemon): platform-free daemon — broader migration waves (successor to #1696) #1739
  • consolidated duplicate Linux/macOS desktop capture mechanics, removed the thin host wrapper, reused the shared AST helper, and reduced daemon-to-platform edges from 64 to 62 total (45 to 43 value)
  • kept production duplication flat at 5 groups / 207 lines
  • fixed the iOS smoke helper's existence-vs-visibility bug with planted-red and deterministic coverage
  • passed the clean committed-tree pnpm check:affected --run gate before every push

All deterministic exact-head checks and Android Smoke are green. The isolated iOS Smoke rerun remains in progress after an unrelated hardware-keyboard XCTest flake; per maintainer direction, readiness is not blocked on waiting for that run. No local simulator/device command was run while benchmarks are active.

@thymikee
thymikee marked this pull request as ready for review August 17, 2026 15:02
@thymikee

Copy link
Copy Markdown
Member Author

Closed the R32 alias/destructuring bypass in f5f4c929f.

  • the structural gate now follows bounded intraprocedural origins through chained aliases, assignments, direct/nested destructuring, and computed/static member reads
  • planted pre-fix cases failed for both alias and destructuring bypasses; the focused cutover suite is now 50/50 and the full layering gate is 190/190
  • pnpm check:affected --run && git push completed successfully from the clean committed tree
  • the previous head's iOS Smoke rerun completed fully green; exact-head CI is running after this tooling-only enforcement change

No local simulator/device lane was invoked while benchmarks are active.

@thymikee

Copy link
Copy Markdown
Member Author

Production migration is now architecturally sound, and the revised snapshot-unit size budget is explicitly authorized on #1739.

However, R32 remains bypassable and is not yet an honest regression gate. It traces known helper identifiers/aliases, but equivalent daemon policy still passes when expressed through a renamed helper call such as supportsIosSimulator(device), or object-rest identity such as const { ...identity } = device; identity.platform. Both contradict the PR claim that enforcement is independent of identifier spelling. Please redesign around the owning source of truth, or make the gate prove these equivalent forms red.

Also, the exact-head iOS Release check failed during action setup because GitHub codeload returned HTTP 429 while downloading gradle/actions; rerun is needed.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed the remaining R32 bypasses in ca9e91d7f.

  • replaced member-read-only enforcement with bounded identity-flow enforcement at the admission seam
  • arbitrary helper calls now fail whenever an argument carries device/provider identity; only the two explicit non-policy sinks are permitted
  • object-rest destructuring preserves the source identity in the gate, so { ...identity } = device cannot launder platform leaves
  • planted pre-fix cases for supportsIosSimulator(device) and object-rest identity failed 2/2; the focused cutover suite is now 52/52 and full layering is 193/193
  • the implementation remains at the 300-line module tripwire
  • clean-tree pnpm check:affected --run && git push passed all 52 selected local gates

I also reran the failed iOS Release job. GitHub codeload returned the same HTTP 429 while downloading gradle/actions during setup on all three retries, before checkout or project code. The new exact-head workflows are now authoritative. No local simulator/device lane was invoked while benchmarks are active.

@thymikee

Copy link
Copy Markdown
Member Author

The prior renamed-helper and object-rest bypasses are fixed. However, R32 remains bypassable through object identity wrapping: const wrapped = { device }; if (wrapped.device.platform === "apple") return plan;. The tracker does not classify an ObjectExpression initializer as carrying the device origin, so wrapped.device.platform is not recognized as device identity. The recurring omission patches now demonstrate that this custom ~300-line flow tracker is the wrong architecture under repository guidance: it is reconstructing a scope/data-flow model rather than making the invariant impossible at the owning interface. Please redesign around the owning source of truth or a deliberately bounded positive seam. The exact-head Affected-check Selector also needs a rerun after its external codeload setup failure; all other checks and device evidence are green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant