fix(daemon): reject unsupported raw --save-script flags at request ingress

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## Unreleased
 
+- `--save-script` is now accepted only by the commands that declare it — `open`, `close`, and `replay`. A hand-built daemon request (or a `batch` step) that set `saveScript` on any other command, such as `record` or `trace`, used to arm script publication and could write a `.ad` artifact; it is now rejected with `INVALID_ARGS` before the request reaches admission, the device, or any handler. CLI, Node, and MCP usage of `--save-script` on its documented commands is unchanged.
 - `diff screenshot` no longer runs the retired best-effort OCR and non-text analyzers. Their optional `ocr` and `nonTextDeltas` fields remain in the result type for source compatibility but are no longer emitted; use the baseline/current images and diff artifact with vision for qualitative interpretation.
 - Breaking: removed the deprecated `--session-locked` and `--session-lock-conflicts` flags. Use `--session-lock reject|strip` instead; passing either old flag now fails with `Unknown flag: ... Use --session-lock reject|strip instead.`
 - Breaking: removed the `replay export --format` flag. `replay export` always writes Maestro YAML.

src/core/command-descriptor/registry.ts

@@ -552,6 +552,7 @@ export const RAW_COMMAND_DESCRIPTORS = [
       refFrameEffect: 'delegated',
       sessionKind: 'replay',
       skipSessionlessProviderDevice: isShardedTestRequest,
+      saveScriptFlagOwner: true,
     },
     // Replay durations are script-dependent; --timeout bounds the envelope.
     timeoutPolicy: { ...DEFAULT_TIMEOUT_POLICY, budget: { source: 'flag' } },
@@ -703,6 +704,7 @@ export const RAW_COMMAND_DESCRIPTORS = [
       route: 'session',
       refFrameEffect: 'may-invalidate',
       allowSessionlessDefaultDevice: allowAnyDeviceSessionless,
+      saveScriptFlagOwner: true,
     },
     dispatch: {},
     capability: VEGA_APP_RUNTIME_CAPABILITY,
@@ -742,7 +744,12 @@ export const RAW_COMMAND_DESCRIPTORS = [
     catalog: { group: 'public' },
     recordsSessionAction: true,
     recordingEffect: 'mutates-app',
-    daemon: { route: 'session', refFrameEffect: 'may-invalidate', allowInvalidRecording: true },
+    daemon: {
+      route: 'session',
+      refFrameEffect: 'may-invalidate',
+      allowInvalidRecording: true,
+      saveScriptFlagOwner: true,
+    },
     dispatch: {},
     capability: VEGA_APP_RUNTIME_CAPABILITY,
     timeoutPolicy: DEFAULT_TIMEOUT_POLICY,

src/daemon/__tests__/request-save-script-policy.test.ts

@@ -0,0 +1,65 @@
+/**
+ * #1478 (P4-pre): `flags.saveScript` is accepted only on the released
+ * `--save-script` flag owners. Everything else is rejected at the daemon
+ * request seam before any admission, session, or device work.
+ */
+import { expect, test } from 'vitest';
+import { listSaveScriptFlagOwnerCommands, ownsSaveScriptFlag } from '../daemon-command-registry.ts';
+import {
+  SAVE_SCRIPT_FLAG_OWNER_COMMANDS,
+  unsupportedSaveScriptFlagResponse,
+} from '../request-save-script-policy.ts';
+import type { DaemonRequest } from '../types.ts';
+
+function request(command: string, flags: DaemonRequest['flags']): DaemonRequest {
+  return { token: 'token', session: 'default', command, positionals: [], flags };
+}
+
+test('the flag owners are exactly the released open/close/replay surface', () => {
+  expect(listSaveScriptFlagOwnerCommands()).toEqual(['close', 'open', 'replay']);
+  expect(SAVE_SCRIPT_FLAG_OWNER_COMMANDS).toEqual(['close', 'open', 'replay']);
+  for (const command of SAVE_SCRIPT_FLAG_OWNER_COMMANDS) {
+    expect(ownsSaveScriptFlag(command)).toBe(true);
+  }
+  // `test` runs replay scripts but never declares `--save-script`, and the
+  // internal publication command carries its path/force as positionals+flags of
+  // its own — neither may arm through the raw flag.
+  expect(ownsSaveScriptFlag('test')).toBe(false);
+  expect(ownsSaveScriptFlag('session_save_script')).toBe(false);
+});
+
+test('owner commands and flag-free requests pass the seam untouched', () => {
+  for (const command of SAVE_SCRIPT_FLAG_OWNER_COMMANDS) {
+    expect(unsupportedSaveScriptFlagResponse(request(command, { saveScript: true }))).toBe(
+      undefined,
+    );
+    expect(unsupportedSaveScriptFlagResponse(request(command, { saveScript: './flow.ad' }))).toBe(
+      undefined,
+    );
+  }
+  expect(unsupportedSaveScriptFlagResponse(request('record', {}))).toBe(undefined);
+  expect(unsupportedSaveScriptFlagResponse(request('record', undefined))).toBe(undefined);
+});
+
+test.each(['record', 'trace', 'click', 'fill', 'snapshot', 'test', 'session_save_script'])(
+  'raw saveScript on %s is rejected with an ADR 0010 shaped INVALID_ARGS error',
+  (command) => {
+    const response = unsupportedSaveScriptFlagResponse(request(command, { saveScript: true }));
+
+    expect(response?.ok).toBe(false);
+    if (!response || response.ok) return;
+    expect(response.error.code).toBe('INVALID_ARGS');
+    expect(response.error.message).toBe('--save-script is supported only by close, open, replay.');
+    // The per-code default hint ("check command arguments") would misdirect, so
+    // the rejection names the surfaces that actually publish a script.
+    expect(response.error.hint).toMatch(/session save-script/);
+  },
+);
+
+test('presence is rejected, not truthiness — a raw false is unsupported too', () => {
+  const response = unsupportedSaveScriptFlagResponse(request('record', { saveScript: false }));
+
+  expect(response?.ok).toBe(false);
+  if (!response || response.ok) return;
+  expect(response.error.code).toBe('INVALID_ARGS');
+});

src/daemon/__tests__/request-save-script-transports.test.ts

@@ -0,0 +1,291 @@
+/**
+ * #1478 (P4-pre): raw-wire counterfactuals for the `flags.saveScript` seam.
+ *
+ * The CLI/Node/MCP surfaces only ever emit `--save-script` for its released
+ * owners, so the arming path is reachable only by a hand-built request. Both
+ * daemon transports funnel through the SAME request handler
+ * (`createRequestHandler`, wired into `createSocketServer` and
+ * `createDaemonHttpServer` by `daemon-runtime.ts`), so the rejection is pinned
+ * here on both wires: an unsupported command carrying the flag must not reach
+ * admission or device work, must not arm publication, and must not leave a
+ * `.ad` artifact behind.
+ */
+import fs from 'node:fs';
+import net from 'node:net';
+import os from 'node:os';
+import path from 'node:path';
+import { afterEach, expect, test } from 'vitest';
+import { LeaseRegistry } from '../lease-registry.ts';
+import { createRequestHandler } from '../request-router.ts';
+import { SessionStore } from '../session-store.ts';
+import { createDaemonHttpServer } from '../server/http-server.ts';
+import { createSocketServer, listenNetServer } from '../server/transport.ts';
+import type { DaemonInvokeFn, DaemonResponse, SessionState } from '../types.ts';
+import { makeIosSession } from '../../__tests__/test-utils/index.ts';
+import {
+  closeLoopbackServer,
+  listenOnLoopback,
+  skipWhenLoopbackUnavailable,
+} from '../../__tests__/test-utils/loopback.ts';
+
+const TOKEN = 'save-script-transport-token';
+const SESSION = 'save-script-transport';
+const UNSUPPORTED_MESSAGE = '--save-script is supported only by close, open, replay.';
+
+type WireRequest = {
+  token?: string;
+  session?: string;
+  command: string;
+  positionals?: string[];
+  flags?: Record<string, unknown>;
+};
+
+type Harness = {
+  root: string;
+  sessionStore: SessionStore;
+  session: SessionState;
+  handleRequest: DaemonInvokeFn;
+};
+
+const roots: string[] = [];
+
+afterEach(() => {
+  for (const root of roots.splice(0)) fs.rmSync(root, { recursive: true, force: true });
+});
+
+function setup(): Harness {
+  const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-save-script-transport-'));
+  roots.push(root);
+  const sessionStore = new SessionStore(path.join(root, 'sessions'));
+  const session = makeIosSession(SESSION);
+  sessionStore.set(SESSION, session);
+  const handleRequest = createRequestHandler({
+    logPath: path.join(root, 'daemon.log'),
+    token: TOKEN,
+    sessionStore,
+    leaseRegistry: new LeaseRegistry(),
+    trackDownloadableArtifact: () => 'artifact-id',
+  });
+  return { root, sessionStore, session, handleRequest };
+}
+
+async function sendOverSocket(
+  handleRequest: DaemonInvokeFn,
+  request: WireRequest,
+): Promise<DaemonResponse> {
+  const server = createSocketServer(handleRequest);
+  try {
+    const port = await listenNetServer(server);
+    const socket = net.createConnection({ host: '127.0.0.1', port });
+    try {
+      await new Promise<void>((resolve, reject) => {
+        socket.once('connect', () => resolve());
+        socket.once('error', reject);
+      });
+      return await new Promise<DaemonResponse>((resolve, reject) => {
+        let buffer = '';
+        socket.setEncoding('utf8');
+        socket.on('data', (chunk: string) => {
+          buffer += chunk;
+          const newline = buffer.indexOf('\n');
+          if (newline === -1) return;
+          resolve(JSON.parse(buffer.slice(0, newline)) as DaemonResponse);
+        });
+        socket.once('error', reject);
+        socket.write(`${JSON.stringify({ token: TOKEN, session: SESSION, ...request })}\n`);
+      });
+    } finally {
+      socket.destroy();
+    }
+  } finally {
+    await closeLoopbackServer(server);
+  }
+}
+
+async function sendOverHttp(
+  handleRequest: DaemonInvokeFn,
+  request: WireRequest,
+): Promise<DaemonResponse> {
+  const server = await createDaemonHttpServer({ handleRequest, token: TOKEN });
+  try {
+    const port = await listenOnLoopback(server);
+    const response = await fetch(`http://127.0.0.1:${port}/rpc`, {
+      method: 'POST',
+      headers: { 'content-type': 'application/json' },
+      body: JSON.stringify({
+        jsonrpc: '2.0',
+        id: 'req-save-script',
+        method: 'agent_device.command',
+        params: { token: TOKEN, session: SESSION, ...request },
+      }),
+    });
+    const body = (await response.json()) as {
+      result?: { ok: boolean; data?: Record<string, unknown> };
+      // Project the ADR 0010 diagnostics fields too: dropping them here would
+      // let `diagnosticId`/`logPath` regress while these tests stayed green.
+      error?: {
+        data?: {
+          code?: string;
+          message?: string;
+          hint?: string;
+          diagnosticId?: string;
+          logPath?: string;
+        };
+      };
+    };
+    if (body.error?.data) {
+      const { code, message, hint, diagnosticId, logPath } = body.error.data;
+      return {
+        ok: false,
+        error: { code: code ?? 'UNKNOWN', message: message ?? '', hint, diagnosticId, logPath },
+      };
+    }
+    return (body.result ?? {
+      ok: false,
+      error: { code: 'UNKNOWN', message: 'no result' },
+    }) as DaemonResponse;
+  } finally {
+    await closeLoopbackServer(server);
+  }
+}
+
+const TRANSPORTS = [
+  ['socket', sendOverSocket],
+  ['http', sendOverHttp],
+] as const satisfies readonly (readonly [
+  string,
+  (handleRequest: DaemonInvokeFn, request: WireRequest) => Promise<DaemonResponse>,
+])[];
+
+function listAdArtifacts(root: string): string[] {
+  return fs
+    .readdirSync(root, { recursive: true, encoding: 'utf8' })
+    .filter((entry) => entry.endsWith('.ad'));
+}
+
+for (const [transport, send] of TRANSPORTS) {
+  test(`${transport}: raw saveScript on a recordable command never arms or writes a script`, async (t) => {
+    if (await skipWhenLoopbackUnavailable(t)) return;
+    const { root, sessionStore, session, handleRequest } = setup();
+
+    const rejected = await send(handleRequest, {
+      command: 'trace',
+      positionals: ['start'],
+      flags: { saveScript: `${root}/forged.ad` },
+    });
+
+    expect(rejected.ok).toBe(false);
+    if (rejected.ok) return;
+    expect(rejected.error.code).toBe('INVALID_ARGS');
+    expect(rejected.error.message).toBe(UNSUPPORTED_MESSAGE);
+    expect(rejected.error.hint).toMatch(/session save-script/);
+    // ADR 0010 decision 6: a failed request stays traceable. Asserted on both
+    // transports so the HTTP projection cannot silently drop these again.
+    expect(rejected.error.diagnosticId).toBeTruthy();
+    expect(rejected.error.logPath).toBeTruthy();
+    const rejectionLogPath = rejected.error.logPath as string;
+    expect(fs.existsSync(rejectionLogPath)).toBe(true);
+    expect(fs.readFileSync(rejectionLogPath, 'utf8')).toMatch(/save_script_flag_rejected/);
+
+    // No handler work: the trace never started and no action was recorded.
+    expect(session.trace).toBe(undefined);
+    expect(session.actions).toEqual([]);
+    // No arming: neither the recording marker nor the publication target moved.
+    expect(session.recordSession).toBe(undefined);
+    expect(session.saveScriptPath).toBe(undefined);
+    // No artifact: the write a later close/teardown would attempt publishes nothing.
+    expect(sessionStore.writeSessionLog(session)).toEqual({ written: false });
+    expect(listAdArtifacts(root)).toEqual([]);
+    expect(fs.existsSync(path.join(root, 'forged.ad'))).toBe(false);
+
+    // Counterfactual: the very same request without the flag does reach the
+    // handler and does record its action, so the rejection above is the flag.
+    const accepted = await send(handleRequest, { command: 'trace', positionals: ['start'] });
+    expect(accepted.ok).toBe(true);
+    expect(session.trace?.outPath).toMatch(/\.trace\.log$/);
+    expect(session.actions.map((action) => action.command)).toEqual(['trace']);
+    expect(session.recordSession).toBe(undefined);
+    expect(listAdArtifacts(root)).toEqual([]);
+  });
+
+  test(`${transport}: the rejection lands before admission`, async (t) => {
+    if (await skipWhenLoopbackUnavailable(t)) return;
+    const { handleRequest } = setup();
+
+    // `sessionIsolation: 'tenant'` without a tenant is rejected by
+    // `scopeRequestSession`, the first step of request admission. Getting the
+    // save-script message instead proves the flag seam runs ahead of it.
+    const response = await send(handleRequest, {
+      command: 'record',
+      positionals: ['stop'],
+      flags: { saveScript: true, sessionIsolation: 'tenant' },
+    });
+
+    expect(response.ok).toBe(false);
+    if (response.ok) return;
+    expect(response.error.message).toBe(UNSUPPORTED_MESSAGE);
+    expect(response.error.message).not.toMatch(/tenant/);
+  });
+
+  test(`${transport}: the released owners still carry the flag to their handlers`, async (t) => {
+    if (await skipWhenLoopbackUnavailable(t)) return;
+    const { handleRequest } = setup();
+
+    // `replay` is a flag owner, so the seam lets it through and the request
+    // fails only on its own missing-path validation, downstream of admission.
+    const response = await send(handleRequest, {
+      command: 'replay',
+      positionals: [],
+      flags: { saveScript: true },
+    });
+
+    expect(response.ok).toBe(false);
+    if (response.ok) return;
+    expect(response.error.message).toBe('replay requires a path');
+  });
+}
+
+test('a batch step cannot smuggle the flag onto a non-owner command', async (t) => {
+  if (await skipWhenLoopbackUnavailable(t)) return;
+  const { root, session, handleRequest } = setup();
+
+  // Batch step flags are free-form passthrough into the same request entry
+  // point, so they are the third face of the same raw arming path — the step's
+  // nested request meets the seam exactly like a top-level one.
+  const response = await sendOverSocket(handleRequest, {
+    command: 'batch',
+    positionals: [],
+    flags: {
+      batchSteps: [
+        { command: 'trace', positionals: ['start'], flags: { saveScript: `${root}/forged.ad` } },
+      ],
+    },
+  });
+
+  expect(response.ok).toBe(false);
+  if (response.ok) return;
+  expect(response.error.message).toMatch(UNSUPPORTED_MESSAGE);
+  expect(session.trace).toBe(undefined);
+  expect(session.recordSession).toBe(undefined);
+  expect(listAdArtifacts(root)).toEqual([]);
+});
+
+test('an owner-armed session still records its target and publishes its script', () => {
+  const { root, sessionStore, session } = setup();
+  const target = path.join(root, 'published.ad');
+
+  // What `open`/`close --save-script` do once past the seam: arm the session,
+  // then publish at teardown. Unchanged by the ingress rejection.
+  sessionStore.recordAction(session, {
+    command: 'open',
+    positionals: ['Example'],
+    flags: { saveScript: target },
+    result: { session: SESSION },
+  });
+  expect(session.recordSession).toBe(true);
+  expect(session.saveScriptPath).toBe(target);
+
+  const result = sessionStore.writeSessionLog(session);
+  expect(result).toEqual({ written: true, path: target, actionCount: 1 });
+  expect(fs.readFileSync(target, 'utf8')).toMatch(/^open /m);
+});