feat: --force/--overwrite for --save-script + arm-time EEXIST preflight (#1266)

* 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.

* refactor: extract write() catch-block into handleSessionScriptWriteFailure

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.

* fix: make persisted save-script force consistent (preflight + per-target)

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=<other>` 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.

* fix: match arm-time preflight force to per-target retarget contract

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.

* fix: expose close savedScript to clients; preserve COMPLETE on retarget reject

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.
This commit is contained in:
Michał Pierzchała
2026-07-15 10:16:42 +02:00
committed by GitHub
parent 6efe54451b
commit 95c1e201cb
14 changed files with 747 additions and 65 deletions
+28
View File
@@ -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') {
+2
View File
@@ -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 },
};
},
+27 -1
View File
@@ -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;
};
@@ -292,6 +298,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,12 +324,21 @@ 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 = {
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;
};
@@ -866,6 +883,8 @@ export type ReplayRunOptions = AgentDeviceRequestOverrides &
* `<stem>.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 +1091,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 +1131,12 @@ export type AgentDeviceClient = {
options?: AgentDeviceRequestOverrides & Pick<AgentDeviceClientConfig, 'stateDir'>,
) => Promise<string>;
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<SessionCloseResult>;
artifacts: (options?: CloudArtifactsOptions) => Promise<AgentArtifactsResult>;
};
@@ -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',
+10 -1
View File
@@ -40,6 +40,9 @@ const openCommandMetadata = defineFieldCommandMetadata(
),
relaunch: booleanField('Force relaunch.'),
saveScript: jsonSchemaField<boolean | string>({ 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<boolean | string>({ 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);
+5
View File
@@ -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<boolean | string>({ 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) => ({
@@ -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
@@ -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,357 @@ 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('#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 preflight is per-target: a --from continuation RETARGETING to an existing <b> WITHOUT live force is refused BEFORE dispatch, preserves the prior COMPLETE transaction, and a later close still commits the ORIGINAL <a>', async () => {
const { root, sessionStore, sessionName, logPath, leaseRegistry } = 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');
// <b> already exists (nobody opted to overwrite it); <a> does not, so leg 1
// arms/forces <a> 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=<a> --force` — arms + PERSISTS force for <a>, 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'),
});
// The transaction for <a> 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=<b>` (explicit RETARGET, NO live force) — <b>
// exists. The persisted force was granted for <a>, and
// `applySaveScriptRetarget` WOULD clear it for <b>; 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 <a>, never retargeted to <b>.
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);
// <b> is byte-for-byte untouched.
expect(fs.readFileSync(targetB, 'utf8')).toBe(beforeB);
// BLOCKER 2 (b): a later bare `close` still COMMITS the ORIGINAL target <a>
// (never the rejected <b>) — 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']);
// <b> stayed untouched through the commit too.
expect(fs.readFileSync(targetB, 'utf8')).toBe(beforeB);
});
test('#1258 force is per-target: re-arming --save-script=<b> WITHOUT --force drops force persisted for <a>, so <b> is NOT overwritten', async () => {
const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup(
'agent-device-repair-transaction-retarget-clears-force-',
);
// Armed and forced for target <a> (flow.healed.ad).
const session = makeCompleteRepairSession(sessionStore, sessionName, root);
session.saveScriptForce = true;
// A DIFFERENT, unrelated file already sits at the retarget destination <b>
// (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=<b>` (NO --force): retargeting from <a> to <b>
// without a live opt-in drops the force that was granted for <a>.
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 <b> 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/);
// <b> 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=<b> WITH --force DOES overwrite <b>', 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=<b> --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-',
+15 -2
View File
@@ -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),
+88 -6
View File
@@ -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';
@@ -245,13 +246,38 @@ export async function runReplayScriptFile(params: {
// key clears any prior reap tombstone before starting a new transaction.
sessionStore.clearRepairTombstone(sessionName);
}
const preRunSession = sessionStore.get(sessionName);
// #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, and
// 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,
liveForce: req.flags?.force,
persistedForce: preRunSession?.saveScriptForce,
sourcePath: resolved,
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.
const preRunSession = sessionStore.get(sessionName);
// 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,
sessionStore,
sessionName,
sourcePath: resolved,
@@ -503,6 +529,54 @@ 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=<path>` always
* wins; otherwise an already-armed session's existing path if this is a
* `--from` continuation leg reusing it, else the default `<stem>.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.
* 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;
liveForce: boolean | undefined;
persistedForce: boolean | undefined;
sourcePath: string;
existingSaveScriptPath: string | undefined;
}): DaemonResponse | 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',
`A file already exists at ${targetPath}; remove it, pass replay --save-script=<other-path>, 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 +610,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,10 +639,11 @@ 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;
@@ -579,12 +655,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;
}
+38 -1
View File
@@ -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,9 +51,23 @@ 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 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;
}
}
const action: SessionAction = {
ts: Date.now(),
@@ -72,6 +108,7 @@ const SANITIZED_FLAG_KEYS = [
...SCREENSHOT_ACTION_FLAG_KEYS,
'relaunch',
'saveScript',
'force',
'noRecord',
'fps',
'quality',
+83 -48
View File
@@ -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,39 +85,16 @@ 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 };
} 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 });
}
}
@@ -132,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
@@ -159,29 +177,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=<path>` alike.
* `--save-script=<path>` 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 +218,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=<other-path> so an existing healed script is never overwritten.`,
`A file already exists at ${scriptPath}; remove it, pass replay --save-script=<other-path>, 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 });
}
}
+6 -3
View File
@@ -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, {
+18
View File
@@ -320,6 +320,24 @@ 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`.
*
* 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=<other>`
* 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;
/**
* ADR 0012 decision 6, R6: `session.actions.length` at the `replay
* --save-script` invocation that armed this session — the repair-run