From 1cf9c4c08359ad06b1aec9a36b6931038f3cd170 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 15:37:50 +0000 Subject: [PATCH 1/2] fix(daemon): reject unsupported raw --save-script flags at request ingress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 --- CHANGELOG.md | 1 + src/core/command-descriptor/registry.ts | 9 +- .../request-save-script-policy.test.ts | 65 +++++ .../request-save-script-transports.test.ts | 271 ++++++++++++++++++ src/daemon/daemon-command-registry.ts | 21 ++ src/daemon/handlers/record-trace-recording.ts | 15 +- src/daemon/request-router.ts | 5 + src/daemon/request-save-script-policy.ts | 65 +++++ 8 files changed, 446 insertions(+), 6 deletions(-) create mode 100644 src/daemon/__tests__/request-save-script-policy.test.ts create mode 100644 src/daemon/__tests__/request-save-script-transports.test.ts create mode 100644 src/daemon/request-save-script-policy.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index e3cb588103..75d5bc4fec 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/src/core/command-descriptor/registry.ts b/src/core/command-descriptor/registry.ts index 349b4bb33a..634c6c60b0 100644 --- a/src/core/command-descriptor/registry.ts +++ b/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, diff --git a/src/daemon/__tests__/request-save-script-policy.test.ts b/src/daemon/__tests__/request-save-script-policy.test.ts new file mode 100644 index 0000000000..44f3103377 --- /dev/null +++ b/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'); +}); diff --git a/src/daemon/__tests__/request-save-script-transports.test.ts b/src/daemon/__tests__/request-save-script-transports.test.ts new file mode 100644 index 0000000000..10af6ad7d7 --- /dev/null +++ b/src/daemon/__tests__/request-save-script-transports.test.ts @@ -0,0 +1,271 @@ +/** + * #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; +}; + +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 { + const server = createSocketServer(handleRequest); + try { + const port = await listenNetServer(server); + const socket = net.createConnection({ host: '127.0.0.1', port }); + try { + await new Promise((resolve, reject) => { + socket.once('connect', () => resolve()); + socket.once('error', reject); + }); + return await new Promise((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 { + 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 }; + error?: { data?: { code?: string; message?: string; hint?: string } }; + }; + if (body.error?.data) { + const { code, message, hint } = body.error.data; + return { ok: false, error: { code: code ?? 'UNKNOWN', message: message ?? '', hint } }; + } + 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, +])[]; + +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/); + + // 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); +}); diff --git a/src/daemon/daemon-command-registry.ts b/src/daemon/daemon-command-registry.ts index 28fd833e58..3221ad6441 100644 --- a/src/daemon/daemon-command-registry.ts +++ b/src/daemon/daemon-command-registry.ts @@ -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; } diff --git a/src/daemon/handlers/record-trace-recording.ts b/src/daemon/handlers/record-trace-recording.ts index 4415a3601d..7cf4df1baa 100644 --- a/src/daemon/handlers/record-trace-recording.ts +++ b/src/daemon/handlers/record-trace-recording.ts @@ -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 { 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; } diff --git a/src/daemon/request-router.ts b/src/daemon/request-router.ts index 39a0418895..aeb297031f 100644 --- a/src/daemon/request-router.ts +++ b/src/daemon/request-router.ts @@ -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 { diff --git a/src/daemon/request-save-script-policy.ts b/src/daemon/request-save-script-policy.ts new file mode 100644 index 0000000000..5a49cb9c65 --- /dev/null +++ b/src/daemon/request-save-script-policy.ts @@ -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 }, + ), + }; +} From ecd3ec6a71a86dd64f2e808e329ba81149d994cd Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 15:55:07 +0000 Subject: [PATCH 2/2] 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 Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 --- .../request-save-script-transports.test.ts | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/daemon/__tests__/request-save-script-transports.test.ts b/src/daemon/__tests__/request-save-script-transports.test.ts index 10af6ad7d7..2cc7f129a1 100644 --- a/src/daemon/__tests__/request-save-script-transports.test.ts +++ b/src/daemon/__tests__/request-save-script-transports.test.ts @@ -121,11 +121,24 @@ async function sendOverHttp( }); const body = (await response.json()) as { result?: { ok: boolean; data?: Record }; - error?: { data?: { code?: string; message?: string; hint?: string } }; + // 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 } = body.error.data; - return { ok: false, error: { code: code ?? 'UNKNOWN', message: message ?? '', hint } }; + 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, @@ -166,6 +179,13 @@ for (const [transport, send] of TRANSPORTS) { 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);