diff --git a/src/__tests__/cli-grammar.test.ts b/src/__tests__/cli-grammar.test.ts index d99195d15..f50e92a14 100644 --- a/src/__tests__/cli-grammar.test.ts +++ b/src/__tests__/cli-grammar.test.ts @@ -1,7 +1,6 @@ import { test } from 'vitest'; import assert from 'node:assert/strict'; import { readInputFromCli } from '../commands/cli-grammar.ts'; -import type { CommandName } from '../commands/command-metadata.ts'; import type { CliFlags } from '../commands/cli-grammar/flag-types.ts'; const BASE_FLAGS: CliFlags = { @@ -139,33 +138,6 @@ test('find and is grammar decodes command action positionals', () => { assert.equal(isOptions.value, 'Welcome'); }); -// --no-record is accepted on every command (COMMON_COMMAND_SUPPORTED_FLAG_KEYS), -// but it only reaches the daemon if the reader forwards it, so a reader that -// never names it accepts the flag and silently ignores it. Cover every command -// whose actions are recorded into a session script. -test('readers forward --no-record instead of silently dropping it', () => { - const recordable: Array<[CommandName, string[]]> = [ - ['press', ['@e5']], - ['click', ['@e5']], - ['fill', ['id=email', 'qa@example.com']], - ['longpress', ['@e5']], - ['swipe', ['up']], - ['focus', ['10', '20']], - ['type', ['hello']], - ['scroll', ['down']], - ['get', ['attrs', '@e5']], - ['is', ['visible', 'id=title']], - ['find', ['label', 'Continue', 'exists']], - ['snapshot', []], - ['wait', ['Continue']], - ]; - - for (const [command, positionals] of recordable) { - const options = readInputFromCli(command, positionals, { ...BASE_FLAGS, noRecord: true }); - assert.equal(options.noRecord, true, `${command} dropped --no-record`); - } -}); - test('readers omit noRecord entirely when the flag is absent', () => { const options = readInputFromCli('press', ['@e5'], BASE_FLAGS); assert.equal(Object.hasOwn(options, 'noRecord'), false); diff --git a/src/__tests__/cli-record-flag-delivery.test.ts b/src/__tests__/cli-record-flag-delivery.test.ts new file mode 100644 index 000000000..5fde1c95d --- /dev/null +++ b/src/__tests__/cli-record-flag-delivery.test.ts @@ -0,0 +1,139 @@ +/** + * `--no-record` / `--record` delivery, asserted at the DAEMON REQUEST — the + * layer that is actually observable. + * + * #1304/#1305 asserted on `readInputFromCli` output instead, and shipped green + * while the flag was inert: the reader's object is an intermediate that two + * later layers rebuild from scratch. `readFieldInput` keeps only declared + * metadata fields plus `readCommonInput`'s output, and each `to*Options` + * projection rebuilds the client options from `commonToClientOptions` plus its + * own named fields. A flag dropped at either one never reaches the daemon, so + * an assertion upstream of both cannot see the bug it is meant to catch. + * + * These tests therefore drive the REAL chain a user's argv takes — + * parseArgs -> readInputFromCli -> runCommand -> client -> transport — and + * assert on the daemon request's flags. + */ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import { createAgentDeviceClient } from '../agent-device-client.ts'; +import type { AgentDeviceClient } from '../client/client-types.ts'; +import { parseArgs } from '../cli/parser/args.ts'; +import { runCliCommandWithOutput } from '../commands/cli-runner.ts'; +import type { CommandName } from '../commands/command-metadata.ts'; + +type SeenRequest = { command: string; noRecord?: boolean; record?: boolean }; + +/** Runs a full argv line and returns the daemon requests it produced. */ +async function runArgv(argv: string[]): Promise { + const seen: SeenRequest[] = []; + const client: AgentDeviceClient = createAgentDeviceClient( + {}, + { + transport: async (req) => { + seen.push({ + command: req.command, + noRecord: req.flags?.noRecord === true ? true : undefined, + record: req.flags?.record === true ? true : undefined, + }); + return { ok: true, data: {} } as never; + }, + }, + ); + const parsed = parseArgs(argv, { strictFlags: true }); + try { + await runCliCommandWithOutput({ + client, + command: argv[0] as CommandName, + positionals: parsed.positionals, + flags: parsed.flags, + }); + } catch { + // Result normalizers reject the stub's empty payload for some commands; the + // request was already captured by then, which is all these tests assert on. + } + return seen; +} + +// Every recordable route, including the ones #1305 hand-listed (which were inert +// end-to-end) and the ones it missed entirely (gesture/back/home + the generic +// and session routes). Positionals are the minimum each reader parses. +const RECORDABLE_ARGV: Array<[CommandName, string[]]> = [ + ['press', ['press', '10', '20']], + ['click', ['click', '10', '20']], + ['fill', ['fill', 'id=email', 'qa@example.com']], + ['longpress', ['longpress', '10', '20']], + ['swipe', ['swipe', '0', '0', '10', '10']], + ['focus', ['focus', '10', '20']], + ['type', ['type', 'hello']], + ['scroll', ['scroll', 'down']], + ['get', ['get', 'attrs', '@e5']], + ['is', ['is', 'visible', 'id=title']], + ['find', ['find', 'label', 'Continue', 'exists']], + ['snapshot', ['snapshot']], + ['wait', ['wait', 'Continue']], + // Missed by #1305 entirely: + ['gesture', ['gesture', 'fling', 'up', '100', '200']], + ['back', ['back']], + ['home', ['home']], + ['app-switcher', ['app-switcher']], + ['orientation', ['orientation', 'landscape-left']], + ['keyboard', ['keyboard', 'dismiss']], + ['clipboard', ['clipboard', 'read']], + ['tv-remote', ['tv-remote', 'select']], + ['alert', ['alert', 'accept']], + ['settings', ['settings', 'wifi', 'on']], + ['screenshot', ['screenshot']], + ['viewport', ['viewport', '100', '200']], + ['open', ['open', 'App']], + ['push', ['push', '/a', '/b']], + ['trigger-app-event', ['trigger-app-event', 'evt']], + ['record', ['record', 'start']], + ['trace', ['trace', 'start', '/tmp/t.log']], + ['perf', ['perf', 'metrics']], + ['react-native', ['react-native', 'dismiss-overlay']], +]; + +test('--no-record reaches the daemon request for every recordable command', async () => { + for (const [command, argv] of RECORDABLE_ARGV) { + const seen = await runArgv([...argv, '--no-record']); + assert.ok(seen.length > 0, `${command} produced no daemon request`); + assert.ok( + seen.some((req) => req.noRecord === true), + `${command} accepted --no-record but never delivered it to the daemon`, + ); + } +}); + +test('--no-record is absent from the daemon request when the flag is not passed', async () => { + const seen = await runArgv(['press', '10', '20']); + assert.equal(seen[0]?.noRecord, undefined); +}); + +// The deliberate asymmetry (ADR 0012 decision 6 amendment): --no-record applies +// to every recordable command and rides the common seam; --record is scoped to +// the observation-only commands the repair-segment exclusion can drop, so it +// must NOT become common as a side effect of the --no-record seam fix. +test('--record reaches the daemon only for the observation-only commands that accept it', async () => { + for (const argv of [ + ['get', 'attrs', '@e5'], + ['is', 'visible', 'id=title'], + ['snapshot'], + ['find', 'label', 'Continue', 'exists'], + ]) { + const seen = await runArgv([...argv, '--record']); + assert.ok( + seen.some((req) => req.record === true), + `${argv[0]} accepted --record but never delivered it to the daemon`, + ); + } +}); + +test('--record stays scoped: the CLI rejects it on a mutating command', () => { + // Grammar-level refusal, before projection: `record` is not in press's + // allowedFlags, so it must not parse as a silently-ignored flag. + assert.throws( + () => parseArgs(['press', '10', '20', '--record'], { strictFlags: true }), + /--record/, + ); +}); diff --git a/src/commands/capture/snapshot.ts b/src/commands/capture/snapshot.ts index d41b2b42b..1fd028422 100644 --- a/src/commands/capture/snapshot.ts +++ b/src/commands/capture/snapshot.ts @@ -5,7 +5,6 @@ import { defineExecutableCommand } from '../command-contract.ts'; import { commonInputFromFlags, direct, - noRecordInputFromFlags, observationRecordInputFromFlags, } from '../cli-grammar/common.ts'; import type { CliReader, DaemonWriter } from '../cli-grammar/types.ts'; @@ -54,7 +53,6 @@ const snapshotCliSchema = { export const snapshotCliReader: CliReader = (_positionals, flags) => ({ ...commonInputFromFlags(flags), - ...noRecordInputFromFlags(flags), ...observationRecordInputFromFlags(flags), interactiveOnly: flags.snapshotInteractiveOnly, depth: flags.snapshotDepth, diff --git a/src/commands/capture/wait.ts b/src/commands/capture/wait.ts index eabe07a03..32a3fe2bf 100644 --- a/src/commands/capture/wait.ts +++ b/src/commands/capture/wait.ts @@ -16,7 +16,6 @@ import { defineExecutableCommand } from '../command-contract.ts'; import { direct, optionalNumber, - noRecordInputFromFlags, selectionOptionsFromFlags, selectorSnapshotOptionsFromFlags, } from '../cli-grammar/common.ts'; @@ -93,7 +92,6 @@ function readWaitOptionsFromPositionals( const base = { ...selectionOptionsFromFlags(flags), ...selectorSnapshotOptionsFromFlags(flags), - ...noRecordInputFromFlags(flags), }; if (parsed.kind === 'sleep') return { ...base, durationMs: parsed.durationMs }; if (parsed.kind === 'text') { diff --git a/src/commands/cli-grammar/common.ts b/src/commands/cli-grammar/common.ts index e7c998e9c..c06ddb241 100644 --- a/src/commands/cli-grammar/common.ts +++ b/src/commands/cli-grammar/common.ts @@ -52,6 +52,20 @@ function readDeviceTarget(value: unknown): InternalRequestOptions['target'] | un export function commonInputFromFlags(flags: CliFlags): Record { return compactRecord({ + // `--no-record` is a COMMON flag (`COMMON_COMMAND_SUPPORTED_FLAG_KEYS`): it + // is accepted on, and meaningful for, every recordable command. It rides + // the common seam every reader already spreads, so a reader cannot forget + // it and a new reader inherits it for free. The three seams it must survive + // are this one, `readCommonInput`, and `commonToClientOptions` + // (`commands/command-input.ts`) — a drop at any one of them silently + // disables the flag (#1304/#1305 fixed only the reader layer, so the flag + // still never reached the daemon). + // + // `--record` deliberately does NOT ride here: it is scoped to the + // observation-only commands the repair-segment exclusion can drop + // (ADR 0012 decision 6 amendment), so it stays on the narrow + // `observationRecordInputFromFlags` seam below. + noRecord: flags.noRecord, session: flags.session, platform: flags.platform, deviceTarget: flags.target, @@ -66,32 +80,13 @@ export function commonInputFromFlags(flags: CliFlags): Record { }); } -// --no-record is a common flag, but it only takes effect if the reader forwards -// it: command-flags.ts maps options.noRecord onto the daemon request, and -// recordActionEntry reads it from there. A reader that never names it accepts -// the flag and silently drops it. -// -// #1271 stage 2: `--record` deliberately does NOT ride along here. It applies -// only to observation-only commands (snapshot/get/is/find — the ones the -// repair-segment default exclusion can drop), so it gets its own helper below -// rather than an `allowRecord` policy argument on this one. The capability is -// the helper's NAME: a mutating reader physically cannot forward `--record` -// without spreading a helper whose name says it is for observations, whereas a -// boolean policy arg would let a future mutating reader opt in by flipping a -// literal with no schema change — the fail-open this split exists to prevent. -export function noRecordInputFromFlags(flags: CliFlags): Record { - return compactRecord({ - noRecord: flags.noRecord, - }); -} - /** * #1271 stage 2 (ADR 0012 decision 6 amendment): the `--record` opt-in that * forces an observation-only action into a repair-armed heal. Spread ONLY by * readers whose command can be excluded by default — `snapshot`, `get`, `is`, * and `find`. Every one of those readers must ALSO spread - * `noRecordInputFromFlags`, which stays universal (`--no-record` applies to - * every recordable command, mutations included). + * the common `commonInputFromFlags` seam, which carries `--no-record` for every + * recordable command, mutations included. */ export function observationRecordInputFromFlags(flags: CliFlags): Record { return compactRecord({ @@ -99,8 +94,18 @@ export function observationRecordInputFromFlags(flags: CliFlags): Record } & Record; export type SelectionOptions = { + /** `--no-record`: common to every recordable command (see `selectionOptionsFromFlags`). */ + noRecord?: boolean; platform?: CliFlags['platform']; target?: CliFlags['target']; device?: string; diff --git a/src/commands/command-input.ts b/src/commands/command-input.ts index 28ac86fc8..65cc629cb 100644 --- a/src/commands/command-input.ts +++ b/src/commands/command-input.ts @@ -30,6 +30,8 @@ export type CommonCommandInput = Pick< iosXctestDerivedDataPath?: string; iosXctestEnvDir?: string; androidDeviceAllowlist?: string; + /** `--no-record`: common to every recordable command (see `commonInputFromFlags`). */ + noRecord?: boolean; }; export type InteractionTargetInput = @@ -292,6 +294,11 @@ export function readCommonInput( iosXctestDerivedDataPath: optionalString(record, 'iosXctestDerivedDataPath'), iosXctestEnvDir: optionalString(record, 'iosXctestEnvDir'), androidDeviceAllowlist: optionalString(record, 'androidDeviceAllowlist'), + // Seam 2 of 3 for `--no-record` (see `commonInputFromFlags`). `readFieldInput` + // keeps ONLY declared metadata fields plus this common input, so a flag + // absent here is filtered out of every field-based command's input before + // the client ever sees it. + noRecord: optionalBoolean(record, 'noRecord'), daemonBaseUrl: optionalString(record, 'daemonBaseUrl'), daemonAuthToken: optionalString(record, 'daemonAuthToken'), tenant: optionalString(record, 'tenant'), @@ -432,6 +439,12 @@ export function commonToClientOptions( input: CommonCommandInput, ): AgentDeviceRequestOverrides & AgentDeviceSelectionOptions { return compactRecord({ + // Seam 3 of 3 for `--no-record` (see `commonInputFromFlags`). Every + // `to*Options` projection (`toPressOptions`, `toGetOptions`, ...) rebuilds + // the client options object from this helper plus its own named fields, so + // a flag absent here is dropped even when the reader forwarded it and + // `readCommonInput` kept it. + noRecord: input.noRecord, session: input.session, platform: input.platform, target: input.deviceTarget, diff --git a/src/commands/interaction/index.ts b/src/commands/interaction/index.ts index e54c85341..873185b12 100644 --- a/src/commands/interaction/index.ts +++ b/src/commands/interaction/index.ts @@ -423,6 +423,14 @@ function toGetOptions(input: GetInput): GetOptions { ...toClientElementTarget(input.target), ...toSelectorSnapshotOptions(input), format: input.format, + // `--record` is scoped (ADR 0012 decision 6 amendment), so it does NOT ride + // the common seam and each observation-capable projection forwards it + // explicitly. `is`/`find`/`snapshot` pass their whole input through, so + // `get` — the one that rebuilds its options object — is the only place this + // is needed. Without it `get --record` parses, reaches the reader, survives + // `readInput`, and is then dropped here (#1303 regression, same re-projection + // cause as the `--no-record` gap this change fixes). + record: input.record, }; } diff --git a/src/commands/interaction/interactions.ts b/src/commands/interaction/interactions.ts index 6d6995e4a..7f297c10a 100644 --- a/src/commands/interaction/interactions.ts +++ b/src/commands/interaction/interactions.ts @@ -23,7 +23,6 @@ import { optionalNumber, readElementTargetFromPositionals, readGetFormat, - noRecordInputFromFlags, observationRecordInputFromFlags, request, requiredDaemonString, @@ -37,7 +36,6 @@ import type { CliReader, DaemonWriter } from '../cli-grammar/types.ts'; export const interactionCliReaders = { click: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...noRecordInputFromFlags(flags), ...selectorSnapshotInputFromFlags(flags), ...repeatedInputFromFlags(flags), ...settleInputFromFlags(flags), @@ -47,7 +45,6 @@ export const interactionCliReaders = { }), press: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...noRecordInputFromFlags(flags), ...selectorSnapshotInputFromFlags(flags), ...repeatedInputFromFlags(flags), ...settleInputFromFlags(flags), @@ -58,7 +55,6 @@ export const interactionCliReaders = { const decoded = readLongPressTargetFromPositionals(positionals); return { ...commonInputFromFlags(flags), - ...noRecordInputFromFlags(flags), ...selectorSnapshotInputFromFlags(flags), ...settleInputFromFlags(flags), target: targetInputFromClientTarget(decoded), @@ -67,7 +63,6 @@ export const interactionCliReaders = { }, swipe: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...noRecordInputFromFlags(flags), ...swipePayloadFromPositionals(positionals, { count: flags.count, pauseMs: flags.pauseMs, @@ -76,13 +71,11 @@ export const interactionCliReaders = { }), focus: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...noRecordInputFromFlags(flags), x: Number(positionals[0]), y: Number(positionals[1]), }), type: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...noRecordInputFromFlags(flags), text: positionals.join(' '), delayMs: flags.delayMs, }), @@ -90,7 +83,6 @@ export const interactionCliReaders = { const decoded = readFillTargetFromPositionals(positionals); return { ...commonInputFromFlags(flags), - ...noRecordInputFromFlags(flags), ...selectorSnapshotInputFromFlags(flags), ...settleInputFromFlags(flags), target: targetInputFromClientTarget(decoded.target), @@ -101,7 +93,6 @@ export const interactionCliReaders = { }, scroll: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...noRecordInputFromFlags(flags), direction: readScrollDirection(positionals[0]), amount: optionalCliNumber(positionals[1]), pixels: flags.pixels, @@ -113,7 +104,6 @@ export const interactionCliReaders = { // `--no-record`. get: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...noRecordInputFromFlags(flags), ...observationRecordInputFromFlags(flags), ...selectorSnapshotInputFromFlags(flags), format: readGetFormat(positionals[0]), diff --git a/src/commands/interaction/selectors.ts b/src/commands/interaction/selectors.ts index b7c4cbf5c..d4a8936ae 100644 --- a/src/commands/interaction/selectors.ts +++ b/src/commands/interaction/selectors.ts @@ -11,7 +11,6 @@ import { direct, optionalCliNumber, optionalNumber, - noRecordInputFromFlags, observationRecordInputFromFlags, request, selectionOptionsFromFlags, @@ -66,7 +65,6 @@ function readFindOptionsFromPositionals(positionals: string[], flags: CliFlags): const base = { ...findSnapshotOptionsFromFlags(flags), ...selectionOptionsFromFlags(flags), - ...noRecordInputFromFlags(flags), ...observationRecordInputFromFlags(flags), first: flags.findFirst, last: flags.findLast, @@ -115,7 +113,6 @@ function readIsOptionsFromPositionals(positionals: string[], flags: CliFlags): I const base = { ...selectorSnapshotOptionsFromFlags(flags), ...selectionOptionsFromFlags(flags), - ...noRecordInputFromFlags(flags), ...observationRecordInputFromFlags(flags), }; const normalized = normalizeIsPositionals(positionals); diff --git a/test/integration/provider-scenarios/no-record-recorder-routes.test.ts b/test/integration/provider-scenarios/no-record-recorder-routes.test.ts new file mode 100644 index 000000000..e6bd45661 --- /dev/null +++ b/test/integration/provider-scenarios/no-record-recorder-routes.test.ts @@ -0,0 +1,80 @@ +/** + * `--no-record` must keep an action out of a recorded session script — proven + * at the RECORDER ROUTE, through the full argv -> reader -> client -> daemon + * chain, with the written `.ad` as the observable. + * + * This is the coverage shape #1304/#1305 lacked. Their assertion stopped at + * `readInputFromCli`'s intermediate object, so the flag could be (and was) + * dropped by `readFieldInput` and `to*Options` downstream while the test stayed + * green. `gesture` (interaction-completion route) and `back`/`home` (generic + * dispatch route) are the concrete regressions that shipped as a result. + */ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { test } from 'vitest'; +import { runCliCommandWithOutput } from '../../../src/commands/cli-runner.ts'; +import type { CommandName } from '../../../src/commands/command-metadata.ts'; +import { parseArgs } from '../../../src/cli/parser/args.ts'; +import { createAndroidSettingsWorld } from './android-world.ts'; +import { withProviderScenarioResource } from './harness.ts'; + +test('--no-record keeps gesture/back/home out of a recorded session script', async () => { + await withProviderScenarioResource(createAndroidSettingsWorld, async (world) => { + const daemon = world.daemon; + const client = daemon.client(); + const selection = world.selection; + const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-no-record-routes-')); + const scriptPath = path.join(tempRoot, 'session.ad'); + + // Ordinary (non-repair) authoring recording. + const open = await daemon.callCommand('open', ['settings'], { + ...selection, + saveScript: scriptPath, + }); + assert.equal(open.statusCode, 200, JSON.stringify(open.json)); + + // Every command below goes through the REAL argv -> reader -> client -> + // daemon chain, exactly as a user types it. + const runCli = async (argv: string[]) => + await runCliCommandWithOutput({ + client, + command: argv[0] as CommandName, + positionals: parseArgs(argv, { strictFlags: true }).positionals, + flags: { ...parseArgs(argv, { strictFlags: true }).flags, ...selection }, + }); + + // Control: a recorded action, no flag — must land in the script, proving + // the recording is live and the absences below are the flag's doing. + await runCli(['press', '10', '20']); + + // The three routes #1305 missed, each suppressed by --no-record. + await runCli(['gesture', 'fling', 'up', '100', '200', '--no-record']); + await runCli(['back', '--no-record']); + await runCli(['home', '--no-record']); + + const close = await daemon.callCommand('close', [], { saveScript: scriptPath }); + assert.equal(close.statusCode, 200, JSON.stringify(close.json)); + + const script = fs.readFileSync(scriptPath, 'utf8'); + const commandsInScript = script + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.length > 0 && !line.startsWith('#') && !line.startsWith('context ')) + .map((line) => line.split(/\s+/)[0]); + + assert.ok( + commandsInScript.includes('press'), + `the unflagged control action must be recorded, got:\n${script}`, + ); + for (const suppressed of ['gesture', 'fling', 'back', 'home']) { + assert.ok( + !commandsInScript.includes(suppressed), + `${suppressed} ran with --no-record but still landed in the recorded script:\n${script}`, + ); + } + + fs.rmSync(tempRoot, { recursive: true, force: true }); + }); +}, 20_000);