refactor: type-safe recording backends + exhaustive capability gating#894
Merged
Conversation
Captures the target architecture (two-registry thesis: CommandDescriptor + PlatformPlugin over a clean folder DAG with a typed-result spine) and a sequenced, strangler-fig migration path, grounded in a survey of the current codebase. This PR implements the first two behaviorless Phase-0 items from that roadmap; the larger registry work is deliberately deferred to later, independently shippable PRs.
RecordingBackend is now generic over the recording's platform tag, so each
backend's stop() receives an already-narrowed recording. This deletes all five
'recording as Extract<ActiveRecording, { platform: ... }>' casts — the textbook
discriminated-union-narrowing-by-cast anti-pattern — and makes a backend/tag
mismatch unrepresentable.
start() stays wide (DaemonResponse | ActiveRecording) because a device platform
does not map 1:1 to a recording tag (an iOS device resolves to either the 'ios' or
'ios-device-runner' recording). Device resolution returns a stop-less view
(RecordingStartBackend); stop is dispatched per active recording via the new
exhaustive stopActiveRecording(), replacing resolveRecordingBackendForRecording().
Behaviorless: pure type-level change, no runtime behavior change.
isCommandSupportedOnDevice resolved the per-platform capability bucket with an if/else ladder whose final branch funneled every unmatched platform into capability.web. That silently absorbs a future Platform with no compile error. Replace it with selectCapabilityForPlatform(), an exhaustive switch over the Platform union with a 'never' guard, so adding a new platform is a compile error here instead of a silent web mis-gate. Identical behavior for all five current platforms (ios/macos -> apple, android, linux, web).
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
Member
Author
|
Review pass on 96282aa: I do not see a blocker. Checked the changed production paths: recording start still resolves by device through a stop-less backend view, recording stop now dispatches exhaustively by the active recording tag, and capability bucket selection is now an exhaustive platform switch with identical current-platform behavior. CI is green, including typecheck, lint/format, unit, integration, smoke, Fallow, and iOS runner compatibility. Residual risk is low because this is type-level/routing cleanup with no device-facing behavior change. |
Ratifies the PR review caveat into the ADR itself: the daemon command registry boundary is about ownership + the predicate interface, not the physical file a trait is typed in. A derived/projected daemon registry is permitted only if it preserves four invariants (daemon-owned declaration, unchanged predicate interface, no leakage into public projections, one declaration per concern enforced by types). The original decision stands; collapsing daemon policy into a public command registry remains forbidden.
- §2/§5.2: CommandDescriptor composes domain-owned facets (surface@commands, capability@core, daemon@src/daemon) and projects them — compose-with, not collapse-into. Adds the four ADR-0003 invariants. - §6: mark the two shipped Phase-0 items (generic RecordingBackend<P>, exhaustive capability selection); link the Apple plan from Phase 3. - §5.1: Apple as the first PlatformPlugin instance, owning an AppleOS leaf axis. - §8: before/after diagrams for the command axis + the two-axis summary.
One 'apple' Platform with an AppleOS discriminant (ios/ipados/tvos/watchos/ visionos/macos) rather than six Platform literals (which would collide with the cross-platform 'target' axis). Captures the 4-investigator survey: ~85% of platforms/ios is already the OS-agnostic Apple engine; the XCTest runner already builds ios|macos|tvos; macOS is included as a distinct AppKit leaf (already entangled). visionOS is scoped net-new work; watchOS is an unsupported sentinel (XCUITest can't drive it). Before/after diagrams, per-OS readiness, sequencing.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two behaviorless type-safety improvements — the first Phase-0 step of the architecture roadmap added in
plans/perfect-shape.md. Both make an invalid state a compile error instead of a latent runtime hazard, with zero runtime behavior change.1. Parametrize
RecordingBackendby recording tag (drops 5 unsound casts)RecordingBackend<P>is now generic over the recording'splatformtag, so each backend'sstop()receives an already-narrowed recording. This deletes all fiverecording as Extract<ActiveRecording, { platform: ... }>casts — the discriminated-union-narrowing-by-cast anti-pattern — and makes a backend/tag mismatch unrepresentable.start()stays wide (DaemonResponse | ActiveRecording) because a device platform doesn't map 1:1 to a recording tag (an iOS device resolves to either theiosorios-device-runnerrecording).RecordingStartBackend); stop is dispatched per active recording via a new exhaustivestopActiveRecording(), replacingresolveRecordingBackendForRecording().2. Make capability platform selection exhaustive
isCommandSupportedOnDeviceresolved the per-platform capability bucket with an if/else ladder whose final branch funneled every unmatched platform intocapability.web. That silently absorbs a futurePlatformwith no compile error. Replaced withselectCapabilityForPlatform()— an exhaustiveswitchover thePlatformunion with aneverguard — so adding a platform is a compile error here instead of a silent web mis-gate. Behavior is identical for all five current platforms (ios/macos→ apple,android,linux,web).Why now
plans/perfect-shape.md(included here) is a target-architecture roadmap. Its typed-result spine is a prerequisite for the later registry work, and these two changes are the cheapest, lowest-risk first dominoes: pure type-level wins that delete real unsoundness today.Validation
pnpm typecheck— passpnpm lint(oxlint --deny-warnings) — passpnpm format:check— passvitest run --project unit) — 278 files / 2729 tests passNet: −10 lines, 5 unsound casts removed, 2 new compile-time exhaustiveness guards.
Roadmap / governance docs (also in this PR)
Phase-0 sits under the roadmap in
plans/perfect-shape.md(already in this PR). This update aligns the docs with maintainer review:docs/adr/0003-daemon-command-registry.md— amended (not reversed) with a "single-declaration / derivation model" clause. Ratifies the review caveat: the futureCommandDescriptorwork must compose with the daemon-registry boundary (daemon facet owned undersrc/daemon/, predicate interface unchanged, no leakage, one declaration per concern), never collapse daemon policy into a public registry.plans/perfect-shape.md— command axis refined to facet composition honoring ADR 0003; the two shipped Phase-0 items marked; before/after diagrams added; Apple linked as the firstPlatformPlugininstance.plans/apple-platform-consolidation.md— new: consolidate Apple underplatforms/applewith anAppleOSleaf axis (oneapplePlatform, not six literals). Per-OS readiness + sequencing.These are docs-only (no source touched); the code scope of this PR is unchanged. Happy to split the docs into their own PR if preferred.