Conversation
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
Reviewed at 8e894a0.
The proxy lease path now sends Could the mismatch be made unrepresentable instead of tolerated? If I read the lease-state code but didn't execute it, so whether the remote daemon then serves the leased iOS device or fails some other way under a forwarded All 19 CI checks pass, with no failing job to attribute. The recorded-leaf fix needs to cover every lease-binding site, not only the proxy one, so a family-recorded non-proxy connection refuses a different leaf, and a live proxy-lease iOS install/open run needs to land alongside it before this is ready to merge. |
…onnection A proxy lease refused every iOS `install` and `open`: the resolved device wrote its internal `apple` platform into the request flags while the connection state held the public `ios`, so the scope check saw two platforms and demanded `connect --force`. Android passed only because its internal and public names are both `android`. Project a resolved device onto the axis a remote connection already records it on (`platform`, `target`, `deviceKey`, `leaseBackend`), in the module that declares that record, so those fields can never disagree about which axis a device was named on. Resolve each rule the projection needs at its owner rather than restating it: - `platformSelectorsConflict` joins the selector vocabulary in the kernel. It is the rule the daemon's request-lock policy already needed -- `apple` and a leaf name overlapping devices while `ios` and `macos` do not -- and answering "different platform?" with string equality is exactly the shape of this bug. `connect` now asks the same question the same way, so `--platform apple` stops reading as a second connection. - `leaseBackendForPlatform` joins `LeaseBackend` beside the enum, replacing the table `resolveRequestedLeaseBackend` kept by hand and the per-device copy in the projection. - `deviceIdentityFlag` was already the kernel's answer for a mismatched `--udid`/`--serial`; a lease request that re-issues a resolved device has to name the same flag or the two drift and it binds a selector resolving a DIFFERENT device. Its `--` prefix moves to the hint that renders it. A platform no lease backend rents (macOS desktop, Vega) names no identity flag: the command fails on the missing backend, which is the real problem, instead of on a `--udid` the daemon reads as iOS-family-only and reports as a conflict against the session being opened. `install_from_source` compared `--platform` against the session device's internal platform, which the projected flags now make load-bearing: it refused an iOS session by its public name and printed the internal `apple` token the public axis must not emit. It asks the device whether the selector names it, which is what `matchesPlatformSelector` is for.
Five boundaries, each failing for its own reason: an iOS-bound connection accepting its own leaf, an iOS-bound connection accepting the `apple` family selector, the same comparison with no device resolution in the way, a genuinely different platform still being refused, and the resolved device's public platform surviving into the state the NEXT command reads back. The last one is what #2962 actually reported: the first command of a session succeeded and the second was refused by state the first had written. These live in their own file because `remote-connection.test.ts` is already past the test-file size tripwire and may not grow (docs/agents/testing.md).
…message text Keying the assertion on /different platform/ reads the error string, which AGENTS.md forbids. The throw site already carries the bound session and platform in typed details, which also pin WHICH scope the guard refused on.
…s bound A connection opened with `--platform apple` records that alias before any device exists. resolveProxyLeaseState already re-keyed deviceKey and leaseBackend from the resolved device while leaving platform on the family axis, so the record named a family while its own deviceKey named one machine. The scope guard answers family-vs-leaf as no-conflict, so a later `--platform macos` walked past it and beat the iOS device's lease under a selector naming a different machine. Binding a device is the moment the family is decided: write the leaf the deviceKey speaks.
The collapse added for the proxy binding was written inside resolveProxyLeaseState, so it only covered the policy that resolves a device itself. The default, cloud-webdriver, and deferred policies record their state through buildMaterializedLeaseState, which kept whatever selector the command was asked with: a connection opened as `connect --platform apple --lease-backend ios-instance` held an `ios-instance` lease while its own record said `apple`, the scope guard answered family-vs-leaf as no-conflict, and a later `--platform macos` went out as `apple` against the iOS device's lease instead of being refused. The rule now runs once, right where the lease backend is settled and before the allocate payload is built, so the record, the flags the request carries, and the lease request all name the leaf the backend rents. The proxy path keeps its own source — the resolved device — and the special case is gone. Two readers still face records that were never rewritten: a state whose lease already matched was never rebuilt, and one saved by an older binary still says `apple` on disk. Both ask one shared predicate now, which is also what `connect` uses to decide whether a connection is reusable — previously the family alias made `--platform macos` look like the connection it wasn't. The selector rule alone answers family-vs-leaf as a match, which stays right for a selection and wrong for a decided record, so both readings live in that one function. The backend-to-leaf map is the inverse of the platform-to-backend map and sits beside it, so the two axes cannot drift apart. A backend that names no platform (`ios-simulator`, a runner guard) and a connection with no backend keep the alias: nothing has decided the family there, and inventing a leaf is the same axis mistake pointed the other way. Four tests, each with its own kill: the default-policy bind records, requests, and allocates as `ios` then refuses `macos`; a stored `apple` record refuses the other leaf and never touches the lease; `connect` refuses to reuse an `apple`-bound connection for the other leaf; and the predicate's own boundaries sit beside its module.
8e894a0 to
cc4c340
Compare
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/kernel/src/contracts.ts">
<violation number="1" location="packages/kernel/src/contracts.ts:101">
P2: Guard this lookup against inherited keys. `readRemoteConnectionState` accepts any string for `leaseBackend`, so `constructor` or `toString` returns a non-platform value that `boundConnectionPlatform` can forward as a platform.</violation>
</file>
<file name="src/cli/commands/connection-runtime.ts">
<violation number="1" location="src/cli/commands/connection-runtime.ts:335">
P3: This added `nextFlags.platform` assignment has no effect: `allocateOrReuseLease` never changes `nextState.platform` (its payload uses `state.platform ?? flags.platform` on the already-collapsed state), and the same expression is assigned again right after the lease call. Drop this line and keep the later assignment.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| }; | ||
|
|
||
| export function platformForLeaseBackend(backend: LeaseBackend): PublicPlatform | undefined { | ||
| return PLATFORM_BY_LEASE_BACKEND[backend]; |
There was a problem hiding this comment.
P2: Guard this lookup against inherited keys. readRemoteConnectionState accepts any string for leaseBackend, so constructor or toString returns a non-platform value that boundConnectionPlatform can forward as a platform.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/kernel/src/contracts.ts, line 101:
<comment>Guard this lookup against inherited keys. `readRemoteConnectionState` accepts any string for `leaseBackend`, so `constructor` or `toString` returns a non-platform value that `boundConnectionPlatform` can forward as a platform.</comment>
<file context>
@@ -83,6 +83,24 @@ export function leaseBackendForPlatform(
+};
+
+export function platformForLeaseBackend(backend: LeaseBackend): PublicPlatform | undefined {
+ return PLATFORM_BY_LEASE_BACKEND[backend];
+}
+
</file context>
| return PLATFORM_BY_LEASE_BACKEND[backend]; | |
| return Object.hasOwn(PLATFORM_BY_LEASE_BACKEND, backend) | |
| ? PLATFORM_BY_LEASE_BACKEND[backend] | |
| : undefined; |
| leaseBackend, | ||
| }), | ||
| }; | ||
| nextFlags.platform = nextState.platform ?? nextFlags.platform; |
There was a problem hiding this comment.
P3: This added nextFlags.platform assignment has no effect: allocateOrReuseLease never changes nextState.platform (its payload uses state.platform ?? flags.platform on the already-collapsed state), and the same expression is assigned again right after the lease call. Drop this line and keep the later assignment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/commands/connection-runtime.ts, line 335:
<comment>This added `nextFlags.platform` assignment has no effect: `allocateOrReuseLease` never changes `nextState.platform` (its payload uses `state.platform ?? flags.platform` on the already-collapsed state), and the same expression is assigned again right after the lease call. Drop this line and keep the later assignment.</comment>
<file context>
@@ -321,6 +322,17 @@ async function materializeLeaseForCommand(options: {
+ leaseBackend,
+ }),
+ };
+ nextFlags.platform = nextState.platform ?? nextFlags.platform;
const materializedLease = await allocateOrReuseLease(
client,
</file context>
|
This is a follow-up on the earlier review at 8e894a0 (#2989 (comment)). At cc4c340 the code still has the same class of problem, just moved.
Not blocking: CI's Smoke Tests failure is a local iOS simulator fixture run that never reaches the remote connection code this PR touches, and #2990 fails with the same signature, so it looks unrelated to this change. I didn't run any tests myself; the analysis above comes from reading the pre- and post-delta code paths, I didn't verify that any remote provider actually hands out an |
This branch has not been deployed
1 participant
Summary
A proxy lease refused every iOS
installandopen:applyResolvedDeviceSelectorwrote the device's internalappleplatform into the request flags while the connection state recorded the public leafios, soassertRequestedConnectionScopesaw two platforms and demandedconnect --force. Android passed only because its internal and public names are bothandroid.A resolved device now projects onto the axis a connection already records it on (
platform,target,deviceKey,leaseBackend) inresolveConnectionDeviceScope, inside the module that declares that record. Each rule it composes resolves at its owner:platformSelectorsConflict(kernel, also replacingconnect's string-equality check and the daemon's request-lock copy),leaseBackendForPlatform, and the now-exporteddeviceIdentityFlag.install_from_sourcecompared--platformagainst the session device's internal platform and echoed theappletoken back; it asks the device whether the selector names it.Closes #2962
11 files, 172 net production lines. Three deferred sites are named in code comments and below; each differs from the shared rule in a way that is its own decision.
Validation
Commit
c68540940.pnpm check:affected --runpassed: 934 files / 8,172 tests, and wire compat reports the protocol unchanged.check:layering,lint,format,typecheckpass. 11 new tests, each mutation-verified (dropping the leaf write fails 3; reverting the conflict rule fails 3; narrowing the identity flag fails its own test).No live device run: the repro needs a remote lease provider.
Deferred:
limrun-profile.ts:66backend copy,device-claim-conflict.ts:53anddevice-selection-resolver.ts:242flag copies,session-selector.ts:64.