Skip to content

refactor: consolidate Apple platform internals#968

Merged
thymikee merged 1 commit into
mainfrom
refactor/apple-platform-consolidation
Jun 30, 2026
Merged

refactor: consolidate Apple platform internals#968
thymikee merged 1 commit into
mainfrom
refactor/apple-platform-consolidation

Conversation

@thymikee

@thymikee thymikee commented Jun 30, 2026

Copy link
Copy Markdown
Member

Summary

Move shared Apple-platform implementation into src/platforms/apple/core, retarget internal callers directly to those Apple modules, and split macOS leaf code under src/platforms/apple/os/macos.
Add visionOS runner profile/build-cache groundwork while keeping watchOS out of scope.
Remove the inert iOS runner request-count gate and its related runtime surface, including cost.runnerRoundTrips, because recent successful main runs captured zero runner round trips and the signal was not proving runner behavior.
Retire plans/apple-platform-consolidation.md; ADR-0009 now owns the decision and plans/phase3-platform-plugin-progress.md tracks the remaining follow-ups.

Rebased onto current main after #967 merged. Scope is 164 touched files across Apple platform internals, tests, runner build scripts, Fallow move metadata, CI cleanup, Xcode project metadata, request-count cleanup, and roadmap docs. Remaining Apple work is explicit follow-up: public Platform collapse, tvOS leaf, plugin facets, per-AppleOS capability table, and the watchOS unsupported sentinel.

Validation

pnpm format
pnpm check:tooling
pnpm check:quick
pnpm check:fallow --base origin/main
pnpm exec vitest run src/daemon/__tests__/request-router-cost.test.ts src/daemon/__tests__/request-router-response-level.test.ts
pnpm exec vitest run src/platforms/ios/__tests__/apple-runner-platform.test.ts src/platforms/ios/__tests__/runner-command-retry.test.ts src/platforms/ios/__tests__/runner-session.test.ts src/daemon/handlers/__tests__/session.test.ts
git diff --check
Previously on this worktree before the rebase: pnpm build, pnpm exec vitest run src/platforms/ios/__tests__, runner-focused Vitest suite, pnpm build:xcuitest, pnpm build:xcuitest:visionos, and git diff --check. visionOS runtime was installed and the XCUITest build passed, but no Vision Pro simulator instance existed for live interaction verification.

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-06-30 19:31 UTC

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.4 MB 1.4 MB -2.0 kB
JS gzip 450.4 kB 450.2 kB -187 B
npm tarball 549.4 kB 549.4 kB -13 B
npm unpacked 1.9 MB 1.9 MB -345 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.2 ms 27.3 ms +0.1 ms
CLI --help 47.4 ms 48.0 ms +0.6 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/cli.js -16.1 kB -5.1 kB
dist/src/generic.js +9.0 kB +3.5 kB
dist/src/495.js +5.7 kB +2.1 kB
dist/src/2948.js -9 B -82 B
dist/src/9722.js +268 B -28 B

@thymikee thymikee force-pushed the refactor/apple-platform-consolidation branch from 6e2e73e to 3162a65 Compare June 30, 2026 15:54
@thymikee

Copy link
Copy Markdown
Member Author

Review — actionable items

Verified the relocation is a clean byte-identical move (runner stack + ~46 apple/core files are import-path-only; all ios/* are one-line shims; no phase strings / command names / runner round-trip logic changed; tsc + lint + runner/apple unit tests pass; full-suite failures are only the known Android fillAndroid timeout flakes, which pass in isolation). Action items:

  1. Resolve the src/daemon/request-router.ts conflict with main toward the import. This PR reintroduces the local const RUNNER_ROUND_TRIP_PHASES = [...] that ci: automate iOS runner request-count gate for the Apple runner unwind (Phase 3 step c prep) #966 removed; main now does import { RUNNER_ROUND_TRIP_PHASES } from './runner-request-count.ts'. On retarget to main, keep main's import (single source of truth), drop the reintroduced local const.

  2. Arm the runner request-count gate so this PR is proven byte-identical, not just asserted. The baseline (scripts/runner-request-count/expected-counts.json) is still established: false, so the gate is inert. The relocation is gate-friendly (phase strings + skip logic moved unchanged; the gate counts by phase from daemon.log), so after rebasing onto main: arm the baseline from a pre-move count, then let smoke-ios confirm the count is unchanged.

  3. src/platforms/apple/core/apple-runner-platform.ts: confirm the disallowed SDK-list edits to existing rows are safe. The iOS/tvOS/macOS profile disallowed lists each gained 'xros'/'xrsimulator' — not additive. Verify this can't reject an xctestrun product those platforms previously accepted.

  4. src/kernel/device.ts: fix the stale comment. It still says watchOS/visionOS default to the iOS profile, which is no longer true after resolveRunnerPlatformNameForAppleOs added the visionos case.

  5. Rewire apple/core intra-package imports to siblings (follow-up OK). Moved files import their siblings via the old ios/ shims (e.g. apple/core/runner/runner-session.ts../../../ios/runner-contract.ts), so apple/core still depends backward on the layer it supersedes. Functional, but a tightening pass should point intra-core imports at siblings and keep ios/ shims only for external callers.

  6. Confirm the fix: respect prepare timeout for runner health checks #967 health-timeout change (session.ts resolvePrepareIosRunnerHealthTimeoutMs) is meant to land via this PR. It's a real runner health-timeout behavior change unrelated to the consolidation, riding along from the stacked base.

Base automatically changed from fix/prepare-runner-health-timeout to main June 30, 2026 16:27
@thymikee thymikee force-pushed the refactor/apple-platform-consolidation branch from 3162a65 to 8ca1ca5 Compare June 30, 2026 16:35
@thymikee

Copy link
Copy Markdown
Member Author

Addressed the review items after rebasing onto current main:

Request-count gate note: I did not arm scripts/runner-request-count/expected-counts.json with zeroes. The latest successful main iOS workflow (28455877600) logged runnerRoundTrips=0 and marked the gate inconclusive, so there is no usable pre-move non-zero baseline to commit from CI. Local validation for this update:

  • pnpm format
  • pnpm check:quick
  • pnpm check:fallow --base origin/main
  • pnpm exec vitest run src/platforms/ios/__tests__/apple-runner-platform.test.ts
  • pnpm exec vitest run src/platforms/ios/__tests__/runner-command-retry.test.ts src/platforms/ios/__tests__/runner-session.test.ts src/daemon/handlers/__tests__/session.test.ts

@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed the current clean #968 head (8ca1ca5) against the prior actionable items, plans/perfect-shape.md, and ADR-0003 boundaries.

The PR is now based on main; src/daemon/request-router.ts uses the mainline RUNNER_ROUND_TRIP_PHASES import, the #967 health-timeout change is no longer part of this branch, the src/kernel/device.ts visionOS/watchOS comment is corrected, and the existing iOS/tvOS/macOS xctestrun disallowed hints are preserved with a focused test while visionOS owns xros/xrsimulator. The remaining apple/core imports through iOS shim paths are the already-called-out follow-up tightening, not a behavior blocker.

The runner request-count gate is still not armed, but the follow-up note explains why: latest successful main iOS workflow had runnerRoundTrips=0, so there is no useful non-zero baseline to commit. Given the move keeps runner phase strings and request-router counting source on main, I would treat that as residual risk rather than blocking this consolidation.

Checks are green, including Typecheck, Unit Tests, Integration Tests, Smoke Tests, Fallow Code Quality, Lint & Format, Swift Runner Unit Compile, Web Platform Smoke, iOS Runner Swift Compatibility, and deploy-preview. I do not see an actionable blocker; ready for maintainer review.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jun 30, 2026
@thymikee thymikee force-pushed the refactor/apple-platform-consolidation branch from 8ca1ca5 to 8510875 Compare June 30, 2026 17:20
@thymikee

Copy link
Copy Markdown
Member Author

Follow-up on the request-count gate: removed it instead of trying to arm it with zero.

The latest two successful main iOS workflow runs both observed runnerRoundTrips=0 and marked the gate inconclusive, so the gate was not proving runner behavior. This update removes:

  • the iOS workflow Assert iOS runner request count step
  • validate:runner-count
  • scripts/runner-request-count/*
  • the harness-only src/daemon/runner-request-count.ts module and tests

The runtime cost.runnerRoundTrips response field remains intact and covered by focused request-router tests; only the broken CI harness is gone.

@thymikee thymikee force-pushed the refactor/apple-platform-consolidation branch from 8510875 to 415672e Compare June 30, 2026 17:29
@thymikee

Copy link
Copy Markdown
Member Author

Dropped the request-count gate and the related runtime surface instead of carrying a half-working signal.

What it was trying to catch: accidental runner behavior drift, mainly extra XCTest runner round trips per logical command, duplicate sends after fallback/retry changes, or a refactor accidentally bypassing the runner for an action that should use it.

Why I removed it: the latest successful main evidence captured runnerRoundTrips=0, so the gate could not distinguish good behavior from broken instrumentation. Keeping cost.runnerRoundTrips would expose an unreliable number in the daemon contract.

Replacement direction: rely on focused live e2e/smoke verification for the user-visible paths (open/snapshot/click/wait/close) plus request diagnostics artifacts when debugging performance. If we bring a count gate back, it should first prove non-zero runner events in a live e2e path and fail when capture is zero; otherwise it is only checking plumbing.

@thymikee

Copy link
Copy Markdown
Member Author

Head changed after the prior readiness review (now at 415672e), and the iOS smoke check is still in progress. I removed ready-for-human for now; please wait for checks to settle and re-review this head before restoring the label.

@thymikee thymikee removed the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jun 30, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed current clean head 415672e against plans/perfect-shape.md and ADR 0003.

The only delta since my prior clean review is removal of the runner request-count gate and cost.runnerRoundTrips surface. That cleanup is consistent: request-router no longer computes the metric, the public ResponseCost type and cost tests no longer expose it, package/workflow hooks are removed, and checks are green. This also avoids preserving an inconclusive gate whose recent main baseline reported zero runner round trips.

Residual risk remains as previously noted: the Apple core move still has compatibility shim imports through src/platforms/ios, and visionOS has build-cache groundwork without live Vision Pro simulator interaction evidence. I do not see an actionable blocker on this head; ready for maintainer review.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jun 30, 2026
@thymikee thymikee force-pushed the refactor/apple-platform-consolidation branch from 415672e to 44c76dd Compare June 30, 2026 18:17
@thymikee

Copy link
Copy Markdown
Member Author

Re-checked current clean head 44c76dd. The delta from the previously reviewed 415672e head is docs/plans only: ADR 0009 now records the shipped/deferred Apple consolidation status, the obsolete standalone Apple plan was removed, and perfect-shape/phase3 progress links now point at ADR 0009 plus phase3-platform-plugin-progress.md.

No code, routing, runner, command-surface, or platform backend behavior changed in this head. Checks remain green, and the existing ready-for-human label is still appropriate.

@thymikee thymikee force-pushed the refactor/apple-platform-consolidation branch from 44c76dd to 171f4d0 Compare June 30, 2026 19:17
@thymikee thymikee merged commit 26ac865 into main Jun 30, 2026
22 checks passed
@thymikee thymikee deleted the refactor/apple-platform-consolidation branch June 30, 2026 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant