diff --git a/src/cli-schema/cli-help-topics.test.ts b/src/cli-schema/cli-help-topics.test.ts index fcd23401a..35d5f1c0a 100644 --- a/src/cli-schema/cli-help-topics.test.ts +++ b/src/cli-schema/cli-help-topics.test.ts @@ -138,7 +138,7 @@ test('usageForCommand resolves workflow help topic', async () => { assert.match(help, /Shapes and platform quirks: help gestures/); assert.match( help, - /open --foreground -> snapshot\. Selection: explicit --device\/--udid\/--serial, then session, booted\/bootable local, or one provider; --platform\/--target only filter\./, + /open --foreground -> snapshot\. Selection: explicit --device\/--udid\/--serial, then session, booted\/bootable local, or provider; --platform\/--target only filter\./, ); assert.match(help, /Never open artifact paths or invent package ids/); assert.match( diff --git a/src/cli-schema/cli-help.ts b/src/cli-schema/cli-help.ts index 2da57dc22..e1d70f63e 100644 --- a/src/cli-schema/cli-help.ts +++ b/src/cli-schema/cli-help.ts @@ -134,7 +134,7 @@ Command shape: Gestures: scroll/swipe for lists/flicks; gesture pan|fling|pinch|rotate|transform|drag for multi-touch. Shapes and platform quirks: help gestures. Bootstrap: - open --foreground -> snapshot. Selection: explicit --device/--udid/--serial, then session, booted/bootable local, or one provider; --platform/--target only filter. Ambiguous/empty fails with bounded retry selectors; providers never fall back. + open --foreground -> snapshot. Selection: explicit --device/--udid/--serial, then session, booted/bootable local, or provider; --platform/--target only filter. Ambiguous/empty fails with bounded retry selectors; no provider fallback. Install arguments are app/package id then artifact path: agent-device install com.example.app ./dist/app.apk --platform android, then open --relaunch for fresh state. Use reinstall only when explicitly requested. Unknown app id: devices, then apps, then open . Never open artifact paths or invent package ids; ask if lookup misses the target. Apple CI: prepare ios-runner after boot/install, before replay/test (help prepare). Remote/cloud: connect -> open -> commands -> close -> disconnect (help remote). Reusable scripts, secret-safe fills, replay repair: help scripting. diff --git a/src/core/command-descriptor/registry.ts b/src/core/command-descriptor/registry.ts index 093feb946..62d1523a6 100644 --- a/src/core/command-descriptor/registry.ts +++ b/src/core/command-descriptor/registry.ts @@ -170,6 +170,14 @@ const isShardedTestRequest = (req: DispatchedCommand): boolean => req.command === 'test' && (typeof req.flags?.shardAll === 'number' || typeof req.flags?.shardSplit === 'number'); +// #2016: a plain `close` (no app-target positional) has nothing to close via +// flags, so it's the only close shape eligible for the sessionless +// no-lease-anywhere admission bypass in request-admission.ts. `close ` +// resolves its device straight from flags when there's no session and must +// stay behind full lease/tenant admission. +const isPlainCloseRequest = (req: DispatchedCommand): boolean => + (req.positionals?.length ?? 0) === 0; + // ADR 0014 request-sensitive ref-frame resolvers. The action is the leading // positional (see keyboard/alert daemon writers in src/commands/system/index.ts // and src/commands/capture/alert.ts). Only the read-only status probes preserve @@ -922,6 +930,7 @@ export const RAW_COMMAND_DESCRIPTORS = [ refFrameEffect: 'may-invalidate', allowInvalidRecording: true, saveScriptFlagOwner: true, + sessionlessPlainCloseAdmissionExempt: isPlainCloseRequest, }, timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, diff --git a/src/daemon/__tests__/request-admission.test.ts b/src/daemon/__tests__/request-admission.test.ts new file mode 100644 index 000000000..9cbe7c1b5 --- /dev/null +++ b/src/daemon/__tests__/request-admission.test.ts @@ -0,0 +1,149 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import { makeIosSession } from '../../__tests__/test-utils/session-factories.ts'; +import { LeaseRegistry } from '../lease-registry.ts'; +import { assertRequestLeaseAdmission } from '../request-admission.ts'; +import type { DaemonRequest } from '../types.ts'; + +function makeRequest(overrides: Partial = {}): DaemonRequest { + return { + token: 'token', + session: 'default', + command: 'close', + positionals: [], + flags: {}, + ...overrides, + }; +} + +// #2016: a tenant-isolated connection (e.g. BrowserStack) whose lease was +// never allocated (`open` was never called) must not surface the generic +// tenant/run/lease admission error when `close` tries to clean it up. +test('close on a tenant-isolated session with no lease ever allocated admits without throwing', () => { + const registry = new LeaseRegistry(); + + const result = assertRequestLeaseAdmission( + makeRequest({ + command: 'close', + meta: { tenantId: 'tenant-a', runId: 'run-1', sessionIsolation: 'tenant' }, + }), + registry, + undefined, + ); + + assert.equal(result, undefined); +}); + +test('close on a lease-less but stored tenant-isolated session still requires a lease id', () => { + // #2016 follow-up: a *stored* session under tenant isolation is keyed by + // tenant, not by run, so a lease-less stored session could belong to + // another run in the same tenant. The bypass must not treat "this session + // record has no lease field" as proof there's nothing to protect — only + // "no session record exists at all" (the actual deferred-connect case) + // qualifies. + const registry = new LeaseRegistry(); + const session = makeIosSession('default'); + + assert.throws( + () => + assertRequestLeaseAdmission( + makeRequest({ + command: 'close', + meta: { tenantId: 'tenant-a', runId: 'run-1', sessionIsolation: 'tenant' }, + }), + registry, + session, + ), + /tenant isolation requires lease id/, + ); +}); + +test('close with an app target still requires a lease id even with no lease anywhere', () => { + // #2016 follow-up: `close ` with no session resolves its device + // straight from flags (`closeWithoutSession`), so it must not bypass + // tenant/lease admission the way a plain `close` (nothing to close) does — + // otherwise any caller could close an arbitrary flag-selected device on a + // tenant-isolated fleet without ever presenting a lease. + const registry = new LeaseRegistry(); + + assert.throws( + () => + assertRequestLeaseAdmission( + makeRequest({ + command: 'close', + positionals: ['com.example.app'], + meta: { tenantId: 'tenant-a', runId: 'run-1', sessionIsolation: 'tenant' }, + }), + registry, + undefined, + ), + /tenant isolation requires lease id/, + ); +}); + +test('close with an explicit lease id but no session lease still requires an active lease', () => { + const registry = new LeaseRegistry(); + + assert.throws( + () => + assertRequestLeaseAdmission( + makeRequest({ + command: 'close', + meta: { + tenantId: 'tenant-a', + runId: 'run-1', + leaseId: 'a'.repeat(32), + sessionIsolation: 'tenant', + }, + }), + registry, + undefined, + ), + /Lease is not active/, + ); +}); + +test('non-close commands on a tenant-isolated session still require a lease id', () => { + const registry = new LeaseRegistry(); + + assert.throws( + () => + assertRequestLeaseAdmission( + makeRequest({ + command: 'snapshot', + meta: { tenantId: 'tenant-a', runId: 'run-1', sessionIsolation: 'tenant' }, + }), + registry, + undefined, + ), + /tenant isolation requires lease id/, + ); +}); + +test('close still admits and heartbeats a real active lease', () => { + let now = 1_000; + const registry = new LeaseRegistry({ now: () => now }); + const lease = registry.allocateLease({ tenantId: 'tenant-a', runId: 'run-1' }); + const session = makeIosSession('default', { + lease: { + leaseId: lease.leaseId, + tenantId: lease.tenantId, + runId: lease.runId, + leaseBackend: lease.backend, + expiresAt: lease.expiresAt, + }, + }); + now = 2_000; + + const result = assertRequestLeaseAdmission( + makeRequest({ + command: 'close', + meta: { tenantId: 'tenant-a', runId: 'run-1', sessionIsolation: 'tenant' }, + }), + registry, + session, + ); + + assert.equal(result?.leaseId, lease.leaseId); + assert.equal(result?.heartbeatAt, 2_000); +}); diff --git a/src/daemon/__tests__/request-execution-scope.test.ts b/src/daemon/__tests__/request-execution-scope.test.ts index bea1818b6..21cf59635 100644 --- a/src/daemon/__tests__/request-execution-scope.test.ts +++ b/src/daemon/__tests__/request-execution-scope.test.ts @@ -1,4 +1,4 @@ -import { afterAll, test, expect } from 'vitest'; +import { afterAll, test, expect, vi } from 'vitest'; import fs from 'node:fs'; import path from 'node:path'; import { @@ -23,6 +23,7 @@ import { resolveSessionRequestLogPath } from '../session-store.ts'; import type { DaemonRequest } from '../types.ts'; import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; import { makeTestScreenRecordingResource } from '../../__tests__/test-utils/screen-recording-live-handle.ts'; +import { handleCloseCommand } from '../handlers/session-close.ts'; const TEST_ROOT = mkdtempForTestSync('agent-device-request-execution-scope-'); const LOG_PATH = path.join(TEST_ROOT, 'diagnostics.log'); @@ -754,6 +755,95 @@ test('runLocked rejects a request canceled while waiting for its execution lock' } }); +// #2016 router-level regression: a deferred remote connection (`connect` +// succeeded, `open` never ran, so the daemon never allocated a lease or +// created a session) reaches `close`'s own SESSION_NOT_FOUND outcome +// through the real tenant-scoping + locked-admission pipeline, rather than +// throwing the generic tenant-isolation error before `handleCloseCommand` +// ever runs. No provider is touched — there is nothing to release. +test('router: deferred tenant connect with no daemon session closes as SESSION_NOT_FOUND without touching the provider', async () => { + const sessionStore = makeSessionStore('agent-device-request-scope-'); + const leaseRegistry = new LeaseRegistry(); + const release = vi.fn(async () => ({})); + + const scope = await createRequestExecutionScope({ + req: makeRequest({ + session: 'default', + command: 'close', + meta: { tenantId: 'tenant-a', runId: 'run-1', sessionIsolation: 'tenant' }, + }), + sessionStore, + leaseRegistry, + }); + expect(scope.sessionName).toBe('tenant-a:default'); + + const response = await scope.runLocked(async () => + handleCloseCommand({ + req: scope.req, + sessionName: scope.sessionName, + logPath: scope.requestLogPath, + sessionStore, + leaseRegistry, + leaseLifecycleProvider: { release }, + }), + ); + + expect(response.ok).toBe(false); + if (!response.ok) { + expect(response.error.code).toBe('SESSION_NOT_FOUND'); + } + expect(release).not.toHaveBeenCalled(); +}); + +// The same deferred connection, but with an app-target `close `. This +// must not reach `handleCloseCommand` at all — an app-target close with no +// session resolves its device straight from flags, so it stays behind full +// lease/tenant admission (the router rejects it before dispatch). +test('router: deferred tenant connect still refuses an app-target close before dispatch', async () => { + const sessionStore = makeSessionStore('agent-device-request-scope-'); + const leaseRegistry = new LeaseRegistry(); + + const scope = await createRequestExecutionScope({ + req: makeRequest({ + session: 'default', + command: 'close', + positionals: ['com.example.app'], + meta: { tenantId: 'tenant-a', runId: 'run-1', sessionIsolation: 'tenant' }, + }), + sessionStore, + leaseRegistry, + }); + + await expect(scope.runLocked(async () => 'unreachable')).rejects.toThrow( + /tenant isolation requires lease id/, + ); +}); + +// A stored session that already exists under this tenant-scoped name but +// happens to carry no lease must still be refused: it could belong to a +// different run in the same tenant (sessions are tenant-scoped, not +// run-scoped), so a missing lease field alone is not proof of ownership. +test('router: an existing lease-less session under tenant isolation still refuses close', async () => { + const sessionStore = makeSessionStore('agent-device-request-scope-'); + sessionStore.set('tenant-a:default', makeIosSession('tenant-a:default')); + const leaseRegistry = new LeaseRegistry(); + + const scope = await createRequestExecutionScope({ + req: makeRequest({ + session: 'default', + command: 'close', + meta: { tenantId: 'tenant-a', runId: 'run-1', sessionIsolation: 'tenant' }, + }), + sessionStore, + leaseRegistry, + }); + expect(scope.sessionName).toBe('tenant-a:default'); + + await expect(scope.runLocked(async () => 'unreachable')).rejects.toThrow( + /tenant isolation requires lease id/, + ); +}); + function makeRequest(overrides: Partial = {}): DaemonRequest { return { token: 'test-token', diff --git a/src/daemon/daemon-command-registry.ts b/src/daemon/daemon-command-registry.ts index 0e76ca935..0229520bb 100644 --- a/src/daemon/daemon-command-registry.ts +++ b/src/daemon/daemon-command-registry.ts @@ -44,6 +44,17 @@ export type DaemonCommandDescriptor = { preferExplicitDeviceOverExistingSession?: boolean; allowSessionlessDefaultDevice?: (req: DaemonRequest) => boolean; skipSessionlessProviderDevice?: (req: DaemonRequest) => boolean; + /** + * #2016: this request shape is eligible for the sessionless, + * no-lease-anywhere lease-admission bypass — a session that was never + * created (deferred `connect`, `open` never ran) has no lease to admit or + * release. Only `close` declares it, and only for the plain-close shape + * (no app-target positional): `close ` resolves its device straight + * from flags when there's no session, so it must stay behind full + * lease/tenant admission. Declared here so `request-admission.ts` asks the + * registry instead of reclassifying `req.command`/`req.positionals` itself. + */ + sessionlessPlainCloseAdmissionExempt?: (req: DaemonRequest) => boolean; }; export type DaemonProviderDeviceResolutionIntent = @@ -121,6 +132,12 @@ export function usesSessionlessDefaultProviderDevice(req: DaemonRequest): boolea return typeof allow === 'function' ? allow(req) : false; } +/** #2016: whether this request qualifies for the sessionless plain-close lease-admission bypass. */ +export function isSessionlessPlainCloseAdmissionExempt(req: DaemonRequest): boolean { + const exempt = getDaemonCommandDescriptor(req.command)?.sessionlessPlainCloseAdmissionExempt; + return typeof exempt === 'function' ? exempt(req) : false; +} + /** * ADR 0014: the ref-frame effect a request resolves to, honoring the * request-sensitive resolver form. Returns `undefined` for commands with no diff --git a/src/daemon/request-admission.ts b/src/daemon/request-admission.ts index a4f302173..655350751 100644 --- a/src/daemon/request-admission.ts +++ b/src/daemon/request-admission.ts @@ -1,7 +1,10 @@ import { AppError } from '@agent-device/kernel/errors'; import { normalizeTenantId, resolveSessionIsolationMode } from './config.ts'; import { isTenantOwnedSessionName, tenantScopedSessionName } from './session-tenant-scope.ts'; -import { isLeaseAdmissionExempt } from './daemon-command-registry.ts'; +import { + isLeaseAdmissionExempt, + isSessionlessPlainCloseAdmissionExempt, +} from './daemon-command-registry.ts'; import { DEFAULT_PROXY_LEASE_TTL_MS, findMissingProxyLeaseFields, @@ -69,6 +72,25 @@ export function assertRequestLeaseAdmission( const requestLeaseScope = resolveLeaseScope(req); assertProxyOpenLeaseMetadata(req, requestLeaseScope); const sessionLease = session?.lease; + // #2016: a tenant-isolated connection that never reached `open` has no + // daemon session and no lease to admit or release. Falling through would + // make the generic tenant/run/lease check below throw "tenant isolation + // requires lease id.", which reads as an access-control failure instead of + // "nothing to close". Let the close handler's own session lookup return + // its SESSION_NOT_FOUND response instead. Requires `session === undefined`, + // not just a lease-less session: a *stored* session under tenant isolation + // is keyed by tenant, not by run, so a lease-less stored session could + // belong to another run in the same tenant — admission must still verify a + // matching lease before that run's session can be torn down. Which request + // shape qualifies (plain `close`, not an app-target `close `) is the + // registry's call, not this module's — see `sessionlessPlainCloseAdmissionExempt`. + if ( + session === undefined && + !requestLeaseScope.leaseId && + isSessionlessPlainCloseAdmissionExempt(req) + ) { + return undefined; + } if (!sessionLease && req.meta?.sessionIsolation !== 'tenant') { if (!requestLeaseScope.leaseId) return undefined; if (!requestLeaseScope.tenantId && !requestLeaseScope.runId) return undefined;