Skip to content

Commit fea40eb

Browse files
thymikeethymikee
committed
fix(daemon): revert-sensitive transport tests, route-derived lease skip, guarded provider-scope resolve
Review round 3 on #1389: 1. The integration acceptance test passed with the interactor transport param removed — the request-boundary scope was routing for it. Tests now run in two worlds: the shared-stack and per-request-id tests use a world WITHOUT getAppleRunnerProvider (the interactor param is the only seam; verified failing when the param is dropped), while the direct-route test keeps the request scope it pins. 2. skipSessionlessProviderDevice for lease-route commands is now derived from daemon.route === 'lease' in shouldSkipSessionlessProviderDevice instead of hand-spread across four descriptors, with a registry-driven invariant test enumerating the route. 3. resolveScopedProviderDevice catches resolveTargetDevice failures and returns undefined: provider-scope plumbing failing to find a device means 'no provider scope', never a failed request. Also collapses the duplicated provider-scope Proxy from android.ts and apple/interactor.ts into core/interactor-scope.ts, and restates the transport param's role (local-tooling partition + out-of-daemon scoping) in its doc comment.
1 parent e7c7c28 commit fea40eb

8 files changed

Lines changed: 110 additions & 69 deletions

File tree

‎src/core/command-descriptor/registry.ts‎

Lines changed: 4 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -90,13 +90,6 @@ const isShardedTestRequest = (req: DaemonRequest): boolean =>
9090
req.command === 'test' &&
9191
(typeof req.flags?.shardAll === 'number' || typeof req.flags?.shardSplit === 'number');
9292

93-
// Lease-route requests manage lease lifecycle/artifacts, not a device session:
94-
// resolving a default device for provider scoping would spuriously trigger
95-
// local device discovery before any lease exists.
96-
const LEASE_ROUTE_NO_PROVIDER_DEVICE = {
97-
skipSessionlessProviderDevice: () => true,
98-
} as const;
99-
10093
// ADR 0014 request-sensitive ref-frame resolvers. The action is the leading
10194
// positional (see keyboard/alert daemon writers in src/commands/system/index.ts
10295
// and src/commands/capture/alert.ts). Only the read-only status probes preserve
@@ -304,12 +297,7 @@ export const RAW_COMMAND_DESCRIPTORS = [
304297
...(ownerFilesEnabled ? { ownerFiles: ['src/daemon/handlers/lease.ts'] as const } : {}),
305298
catalog: { group: 'internal', key: 'leaseAllocate' },
306299
recordsSessionAction: false,
307-
daemon: {
308-
route: 'lease',
309-
refFrameEffect: 'preserve',
310-
...ADMISSION_AND_LOCK_EXEMPT,
311-
...LEASE_ROUTE_NO_PROVIDER_DEVICE,
312-
},
300+
daemon: { route: 'lease', refFrameEffect: 'preserve', ...ADMISSION_AND_LOCK_EXEMPT },
313301
timeoutPolicy: DEFAULT_TIMEOUT_POLICY,
314302
batchable: false,
315303
},
@@ -318,12 +306,7 @@ export const RAW_COMMAND_DESCRIPTORS = [
318306
...(ownerFilesEnabled ? { ownerFiles: ['src/daemon/handlers/lease.ts'] as const } : {}),
319307
catalog: { group: 'internal', key: 'leaseHeartbeat' },
320308
recordsSessionAction: false,
321-
daemon: {
322-
route: 'lease',
323-
refFrameEffect: 'preserve',
324-
...ADMISSION_AND_LOCK_EXEMPT,
325-
...LEASE_ROUTE_NO_PROVIDER_DEVICE,
326-
},
309+
daemon: { route: 'lease', refFrameEffect: 'preserve', ...ADMISSION_AND_LOCK_EXEMPT },
327310
timeoutPolicy: DEFAULT_TIMEOUT_POLICY,
328311
batchable: false,
329312
},
@@ -332,12 +315,7 @@ export const RAW_COMMAND_DESCRIPTORS = [
332315
...(ownerFilesEnabled ? { ownerFiles: ['src/daemon/handlers/lease.ts'] as const } : {}),
333316
catalog: { group: 'internal', key: 'leaseRelease' },
334317
recordsSessionAction: false,
335-
daemon: {
336-
route: 'lease',
337-
refFrameEffect: 'preserve',
338-
...ADMISSION_AND_LOCK_EXEMPT,
339-
...LEASE_ROUTE_NO_PROVIDER_DEVICE,
340-
},
318+
daemon: { route: 'lease', refFrameEffect: 'preserve', ...ADMISSION_AND_LOCK_EXEMPT },
341319
timeoutPolicy: DEFAULT_TIMEOUT_POLICY,
342320
batchable: false,
343321
},
@@ -346,12 +324,7 @@ export const RAW_COMMAND_DESCRIPTORS = [
346324
...(ownerFilesEnabled ? { ownerFiles: ['src/commands/management/artifacts.ts'] as const } : {}),
347325
catalog: { group: 'public' },
348326
recordsSessionAction: false,
349-
daemon: {
350-
route: 'lease',
351-
refFrameEffect: 'preserve',
352-
...ADMISSION_AND_LOCK_EXEMPT,
353-
...LEASE_ROUTE_NO_PROVIDER_DEVICE,
354-
},
327+
daemon: { route: 'lease', refFrameEffect: 'preserve', ...ADMISSION_AND_LOCK_EXEMPT },
355328
timeoutPolicy: DEFAULT_TIMEOUT_POLICY,
356329
batchable: false,
357330
},

‎src/core/interactor-scope.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
import type { Interactor } from './interactor-types.ts';
2+
3+
/**
4+
* Wraps every interactor method call in a provider transport scope, so leaf
5+
* code below the method resolves the injected transport instead of the local
6+
* default. Shared by the Android adb-provider and Apple runner-provider seams.
7+
*/
8+
export function withProviderScopedInteractor(
9+
interactor: Interactor,
10+
runScope: <T>(task: () => Promise<T>) => Promise<T>,
11+
): Interactor {
12+
return new Proxy(interactor, {
13+
get(target, property, receiver) {
14+
const value = Reflect.get(target, property, receiver);
15+
if (typeof value !== 'function') return value;
16+
return (...args: unknown[]) => runScope(async () => await value.apply(target, args));
17+
},
18+
});
19+
}

‎src/core/interactors/android.ts‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import { snapshotAndroid } from '../../platforms/android/snapshot.ts';
3333
import { screenshotAndroid } from '../../platforms/android/screenshot.ts';
3434
import { withDiagnosticTimer } from '../../utils/diagnostics.ts';
3535
import type { DeviceInfo } from '../../kernel/device.ts';
36+
import { withProviderScopedInteractor } from '../interactor-scope.ts';
3637
import type { Interactor } from '../interactor-types.ts';
3738
import { snapshotCaptureAnnotationsFrom } from '../../snapshot-capture-annotations.ts';
3839

@@ -98,16 +99,7 @@ export function createAndroidInteractor(
9899
setAndroidSetting(device, setting, state, appId, options),
99100
};
100101
if (!provider) return interactor;
101-
return new Proxy(interactor, {
102-
get(target, property, receiver) {
103-
const value = Reflect.get(target, property, receiver);
104-
if (typeof value !== 'function') return value;
105-
return (...args: unknown[]) =>
106-
withAndroidAdbProvider(
107-
provider,
108-
{ serial: device.id },
109-
async () => await value.apply(target, args),
110-
);
111-
},
112-
});
102+
return withProviderScopedInteractor(interactor, (task) =>
103+
withAndroidAdbProvider(provider, { serial: device.id }, task),
104+
);
113105
}

‎src/daemon/__tests__/daemon-command-registry.test.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,27 @@ test('daemon command registry preserves provider device resolution traits', () =
216216
);
217217
});
218218

219+
test('every lease-route command skips sessionless provider-device resolution', () => {
220+
// Enumerated from the registry route, not a hand list: lease-route requests
221+
// manage lease lifecycle/artifacts, never a device session, so sessionless
222+
// provider scoping must not trigger local device discovery for them.
223+
const leaseRouteCommands = [
224+
...Object.values(PUBLIC_COMMANDS),
225+
...Object.values(INTERNAL_COMMANDS),
226+
].filter((command) => getDaemonCommandRoute(command) === 'lease');
227+
assert.ok(leaseRouteCommands.length >= 4, 'expected the lease route to enumerate its commands');
228+
for (const command of leaseRouteCommands) {
229+
assert.equal(
230+
resolveProviderDeviceResolutionIntent(makeRequest(command), {
231+
hasExistingSession: false,
232+
hasExplicitDeviceSelector: true,
233+
}),
234+
'skip',
235+
`${command} must not resolve a provider device sessionless`,
236+
);
237+
}
238+
});
239+
219240
function makeRequest(command: string, positionals: string[] = []): DaemonRequest {
220241
return {
221242
command,

‎src/daemon/daemon-command-registry.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,12 @@ function buildDaemonCommandRegistry(descriptors: readonly DaemonCommandDescripto
156156
}
157157

158158
function shouldSkipSessionlessProviderDevice(req: DaemonRequest): boolean {
159-
const skip = getDaemonCommandDescriptor(req.command)?.skipSessionlessProviderDevice;
159+
const descriptor = getDaemonCommandDescriptor(req.command);
160+
// Lease-route requests manage lease lifecycle/artifacts, not a device
161+
// session: resolving a default device for provider scoping would spuriously
162+
// trigger local device discovery before any lease exists. Derived from the
163+
// route so a new lease-route command cannot regress it.
164+
if (descriptor?.route === 'lease') return true;
165+
const skip = descriptor?.skipSessionlessProviderDevice;
160166
return typeof skip === 'function' ? skip(req) : false;
161167
}

‎src/daemon/request-platform-providers.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,14 @@ async function resolveScopedProviderDevice(
350350
return existingSession?.device;
351351
case 'explicit-device':
352352
case 'sessionless-default-device':
353-
return await resolveTargetDevice(req.flags ?? {});
353+
// Provider-scope plumbing only: an unresolvable device means "no provider
354+
// scope for this request", never a failed request — the command's own
355+
// device resolution still reports its errors downstream.
356+
try {
357+
return await resolveTargetDevice(req.flags ?? {});
358+
} catch {
359+
return undefined;
360+
}
354361
case 'skip':
355362
return undefined;
356363
}

‎src/platforms/apple/interactor.ts‎

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import { toAppleTvRemoteButton } from '../../contracts/tv-remote.ts';
2121
import { withDiagnosticTimer } from '../../utils/diagnostics.ts';
2222
import { isMacOs, isTvOsDevice, type DeviceInfo } from '../../kernel/device.ts';
2323
import { AppError } from '../../kernel/errors.ts';
24+
import { withProviderScopedInteractor } from '../../core/interactor-scope.ts';
2425
import type { RawSnapshotNode } from '../../kernel/snapshot.ts';
2526
import type {
2627
Interactor,
@@ -167,10 +168,13 @@ export function createAppleInteractor(
167168
}
168169

169170
/**
170-
* Routes every runner-command method through the injected provider transport.
171-
* Methods backed by local Apple tooling (simctl/devicectl) have no
172-
* provider-neutral transport, so they fail fast until the provider session
173-
* composes its own implementation on top.
171+
* Partitions the interactor for an injected provider transport. Its unique
172+
* jobs are the local-tooling rejection and in-process scoping for interactors
173+
* composed OUTSIDE a daemon request (on daemon requests the request-boundary
174+
* `appleRunnerProvider` resolver scopes the same transport around everything):
175+
* runner-command methods run inside the provider scope, while methods backed
176+
* by local Apple tooling (simctl/devicectl) have no provider-neutral transport
177+
* and fail fast until the provider session composes its own on top.
174178
*/
175179
function withInjectedAppleRunnerTransport(
176180
device: DeviceInfo,
@@ -188,18 +192,13 @@ function withInjectedAppleRunnerTransport(
188192
writeClipboard: async () => rejectLocalAppleToolMethod('writeClipboard'),
189193
setSetting: async () => rejectLocalAppleToolMethod('setSetting'),
190194
};
191-
return new Proxy(providerInteractor, {
192-
get(target, property, receiver) {
193-
const value = Reflect.get(target, property, receiver);
194-
if (typeof value !== 'function') return value;
195-
return (...args: unknown[]) =>
196-
withAppleRunnerProvider(
197-
runnerProvider,
198-
{ deviceId: device.id, requestId: runnerContext.requestId },
199-
async () => await value.apply(target, args),
200-
);
201-
},
202-
});
195+
return withProviderScopedInteractor(providerInteractor, (task) =>
196+
withAppleRunnerProvider(
197+
runnerProvider,
198+
{ deviceId: device.id, requestId: runnerContext.requestId },
199+
task,
200+
),
201+
);
203202
}
204203

205204
function rejectLocalAppleToolMethod(method: string): never {

‎test/integration/provider-scenarios/provider-ios-runner-transport.test.ts‎

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,11 @@ type RunnerTransportCalls = { runner: RecordedRunnerCall[]; opens: number };
3737
// AppleRunnerProvider transport (plus its own `open`) reuses the SHARED Apple
3838
// interactor — selector resolution, tap, fill, and snapshot all arrive at the
3939
// provider transport as runner-protocol commands instead of local XCTest.
40+
// This world has NO request-boundary resolver, so the interactor's injected
41+
// transport is the only thing keeping runner traffic off the local runtime:
42+
// removing the createAppleInteractor provider param fails this test.
4043
test('provider-supplied Apple runner transport reuses the shared interactor stack', async () => {
41-
await withProviderScenarioResource(createRunnerTransportWorld, async ({ daemon, calls }) => {
44+
await withProviderScenarioResource(createInteractorSeamWorld, async ({ daemon, calls }) => {
4245
const lease = await allocateLease(daemon);
4346
const request = { flags: leaseFlags(lease.leaseId), meta: leaseMeta(lease.leaseId) };
4447

@@ -106,7 +109,7 @@ test('provider-supplied Apple runner transport reuses the shared interactor stac
106109
// local XCTest runtime — and that scope must preserve the request's id, since
107110
// scoped-provider resolution requires the scope and call requestIds to match.
108111
test('daemon direct runner routes reach the provider transport through the request scope', async () => {
109-
await withProviderScenarioResource(createRunnerTransportWorld, async ({ daemon, calls }) => {
112+
await withProviderScenarioResource(createRequestScopeWorld, async ({ daemon, calls }) => {
110113
const lease = await allocateLease(daemon);
111114
const flags = leaseFlags(lease.leaseId);
112115
assertRpcOk(
@@ -130,9 +133,10 @@ test('daemon direct runner routes reach the provider transport through the reque
130133

131134
// Per-request RunnerContext threading: the daemon builds the interactor per
132135
// request, so runner calls carry the request's id (cancellation/accounting)
133-
// instead of the construction-time context.
136+
// instead of the construction-time context. Runs without the request-boundary
137+
// resolver so the id can only arrive via getInteractor's runnerContext.
134138
test('provider transport runner calls carry the per-request id', async () => {
135-
await withProviderScenarioResource(createRunnerTransportWorld, async ({ daemon, calls }) => {
139+
await withProviderScenarioResource(createInteractorSeamWorld, async ({ daemon, calls }) => {
136140
const lease = await allocateLease(daemon);
137141
const flags = leaseFlags(lease.leaseId);
138142
const openMeta = { ...leaseMeta(lease.leaseId), requestId: 'req-open-1' };
@@ -163,9 +167,21 @@ async function allocateLease(
163167
return assertRpcOk<{ lease: DeviceLease }>(response).lease;
164168
}
165169

166-
async function createRunnerTransportWorld() {
170+
// The interactor seam alone: no getAppleRunnerProvider, so nothing scopes the
171+
// request — only the transport injected into createAppleInteractor routes.
172+
async function createInteractorSeamWorld() {
173+
return await createRunnerTransportWorld({ requestScope: false });
174+
}
175+
176+
// The request-boundary seam: getAppleRunnerProvider scopes the whole request,
177+
// covering runner commands issued outside interactor methods.
178+
async function createRequestScopeWorld() {
179+
return await createRunnerTransportWorld({ requestScope: true });
180+
}
181+
182+
async function createRunnerTransportWorld(options: { requestScope: boolean }) {
167183
const calls: RunnerTransportCalls = { runner: [], opens: 0 };
168-
const runtime = createProviderRuntime(calls);
184+
const runtime = createProviderRuntime(calls, options);
169185
const providers = createProviderDeviceRuntimeRequestProviders([runtime]);
170186
const daemon = await createProviderScenarioHarness({
171187
...providers,
@@ -181,7 +197,10 @@ async function createRunnerTransportWorld() {
181197
};
182198
}
183199

184-
function createProviderRuntime(calls: RunnerTransportCalls): ProviderDeviceRuntime {
200+
function createProviderRuntime(
201+
calls: RunnerTransportCalls,
202+
options: { requestScope: boolean },
203+
): ProviderDeviceRuntime {
185204
const transport: AppleRunnerProvider = {
186205
runCommand: async (_device, command, options) => {
187206
calls.runner.push({ command, options });
@@ -203,7 +222,12 @@ function createProviderRuntime(calls: RunnerTransportCalls): ProviderDeviceRunti
203222
device.id === DEVICE.id
204223
? createRunnerTransportInteractor(calls, transport, runnerContext)
205224
: undefined,
206-
getAppleRunnerProvider: (device) => (device.id === DEVICE.id ? transport : undefined),
225+
...(options.requestScope
226+
? {
227+
getAppleRunnerProvider: (device: DeviceInfo) =>
228+
device.id === DEVICE.id ? transport : undefined,
229+
}
230+
: {}),
207231
shutdown: async () => undefined,
208232
};
209233
}

0 commit comments

Comments
 (0)