Skip to content

Commit bfbbf36

Browse files
committed
docs(daemon): scope refuse-on-exist contract/comments to ordinary recording too
PR #1235 review blocker: SessionScriptWriter.write's refuse-on-exist publish is uniform across repair-armed heals AND ordinary (non-repair) open/close --save-script recording, but the ADR contract and several comments still read as if only healed repair publication is refused and ordinary recording keeps the old rename-replace overwrite. Maintainer decision is to keep the behavior uniform and fix the docs/comments/coverage instead of re-scoping. - session-script-writer.ts: clarify isRepairArmedWriteBlocked only gates whether a publish is attempted, not what refuse-on-exist does once it is; fix write()'s catch-block comment, which claimed no AppError was ever raised on the ordinary path (now false since refuse-on-exist is uniform); broaden publishHealedScriptAtomically's doc to state it is write()'s only publish primitive for every target, referencing the removed publishOverwriteAtomically and the future --force/--overwrite (#1258). - session-action-recorder.ts / session-replay-runtime.ts / types.ts: fix comments claiming an explicit --save-script=<path> (or the saveScriptDefaultedHealedPath marker) is exempt from the clobber guard; the guard is uniform regardless of path origin or repair-armed status. - docs/adr/0012-interactive-replay.md: add a "Scope" paragraph making the refusal explicitly uniform across repair and ordinary recording, and extend the decision-6 acceptance-test bullet and migration-plan step 9 bullet to require ordinary-recording no-clobber coverage too. - session-script-writer.test.ts: add the missing existing-target coverage for the ORDINARY (non-repair, no saveScriptBoundary) path — refused with bytes unchanged when the target exists (thrown, since ordinary writes rethrow AppErrors rather than returning them), and confirmed to still succeed against an absent target. No runtime behavior change: publish is still a single uniform exclusive linkSync for every --save-script target.
1 parent 8ecbb75 commit bfbbf36

6 files changed

Lines changed: 127 additions & 14 deletions

File tree

‎docs/adr/0012-interactive-replay.md‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -579,7 +579,9 @@ Implementation is not accepted on benchmark evidence alone. Required automated c
579579
created in the target's own directory and published via a single exclusive `linkSync`
580580
(create-if-absent, first writer wins); and a no-clobber test proving publication refuses ANY
581581
pre-existing target — complete or partial alike, byte-for-byte unchanged — for both the default healed
582-
sibling and an explicit `--save-script=<path>`.
582+
sibling and an explicit `--save-script=<path>`, **and for an ordinary (non-repair) `open`/`close
583+
--save-script` recording whose target already exists** — the writer entry point and publish primitive
584+
are shared, so the refusal is uniform, not repair-only (see "Scope" below).
583585

584586
Extend the settle benchmark (`~/.agent-device-bench/rnnav-matrix.py` pattern, external harness) with a
585587
replay arm only after these contracts pass: measure clean replay and one induced divergence repaired
@@ -809,6 +811,20 @@ a complete, review-worthy repair artifact for any other reader — but it no lon
809811
publish decision. Auto-versioned output names (e.g. `.healed.2.ad`) are explicitly **out of scope** here —
810812
a separate naming change, not part of this decision.
811813

814+
**Scope: this refusal is uniform across repair AND ordinary recording, not repair-only.** Everything
815+
above is written in the context of decision 6's repair-armed heal, but `publishHealedScriptAtomically`
816+
(`src/daemon/session-script-writer.ts`) is the single publish primitive `SessionScriptWriter.write` calls
817+
for every `--save-script` target, with no repair-armed-vs-ordinary branch — a repair-armed heal
818+
(`saveScriptBoundary` set) and an ordinary, non-repair `open --save-script`/`close --save-script`
819+
recording (`saveScriptBoundary` absent) publish through the exact same exclusive `linkSync`. Before this
820+
decision, ordinary recording published via a separate atomic rename-replace path
821+
(`publishOverwriteAtomically`, since removed), silently overwriting an existing target; that overwrite
822+
path is gone. An ordinary recording whose target already exists is now refused exactly like a healed
823+
repair publish would be: `EEXIST` surfaces as the same `AppError`, the existing file is left
824+
byte-for-byte unchanged, and the caller must remove it or choose a different `--save-script=<path>`.
825+
There is no `--force`/`--overwrite` escape hatch for either path today; that is tracked as a future
826+
follow-up (#1258), not part of this decision.
827+
812828
**Repair-session tombstone (R7 ownership).** When a bounded-expiry escape hatch idle-reaps (or a daemon
813829
shutdown tears down) a repair-armed transaction that has **not** reached `COMPLETE`, the teardown aborts
814830
(no publish, per the state machine) and must leave a **tombstone** rather than deleting the session record
@@ -929,7 +945,9 @@ each states its dependencies explicitly.
929945
`REPAIR_SESSION_EXPIRED` tombstone for an incomplete-transaction reap/shutdown (keyed by session key,
930946
owner + bounded expiry, cleared by a fresh `replay --save-script`); and race-safe atomic publication
931947
(temp file in the target's own directory, published via a single exclusive `linkSync` that refuses ANY
932-
pre-existing target — complete or partial, default or explicit path alike — never overwriting one).
948+
pre-existing target — complete or partial, default or explicit path alike, and uniformly for an
949+
ordinary non-repair recording's target too, since the publish primitive is shared — never overwriting
950+
one; see "Scope" under Decision 6 above).
933951

934952
## Migration progress
935953

‎src/daemon/__tests__/session-script-writer.test.ts‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,69 @@ test('write() now refuses an explicit --save-script=<path> pointing at an existi
410410
expect(fs.readFileSync(outPath, 'utf8')).toBe(before);
411411
});
412412

413+
// (e) the refuse-on-exist publish primitive is shared with ORDINARY
414+
// (non-repair) recording too — no `saveScriptBoundary` at all, i.e. a plain
415+
// `open --save-script`/`close --save-script` session, never `replay
416+
// --save-script`. Before the maintainer-approved uniform simplification,
417+
// this path published via a separate atomic rename-replace and silently
418+
// overwrote an existing target; that overwrite path is gone (see the ADR's
419+
// "Scope" note under decision 6). This is the coverage gap the reviewer
420+
// flagged on #1235: only repair-armed (`saveScriptBoundary` set, even to 0)
421+
// sessions were exercised above.
422+
test('an ordinary (non-repair) recording now refuses an existing target too (behavior change: no more rename-replace overwrite)', () => {
423+
const root = fs.mkdtempSync(
424+
path.join(os.tmpdir(), 'agent-device-script-writer-ordinary-clobber-'),
425+
);
426+
const writer = new SessionScriptWriter(path.join(root, 'sessions'));
427+
const outPath = path.join(root, 'flows', 'existing.ad');
428+
fs.mkdirSync(path.dirname(outPath), { recursive: true });
429+
fs.writeFileSync(outPath, 'context platform=ios device="x"\nclick id="old"\n');
430+
const before = fs.readFileSync(outPath, 'utf8');
431+
432+
const session = makeIosSession('default', {
433+
recordSession: true,
434+
// No saveScriptBoundary: an ordinary open/close --save-script recording,
435+
// never armed via `replay --save-script` — this is NOT a repair.
436+
saveScriptPath: outPath,
437+
actions: [action({ command: 'click', positionals: ['id="new"'] })],
438+
});
439+
440+
// Unlike a repair-armed write (which returns `{written:false, error}`), an
441+
// ordinary (non-repair-armed) write's catch block RETHROWS an AppError
442+
// (`write()`: `if (repairArmed) return {...}; if (error instanceof
443+
// AppError) throw error;`) — so refusal here surfaces as a thrown error,
444+
// not a returned result.
445+
expect(() => writer.write(session)).toThrow(/already exists/);
446+
// The pre-existing target is left byte-for-byte unchanged — refused, not
447+
// clobbered.
448+
expect(fs.readFileSync(outPath, 'utf8')).toBe(before);
449+
});
450+
451+
// (f) confirm the absent-target case still succeeds for ordinary recording —
452+
// the refusal is scoped to an EXISTING target, not a blanket block.
453+
test('an ordinary (non-repair) recording still publishes cleanly when its target does not exist yet', () => {
454+
const root = fs.mkdtempSync(
455+
path.join(os.tmpdir(), 'agent-device-script-writer-ordinary-absent-'),
456+
);
457+
const writer = new SessionScriptWriter(path.join(root, 'sessions'));
458+
const outPath = path.join(root, 'flows', 'fresh.ad');
459+
460+
const session = makeIosSession('default', {
461+
recordSession: true,
462+
// No saveScriptBoundary: ordinary recording, not a repair.
463+
saveScriptPath: outPath,
464+
actions: [action({ command: 'click', positionals: ['id="new"'] })],
465+
});
466+
467+
const result = writer.write(session);
468+
expect(result.written).toBe(true);
469+
expect(result.written && result.path).toBe(outPath);
470+
const parsed = parseReplayScriptDetailed(fs.readFileSync(outPath, 'utf8'));
471+
expect(parsed.actions.map((a) => a.positionals[0])).toEqual(['id="new"']);
472+
// Ordinary recording never carries the repair-only completeness sentinel.
473+
expect(fs.readFileSync(outPath, 'utf8')).not.toContain(HEAL_COMPLETE_SENTINEL);
474+
});
475+
413476
test('close --save-script=<explicit path> clears the defaulted marker, and a write to that (absent) explicit path succeeds', () => {
414477
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-close-explicit-'));
415478
const writer = new SessionScriptWriter(path.join(root, 'sessions'));

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -548,9 +548,13 @@ function armReplaySaveScriptStep(params: {
548548
if (!session) return;
549549
session.recordSession = true;
550550
if (typeof saveScript === 'string') {
551-
// An EXPLICIT `--save-script=<path>` clears the defaulted marker so the
552-
// clobber guard never refuses a path the caller directed (invariant: the
553-
// marker is set iff the current `saveScriptPath` was defaulted).
551+
// An EXPLICIT `--save-script=<path>` clears the defaulted marker
552+
// (invariant: the marker is set iff the current `saveScriptPath` was
553+
// defaulted, not caller-directed). This no longer affects the publish
554+
// decision either way — the writer's refuse-on-exist guard is uniform
555+
// (`publishHealedScriptAtomically`) and refuses ANY pre-existing target,
556+
// an explicit caller-directed path included, exactly like the default
557+
// healed sibling.
554558
session.saveScriptPath = expandSessionPath(saveScript);
555559
session.saveScriptDefaultedHealedPath = false;
556560
} else if (session.saveScriptPath === undefined) {

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,12 @@ export function recordActionEntry(
2323
session.recordSession = true;
2424
if (typeof entry.flags.saveScript === 'string') {
2525
// ADR 0012 decision 6: an explicit `--save-script=<path>` (e.g. `close
26-
// --save-script=<path>`) clears the defaulted-healed marker so the
27-
// writer's clobber guard never refuses an overwrite the caller directed.
26+
// --save-script=<path>`) clears the defaulted-healed marker, since this
27+
// path was no longer defaulted to the healed sibling. This marker plays
28+
// no role in the publish decision itself: the writer's refuse-on-exist
29+
// guard is uniform (see `publishHealedScriptAtomically`) and refuses
30+
// ANY pre-existing target — an explicit, caller-DIRECTED path included.
31+
// Directing the path is not the same as authorizing an overwrite.
2832
session.saveScriptPath = expandSessionPath(entry.flags.saveScript);
2933
session.saveScriptDefaultedHealedPath = false;
3034
}

‎src/daemon/session-script-writer.ts‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,12 @@ export const HEAL_COMPLETE_SENTINEL = '# agent-device:heal-complete';
5555
* after a divergence but before the plan finishes from committing a PREFIX;
5656
* every non-completion teardown (divergence-only exit, daemon shutdown,
5757
* idle-reap) lands here too.
58-
* Ordinary (non-repair) recording is never blocked (no `saveScriptBoundary`).
58+
* Ordinary (non-repair) recording is never blocked here (no `saveScriptBoundary`) —
59+
* this gate only decides whether `write()` attempts a publish AT ALL. It says
60+
* nothing about what happens once it does: `publishHealedScriptAtomically`'s
61+
* refuse-on-exist applies to that attempted publish uniformly, repair-armed or
62+
* not (see its doc comment) — ordinary recording is never blocked from trying,
63+
* but it can still be refused if the target already exists.
5964
*/
6065
function isRepairArmedWriteBlocked(session: SessionState): boolean {
6166
if (session.saveScriptBoundary === undefined) return false;
@@ -98,9 +103,16 @@ export class SessionScriptWriter {
98103
// SURFACED (no-clobber refusal, bare-`@ref`, or a filesystem error alike)
99104
// so close/teardown can report it and keep the session for retry — never
100105
// swallowed into a silent `{written:false}`. Ordinary (non-repair)
101-
// recording keeps its existing behavior: an unexpected AppError still
102-
// fails loud (none is raised on that path today), an fs error is a quiet
103-
// skip.
106+
// recording keeps its existing SHAPE of behavior: an AppError still
107+
// fails loud (thrown, not swallowed into `{written:false}`) and any other
108+
// fs error is a quiet skip — but an AppError is no longer only
109+
// theoretical here. Since `publishHealedScriptAtomically` refuses ANY
110+
// pre-existing target uniformly (maintainer-approved: refuse-on-exist
111+
// applies to ordinary recording too, not just repair heals), an ordinary
112+
// `open`/`close --save-script` write against an existing target now
113+
// throws that same no-clobber AppError, surfacing here as a genuine
114+
// "fails loud" case rather than the "none is raised on that path" it was
115+
// before that change.
104116
if (repairArmed) {
105117
return { written: false, error: toRepairCommitFailure(error, scriptPath) };
106118
}
@@ -149,6 +161,15 @@ function formatSessionScript(session: SessionState, appendCompleteSentinel: bool
149161
* target — complete or partial, the default healed sibling or an explicit
150162
* `--save-script=<path>` alike.
151163
*
164+
* This is `write()`'s ONLY publish primitive, called unconditionally for
165+
* every target — a repair-armed heal AND an ordinary, non-repair
166+
* `open`/`close --save-script` recording alike. There is no
167+
* repair-armed-vs-ordinary branch here (an earlier design had one —
168+
* `publishOverwriteAtomically`, rename-replace for ordinary writes — since
169+
* removed): an ordinary recording's target is refused exactly like a healed
170+
* repair's, never silently overwritten. `--force`/`--overwrite` is a future
171+
* escape hatch (#1258), not implemented today.
172+
*
152173
* The temp file is created in the SAME DIRECTORY as the target (never
153174
* `/tmp`), so the publish itself is a single intra-directory `linkSync`:
154175
* atomic create-exclusive, first writer wins. That single primitive is

‎src/daemon/types.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -282,9 +282,12 @@ export type SessionState = {
282282
/**
283283
* ADR 0012 decision 6: set when `saveScriptPath` was DEFAULTED to the
284284
* `<original-stem>.healed.ad` sibling (no explicit `--save-script=<out>`).
285-
* The writer refuses to clobber an existing default healed script, so a
286-
* second repair against the same original never destroys an unreviewed
287-
* prior `.healed.ad` diff.
285+
* Bookkeeping only — it does not gate the writer's refuse-on-exist
286+
* decision, which is uniform regardless of this flag (see
287+
* `publishHealedScriptAtomically`): a second repair against the same
288+
* original is refused at the default healed sibling exactly like an
289+
* explicit `--save-script=<path>` or an ordinary recording's target would
290+
* be, never destroying an unreviewed prior `.healed.ad` diff.
288291
*/
289292
saveScriptDefaultedHealedPath?: boolean;
290293
/**

0 commit comments

Comments
 (0)