From 20d3386ddd6285739daeeb1edc0c1dc1b6b9180e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 14 Jul 2026 18:39:23 +0200 Subject: [PATCH 1/5] feat: add --force/--overwrite for --save-script, arm-time EEXIST preflight #1235 made healed-script publication refuse-on-exist. #1258 adds an escape hatch: --force (alias --overwrite) on open/close/replay makes publishHealedScriptAtomically atomically REPLACE an existing target (renameSync) instead of refusing. The flag threads CLI -> daemon request -> SessionState.saveScriptForce (persisted at arm time, like saveScriptPath, so a later close/auto-commit that doesn't repeat the flag still honors it) -> the publish primitive. Default (flag absent) is unchanged: refuse-on-exist. Second half: an arm-time EEXIST preflight in session-replay-runtime.ts now fails a repair-armed replay --save-script BEFORE any step dispatches when its target already exists and --force is not set, instead of only failing at publish time after the whole repair run (and its corrective steps) has already executed against the device. --- src/client/client-types.ts | 16 ++- .../flag-definitions-connection.ts | 7 +- src/commands/management/app.ts | 11 +- src/commands/replay/index.ts | 5 + .../__tests__/session-script-writer.test.ts | 71 ++++++++++++ .../session-replay-repair-transaction.test.ts | 103 ++++++++++++++++++ src/daemon/handlers/session-close.ts | 17 ++- src/daemon/handlers/session-replay-runtime.ts | 59 +++++++++- src/daemon/session-action-recorder.ts | 10 ++ src/daemon/session-script-writer.ts | 61 +++++++---- src/daemon/session-store.ts | 9 +- src/daemon/types.ts | 12 ++ 12 files changed, 348 insertions(+), 33 deletions(-) diff --git a/src/client/client-types.ts b/src/client/client-types.ts index 338baf409..a7e87dcb6 100644 --- a/src/client/client-types.ts +++ b/src/client/client-types.ts @@ -292,6 +292,8 @@ export type AppOpenOptions = AgentDeviceRequestOverrides & launchArgs?: string[]; relaunch?: boolean; saveScript?: boolean | string; + /** #1258: overwrite an existing --save-script target instead of refusing. Alias: --overwrite. */ + force?: boolean; deviceHub?: boolean; testIme?: boolean; noRecord?: boolean; @@ -316,6 +318,9 @@ export type AppOpenResult = { export type AppCloseOptions = AgentDeviceRequestOverrides & { app?: string; shutdown?: boolean; + saveScript?: boolean | string; + /** #1258: overwrite an existing --save-script target instead of refusing. Alias: --overwrite. */ + force?: boolean; }; export type AppCloseResult = { @@ -866,6 +871,8 @@ export type ReplayRunOptions = AgentDeviceRequestOverrides & * `.healed.ad` when the repair ends with `close --save-script`. */ saveScript?: boolean | string; + /** #1258: overwrite an existing --save-script target instead of refusing. Alias: --overwrite. */ + force?: boolean; }; export type ReplayTestOptions = AgentDeviceRequestOverrides & @@ -1072,6 +1079,8 @@ export type InternalRequestOptions = AgentDeviceClientConfig & relaunch?: boolean; shutdown?: boolean; saveScript?: boolean | string; + /** #1258: overwrite an existing --save-script target instead of refusing. Alias: --overwrite. */ + force?: boolean; deviceHub?: boolean; testIme?: boolean; noRecord?: boolean; @@ -1110,7 +1119,12 @@ export type AgentDeviceClient = { options?: AgentDeviceRequestOverrides & Pick, ) => Promise; close: ( - options?: AgentDeviceRequestOverrides & { shutdown?: boolean }, + options?: AgentDeviceRequestOverrides & { + shutdown?: boolean; + saveScript?: boolean | string; + /** #1258: overwrite an existing --save-script target instead of refusing. Alias: --overwrite. */ + force?: boolean; + }, ) => Promise; artifacts: (options?: CloudArtifactsOptions) => Promise; }; diff --git a/src/commands/cli-grammar/flag-definitions-connection.ts b/src/commands/cli-grammar/flag-definitions-connection.ts index b9b0abb42..612a93d19 100644 --- a/src/commands/cli-grammar/flag-definitions-connection.ts +++ b/src/commands/cli-grammar/flag-definitions-connection.ts @@ -195,10 +195,11 @@ export const CONNECTION_FLAG_DEFINITIONS: readonly FlagDefinition[] = [ }, { key: 'force', - names: ['--force'], + names: ['--force', '--overwrite'], type: 'boolean', - usageLabel: '--force', - usageDescription: 'Force connection state replacement when reconnecting', + usageLabel: '--force, --overwrite', + usageDescription: + 'Force replacement of existing state (connect: replace an active connection; --save-script: overwrite an existing target instead of refusing)', }, { key: 'noLogin', diff --git a/src/commands/management/app.ts b/src/commands/management/app.ts index f0538172d..cd0677772 100644 --- a/src/commands/management/app.ts +++ b/src/commands/management/app.ts @@ -40,6 +40,9 @@ const openCommandMetadata = defineFieldCommandMetadata( ), relaunch: booleanField('Force relaunch.'), saveScript: jsonSchemaField({ oneOf: [booleanSchema(), stringSchema()] }), + force: booleanField( + 'Overwrite an existing --save-script target instead of refusing (alias: --overwrite).', + ), deviceHub: booleanField('Use Xcode Device Hub when surfacing Apple simulators.'), testIme: booleanField( 'Activate the headless Android test IME for deterministic Unicode text entry (default on for emulators; opt-in on real devices).', @@ -65,6 +68,9 @@ const closeCommandMetadata = defineFieldCommandMetadata( app: stringField('Optional app to close.'), shutdown: booleanField('Shutdown the session/device where supported.'), saveScript: jsonSchemaField({ oneOf: [booleanSchema(), stringSchema()] }), + force: booleanField( + 'Overwrite an existing --save-script target instead of refusing (alias: --overwrite).', + ), }, ); @@ -131,6 +137,7 @@ const openCliSchema = { 'deviceHub', 'testIme', 'saveScript', + 'force', 'noRecord', 'relaunch', 'surface', @@ -143,7 +150,7 @@ const openCliSchema = { const closeCliSchema = { positionalArgs: ['app?'], - allowedFlags: ['saveScript', 'shutdown'], + allowedFlags: ['saveScript', 'force', 'shutdown'], } as const satisfies CommandSchemaOverride; const appsCliReader: CliReader = (_positionals, flags) => ({ @@ -161,6 +168,7 @@ const openCliReader: CliReader = (positionals, flags) => ({ launchArgs: flags.launchArgs, relaunch: flags.relaunch, saveScript: flags.saveScript, + force: flags.force, deviceHub: flags.deviceHub, testIme: flags.testIme, noRecord: flags.noRecord, @@ -175,6 +183,7 @@ const closeCliReader: CliReader = (positionals, flags) => ({ app: positionals[0], shutdown: flags.shutdown, saveScript: flags.saveScript, + force: flags.force, }); const appsDaemonWriter: DaemonWriter = direct(PUBLIC_COMMANDS.apps); diff --git a/src/commands/replay/index.ts b/src/commands/replay/index.ts index 365dbc879..a0e85a9ae 100644 --- a/src/commands/replay/index.ts +++ b/src/commands/replay/index.ts @@ -49,6 +49,9 @@ export const replayCommandMetadata = defineFieldCommandMetadata( // from the first replay attempt; optional string value is the healed // script's output path. saveScript: jsonSchemaField({ oneOf: [booleanSchema(), stringSchema()] }), + // #1258: overwrite an existing --save-script target (arm-time preflight + + // publish) instead of refusing. Alias: --overwrite. + force: booleanField(), }, ); @@ -96,6 +99,7 @@ const replayCliSchema = { 'timeoutMs', 'out', 'saveScript', + 'force', ], } as const satisfies CommandSchemaOverride; @@ -130,6 +134,7 @@ export const replayCliReader: CliReader = (positionals, flags) => ({ resumeFrom: flags.replayFrom, resumePlanDigest: flags.replayPlanDigest, saveScript: flags.saveScript, + force: flags.force, }); export const testCliReader: CliReader = (positionals, flags) => ({ diff --git a/src/daemon/__tests__/session-script-writer.test.ts b/src/daemon/__tests__/session-script-writer.test.ts index 56565f213..6c41076a9 100644 --- a/src/daemon/__tests__/session-script-writer.test.ts +++ b/src/daemon/__tests__/session-script-writer.test.ts @@ -283,6 +283,77 @@ test('write() now refuses to clobber a stale PARTIAL (non-sentinel) .healed.ad a expect(fs.readFileSync(healedPath, 'utf8')).toBe(before); }); +// --- #1258: `--force`/`--overwrite` opts into replacing an existing target +// atomically instead of refusing. --- + +test('write(session, { force: true }) overwrites an existing COMPLETE target atomically', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-force-')); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const healedPath = path.join(root, 'flows', 'login.healed.ad'); + fs.mkdirSync(path.dirname(healedPath), { recursive: true }); + fs.writeFileSync( + healedPath, + `context platform=ios device="x"\nclick id="old"\n${HEAL_COMPLETE_SENTINEL}\n`, + ); + + const session = makeIosSession('default', { + recordSession: true, + saveScriptBoundary: 0, + saveScriptComplete: true, + saveScriptPath: healedPath, + saveScriptDefaultedHealedPath: true, + actions: [action({ command: 'click', positionals: ['id="new"'] })], + }); + + const result = writer.write(session, { force: true }); + expect(result.written).toBe(true); + expect(result.written && result.path).toBe(healedPath); + const parsed = parseReplayScriptDetailed(fs.readFileSync(healedPath, 'utf8')); + expect(parsed.actions.map((a) => a.positionals[0])).toEqual(['id="new"']); + // The overwrite is atomic (rename-replace): only the published target + // remains in the directory, no stray temp file left behind. + expect(fs.readdirSync(path.dirname(healedPath))).toEqual([path.basename(healedPath)]); +}); + +test('write(session, { force: true }) overwrites an existing target for ORDINARY (non-repair) recording too', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-force-ordinary-')); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const outPath = path.join(root, 'flows', 'existing.ad'); + fs.mkdirSync(path.dirname(outPath), { recursive: true }); + fs.writeFileSync(outPath, 'context platform=ios device="x"\nclick id="old"\n'); + + const session = makeIosSession('default', { + recordSession: true, + // No saveScriptBoundary: ordinary open/close --save-script, not a repair. + saveScriptPath: outPath, + actions: [action({ command: 'click', positionals: ['id="new"'] })], + }); + + const result = writer.write(session, { force: true }); + expect(result.written).toBe(true); + const parsed = parseReplayScriptDetailed(fs.readFileSync(outPath, 'utf8')); + expect(parsed.actions.map((a) => a.positionals[0])).toEqual(['id="new"']); +}); + +test('write(session) without { force: true } still refuses, even when a prior write in the same test used force', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-force-default-')); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const outPath = path.join(root, 'flows', 'existing.ad'); + fs.mkdirSync(path.dirname(outPath), { recursive: true }); + fs.writeFileSync(outPath, 'context platform=ios device="x"\nclick id="old"\n'); + const before = fs.readFileSync(outPath, 'utf8'); + + const session = makeIosSession('default', { + recordSession: true, + saveScriptPath: outPath, + actions: [action({ command: 'click', positionals: ['id="new"'] })], + }); + + // Default (no options / force omitted): refuse-on-exist, unchanged. + expect(() => writer.write(session)).toThrow(/already exists/); + expect(fs.readFileSync(outPath, 'utf8')).toBe(before); +}); + // The lock/lease machinery previously here (acquireLease/releaseLease/ // stealExpiredLease/the three-writer interleaving) is gone: the publish // primitive is now a single exclusive `linkSync`, which already decides a diff --git a/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts b/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts index 83d8062c5..8b9ed460c 100644 --- a/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts @@ -65,6 +65,7 @@ import { AppError } from '../../../kernel/errors.ts'; import { makeIosSession } from '../../../__tests__/test-utils/session-factories.ts'; import { HEAL_COMPLETE_SENTINEL } from '../../session-script-writer.ts'; import { parseReplayScriptDetailed } from '../../../replay/script.ts'; +import type { DaemonRequest } from '../../types.ts'; import { baseReplayRequest as baseReq, writeReplayFile, @@ -512,6 +513,108 @@ test('BLOCKER 2b/2c: a close whose commit FAILS (no-clobber) keeps the session f expect(promoted.actions.filter((a) => a.command === 'close')).toHaveLength(1); }); +// --- #1258: `--force`/`--overwrite` on `--save-script`. --- + +test('#1258: close --save-script --force overwrites an existing COMPLETE healed .ad instead of refusing', async () => { + const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup( + 'agent-device-repair-transaction-force-overwrite-', + ); + makeCompleteRepairSession(sessionStore, sessionName, root); + // A prior COMPLETE (sentinel-marked) healed artifact already sits at the + // default path — `--force` must overwrite it instead of refusing. + fs.writeFileSync( + path.join(root, 'flow.healed.ad'), + `context platform=ios device="x"\nclick id="old"\n${HEAL_COMPLETE_SENTINEL}\n`, + ); + + const closeResponse = await handleCloseCommand({ + req: { + token: 't', + session: sessionName, + command: 'close', + positionals: [], + flags: { force: true }, + }, + sessionName, + logPath, + sessionStore, + leaseRegistry, + }); + + expect(closeResponse.ok).toBe(true); + expect(sessionStore.get(sessionName)).toBeUndefined(); + const healedPath = path.join(root, 'flow.healed.ad'); + if (closeResponse.ok) expect(closeResponse.data?.savedScript).toBe(healedPath); + const script = fs.readFileSync(healedPath, 'utf8'); + expect(script).toContain(HEAL_COMPLETE_SENTINEL); + const parsed = parseReplayScriptDetailed(script); + // The new repair's own actions replaced the stale prior artifact — the old + // content is gone, never merged/appended. + expect(parsed.actions.some((a) => a.positionals[0] === 'id="old"')).toBe(false); + expect(parsed.actions.map((a) => a.command)).toEqual(['open', 'press', 'close']); +}); + +test('#1258 arm-time preflight: an existing --save-script target rejects BEFORE any step runs', async () => { + const { root, sessionStore, sessionName, logPath } = setup( + 'agent-device-repair-transaction-arm-preflight-', + ); + const filePath = writeReplayFile(root, ['open "Demo" --relaunch', 'close']); + // The default healed sibling already exists — a prior repair, or an + // unrelated stale file sitting at that path. + fs.writeFileSync(path.join(root, 'flow.healed.ad'), 'context platform=ios device="x"\n'); + + const spy: DaemonRequest[] = []; + const invoke = makeRecordingReplayInvoke({ sessionStore, sessionName, spy }); + + const result = await runReplayScriptFile({ + req: baseReq({ positionals: [filePath], flags: { saveScript: true } }), + sessionName, + logPath, + sessionStore, + invoke, + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error.code).toBe('COMMAND_FAILED'); + expect(result.error.message).toMatch(/already exists/); + } + // Not a single step dispatched — the preflight fired BEFORE the loop, not + // merely at the end (publish time). Even `open` never ran. + expect(spy).toHaveLength(0); + // The session was never armed (no boundary stamped) either — the whole + // repair-arm side effect is skipped, not just the dispatch. + expect(sessionStore.get(sessionName)?.saveScriptBoundary).toBeUndefined(); +}); + +test('#1258: --force skips the arm-time preflight and the replay proceeds despite the existing target', async () => { + const { root, sessionStore, sessionName, logPath } = setup( + 'agent-device-repair-transaction-arm-preflight-force-', + ); + const filePath = writeReplayFile(root, ['open "Demo" --relaunch', 'close']); + fs.writeFileSync(path.join(root, 'flow.healed.ad'), 'context platform=ios device="x"\n'); + + const spy: DaemonRequest[] = []; + const invoke = makeRecordingReplayInvoke({ sessionStore, sessionName, spy }); + + const result = await runReplayScriptFile({ + req: baseReq({ positionals: [filePath], flags: { saveScript: true, force: true } }), + sessionName, + logPath, + sessionStore, + invoke, + }); + + expect(result.ok).toBe(true); + // `open` dispatched — the preflight did not block it. The terminal `close` + // is skipped while repair-armed (existing, unrelated behavior), so `open` + // is the only step this minimal script actually dispatches. + expect(spy.map((r) => r.command)).toEqual(['open']); + // `force` is persisted on the session from arm time, so a LATER commit + // (e.g. a bare `close`, or teardown) still honors the overwrite. + expect(sessionStore.get(sessionName)?.saveScriptForce).toBe(true); +}); + test('BLOCKER 2 (new): a repair close whose PLATFORM close fails never commits a healed .ad claiming a successful close', async () => { const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup( 'agent-device-repair-transaction-platform-close-fail-', diff --git a/src/daemon/handlers/session-close.ts b/src/daemon/handlers/session-close.ts index 2aea79e71..1119a9b63 100644 --- a/src/daemon/handlers/session-close.ts +++ b/src/daemon/handlers/session-close.ts @@ -48,6 +48,17 @@ async function maybeShutdownSessionTarget(params: { return await shutdownDeviceTarget(device); } +/** + * #1258: the effective `--force`/`--overwrite` decision for a `close`-time + * publish — this close request's own flag OR'd with whatever was persisted + * on the session at an earlier arm (`open --save-script --force`, or a + * repair's first `replay --save-script --force`). Either source authorizes + * overwriting; neither being set keeps the default refuse-on-exist. + */ +function resolveEffectiveSaveScriptForce(req: DaemonRequest, session: SessionState): boolean { + return Boolean(req.flags?.force || session.saveScriptForce); +} + function shouldRetainAppleRunnerAfterClose(req: DaemonRequest, session: SessionState): boolean { return ( isIosSimulator(session.device) && @@ -91,7 +102,9 @@ function commitRepairBeforeClose( session: session.name, ...successText(`Closed: ${session.name}`), }); - const result = sessionStore.writeSessionLog(session); + const result = sessionStore.writeSessionLog(session, { + force: resolveEffectiveSaveScriptForce(req, session), + }); if (result.written) return { kind: 'committed', path: result.path }; if (result.error) { // The session is kept for retry (BLOCKER 2b): roll back the just-recorded @@ -207,7 +220,7 @@ async function runSessionCloseTeardown(params: { if (req.flags?.saveScript) { session.recordSession = true; } - sessionStore.writeSessionLog(session); + sessionStore.writeSessionLog(session, { force: resolveEffectiveSaveScriptForce(req, session) }); } await attemptCleanup('materialized_paths', () => cleanupRetainedMaterializedPathsForSession(sessionName), diff --git a/src/daemon/handlers/session-replay-runtime.ts b/src/daemon/handlers/session-replay-runtime.ts index ddd85f852..b83b86a84 100644 --- a/src/daemon/handlers/session-replay-runtime.ts +++ b/src/daemon/handlers/session-replay-runtime.ts @@ -250,8 +250,22 @@ export async function runReplayScriptFile(params: { // flag and let a later `close` commit a now-stale transaction. const preRunSession = sessionStore.get(sessionName); if (preRunSession?.saveScriptBoundary !== undefined) preRunSession.saveScriptComplete = false; + // #1258: arm-time EEXIST preflight — fail HERE, before any step of this + // invocation dispatches, rather than letting the whole run (and, for a + // fresh repair, its very first `open`) execute only to hit the SAME + // refuse-on-exist at publish time (`publishHealedScriptAtomically`, at + // `close`/completion). Computes the SAME target `armReplaySaveScriptStep` + // below would resolve, without needing the session to exist yet. + const saveScriptPreflight = preflightSaveScriptTarget({ + saveScript: req.flags?.saveScript, + force: req.flags?.force, + sourcePath: resolved, + existingSaveScriptPath: preRunSession?.saveScriptPath, + }); + if (saveScriptPreflight) return saveScriptPreflight; const armReplaySaveScriptStep = createReplaySaveScriptArmer({ saveScript: req.flags?.saveScript, + force: req.flags?.force, sessionStore, sessionName, sourcePath: resolved, @@ -503,6 +517,39 @@ function preflightReplayAgainstActiveRepair(params: { ); } +/** + * #1258: arm-time EEXIST preflight. Absent this, a repair-armed run's target + * is only checked at PUBLISH time (`publishHealedScriptAtomically`, on + * `close`/completion) — by then the ENTIRE repair (agent's corrective steps + * included) may already have executed against the device, only to fail on a + * pre-existing target at the very end. Resolves the SAME target + * `armReplaySaveScriptStep` would (explicit `--save-script=` always + * wins; otherwise an already-armed session's existing path if this is a + * `--from` continuation leg reusing it, else the default `.healed.ad` + * sibling) WITHOUT needing the session to exist yet, so it runs before step 1 + * dispatches even when that step is the `open` that creates the session. A + * no-op when `--save-script` was not passed this invocation, or when `force` + * was — the caller explicitly opted into overwriting. + */ +function preflightSaveScriptTarget(params: { + saveScript: boolean | string | undefined; + force: boolean | undefined; + sourcePath: string; + existingSaveScriptPath: string | undefined; +}): DaemonResponse | undefined { + const { saveScript, force, sourcePath, existingSaveScriptPath } = params; + if (!saveScript || force) return undefined; + const targetPath = + typeof saveScript === 'string' + ? expandSessionPath(saveScript) + : (existingSaveScriptPath ?? healedScriptSiblingPath(sourcePath)); + if (!fs.existsSync(targetPath)) return undefined; + return errorResponse( + 'COMMAND_FAILED', + `A file already exists at ${targetPath}; remove it, pass replay --save-script=, or pass --force/--overwrite to replace it.`, + ); +} + /** * ADR 0012 decision 6 (Fix 3): the source plan's own terminal `close` is * lifecycle, not a script step to replay, while a repair is armed — the agent @@ -536,15 +583,16 @@ function isRepairArmedTerminalClose(params: { */ function createReplaySaveScriptArmer(params: { saveScript: boolean | string | undefined; + force: boolean | undefined; sessionStore: SessionStore; sessionName: string; sourcePath: string; }): () => void { - const { saveScript, sessionStore, sessionName, sourcePath } = params; + const { saveScript, force, sessionStore, sessionName, sourcePath } = params; if (!saveScript) return () => {}; let firstArm = true; return () => { - armReplaySaveScriptStep({ sessionStore, sessionName, saveScript, sourcePath, firstArm }); + armReplaySaveScriptStep({ sessionStore, sessionName, saveScript, force, sourcePath, firstArm }); firstArm = false; }; } @@ -564,13 +612,18 @@ function armReplaySaveScriptStep(params: { sessionStore: SessionStore; sessionName: string; saveScript: boolean | string; + force: boolean | undefined; sourcePath: string; firstArm: boolean; }): void { - const { sessionStore, sessionName, saveScript, sourcePath, firstArm } = params; + const { sessionStore, sessionName, saveScript, force, sourcePath, firstArm } = params; const session = sessionStore.get(sessionName); if (!session) return; session.recordSession = true; + // #1258: sticky, like `saveScriptPath` — persisted so a LATER `--from` + // continuation leg or an unattended auto-commit teardown (no live request) + // still honors a `--force`/`--overwrite` that was only passed at this arm. + if (force) session.saveScriptForce = true; if (typeof saveScript === 'string') { // An EXPLICIT `--save-script=` clears the defaulted marker // (invariant: the marker is set iff the current `saveScriptPath` was diff --git a/src/daemon/session-action-recorder.ts b/src/daemon/session-action-recorder.ts index 4b056a1f3..784d2e177 100644 --- a/src/daemon/session-action-recorder.ts +++ b/src/daemon/session-action-recorder.ts @@ -32,6 +32,15 @@ export function recordActionEntry( session.saveScriptPath = expandSessionPath(entry.flags.saveScript); session.saveScriptDefaultedHealedPath = false; } + // #1258: persist `--force`/`--overwrite`, like `saveScriptPath`, so a + // LATER write that does not repeat the flag (a bare `close` finishing a + // session opened with `open --save-script --force`, or an unattended + // auto-commit teardown with no live request) still honors it. Sticky — + // once true, a later action carrying `saveScript` without `force` must + // not clear it back to false. + if (entry.flags.force) { + session.saveScriptForce = true; + } } const action: SessionAction = { ts: Date.now(), @@ -72,6 +81,7 @@ const SANITIZED_FLAG_KEYS = [ ...SCREENSHOT_ACTION_FLAG_KEYS, 'relaunch', 'saveScript', + 'force', 'noRecord', 'fps', 'quality', diff --git a/src/daemon/session-script-writer.ts b/src/daemon/session-script-writer.ts index 2ca176a78..82bd76112 100644 --- a/src/daemon/session-script-writer.ts +++ b/src/daemon/session-script-writer.ts @@ -75,7 +75,7 @@ export class SessionScriptWriter { this.sessionsDir = sessionsDir; } - write(session: SessionState): SessionScriptWriteResult { + write(session: SessionState, options?: { force?: boolean }): SessionScriptWriteResult { const repairArmed = session.saveScriptBoundary !== undefined; let scriptPath: string | undefined; try { @@ -85,7 +85,11 @@ export class SessionScriptWriter { const scriptDir = path.dirname(scriptPath); if (!fs.existsSync(scriptDir)) fs.mkdirSync(scriptDir, { recursive: true }); const script = formatSessionScript(session, repairArmed); - publishHealedScriptAtomically({ scriptPath, script }); + // #1258: `options.force` is the caller's already-merged decision + // (typically `req.flags?.force || session.saveScriptForce` — see + // `session-close.ts`/`session-store.ts`), not read from `session` + // directly here, so this stays a pure formatting+publish step. + publishHealedScriptAtomically({ scriptPath, script, force: options?.force }); // COMMITTED: idempotent guard above + teardown's abort/tombstone routing. if (repairArmed) session.saveScriptCommitted = true; return { written: true, path: scriptPath }; @@ -159,29 +163,40 @@ function formatSessionScript(session: SessionState, appendCompleteSentinel: bool * ADR 0012 decision 6, no-clobber (maintainer-approved simplification): * publishes `script` to `scriptPath` atomically, refusing ANY pre-existing * target — complete or partial, the default healed sibling or an explicit - * `--save-script=` alike. + * `--save-script=` alike — UNLESS `force` is set (#1258). * * This is `write()`'s ONLY publish primitive, called unconditionally for * every target — a repair-armed heal AND an ordinary, non-repair * `open`/`close --save-script` recording alike. There is no - * repair-armed-vs-ordinary branch here (an earlier design had one — - * `publishOverwriteAtomically`, rename-replace for ordinary writes — since - * removed): an ordinary recording's target is refused exactly like a healed - * repair's, never silently overwritten. `--force`/`--overwrite` is a future - * escape hatch (#1258), not implemented today. + * repair-armed-vs-ordinary branch here: an ordinary recording's target is + * refused exactly like a healed repair's, and `force` overwrites exactly + * like a healed repair's. * * The temp file is created in the SAME DIRECTORY as the target (never - * `/tmp`), so the publish itself is a single intra-directory `linkSync`: - * atomic create-exclusive, first writer wins. That single primitive is - * enough — a concurrent complete-vs-complete race is already correct this - * way (the loser sees `EEXIST` and is refused), and a partial healed file - * left behind by an aborted/reaped repair is a degenerate state: the caller - * clears it explicitly (remove it, or pick another `--save-script` path) - * rather than having it silently replaced. No lock, no lease, no steal, no - * overwrite. + * `/tmp`). Default (no `force`): the publish is a single intra-directory + * `linkSync`: atomic create-exclusive, first writer wins. That single + * primitive is enough — a concurrent complete-vs-complete race is already + * correct this way (the loser sees `EEXIST` and is refused), and a partial + * healed file left behind by an aborted/reaped repair is a degenerate + * state: the caller clears it explicitly (remove it, or pick another + * `--save-script` path) rather than having it silently replaced. No lock, + * no lease, no steal, no overwrite. + * + * `force`/`--overwrite` (#1258): `renameSync` instead — an atomic REPLACE + * within the same directory (on POSIX; Node's Windows implementation uses + * `MoveFileEx` with `MOVEFILE_REPLACE_EXISTING`), so there is no window + * where `scriptPath` is briefly missing and no separate unlink step. The + * caller opted into overwriting explicitly (a live `--force`/`--overwrite`, + * or one persisted on the session from arm time — see + * `SessionState.saveScriptForce`), so first-writer-wins no longer applies: + * the last write wins instead, same as any ordinary file overwrite. */ -function publishHealedScriptAtomically(params: { scriptPath: string; script: string }): void { - const { scriptPath, script } = params; +function publishHealedScriptAtomically(params: { + scriptPath: string; + script: string; + force?: boolean; +}): void { + const { scriptPath, script, force } = params; const dir = path.dirname(scriptPath); const tempPath = path.join( dir, @@ -189,17 +204,23 @@ function publishHealedScriptAtomically(params: { scriptPath: string; script: str ); fs.writeFileSync(tempPath, script); try { + if (force) { + fs.renameSync(tempPath, scriptPath); + return; + } // Atomic create-exclusive: EEXIST iff a file already sits at scriptPath. fs.linkSync(tempPath, scriptPath); } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; throw new AppError( 'COMMAND_FAILED', - `A file already exists at ${scriptPath}; remove it or pass replay --save-script= so an existing healed script is never overwritten.`, + `A file already exists at ${scriptPath}; remove it, pass replay --save-script=, or pass --force/--overwrite to replace it.`, ); } finally { // linkSync leaves the temp hard-link behind on success; an error leaves - // it too — always clean up whatever of our own temp remains. + // it too — always clean up whatever of our own temp remains. A `force` + // rename already consumed tempPath (it no longer exists at this path), + // so this is a harmless no-op in that branch. fs.rmSync(tempPath, { force: true }); } } diff --git a/src/daemon/session-store.ts b/src/daemon/session-store.ts index 7c82f6db7..d6c64f2cb 100644 --- a/src/daemon/session-store.ts +++ b/src/daemon/session-store.ts @@ -110,8 +110,8 @@ export class SessionStore { ); } - writeSessionLog(session: SessionState): SessionScriptWriteResult { - const result = this.scriptWriter.write(session); + writeSessionLog(session: SessionState, options?: { force?: boolean }): SessionScriptWriteResult { + const result = this.scriptWriter.write(session, options); if (result.written) { emitDiagnostic({ level: 'info', @@ -151,7 +151,10 @@ export class SessionStore { */ finalizeRepairTeardown(session: SessionState): void { this.recordRepairFinalizeCloseIfCommitting(session); - const result = this.writeSessionLog(session); + // #1258: no live request here (idle-reap/daemon-shutdown teardown), so + // the only source of `force` is whatever was persisted on the session at + // arm time. + const result = this.writeSessionLog(session, { force: session.saveScriptForce }); if (session.saveScriptBoundary !== undefined && session.saveScriptCommitted !== true) { if (!result.written && result.error) { this.writeRepairTombstone(session, REPAIR_TOMBSTONE_TTL_MS, { diff --git a/src/daemon/types.ts b/src/daemon/types.ts index 2481c244c..289d9351a 100644 --- a/src/daemon/types.ts +++ b/src/daemon/types.ts @@ -320,6 +320,18 @@ export type SessionState = { recordOnlySession?: boolean; recordSession?: boolean; saveScriptPath?: string; + /** + * #1258: `--force`/`--overwrite` captured at the moment `--save-script` was + * armed (`open --save-script --force`, `close --save-script --force`, or + * `replay --save-script --force`'s first arm) — persisted here, like + * `saveScriptPath`, so a LATER write that does not repeat the flag (a bare + * `close` finishing a session opened with `open --save-script --force`, a + * `--from` continuation leg, or an unattended auto-commit teardown with no + * live request at all) still honors the overwrite the caller opted into up + * front. The effective decision at any write site is `req.flags?.force || + * session.saveScriptForce`; once set true it is never cleared. + */ + saveScriptForce?: boolean; /** * ADR 0012 decision 6, R6: `session.actions.length` at the `replay * --save-script` invocation that armed this session — the repair-run From ec0e7f331de8acd3856671b2a5963f070be3d634 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 14 Jul 2026 18:53:30 +0200 Subject: [PATCH 2/5] refactor: extract write() catch-block into handleSessionScriptWriteFailure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pure structural refactor, no behavior change: moves the diagnose + classify-and-return / AppError-rethrow logic out of SessionScriptWriter.write into a module-level helper. Drops write()'s cyclomatic below Fallow's threshold (the finding CI flagged on #1266) — the extracted throw still propagates out of the catch exactly as before. --- src/daemon/session-script-writer.ts | 70 +++++++++++++++++------------ 1 file changed, 42 insertions(+), 28 deletions(-) diff --git a/src/daemon/session-script-writer.ts b/src/daemon/session-script-writer.ts index 82bd76112..6dfcb60f6 100644 --- a/src/daemon/session-script-writer.ts +++ b/src/daemon/session-script-writer.ts @@ -94,34 +94,7 @@ export class SessionScriptWriter { if (repairArmed) session.saveScriptCommitted = true; return { written: true, path: scriptPath }; } catch (error) { - emitDiagnostic({ - level: 'warn', - phase: 'session_script_write_failed', - data: { - session: session.name, - path: scriptPath, - error: error instanceof Error ? error.message : String(error), - }, - }); - // ADR 0012 decision 6, R4 + BLOCKER 2: a repair COMMIT failure must be - // SURFACED (no-clobber refusal, bare-`@ref`, or a filesystem error alike) - // so close/teardown can report it and keep the session for retry — never - // swallowed into a silent `{written:false}`. Ordinary (non-repair) - // recording keeps its existing SHAPE of behavior: an AppError still - // fails loud (thrown, not swallowed into `{written:false}`) and any other - // fs error is a quiet skip — but an AppError is no longer only - // theoretical here. Since `publishHealedScriptAtomically` refuses ANY - // pre-existing target uniformly (maintainer-approved: refuse-on-exist - // applies to ordinary recording too, not just repair heals), an ordinary - // `open`/`close --save-script` write against an existing target now - // throws that same no-clobber AppError, surfacing here as a genuine - // "fails loud" case rather than the "none is raised on that path" it was - // before that change. - if (repairArmed) { - return { written: false, error: toRepairCommitFailure(error, scriptPath) }; - } - if (error instanceof AppError) throw error; - return { written: false }; + return handleSessionScriptWriteFailure({ session, error, scriptPath, repairArmed }); } } @@ -136,6 +109,47 @@ export class SessionScriptWriter { } } +/** + * `write()`'s catch-block classifier, extracted verbatim (no behavior change): + * diagnose, then route by whether the session is repair-armed. + * + * ADR 0012 decision 6, R4 + BLOCKER 2: a repair COMMIT failure must be + * SURFACED (no-clobber refusal, bare-`@ref`, or a filesystem error alike) so + * close/teardown can report it and keep the session for retry — never + * swallowed into a silent `{written:false}`. Ordinary (non-repair) recording + * keeps its existing SHAPE of behavior: an AppError still fails loud (thrown, + * not swallowed into `{written:false}`) and any other fs error is a quiet + * skip — but an AppError is no longer only theoretical here. Since + * `publishHealedScriptAtomically` refuses ANY pre-existing target uniformly + * (maintainer-approved: refuse-on-exist applies to ordinary recording too, not + * just repair heals), an ordinary `open`/`close --save-script` write against + * an existing target now throws that same no-clobber AppError, surfacing here + * as a genuine "fails loud" case rather than the "none is raised on that path" + * it was before that change. + */ +function handleSessionScriptWriteFailure(params: { + session: SessionState; + error: unknown; + scriptPath: string | undefined; + repairArmed: boolean; +}): SessionScriptWriteResult { + const { session, error, scriptPath, repairArmed } = params; + emitDiagnostic({ + level: 'warn', + phase: 'session_script_write_failed', + data: { + session: session.name, + path: scriptPath, + error: error instanceof Error ? error.message : String(error), + }, + }); + if (repairArmed) { + return { written: false, error: toRepairCommitFailure(error, scriptPath) }; + } + if (error instanceof AppError) throw error; + return { written: false }; +} + /** * ADR 0012 decision 6 (BLOCKER 2c): normalizes a repair-commit failure into a * distinct, surfaceable AppError. A no-clobber refusal or a bare-`@ref` failure From 579bae354ffffe4d95fe48e50f119117cf07aa5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 14 Jul 2026 20:16:37 +0200 Subject: [PATCH 3/5] fix: make persisted save-script force consistent (preflight + per-target) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two authorization inconsistencies in the #1258 --force work flagged in re-review: 1. Arm-time EEXIST preflight now uses the SAME effective force decision as publication — `req.flags?.force || preRunSession.saveScriptForce` — instead of the live flag alone. A repair armed with `--save-script --force` and continued via `replay --from … --save-script` (without repeating --force) is no longer rejected on a target the earlier forced leg authorized. 2. Force is now per-target, not sticky-across-retarget. Re-arming a DIFFERENT `--save-script=` without a live --force CLEARS the persisted `saveScriptForce` (new shared `applySaveScriptRetarget`, used by both the replay armer and `recordActionEntry`/close), so a retarget can never silently overwrite a file nobody opted into. A live --force on the retarget re-grants it for the new target. Updated the SessionState doc accordingly. Regressions: persisted-force `--from` continuation is not preflight-rejected; retarget-without-force refuses an existing target (and the --force contrast overwrites it). Both verified to fail without their respective fix. --- .../session-replay-repair-transaction.test.ts | 142 ++++++++++++++++++ src/daemon/handlers/session-replay-runtime.ts | 21 ++- src/daemon/session-action-recorder.ts | 35 ++++- src/daemon/types.ts | 8 +- 4 files changed, 194 insertions(+), 12 deletions(-) diff --git a/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts b/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts index 8b9ed460c..edaa9e14d 100644 --- a/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts @@ -615,6 +615,148 @@ test('#1258: --force skips the arm-time preflight and the replay proceeds despit expect(sessionStore.get(sessionName)?.saveScriptForce).toBe(true); }); +test('#1258 preflight honors PERSISTED force: a --from continuation without --force is NOT rejected on an existing target a prior --force leg authorized', async () => { + const { root, sessionStore, sessionName, logPath } = setup( + 'agent-device-repair-transaction-preflight-persisted-force-', + ); + const filePath = writeReplayFile(root, [ + 'open "Demo" --relaunch', + SAVE_ANNOTATION, + 'click id="save"', + 'click id="confirm"', + 'close', + ]); + // The healed target already exists — the preflight WOULD reject a + // continuation that only saw the (absent, this leg) LIVE force flag. + fs.writeFileSync(path.join(root, 'flow.healed.ad'), 'context platform=ios device="x"\n'); + const invoke = makeRecordingReplayInvoke({ + sessionStore, + sessionName, + evidence: (req) => (req.command === 'click' ? freshEvidence('confirm', 'Confirm') : undefined), + }); + + // Leg 1: `replay --save-script --force` — the LIVE force passes the preflight + // (target exists), arms the session, PERSISTS saveScriptForce, then diverges + // (nothing published, so the pre-existing target survives). + const leg1 = await runReplayScriptFile({ + req: baseReq({ positionals: [filePath], flags: { saveScript: true, force: true } }), + sessionName, + logPath, + sessionStore, + invoke, + }); + expect(leg1.ok).toBe(false); + if (leg1.ok) return; + expect(leg1.error.code).toBe('REPLAY_DIVERGENCE'); + const divergence = leg1.error.details?.divergence as { resume: { planDigest: string } }; + expect(sessionStore.get(sessionName)?.saveScriptForce).toBe(true); + + // The agent's corrective press (blessed @ref), recorded live. + const session = sessionStore.get(sessionName)!; + sessionStore.recordAction(session, { + command: 'press', + positionals: ['@e7'], + flags: {}, + result: { selectorChain: ['id="save-v2"'] }, + targetEvidence: freshEvidence('save-v2', 'Save V2'), + }); + + // Leg 2: `replay --from N --save-script` WITHOUT --force — the target still + // exists. The persisted saveScriptForce must make the arm-time preflight use + // the SAME effective decision publication uses, so this is NOT rejected. + const leg2 = await runReplayScriptFile({ + req: baseReq({ + positionals: [filePath], + flags: { saveScript: true, replayFrom: 3, replayPlanDigest: divergence.resume.planDigest }, + }), + sessionName, + logPath, + sessionStore, + invoke, + }); + // Not rejected by the arm-time preflight (no "already exists"): the + // transaction reached completion instead. + expect(leg2.ok).toBe(true); + expect(sessionStore.get(sessionName)?.saveScriptComplete).toBe(true); + // A bare-boolean continuation never retargets, so force stays persisted. + expect(sessionStore.get(sessionName)?.saveScriptForce).toBe(true); +}); + +test('#1258 force is per-target: re-arming --save-script= WITHOUT --force drops force persisted for , so is NOT overwritten', async () => { + const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup( + 'agent-device-repair-transaction-retarget-clears-force-', + ); + // Armed and forced for target (flow.healed.ad). + const session = makeCompleteRepairSession(sessionStore, sessionName, root); + session.saveScriptForce = true; + // A DIFFERENT, unrelated file already sits at the retarget destination + // (flow.promoted.ad) — nobody opted to overwrite THIS one. + const promotedPath = path.join(root, 'flow.promoted.ad'); + fs.writeFileSync( + promotedPath, + `context platform=ios device="x"\nclick id="unrelated"\n${HEAL_COMPLETE_SENTINEL}\n`, + ); + const before = fs.readFileSync(promotedPath, 'utf8'); + + // `close --save-script=` (NO --force): retargeting from to + // without a live opt-in drops the force that was granted for . + const closeResponse = await handleCloseCommand({ + req: { + token: 't', + session: sessionName, + command: 'close', + positionals: [], + flags: { saveScript: promotedPath }, + }, + sessionName, + logPath, + sessionStore, + leaseRegistry, + }); + + // Refused — the retarget cleared the sticky force, so is protected by the + // default no-clobber exactly like a fresh target would be. + expect(closeResponse.ok).toBe(false); + if (!closeResponse.ok) expect(closeResponse.error.message).toMatch(/already exists/); + // is untouched, and the session is kept for retry. + expect(fs.readFileSync(promotedPath, 'utf8')).toBe(before); + expect(sessionStore.get(sessionName)).toBeDefined(); + expect(sessionStore.get(sessionName)?.saveScriptForce).toBeUndefined(); +}); + +test('#1258 force per-target, contrast: re-arming --save-script= WITH --force DOES overwrite ', async () => { + const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup( + 'agent-device-repair-transaction-retarget-force-overwrites-', + ); + const session = makeCompleteRepairSession(sessionStore, sessionName, root); + session.saveScriptForce = true; + const promotedPath = path.join(root, 'flow.promoted.ad'); + fs.writeFileSync( + promotedPath, + `context platform=ios device="x"\nclick id="unrelated"\n${HEAL_COMPLETE_SENTINEL}\n`, + ); + + // `close --save-script= --force`: the live opt-in re-grants force for the + // new target, overwriting it. + const closeResponse = await handleCloseCommand({ + req: { + token: 't', + session: sessionName, + command: 'close', + positionals: [], + flags: { saveScript: promotedPath, force: true }, + }, + sessionName, + logPath, + sessionStore, + leaseRegistry, + }); + + expect(closeResponse.ok).toBe(true); + const parsed = parseReplayScriptDetailed(fs.readFileSync(promotedPath, 'utf8')); + expect(parsed.actions.some((a) => a.positionals[0] === 'id="unrelated"')).toBe(false); +}); + test('BLOCKER 2 (new): a repair close whose PLATFORM close fails never commits a healed .ad claiming a successful close', async () => { const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup( 'agent-device-repair-transaction-platform-close-fail-', diff --git a/src/daemon/handlers/session-replay-runtime.ts b/src/daemon/handlers/session-replay-runtime.ts index b83b86a84..97154d735 100644 --- a/src/daemon/handlers/session-replay-runtime.ts +++ b/src/daemon/handlers/session-replay-runtime.ts @@ -10,6 +10,7 @@ import { } from '../../request/progress.ts'; import { SessionStore } from '../session-store.ts'; import { expandSessionPath } from '../session-paths.ts'; +import { applySaveScriptRetarget } from '../session-action-recorder.ts'; import { type ReplayScriptMetadata } from '../../replay/script.ts'; import { computeReplayPlanDigest } from '../../replay/plan-digest.ts'; import { errorResponse, noActiveSessionError } from './response.ts'; @@ -255,10 +256,14 @@ export async function runReplayScriptFile(params: { // fresh repair, its very first `open`) execute only to hit the SAME // refuse-on-exist at publish time (`publishHealedScriptAtomically`, at // `close`/completion). Computes the SAME target `armReplaySaveScriptStep` - // below would resolve, without needing the session to exist yet. + // below would resolve, without needing the session to exist yet, and + // honors the SAME effective force decision publication uses — the live + // `--force`/`--overwrite` OR the one persisted at arm time + // (`saveScriptForce`), so a `--from` continuation that does not repeat the + // flag is not rejected on a target a prior `--force` leg already authorized. const saveScriptPreflight = preflightSaveScriptTarget({ saveScript: req.flags?.saveScript, - force: req.flags?.force, + force: Boolean(req.flags?.force || preRunSession?.saveScriptForce), sourcePath: resolved, existingSaveScriptPath: preRunSession?.saveScriptPath, }); @@ -620,10 +625,6 @@ function armReplaySaveScriptStep(params: { const session = sessionStore.get(sessionName); if (!session) return; session.recordSession = true; - // #1258: sticky, like `saveScriptPath` — persisted so a LATER `--from` - // continuation leg or an unattended auto-commit teardown (no live request) - // still honors a `--force`/`--overwrite` that was only passed at this arm. - if (force) session.saveScriptForce = true; if (typeof saveScript === 'string') { // An EXPLICIT `--save-script=` clears the defaulted marker // (invariant: the marker is set iff the current `saveScriptPath` was @@ -632,12 +633,18 @@ function armReplaySaveScriptStep(params: { // (`publishHealedScriptAtomically`) and refuses ANY pre-existing target, // an explicit caller-directed path included, exactly like the default // healed sibling. - session.saveScriptPath = expandSessionPath(saveScript); + applySaveScriptRetarget(session, expandSessionPath(saveScript), force); session.saveScriptDefaultedHealedPath = false; } else if (session.saveScriptPath === undefined) { session.saveScriptPath = healedScriptSiblingPath(sourcePath); session.saveScriptDefaultedHealedPath = true; } + // #1258: force is per-target — a LIVE `--force`/`--overwrite` persists onto + // the session (`saveScriptForce`) so a LATER `--from` continuation leg or an + // unattended auto-commit teardown (no live request) still honors it. Set + // AFTER `applySaveScriptRetarget` so a live flag always wins over a + // retarget-clear. + if (force) session.saveScriptForce = true; if (session.saveScriptBoundary === undefined) { session.saveScriptBoundary = firstArm ? session.actions.length : 0; } diff --git a/src/daemon/session-action-recorder.ts b/src/daemon/session-action-recorder.ts index 784d2e177..a80b6ef64 100644 --- a/src/daemon/session-action-recorder.ts +++ b/src/daemon/session-action-recorder.ts @@ -14,6 +14,28 @@ export type RecordActionEntry = { targetEvidence?: TargetAnnotationV1; }; +/** + * #1258: point the session at `nextPath` and, when this is a RETARGET to a + * different path than the one currently persisted AND no live + * `--force`/`--overwrite` accompanies it, drop any `saveScriptForce` persisted + * for the OLD target. Force is authorization for the target it was opted into + * (`--save-script=a.ad --force`), NOT a session-wide standing grant that + * silently follows a later `--save-script=b.ad` and overwrites a file nobody + * opted to overwrite. A live `force` on this same retarget re-grants it for the + * new target (handled by the caller, AFTER this — so a live flag always wins). + * Shared by both re-arming paths: `armReplaySaveScriptStep` (replay) and + * `recordActionEntry` (close --save-script). + */ +export function applySaveScriptRetarget( + session: SessionState, + nextPath: string, + liveForce: boolean | undefined, +): void { + const retargeted = session.saveScriptPath !== undefined && session.saveScriptPath !== nextPath; + if (retargeted && !liveForce) session.saveScriptForce = undefined; + session.saveScriptPath = nextPath; +} + export function recordActionEntry( session: SessionState, entry: RecordActionEntry, @@ -29,15 +51,20 @@ export function recordActionEntry( // guard is uniform (see `publishHealedScriptAtomically`) and refuses // ANY pre-existing target — an explicit, caller-DIRECTED path included. // Directing the path is not the same as authorizing an overwrite. - session.saveScriptPath = expandSessionPath(entry.flags.saveScript); + applySaveScriptRetarget( + session, + expandSessionPath(entry.flags.saveScript), + entry.flags.force, + ); session.saveScriptDefaultedHealedPath = false; } // #1258: persist `--force`/`--overwrite`, like `saveScriptPath`, so a // LATER write that does not repeat the flag (a bare `close` finishing a // session opened with `open --save-script --force`, or an unattended - // auto-commit teardown with no live request) still honors it. Sticky — - // once true, a later action carrying `saveScript` without `force` must - // not clear it back to false. + // auto-commit teardown with no live request) still honors it. Sticky FOR + // THE SAME TARGET — a retarget to a different path without a live `force` + // drops it (see `applySaveScriptRetarget`); a same-path later action + // carrying `saveScript` without `force` still must not clear it. if (entry.flags.force) { session.saveScriptForce = true; } diff --git a/src/daemon/types.ts b/src/daemon/types.ts index 289d9351a..2b7095a4e 100644 --- a/src/daemon/types.ts +++ b/src/daemon/types.ts @@ -329,7 +329,13 @@ export type SessionState = { * `--from` continuation leg, or an unattended auto-commit teardown with no * live request at all) still honors the overwrite the caller opted into up * front. The effective decision at any write site is `req.flags?.force || - * session.saveScriptForce`; once set true it is never cleared. + * session.saveScriptForce`. + * + * Force is PER-TARGET, not a session-wide standing grant: it stays set while + * the target is unchanged, but re-arming a DIFFERENT `--save-script=` + * without a live `--force` CLEARS it (`applySaveScriptRetarget`), so a later + * retarget can never silently overwrite a file the caller never opted into. + * A live `--force` on the retarget re-grants it for the new target. */ saveScriptForce?: boolean; /** From 20e054259f9b74270e34c045e62c1bd76b29d698 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 14 Jul 2026 21:08:17 +0200 Subject: [PATCH 4/5] fix: match arm-time preflight force to per-target retarget contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review ordering-mismatch fix. The preflight computed effective force as `req.flags?.force || preRunSession.saveScriptForce`, accepting persisted force regardless of whether THIS request retargets. So a `--from` continuation with explicit `--save-script=b.ad` (no live force) on a session forced for a.ad would PASS the preflight, run every step, then have `applySaveScriptRetarget` clear the force for b.ad, and finally refuse the existing b.ad at publish time — defeating the arm-time-preflight point and mutating the session mid-run. Now the effective-force decision is computed inside `preflightSaveScriptTarget` against the target THIS request resolves to (the same one the armer will set), matching `applySaveScriptRetarget`'s per-target contract: live force always bypasses; persisted force bypasses ONLY when `targetPath === existingSaveScriptPath` (same target). A retarget to a different path without live force is now refused BEFORE any step dispatches. The preflight stays read-only (runs before the armer; never mutates the session). Regression: a --from continuation retargeting to an existing b.ad without live force fails at the preflight, dispatches zero steps, and leaves saveScriptPath/ saveScriptForce unchanged. Verified it fails (leg passes preflight, runs) when the fix is reverted. Same-target continuation and live-force retarget stay correct. --- .../session-replay-repair-transaction.test.ts | 80 +++++++++++++++++++ src/daemon/handlers/session-replay-runtime.ts | 38 ++++++--- 2 files changed, 107 insertions(+), 11 deletions(-) diff --git a/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts b/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts index edaa9e14d..c9af0e656 100644 --- a/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts @@ -682,6 +682,86 @@ test('#1258 preflight honors PERSISTED force: a --from continuation without --fo expect(sessionStore.get(sessionName)?.saveScriptForce).toBe(true); }); +test('#1258 preflight is per-target: a --from continuation RETARGETING to an existing WITHOUT live force is refused BEFORE dispatch and leaves the session target unmutated', async () => { + const { root, sessionStore, sessionName, logPath } = setup( + 'agent-device-repair-transaction-preflight-retarget-', + ); + const filePath = writeReplayFile(root, [ + 'open "Demo" --relaunch', + SAVE_ANNOTATION, + 'click id="save"', + 'click id="confirm"', + 'close', + ]); + const targetA = path.join(root, 'a.ad'); + const targetB = path.join(root, 'b.ad'); + // already exists (nobody opted to overwrite it); does not, so leg 1 + // arms/forces cleanly. + fs.writeFileSync(targetB, 'context platform=ios device="x"\nclick id="unrelated"\n'); + const beforeB = fs.readFileSync(targetB, 'utf8'); + const spy: DaemonRequest[] = []; + const invoke = makeRecordingReplayInvoke({ + sessionStore, + sessionName, + spy, + evidence: (req) => (req.command === 'click' ? freshEvidence('confirm', 'Confirm') : undefined), + }); + + // Leg 1: `--save-script= --force` — arms + PERSISTS force for , then + // diverges (nothing published). + const leg1 = await runReplayScriptFile({ + req: baseReq({ positionals: [filePath], flags: { saveScript: targetA, force: true } }), + sessionName, + logPath, + sessionStore, + invoke, + }); + expect(leg1.ok).toBe(false); + if (leg1.ok) return; + const divergence = leg1.error.details?.divergence as { resume: { planDigest: string } }; + const session = sessionStore.get(sessionName)!; + expect(session.saveScriptPath).toBe(targetA); + expect(session.saveScriptForce).toBe(true); + // The agent's corrective press. + sessionStore.recordAction(session, { + command: 'press', + positionals: ['@e7'], + flags: {}, + result: { selectorChain: ['id="save-v2"'] }, + targetEvidence: freshEvidence('save-v2', 'Save V2'), + }); + const dispatchesBeforeLeg2 = spy.length; + + // Leg 2: `--from N --save-script=` (explicit RETARGET, NO live force) — + // exists. The persisted force was granted for , and + // `applySaveScriptRetarget` WOULD clear it for ; the preflight must MATCH + // that per-target contract and REFUSE here, before any step dispatches, + // instead of executing the leg and only refusing at publish time. + const leg2 = await runReplayScriptFile({ + req: baseReq({ + positionals: [filePath], + flags: { saveScript: targetB, replayFrom: 3, replayPlanDigest: divergence.resume.planDigest }, + }), + sessionName, + logPath, + sessionStore, + invoke, + }); + expect(leg2.ok).toBe(false); + if (!leg2.ok) { + expect(leg2.error.code).toBe('COMMAND_FAILED'); + expect(leg2.error.message).toMatch(/already exists/); + } + // Not a single step of leg 2 dispatched — refused BEFORE the loop. + expect(spy.length).toBe(dispatchesBeforeLeg2); + // READ-ONLY: the rejected request left the session target untouched — still + // armed/forced for , never retargeted to . + expect(sessionStore.get(sessionName)?.saveScriptPath).toBe(targetA); + expect(sessionStore.get(sessionName)?.saveScriptForce).toBe(true); + // is byte-for-byte untouched. + expect(fs.readFileSync(targetB, 'utf8')).toBe(beforeB); +}); + test('#1258 force is per-target: re-arming --save-script= WITHOUT --force drops force persisted for , so is NOT overwritten', async () => { const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup( 'agent-device-repair-transaction-retarget-clears-force-', diff --git a/src/daemon/handlers/session-replay-runtime.ts b/src/daemon/handlers/session-replay-runtime.ts index 97154d735..550ce02b6 100644 --- a/src/daemon/handlers/session-replay-runtime.ts +++ b/src/daemon/handlers/session-replay-runtime.ts @@ -257,13 +257,14 @@ export async function runReplayScriptFile(params: { // refuse-on-exist at publish time (`publishHealedScriptAtomically`, at // `close`/completion). Computes the SAME target `armReplaySaveScriptStep` // below would resolve, without needing the session to exist yet, and - // honors the SAME effective force decision publication uses — the live - // `--force`/`--overwrite` OR the one persisted at arm time - // (`saveScriptForce`), so a `--from` continuation that does not repeat the - // flag is not rejected on a target a prior `--force` leg already authorized. + // honors the SAME effective force decision publication uses — read-only: + // it never mutates the session (it runs BEFORE `applySaveScriptRetarget`), + // so `liveForce`/`persistedForce` are passed separately for it to apply the + // per-target rule against the target it computes. const saveScriptPreflight = preflightSaveScriptTarget({ saveScript: req.flags?.saveScript, - force: Boolean(req.flags?.force || preRunSession?.saveScriptForce), + liveForce: req.flags?.force, + persistedForce: preRunSession?.saveScriptForce, sourcePath: resolved, existingSaveScriptPath: preRunSession?.saveScriptPath, }); @@ -532,22 +533,37 @@ function preflightReplayAgainstActiveRepair(params: { * wins; otherwise an already-armed session's existing path if this is a * `--from` continuation leg reusing it, else the default `.healed.ad` * sibling) WITHOUT needing the session to exist yet, so it runs before step 1 - * dispatches even when that step is the `open` that creates the session. A - * no-op when `--save-script` was not passed this invocation, or when `force` - * was — the caller explicitly opted into overwriting. + * dispatches even when that step is the `open` that creates the session. + * READ-ONLY: it never mutates the session (it runs before + * `applySaveScriptRetarget`). + * + * The effective-force decision MATCHES `applySaveScriptRetarget`'s per-target + * contract, computed against the target THIS request resolves to: a live + * `--force`/`--overwrite` always bypasses; a PERSISTED `saveScriptForce` + * bypasses ONLY when this request writes to the SAME target it was granted for + * (`targetPath === existingSaveScriptPath`). An explicit RETARGET to a + * different path without a live force does NOT bypass here — because + * `applySaveScriptRetarget` will CLEAR that persisted force for the new target + * before publication anyway, so letting the run execute (mutating the session + * mid-flight) only to refuse the existing target at the end is exactly what + * this preflight exists to prevent. A no-op when `--save-script` was not passed. */ function preflightSaveScriptTarget(params: { saveScript: boolean | string | undefined; - force: boolean | undefined; + liveForce: boolean | undefined; + persistedForce: boolean | undefined; sourcePath: string; existingSaveScriptPath: string | undefined; }): DaemonResponse | undefined { - const { saveScript, force, sourcePath, existingSaveScriptPath } = params; - if (!saveScript || force) return undefined; + const { saveScript, liveForce, persistedForce, sourcePath, existingSaveScriptPath } = params; + if (!saveScript) return undefined; const targetPath = typeof saveScript === 'string' ? expandSessionPath(saveScript) : (existingSaveScriptPath ?? healedScriptSiblingPath(sourcePath)); + const effectiveForce = + Boolean(liveForce) || (Boolean(persistedForce) && targetPath === existingSaveScriptPath); + if (effectiveForce) return undefined; if (!fs.existsSync(targetPath)) return undefined; return errorResponse( 'COMMAND_FAILED', From b4d0b1765735a2de4bee638a112759c4df3feceb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 15 Jul 2026 07:49:54 +0200 Subject: [PATCH 5/5] fix: expose close savedScript to clients; preserve COMPLETE on retarget reject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two re-review blockers. BLOCKER 1 (client-contract gap): the typed close APIs accept --save-script/ --force inputs but dropped the daemon's `savedScript` response, so a Node client could request publication but not learn where the file landed. Add `savedScript?: string` to SessionCloseResult and AppCloseResult, and project it via readOptionalString in both close normalizers (sessions.close, apps.close). New public-client coverage asserts it round-trips (and is absent when the daemon published nothing). BLOCKER 2 (P1 ordering): the C2 `saveScriptComplete = false` reset ran BEFORE the EEXIST preflight's early-return, so a retarget REJECTION corrupted a prior COMPLETE transaction — a later close would then refuse to commit the original target. Move the reset to AFTER `if (saveScriptPreflight) return ...` (verified nothing between the two positions reads saveScriptComplete; applySaveScriptRetarget already runs later, so the original saveScriptPath is preserved). The reset is correct only when the run proceeds to re-arm. Strengthened the retarget-rejection regression to assert (a) saveScriptComplete survives the rejection and (b) a later bare close commits the ORIGINAL target, not the rejected new one. Both verified to fail without their respective fix. --- src/__tests__/client.test.ts | 28 +++++++++++++++++ src/agent-device-client.ts | 2 ++ src/client/client-types.ts | 12 +++++++ .../session-replay-repair-transaction.test.ts | 31 +++++++++++++++++-- src/daemon/handlers/session-replay-runtime.ts | 14 ++++++--- 5 files changed, 81 insertions(+), 6 deletions(-) diff --git a/src/__tests__/client.test.ts b/src/__tests__/client.test.ts index 2d7eb46c0..7141f2e45 100644 --- a/src/__tests__/client.test.ts +++ b/src/__tests__/client.test.ts @@ -319,6 +319,34 @@ test('client close normalizes target shutdown results', async () => { assert.equal(appClose.shutdown, undefined); }); +test('client close surfaces the daemon savedScript path on both sessions.close and apps.close (#1258)', async () => { + const savedScriptPath = '/tmp/agent-device/flows/login.healed.ad'; + const setup = createTransport(async () => ({ + ok: true, + data: { session: 'qa', savedScript: savedScriptPath }, + })); + const client = createAgentDeviceClient(setup.config, { transport: setup.transport }); + + const sessionClose = await client.sessions.close({ saveScript: true }); + const appClose = await client.apps.close({ saveScript: true }); + + // The committed artifact path round-trips so a Node client that requested + // publication learns where the file landed. + assert.equal(sessionClose.savedScript, savedScriptPath); + assert.equal(appClose.savedScript, savedScriptPath); +}); + +test('client close omits savedScript when the daemon published nothing (#1258)', async () => { + const setup = createTransport(async () => ({ ok: true, data: { session: 'qa' } })); + const client = createAgentDeviceClient(setup.config, { transport: setup.transport }); + + const sessionClose = await client.sessions.close({}); + const appClose = await client.apps.close({}); + + assert.equal(sessionClose.savedScript, undefined); + assert.equal(appClose.savedScript, undefined); +}); + test('observability.perf projects structured frame area to daemon positionals', async () => { const setup = createTransport(async (req) => { if (req.command === 'perf') { diff --git a/src/agent-device-client.ts b/src/agent-device-client.ts index 6c7f7af09..f003d55a7 100644 --- a/src/agent-device-client.ts +++ b/src/agent-device-client.ts @@ -187,6 +187,7 @@ export function createAgentDeviceClient( session, shutdown: normalizeTargetShutdownResult(data.shutdown), provider: readObject(data.provider), + savedScript: readOptionalString(data, 'savedScript'), identifiers: { session }, }; } finally { @@ -259,6 +260,7 @@ export function createAgentDeviceClient( session, closedApp: options.app, shutdown: normalizeTargetShutdownResult(data.shutdown), + savedScript: readOptionalString(data, 'savedScript'), identifiers: { session }, }; }, diff --git a/src/client/client-types.ts b/src/client/client-types.ts index a7e87dcb6..432aba894 100644 --- a/src/client/client-types.ts +++ b/src/client/client-types.ts @@ -252,6 +252,12 @@ export type SessionCloseResult = { session: string; shutdown?: TargetShutdownResult; provider?: CloudProviderSessionResult; + /** + * #1258: absolute path of the committed session/healed script when this close + * published one (`close --save-script`, or a repair-armed session's finalize) + * — so a client that requested publication learns where the file landed. + */ + savedScript?: string; identifiers: AgentDeviceIdentifiers; }; @@ -327,6 +333,12 @@ export type AppCloseResult = { session: string; closedApp?: string; shutdown?: TargetShutdownResult; + /** + * #1258: absolute path of the committed session/healed script when this close + * published one (`close --save-script`, or a repair-armed session's finalize) + * — so a client that requested publication learns where the file landed. + */ + savedScript?: string; identifiers: AgentDeviceIdentifiers; }; diff --git a/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts b/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts index c9af0e656..7b93b3801 100644 --- a/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts @@ -682,8 +682,8 @@ test('#1258 preflight honors PERSISTED force: a --from continuation without --fo expect(sessionStore.get(sessionName)?.saveScriptForce).toBe(true); }); -test('#1258 preflight is per-target: a --from continuation RETARGETING to an existing WITHOUT live force is refused BEFORE dispatch and leaves the session target unmutated', async () => { - const { root, sessionStore, sessionName, logPath } = setup( +test('#1258 preflight is per-target: a --from continuation RETARGETING to an existing WITHOUT live force is refused BEFORE dispatch, preserves the prior COMPLETE transaction, and a later close still commits the ORIGINAL ', async () => { + const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup( 'agent-device-repair-transaction-preflight-retarget-', ); const filePath = writeReplayFile(root, [ @@ -730,6 +730,12 @@ test('#1258 preflight is per-target: a --from continuation RETARGETING to an exi result: { selectorChain: ['id="save-v2"'] }, targetEvidence: freshEvidence('save-v2', 'Save V2'), }); + // The transaction for has now reached COMPLETE (a resume leg ran to the + // end) — set directly here, mirroring `makeCompleteRepairSession`'s + // convention, to isolate THIS test's concern: a later retarget REJECTION must + // not corrupt this flag (BLOCKER 2 — the C2 `saveScriptComplete = false` + // reset must run AFTER the preflight's early-return, never before it). + session.saveScriptComplete = true; const dispatchesBeforeLeg2 = spy.length; // Leg 2: `--from N --save-script=` (explicit RETARGET, NO live force) — @@ -758,8 +764,29 @@ test('#1258 preflight is per-target: a --from continuation RETARGETING to an exi // armed/forced for , never retargeted to . expect(sessionStore.get(sessionName)?.saveScriptPath).toBe(targetA); expect(sessionStore.get(sessionName)?.saveScriptForce).toBe(true); + // BLOCKER 2 (a): the prior COMPLETE transaction SURVIVES the rejection — the + // C2 completion reset never ran, because the preflight returned first. + expect(sessionStore.get(sessionName)?.saveScriptComplete).toBe(true); // is byte-for-byte untouched. expect(fs.readFileSync(targetB, 'utf8')).toBe(beforeB); + + // BLOCKER 2 (b): a later bare `close` still COMMITS the ORIGINAL target + // (never the rejected ) — proof the rejected retarget corrupted neither + // the completion flag nor the target path. + const closeResponse = await handleCloseCommand({ + req: { token: 't', session: sessionName, command: 'close', positionals: [], flags: {} }, + sessionName, + logPath, + sessionStore, + leaseRegistry, + }); + expect(closeResponse.ok).toBe(true); + if (closeResponse.ok) expect(closeResponse.data?.savedScript).toBe(targetA); + expect(fs.existsSync(targetA)).toBe(true); + const committed = parseReplayScriptDetailed(fs.readFileSync(targetA, 'utf8')); + expect(committed.actions.map((a) => a.command)).toEqual(['open', 'press', 'close']); + // stayed untouched through the commit too. + expect(fs.readFileSync(targetB, 'utf8')).toBe(beforeB); }); test('#1258 force is per-target: re-arming --save-script= WITHOUT --force drops force persisted for , so is NOT overwritten', async () => { diff --git a/src/daemon/handlers/session-replay-runtime.ts b/src/daemon/handlers/session-replay-runtime.ts index 550ce02b6..13215e4b6 100644 --- a/src/daemon/handlers/session-replay-runtime.ts +++ b/src/daemon/handlers/session-replay-runtime.ts @@ -246,11 +246,7 @@ export async function runReplayScriptFile(params: { // key clears any prior reap tombstone before starting a new transaction. sessionStore.clearRepairTombstone(sessionName); } - // ADR 0012 decision 6 (C2): a repair-armed resume re-opens the completion - // window — a leg that re-diverges must not inherit a prior leg's COMPLETE - // flag and let a later `close` commit a now-stale transaction. const preRunSession = sessionStore.get(sessionName); - if (preRunSession?.saveScriptBoundary !== undefined) preRunSession.saveScriptComplete = false; // #1258: arm-time EEXIST preflight — fail HERE, before any step of this // invocation dispatches, rather than letting the whole run (and, for a // fresh repair, its very first `open`) execute only to hit the SAME @@ -269,6 +265,16 @@ export async function runReplayScriptFile(params: { existingSaveScriptPath: preRunSession?.saveScriptPath, }); if (saveScriptPreflight) return saveScriptPreflight; + // ADR 0012 decision 6 (C2): a repair-armed resume re-opens the completion + // window — a leg that re-diverges must not inherit a prior leg's COMPLETE + // flag and let a later `close` commit a now-stale transaction. #1258: this + // reset MUST run AFTER the preflight's early-return above — a preflight + // REJECTION (a retarget to an existing target without force) leaves the + // session untouched, so a prior COMPLETE transaction stays committable at + // its ORIGINAL target; resetting before the return would corrupt it (a + // later `close` would then refuse to commit the original). The reset is + // correct only when this run actually proceeds to re-arm. + if (preRunSession?.saveScriptBoundary !== undefined) preRunSession.saveScriptComplete = false; const armReplaySaveScriptStep = createReplaySaveScriptArmer({ saveScript: req.flags?.saveScript, force: req.flags?.force,