fix(cli): deliver --no-record to the daemon (supersedes #1305)

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);

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<SeenRequest[]> {
+  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/,
+  );
+});

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,

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') {

src/commands/cli-grammar/common.ts

@@ -52,6 +52,20 @@ function readDeviceTarget(value: unknown): InternalRequestOptions['target'] | un
 
 export function commonInputFromFlags(flags: CliFlags): Record<string, unknown> {
   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,41 +80,32 @@ export function commonInputFromFlags(flags: CliFlags): Record<string, unknown> {
   });
 }
 
-// --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<string, unknown> {
-  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<string, unknown> {
   return compactRecord({
     record: flags.record,
   });
 }
 
+/**
+ * The reader layer has TWO parallel common seams, not one: this builds the
+ * client-options shape (`target`) for readers that construct a typed Options
+ * object directly (`is`/`find`/`wait`/`settings`), while `commonInputFromFlags`
+ * above builds the reader-input shape (`deviceTarget`). They are different
+ * projections, not duplicates — so `--no-record` has to ride BOTH or the
+ * readers using this one silently drop it (which is what #1304/#1305's
+ * per-reader helper was papering over).
+ */
 export function selectionOptionsFromFlags(flags: CliFlags): SelectionOptions {
   return {
+    noRecord: flags.noRecord,
     platform: flags.platform,
     target: flags.target,
     device: flags.device,

src/commands/cli-grammar/types.ts

@@ -75,6 +75,8 @@ export type CommandInput = Omit<InternalRequestOptions, 'batchSteps' | 'target'>
   } & Record<string, unknown>;
 
 export type SelectionOptions = {
+  /** `--no-record`: common to every recordable command (see `selectionOptionsFromFlags`). */
+  noRecord?: boolean;
   platform?: CliFlags['platform'];
   target?: CliFlags['target'];
   device?: string;

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,

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,
   };
 }