Skip to content

Commit 579bae3

Browse files
thymikeethymikee
committed
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.
1 parent ec0e7f3 commit 579bae3

4 files changed

Lines changed: 194 additions & 12 deletions

File tree

‎src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -615,6 +615,148 @@ test('#1258: --force skips the arm-time preflight and the replay proceeds despit
615615
expect(sessionStore.get(sessionName)?.saveScriptForce).toBe(true);
616616
});
617617

618+
test('#1258 preflight honors PERSISTED force: a --from continuation without --force is NOT rejected on an existing target a prior --force leg authorized', async () => {
619+
const { root, sessionStore, sessionName, logPath } = setup(
620+
'agent-device-repair-transaction-preflight-persisted-force-',
621+
);
622+
const filePath = writeReplayFile(root, [
623+
'open "Demo" --relaunch',
624+
SAVE_ANNOTATION,
625+
'click id="save"',
626+
'click id="confirm"',
627+
'close',
628+
]);
629+
// The healed target already exists — the preflight WOULD reject a
630+
// continuation that only saw the (absent, this leg) LIVE force flag.
631+
fs.writeFileSync(path.join(root, 'flow.healed.ad'), 'context platform=ios device="x"\n');
632+
const invoke = makeRecordingReplayInvoke({
633+
sessionStore,
634+
sessionName,
635+
evidence: (req) => (req.command === 'click' ? freshEvidence('confirm', 'Confirm') : undefined),
636+
});
637+
638+
// Leg 1: `replay --save-script --force` — the LIVE force passes the preflight
639+
// (target exists), arms the session, PERSISTS saveScriptForce, then diverges
640+
// (nothing published, so the pre-existing target survives).
641+
const leg1 = await runReplayScriptFile({
642+
req: baseReq({ positionals: [filePath], flags: { saveScript: true, force: true } }),
643+
sessionName,
644+
logPath,
645+
sessionStore,
646+
invoke,
647+
});
648+
expect(leg1.ok).toBe(false);
649+
if (leg1.ok) return;
650+
expect(leg1.error.code).toBe('REPLAY_DIVERGENCE');
651+
const divergence = leg1.error.details?.divergence as { resume: { planDigest: string } };
652+
expect(sessionStore.get(sessionName)?.saveScriptForce).toBe(true);
653+
654+
// The agent's corrective press (blessed @ref), recorded live.
655+
const session = sessionStore.get(sessionName)!;
656+
sessionStore.recordAction(session, {
657+
command: 'press',
658+
positionals: ['@e7'],
659+
flags: {},
660+
result: { selectorChain: ['id="save-v2"'] },
661+
targetEvidence: freshEvidence('save-v2', 'Save V2'),
662+
});
663+
664+
// Leg 2: `replay --from N --save-script` WITHOUT --force — the target still
665+
// exists. The persisted saveScriptForce must make the arm-time preflight use
666+
// the SAME effective decision publication uses, so this is NOT rejected.
667+
const leg2 = await runReplayScriptFile({
668+
req: baseReq({
669+
positionals: [filePath],
670+
flags: { saveScript: true, replayFrom: 3, replayPlanDigest: divergence.resume.planDigest },
671+
}),
672+
sessionName,
673+
logPath,
674+
sessionStore,
675+
invoke,
676+
});
677+
// Not rejected by the arm-time preflight (no "already exists"): the
678+
// transaction reached completion instead.
679+
expect(leg2.ok).toBe(true);
680+
expect(sessionStore.get(sessionName)?.saveScriptComplete).toBe(true);
681+
// A bare-boolean continuation never retargets, so force stays persisted.
682+
expect(sessionStore.get(sessionName)?.saveScriptForce).toBe(true);
683+
});
684+
685+
test('#1258 force is per-target: re-arming --save-script=<b> WITHOUT --force drops force persisted for <a>, so <b> is NOT overwritten', async () => {
686+
const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup(
687+
'agent-device-repair-transaction-retarget-clears-force-',
688+
);
689+
// Armed and forced for target <a> (flow.healed.ad).
690+
const session = makeCompleteRepairSession(sessionStore, sessionName, root);
691+
session.saveScriptForce = true;
692+
// A DIFFERENT, unrelated file already sits at the retarget destination <b>
693+
// (flow.promoted.ad) — nobody opted to overwrite THIS one.
694+
const promotedPath = path.join(root, 'flow.promoted.ad');
695+
fs.writeFileSync(
696+
promotedPath,
697+
`context platform=ios device="x"\nclick id="unrelated"\n${HEAL_COMPLETE_SENTINEL}\n`,
698+
);
699+
const before = fs.readFileSync(promotedPath, 'utf8');
700+
701+
// `close --save-script=<b>` (NO --force): retargeting from <a> to <b>
702+
// without a live opt-in drops the force that was granted for <a>.
703+
const closeResponse = await handleCloseCommand({
704+
req: {
705+
token: 't',
706+
session: sessionName,
707+
command: 'close',
708+
positionals: [],
709+
flags: { saveScript: promotedPath },
710+
},
711+
sessionName,
712+
logPath,
713+
sessionStore,
714+
leaseRegistry,
715+
});
716+
717+
// Refused — the retarget cleared the sticky force, so <b> is protected by the
718+
// default no-clobber exactly like a fresh target would be.
719+
expect(closeResponse.ok).toBe(false);
720+
if (!closeResponse.ok) expect(closeResponse.error.message).toMatch(/already exists/);
721+
// <b> is untouched, and the session is kept for retry.
722+
expect(fs.readFileSync(promotedPath, 'utf8')).toBe(before);
723+
expect(sessionStore.get(sessionName)).toBeDefined();
724+
expect(sessionStore.get(sessionName)?.saveScriptForce).toBeUndefined();
725+
});
726+
727+
test('#1258 force per-target, contrast: re-arming --save-script=<b> WITH --force DOES overwrite <b>', async () => {
728+
const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup(
729+
'agent-device-repair-transaction-retarget-force-overwrites-',
730+
);
731+
const session = makeCompleteRepairSession(sessionStore, sessionName, root);
732+
session.saveScriptForce = true;
733+
const promotedPath = path.join(root, 'flow.promoted.ad');
734+
fs.writeFileSync(
735+
promotedPath,
736+
`context platform=ios device="x"\nclick id="unrelated"\n${HEAL_COMPLETE_SENTINEL}\n`,
737+
);
738+
739+
// `close --save-script=<b> --force`: the live opt-in re-grants force for the
740+
// new target, overwriting it.
741+
const closeResponse = await handleCloseCommand({
742+
req: {
743+
token: 't',
744+
session: sessionName,
745+
command: 'close',
746+
positionals: [],
747+
flags: { saveScript: promotedPath, force: true },
748+
},
749+
sessionName,
750+
logPath,
751+
sessionStore,
752+
leaseRegistry,
753+
});
754+
755+
expect(closeResponse.ok).toBe(true);
756+
const parsed = parseReplayScriptDetailed(fs.readFileSync(promotedPath, 'utf8'));
757+
expect(parsed.actions.some((a) => a.positionals[0] === 'id="unrelated"')).toBe(false);
758+
});
759+
618760
test('BLOCKER 2 (new): a repair close whose PLATFORM close fails never commits a healed .ad claiming a successful close', async () => {
619761
const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup(
620762
'agent-device-repair-transaction-platform-close-fail-',

‎src/daemon/handlers/session-replay-runtime.ts‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
} from '../../request/progress.ts';
1111
import { SessionStore } from '../session-store.ts';
1212
import { expandSessionPath } from '../session-paths.ts';
13+
import { applySaveScriptRetarget } from '../session-action-recorder.ts';
1314
import { type ReplayScriptMetadata } from '../../replay/script.ts';
1415
import { computeReplayPlanDigest } from '../../replay/plan-digest.ts';
1516
import { errorResponse, noActiveSessionError } from './response.ts';
@@ -255,10 +256,14 @@ export async function runReplayScriptFile(params: {
255256
// fresh repair, its very first `open`) execute only to hit the SAME
256257
// refuse-on-exist at publish time (`publishHealedScriptAtomically`, at
257258
// `close`/completion). Computes the SAME target `armReplaySaveScriptStep`
258-
// below would resolve, without needing the session to exist yet.
259+
// below would resolve, without needing the session to exist yet, and
260+
// honors the SAME effective force decision publication uses — the live
261+
// `--force`/`--overwrite` OR the one persisted at arm time
262+
// (`saveScriptForce`), so a `--from` continuation that does not repeat the
263+
// flag is not rejected on a target a prior `--force` leg already authorized.
259264
const saveScriptPreflight = preflightSaveScriptTarget({
260265
saveScript: req.flags?.saveScript,
261-
force: req.flags?.force,
266+
force: Boolean(req.flags?.force || preRunSession?.saveScriptForce),
262267
sourcePath: resolved,
263268
existingSaveScriptPath: preRunSession?.saveScriptPath,
264269
});
@@ -620,10 +625,6 @@ function armReplaySaveScriptStep(params: {
620625
const session = sessionStore.get(sessionName);
621626
if (!session) return;
622627
session.recordSession = true;
623-
// #1258: sticky, like `saveScriptPath` — persisted so a LATER `--from`
624-
// continuation leg or an unattended auto-commit teardown (no live request)
625-
// still honors a `--force`/`--overwrite` that was only passed at this arm.
626-
if (force) session.saveScriptForce = true;
627628
if (typeof saveScript === 'string') {
628629
// An EXPLICIT `--save-script=<path>` clears the defaulted marker
629630
// (invariant: the marker is set iff the current `saveScriptPath` was
@@ -632,12 +633,18 @@ function armReplaySaveScriptStep(params: {
632633
// (`publishHealedScriptAtomically`) and refuses ANY pre-existing target,
633634
// an explicit caller-directed path included, exactly like the default
634635
// healed sibling.
635-
session.saveScriptPath = expandSessionPath(saveScript);
636+
applySaveScriptRetarget(session, expandSessionPath(saveScript), force);
636637
session.saveScriptDefaultedHealedPath = false;
637638
} else if (session.saveScriptPath === undefined) {
638639
session.saveScriptPath = healedScriptSiblingPath(sourcePath);
639640
session.saveScriptDefaultedHealedPath = true;
640641
}
642+
// #1258: force is per-target — a LIVE `--force`/`--overwrite` persists onto
643+
// the session (`saveScriptForce`) so a LATER `--from` continuation leg or an
644+
// unattended auto-commit teardown (no live request) still honors it. Set
645+
// AFTER `applySaveScriptRetarget` so a live flag always wins over a
646+
// retarget-clear.
647+
if (force) session.saveScriptForce = true;
641648
if (session.saveScriptBoundary === undefined) {
642649
session.saveScriptBoundary = firstArm ? session.actions.length : 0;
643650
}

‎src/daemon/session-action-recorder.ts‎

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,28 @@ export type RecordActionEntry = {
1414
targetEvidence?: TargetAnnotationV1;
1515
};
1616

17+
/**
18+
* #1258: point the session at `nextPath` and, when this is a RETARGET to a
19+
* different path than the one currently persisted AND no live
20+
* `--force`/`--overwrite` accompanies it, drop any `saveScriptForce` persisted
21+
* for the OLD target. Force is authorization for the target it was opted into
22+
* (`--save-script=a.ad --force`), NOT a session-wide standing grant that
23+
* silently follows a later `--save-script=b.ad` and overwrites a file nobody
24+
* opted to overwrite. A live `force` on this same retarget re-grants it for the
25+
* new target (handled by the caller, AFTER this — so a live flag always wins).
26+
* Shared by both re-arming paths: `armReplaySaveScriptStep` (replay) and
27+
* `recordActionEntry` (close --save-script).
28+
*/
29+
export function applySaveScriptRetarget(
30+
session: SessionState,
31+
nextPath: string,
32+
liveForce: boolean | undefined,
33+
): void {
34+
const retargeted = session.saveScriptPath !== undefined && session.saveScriptPath !== nextPath;
35+
if (retargeted && !liveForce) session.saveScriptForce = undefined;
36+
session.saveScriptPath = nextPath;
37+
}
38+
1739
export function recordActionEntry(
1840
session: SessionState,
1941
entry: RecordActionEntry,
@@ -29,15 +51,20 @@ export function recordActionEntry(
2951
// guard is uniform (see `publishHealedScriptAtomically`) and refuses
3052
// ANY pre-existing target — an explicit, caller-DIRECTED path included.
3153
// Directing the path is not the same as authorizing an overwrite.
32-
session.saveScriptPath = expandSessionPath(entry.flags.saveScript);
54+
applySaveScriptRetarget(
55+
session,
56+
expandSessionPath(entry.flags.saveScript),
57+
entry.flags.force,
58+
);
3359
session.saveScriptDefaultedHealedPath = false;
3460
}
3561
// #1258: persist `--force`/`--overwrite`, like `saveScriptPath`, so a
3662
// LATER write that does not repeat the flag (a bare `close` finishing a
3763
// session opened with `open --save-script --force`, or an unattended
38-
// auto-commit teardown with no live request) still honors it. Sticky —
39-
// once true, a later action carrying `saveScript` without `force` must
40-
// not clear it back to false.
64+
// auto-commit teardown with no live request) still honors it. Sticky FOR
65+
// THE SAME TARGET — a retarget to a different path without a live `force`
66+
// drops it (see `applySaveScriptRetarget`); a same-path later action
67+
// carrying `saveScript` without `force` still must not clear it.
4168
if (entry.flags.force) {
4269
session.saveScriptForce = true;
4370
}

‎src/daemon/types.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -329,7 +329,13 @@ export type SessionState = {
329329
* `--from` continuation leg, or an unattended auto-commit teardown with no
330330
* live request at all) still honors the overwrite the caller opted into up
331331
* front. The effective decision at any write site is `req.flags?.force ||
332-
* session.saveScriptForce`; once set true it is never cleared.
332+
* session.saveScriptForce`.
333+
*
334+
* Force is PER-TARGET, not a session-wide standing grant: it stays set while
335+
* the target is unchanged, but re-arming a DIFFERENT `--save-script=<other>`
336+
* without a live `--force` CLEARS it (`applySaveScriptRetarget`), so a later
337+
* retarget can never silently overwrite a file the caller never opted into.
338+
* A live `--force` on the retarget re-grants it for the new target.
333339
*/
334340
saveScriptForce?: boolean;
335341
/**

0 commit comments

Comments
 (0)