fix(apple): refuse fold on a scoped simulator set at runtime admission - #2887
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at 4b87e09. The code looks good: the scoped-set refusal sits at the One question before the label: the fold Not blocking: the test at I did not run the tests or repeat the live scoped iPhone Duo run; that reported run is the evidence that a scoped session carries The seven checks that were not passing were still running at review time, with no failures yet. Analyze (java-kotlin) does not touch this TypeScript-only diff. |
4b87e09 to
6643e1e
Compare
|
Rebased onto latest A vs B (fold Test at Repeated rationale: collapsed the code-side copy to the single owning |
|
Follow-up The
No route's wire shape changes; |
|
Closing the one remaining non-blocking nit (repeated rationale) in No open review threads or new comments, and CI is green on the head. |
|
This follows up on the earlier review (4b87e09, evidence-pending). At 2976bab, the fold refusal itself looks right; one duplication issue remains.
No live scoped-simulator fold run ("iPhone Duo" with The diff only touches |
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Thermo-nuclear structural pass. The Apple-side fix is right and I'd take it as-is: refusal in exactly one owning fact, precedence as ordered guards (kind -> leaf -> scope) pinned by a test that scope does not outrank kind/OS, typed reason end-to-end with a real parser round-trip (narrowDeviceBinding -> requireRuntimeFact -> normalizeRuntimeFact), and no display probe / helper build / HID on the refused path. simulatorSetPath truthiness matches the kernel's own scoped-set convention, and unsupported-device-scope is handled by both consumers with no silent fall-through.
The only thing I would not merge as-is is the daemon half: this PR extracts a shared refusal builder while a byte-identical sibling that already carries the reason stays live and serves more routes — so the UNSUPPORTED_OPERATION wire shape is still seam-dependent. Two inline (the second mostly dissolves once the first is taken).
Two NITs (not inline):
simulator-hid.test.tsstill asserts a scopedsendSimulatorFoldPosepath that production can no longer reach —setAppleFoldPoseis only bound throughwhenAdmitted, which now refuses a scoped device before dispatch. The test is still a valid statement aboutrunSimctlForDevicescoping, but a reader who lands there first concludes scoped fold is partially supported. Retire it or add a line naming the owning fact that refuses the scope, so the layer's capability and the operation's admission are cross-linked rather than silently contradicting.fold-runtime.test.ts's scoped case hard-codes a paraphrase of the production hint sentence ("fold cannot resolve a simulator scoped to the set at ..."). Assert propagation instead of echoing wording (hint: expect.stringContaining('/tmp/scoped-set'), or a sentinel the route could never produce) so a real hint edit doesn't invite someone to "fix" the fake.
fold on an iPhone Duo inside --ios-simulator-device-set failed after the HID send because CoreDevice's display inventory and hinge-angle readback resolve a scoped simulator as not found, so ADR 0025's post-dispatch verification is impossible. Refuse it instead at the owning setFoldPose fact when device.simulatorSetPath is present, with the typed unsupported-device-scope reason; the routed response derives the same reason and a hint naming the set. The default set and the existing kind/OS refusals are unchanged.
Move the zero-side-effect proof onto the real fold route: a scoped setFoldPose fact is refused at admission, so the wire error carries the typed unsupported-device-scope reason and the set name and the device is never bound. Refocus the colocated narrowDeviceBinding test to what it proves, and collapse the repeated CoreDevice-scope rationale to one owning comment.
The fold override restated the same UNSUPPORTED_OPERATION + '<command> is not supported on this device' + hint the admission default already built, only to add details.reason. Own that refusal once in admitRuntimeOperations and reuse it: the default is byte-identical, fold passes its typed reason as details, and no other route's wire shape changes.
The terse fold command description restated the same CoreDevice-scope rationale the deep help topic and commands.md carry. The description now states only the refusal, its typed reason, and the fix; the why lives on the surfaces a user opens to read more.
…ime seams build
The generic route and the request-scoped session handlers each owned a
byte-identical UNSUPPORTED_OPERATION refusal, but only the session seam
attached details.reason, so the shared default emitted the same sentence
with a different wire shape and fold had to re-add the reason through a
custom unavailableResponse. unsupportedOperationResponse now always attaches
details: { reason } and unavailableRuntimeOperationResponse delegates to it,
so one builder owns sentence + reason + hint for both seams. fold drops its
override and its second 'fold' mention; conformance asserts the single shape.
Nits: cross-link the scoped HID transport test to the owning fact that
refuses the scope, and assert hint propagation with a sentinel instead of
echoing production wording.
2976bab to
f9a85fe
Compare
|
Addressed the structural pass in Inline (both) — one refusal builder now owns sentence + typed
NITs:
The Android job still fails inside |
|
Reviewed at f9a85fe. The earlier point is fixed: the fold refusal details now go through the shared daemon error-response builder, and I found nothing new to fix. All checks pass, and there are no conflicts. |
thymikee
added
the
ready-for-human
1 participant
Summary
foldon an iPhone Duo scoped with--ios-simulator-device-setreached HID dispatch, then died at CoreDevice's display inventory:devicectl device info displaysanddevicectl device motion hinge-angleaccept only--deviceand resolve a scoped simulator as not found, so ADR 0025's post-dispatch readback can never verify the pose.Refuse it instead at the owning
setFoldPosefact wheneverdevice.simulatorSetPathis present, with a new typed reasonunsupported-device-scopeadded toRuntimeOperationUnavailabilityand its parser. The routedfoldresponse derives its code/message/hint/reason from that same fact, so capability and response cannot diverge. Existing kind/OS refusals and default-set behavior are unchanged; no display probe,simctl spawn, or helper build runs.9 files (+167; ~30 production lines, rest tests/help/docs). Closes #2871
Validation
Commits
693fd7ee3(fix) +6643e1e13(review follow-up), rebased on72862063c.pnpm check:affected --run: all runnable checks pass.pnpm typecheckclean.Focused:
src/daemon/__tests__/fold-runtime.test.tsadds a scoped-DeviceInforoute case (resolveBoundFoldRuntime→ wire error carriesdetails.reason: unsupported-device-scope+ the set name,bindDevice/setFoldPosenever called); newfoldable/runtime.test.ts(fact, capability, no pose op bound, narrowed use carries the typed reason, and the existing kind/OS refusals are not outranked) plus an owner-level scoped-fact case inruntime.test.ts. Planted-failure check: the scoped tests fail without the fact guard.Live (Xcode 27.1, throwaway iPhone Duo in a temp set): scoped
fold half-open --jsonreturnedUNSUPPORTED_OPERATION/unsupported-device-scopenaming the set in ~1s (a real fold is 10-16s). The request log recorded onlyrequest_start/request_failed(noapple_fold_pose_dispatched, helper build, or HID), anddevicectl device motion hinge-angleon the scoped sim returns "The specified device was not found" — corroborating the un-verifiable pose. Throwaway sim + set deleted, session closed.