diff --git a/docs/adr/0016-active-session-script-publication.md b/docs/adr/0016-active-session-script-publication.md index 6a9d21f749..b783660075 100644 --- a/docs/adr/0016-active-session-script-publication.md +++ b/docs/adr/0016-active-session-script-publication.md @@ -96,6 +96,20 @@ work, so the caller can retry with plain `close`. Plain `close` tears down ABORT writing; closing an unpublished ARMED recording retains the existing close-time publication behavior. A fresh session is the only re-arming boundary. +> **Amendment (2026-08-02, shipped).** A session that was never armed — no recorded +> `open --save-script` at all — has no fourth pre-ARMED state name here, but it previously fell +> through the same close-time write path as an ARMED recording: `close --save-script` on it folded +> the request into the authoring lifecycle at record time and published anyway. Live evidence +> showed this produces a script whose actions carry selector fallback chains but no recording-time +> `target-v1` evidence, with no signal to the caller that evidence capture never ran — degraded +> replay verification is worse than a loud refusal. `close --save-script` on a never-armed session +> is now rejected before any teardown or filesystem work, the same way ABORTED/PUBLISHED are, +> naming `open --save-script` as the recovery; a plain `close` still tears the session down +> without writing. This is distinct from +> [#1533](https://github.com/callstack/agent-device/issues/1533), which is about an +> already-ARMED-then-ABORTED session whose flag ingress re-enables `recordSession` and lets a +> *bare* `close` (no `--save-script` on the close itself) publish; that case is unresolved here. + This lifecycle is distinct from ADR 0012's repair transaction. `session save-script` rejects a session with `saveScriptBoundary` set and directs the caller to finish or abort the repair through its existing `replay --from` and teardown commit protocol. Active-session publication never marks a repair COMPLETE, @@ -264,6 +278,9 @@ executing that script, not the artifact being saved. - In ABORTED/PUBLISHED, `close --save-script[=]` is rejected before platform close and plain `close` tears down without writing; closing an unpublished ARMED recording preserves current close-time publication behavior. +- On a never-armed session (2026-08-02 amendment), `close --save-script` is likewise rejected before + platform close or filesystem work, naming `open --save-script` as the recovery; plain `close` still + tears down without writing, and the session is not deleted by the rejected request. - Descriptor completeness tests classify every recordable request's mutation effect, including request-sensitive read-only/mutating subcommands, and destination-guard ordering consumes only that trait. diff --git a/src/cli-schema/types.ts b/src/cli-schema/types.ts index 4107169b2b..da303787d6 100644 --- a/src/cli-schema/types.ts +++ b/src/cli-schema/types.ts @@ -11,6 +11,10 @@ export type CommandSchema = { defaults?: Partial; usageOverride?: string; listUsageOverride?: string; + // Swaps a shared flag's usageDescription for this command only, when the flag's generic + // documentation (flag-definitions-*.ts) does not fit every command it is allowed on — for + // example `--save-script` arms authoring on open/close but a repair transaction on replay. + flagDescriptionOverrides?: Partial>; }; export type CommandSchemaOverride = Partial; diff --git a/src/cli/parser/__tests__/cli-help-command-usage.test.ts b/src/cli/parser/__tests__/cli-help-command-usage.test.ts index 46211f8dbb..5c53abeb4e 100644 --- a/src/cli/parser/__tests__/cli-help-command-usage.test.ts +++ b/src/cli/parser/__tests__/cli-help-command-usage.test.ts @@ -407,3 +407,35 @@ test('removed trigger aliases are no longer documented as commands', async () => const help = await usageForCommand('trigger-screenshot-notification'); assert.equal(help, null); }); + +// #1558 follow-up: replay's --save-script arms an ADR 0012 repair transaction, not the +// open/close authoring lifecycle (arm-on-open, publish-on-close) the shared flag describes. +// `flagDescriptionOverrides` (CommandSchema) swaps the text for replay only; open/close keep the +// shared FlagDefinition description unchanged. +test('replay --save-script help describes arming a repair transaction, not open/close authoring', async () => { + const help = await usageForCommand('replay'); + if (help === null) throw new Error('Expected replay help text'); + assert.match(help, /Arm a repair transaction from this replay \(ADR 0012\)/); + assert.match(help, /commits when the repair-armed session tears down/); + assert.doesNotMatch(help, /Arm evidence capture on open, publish the armed recording on close/); +}); + +test('open --save-script help keeps the shared open\\/close authoring description', async () => { + const help = await usageForCommand('open'); + if (help === null) throw new Error('Expected open help text'); + assert.match( + help, + /Arm evidence capture on open, publish the armed recording on close; close --save-script alone \(without an armed open\) is rejected/, + ); + assert.doesNotMatch(help, /Arm a repair transaction from this replay/); +}); + +test('close --save-script help keeps the shared open\\/close authoring description', async () => { + const help = await usageForCommand('close'); + if (help === null) throw new Error('Expected close help text'); + assert.match( + help, + /Arm evidence capture on open, publish the armed recording on close; close --save-script alone \(without an armed open\) is rejected/, + ); + assert.doesNotMatch(help, /Arm a repair transaction from this replay/); +}); diff --git a/src/cli/parser/cli-help.ts b/src/cli/parser/cli-help.ts index 3891f1f46a..979e6c6850 100644 --- a/src/cli/parser/cli-help.ts +++ b/src/cli/parser/cli-help.ts @@ -1243,6 +1243,21 @@ function listHelpFlags(keys: ReadonlySet): FlagDefinition[] { ); } +// Command-specific override for a shared flag's help text (see CommandSchema.flagDescriptionOverrides): +// keeps the FlagDefinition registry as one shared row per flag while letting a command whose +// semantics genuinely differ (e.g. `replay --save-script` arms a repair transaction, not the +// open/close authoring lifecycle) show its own description without duplicating the flag entry. +function applyFlagDescriptionOverrides( + definitions: FlagDefinition[], + overrides: Partial> | undefined, +): FlagDefinition[] { + if (!overrides) return definitions; + return definitions.map((definition) => { + const override = overrides[definition.key]; + return override === undefined ? definition : { ...definition, usageDescription: override }; + }); +} + function renderFlagSection(title: string, definitions: FlagDefinition[]): string { return renderAlignedSection( title, @@ -1302,7 +1317,10 @@ export function buildCommandUsageText(commandName: string): string | null { const schema = getCommandSchema(commandName); if (!schema) return null; const usage = buildCommandUsage(commandName, schema); - const commandFlags = listHelpFlags(new Set(schema.allowedFlags ?? [])); + const commandFlags = applyFlagDescriptionOverrides( + listHelpFlags(new Set(schema.allowedFlags ?? [])), + schema.flagDescriptionOverrides, + ); const sections: string[] = []; if (commandFlags.length > 0) { sections.push(renderFlagSection('Command flags:', commandFlags)); diff --git a/src/commands/cli-grammar/flag-definitions-action.ts b/src/commands/cli-grammar/flag-definitions-action.ts index 9b274bdb7c..c688c6444a 100644 --- a/src/commands/cli-grammar/flag-definitions-action.ts +++ b/src/commands/cli-grammar/flag-definitions-action.ts @@ -248,7 +248,8 @@ export const ACTION_FLAG_DEFINITIONS: readonly FlagDefinition[] = [ names: ['--save-script'], type: 'booleanOrString', usageLabel: '--save-script [path]', - usageDescription: 'Save session script (.ad) on close; optional custom output path', + usageDescription: + 'Arm evidence capture on open, publish the armed recording on close; close --save-script alone (without an armed open) is rejected — start with open --save-script, or use session save-script mid-session. Optional custom output path.', }, { key: 'networkInclude', diff --git a/src/commands/command-explain.ts b/src/commands/command-explain.ts index e0c768423a..fb190abefa 100644 --- a/src/commands/command-explain.ts +++ b/src/commands/command-explain.ts @@ -280,22 +280,26 @@ function describeCliSurface(command: string, schema: CommandSchema): CommandExpl return { usage: buildCommandUsage(command, schema), positionalArgs: schema.positionalArgs ?? [], - commandFlags: describeFlags(schema.allowedFlags ?? []), - supportedFlags: describeFlags(schema.supportedFlags ?? []), + commandFlags: describeFlags(schema.allowedFlags ?? [], schema.flagDescriptionOverrides), + supportedFlags: describeFlags(schema.supportedFlags ?? [], schema.flagDescriptionOverrides), globalFlags: describeFlags([...GLOBAL_FLAG_KEYS]), }; } -function describeFlags(keys: readonly FlagKey[]): CommandFlagExplanation[] { +function describeFlags( + keys: readonly FlagKey[], + overrides?: Partial>, +): CommandFlagExplanation[] { return [...new Set(keys)].sort().map((key) => { const definitions = flagDefinitionsByKey.get(key) ?? []; const preferred = definitions.find((definition) => definition.usageLabel) ?? definitions[0]; + const description = overrides?.[key] ?? preferred?.usageDescription; return { key, syntax: preferred?.usageLabel ?? [...new Set(definitions.flatMap((definition) => definition.names))].join('/'), - ...(preferred?.usageDescription ? { description: preferred.usageDescription } : {}), + ...(description ? { description } : {}), }; }); } diff --git a/src/commands/replay/index.ts b/src/commands/replay/index.ts index c507d4969f..eb0c1743b6 100644 --- a/src/commands/replay/index.ts +++ b/src/commands/replay/index.ts @@ -112,6 +112,13 @@ const replayCliSchema = { 'saveScript', 'force', ], + // ADR 0012 decision 6: on replay, --save-script arms a repair transaction from step 1 (not the + // open/close authoring lifecycle the shared flag description documents) and the healed script + // commits on that transaction's own teardown, not on a plain close. + flagDescriptionOverrides: { + saveScript: + 'Arm a repair transaction from this replay (ADR 0012): recording starts at step 1, and the healed script commits when the repair-armed session tears down (close, close --save-script, or idle-reap). Independent of the open/close authoring arm-on-open. Optional custom output path.', + }, } as const satisfies CommandSchemaOverride; const testCliSchema = { diff --git a/src/daemon/handlers/__tests__/session-close-shutdown.test.ts b/src/daemon/handlers/__tests__/session-close-shutdown.test.ts index 7434422c99..99669b7472 100644 --- a/src/daemon/handlers/__tests__/session-close-shutdown.test.ts +++ b/src/daemon/handlers/__tests__/session-close-shutdown.test.ts @@ -1097,7 +1097,11 @@ test('targeted close skips platform dispatch and preserves the error when the re reason: 'runner_stop_failed', hint: 'Retry once the runner is reachable.', }); - mockStopIosRunnerSession.mockRejectedValue(preCloseError); + // Scoped to this test's own two internal calls (pre-close stop, then the later + // independent-cleanup retry) — a persistent `mockRejectedValue` here would leak into every + // later test that exercises an Apple-platform runner stop, since `vi.clearAllMocks()` in + // `beforeEach` clears call history but not a mock's implementation. + mockStopIosRunnerSession.mockRejectedValueOnce(preCloseError); await expect( handleSessionCommands({ @@ -1131,3 +1135,117 @@ test('targeted close skips platform dispatch and preserves the error when the re expect(mockStopIosRunnerSession.mock.calls.length).toBeGreaterThan(1); expect(sessionStore.get(sessionName)).toBeUndefined(); }); + +// Live evidence (2026-08-02): a plain `open` followed by `close --save-script` used to fold the +// never-armed session into the authoring lifecycle and publish anyway, producing a script with +// selector fallback chains but no recording-time `target-v1` evidence. These two tests prove the +// daemon-seam fix: the rejection fires before ANY teardown work (no dispatch mock needed — a +// no-target close on Android never reaches `dispatchCommand`), the session survives so the agent +// can retry, and no script file is written. +test('close --save-script on a never-armed session is rejected before teardown, with no script written', async () => { + const sessionStore = makeSessionStore(); + const sessionName = 'ios-unarmed-close-save-script-session'; + const scriptPath = path.join(os.tmpdir(), `agent-device-unarmed-close-${Date.now()}.ad`); + // The fixture must carry real cleanup-bearing state (here: an active recording, like + // `makeIosSimulatorRecordingSession`'s other consumers) so this test can actually prove the + // guard runs *before* `stopBestEffortSessionResources` — not just that the response rejects. + // Without it, moving the guard after teardown would still pass: there would be nothing for + // teardown to observably touch. + const session = makeIosSimulatorRecordingSession(sessionName); + const kill = recordingKillMock(session); + sessionStore.set(sessionName, session); + + await expect( + handleSessionCommands({ + req: { + token: 't', + session: sessionName, + command: 'close', + positionals: [], + flags: { saveScript: scriptPath }, + }, + sessionName, + logPath: path.join(os.tmpdir(), 'daemon.log'), + sessionStore, + invoke: noopInvoke, + }), + ).rejects.toMatchObject({ + code: 'INVALID_ARGS', + message: expect.stringMatching(/not armed/), + details: expect.objectContaining({ + hint: expect.stringMatching(/open --save-script/), + }), + }); + + // The rejection does not tear down the session — it stays retryable/recoverable. + expect(sessionStore.get(sessionName)).toBeDefined(); + expect(fs.existsSync(scriptPath)).toBe(false); + // No teardown hook ran: the recording is still live (recorder never signaled) and the runner + // was never told to stop. This is the assertion that goes red if the guard moves after + // `stopBestEffortSessionResources` — see the counterfactual in the PR description. + expect(kill).not.toHaveBeenCalled(); + expect(session.recording).toBeDefined(); + expect(mockStopIosRunnerSession).not.toHaveBeenCalled(); + + // A plain close (no --save-script) still closes the same session cleanly afterward, and now + // teardown genuinely does run: the recorder is signaled and the session deleted. + const plainClose = await handleSessionCommands({ + req: { + token: 't', + session: sessionName, + command: 'close', + positionals: [], + flags: {}, + }, + sessionName, + logPath: path.join(os.tmpdir(), 'daemon.log'), + sessionStore, + invoke: noopInvoke, + }); + expect(plainClose?.ok).toBe(true); + expect(kill).toHaveBeenCalledWith('SIGINT'); + expect(session.recording).toBeUndefined(); + expect(mockStopIosRunnerSession).toHaveBeenCalledWith(session.device.id); + expect(sessionStore.get(sessionName)).toBeUndefined(); +}); + +test('close --save-script on a session with an active .ad repair transaction is unaffected by the unarmed-authoring guard', async () => { + const sessionStore = makeSessionStore(); + const sessionName = 'android-repair-close-save-script-session'; + const session = { + ...makeSession(sessionName, { + platform: 'android', + id: 'emulator-5554', + name: 'Pixel_9_API_35', + kind: 'emulator', + booted: true, + }), + recordSession: true, + scriptPublication: { + kind: 'repair' as const, + status: 'complete' as const, + target: { kind: 'default' as const, force: false }, + boundary: 0, + }, + }; + sessionStore.set(sessionName, session); + + const response = await handleSessionCommands({ + req: { + token: 't', + session: sessionName, + command: 'close', + positionals: [], + flags: { saveScript: true }, + }, + sessionName, + logPath: path.join(os.tmpdir(), 'daemon.log'), + sessionStore, + invoke: noopInvoke, + }); + + // Repair transactions are a disjoint lifecycle (ADR 0012) with their own arming and close-time + // commit protocol; the new unarmed-authoring guard must not intercept them. + expect(response?.ok).toBe(true); + expect(sessionStore.get(sessionName)).toBeUndefined(); +}); diff --git a/src/daemon/handlers/session-close.ts b/src/daemon/handlers/session-close.ts index 1fa738e98c..f652fb844a 100644 --- a/src/daemon/handlers/session-close.ts +++ b/src/daemon/handlers/session-close.ts @@ -285,11 +285,29 @@ async function stopOrRetainAppleRunnerAfterClose( scheduleIosRunnerIdleStop(session.device.id); } +// Live evidence (2026-08-02): a plain `open` followed by `close --save-script` used to fold into +// the authoring lifecycle at close time (`applyRecordedSaveScriptFlags`'s `none -> authoring` +// branch) and publish anyway. That silently produces a script whose actions carry selector +// fallback chains but no `target-v1` recording-time evidence — degraded replay verification with +// no signal to the caller. Recording-time evidence can only be captured from action zero +// (`armAuthoringOnOpen`), so an unarmed session has nothing to retroactively arm; the only +// correct response is refusal, before any teardown or publication work runs. This intentionally +// does not resolve #1533 (aborted-mid-recording close --save-script); that is a distinct, +// already-armed case with its own resolution. function assertTerminalRecordingCloseAllowed(req: DaemonRequest, session: SessionState): void { if (!req.flags?.saveScript) return; if (isAuthoringArmedSession(session)) return; const state = session.scriptPublication; - if (state?.kind !== 'authoring') return; + if (state?.kind === 'repair') return; + if (state === undefined || state.kind === 'none') { + throw new AppError( + 'INVALID_ARGS', + 'close --save-script cannot publish this session: recording was not armed before this journey began, so there is no recording-time target evidence to publish.', + { + hint: 'Retry with plain close (it tears down without writing). To capture a publishable recording, start a fresh session with open --save-script[=].', + }, + ); + } throw new AppError( 'INVALID_ARGS', `close --save-script cannot ${state.status === 'published' ? 're-publish' : 'publish'} this terminal recording. Retry with plain close; it will tear down the session without writing.`, diff --git a/src/daemon/session-script-publication-capability.ts b/src/daemon/session-script-publication-capability.ts index 7d8fcdd6c3..088d378a06 100644 --- a/src/daemon/session-script-publication-capability.ts +++ b/src/daemon/session-script-publication-capability.ts @@ -61,11 +61,14 @@ export function abortAuthoringOnSecondOpen(session: SessionState): void { * other command's raw flag closed at the router). Arms recording and applies target/force to * whichever lifecycle the session is in: * - * - `none` -> ordinary authoring armed. This is how a never-armed `close --save-script` - * publishes the whole log: the close request arms at record time and publishes moments later - * in the same request, folding the former "third mode" into the authoring lifecycle. The - * session is deleted by every close path that gets this far, so the transient armed state is - * unobservable to `session save-script` eligibility. + * - `none` -> ordinary authoring armed. This branch is UNREACHABLE for `close`: live evidence + * (2026-08-02) showed it used to let a never-armed `close --save-script` fold into the + * authoring lifecycle and publish moments later in the same request — a script with selector + * fallback chains but no recording-time `target-v1` evidence, and no signal to the caller. + * `session-close.ts`'s `assertTerminalRecordingCloseAllowed` now rejects an unarmed + * `close --save-script` before any action recording runs, so this arm only fires for a + * future non-close caller of the shared ingress; it is kept as that caller's safety net, not + * as a documented close-time behavior. * - `authoring` -> retarget under the #1258 per-target force rule (`resolveScriptTarget`). * - `repair` -> retarget the repair target the same way (a replayed step may carry the flag). */ diff --git a/test/integration/provider-scenarios/active-session-script-publication.test.ts b/test/integration/provider-scenarios/active-session-script-publication.test.ts index 21f6bfe00c..2503657c07 100644 --- a/test/integration/provider-scenarios/active-session-script-publication.test.ts +++ b/test/integration/provider-scenarios/active-session-script-publication.test.ts @@ -194,6 +194,68 @@ test('a second successful open aborts publication and terminal save flags fail b }); }, 20_000); +// Live evidence (2026-08-02): a plain `open` (no arming) followed by `close --save-script` used to +// silently publish anyway — the close request armed authoring at record time and published moments +// later in the same request, producing a script with selector fallback chains but NO recording-time +// `target-v1` evidence and no signal to the caller that the evidence was missing. The daemon now +// rejects this before any teardown or filesystem work, and the session stays open so a plain close +// still completes cleanly (it just does not publish). +test('an unarmed session refuses close --save-script and closes cleanly on plain close', async () => { + await withProviderScenarioResource(createAndroidSettingsWorld, async (world) => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-unarmed-close-provider-')); + const scriptPath = path.join(root, 'unarmed.ad'); + try { + const opened = await world.daemon.callCommand('open', ['settings'], { ...world.selection }); + assertRpcOk(opened); + assert.equal(authoringPublicationStatus(world), undefined); + + const flaggedClose = await world.daemon.callCommand('close', [], { saveScript: scriptPath }); + assertRpcError(flaggedClose, 'INVALID_ARGS', /not armed/); + assert.ok(world.daemon.session(), 'flagged close must not tear down the unarmed session'); + assert.equal(fs.existsSync(scriptPath), false); + + const plainClose = await world.daemon.callCommand('close'); + assertRpcOk(plainClose); + assert.equal(world.daemon.session(), undefined); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); +}, 20_000); + +// The counterpart to the unarmed refusal above: an `open --save-script`-armed session must still +// publish through the ordinary close-time route (not just through `session save-script`), and the +// published script must carry the same recording-time `target-v1` evidence. +test('an armed session still publishes target-v1 evidence through close --save-script', async () => { + await withProviderScenarioResource(createAndroidSettingsWorld, async (world) => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-armed-close-provider-')); + const scriptPath = path.join(root, 'armed-close.ad'); + const client = world.daemon.client(); + try { + await client.apps.open({ + app: 'settings', + saveScript: scriptPath, + ...world.selection, + }); + assert.equal(authoringPublicationStatus(world), 'armed'); + + const snapshot = await client.capture.snapshot({ interactiveOnly: true, ...world.selection }); + const search = snapshot.nodes.find((node) => node.label === 'Search'); + assert.ok(search?.ref, JSON.stringify(snapshot.nodes)); + await client.interactions.click({ ref: `@${search.ref}`, ...world.selection }); + + const close = await world.daemon.callCommand('close', [], { saveScript: scriptPath }); + assertRpcOk(close); + assert.equal(fs.existsSync(scriptPath), true); + const script = fs.readFileSync(scriptPath, 'utf8'); + assert.match(script, /agent-device:target-v1/); + assert.equal(world.daemon.session(), undefined); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); +}, 20_000); + test('parameterized fill publishes only ${VAR} and replay resolves it immediately before fill', async () => { const secret = 'OpaqueProviderValue1348'; let injectedText: string | undefined; diff --git a/test/integration/provider-scenarios/android-lifecycle.test.ts b/test/integration/provider-scenarios/android-lifecycle.test.ts index 52139225f0..6af601164d 100644 --- a/test/integration/provider-scenarios/android-lifecycle.test.ts +++ b/test/integration/provider-scenarios/android-lifecycle.test.ts @@ -1325,7 +1325,22 @@ async function runAndroidCaptureInteractionAndReplayWorkflow( assert.equal(beforeCloseOpen.appBundleId, 'com.example.demo'); const logsBeforeClose = await client.observability.logs({ action: 'start', ...selection }); assert.equal(logsBeforeClose.started, true); + + // close --save-script now requires the session to have been armed at open (recording-time + // target-v1 evidence cannot be reconstructed retroactively for a session that never recorded + // it). End this long-lived unarmed session plainly, then arm a fresh one before exercising + // close --save-script + shutdown below. + const plainCloseBeforeArm = await daemon.callCommand('close'); + assert.equal(plainCloseBeforeArm.statusCode, 200, JSON.stringify(plainCloseBeforeArm.json)); + assert.equal(daemon.session(), undefined); + const savedReplayPath = path.join(tempRoot, 'saved-session.ad'); + const armedOpen = await client.apps.open({ + app: 'com.example.demo', + saveScript: savedReplayPath, + ...selection, + }); + assert.equal(armedOpen.appBundleId, 'com.example.demo'); const close = await daemon.callCommand('close', [], { saveScript: savedReplayPath, shutdown: true, diff --git a/website/docs/docs/migrating-gestures.md b/website/docs/docs/migrating-gestures.md index a9503fc5c0..fe0b2217ed 100644 --- a/website/docs/docs/migrating-gestures.md +++ b/website/docs/docs/migrating-gestures.md @@ -148,7 +148,9 @@ grep -rnE "\\bgesture[[:space:]]+rotate([[:space:]]+$num){4}" --include='*.ad' . ``` Re-recording also produces a migrated script: the recorder writes the canonical form, so a fresh -`open` → interact → `close --save-script` run is a valid alternative to editing by hand. +`open --save-script` → interact → `close` run is a valid alternative to editing by hand. Recording +evidence is only captured from action zero, so arm at `open`; a bare `close --save-script` on a +session that was not armed at `open` is rejected. ### Maestro flows