mirror of
https://github.com/callstack/agent-device.git
synced 2026-09-14 20:06:34 +08:00
fix(daemon): reject unsupported raw --save-script flags at request ingress (#1501)
* fix(daemon): reject unsupported raw --save-script flags at request ingress `flags.saveScript` arms session-script publication the moment a successful handler records the request's action, so any recordable command reaching a handler with the raw flag set could arm publication and write a `.ad` artifact — `record stop` immediately through record-only cleanup, `trace` and the interaction commands later on close. The CLI/Node/MCP surfaces only emit the flag for its released owners, so that arming path was reachable only by a hand-built wire request or a free-form `batch` step. Close it at the daemon request seam (`createRequestHandler`), the single entry point both the socket and HTTP transports funnel through, and ahead of admission, session lookup, device resolution, and handler dispatch. Ownership is declared on the command descriptor (`saveScriptFlagOwner`) rather than re-stated as a string set, so `open`, `close`, and `replay` keep the flag and everything else is rejected with an ADR 0010 shaped `INVALID_ARGS` carrying a hint, diagnosticId, and logPath. With that path pinned closed, a record-only session can no longer carry `recordSession`, so `record stop`'s immediate `writeSessionLog` could only ever be a no-op; delete it. Refs #1478. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * test(daemon): assert ADR 0010 diagnostics on save-script rejection The HTTP test helper projected only code/message/hint, so `diagnosticId` and `logPath` could regress while the transport tests stayed green. Preserve both in the helper and assert on each transport that `diagnosticId` is present, `logPath` is present and exists, and the log records `save_script_flag_rejected`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
committed by
GitHub
parent
0ee2a86129
commit
3c09bb2566
@@ -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.
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
@@ -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);
|
||||
});
|
||||
@@ -31,6 +31,15 @@ export type DaemonCommandDescriptor = {
|
||||
selectorValidationExempt?: boolean;
|
||||
replayScopedAction?: boolean;
|
||||
allowInvalidRecording?: boolean;
|
||||
/**
|
||||
* #1478: this command's REQUEST may carry `flags.saveScript` to arm session
|
||||
* script publication. Only the released flag owners (`open`, `close`,
|
||||
* `replay` — the commands whose CLI grammar declares `--save-script`) set
|
||||
* this; every other command's raw request is rejected at the daemon request
|
||||
* seam by `unsupportedSaveScriptFlagResponse`, so a recordable command such
|
||||
* as `record` or `trace` cannot arm publication over the wire.
|
||||
*/
|
||||
saveScriptFlagOwner?: boolean;
|
||||
lockPolicySelectorOverride?: boolean;
|
||||
androidBlockingDialogGuard?: boolean;
|
||||
preferExplicitDeviceOverExistingSession?: boolean;
|
||||
@@ -84,6 +93,18 @@ export function shouldBlockForInvalidRecording(command: string): boolean {
|
||||
return getDaemonCommandDescriptor(command)?.allowInvalidRecording !== true;
|
||||
}
|
||||
|
||||
/** #1478: whether `flags.saveScript` is accepted on this command's request. */
|
||||
export function ownsSaveScriptFlag(command: string): boolean {
|
||||
return getDaemonCommandDescriptor(command)?.saveScriptFlagOwner === true;
|
||||
}
|
||||
|
||||
/** #1478: the released `--save-script` flag owners, sorted for stable messages. */
|
||||
export function listSaveScriptFlagOwnerCommands(): string[] {
|
||||
return DAEMON_COMMAND_DESCRIPTORS.filter((descriptor) => descriptor.saveScriptFlagOwner === true)
|
||||
.map((descriptor) => descriptor.command)
|
||||
.sort();
|
||||
}
|
||||
|
||||
export function canOverrideLockPolicySelector(command: string): boolean {
|
||||
return getDaemonCommandDescriptor(command)?.lockPolicySelectorOverride === true;
|
||||
}
|
||||
|
||||
@@ -509,20 +509,25 @@ function deriveClientTelemetryPath(
|
||||
return deriveRecordingTelemetryPath(recording.clientOutPath);
|
||||
}
|
||||
|
||||
/**
|
||||
* #1478 (P4-pre): a record-only session is created by `record` itself and never
|
||||
* by `open`, so the only way it could ever have carried `recordSession` was a
|
||||
* raw `record --save-script` request — the arming path now rejected at the
|
||||
* daemon request seam (`unsupportedSaveScriptFlagResponse`). With that closed,
|
||||
* the immediate `writeSessionLog` this used to run at `record stop` could only
|
||||
* ever be a no-op, so it is gone: releasing a record-only session is backend
|
||||
* cleanup plus store removal.
|
||||
*/
|
||||
async function releaseRecordOnlySession(
|
||||
sessionStore: SessionStore,
|
||||
sessionName: string,
|
||||
session: SessionState,
|
||||
options: { writeLog?: boolean } = {},
|
||||
): Promise<void> {
|
||||
if (!session.recordOnlySession) {
|
||||
return;
|
||||
}
|
||||
const backend = resolveRecordingBackendForDevice(session.device);
|
||||
await backend.cleanupRecordOnlySession?.(session);
|
||||
if (options.writeLog) {
|
||||
sessionStore.writeSessionLog(session);
|
||||
}
|
||||
sessionStore.delete(sessionName);
|
||||
}
|
||||
|
||||
@@ -656,7 +661,7 @@ export async function handleRecordCommand(params: {
|
||||
...requestedRecordingEventDetails,
|
||||
showTouches: response.data?.showTouches,
|
||||
});
|
||||
await releaseRecordOnlySession(sessionStore, sessionName, activeSession, { writeLog: true });
|
||||
await releaseRecordOnlySession(sessionStore, sessionName, activeSession);
|
||||
return response;
|
||||
}
|
||||
|
||||
|
||||
@@ -49,6 +49,7 @@ import {
|
||||
type RequestExecutionScope,
|
||||
} from './request-execution-scope.ts';
|
||||
import { buildRequestFinishedEvent, shouldRecordEventForRequest } from './session-event-log.ts';
|
||||
import { unsupportedSaveScriptFlagResponse } from './request-save-script-policy.ts';
|
||||
import { canRunReplayScopedAction } from './daemon-command-registry.ts';
|
||||
import { createAgentBrowserWebProvider } from '../platforms/web/agent-browser-provider.ts';
|
||||
import { openWebSessionNames } from './web-session-names.ts';
|
||||
@@ -148,6 +149,10 @@ export function createRequestHandler(deps: RequestRouterDeps): DaemonInvokeFn {
|
||||
registerParameterizedFillDiagnosticValue(req);
|
||||
const invalidRecordingFlags = recordingFlagsResponse(req);
|
||||
if (invalidRecordingFlags) return invalidRecordingFlags;
|
||||
// #1478: raw `flags.saveScript` on a non-owner command never reaches
|
||||
// admission, device work, or a handler that could arm publication.
|
||||
const unsupportedSaveScript = unsupportedSaveScriptFlagResponse(req);
|
||||
if (unsupportedSaveScript) return unsupportedSaveScript;
|
||||
|
||||
let scope: RequestExecutionScope | undefined;
|
||||
try {
|
||||
|
||||
@@ -0,0 +1,65 @@
|
||||
import { AppError, normalizeError } from '@agent-device/kernel/errors';
|
||||
import {
|
||||
emitDiagnostic,
|
||||
flushDiagnosticsToSessionFile,
|
||||
getDiagnosticsMeta,
|
||||
} from '../utils/diagnostics.ts';
|
||||
import { listSaveScriptFlagOwnerCommands, ownsSaveScriptFlag } from './daemon-command-registry.ts';
|
||||
import type { DaemonRequest, DaemonResponse } from './types.ts';
|
||||
|
||||
/**
|
||||
* #1478: the released `--save-script` flag owners, read from the command
|
||||
* descriptor registry rather than re-stated here (AGENTS.md: never re-create
|
||||
* command string sets outside the declaration site).
|
||||
*/
|
||||
export const SAVE_SCRIPT_FLAG_OWNER_COMMANDS: readonly string[] = listSaveScriptFlagOwnerCommands();
|
||||
|
||||
const UNSUPPORTED_SAVE_SCRIPT_MESSAGE = `--save-script is supported only by ${SAVE_SCRIPT_FLAG_OWNER_COMMANDS.join(', ')}.`;
|
||||
|
||||
const UNSUPPORTED_SAVE_SCRIPT_HINT =
|
||||
'Arm publication on the session instead: open/close --save-script, replay --save-script, or session save-script to publish an armed recording without closing its session.';
|
||||
|
||||
/**
|
||||
* #1478 (P4-pre): `flags.saveScript` arms session-script publication the moment
|
||||
* a successful handler records the request's action (`recordActionEntry` sets
|
||||
* `recordSession` and the target path), so ANY recordable command that reaches
|
||||
* a handler with the raw flag set can arm publication and later write a `.ad`
|
||||
* artifact — `record stop` writes one immediately through record-only cleanup,
|
||||
* `trace` and the interaction commands publish later on close.
|
||||
*
|
||||
* The CLI/Node/MCP surfaces only ever emit the flag for its released owners, so
|
||||
* the arming path is reachable only by a hand-built wire request. This closes
|
||||
* it at the daemon request seam — the single entry point BOTH the socket and
|
||||
* HTTP transports funnel through (`createRequestHandler`), and ahead of
|
||||
* admission, session lookup, device resolution, and handler dispatch — so an
|
||||
* unsupported request can neither arm publication nor perform any device work.
|
||||
*
|
||||
* Returns `undefined` when the request is allowed through: no flag at all
|
||||
* (including the flag absent from a request that never had it), or a released
|
||||
* owner command. Presence is what is rejected, not truthiness — a raw
|
||||
* `saveScript: false` is just as unsupported on `record` as `true` is, and
|
||||
* mirrors the maestro replay guard's `!== undefined` reading of the same flag.
|
||||
*/
|
||||
export function unsupportedSaveScriptFlagResponse(req: DaemonRequest): DaemonResponse | undefined {
|
||||
if (req.flags?.saveScript === undefined) return undefined;
|
||||
if (ownsSaveScriptFlag(req.command)) return undefined;
|
||||
|
||||
emitDiagnostic({
|
||||
level: 'warn',
|
||||
phase: 'save_script_flag_rejected',
|
||||
data: { command: req.command },
|
||||
});
|
||||
// ADR 0010 decision 6: a failed request always carries its diagnosticId +
|
||||
// ndjson logPath, so this rejection is as traceable as a thrown one.
|
||||
const meta = getDiagnosticsMeta();
|
||||
const logPath = flushDiagnosticsToSessionFile({ force: true }) ?? undefined;
|
||||
return {
|
||||
ok: false,
|
||||
error: normalizeError(
|
||||
new AppError('INVALID_ARGS', UNSUPPORTED_SAVE_SCRIPT_MESSAGE, {
|
||||
hint: UNSUPPORTED_SAVE_SCRIPT_HINT,
|
||||
}),
|
||||
{ diagnosticId: meta.diagnosticId, logPath },
|
||||
),
|
||||
};
|
||||
}
|
||||
Reference in New Issue
Block a user